> The main instructions being targeted are BPF_ST, BPF_STX and BPF_LDX,
> but not all of them are being instrumented:
> - if the load/store instruction is in fact accessing the program stack,
> emit_kasan_check silently skips the instrumentation, as we can already
> benefit from guard pages to monitor stack accesses.
> - if the load/store instruction is a BPF_PROBE_MEM or a BPF_PROBE_ATOMIC
> instruction, we do not instrument it, as the passed address can fault
> (hence the custom fault management with BPF_PROBE_XXX instructions),
> and so the corresponding kasan check could fault as well.
This isn't a bug, but could the changelog be more precise about where
the stack-access skip happens?
The text says emit_kasan_check() "silently skips the instrumentation"
for stack accesses, but the decision is actually made in do_jit() by the
new accesses_stack_only test:
arch/x86/net/bpf_jit_comp.c:do_jit() {
...
insn_idx = i - 1 + bpf_prog->aux->subprog_start;
accesses_stack_only =
env ? !env->insn_aux_data[insn_idx].non_stack_access :
false;
...
}
which reads the verifier's per-instruction non_stack_access flag set by
mark_non_stack_access() at kernel/bpf/verifier.c:6665.
emit_kasan_check() only skips when the address register is literally
BPF_REG_FP or BPF_REG_PARAMS. The list also omits BPF_PROBE_MEM32
(arena) accesses, which are handled by separate case labels and likewise
left uninstrumented.
[ ... ]
> @@ -1914,6 +2032,10 @@
> EMIT_ENDBR();
>
> ip = image + addrs[i - 1] + (prog - temp);
> + insn_idx = i - 1 + bpf_prog->aux->subprog_start;
> + accesses_stack_only =
> + env ? !env->insn_aux_data[insn_idx].non_stack_access :
> + false;
This isn't a bug, but would a positively named predicate read more
directly here?
When env is NULL the fallback value false means "this access is not
stack-only", so every load/store takes the !accesses_stack_only path and
calls emit_kasan_check(), which immediately returns 0 because of its own
if (!env) return 0; guard at arch/x86/net/bpf_jit_comp.c:1129.
The intent (no instrumentation without a verifier env) is expressed
through a double negative plus a second guard in the callee. Would
accesses_stack_only = !env || !env->insn_aux_data[insn_idx].non_stack_access
(or a want_kasan_check predicate) make the no-env case self-evident and
drop the redundant callee check?
---
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/33766012402
