* [PATCH bpf-next 0/5] bpf: infer zext_dst based on static register liveness analysis
@ 2026-07-31 19:07 Eduard Zingerman
2026-07-31 19:07 ` [PATCH bpf-next 1/5] bpf: do not print a newline after disassembly in bpf_verbose_insn() Eduard Zingerman
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Eduard Zingerman @ 2026-07-31 19:07 UTC (permalink / raw)
To: bpf, ast, andrii
Cc: daniel, martin.lau, kernel-team, yonghong.song, eddyz87, memxor,
iii, gimm78064
Min-gyu Kim reported a bug in 32-bit operations zero extension
handling [1]. The bug was introduced by the commit [2].
The tl;dr of the bug is that the verifier does not carry zero
extension marks across pruning points. The detailed mechanism is
described in patch #3.
Fix this by reworking zero extension logic to avoid main-path based
subreg_def tracking, and instead extend the live registers analysis to
track upper register halves' liveness.
As noted in the commit message for patch #3:
There is one notable drop in precision: whenever a BPF subprogram is
called, all 64 bits of parameter registers are presumed to be used.
The assumption is that such a drop in precision would not inflict
noticeable performance penalty.
Ilya, could you please help with testing this conjecture?
Min-gyu, could you please test the proposed fix against your
reproducer?
Side note:
Patch #1 is a small refactoring for disasm.c, as patch #3 adds a new
location where bpf_verbose_insn() is called and there as well I have
to wrangle with the newline added by the disasm code.
[1] https://lore.kernel.org/bpf/CAGKGUv=sOuqQtA1Ub-5JXfA4FPosJFYKAQE4B79cK+P1erxqtg@mail.gmail.com/
[2] commit 107e16979905 ("bpf: disable and remove registers chain based liveness")
---
Eduard Zingerman (5):
bpf: do not print a newline after disassembly in bpf_verbose_insn()
bpf: track upper 32-bit register halves' liveness in compute_live_registers()
bpf: infer zext_dst based on static register liveness analysis
bpf: simplify the bpf_is_reg64() signature
selftests/bpf: verify zext_dst annotations for various instructions
include/linux/bpf_verifier.h | 10 +-
kernel/bpf/backtrack.c | 1 +
kernel/bpf/disasm.c | 68 +++---
kernel/bpf/fixups.c | 19 +-
kernel/bpf/liveness.c | 139 +++++++++---
kernel/bpf/verifier.c | 150 ++-----------
tools/bpf/bpftool/xlated_dumper.c | 10 +-
tools/testing/selftests/bpf/disasm_helpers.c | 3 +-
tools/testing/selftests/bpf/prog_tests/verifier.c | 2 +
tools/testing/selftests/bpf/progs/verifier_zext.c | 260 ++++++++++++++++++++++
10 files changed, 433 insertions(+), 229 deletions(-)
---
base-commit: fdec474c65fd35d5a6e1497ed50a9f98c07192f0
change-id: 20260731-static-zext-938cd128273c
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH bpf-next 1/5] bpf: do not print a newline after disassembly in bpf_verbose_insn() 2026-07-31 19:07 [PATCH bpf-next 0/5] bpf: infer zext_dst based on static register liveness analysis Eduard Zingerman @ 2026-07-31 19:07 ` Eduard Zingerman 2026-07-31 20:24 ` bot+bpf-ci 2026-07-31 19:07 ` [PATCH bpf-next 2/5] bpf: track upper 32-bit register halves' liveness in compute_live_registers() Eduard Zingerman ` (3 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Eduard Zingerman @ 2026-07-31 19:07 UTC (permalink / raw) To: bpf, ast, andrii Cc: daniel, martin.lau, kernel-team, yonghong.song, eddyz87, memxor, iii, gimm78064, Quentin Monnet At the moment there are more callsites that want bpf_verbose_insn() to not print a newline after the instruction, than callsites that want a newline. Drop '\n' from disasm.c. Non-functional change. The changes in bpftool are verified by writing a bpf program using a variety of instructions and comparing `prog dump xlated` output in the following modes: plain, opcodes, visual, visual opcodes. The output before and after the changes is identical. Cc: Quentin Monnet <qmo@kernel.org> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> --- kernel/bpf/backtrack.c | 1 + kernel/bpf/disasm.c | 68 ++++++++++++++-------------- kernel/bpf/liveness.c | 6 +-- kernel/bpf/verifier.c | 1 + tools/bpf/bpftool/xlated_dumper.c | 10 ++-- tools/testing/selftests/bpf/disasm_helpers.c | 3 +- 6 files changed, 42 insertions(+), 47 deletions(-) diff --git a/kernel/bpf/backtrack.c b/kernel/bpf/backtrack.c index 2e4ae0ef0860..7c4599e65ece 100644 --- a/kernel/bpf/backtrack.c +++ b/kernel/bpf/backtrack.c @@ -285,6 +285,7 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx, verbose(env, "stack=%s before ", env->tmp_str_buf); verbose(env, "%d: ", idx); bpf_verbose_insn(env, insn); + verbose(env, "\n"); } /* If there is a history record that some registers gained range at this insn, diff --git a/kernel/bpf/disasm.c b/kernel/bpf/disasm.c index 0391b3bc0073..50b3ca5149a0 100644 --- a/kernel/bpf/disasm.c +++ b/kernel/bpf/disasm.c @@ -139,7 +139,7 @@ static void print_bpf_end_insn(bpf_insn_print_t verbose, void *private_data, const struct bpf_insn *insn) { - verbose(private_data, "(%02x) r%d = %s%d r%d\n", + verbose(private_data, "(%02x) r%d = %s%d r%d", insn->code, insn->dst_reg, BPF_SRC(insn->code) == BPF_TO_BE ? "be" : "le", insn->imm, insn->dst_reg); @@ -149,7 +149,7 @@ static void print_bpf_bswap_insn(bpf_insn_print_t verbose, void *private_data, const struct bpf_insn *insn) { - verbose(private_data, "(%02x) r%d = bswap%d r%d\n", + verbose(private_data, "(%02x) r%d = bswap%d r%d", insn->code, insn->dst_reg, insn->imm, insn->dst_reg); } @@ -197,19 +197,19 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs, else print_bpf_end_insn(verbose, cbs->private_data, insn); } else if (BPF_OP(insn->code) == BPF_NEG) { - verbose(cbs->private_data, "(%02x) %c%d = -%c%d\n", + verbose(cbs->private_data, "(%02x) %c%d = -%c%d", insn->code, class == BPF_ALU ? 'w' : 'r', insn->dst_reg, class == BPF_ALU ? 'w' : 'r', insn->dst_reg); } else if (is_addr_space_cast(insn)) { - verbose(cbs->private_data, "(%02x) r%d = addr_space_cast(r%d, %u, %u)\n", + verbose(cbs->private_data, "(%02x) r%d = addr_space_cast(r%d, %u, %u)", insn->code, insn->dst_reg, insn->src_reg, ((u32)insn->imm) >> 16, (u16)insn->imm); } else if (is_mov_percpu_addr(insn)) { - verbose(cbs->private_data, "(%02x) r%d = &(void __percpu *)(r%d)\n", + verbose(cbs->private_data, "(%02x) r%d = &(void __percpu *)(r%d)", insn->code, insn->dst_reg, insn->src_reg); } else if (BPF_SRC(insn->code) == BPF_X) { - verbose(cbs->private_data, "(%02x) %c%d %s %s%c%d\n", + verbose(cbs->private_data, "(%02x) %c%d %s %s%c%d", insn->code, class == BPF_ALU ? 'w' : 'r', insn->dst_reg, is_sdiv_smod(insn) ? bpf_alu_sign_string[BPF_OP(insn->code) >> 4] @@ -218,7 +218,7 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs, class == BPF_ALU ? 'w' : 'r', insn->src_reg); } else { - verbose(cbs->private_data, "(%02x) %c%d %s %d\n", + verbose(cbs->private_data, "(%02x) %c%d %s %d", insn->code, class == BPF_ALU ? 'w' : 'r', insn->dst_reg, is_sdiv_smod(insn) ? bpf_alu_sign_string[BPF_OP(insn->code) >> 4] @@ -227,7 +227,7 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs, } } else if (class == BPF_STX) { if (BPF_MODE(insn->code) == BPF_MEM) - verbose(cbs->private_data, "(%02x) *(%s *)(r%d %+d) = r%d\n", + verbose(cbs->private_data, "(%02x) *(%s *)(r%d %+d) = r%d", insn->code, bpf_ldst_string[BPF_SIZE(insn->code) >> 3], insn->dst_reg, @@ -235,7 +235,7 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs, else if (BPF_MODE(insn->code) == BPF_ATOMIC && (insn->imm == BPF_ADD || insn->imm == BPF_AND || insn->imm == BPF_OR || insn->imm == BPF_XOR)) { - verbose(cbs->private_data, "(%02x) lock *(%s *)(r%d %+d) %s r%d\n", + verbose(cbs->private_data, "(%02x) lock *(%s *)(r%d %+d) %s r%d", insn->code, bpf_ldst_string[BPF_SIZE(insn->code) >> 3], insn->dst_reg, insn->off, @@ -246,7 +246,7 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs, insn->imm == (BPF_AND | BPF_FETCH) || insn->imm == (BPF_OR | BPF_FETCH) || insn->imm == (BPF_XOR | BPF_FETCH))) { - verbose(cbs->private_data, "(%02x) r%d = atomic%s_fetch_%s((%s *)(r%d %+d), r%d)\n", + verbose(cbs->private_data, "(%02x) r%d = atomic%s_fetch_%s((%s *)(r%d %+d), r%d)", insn->code, insn->src_reg, BPF_SIZE(insn->code) == BPF_DW ? "64" : "", bpf_atomic_alu_string[BPF_OP(insn->imm) >> 4], @@ -254,7 +254,7 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs, insn->dst_reg, insn->off, insn->src_reg); } else if (BPF_MODE(insn->code) == BPF_ATOMIC && insn->imm == BPF_CMPXCHG) { - verbose(cbs->private_data, "(%02x) r0 = atomic%s_cmpxchg((%s *)(r%d %+d), r0, r%d)\n", + verbose(cbs->private_data, "(%02x) r0 = atomic%s_cmpxchg((%s *)(r%d %+d), r0, r%d)", insn->code, BPF_SIZE(insn->code) == BPF_DW ? "64" : "", bpf_ldst_string[BPF_SIZE(insn->code) >> 3], @@ -262,44 +262,44 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs, insn->src_reg); } else if (BPF_MODE(insn->code) == BPF_ATOMIC && insn->imm == BPF_XCHG) { - verbose(cbs->private_data, "(%02x) r%d = atomic%s_xchg((%s *)(r%d %+d), r%d)\n", + verbose(cbs->private_data, "(%02x) r%d = atomic%s_xchg((%s *)(r%d %+d), r%d)", insn->code, insn->src_reg, BPF_SIZE(insn->code) == BPF_DW ? "64" : "", bpf_ldst_string[BPF_SIZE(insn->code) >> 3], insn->dst_reg, insn->off, insn->src_reg); } else if (BPF_MODE(insn->code) == BPF_ATOMIC && insn->imm == BPF_LOAD_ACQ) { - verbose(cbs->private_data, "(%02x) r%d = load_acquire((%s *)(r%d %+d))\n", + verbose(cbs->private_data, "(%02x) r%d = load_acquire((%s *)(r%d %+d))", insn->code, insn->dst_reg, bpf_ldst_string[BPF_SIZE(insn->code) >> 3], insn->src_reg, insn->off); } else if (BPF_MODE(insn->code) == BPF_ATOMIC && insn->imm == BPF_STORE_REL) { - verbose(cbs->private_data, "(%02x) store_release((%s *)(r%d %+d), r%d)\n", + verbose(cbs->private_data, "(%02x) store_release((%s *)(r%d %+d), r%d)", insn->code, bpf_ldst_string[BPF_SIZE(insn->code) >> 3], insn->dst_reg, insn->off, insn->src_reg); } else { - verbose(cbs->private_data, "BUG_%02x\n", insn->code); + verbose(cbs->private_data, "BUG_%02x", insn->code); } } else if (class == BPF_ST) { if (BPF_MODE(insn->code) == BPF_MEM) { - verbose(cbs->private_data, "(%02x) *(%s *)(r%d %+d) = %d\n", + verbose(cbs->private_data, "(%02x) *(%s *)(r%d %+d) = %d", insn->code, bpf_ldst_string[BPF_SIZE(insn->code) >> 3], insn->dst_reg, insn->off, insn->imm); } else if (BPF_MODE(insn->code) == 0xc0 /* BPF_NOSPEC, no UAPI */) { - verbose(cbs->private_data, "(%02x) nospec\n", insn->code); + verbose(cbs->private_data, "(%02x) nospec", insn->code); } else { - verbose(cbs->private_data, "BUG_st_%02x\n", insn->code); + verbose(cbs->private_data, "BUG_st_%02x", insn->code); } } else if (class == BPF_LDX) { if (BPF_MODE(insn->code) != BPF_MEM && BPF_MODE(insn->code) != BPF_MEMSX) { - verbose(cbs->private_data, "BUG_ldx_%02x\n", insn->code); + verbose(cbs->private_data, "BUG_ldx_%02x", insn->code); return; } - verbose(cbs->private_data, "(%02x) r%d = *(%s *)(r%d %+d)\n", + verbose(cbs->private_data, "(%02x) r%d = *(%s *)(r%d %+d)", insn->code, insn->dst_reg, BPF_MODE(insn->code) == BPF_MEM ? bpf_ldst_string[BPF_SIZE(insn->code) >> 3] : @@ -307,12 +307,12 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs, insn->src_reg, insn->off); } else if (class == BPF_LD) { if (BPF_MODE(insn->code) == BPF_ABS) { - verbose(cbs->private_data, "(%02x) r0 = *(%s *)skb[%d]\n", + verbose(cbs->private_data, "(%02x) r0 = *(%s *)skb[%d]", insn->code, bpf_ldst_string[BPF_SIZE(insn->code) >> 3], insn->imm); } else if (BPF_MODE(insn->code) == BPF_IND) { - verbose(cbs->private_data, "(%02x) r0 = *(%s *)skb[r%d + %d]\n", + verbose(cbs->private_data, "(%02x) r0 = *(%s *)skb[r%d + %d]", insn->code, bpf_ldst_string[BPF_SIZE(insn->code) >> 3], insn->src_reg, insn->imm); @@ -332,12 +332,12 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs, if (is_ptr && !allow_ptr_leaks) imm = 0; - verbose(cbs->private_data, "(%02x) r%d = %s\n", + verbose(cbs->private_data, "(%02x) r%d = %s", insn->code, insn->dst_reg, __func_imm_name(cbs, insn, imm, tmp, sizeof(tmp))); } else { - verbose(cbs->private_data, "BUG_ld_%02x\n", insn->code); + verbose(cbs->private_data, "BUG_ld_%02x", insn->code); return; } } else if (class == BPF_JMP32 || class == BPF_JMP) { @@ -347,35 +347,35 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs, char tmp[64]; if (insn->src_reg == BPF_PSEUDO_CALL) { - verbose(cbs->private_data, "(%02x) call pc%s\n", + verbose(cbs->private_data, "(%02x) call pc%s", insn->code, __func_get_name(cbs, insn, tmp, sizeof(tmp))); } else { strcpy(tmp, "unknown"); - verbose(cbs->private_data, "(%02x) call %s#%d\n", insn->code, + verbose(cbs->private_data, "(%02x) call %s#%d", insn->code, __func_get_name(cbs, insn, tmp, sizeof(tmp)), insn->imm); } } else if (insn->code == (BPF_JMP | BPF_JA)) { - verbose(cbs->private_data, "(%02x) goto pc%+d\n", + verbose(cbs->private_data, "(%02x) goto pc%+d", insn->code, insn->off); } else if (insn->code == (BPF_JMP | BPF_JA | BPF_X)) { - verbose(cbs->private_data, "(%02x) gotox r%d\n", + verbose(cbs->private_data, "(%02x) gotox r%d", insn->code, insn->dst_reg); } else if (insn->code == (BPF_JMP | BPF_JCOND) && insn->src_reg == BPF_MAY_GOTO) { - verbose(cbs->private_data, "(%02x) may_goto pc%+d\n", + verbose(cbs->private_data, "(%02x) may_goto pc%+d", insn->code, insn->off); } else if (insn->code == (BPF_JMP32 | BPF_JA)) { - verbose(cbs->private_data, "(%02x) gotol pc%+d\n", + verbose(cbs->private_data, "(%02x) gotol pc%+d", insn->code, insn->imm); } else if (insn->code == (BPF_JMP | BPF_EXIT)) { - verbose(cbs->private_data, "(%02x) exit\n", insn->code); + verbose(cbs->private_data, "(%02x) exit", insn->code); } else if (BPF_SRC(insn->code) == BPF_X) { verbose(cbs->private_data, - "(%02x) if %c%d %s %c%d goto pc%+d\n", + "(%02x) if %c%d %s %c%d goto pc%+d", insn->code, class == BPF_JMP32 ? 'w' : 'r', insn->dst_reg, bpf_jmp_string[BPF_OP(insn->code) >> 4], @@ -383,14 +383,14 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs, insn->src_reg, insn->off); } else { verbose(cbs->private_data, - "(%02x) if %c%d %s 0x%x goto pc%+d\n", + "(%02x) if %c%d %s 0x%x goto pc%+d", insn->code, class == BPF_JMP32 ? 'w' : 'r', insn->dst_reg, bpf_jmp_string[BPF_OP(insn->code) >> 4], (u32)insn->imm, insn->off); } } else { - verbose(cbs->private_data, "(%02x) %s\n", + verbose(cbs->private_data, "(%02x) %s", insn->code, bpf_class_string[class]); } } diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c index 0aadfbae0acc..ff1e68cc4bd1 100644 --- a/kernel/bpf/liveness.c +++ b/kernel/bpf/liveness.c @@ -497,7 +497,6 @@ static void print_instance(struct bpf_verifier_env *env, struct func_instance *i pos = env->log.end_pos; verbose(env, "%3d: ", insn_idx); bpf_verbose_insn(env, &insns[insn_idx]); - bpf_vlog_reset(&env->log, env->log.end_pos - 1); /* remove \n */ insn_pos = env->log.end_pos; verbose(env, "%*c;", bpf_vlog_alignment(insn_pos - pos), ' '); pos = env->log.end_pos; @@ -1043,7 +1042,6 @@ static void arg_track_log(struct bpf_verifier_env *env, struct bpf_insn *insn, i if (!printed) { verbose(env, "%3d: ", idx); bpf_verbose_insn(env, insn); - bpf_vlog_reset(&env->log, env->log.end_pos - 1); printed = true; } verbose(env, "\tr%d: ", i); verbose_arg_track(env, &at_in[i]); @@ -1058,7 +1056,6 @@ static void arg_track_log(struct bpf_verifier_env *env, struct bpf_insn *insn, i if (!printed) { verbose(env, "%3d: ", idx); bpf_verbose_insn(env, insn); - bpf_vlog_reset(&env->log, env->log.end_pos - 1); printed = true; } verbose(env, "\tsa%d: ", i); verbose_arg_track(env, &at_in[ai]); @@ -1070,7 +1067,6 @@ static void arg_track_log(struct bpf_verifier_env *env, struct bpf_insn *insn, i if (!printed) { verbose(env, "%3d: ", idx); bpf_verbose_insn(env, insn); - bpf_vlog_reset(&env->log, env->log.end_pos - 1); printed = true; } verbose(env, "\tfp%+d: ", -(i + 1) * 8); verbose_arg_track(env, &at_stack_in[i]); @@ -1545,6 +1541,7 @@ static void print_subprog_arg_access(struct bpf_verifier_env *env, verbose(env, "%3d: ", idx); bpf_verbose_insn(env, &insns[idx]); + verbose(env, "\n"); /* Collect what needs printing */ if (is_ldx_stx_call && @@ -2285,6 +2282,7 @@ int bpf_compute_live_registers(struct bpf_verifier_env *env) verbose(env, "."); verbose(env, " "); bpf_verbose_insn(env, &insns[i]); + verbose(env, "\n"); if (bpf_is_ldimm64(&insns[i])) i++; } diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index e6f35f4e715b..6123e7e4a320 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -17475,6 +17475,7 @@ static int do_check(struct bpf_verifier_env *env) env->prev_log_pos = env->log.end_pos; verbose(env, "%d: ", env->insn_idx); bpf_verbose_insn(env, insn); + verbose(env, "\n"); env->prev_insn_print_pos = env->log.end_pos - env->prev_log_pos; env->prev_log_pos = env->log.end_pos; } diff --git a/tools/bpf/bpftool/xlated_dumper.c b/tools/bpf/bpftool/xlated_dumper.c index 5e7cb8b36fef..7b33d847d800 100644 --- a/tools/bpf/bpftool/xlated_dumper.c +++ b/tools/bpf/bpftool/xlated_dumper.c @@ -129,16 +129,10 @@ print_insn_for_graph(void *private_data, const char *fmt, ...) static void __printf(2, 3) print_insn_json(void *private_data, const char *fmt, ...) { - unsigned int l = strlen(fmt); - char chomped_fmt[l]; va_list args; va_start(args, fmt); - if (l > 0) { - strncpy(chomped_fmt, fmt, l - 1); - chomped_fmt[l - 1] = '\0'; - } - jsonw_vprintf_enquote(json_wtr, chomped_fmt, args); + jsonw_vprintf_enquote(json_wtr, fmt, args); va_end(args); } @@ -351,6 +345,7 @@ void dump_xlated_plain(struct dump_data *dd, void *buf, unsigned int len, printf("%4u: ", i); print_bpf_insn(&cbs, insn + i, true); + printf("\n"); if (opcodes) { printf(" "); @@ -417,6 +412,7 @@ void dump_xlated_for_graph(struct dump_data *dd, void *buf_start, void *buf_end, printf("%u: ", insn_off); print_bpf_insn(&cbs, cur, true); + printf("\\l\\\n"); if (opcodes) { printf("\\ \\ \\ \\ "); diff --git a/tools/testing/selftests/bpf/disasm_helpers.c b/tools/testing/selftests/bpf/disasm_helpers.c index f529f1c8c171..30221352568d 100644 --- a/tools/testing/selftests/bpf/disasm_helpers.c +++ b/tools/testing/selftests/bpf/disasm_helpers.c @@ -55,10 +55,9 @@ struct bpf_insn *disasm_insn(struct bpf_insn *insn, char *buf, size_t buf_sz) * for each instruction (FF stands for instruction `code` byte). * Remove the prefix inplace, and also simplify call instructions. * E.g.: "(85) call foo#10" -> "call foo". - * Also remove newline in the end (the 'max(strlen(buf) - 1, 0)' thing). */ pfx_end = buf + 5; - sfx_start = buf + max((int)strlen(buf) - 1, 0); + sfx_start = buf + (int)strlen(buf); if (strncmp(pfx_end, "call ", 5) == 0 && (tmp = strrchr(buf, '#'))) sfx_start = tmp; len = sfx_start - pfx_end; -- 2.55.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/5] bpf: do not print a newline after disassembly in bpf_verbose_insn() 2026-07-31 19:07 ` [PATCH bpf-next 1/5] bpf: do not print a newline after disassembly in bpf_verbose_insn() Eduard Zingerman @ 2026-07-31 20:24 ` bot+bpf-ci 2026-07-31 21:12 ` Eduard Zingerman 0 siblings, 1 reply; 13+ messages in thread From: bot+bpf-ci @ 2026-07-31 20:24 UTC (permalink / raw) To: eddyz87, bpf, ast, andrii Cc: daniel, martin.lau, kernel-team, yonghong.song, eddyz87, memxor, iii, gimm78064, qmo, ast, andrii, daniel, martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai [-- Attachment #1: Type: text/plain, Size: 1799 bytes --] > diff --git a/tools/bpf/bpftool/xlated_dumper.c b/tools/bpf/bpftool/xlated_dumper.c > index 5e7cb8b36fef2..7b33d847d800c 100644 > --- a/tools/bpf/bpftool/xlated_dumper.c > +++ b/tools/bpf/bpftool/xlated_dumper.c [ ... ] > @@ -417,6 +412,7 @@ void dump_xlated_for_graph(struct dump_data *dd, void *buf_start, void *buf_end, > > printf("%u: ", insn_off); > print_bpf_insn(&cbs, cur, true); > + printf("\\l\\\n"); > > if (opcodes) { > printf("\\ \\ \\ \\ "); This isn't a bug, but now that no disasm.c format string carries a newline and dump_xlated_for_graph() emits "\\l\\\n" itself, is the '\n' arm of print_insn_for_graph() still reachable? Looking at print_insn_for_graph(): static void __printf(2, 3) print_insn_for_graph(void *private_data, const char *fmt, ...) { ... p = buf; while (*p != '\0') { if (*p == '\n') { memmove(p + 3, p, strlen(buf) + 1 - (p - buf)); /* Align each instruction dump row left. */ *p++ = '\\'; *p++ = 'l'; /* Output multiline concatenation. */ *p++ = '\\'; } else if (*p == '<' || *p == '>' || *p == '|' || *p == '&') { ... } It looks like the `if (*p == '\n')` arm no longer has any producer, since this patch drops '\n' from all 34 verbose() format strings in kernel/bpf/disasm.c and the interpolated values (symbol names parsed with sscanf "%s") cannot contain whitespace either. Could this arm be dropped so only the '<'/'>'/'|'/'&' escaping remains? Note this would also retire the latent `memmove(p + 3, ...)` expansion that had no headroom check against the 64-byte buf. --- 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/30658911996 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/5] bpf: do not print a newline after disassembly in bpf_verbose_insn() 2026-07-31 20:24 ` bot+bpf-ci @ 2026-07-31 21:12 ` Eduard Zingerman 0 siblings, 0 replies; 13+ messages in thread From: Eduard Zingerman @ 2026-07-31 21:12 UTC (permalink / raw) To: bot+bpf-ci, bpf, ast, andrii Cc: daniel, martin.lau, kernel-team, yonghong.song, memxor, iii, gimm78064, qmo, martin.lau, clm, ihor.solodrai On Fri, 2026-07-31 at 20:24 +0000, bot+bpf-ci@kernel.org wrote: > > diff --git a/tools/bpf/bpftool/xlated_dumper.c b/tools/bpf/bpftool/xlated_dumper.c > > index 5e7cb8b36fef2..7b33d847d800c 100644 > > --- a/tools/bpf/bpftool/xlated_dumper.c > > +++ b/tools/bpf/bpftool/xlated_dumper.c > > [ ... ] > > > @@ -417,6 +412,7 @@ void dump_xlated_for_graph(struct dump_data *dd, void *buf_start, void *buf_end, > > > > printf("%u: ", insn_off); > > print_bpf_insn(&cbs, cur, true); > > + printf("\\l\\\n"); > > > > if (opcodes) { > > printf("\\ \\ \\ \\ "); > > This isn't a bug, but now that no disasm.c format string carries a > newline and dump_xlated_for_graph() emits "\\l\\\n" itself, is the > '\n' arm of print_insn_for_graph() still reachable? It is a dead code and it should be removed. ... ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next 2/5] bpf: track upper 32-bit register halves' liveness in compute_live_registers() 2026-07-31 19:07 [PATCH bpf-next 0/5] bpf: infer zext_dst based on static register liveness analysis Eduard Zingerman 2026-07-31 19:07 ` [PATCH bpf-next 1/5] bpf: do not print a newline after disassembly in bpf_verbose_insn() Eduard Zingerman @ 2026-07-31 19:07 ` Eduard Zingerman 2026-07-31 19:18 ` sashiko-bot 2026-07-31 19:07 ` [PATCH bpf-next 3/5] bpf: infer zext_dst based on static register liveness analysis Eduard Zingerman ` (2 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Eduard Zingerman @ 2026-07-31 19:07 UTC (permalink / raw) To: bpf, ast, andrii Cc: daniel, martin.lau, kernel-team, yonghong.song, eddyz87, memxor, iii, gimm78064 Extend compute_live_registers() to track upper and lower register halves' liveness separately. This is mostly straightforward: - use/def masks are extended to track 2 bits per register; - compute_insn_live_regs() is updated to properly track these 2 bits according to the instruction semantics. The only quirk is kfunc call processing logic, where we follow the verifier.c:check_kfunc_call() and verifier.c:mark_btf_func_reg_size() and infer whether the upper half of the parameter register is used by the call based on the parameter's BTF type size. Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> --- include/linux/bpf_verifier.h | 2 + kernel/bpf/liveness.c | 119 +++++++++++++++++++++++++++++++------------ kernel/bpf/verifier.c | 5 +- 3 files changed, 93 insertions(+), 33 deletions(-) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 682c2cd3b844..d4bffed94e0a 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -1050,6 +1050,8 @@ static inline struct bpf_subprog_info *subprog_info(struct bpf_verifier_env *env } struct bpf_call_summary { + const struct btf *btf; + const struct btf_type *func_proto; u8 num_params; bool is_void; bool fastcall; diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c index ff1e68cc4bd1..c996b8a03e79 100644 --- a/kernel/bpf/liveness.c +++ b/kernel/bpf/liveness.c @@ -2047,29 +2047,67 @@ int bpf_compute_subprog_arg_access(struct bpf_verifier_env *env) /* Each field is a register bitmask */ struct insn_live_regs { - u16 use; /* registers read by instruction */ - u16 def; /* registers written by instruction */ - u16 in; /* registers that may be alive before instruction */ - u16 out; /* registers that may be alive after instruction */ + u32 use; /* registers read by instruction */ + u32 def; /* registers written by instruction */ + u32 in; /* registers that may be alive before instruction */ + u32 out; /* registers that may be alive after instruction */ }; /* Bitmask with 1s for all caller saved registers */ #define ALL_CALLER_SAVED_REGS ((1u << CALLER_SAVED_REGS) - 1) +static inline u32 reg32_mask(u32 n) { return BIT(n); } +static inline u32 reg64_mask(u32 n) { return BIT(n) | BIT(n + 16); } +static inline u32 mask_widen(u32 m) { return m | (m << 16); } +static inline u16 mask_lo(u32 m) { return (u16)m; } +static inline u16 mask_hi(u32 m) { return (u16)(m >> 16); } + +/* Infer use mask based on number and sizes of the function parameters. */ +static u32 call_use_mask(struct bpf_verifier_env *env, struct bpf_insn *insn) +{ + u32 i, use, size, reg_params_num; + struct bpf_call_summary cs; + const struct btf_param *p; + const struct btf_type *t; + + if (!bpf_get_call_summary(env, insn, &cs)) + /* e.g. a bpf-to-bpf call, assume that calls use full 64-bit for each param */ + return mask_widen(ALL_CALLER_SAVED_REGS & ~BIT(BPF_REG_0)); + + if (cs.func_proto) { + use = 0; + p = btf_params(cs.func_proto); + reg_params_num = min(btf_type_vlen(cs.func_proto), MAX_BPF_FUNC_REG_ARGS); + for (i = 0; i < reg_params_num; i++) { + t = btf_type_by_id(cs.btf, p[i].type); + if (IS_ERR(btf_resolve_size(cs.btf, t, &size))) + goto fallback; + use |= size <= sizeof(u32) ? reg32_mask(i + 1) : reg64_mask(i + 1); + } + return use; + } + +fallback: + /* by default assume that calls use full 64-bit for each param */ + return mask_widen(GENMASK(min_t(u8, cs.num_params, MAX_BPF_FUNC_REG_ARGS), 1)); +} + /* Compute info->{use,def} fields for the instruction */ static void compute_insn_live_regs(struct bpf_verifier_env *env, struct bpf_insn *insn, struct insn_live_regs *info) { - struct bpf_call_summary cs; - u8 class = BPF_CLASS(insn->code); - u8 code = BPF_OP(insn->code); - u8 mode = BPF_MODE(insn->code); - u16 src = BIT(insn->src_reg); - u16 dst = BIT(insn->dst_reg); - u16 r0 = BIT(0); - u16 def = 0; - u16 use = 0xffff; + const u8 class = BPF_CLASS(insn->code); + const u8 code = BPF_OP(insn->code); + const u8 mode = BPF_MODE(insn->code); + const u8 size = BPF_SIZE(insn->code); + const u32 src = reg64_mask(insn->src_reg); + const u32 dst = reg64_mask(insn->dst_reg); + const u32 src32 = mask_lo(src); + const u32 dst32 = mask_lo(dst); + const u32 r0 = reg64_mask(0); + u32 def = 0; + u32 use = U32_MAX; switch (class) { case BPF_LD: @@ -2080,8 +2118,8 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env, use = 0; } break; - case BPF_LD | BPF_ABS: - case BPF_LD | BPF_IND: + case BPF_ABS: + case BPF_IND: /* stick with defaults */ break; } @@ -2089,7 +2127,15 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env, case BPF_LDX: switch (mode) { case BPF_MEM: + /* a narrow load still redefines the whole register */ + def = dst; + use = src; + break; case BPF_MEMSX: + /* + * sign extension defines the whole register; + * src holds a pointer, hence is used as 64-bit. + */ def = dst; use = src; break; @@ -2107,12 +2153,19 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env, switch (mode) { case BPF_MEM: def = 0; - use = dst | src; + use = dst | (size == BPF_DW ? src : src32); break; - case BPF_ATOMIC: + case BPF_ATOMIC: { + /* + * dst holds a pointer and is always used as 64-bit; + * the value operand and r0 are read as 32-bit for BPF_W atomics. + */ + u32 srcv = size == BPF_DW ? src : src32; + u32 r0v = size == BPF_DW ? r0 : mask_lo(r0); + switch (insn->imm) { case BPF_CMPXCHG: - use = r0 | dst | src; + use = r0v | dst | srcv; def = r0; break; case BPF_LOAD_ACQ: @@ -2121,10 +2174,10 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env, break; case BPF_STORE_REL: def = 0; - use = dst | src; + use = dst | srcv; break; default: - use = dst | src; + use = dst | srcv; if (insn->imm & BPF_FETCH) def = src; else @@ -2132,6 +2185,7 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env, } break; } + } break; case BPF_ALU: case BPF_ALU64: @@ -2145,14 +2199,14 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env, if (BPF_SRC(insn->code) == BPF_K) use = 0; else - use = src; + use = class == BPF_ALU64 ? src : src32; break; default: def = dst; if (BPF_SRC(insn->code) == BPF_K) - use = dst; + use = class == BPF_ALU64 ? dst : dst32; else - use = dst | src; + use = class == BPF_ALU64 ? (dst | src) : (dst32 | src32); } break; case BPF_JMP: @@ -2174,17 +2228,15 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env, use = r0; break; case BPF_CALL: - def = ALL_CALLER_SAVED_REGS; - use = def & ~BIT(BPF_REG_0); - if (bpf_get_call_summary(env, insn, &cs)) - use = GENMASK(min_t(u8, cs.num_params, MAX_BPF_FUNC_REG_ARGS), 1); + def = mask_widen(ALL_CALLER_SAVED_REGS); + use = call_use_mask(env, insn); break; default: def = 0; if (BPF_SRC(insn->code) == BPF_K) use = dst; else - use = dst | src; + use = dst | (code == BPF_JMP32 ? src : src32); } break; } @@ -2249,8 +2301,8 @@ int bpf_compute_live_registers(struct bpf_verifier_env *env) int insn_idx = env->cfg.insn_postorder[i]; struct insn_live_regs *live = &state[insn_idx]; struct bpf_iarray *succ; - u16 new_out = 0; - u16 new_in = 0; + u32 new_out = 0; + u32 new_in = 0; succ = bpf_insn_successors(env, insn_idx); for (int s = 0; s < succ->cnt; ++s) @@ -2264,8 +2316,11 @@ int bpf_compute_live_registers(struct bpf_verifier_env *env) } } - for (i = 0; i < insn_cnt; ++i) - insn_aux[i].live_regs_before = state[i].in; + for (i = 0; i < insn_cnt; ++i) { + u32 in = state[i].in; + + insn_aux[i].live_regs_before = mask_lo(in) | mask_hi(in); + } if (env->log.level & BPF_LOG_LEVEL2) { verbose(env, "Live regs before insn:\n"); diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6123e7e4a320..8304452edc0f 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -16730,10 +16730,11 @@ bool bpf_get_call_summary(struct bpf_verifier_env *env, struct bpf_insn *call, int i; if (bpf_helper_call(call)) { - if (bpf_get_helper_proto(env, call->imm, &fn) < 0) /* error would be reported later */ return false; + cs->btf = NULL; + cs->func_proto = NULL; cs->fastcall = fn->allow_fastcall && (bpf_verifier_inlines_helper_call(env, call->imm) || bpf_jit_inlines_helper_call(call->imm)); @@ -16754,6 +16755,8 @@ bool bpf_get_call_summary(struct bpf_verifier_env *env, struct bpf_insn *call, if (err < 0) /* error would be reported later */ return false; + cs->btf = meta.btf; + cs->func_proto = meta.func_proto; cs->num_params = btf_type_vlen(meta.func_proto); cs->fastcall = meta.kfunc_flags & KF_FASTCALL; cs->is_void = btf_type_is_void(btf_type_by_id(meta.btf, meta.func_proto->type)); -- 2.55.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/5] bpf: track upper 32-bit register halves' liveness in compute_live_registers() 2026-07-31 19:07 ` [PATCH bpf-next 2/5] bpf: track upper 32-bit register halves' liveness in compute_live_registers() Eduard Zingerman @ 2026-07-31 19:18 ` sashiko-bot 0 siblings, 0 replies; 13+ messages in thread From: sashiko-bot @ 2026-07-31 19:18 UTC (permalink / raw) To: Eduard Zingerman; +Cc: bpf Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Critical verifier bug in conditional jump liveness tracking causing missing zero-extensions. - [Low] Suboptimal liveness tracking for 32-bit conditional jumps causes unnecessary zero-extensions. -- --- Patch [2]: [PATCH bpf-next 2/5] bpf: track upper 32-bit register halves' liveness in compute_live_registers() --- commit 977deff7b46d8e36a957595add75da7e58a91c59 Author: Eduard Zingerman <eddyz87@gmail.com> bpf: track upper 32-bit register halves' liveness in compute_live_registers() This commit extends compute_live_registers() to track upper and lower register halves' liveness separately. It uses 2 bits per register and updates compute_insn_live_regs() and kfunc call processing to infer usage based on BTF type sizes. > diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c > index ff1e68cc4bd1c..c996b8a03e792 100644 > --- a/kernel/bpf/liveness.c > +++ b/kernel/bpf/liveness.c [ ... ] > @@ -2174,17 +2228,15 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env, [ ... ] > default: > def = 0; > if (BPF_SRC(insn->code) == BPF_K) > use = dst; [Severity: Low] Does this unconditionally apply the 64-bit mask for 32-bit jumps? Because dst is defined as the 64-bit mask, using it for BPF_JMP32 conditional jumps incorrectly assumes the upper 32 bits are alive. Could this trigger unnecessary zero-extension instructions in the JIT? > else > - use = dst | src; > + use = dst | (code == BPF_JMP32 ? src : src32); [Severity: Critical] Are the conditions and logic for tracking 64-bit jumps correct here? The variable code is derived using BPF_OP(), which masks out the class bits. Because of this, it will never equal the instruction class BPF_JMP32, making the condition always false. Additionally, if this were checking the class instead of code, it appears the branches of the ternary operator are reversed. As written, 64-bit jumps always evaluate to src32, marking only the lower 32 bits of the source register as live. Could this cause the verifier to incorrectly omit the zero-extension flag for the preceding instruction that produced the source register, potentially leaving garbage data in the upper 32 bits during 64-bit comparisons? > } > break; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/20260731-static-zext-v1-0-98a4dc73e94b@gmail.com?part=2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next 3/5] bpf: infer zext_dst based on static register liveness analysis 2026-07-31 19:07 [PATCH bpf-next 0/5] bpf: infer zext_dst based on static register liveness analysis Eduard Zingerman 2026-07-31 19:07 ` [PATCH bpf-next 1/5] bpf: do not print a newline after disassembly in bpf_verbose_insn() Eduard Zingerman 2026-07-31 19:07 ` [PATCH bpf-next 2/5] bpf: track upper 32-bit register halves' liveness in compute_live_registers() Eduard Zingerman @ 2026-07-31 19:07 ` Eduard Zingerman 2026-07-31 19:26 ` sashiko-bot 2026-07-31 19:07 ` [PATCH bpf-next 4/5] bpf: simplify the bpf_is_reg64() signature Eduard Zingerman 2026-07-31 19:07 ` [PATCH bpf-next 5/5] selftests/bpf: verify zext_dst annotations for various instructions Eduard Zingerman 4 siblings, 1 reply; 13+ messages in thread From: Eduard Zingerman @ 2026-07-31 19:07 UTC (permalink / raw) To: bpf, ast, andrii Cc: daniel, martin.lau, kernel-team, yonghong.song, eddyz87, memxor, iii, gimm78064 As reported in the thread [1], the verifier's 32-bit operations zero extension logic is broken. This logic is responsible for correct semantics of 32-bit operations on s390 architecture. According to BPF semantics, operation `w1 += 1` is supposed to zero extend the upper half of the register `r1`. On s390 the JIT relies on the verifier emitting explicit zero extension before such operations. The verifier attempts to minimize the amount of zero extensions inserted by tracking whether upper halves of the 64-bit registers are ever used. Previously such tracking worked as follows: - bpf_reg_state->subreg_def field was set by do_check_insn() for each operation defining lower but not the upper halves of the register. - Whenever an operation reading the whole register was verified, the verifier checked register's subreg_def and set bpf_insn_aux_data->zext_dst flag as true via a call to mark_insn_zext() function. - After the verification was complete, a special pass bpf_opt_subreg_zext_lo32_rnd_hi32() extended 32-bit operations with bpf_insn_aux_data->zext_dst set as true by adding explicit zero extension. Note that the logic above relies on bpf_reg_state->subreg_def, which is a property of a current verifier state. Before the commit [2] two additional steps happened: - The verifier tracked upper and lower register halves' liveness as flags REG_LIVE_READ{32,64} in bpf_reg_state->live. - The function propagate_liveness() called mark_insn_zext() in order to transfer the knowledge about which registers have their upper halves alive (and thus might require zero extension). The commit [2] removed the two steps described above, hence making possible a situation like below: - The register's upper half is set and is used on some verification path P1 and the register happens not to be marked as precise. - The checkpoint C is created while processing some instruction between register initialization and usage. - On some other verification path P2 the register's upper half is not initialized and that path ends hitting the checkpoint C. - In such a case the register's initialization on path P2 would lack zext_dst mark, making it possible for the program to inject an arbitrary value in the register's upper half. This commit replaces subreg_def based logic with computing zext_dst statically, as a part of the bpf_compute_live_registers() analysis: - The analysis now tracks usage of upper and lower halves of the registers separately. - If some instruction defines a 32-bit subregister, but not the whole register, *and* the upper half of the register is alive after that instruction, the instruction is marked as zext_dst. There is one notable drop in precision: whenever a BPF subprogram is called, all 64 bits of parameter registers are presumed to be used. The assumption is that such a drop in precision would not inflict a noticeable performance penalty. [1] https://lore.kernel.org/bpf/CAGKGUv=sOuqQtA1Ub-5JXfA4FPosJFYKAQE4B79cK+P1erxqtg@mail.gmail.com/ [2] commit 107e16979905 ("bpf: disable and remove registers chain based liveness") Fixes: 107e16979905 ("bpf: disable and remove registers chain based liveness") Reported-by: Min-gyu Kim <gimm78064@gmail.com> Link: https://lore.kernel.org/bpf/CAGKGUv=sOuqQtA1Ub-5JXfA4FPosJFYKAQE4B79cK+P1erxqtg@mail.gmail.com/ Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> --- include/linux/bpf_verifier.h | 6 +-- kernel/bpf/fixups.c | 17 ++++--- kernel/bpf/liveness.c | 14 ++++++ kernel/bpf/verifier.c | 105 +------------------------------------------ 4 files changed, 27 insertions(+), 115 deletions(-) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index d4bffed94e0a..66f506535cbd 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -162,11 +162,6 @@ struct bpf_reg_state { * pointing to bpf_func_state. */ u32 frameno; - /* Tracks subreg definition. The stored value is the insn_idx of the - * writing insn. This is safe because subreg_def is used before any insn - * patching which only happens after main verification finished. - */ - s32 subreg_def; /* if (!precise && SCALAR_VALUE) min/max/tnum don't affect safety */ bool precise; }; @@ -1657,5 +1652,6 @@ int bpf_convert_ctx_accesses(struct bpf_verifier_env *env); int bpf_jit_subprogs(struct bpf_verifier_env *env); int bpf_fixup_call_args(struct bpf_verifier_env *env); int bpf_do_misc_fixups(struct bpf_verifier_env *env); +int bpf_insn_def32(struct bpf_insn *insn); #endif /* _LINUX_BPF_VERIFIER_H */ diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c index a0bddada7964..6125598d16a8 100644 --- a/kernel/bpf/fixups.c +++ b/kernel/bpf/fixups.c @@ -44,15 +44,18 @@ static int insn_def_regno(const struct bpf_insn *insn) } } -/* Return TRUE if INSN has defined any 32-bit value explicitly. */ -static bool insn_has_def32(struct bpf_insn *insn) +/* + * Return the 32-bit subregister defined by INSN, or -1 if INSN does not + * explicitly define a 32-bit value. + */ +int bpf_insn_def32(struct bpf_insn *insn) { int dst_reg = insn_def_regno(insn); - if (dst_reg == -1) - return false; + if (dst_reg < 0 || bpf_is_reg64(insn, dst_reg, NULL, DST_OP)) + return -1; - return !bpf_is_reg64(insn, dst_reg, NULL, DST_OP); + return dst_reg; } static int kfunc_desc_cmp_by_imm_off(const void *a, const void *b) @@ -169,7 +172,7 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env, * (cnt == 1) is taken or not. There is no guarantee INSN at OFF is the * original insn at old prog. */ - data[off].zext_dst = insn_has_def32(insn + off + cnt - 1); + data[off].zext_dst = bpf_insn_def32(insn + off + cnt - 1) >= 0; if (cnt == 1) return; @@ -181,7 +184,7 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env, for (i = off; i < off + cnt - 1; i++) { /* Expand insni[off]'s seen count to the patched range. */ data[i].seen = old_seen; - data[i].zext_dst = insn_has_def32(insn + i); + data[i].zext_dst = bpf_insn_def32(insn + i) >= 0; } /* diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c index c996b8a03e79..ea002550671a 100644 --- a/kernel/bpf/liveness.c +++ b/kernel/bpf/liveness.c @@ -2258,6 +2258,7 @@ int bpf_compute_live_registers(struct bpf_verifier_env *env) struct bpf_insn *insns = env->prog->insnsi; struct insn_live_regs *state; int insn_cnt = env->prog->len; + u64 pos, insn_pos; int err = 0, i, j; bool changed; @@ -2317,9 +2318,18 @@ int bpf_compute_live_registers(struct bpf_verifier_env *env) } for (i = 0; i < insn_cnt; ++i) { + int def32 = bpf_insn_def32(&insns[i]); + u32 out = state[i].out; u32 in = state[i].in; insn_aux[i].live_regs_before = mask_lo(in) | mask_hi(in); + /* + * On architectures where 32-bit operations do not reset upper halves + * of the registers, the verifier needs to zero extend a destination + * register if an instruction defines a 32-bit subregister and the + * upper half of that register is alive after the instruction. + */ + insn_aux[i].zext_dst = def32 >= 0 && (mask_hi(out) & BIT(def32)); } if (env->log.level & BPF_LOG_LEVEL2) { @@ -2336,7 +2346,11 @@ int bpf_compute_live_registers(struct bpf_verifier_env *env) else verbose(env, "."); verbose(env, " "); + pos = env->log.end_pos; bpf_verbose_insn(env, &insns[i]); + insn_pos = env->log.end_pos; + if (insn_aux[i].zext_dst) + verbose(env, "%*c; zext", bpf_vlog_alignment(insn_pos - pos), ' '); verbose(env, "\n"); if (bpf_is_ldimm64(&insns[i])) i++; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 8304452edc0f..0709dded013e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2129,12 +2129,9 @@ static int reg_bounds_sanity_check(struct bpf_verifier_env *env, /* Mark a register as having a completely unknown (scalar) value. */ void bpf_mark_reg_unknown_imprecise(struct bpf_reg_state *reg) { - s32 subreg_def = reg->subreg_def; - memset(reg, 0, sizeof(*reg)); reg->type = SCALAR_VALUE; reg->var_off = tnum_unknown; - reg->subreg_def = subreg_def; __mark_reg_unbounded(reg); } @@ -2210,7 +2207,6 @@ static int mark_btf_ld_reg(struct bpf_verifier_env *env, } } -#define DEF_NOT_SUBREG (0) static void init_reg_state(struct bpf_verifier_env *env, struct bpf_func_state *state) { @@ -2219,7 +2215,6 @@ static void init_reg_state(struct bpf_verifier_env *env, for (i = 0; i < MAX_BPF_REG; i++) { bpf_mark_reg_not_init(env, ®s[i]); - regs[i].subreg_def = DEF_NOT_SUBREG; } /* frame pointer */ @@ -3122,30 +3117,14 @@ bool bpf_is_reg64(struct bpf_insn *insn, return true; } -static void mark_insn_zext(struct bpf_verifier_env *env, - struct bpf_reg_state *reg) -{ - s32 def_idx = reg->subreg_def; - - if (def_idx == DEF_NOT_SUBREG) - return; - - env->insn_aux_data[def_idx - 1].zext_dst = true; - /* The dst will be zero extended, so won't be sub-register anymore. */ - reg->subreg_def = DEF_NOT_SUBREG; -} - static int __check_reg_arg(struct bpf_verifier_env *env, struct bpf_reg_state *regs, u32 regno, enum bpf_reg_arg_type t) { - struct bpf_insn *insn = env->prog->insnsi + env->insn_idx; struct bpf_reg_state *reg; - bool rw64; mark_reg_scratched(env, regno); reg = ®s[regno]; - rw64 = bpf_is_reg64(insn, regno, reg, t); if (t == SRC_OP) { /* check whether register used as source operand can be read */ if (reg->type == NOT_INIT) { @@ -3156,9 +3135,6 @@ static int __check_reg_arg(struct bpf_verifier_env *env, struct bpf_reg_state *r if (regno == BPF_REG_FP) return 0; - if (rw64) - mark_insn_zext(env, reg); - return 0; } else { /* check whether register used as dest operand can be written to */ @@ -3166,7 +3142,6 @@ static int __check_reg_arg(struct bpf_verifier_env *env, struct bpf_reg_state *r verbose(env, "frame pointer is read only\n"); return -EACCES; } - reg->subreg_def = rw64 ? DEF_NOT_SUBREG : env->insn_idx + 1; if (t == DST_OP) mark_reg_unknown(env, regs, regno); } @@ -3822,11 +3797,6 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env, if (size <= spill_size && bpf_stack_narrow_access_ok(off, size, spill_size)) { - /* The earlier check_reg_arg() has decided the - * subreg_def for this insn. Save it first. - */ - s32 subreg_def = state->regs[dst_regno].subreg_def; - if (env->bpf_capable && size == 4 && spill_size == 4 && get_reg_width(reg) <= 32) /* Ensure stack slot has an ID to build a relation @@ -3834,7 +3804,6 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env, */ assign_scalar_id_before_mov(env, reg); state->regs[dst_regno] = *reg; - state->regs[dst_regno].subreg_def = subreg_def; /* Break the relation on a narrowing fill. * coerce_reg_to_size will adjust the boundaries. @@ -6271,12 +6240,6 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, struct b } else { mark_reg_known_zero(env, regs, value_regno); - /* A load of ctx field could have different - * actual load size with the one encoded in the - * insn. When the dst is PTR, it is for sure not - * a sub-register. - */ - regs[value_regno].subreg_def = DEF_NOT_SUBREG; if (base_type(info.reg_type) == PTR_TO_BTF_ID) { regs[value_regno].btf = info.btf; regs[value_regno].btf_id = info.btf_id; @@ -7380,10 +7343,6 @@ static int process_dynptr_func(struct bpf_verifier_env *env, struct bpf_reg_stat if (spi < 0) return spi; - /* - * For CONST_PTR_TO_DYNPTR, reg is already scratched by check_reg_arg - * in check_helper_call and mark_btf_func_reg_size in check_kfunc_call. - */ mark_stack_slots_scratched(env, spi, BPF_DYNPTR_NR_SLOTS); reg = &state->stack[spi].spilled_ptr; @@ -9463,7 +9422,6 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn, /* All non-void global functions return a 64-bit SCALAR_VALUE. */ if (!subprog_returns_void(env, subprog)) { mark_reg_unknown(env, caller->regs, BPF_REG_0); - caller->regs[BPF_REG_0].subreg_def = DEF_NOT_SUBREG; } if (env->subprog_info[subprog].might_throw) { @@ -10482,9 +10440,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn } invalidate_outgoing_stack_args(env, cur_func(env)); - /* helper call returns 64-bit value. */ - regs[BPF_REG_0].subreg_def = DEF_NOT_SUBREG; - /* update return register (already marked as written above) */ ret_type = fn->ret_type; ret_flag = type_flag(ret_type); @@ -10722,30 +10677,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn return 0; } -/* mark_btf_func_reg_size() is used when the reg size is determined by - * the BTF func_proto's return value size and argument. - */ -static void __mark_btf_func_reg_size(struct bpf_verifier_env *env, struct bpf_reg_state *regs, - u32 regno, size_t reg_size) -{ - struct bpf_reg_state *reg = ®s[regno]; - - if (regno == BPF_REG_0) { - /* Function return value */ - reg->subreg_def = reg_size == sizeof(u64) ? - DEF_NOT_SUBREG : env->insn_idx + 1; - } else if (reg_size == sizeof(u64)) { - /* Function argument */ - mark_insn_zext(env, reg); - } -} - -static void mark_btf_func_reg_size(struct bpf_verifier_env *env, u32 regno, - size_t reg_size) -{ - return __mark_btf_func_reg_size(env, cur_regs(env), regno, reg_size); -} - static bool is_kfunc_acquire(struct bpf_call_arg_meta *meta) { return meta->kfunc_flags & KF_ACQUIRE; @@ -12974,7 +12905,6 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, struct bpf_call_arg_meta meta; struct bpf_insn_aux_data *insn_aux; int err, insn_idx = *insn_idx_p; - const struct btf_param *args; u32 i, nargs, ptr_type_id; struct btf *desc_btf; int id; @@ -13018,7 +12948,6 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, verbose(env, "failed to mark s32 range for retval in forked state for lock\n"); return err; } - __mark_btf_func_reg_size(env, regs, BPF_REG_0, sizeof(u32)); } else if (!insn->off && insn->imm == special_kfunc_list[KF___bpf_trap]) { verbose(env, "unexpected __bpf_trap() due to uninitialized variable?\n"); return -EFAULT; @@ -13168,7 +13097,6 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, u32 regno = caller_saved[i]; bpf_mark_reg_not_init(env, ®s[regno]); - regs[regno].subreg_def = DEF_NOT_SUBREG; } invalidate_outgoing_stack_args(env, cur_func(env)); @@ -13190,7 +13118,6 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, if (meta.btf == btf_vmlinux && (meta.func_id == special_kfunc_list[KF_bpf_res_spin_lock] || meta.func_id == special_kfunc_list[KF_bpf_res_spin_lock_irqsave])) __mark_reg_const_zero(env, ®s[BPF_REG_0]); - mark_btf_func_reg_size(env, BPF_REG_0, t->size); } else if (btf_type_is_ptr(t)) { ptr_type = btf_type_skip_modifiers(desc_btf, t->type, &ptr_type_id); err = check_special_kfunc(env, &meta, regs, insn_aux, ptr_type, desc_btf); @@ -13281,7 +13208,6 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, /* For mark_ptr_or_null_reg, see 93c230e3f5bd6 */ regs[BPF_REG_0].id = ++env->id_gen; } - mark_btf_func_reg_size(env, BPF_REG_0, sizeof(void *)); if (is_kfunc_acquire(&meta)) { id = acquire_reference(env, insn_idx, 0); if (id < 0) @@ -13318,18 +13244,6 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, caller_info->stack_arg_cnt = stack_arg_cnt; } - args = (const struct btf_param *)(meta.func_proto + 1); - for (i = 0; i < min_t(int, nargs, MAX_BPF_FUNC_REG_ARGS); i++) { - u32 regno = i + 1; - - t = btf_type_skip_modifiers(desc_btf, args[i].type, NULL); - if (btf_type_is_ptr(t)) - mark_btf_func_reg_size(env, regno, sizeof(void *)); - else - /* scalar. ensured by check_kfunc_args() */ - mark_btf_func_reg_size(env, regno, t->size); - } - if (bpf_is_iter_next_kfunc(&meta)) { err = process_iter_next_call(env, insn_idx, &meta); if (err) @@ -14823,8 +14737,6 @@ static int adjust_reg_min_max_vals(struct bpf_verifier_env *env, if (dst_reg->type != PTR_TO_ARENA) *dst_reg = *src_reg; - dst_reg->subreg_def = env->insn_idx + 1; - if (BPF_CLASS(insn->code) == BPF_ALU64) /* * 32-bit operations zero upper bits automatically. @@ -15028,8 +14940,6 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn) mark_reg_unknown(env, regs, insn->dst_reg); if (insn->imm == 1) { /* cast from as(1) to as(0) */ dst_reg->type = PTR_TO_ARENA; - /* PTR_TO_ARENA is 32-bit */ - dst_reg->subreg_def = env->insn_idx + 1; } } else if (insn->off == 0) { /* case: R1 = R2 @@ -15037,7 +14947,6 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn) */ assign_scalar_id_before_mov(env, src_reg); *dst_reg = *src_reg; - dst_reg->subreg_def = DEF_NOT_SUBREG; } else { /* case: R1 = (s8, s16 s32)R2 */ if (is_pointer_value(env, insn->src_reg)) { @@ -15055,7 +14964,6 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn) if (!no_sext) clear_scalar_id(dst_reg); coerce_reg_to_size_sx(dst_reg, insn->off >> 3); - dst_reg->subreg_def = DEF_NOT_SUBREG; } else { mark_reg_unknown(env, regs, insn->dst_reg); } @@ -15080,7 +14988,6 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn) */ if (!is_src_reg_u32) clear_scalar_id(dst_reg); - dst_reg->subreg_def = env->insn_idx + 1; } else { /* case: W1 = (s8, s16)W2 */ bool no_sext = reg_umax(src_reg) < (1ULL << (insn->off - 1)); @@ -15090,7 +14997,6 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn) *dst_reg = *src_reg; if (!no_sext) clear_scalar_id(dst_reg); - dst_reg->subreg_def = env->insn_idx + 1; coerce_subreg_to_size_sx(dst_reg, insn->off >> 3); } } else { @@ -15959,12 +15865,8 @@ static void sync_linked_regs(struct bpf_verifier_env *env, struct bpf_verifier_s continue; if ((!(reg->id & BPF_ADD_CONST) && !(known_reg->id & BPF_ADD_CONST)) || reg->delta == known_reg->delta) { - s32 saved_subreg_def = reg->subreg_def; - *reg = *known_reg; - reg->subreg_def = saved_subreg_def; } else { - s32 saved_subreg_def = reg->subreg_def; s32 saved_off = reg->delta; u32 saved_id = reg->id; @@ -15974,12 +15876,11 @@ static void sync_linked_regs(struct bpf_verifier_env *env, struct bpf_verifier_s /* reg = known_reg; reg += delta */ *reg = *known_reg; /* - * Must preserve off, id and subreg_def flag, - * otherwise another sync_linked_regs() will be incorrect. + * Must preserve off and id, otherwise another sync_linked_regs() + * will be incorrect. */ reg->delta = saved_off; reg->id = saved_id; - reg->subreg_def = saved_subreg_def; scalar32_min_max_add(reg, &fake_reg); scalar_min_max_add(reg, &fake_reg); @@ -16414,8 +16315,6 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn) * Already marked as written above. */ mark_reg_unknown(env, regs, BPF_REG_0); - /* ld_abs load up to 32-bit skb data. */ - regs[BPF_REG_0].subreg_def = env->insn_idx + 1; /* * See bpf_gen_ld_abs() which emits a hidden BPF_EXIT with r0=0 * which must be explored by the verifier when in a subprog. -- 2.55.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 3/5] bpf: infer zext_dst based on static register liveness analysis 2026-07-31 19:07 ` [PATCH bpf-next 3/5] bpf: infer zext_dst based on static register liveness analysis Eduard Zingerman @ 2026-07-31 19:26 ` sashiko-bot 2026-07-31 21:17 ` Eduard Zingerman 0 siblings, 1 reply; 13+ messages in thread From: sashiko-bot @ 2026-07-31 19:26 UTC (permalink / raw) To: Eduard Zingerman; +Cc: bpf Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] Incorrect static liveness tracking for 64-bit conditional jumps leads to missing zero-extensions (`zext_dst`), resulting in verifier/runtime state divergence and potential privilege escalation. -- --- Patch [3]: [PATCH bpf-next 3/5] bpf: infer zext_dst based on static register liveness analysis --- commit ab8592d6a434dbfed0cbb0bd067d9032b308db8d Author: Eduard Zingerman <eddyz87@gmail.com> bpf: infer zext_dst based on static register liveness analysis This commit replaces the dynamic subreg_def based zero-extension tracking with static liveness analysis to correctly infer zext_dst and prevent missing zero-extensions on architectures like s390x. Link: https://lore.kernel.org/bpf/CAGKGUv=sOuqQtA1Ub-5JXfA4FPosJFYKAQE4B79cK+P1erxqtg@mail.gmail.com/ > diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c > index c996b8a03e792..ea002550671a8 100644 > --- a/kernel/bpf/liveness.c > +++ b/kernel/bpf/liveness.c > @@ -2317,9 +2318,18 @@ int bpf_compute_live_registers(struct bpf_verifier_env *env) > } > > for (i = 0; i < insn_cnt; ++i) { > + int def32 = bpf_insn_def32(&insns[i]); > + u32 out = state[i].out; > u32 in = state[i].in; > > insn_aux[i].live_regs_before = mask_lo(in) | mask_hi(in); > + /* > + * On architectures where 32-bit operations do not reset upper halves > + * of the registers, the verifier needs to zero extend a destination > + * register if an instruction defines a 32-bit subregister and the > + * upper half of that register is alive after the instruction. > + */ > + insn_aux[i].zext_dst = def32 >= 0 && (mask_hi(out) & BIT(def32)); [Severity: Critical] Does this code correctly evaluate liveness for 64-bit conditional jumps? The new zext_dst logic relies on mask_hi(out) accurately reflecting if the upper half of a register is alive. However, looking at compute_insn_live_regs() in kernel/bpf/liveness.c for conditional jumps: const u8 class = BPF_CLASS(insn->code); const u8 code = BPF_OP(insn->code); ... case BPF_JMP: case BPF_JMP32: switch (code) { ... default: def = 0; if (BPF_SRC(insn->code) == BPF_K) use = dst; else use = dst | (code == BPF_JMP32 ? src : src32); } Can the code variable ever equal BPF_JMP32? BPF_OP() extracts the operation code (which is a multiple of 16), while BPF_JMP32 is a class (0x06). This means the check code == BPF_JMP32 evaluates to false, and the source register usage always falls back to src32, even for 64-bit jumps (BPF_JMP). If a 32-bit operation defines a register, and it is followed by a 64-bit conditional jump using that register as a source with no other uses of the upper 32 bits, the upper half won't be marked as alive. As a result, zext_dst wouldn't be set, which could lead to the JIT skipping the mandatory zero extension. This would cause the 64-bit jump to compare against uninitialized garbage in the upper bits, potentially allowing out-of-bounds access. Should this check use class == BPF_JMP32 instead? > } -- Sashiko AI review · https://sashiko.dev/#/patchset/20260731-static-zext-v1-0-98a4dc73e94b@gmail.com?part=3 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 3/5] bpf: infer zext_dst based on static register liveness analysis 2026-07-31 19:26 ` sashiko-bot @ 2026-07-31 21:17 ` Eduard Zingerman 0 siblings, 0 replies; 13+ messages in thread From: Eduard Zingerman @ 2026-07-31 21:17 UTC (permalink / raw) To: sashiko-reviews; +Cc: bpf On Fri, 2026-07-31 at 19:26 +0000, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: > - [Critical] Incorrect static liveness tracking for 64-bit conditional jumps leads to missing zero-extensions (`zext_dst`), resulting in verifier/runtime state divergence and potential privilege escalation. > -- > > --- Patch [3]: [PATCH bpf-next 3/5] bpf: infer zext_dst based on static register liveness analysis --- > commit ab8592d6a434dbfed0cbb0bd067d9032b308db8d > Author: Eduard Zingerman <eddyz87@gmail.com> > > bpf: infer zext_dst based on static register liveness analysis > > This commit replaces the dynamic subreg_def based zero-extension > tracking with static liveness analysis to correctly infer zext_dst > and prevent missing zero-extensions on architectures like s390x. > > Link: https://lore.kernel.org/bpf/CAGKGUv=sOuqQtA1Ub-5JXfA4FPosJFYKAQE4B79cK+P1erxqtg@mail.gmail.com/ > > > diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c > > index c996b8a03e792..ea002550671a8 100644 > > --- a/kernel/bpf/liveness.c > > +++ b/kernel/bpf/liveness.c > > @@ -2317,9 +2318,18 @@ int bpf_compute_live_registers(struct bpf_verifier_env *env) > > } > > > > for (i = 0; i < insn_cnt; ++i) { > > + int def32 = bpf_insn_def32(&insns[i]); > > + u32 out = state[i].out; > > u32 in = state[i].in; > > > > insn_aux[i].live_regs_before = mask_lo(in) | mask_hi(in); > > + /* > > + * On architectures where 32-bit operations do not reset upper halves > > + * of the registers, the verifier needs to zero extend a destination > > + * register if an instruction defines a 32-bit subregister and the > > + * upper half of that register is alive after the instruction. > > + */ > > + insn_aux[i].zext_dst = def32 >= 0 && (mask_hi(out) & BIT(def32)); > > [Severity: Critical] > Does this code correctly evaluate liveness for 64-bit conditional jumps? > > The new zext_dst logic relies on mask_hi(out) accurately reflecting if the > upper half of a register is alive. > > However, looking at compute_insn_live_regs() in kernel/bpf/liveness.c for > conditional jumps: > > const u8 class = BPF_CLASS(insn->code); > const u8 code = BPF_OP(insn->code); > ... > case BPF_JMP: > case BPF_JMP32: > switch (code) { > ... > default: > def = 0; > if (BPF_SRC(insn->code) == BPF_K) > use = dst; > else > use = dst | (code == BPF_JMP32 ? src : src32); > } > > Can the code variable ever equal BPF_JMP32? > > BPF_OP() extracts the operation code (which is a multiple of 16), while > BPF_JMP32 is a class (0x06). This means the check code == BPF_JMP32 evaluates > to false, and the source register usage always falls back to src32, even for > 64-bit jumps (BPF_JMP). > > If a 32-bit operation defines a register, and it is followed by a 64-bit > conditional jump using that register as a source with no other uses of the > upper 32 bits, the upper half won't be marked as alive. > > As a result, zext_dst wouldn't be set, which could lead to the JIT skipping > the mandatory zero extension. This would cause the 64-bit jump to compare > against uninitialized garbage in the upper bits, potentially allowing > out-of-bounds access. > > Should this check use class == BPF_JMP32 instead? > > > } This is the same (real) issue as reported for the previous patch. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next 4/5] bpf: simplify the bpf_is_reg64() signature 2026-07-31 19:07 [PATCH bpf-next 0/5] bpf: infer zext_dst based on static register liveness analysis Eduard Zingerman ` (2 preceding siblings ...) 2026-07-31 19:07 ` [PATCH bpf-next 3/5] bpf: infer zext_dst based on static register liveness analysis Eduard Zingerman @ 2026-07-31 19:07 ` Eduard Zingerman 2026-07-31 19:18 ` sashiko-bot 2026-07-31 20:09 ` bot+bpf-ci 2026-07-31 19:07 ` [PATCH bpf-next 5/5] selftests/bpf: verify zext_dst annotations for various instructions Eduard Zingerman 4 siblings, 2 replies; 13+ messages in thread From: Eduard Zingerman @ 2026-07-31 19:07 UTC (permalink / raw) To: bpf, ast, andrii Cc: daniel, martin.lau, kernel-team, yonghong.song, eddyz87, memxor, iii, gimm78064 After the previous commit this function is only used in a context where destination register's property is queried. Hence, simplify the function by removing the 'regno' and 't' parameters. Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> --- include/linux/bpf_verifier.h | 2 +- kernel/bpf/fixups.c | 4 ++-- kernel/bpf/verifier.c | 39 +++++++-------------------------------- 3 files changed, 10 insertions(+), 35 deletions(-) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 66f506535cbd..321ebd308d38 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -1628,7 +1628,7 @@ struct bpf_kfunc_desc_tab { }; /* Functions exported from verifier.c, used by fixups.c */ -bool bpf_is_reg64(struct bpf_insn *insn, u32 regno, struct bpf_reg_state *reg, enum bpf_reg_arg_type t); +bool bpf_is_reg64(struct bpf_insn *insn); void bpf_clear_insn_aux_data(struct bpf_verifier_env *env, int start, int len); void bpf_mark_subprog_exc_cb(struct bpf_verifier_env *env, int subprog); bool bpf_allow_tail_call_in_subprogs(struct bpf_verifier_env *env); diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c index 6125598d16a8..73ab95573f60 100644 --- a/kernel/bpf/fixups.c +++ b/kernel/bpf/fixups.c @@ -52,7 +52,7 @@ int bpf_insn_def32(struct bpf_insn *insn) { int dst_reg = insn_def_regno(insn); - if (dst_reg < 0 || bpf_is_reg64(insn, dst_reg, NULL, DST_OP)) + if (dst_reg < 0 || bpf_is_reg64(insn)) return -1; return dst_reg; @@ -623,7 +623,7 @@ int bpf_opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, * BPF_STX + SRC_OP, so it is safe to pass NULL * here. */ - if (bpf_is_reg64(&insn, load_reg, NULL, DST_OP)) { + if (bpf_is_reg64(&insn)) { if (class == BPF_LD && BPF_MODE(code) == BPF_IMM) i++; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 0709dded013e..d5b6fef1ad89 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3028,11 +3028,10 @@ static void mark_stack_slots_scratched(struct bpf_verifier_env *env, } /* This function is supposed to be used by the following 32-bit optimization - * code only. It returns TRUE if the source or destination register operates - * on 64-bit, otherwise return FALSE. + * code only. It returns TRUE if the destination register operates on 64-bit, + * otherwise return FALSE. */ -bool bpf_is_reg64(struct bpf_insn *insn, - u32 regno, struct bpf_reg_state *reg, enum bpf_reg_arg_type t) +bool bpf_is_reg64(struct bpf_insn *insn) { u8 code, class, op; @@ -3053,11 +3052,6 @@ bool bpf_is_reg64(struct bpf_insn *insn, */ if (insn->src_reg == BPF_PSEUDO_CALL) return false; - /* Helper call will reach here because of arg type - * check, conservatively return TRUE. - */ - if (t == SRC_OP) - return true; return false; } @@ -3073,22 +3067,11 @@ bool bpf_is_reg64(struct bpf_insn *insn, if (class == BPF_ALU || class == BPF_JMP32) return false; - if (class == BPF_LDX) { - if (t != SRC_OP) - return BPF_SIZE(code) == BPF_DW || BPF_MODE(code) == BPF_MEMSX; - /* LDX source must be ptr. */ - return true; - } + if (class == BPF_LDX) + return BPF_SIZE(code) == BPF_DW || BPF_MODE(code) == BPF_MEMSX; - if (class == BPF_STX) { - /* BPF_STX (including atomic variants) has one or more source - * operands, one of which is a ptr. Check whether the caller is - * asking about it. - */ - if (t == SRC_OP && reg->type != SCALAR_VALUE) - return true; + if (class == BPF_STX) return BPF_SIZE(code) == BPF_DW; - } if (class == BPF_LD) { u8 mode = BPF_MODE(code); @@ -3098,15 +3081,7 @@ bool bpf_is_reg64(struct bpf_insn *insn, return true; /* Both LD_IND and LD_ABS return 32-bit data. */ - if (t != SRC_OP) - return false; - - /* Implicit ctx ptr. */ - if (regno == BPF_REG_6) - return true; - - /* Explicit source could be any width. */ - return true; + return false; } if (class == BPF_ST) -- 2.55.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 4/5] bpf: simplify the bpf_is_reg64() signature 2026-07-31 19:07 ` [PATCH bpf-next 4/5] bpf: simplify the bpf_is_reg64() signature Eduard Zingerman @ 2026-07-31 19:18 ` sashiko-bot 2026-07-31 20:09 ` bot+bpf-ci 1 sibling, 0 replies; 13+ messages in thread From: sashiko-bot @ 2026-07-31 19:18 UTC (permalink / raw) To: Eduard Zingerman; +Cc: bpf Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Low] A comment explicitly referring to removed arguments of bpf_is_reg64() was left intact. - [Low] The handling of BPF_JMP, BPF_JMP32, and BPF_ST classes inside bpf_is_reg64() is now completely dead code and contains misleading comments. -- --- Patch [4]: [PATCH bpf-next 4/5] bpf: simplify the bpf_is_reg64() signature --- commit ed53634b5ab05a53b4f5cd2fc3aac98c66a8ccd4 Author: Eduard Zingerman <eddyz87@gmail.com> bpf: simplify the bpf_is_reg64() signature After the previous commit this function is only used in a context where destination register's property is queried. Hence, simplify the function by removing the 'regno' and 't' parameters. > diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c > index 6125598d16a84..73ab95573f609 100644 > --- a/kernel/bpf/fixups.c > +++ b/kernel/bpf/fixups.c [ ... ] > @@ -623,7 +623,7 @@ int bpf_opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, > * BPF_STX + SRC_OP, so it is safe to pass NULL > * here. > */ > - if (bpf_is_reg64(&insn, load_reg, NULL, DST_OP)) { > + if (bpf_is_reg64(&insn)) { [Severity: Low] Since the parameters have been removed from bpf_is_reg64(), should we also remove or update this comment referring to the "fourth one"? > if (class == BPF_LD && > BPF_MODE(code) == BPF_IMM) > i++; > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 0709dded013ea..d5b6fef1ad897 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -3028,11 +3028,10 @@ static void mark_stack_slots_scratched(struct bpf_verifier_env *env, > } > > /* This function is supposed to be used by the following 32-bit optimization > - * code only. It returns TRUE if the source or destination register operates > - * on 64-bit, otherwise return FALSE. > + * code only. It returns TRUE if the destination register operates on 64-bit, > + * otherwise return FALSE. > */ > -bool bpf_is_reg64(struct bpf_insn *insn, > - u32 regno, struct bpf_reg_state *reg, enum bpf_reg_arg_type t) > +bool bpf_is_reg64(struct bpf_insn *insn) [Severity: Low] Are the branches handling BPF_JMP, BPF_JMP32, and BPF_ST inside this function now dead code? The callers in bpf_insn_def32() and bpf_opt_subreg_zext_lo32_rnd_hi32() only evaluate bpf_is_reg64() if insn_def_regno() returns a value >= 0. Since insn_def_regno() returns -1 for BPF_JMP, BPF_JMP32, and BPF_ST, execution should never reach those branches here anymore. This leaves behind unreachable code and makes the corresponding comments (like "BPF_EXIT for 'main' will reach here") inaccurate. > { > u8 code, class, op; > -- Sashiko AI review · https://sashiko.dev/#/patchset/20260731-static-zext-v1-0-98a4dc73e94b@gmail.com?part=4 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 4/5] bpf: simplify the bpf_is_reg64() signature 2026-07-31 19:07 ` [PATCH bpf-next 4/5] bpf: simplify the bpf_is_reg64() signature Eduard Zingerman 2026-07-31 19:18 ` sashiko-bot @ 2026-07-31 20:09 ` bot+bpf-ci 1 sibling, 0 replies; 13+ messages in thread From: bot+bpf-ci @ 2026-07-31 20:09 UTC (permalink / raw) To: eddyz87, bpf, ast, andrii Cc: daniel, martin.lau, kernel-team, yonghong.song, eddyz87, memxor, iii, gimm78064, ast, andrii, daniel, martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai [-- Attachment #1: Type: text/plain, Size: 1513 bytes --] > bpf: simplify the bpf_is_reg64() signature > > After the previous commit this function is only used in a context > where destination register's property is queried. > Hence, simplify the function by removing the 'regno' and 't' > parameters. The changelog appears to undercount the removed parameters. Looking at the actual signature change: include/linux/bpf_verifier.h: -bool bpf_is_reg64(struct bpf_insn *insn, u32 regno, struct bpf_reg_state *reg, enum bpf_reg_arg_type t); +bool bpf_is_reg64(struct bpf_insn *insn); Three parameters are dropped: regno, reg, and t. The message mentions only 'regno' and 't', omitting 'struct bpf_reg_state *reg'. The reg parameter isn't incidental: it was the only pointer argument, both call sites passed NULL for it, and its removal retires the contract that the BPF_STX SRC_OP arm relied on. The fixups.c comment at line 622 explicitly documented this: kernel/bpf/fixups.c: /* NOTE: arg "reg" (the fourth one) is only used for * BPF_STX + SRC_OP, so it is safe to pass NULL * here. */ if (bpf_is_reg64(&insn)) { Could the message say 'removing the regno, reg and t parameters' (or 'removing all parameters except insn') so readers following the series can account for all three removals? --- 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/30658911996 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next 5/5] selftests/bpf: verify zext_dst annotations for various instructions 2026-07-31 19:07 [PATCH bpf-next 0/5] bpf: infer zext_dst based on static register liveness analysis Eduard Zingerman ` (3 preceding siblings ...) 2026-07-31 19:07 ` [PATCH bpf-next 4/5] bpf: simplify the bpf_is_reg64() signature Eduard Zingerman @ 2026-07-31 19:07 ` Eduard Zingerman 4 siblings, 0 replies; 13+ messages in thread From: Eduard Zingerman @ 2026-07-31 19:07 UTC (permalink / raw) To: bpf, ast, andrii Cc: daniel, martin.lau, kernel-team, yonghong.song, eddyz87, memxor, iii, gimm78064 Includes the following test cases: - a test showing that zero extension flags do not propagate through state pruning in the unpatched kernel. - a 32-bit ALU result read as 64-bit (zext) and read only as 32-bit (no zext); - a 64-bit MOV (never zext'ed); - a narrow (32-bit) BPF_LDX load whose result is read as 64-bit; - 32-bit atomic fetch_add and cmpxchg whose result is read as 64-bit; - a CFG case where a 32-bit definition's upper half is used only on one of two branches; - no zext for dead registers; - LD_ABS defines only lower 32 bits, hence needs zext when the result is used as 64-bits; - Helper and subprogram parameters are considered to use full 64 bits; - kfunc parameters are read according to their BTF type width. Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> --- tools/testing/selftests/bpf/prog_tests/verifier.c | 2 + tools/testing/selftests/bpf/progs/verifier_zext.c | 260 ++++++++++++++++++++++ 2 files changed, 262 insertions(+) diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c index be97f6887f0e..fba562c81969 100644 --- a/tools/testing/selftests/bpf/prog_tests/verifier.c +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c @@ -124,6 +124,7 @@ #include "verifier_jit_inline.skel.h" #include "irq.skel.h" #include "verifier_ctx_ptr_param.skel.h" +#include "verifier_zext.skel.h" #define MAX_ENTRIES 11 @@ -277,6 +278,7 @@ void test_irq(void) { RUN(irq); } void test_verifier_mtu(void) { RUN(verifier_mtu); } void test_verifier_jit_inline(void) { RUN(verifier_jit_inline); } void test_verifier_ctx_ptr_param(void) { RUN(verifier_ctx_ptr_param); } +void test_verifier_zext(void) { RUN_TESTS(verifier_zext); } static int init_test_val_map(struct bpf_object *obj, char *map_name) { diff --git a/tools/testing/selftests/bpf/progs/verifier_zext.c b/tools/testing/selftests/bpf/progs/verifier_zext.c new file mode 100644 index 000000000000..b4c82a31e668 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/verifier_zext.c @@ -0,0 +1,260 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> +#include "../../../include/linux/filter.h" +#include "bpf_misc.h" + +SEC("socket") +__flag(BPF_F_TEST_STATE_FREQ) +__flag(BPF_F_TEST_RND_HI32) +__success __retval(0) +__naked void zext_lost_across_checkpoint(void) +{ + asm volatile (" \ + call %[bpf_ktime_get_ns]; \ + r8 = r0; \ + r6 = 0xdeadbeefcafebabe ll; /* inject some value for r6's upper half */ \ + if r8 != 0 goto 1f; /* fall-through cached first, branch pruned */ \ + r6 = 32; /* full 64-bit def */ \ + goto 2f; \ +1: \ + w6 = 32; /* 32-bit def, zext mark lost */ \ +2: \ + r0 = r6; /* buggy verifier believed upper 32 bits are 0 */ \ + /* and thus did not zero extended w6 = 32. */ \ + r0 >>= 32; \ + exit; \ +" : + : __imm(bpf_ktime_get_ns) + : __clobber_all); +} + +/* 32-bit ALU result read as 64-bit -> zext */ +SEC("socket") +__success __log_level(2) +__msg("w1 = w0{{ +}}; zext") +__naked void zext_alu32_hi_used(void) +{ + asm volatile (" \ + call %[bpf_get_prandom_u32]; \ + w1 = w0; \ + r0 = r1; \ + exit; \ +" : + : __imm(bpf_get_prandom_u32) + : __clobber_all); +} + +/* 32-bit ALU result read only as 32-bit -> no zext */ +SEC("socket") +__success __log_level(2) +__not_msg("w1 = w0{{.*}}; zext") +__not_msg("w2 = w1{{.*}}; zext") +__naked void no_zext_alu32_hi_unused(void) +{ + asm volatile (" \ + call %[bpf_get_prandom_u32]; \ + w1 = w0; \ + w2 = w1; \ + r0 = 0; \ + exit; \ +" : + : __imm(bpf_get_prandom_u32) + : __clobber_all); +} + +/* 64-bit definition is never zero extended */ +SEC("socket") +__success __log_level(2) +__not_msg("r1 = r0{{.*}}; zext") +__naked void no_zext_mov64(void) +{ + asm volatile (" \ + call %[bpf_get_prandom_u32]; \ + r1 = r0; \ + r0 = r1; \ + exit; \ +" : + : __imm(bpf_get_prandom_u32) + : __clobber_all); +} + +/* Narrow load result read as 64-bit -> zext */ +SEC("socket") +__success __log_level(2) +__msg("r1 = *(u32 *)(r10 -8){{ +}}; zext") +__naked void zext_narrow_load_hi_used(void) +{ + asm volatile (" \ + r0 = 0; \ + *(u64 *)(r10 - 8) = r0; \ + r1 = *(u32 *)(r10 - 8); \ + r0 = r1; \ + exit; \ +" ::: __clobber_all); +} + +/* 32-bit atomic fetch result read as 64-bit -> zext */ +SEC("socket") +__success __log_level(2) +__msg("r1 = atomic_fetch_add((u32 *)(r10 -8), r1){{ +}}; zext") +__naked void zext_atomic_fetch32_hi_used(void) +{ + asm volatile (" \ + r1 = 0; \ + *(u64 *)(r10 - 8) = r1; \ + w1 = 1; \ + .8byte %[fetch_add32]; \ + r0 = r1; \ + exit; \ +" : + : __imm_insn(fetch_add32, + BPF_ATOMIC_OP(BPF_W, BPF_ADD | BPF_FETCH, BPF_REG_10, BPF_REG_1, -8)) + : __clobber_all); +} + +/* 32-bit atomic cmpxchg result (r0) read as 64-bit -> zext */ +SEC("socket") +__success __log_level(2) +__msg("r0 = atomic_cmpxchg((u32 *)(r10 -8), r0, r1){{ +}}; zext") +__naked void zext_cmpxchg32_hi_used(void) +{ + asm volatile (" \ + r1 = 0; \ + *(u64 *)(r10 - 8) = r1; \ + w0 = 0; \ + w1 = 1; \ + .8byte %[cmpxchg32]; \ + r2 = r0; \ + r0 = r2; \ + exit; \ +" : + : __imm_insn(cmpxchg32, + BPF_ATOMIC_OP(BPF_W, BPF_CMPXCHG, BPF_REG_10, BPF_REG_1, -8)) + : __clobber_all); +} + +/* 32-bit def before a branch, upper half used on one branch -> zext */ +SEC("socket") +__success __log_level(2) +__msg("w6 = 32{{ +}}; zext") +__naked void zext_cfg_hi_used_one_branch(void) +{ + asm volatile (" \ + call %[bpf_get_prandom_u32]; \ + w6 = 32; \ + if r0 == 0 goto l0_%=; \ + r0 = r6; \ + exit; \ +l0_%=: \ + r0 = 0; \ + exit; \ +" : + : __imm(bpf_get_prandom_u32) + : __clobber_all); +} + +/* r1's upper half is dead, so 'w1 = 1' must NOT be marked for zero extension. */ +SEC("socket") +__success __log_level(2) +__not_msg("w1 = 1{{.*}}; zext") +__naked void no_zext_other_reg_hi_used(void) +{ + asm volatile (" \ + call %[bpf_get_prandom_u32]; \ + r6 = r0; \ + r6 <<= 32; \ + w1 = 1; \ + r0 = r6; \ + exit; \ +" : + : __imm(bpf_get_prandom_u32) + : __clobber_all); +} + +/* LD_ABS defines r0; when r0 is read as 64-bit it must be zero extended */ +SEC("socket") +__success __log_level(2) +__msg("r0 = *(u8 *)skb[0]{{.*}}; zext") +__naked void zext_ld_abs_hi_used(void) +{ + asm volatile (" \ + r6 = r1; \ + r0 = *(u8 *)skb[0]; \ + r7 = r0; \ + r0 = r7; \ + exit; \ +" ::: __clobber_all); +} + +/* Helper parameters are read as 64-bit (call_use_mask() fallback) */ +SEC("socket") +__success __log_level(2) +__msg("w2 = 1{{ +}}; zext") +__naked void helper_param_read_as_64bit(void) +{ + asm volatile (" \ + r1 = r10; \ + r1 += -8; \ + w2 = 1; \ + call %[bpf_trace_printk]; \ + r0 = 0; \ + exit; \ +" : + : __imm(bpf_trace_printk) + : __clobber_all); +} + +static __used __naked int subprog_reads_arg_as_64bit(void) +{ + asm volatile (" \ + r0 = r1; \ + exit; \ +" ::: __clobber_all); +} + +/* subprogram parameters are conservatively read as 64-bit */ +SEC("socket") +__success __log_level(2) +__msg("w1 = w0{{ +}}; zext") +__naked void subprog_param_read_as_64bit(void) +{ + asm volatile (" \ + call %[bpf_get_prandom_u32]; \ + w1 = w0; \ + call subprog_reads_arg_as_64bit; \ + r0 = 0; \ + exit; \ +" : + : __imm(bpf_get_prandom_u32) + : __clobber_all); +} + +extern long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym; + +/* Force kfunc extern BTF generation. */ +int __kfunc_btf_root(void) +{ + return bpf_kfunc_call_test4(0, 0, 0, 0); +} + +/* kfunc parameters are read according to their BTF type width */ +SEC("tc") +__success __log_level(2) +__not_msg("w3 = 1{{.*}}; zext") /* int c -> read as 32-bit */ +__msg("w4 = 1{{ +}}; zext") /* long d -> read as 64-bit */ +__naked void kfunc_param_read_per_btf(void) +{ + asm volatile (" \ + w1 = 1; \ + w2 = 1; \ + w3 = 1; \ + w4 = 1; \ + call bpf_kfunc_call_test4; \ + r0 = 0; \ + exit; \ +" ::: __clobber_all); +} + +char _license[] SEC("license") = "GPL"; -- 2.55.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2026-07-31 21:17 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-31 19:07 [PATCH bpf-next 0/5] bpf: infer zext_dst based on static register liveness analysis Eduard Zingerman 2026-07-31 19:07 ` [PATCH bpf-next 1/5] bpf: do not print a newline after disassembly in bpf_verbose_insn() Eduard Zingerman 2026-07-31 20:24 ` bot+bpf-ci 2026-07-31 21:12 ` Eduard Zingerman 2026-07-31 19:07 ` [PATCH bpf-next 2/5] bpf: track upper 32-bit register halves' liveness in compute_live_registers() Eduard Zingerman 2026-07-31 19:18 ` sashiko-bot 2026-07-31 19:07 ` [PATCH bpf-next 3/5] bpf: infer zext_dst based on static register liveness analysis Eduard Zingerman 2026-07-31 19:26 ` sashiko-bot 2026-07-31 21:17 ` Eduard Zingerman 2026-07-31 19:07 ` [PATCH bpf-next 4/5] bpf: simplify the bpf_is_reg64() signature Eduard Zingerman 2026-07-31 19:18 ` sashiko-bot 2026-07-31 20:09 ` bot+bpf-ci 2026-07-31 19:07 ` [PATCH bpf-next 5/5] selftests/bpf: verify zext_dst annotations for various instructions Eduard Zingerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox