Re: [PATCH bpf-next v3 04/13] bpf: Track R2 of register-pair returns in precision backtracking

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Multi-line comments do not follow the required BPF subsystem comment style.
--

--- Patch [4]: [PATCH bpf-next v3 04/13] bpf: Track R2 of register-pair returns in precision backtracking ---
commit 031a41deb76f72f449ddf5239bbf769a2cbe7e5c
Author: Yonghong Song <[email protected]>

bpf: Track R2 of register-pair returns in precision backtracking

This commit updates the BPF verifier's precision backtracking to correctly
handle R2 for functions returning values larger than 8 bytes. It ensures
R2 is cleared from or added to the register mask across function call
boundaries to prevent unexpected register mask verification failures.

> diff --git a/kernel/bpf/backtrack.c b/kernel/bpf/backtrack.c
> index 40bd04421a991..9b112bf427a57 100644
> --- a/kernel/bpf/backtrack.c
> +++ b/kernel/bpf/backtrack.c
[ ... ]
> @@ -425,6 +425,14 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
>  				 */
>  				verifier_bug_if(idx + 1 != subseq_idx, env,
>  						"extra insn from subprog");
> +				/* a global subprog returning more than 8 bytes
> +				 * sets R2 as well. R2 is part of the args mask
> +				 * checked just below, so it has to be cleared
> +				 * here rather than next to R0.
> +				 */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the preferred
BPF subsystem style? The guidelines specify that the opening /* should be
on its own line rather than sharing a line with the text.

> +				if (bt_is_reg_set(bt, BPF_REG_2) &&
> +				    bpf_ret_reg_pair(env, subprog))
> +					bt_clear_reg(bt, BPF_REG_2);
[ ... ]
> @@ -522,7 +543,29 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
>  					return -EFAULT;
>  			}
>  		} else if (opcode == BPF_EXIT) {
> -			bool r0_precise;
> +			bool from_subprog_call, r0_precise, r2_precise = false;
> +
> +			/* BPF_EXIT in subprog or callback always returns
> +			 * right after the call instruction, so by checking
> +			 * whether the instruction at subseq_idx-1 is subprog
> +			 * call or not we can distinguish actual exit from
> +			 * *subprog* from exit from *callback*. In the former

[Severity: Low]
This is also not a bug, but since this comment is being heavily modified,
could it be updated to place the opening /* on a separate line to match
the modern BPF comment style?

> +			 * case, we need to propagate the precision of the
> +			 * return registers, if necessary. In the latter we
> +			 * never do that.
> +			 */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4