Re: [PATCH bpf-next v3 10/12] selftests/bpf: Cover the low-32 link for narrowing stack fills

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

On 10/20/26 9:52 AM, bot+bpf-ci@kernel.org wrote:
>> selftests/bpf: Cover the low-32 link for narrowing stack fills
> TL;DR: Test issue: sext_fill_does_not_inherit_zext_link appears never to put a
> zero-extending link on the stack slot, so it can't distinguish the new
> check_mem_access() condition from the old one; its comment and commit message
> look inaccurate.
>
>> diff --git a/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c b/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
>> index 69efe1381eca..ab78285153fb 100644
>> --- a/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
>> +++ b/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
>> +/*
>> + * A sign-extending fill must not come away with a zero-extending link. The
>> + * slot below already fits 32 bits, so the fill is not narrowing and assigns
>> + * no link kind of its own, and "state->regs[dst] = *reg" copies the slot's
>> + * verbatim; check_mem_access() is what has to drop it, since it would leave
>> + * r2 recording a zero-extension of a value it has just sign-extended.
>> + * Asserted on the register dump rather than on a guarded branch, so it pins
>> + * the rule itself and not a downstream effect.
>> + */
>> +SEC("socket")
>> +__success __log_level(2)
>> +/* a copied link would print as R2=scalar(id=N+0,smin=... */
>> +__msg("(81) r2 = *(s32 *)(r10 {{.*}} R2=scalar(smin=")
>> +__naked void sext_fill_does_not_inherit_zext_link(void)
>> +{
>> +	asm volatile ("						\
>> +	call %[bpf_get_prandom_u32];				\
>> +	r6 = r0;						\
>> +	call %[bpf_get_prandom_u32];				\
>> +	r0 <<= 32;						\
>> +	r6 |= r0;		/* r6 = full 64-bit unknown */	\
>> +	w7 = w6;		/* r7 = zext32(r6), fits 32 bits */ \
>> +	*(u64 *)(r10 - 8) = r7;					\
> Does this test ever put a zero-extending link on the slot?

Sorry, it doesn't and nothing in this patch can, so it is dropped in v4.


> The w7 = w6 above does give r7 BPF_REG_LINK_ZEXT, but the spill that follows
> clears it.  In check_stack_write_fixed_off(), r7 fits in 32 bits, so
> assign_scalar_id_before_mov() runs before save_register_state():
>
>      if (src_reg->link_kind && src_reg->link_kind != BPF_REG_LINK_SEXT)
>          bpf_clear_scalar_id(src_reg);
>      ...
>      if (!src_reg->id && !tnum_is_const(src_reg->var_off))
>          src_reg->id = ++env->id_gen;
>
> The slot ends up with a fresh id and link_kind == BPF_REG_LINK_NONE.  The
> 4-byte ldsx is not narrowing, so the fill copies a plain id, and
> check_mem_access() drops it through the existing !no_sext clear.  The
> link_kind == BPF_REG_LINK_SEXT part of the new condition never sees a ZEXT
> kind here.
>
> This leaves two problems.
>
> First, the in-test comment looks wrong:
>
>      /* a copied link would print as R2=scalar(id=N+0,smin=... */
>
> print_reg_state() prints +delta only when link_kind is set, so a surviving
> plain id would print as R2=scalar(id=N,smin=...
>
> Second, the commit message says "The last two pin the link-kind checks in
> check_mem_access()", but this test cannot tell the new check from the old
> one.  It passes with the pre-series:

Agreed. Pre-series the fill gets a plain id from 
assign_scalar_id_before_mov() and check_mem_access() drops it through 
the existing !no_sext clear, so R2 prints without an id either way and 
the assertion cannot separate them.
The changelog said "the last two"; it is now "the last one", counting 
only sext_byte_fill_drops_sext_link.


> That holds even after 3649346488fc ("bpf: Track low-32 scalar equality across
> narrowing stack spills"), because the fill-side
> assign_scalar_id_before_mov(reg) clears the slot's new ZEXT kind before the
> copy.  No later commit in the range (3649346488fc, 73fbac05c8bd) changes this
> test.

Right.

> Should the comment, the test name and the commit message describe what is
> actually pinned here (a plain id dropped across a range-changing 4-byte
> ldsx)?  Or should the test be reworked once a ZEXT-carrying slot is
> reachable?

Neither: the test is dropped in v4.

Re-describing seems viable but is conceptually wrong. It pins a plain id 
dropped across a range-changing ldsx which is pre-existing behavior that 
this series does not touch, so doesn't make sense to introduce here. 
Reworking after "a ZEXT-carrying slot is reachable" might be better 
suited, but that is 11/12 and 12/12, which are tagged [do not merge]. 
12/12's version has the same defect and is dropped too, so both go.

Thx,
-Vineet

> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39458500836