Re: [RFC bpf-next 4/6] selftests/bpf: cover low-32 subreg-equal link for zero-extending movs

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Logic error in regression test rendering it ineffective
- [Low] Multi-line comment style violation in BPF subsystem
--

--- Patch [4]: [RFC bpf-next 4/6] selftests/bpf: cover low-32 subreg-equal link for zero-extending movs ---
commit c52a4172a626e9c98f4a644af71ceb18fe331f7d
Author: Vineet Gupta <[email protected]>

selftests/bpf: cover low-32 subreg-equal link for zero-extending movs

This commit adds a new test subreg_eq_zext_mov_narrow and updates existing
tests to verify the verifier's low-32 BPF_FLAG_SUBREG_ZEXT link behavior
for zero-extending moves. It also introduces tests to cover the destination
driven direction and the preservation of wide ADD_CONST sources.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c b/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
> index c80747c16bcf7..2cc6f9e45aff3 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c

[ ... ]

> @@ -711,4 +711,119 @@ l_exit_%=:							\
>  	: __clobber_all);
>  }

[ ... ]

> +/*
> + * Dest-driven direction, zero-extend flavour: narrowing the LINKED register
> + * must not narrow the wide base.
> + *
> + * w7 = w6 shares only r6's low 32 bits; r7's high half is zero, r6's is
> + * unknown. Learning r7 == 0 therefore says nothing about r6, and
> + * sync_linked_regs() must not copy r7's state onto it. Rejected iff the base
> + * is left alone.
> + *
> + * This is the shape that catches a lost BPF_FLAG_SUBREG_ZEXT: if the flag is
> + * dropped while the shared ->id survives, the pair looks like a full 64-bit
> + * equality, the dest-driven guard is bypassed and r6 wrongly becomes 0.
> + */
> +SEC("socket")
> +__failure __msg("div by zero")
> +__flag(BPF_F_TEST_STATE_FREQ)
> +__naked void zext_dest_driven_does_not_narrow_base(void)
> +{
> +	asm volatile ("						\
> +	call %[bpf_get_prandom_u32];				\
> +	r6 = r0;		/* r6 low = unknown u32 */	\
> +	call %[bpf_get_prandom_u32];				\
> +	r0 <<= 32;						\
> +	r6 |= r0;		/* r6 = full 64-bit unknown (base) */ \
> +	w7 = w6;		/* low-32 ZEXT link */		\
> +	if r7 != 0 goto l_out_%=;/* r7 == 0: low 32 bits are 0 */ \
> +	if r6 != 0 goto l_out_%=;/* r6 may still have high bits set */ \

[Severity: Medium]
Does this logic correctly catch a regression when a buggy verifier is tested?

If a buggy verifier incorrectly narrows r6 to exactly 0, wouldn't it evaluate
"r6 != 0" as statically false, and then fall through to the division by zero,
just like a correct verifier that sees r6 with unknown high bits?

Since both correct and buggy verifiers evaluate both branches and fall through
to the division, wouldn't this test always pass?

Would changing the branch to "if r6 == 0 goto l_out_%=;" correctly allow a
buggy verifier to skip the division and incorrectly accept the program,
failing the test?

> +	r0 /= 0;		/* must stay reachable */	\
> +l_out_%=:							\
> +	r0 = 0;							\
> +	exit;							\
> +"	:
> +	: __imm(bpf_get_prandom_u32)
> +	: __clobber_all);
> +}
> +
>  char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/progs/verifier_reg_equal.c b/tools/testing/selftests/bpf/progs/verifier_reg_equal.c
> index dc1d8c30fb0ee..e6fbbfaeedcb7 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_reg_equal.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_reg_equal.c
> @@ -31,23 +31,25 @@ l1_%=:	exit;						\
>  }
>  
>  SEC("socket")
> -__description("check w reg not equal if r reg upper32 bits not 0")
> -__failure __msg("R1 !read_ok")
> +__description("w reg shares r reg low32 via subreg link even if upper32 not 0")
> +__success
>  __naked void subreg_equality_2(void)
>  {
>  	asm volatile ("					\
>  	call %[bpf_ktime_get_ns];			\
>  	r2 = r0;					\
> -	/* Upper 4-bytes of r2 may not be 0, thus insn	\
> -	 * w3 = w2 should not propagate reg id,	and	\
> -	 * w2 < 9 comparison should not propagate	\
> -	 * the range for r3 either.			\
> +	/* Upper 4-bytes of r2 may not be 0. w3 = w2 is a 32-bit	\

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the BPF subsystem
style guide?

The modern kernel multi-line comment style used in the BPF subsystem requires
the opening "/*" to be on its own line.

> +	 * zero-extending mov, so w3 shares only r2 low 32 bits	\
> +	 * (a BPF_FLAG_SUBREG_ZEXT link) and its high bits are zero. The	\
> +	 * w2 < 9 comparison then narrows r3 to [0, 8] via the link,	\
> +	 * so if r3 < 9 is always taken and the illegal r1 read below	\
> +	 * is unreachable.				\
>  	 */						\
>  	w3 = w2;					\
>  	if w2 < 9 goto l0_%=;				\
>  	exit;						\

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