On 9/12/26 11:50 AM, Alexei Starovoitov wrote:
On Thu Sep 10, 2026 at 9:46 AM PDT, Vineet Gupta wrote:bpf_reg_state->id doubles as a linked-register id and, in its top two bits, as a record of how the register relates to that set: #define BPF_ADD_CONST64 (1U << 31) #define BPF_ADD_CONST32 (1U << 30) Every user of ->id therefore has to mask, and more link kinds are coming. Move the two bits into a bitfield next to ->precise, which is the last field of the struct and outside every memcmp() window used for state comparison, so the layout and all byte-wise comparisons are unchanged. The two kinds are mutually exclusive, so a 2-bit enum captures them and makes ADD_CONST_32 vs ADD_CONST_64 explicit at each use. ->id becomes a plain 32-bit identifier: no masking anywhere, and check_scalar_ids() loses its two-level "check the compound id, then the base id" dance in favour of a single check_ids(). While here, use regs_exact() for the explore_alu_limits case in regsafe(): it is what that open-coded memcmp+check_scalar_ids pair amounts to, and it picks up the add_const comparison for free (parent_id is 0 for SCALAR_VALUE). check_stack_write_fixed_off() cleared ->id directly on a narrowing spill, which would now leave ->add_const set without an id; use clear_scalar_id(). Moving the kind out of ->id also drops an incidental comparison in regs_exact(), which used to see it as part of the idmap key; the next patch restores it. Otherwise no functional change intended. Suggested-by: Eduard Zingerman <[email protected]> Signed-off-by: Vineet Gupta <[email protected]> --- v2: was RFC 2/6. - kinds are a 2-bit enum bitfield, not a byte of flags; RFC 1/6, which turned ->precise into that byte, is dropped (Eduard) - use regs_exact() for the explore_alu_limits case - clear_scalar_id() on the narrowing spill, which would otherwise leave ->add_const set without an id include/linux/bpf_verifier.h | 25 ++++++++----- kernel/bpf/log.c | 4 +-- kernel/bpf/states.c | 35 +++++-------------- kernel/bpf/verifier.c | 35 +++++++++++-------- .../bpf/progs/verifier_linked_scalars.c | 34 +++++++++--------- 5 files changed, 65 insertions(+), 68 deletions(-) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 9727df5af83a..afb1e5628698 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -35,6 +35,17 @@ enum bpf_iter_state { BPF_ITER_STATE_DRAINED, };+/*+ * Records that a register is (base + ->delta) within its ->id set: + * r1 += 10; r1 gets ADD_CONST_64 delta + * w3 += 10; r3 gets ADD_CONST_32 deltaw3 gets ?
OK.
+ */ +enum bpf_add_const { + ADD_CONST_NONE = 0, + ADD_CONST_32, /* delta was added with a 32-bit ALU op */ + ADD_CONST_64, /* ... with a 64-bit ALU op */ +}; + struct bpf_reg_state { /* Ordering of fields matters. See states_equal() */ enum bpf_reg_type type; @@ -136,16 +147,9 @@ struct bpf_reg_state { * to a specific instance of bpf_iter. */ /* - * Upper bit of ID is used to remember relationship between "linked" - * registers. Example: + * Registers sharing an ->id are "linked": * r1 = r2; both will have r1->id == r2->id == N - * r1 += 10; r1->id == N | BPF_ADD_CONST and r1->delta == 10 - * r3 = r2; both will have r3->id == r2->id == N - * w3 += 10; r3->id == N | BPF_ADD_CONST32 and r3->delta == 10 */ -#define BPF_ADD_CONST64 (1U << 31) -#define BPF_ADD_CONST32 (1U << 30) -#define BPF_ADD_CONST (BPF_ADD_CONST64 | BPF_ADD_CONST32) u32 id; /* * Tracks the parent object this register was derived from. @@ -164,6 +168,11 @@ struct bpf_reg_state { u32 frameno; /* if (!precise && SCALAR_VALUE) min/max/tnum don't affect safety */ bool precise; + /* + * How this register relates to the others sharing its ->id. + * Non-zero only if ->id is. + */ + enum bpf_add_const add_const:2; };static inline s64 reg_smin(const struct bpf_reg_state *reg)diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c index fb032dfdc0de..f8d7a5c8052f 100644 --- a/kernel/bpf/log.c +++ b/kernel/bpf/log.c @@ -651,8 +651,8 @@ static void print_reg_state(struct bpf_verifier_env *env, verbose(env, "%s", btf_type_name(reg->btf, reg->btf_id)); verbose(env, "("); if (reg->id) - verbose_a("id=%d", reg->id & ~BPF_ADD_CONST); - if (reg->id & BPF_ADD_CONST) + verbose_a("id=%d", reg->id); + if (reg->add_const) verbose(env, "%+d", reg->delta); if (reg->parent_id) verbose_a("parent_id=%d", reg->parent_id); diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c index 66fb11b6c6a7..d974baad37ee 100644 --- a/kernel/bpf/states.c +++ b/kernel/bpf/states.c @@ -369,13 +369,6 @@ static bool check_ids(u32 old_id, u32 cur_id, struct bpf_idmap *idmap) * and r7.id=0 (both independent), without temp IDs both would map old_id=X * to cur_id=0 and pass. With temp IDs: r6 maps X->temp1, r7 tries to map * X->temp2, but X is already mapped to temp1, so the check fails correctly. - * - * When old_id has BPF_ADD_CONST set, the compound id (base | flag) and the - * base id (flag stripped) must both map consistently. Example: old has - * r2.id=A, r3.id=A|flag (r3 = r2 + delta), cur has r2.id=B, r3.id=C|flag - * (r3 derived from unrelated r4). Without the base check, idmap gets two - * independent entries A->B and A|flag->C|flag, missing that A->C conflicts - * with A->B. The base ID cross-check catches this. */ static bool check_scalar_ids(u32 old_id, u32 cur_id, struct bpf_idmap *idmap) { @@ -384,15 +377,7 @@ static bool check_scalar_ids(u32 old_id, u32 cur_id, struct bpf_idmap *idmap)cur_id = cur_id ? cur_id : ++idmap->tmp_id_gen; - if (!check_ids(old_id, cur_id, idmap))- return false; - if (old_id & BPF_ADD_CONST) { - old_id &= ~BPF_ADD_CONST; - cur_id &= ~BPF_ADD_CONST; - if (!check_ids(old_id, cur_id, idmap)) - return false; - } - return true; + return check_ids(old_id, cur_id, idmap); }static void __clean_func_state(struct bpf_verifier_env *env,@@ -542,8 +527,7 @@ static bool regsafe(struct bpf_verifier_env *env, struct bpf_reg_state *rold, /* explore_alu_limits disables tnum_in() and range_within() * logic and requires everything to be strict */ - return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0 && - check_scalar_ids(rold->id, rcur->id, idmap); + return regs_exact(rold, rcur, idmap);Why drop memcmp() ? Doesn't look correct.
regs_exact has the exact same memcmp so I don't think we are dropping it, but .....
| static bool regs_exact(const struct bpf_reg_state *rold,
| const struct bpf_reg_state *rcur,
| struct bpf_idmap *idmap)
| {
| return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0 &&
| check_ids(rold->id, rcur->id, idmap) &&
| check_ids(rold->parent_id, rcur->parent_id, idmap);
| }
Also even after above change to check_scalar_ids() the check_scalar_ids() is still no equivalent to check_ids() that regs_exact() is doing.
... Right: reverting back to what we had before.FWIW Eduard had suggested to use regs_exact; I'm not sure if he had something else in mind which we might be overlooking.
This patch should have been refactoring, if so, this change looks unrelated and dubious.
Back when I started, keeping it NFC made more sense. But with all the nuances discovered in the process, I'm inclined to drop the NFC stance and make functional changes - there's at least one which is the following accepted pre-series and now rejected.
old {r2.id=A+delta32} vs cur {r2.id=B+delta64}
The rest looks fine.
Thanks for taking a look. -Vineet
