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

[Vineet Gupta <vineet.gupta@linux.dev>]

On 8/15/26 4:57 AM, sashiko-bot@kernel.org wrote:
> 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
> --

...

> +/*
> + * 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?

Indeed this is not tight enough. Fixed and add the side-band comment for 
that.

>> 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.

This hunk is dropped in v2.

Thx,
-Vineet