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
