Lab 9: Review a Patch (Milestone 10)

Background

This is the lab people skip, and it is the one that changes how you are seen.

Review is the scarcest resource in the kernel. Maintainers are not short of patches; they are short of people who will read someone else's patch carefully and say something useful. A contributor who reviews in a subsystem becomes a person whose own patches get read first — not as a favour, but because they have demonstrated they understand the area well enough to be worth engaging with.

It is also, bluntly, the fastest way to learn a subsystem. Reading a patch with the intent to find something wrong forces a depth of attention that reading code for understanding does not.

Why This Lab Matters

• Reciprocity is the economy. You are asking strangers to spend attention on your patches.
• Reviewing teaches you what maintainers look for, which makes your own patches better.
• A Tested-by: from someone with hardware the author does not have is genuinely valuable and requires no expertise you do not already have.
• It is the best possible use of the two weeks you spend waiting for Lab 7 to get a reply.

Prerequisites

• Review and Etiquette read.
• b4 installed.
• A working lab rig — you will apply, build, and boot someone else's patch.
• Enough familiarity with one subsystem to have opinions. Reviewing code you do not understand produces noise.

Predict First

1. What fraction of patches on a busy subsystem list get any review at all before being applied?
2. You are a newcomer. Which of Reviewed-by:, Acked-by:, Tested-by: are you entitled to give?
3. What is the most common useless review comment from a newcomer?
4. If you apply a patch, build it, and it works — is that worth posting? Why or why not?
5. How long will a careful review of a 100-line patch take you the first time?

Step-by-Step Tasks

Step 1: Find a patch you can actually review

The constraint that makes this work: an area you understand. Ideally the subsystem you have been reading, or one adjacent to your own patch.

# Browse the list for your subsystem:
#   https://lore.kernel.org/<list-name>/

# Or pull recent unanswered postings locally with lei:
lei q -o ~/mail/review -I https://lore.kernel.org/all/ --threads \
    '(l:linux-<subsys>.vger.kernel.org AND s:"[PATCH" AND rt:1.week.ago..)'

What makes a good target:

# Check nobody has already covered it, and it has not landed:
b4 mbox <message-id> -o /tmp/r && grep -c '^Reviewed-by:' /tmp/r/*.mbx
git log --oneline --all --grep="<the patch subject>" | head

Step 2: Apply it, build it, run it

This is the step that makes your review worth reading. A review from someone who compiled the patch carries different weight from one that did not, and you must say which you did.

b4 shazam <message-id>              # fetch and apply the latest version
git log --oneline -5

make O=../build -j"$(nproc)"
make O=../build C=2 W=1 <the/dirs/it/touched>/
./scripts/checkpatch.pl --strict -g HEAD~<n>..HEAD

# If it is a series, does it bisect?
git rebase --exec "make -s -j$(nproc) O=../build" <base>

# Boot it, ideally under the paranoid kernel:
PROFILE=paranoid ~/kernel-labs/scripts/build-kernel.sh
~/kernel-labs/scripts/run-qemu.sh

Step 3: Read it properly

Read it three times, for different things.

PASS 1 — WHAT IS IT TRYING TO DO?
  Read the commit message ALONE, without the diff. Can you state the problem
  and the approach? If not, that is your first comment, and it is a real one:
  a message that does not explain itself is a defect.

PASS 2 — IS IT CORRECT?
  □ Context: what context does each new function run in? May it sleep there?
  □ Locking: what protects each new shared field? Is the lock order consistent
    with the rest of the file?
  □ Error paths: does every failure unwind everything acquired, in reverse?
  □ Allocation: right allocator, right GFP flags for the context?
  □ User input: every copy_*_user return checked? Bounds checked? Unknown
    flag bits rejected? Structs memset before copy_to_user?
  □ Teardown: is anything left running that references freed memory?
  □ Integer safety: overflow in a size computation? Signedness?
  □ ABI: does this change anything user space can observe? Is it extensible?

PASS 3 — IS IT THE RIGHT CHANGE?
  □ Does it belong in this subsystem?
  □ Is there an existing helper that already does this?
  □ Will it need to change again for the obvious next case?
  □ Is it tested? Would the test catch a regression?

Pass 2 is a checklist because that is genuinely how experienced reviewers read: they are pattern matching against the failure modes from Foundations, not reading line by line for beauty.

Step 4: Write the review

On Tue, Jun 17, 2026 at 09:31:02AM +0200, Author Name wrote:
> +	ml->buf = kmalloc(len, GFP_KERNEL);
> +	if (!ml->buf)
> +		return -ENOMEM;
> +
> +	spin_lock(&ml->lock);
> +	list_add(&ml->node, &ml->items);

This looks right, but a few lines further down:

> +	spin_lock(&ml->lock);
> +	tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);

This one allocates with GFP_KERNEL while holding ml->lock, which can sleep.
CONFIG_DEBUG_ATOMIC_SLEEP should catch it -- did this get tested with that
enabled? Moving the allocation above the spin_lock and re-validating after
taking it would be the usual fix.

> +	if (copy_from_user(&karg, uarg, sizeof(karg)))
> +		return -EFAULT;
> +	if (karg.flags & LAB_F_KNOWN)

Should this reject unknown bits rather than testing for known ones? As
written, a program that sets an undefined bit today silently succeeds, and
you can never give that bit a meaning later.

Otherwise this looks good to me. I applied it on top of net-next/main
(abc1234), built for x86_64 and arm64, and ran the selftest -- all passing
with KASAN and lockdep enabled.

What that review does right, and what to copy:

Step 5: Choose the right trailer

Only give what you have earned. These are attestations with your name on them, permanently.

Format them exactly, on their own line at the end:

Reviewed-by: Your Real Name <you@example.com>
Tested-by: Your Real Name <you@example.com>

Note: As a newcomer, Tested-by: is the trailer you can most confidently give and the one most likely to be genuinely useful — especially if you have hardware, an architecture, or a config the author does not. It requires no expertise you do not have, and it is scarce.

Step 6: Send it

# Reply to the SPECIFIC patch you are commenting on, not the cover letter,
# unless the comment is about the series as a whole.
# Reply-all: keep the list and everyone on the original Cc.

Use a mail client that quotes properly and does not wrap — the same constraints as sending a patch, minus the diff-fidelity requirement. mutt, aerc, neomutt, or b4's output piped into your editor all work.

Step 7: Follow through

# Did the author reply? Did v2 address your point?
b4 diff <message-id>                     # what changed between versions
b4 shazam <v2-message-id>                # apply v2 and re-check

If v2 fixes what you raised, say so briefly and give the trailer you withheld. If v2 ignores it, ask once, politely, whether it was intentional. If you were wrong, say so — that costs nothing and builds more credibility than being right did.

Implementation Requirements / Deliverables

• A patch reviewed in an area you actually understand.
• Applied and built, and your review says so explicitly, with the base commit.
• Booted and exercised if it is a behavioral change.
• All three reading passes done; the pass-2 checklist worked through.
• The review posted on-list, inline, trimmed, bottom-posted, reply-all.
• At least one comment about correctness — locking, context, error paths, or the ABI — not style.
• No comment that repeats what checkpatch already says.
• The right trailer, or none, and you can defend which.
• Followed through to the next version.
• Your five predictions recorded with results.

Expected Output

Your review, on lore, in the thread:

Re: [PATCH 2/4] subsys: add the capability
Your Real Name  Tue, 17 Jun 2026 14:22:08 +0000

And, if it went well, one of:

> This one allocates with GFP_KERNEL while holding ml->lock

Good catch, thanks -- I had not tested with DEBUG_ATOMIC_SLEEP. Fixed in v2.

Applied to subsys/for-next, thanks.

Reviewed-by: Your Real Name <you@example.com>

That second one — your name in git log on someone else's commit — is a real artifact of this milestone, and it arrives faster than your own patch will.

Debugging Steps

b4 shazam fails to apply

The series is based on a tree you do not have, or on a newer tip. Read the base-commit: line in the patch, fetch that tree, and retry:

grep '^base-commit:' /tmp/r/*.mbx
git fetch <the right remote> && git switch -c review <that base>
b4 shazam <message-id>

If there is no base-commit:, say so in your review — it is a legitimate, mild comment.

You cannot tell whether it is correct

Then say that, specifically: "I do not know this path well enough to judge whether X is safe — can you say what guarantees Y?" A precise question is a genuinely useful review. A vague endorsement is not.

Your review got no response

Normal, especially if the author is preparing v2. Check whether v2 addressed it before following up.

You gave Reviewed-by: and then found a bug

Say so immediately in the thread. Everyone would far rather have a late correction than a wrong attestation standing.

You disagree with the maintainer's review of the same patch

Fine, and say so with evidence. Two reviewers disagreeing in public is how the tree gets better; it is not a conflict.

Experiment

CLAIM. Review quality is a learnable, observable skill, and you can calibrate against the people who are good at it before you post anything.

METHOD.

1. Pick a maintainer who reviews a lot in your subsystem.
2. Read their last twenty reviews on lore.
3. Tabulate:

PREDICTION. Fill in what you expect first — especially the style row. Most people badly overestimate how much experienced reviewers comment on style, and badly underestimate how often they ask questions rather than assert.

RESULT. Then write your own review, and score it against the same table. The gap is your calibration.

Test

Review has no automated test, so make it checkable another way: predict, then compare.

1. Before posting, write down the three things you expect the author will
   push back on.
2. Post the review.
3. When the author replies, check your three predictions.
4. For each one you got wrong, write one sentence naming the false belief.

Then the harder version:

5. Before reading anyone else's review of the same patch, write down what
   you think the maintainer will comment on.
6. Read their review.
7. What did they catch that you missed? THAT is your reading list.

Do this for three patches. Your hit rate on step 7 is the actual measure of this milestone, and it improves fast.

Challenge Extensions

1. Review a series, not a patch. A 5–8 patch series exercises the ordering and bisectability review that single patches do not. Comment on the split as well as the contents.

2. Be the person with the unusual hardware. Find a driver patch for hardware or an architecture you have and the author does not, test it, and post Tested-by: with the details. This is disproportionately valued and almost nobody does it.

3. Review a patch you think is wrong. Then defend the position through a reply or two. Learning to disagree without escalating is a distinct skill from finding the bug.

4. Track a patch you reviewed all the way to mainline. Note what the final applied version looks like versus v1, and how much of the difference came from review.

5. Review with the bots. Run allmodconfig, a clang build, and coccicheck on the patch and report anything the author's testing missed. You are doing a slice of the 0-day bot's job, faster than it will.

6. Sustain it. Review one patch a week for a month in one subsystem. That is the point at which the maintainer starts recognizing your name — which is the actual goal of this milestone, and it cannot be achieved in one sitting.

Validation / Self-check

1. Why is review the scarcest resource in the kernel, and what does giving it buy you?
2. What makes a patch a good review target for you specifically? Give three criteria.
3. Why must you say whether you applied and built it?
4. Give the three reading passes and what each is for.
5. List six things on the pass-2 correctness checklist, and name the Foundations chapter each comes from.
6. Why quote specific lines instead of referring to line numbers?
7. Distinguish Reviewed-by:, Acked-by:, and Tested-by:. Which are you entitled to give today?
8. What should you do when you skimmed a patch and it looked fine?
9. What is the most useful review a newcomer can give, and why is it scarce?
10. You gave Reviewed-by: and later found a bug in that patch. What do you do?
11. From the calibration experiment: what fraction of experienced reviewers' comments were about style, and how did that compare with your prediction?
12. What did the maintainer catch on your test patches that you missed? What does that tell you to go read?

Contribution Complete

You can now find the right people, base your work correctly, split it into a bisectable series, send it without corrupting it, respond to review, and give review that someone acts on.

That is the entire mechanical barrier, and it is behind you. What remains is the part that actually takes years: knowing enough about a subsystem to have a change worth making.

Next: Subsystems — read all eight, go deep in one. And keep a patch in flight the whole time; the clock you started in Lab 7 runs in parallel with everything you are about to read.