Re: [RFC bpf-next 5/6] bpf: support low-32 subreg scalar linking for sign-extending movs

[Vineet Gupta]

On 8/19/26 11:48 AM, Eduard Zingerman wrote:
> > @@ -570,9 +572,18 @@ static bool regsafe(struct bpf_verifier_env *env, struct bpf_reg_state *rold,
> >   		 * on a path that predates this series, which is a pruning change
> >   		 * that wants measuring on its own; it is deliberately left
> >   		 * alone here.
> > +		 *
> > +		 * Only demand a match when the old state carries a link at all.
> > +		 * These flags are only ever set together with an ->id, so
> > +		 * rold->id == 0 implies none is set, and the only case this
> > +		 * admits is "old knows no low-32 relationship, cur does" -- cur
> > +		 * is then strictly more constrained than old, which is the safe
> > +		 * direction for pruning. The reverse is still rejected. Without
> > +		 * this a register that first acquires a link inside a loop would
> > +		 * never match its pre-loop state and pruning would not converge.
> >   		 */
> >   		if (rold->id &&
> > -		    (rold->flags & BPF_FLAG_SUBREG_ZEXT) != (rcur->flags & BPF_FLAG_SUBREG_ZEXT))
> > +		    (rold->flags & BPF_FLAG_SUBREG) != (rcur->flags & BPF_FLAG_SUBREG))
> >   			return false;
> Same comment as for ZEXT patch.

The prior zext patch compares ->subreg with enum distinguishing zext and 
sext: no sext specific code needed.

> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 8a802d49d0a4..45cb67dc3999 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -14976,6 +14976,8 @@ static int adjust_reg_min_max_vals(struct bpf_verifier_env *env,
> >   	return 0;
> >   }
> >   
> > +static void reconstruct_sext32(struct bpf_reg_state *reg, struct bpf_reg_state *src);
> Nit: is it possible to avoid forward declaration?

Removed.

> >   /* check validity of 32-bit and 64-bit arithmetic operations */
> >   static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
> >   {
> > @@ -15052,15 +15054,65 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
> >   							insn->src_reg);
> >   						return -EACCES;
> >   					} else if (src_reg->type == SCALAR_VALUE) {
> > +						int sz = insn->off >> 3;
> >   						bool no_sext;
> > +						bool subreg_link;
> >   
> >   						no_sext = reg_umax(src_reg) < (1ULL << (insn->off - 1));
> > -						if (no_sext)
> > +						/*
> > +						 * When no_sext, dst == src exactly, so link them
> > +						 * (existing behavior). When !no_sext for a 32-bit sign
> > +						 * extension the low 32 bits are still identical (sext
> > +						 * preserves them), so form a BPF_FLAG_SUBREG_SEXT
> > +						 * link: a later narrowing of the low 32 bits
> > +						 * propagates here, and sync_linked_regs() rebuilds
> > +						 * the high half via reconstruct_sext32().
> > +						 *
> > +						 * An ADD_CONST-linked src is excluded for the same
> > +						 * reason as in the zero-extending arm below:
> > +						 * assign_scalar_id_before_mov() would clear its
> > +						 * base+delta link, and a combined subreg+delta link
> > +						 * isn't modeled anyway. Unlike that arm a self-mov is
> > +						 * NOT excluded -- r0 = (s32)r0 is the case this is
> > +						 * here for.
> > +						 */
> > +						subreg_link = (sz == 4) &&
> > +							      !(src_reg->flags & BPF_FLAG_ADD_CONST);
> > +
> > +						if (no_sext || subreg_link)
> >   							assign_scalar_id_before_mov(env, src_reg);
> >   						*dst_reg = *src_reg;
> > -						if (!no_sext)
> > -							clear_scalar_id(dst_reg);
> > -						coerce_reg_to_size_sx(dst_reg, insn->off >> 3);
> > +						if (!no_sext) {
> > +							if (subreg_link && src_reg->id) {
> > +								/* ->id already copied above */
> > +								dst_reg->flags = (dst_reg->flags & ~BPF_FLAG_SUBREG) |
> > +										 BPF_FLAG_SUBREG_SEXT;
> > +							} else {
> > +								clear_scalar_id(dst_reg);
> > +							}
> > +						}
> > +						/*
> > +						 * coerce_reg_to_size_sx() falls back to the full sext
> > +						 * range when smin/smax straddle the sign boundary (e.g.
> > +						 * an errno-or-zero value clamped to [-4095, 0]). For a
> > +						 * register tracked as the sign-extension of its low 32
> > +						 * bits the high half IS that sign-extension, so rebuild
> > +						 * the tighter 64-bit range from the low bounds, taken
> > +						 * from a snapshot because coerce overwrites them.
> > +						 *
> > +						 * Gated on sz == 4, not on the flag alone: an (s8)/(s16)
> > +						 * mov whose src is already SEXT-linked copies the flag
> > +						 * across in the *dst_reg = *src_reg above, and a 32-bit
> > +						 * reconstruction must not run for a narrower operation.
> > +						 */
> > +						if (sz == 4 && (dst_reg->flags & BPF_FLAG_SUBREG_SEXT)) {
> > +							struct bpf_reg_state sext_src = *dst_reg;
> > +
> > +							coerce_reg_to_size_sx(dst_reg, sz);
> > +							reconstruct_sext32(dst_reg, &sext_src);
> It does not make sense to maintain two functions that do register sign
> extension. If coerce_reg_to_size_sx() is not precise enough for the
> 32-bit case, then it should be adapted instead of special-cased.

Adapting coerce_reg_to_size_sx () update is now a standalone patch, with 
it's own test.

+       if (size == 1) {
+               field_smin = S8_MIN;
+               field_smax = S8_MAX;
+       } else if (size == 2) {
+               field_smin = S16_MIN;
+               field_smax = S16_MAX;
+       } else {
+               /* size == 4 */
+               field_smin = S32_MIN;
+               field_smax = S32_MAX;
+       }
+
+       /*
+        * The range already fits the field, so (sN)v == v for every 
value the
+        * register can hold and the sign extension changes nothing. The 
tests
+        * below cannot reach this case once smin is negative: a 
negative smin
+        * and a non-negative smax never share their high bits.
+        */
+       if (reg_smin(reg) >= field_smin && reg_smax(reg) <= field_smax)
+               return;


It existing usage is retained in check_alu_op, w/o any reconstruct business.

However in sync routine reconstruct_sext32 is needed, see below....

> > +						} else {
> > +							coerce_reg_to_size_sx(dst_reg, sz);
> > +						}
> >   					} else {
> >   						mark_reg_unknown(env, regs, insn->dst_reg);
> >   					}
> > @@ -15107,7 +15159,15 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
> >   						if (!is_src_reg_u32) {
> >   							if (wide_subreg_link && src_reg->id) {
> >   								/* ->id already copied above */
> > -								dst_reg->flags |= BPF_FLAG_SUBREG_ZEXT;
> > +								/*
> > +								 * Zero-extension: high bits are 0, not a
> > +								 * sign-extension of the low field. Drop any
> > +								 * SUBREG_SEXT copied from a sext-linked src
> > +								 * so sync_linked_regs() rebuilds dst by
> > +								 * zero-extension, not reconstruct_sext32().
> > +								 */
> > +								dst_reg->flags = (dst_reg->flags & ~BPF_FLAG_SUBREG) |
> > +										 BPF_FLAG_SUBREG_ZEXT;
> Nit: please find a way to reduce indentation (e.g. less if-nesting, or a utility function).

Flattened.

> >   							} else {
> >   								clear_scalar_id(dst_reg);
> >   							}
> > @@ -15961,6 +16021,32 @@ static void collect_linked_regs(struct bpf_verifier_env *env,
> >   	}
> >   }
> >   
> > +/*
> > + * Set @reg to the sign-extension of the low 32 bits currently held by @src.
> > + * A BPF_FLAG_SUBREG_SEXT-linked register came from a 32-bit sign
> > + * extension (r0 = (s32)r0): it shares @src's low 32 bits and its high bits are
> > + * the sign-extension of that low field. Only the value fields are written;
> > + * @reg's linkage fields (id, delta, flags) are left intact by
> > + * the caller (___mark_reg_known touches only var_off/r64/r32). Callers must
> > + * ensure no ADD_CONST delta is involved (see sync_linked_regs()).
> > + */
> > +static void reconstruct_sext32(struct bpf_reg_state *reg, struct bpf_reg_state *src)
> Nit: let's rename src -> known_reg, to make reading sync_linked_regs() simpler.

Done.


> > +{
> > +	s32 s32min = reg_s32_min(src);
> > +	s32 s32max = reg_s32_max(src);
> > +
> > +	if (s32min == s32max) {
> > +		/* Low 32 bits are constant -> the whole value is the sext constant. */
> > +		___mark_reg_known(reg, (u64)(s64)s32min);
> > +	} else {
> > +		/* Sign-extension is monotonic over the signed-32 range. */
> > +		reg_set_srange64(reg, (s64)s32min, (s64)s32max);
> > +		reg_set_srange32(reg, s32min, s32max);
> > +		reg->var_off = tnum_range((u64)(s64)s32min, (u64)(s64)s32max);
> Note that known lower 32-bits of the known_reg->var_off are lost,
> we might benefit from adding a dedicated tnum_sext().

Added.

> > +		reg_bounds_sync(reg);
> > +	}
> > +}
> > +
> >   /* For all R in linked_regs, copy known_reg range into R
> >    * if R->id == known_reg->id.
> >    */
> > @@ -15984,17 +16070,21 @@ static void sync_linked_regs(struct bpf_verifier_env *env, struct bpf_verifier_s
> >   		 * A low-32 linked register shares only the base's low 32 bits;
> >   		 * the flag says how its high bits are derived. For
> >   		 * BPF_FLAG_SUBREG_ZEXT they are zero (32-bit zero-extending mov).
> > +		 * For BPF_FLAG_SUBREG_SEXT they are the sign-extension of the low
> > +		 * field (32-bit sign extension).
> >   		 * Rebuild it from known_reg's low 32 bits accordingly, but only
> >   		 * when neither side carries an ADD_CONST delta -- with a delta
> >   		 * the low bits differ from the base by that delta and the combined
> >   		 * subreg+ADD_CONST reconstruction isn't modeled here, so leave reg
> >   		 * unchanged (sound, just less precise).
> >   		 */
> > -		if (reg->flags & BPF_FLAG_SUBREG_ZEXT) {
> > +		if (reg->flags & BPF_FLAG_SUBREG) {
> >   			if (!((reg->flags | known_reg->flags) & BPF_FLAG_ADD_CONST)) {
> > -				{
> > +				if (reg->flags & BPF_FLAG_SUBREG_SEXT) {
> > +					reconstruct_sext32(reg, known_reg);
> > +				} else {
> Let's move this branch to a dedicated utility function as well.

This is all simplified in-place now.

-                       reconstruct_zext32(reg, known_reg);
+                       if (reg->subreg == SUBREG_ZEXT)
+                               reconstruct_zext32(reg, known_reg);
+                       else
+                               reconstruct_sext32(reg, known_reg);



coerce_reg_to_size_sx() reads the 64-bit smin/smax, while a 32-bit 
compare narrows the 32-bit range.

For a simple test (actually added)

call %[bpf_get_prandom_u32];
r6 = r0;
r7 = (s32)r6;         /* forms the link */
if w6 != -1 goto 1f;  /* narrows r6's low 32 to all ones */
if r7 == -1 goto 1f;  /* r7 must follow, sign-extended */
r0 /= 0;              /* reachable only if it didn't */
1:
r0 = 0;
exit;

With coerce_reg_to_size_sx(reg, 4) it fails

2: (bf) r7 = (s32)r6   ; 
R7=scalar(id=1.lo32sx,smin=0xffffffff80000000,smax=0x7fffffff)
3: (56) if w6 != 0xffffffff goto pc+2
 R6=scalar(id=1,smin=0x80000000ffffffff,smin32=-1,smax32=-1,var_off=(0xffffffff; 
0xffffffff00000000))
     R7=scalar(id=1.lo32sx,smin=0xffffffff80000000,smax=0x7fffffff) 
 <-- unchanged

With sext_32_to_64() it passes:

3: (56) if w6 != 0xffffffff goto pc+2
 R6=scalar(id=1,smin=0x80000000ffffffff,smin32=-1,smax32=-1,var_off=(0xffffffff; 
0xffffffff00000000))
     R7=-1   <-- reconstructed

Thx,
-Vineet