Re: [PATCH bpf-next v2 07/13] bpf: track low-32 scalar equality across sign-extending movs

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

On 9/10/26 11:22 PM, bot+bpf-ci@kernel.org wrote:
>> @@ -16248,12 +16258,23 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
>>   						return -EACCES;
>>   					} else if (src_reg->type == SCALAR_VALUE) {
>>   						bool no_sext;
>> +						/*
>> +						 * A 32-bit sign extension keeps the low 32
>> +						 * bits, so record a low-32 link as the
>> +						 * zero-extending mov does. A self-mov
>> +						 * qualifies only if src is already linked.
>> +						 */
>> +						bool subreg_link = (insn->off >> 3) == 4 &&
>> +								   (src_reg != dst_reg ||
>> +								    src_reg->id);
>>
>>   						no_sext = reg_umax(src_reg) < (1ULL << (insn->off - 1));
>> -						if (no_sext)
>> +						if (no_sext || subreg_link)
>>   							assign_scalar_id_before_mov(env, src_reg);
>                                                          ^^^^
>
> Can widening this guard drop an existing ADD_CONST link on the source
> register?  With 'no_sext || subreg_link', assign_scalar_id_before_mov() now
> also runs on src_reg when !no_sext, and that helper discards the source's
> delta tracking:
>
> kernel/bpf/verifier.c:assign_scalar_id_before_mov() {
> 	...
> 	if (src_reg->add_const)
> 		clear_scalar_id(src_reg);
> 	if (!src_reg->id && !tnum_is_const(src_reg->var_off))
> 		src_reg->id = ++env->id_gen;
> 	...
> }
>
> clear_scalar_id() zeroes id, delta, add_const and subreg, and the following
> statement then mints a brand-new id, so src_reg is unlinked from the
> register set it was tracking.  Before this commit the !no_sext arm left
> src_reg untouched.
>
> For a BPF_ALU64 | BPF_MOV | BPF_X with insn->off == 32:
>
> 	r1 = r2			/* r1, r2 share id N */
> 	r1 += 8			/* r1->add_const = ADD_CONST_64, r1->delta = 8 */
> 	r3 = (s32)r1		/* r1 is wide -> !no_sext, subreg_link true */
> 	if r2 < 100 goto out	/* sync_linked_regs() no longer reaches r1 */
> 	... use r1 ...
>
> At the mov, r1 loses its ADD_CONST_64 / delta = 8 link and gets a fresh id,
> so the following 'if r2 < 100' compare no longer refines r1 to [8, 107].
> Is that the intent?  This is the compiler idiom described in the comment
> above adjust_reg_min_max_vals()'s delta handling.
>
> The clear also does not look necessary for the new link: since !no_sext, the
> SUBREG_SEXT link is established through the freshly minted id.
>
> Would excluding add_const sources be enough, something like:
>
> 	bool subreg_link = (insn->off >> 3) == 4 && !src_reg->add_const &&
> 			   (src_reg != dst_reg || src_reg->id);

Indeed it needs to fixed with the additional !add_const check.

> The zero-extending arm added earlier in the series uses the same guard
> shape, so a fix likely wants to cover both.  Looking forward through
> 2c5433cf9fbb..1d3c4a0831be, the later commits extend the same mechanism to
> narrowing stack fills and spills and reuse the identical guard, so none of
> them change this.
>
>>   						*dst_reg = *src_reg;
>> -						if (!no_sext)
>> +						if (!no_sext && subreg_link && src_reg->id)
>> +							dst_reg->subreg = SUBREG_SEXT;
>                                                          ^^^^
>
> On a related note, for 'r0 = (s32)r0' where r0 carries add_const,
> subreg_link is evaluated before the clear and sees the old src_reg->id as
> non-zero, so it is true.  assign_scalar_id_before_mov() then clears and
> re-mints the id, and SUBREG_SEXT is applied to a register that is the sole
> holder of that id.
>
> The changelog says:
>
>> Unlike the zero-extending arm, a self-mov can form a link here, but only
>> when src is already linked: r0 = (s32)r0 is how a sign-extended int return
>> lands. On an unlinked register there is nothing to link to, and minting an
>> id would leave the register describing itself.
> bpf_clear_singular_ids() drops singleton ids before state comparison, so
> this does not look unsound, but does it match the rationale above?

yes the changelog needs to be fixed.

Thx,
-Vineet

>> +						else if (!no_sext)
>>   							clear_scalar_id(dst_reg);
>>   						coerce_reg_to_size_sx(dst_reg, insn->off >> 3);
>>   					} else {
> [ ... ]
>
>
> ---
> 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/34506184282