On Fri Aug 28, 2026 at 12:30 PM CEST, bot+bpf-ci wrote:
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> index 004b06785521c..412a13a664fa3 100644
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
>> @@ -725,6 +725,8 @@ struct bpf_insn_aux_data {
>> u16 const_reg_map_mask;
>> u16 const_reg_subprog_mask;
>> u32 const_reg_vals[10];
>> + /* instruction can access non-stack memory */
>> + bool non_stack_access;
>> };
>
> The new field is placed at the very end of struct bpf_insn_aux_data, pushing
> the struct from 136 to 144 bytes due to alignment, which adds 8 bytes of
> aux data per BPF instruction. For a program at BPF_COMPLEXITY_LIMIT_INSNS
> this is roughly 8 MB of extra vmalloc space for one boolean.
>
> There is already a 3-byte padding hole before orig_idx and 27 unused bits
> in the adjacent u32 bitfield group (jmp_point:1 ... indirect_target:1).
>
> Would declaring non_stack_access as a bit in the existing u32 bitfield
> group avoid the size increase?
Yes, I should have gone directly with a bit-field here. I'll update
accordingly.
> [ ... ]
>
>> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
>> index 65b441e4a3517..edcb0cbbb13de 100644
>> --- a/kernel/bpf/fixups.c
>> +++ b/kernel/bpf/fixups.c
>> @@ -208,12 +208,25 @@ static int get_callee_stack_depth(struct
>> bpf_verifier_env *env,
>> }
>> #endif
>>
>> +static bool is_mem_insn(struct bpf_insn *insn)
>> +{
>> + if (BPF_CLASS(insn->code) != BPF_ST &&
>> + BPF_CLASS(insn->code) != BPF_STX &&
>> + BPF_CLASS(insn->code) != BPF_LDX)
>> + return false;
>> +
>> + return (BPF_MODE(insn->code) == BPF_MEM ||
>> + BPF_MODE(insn->code) == BPF_MEMSX ||
>> + BPF_MODE(insn->code) == BPF_ATOMIC);
>> +}
>
> Does is_mem_insn() correctly handle the speculation barrier
> pseudo-instruction BPF_ST_NOSPEC()?
>
> BPF_NOSPEC is defined as 0xc0, the same mode value as BPF_ATOMIC (0xc0),
> and BPF_ST_NOSPEC() encodes code = BPF_ST | BPF_NOSPEC (0x02 | 0xc0 = 0xc2).
> The class test passes (BPF_ST) and the mode test passes via the
> BPF_MODE(insn->code) == BPF_ATOMIC arm, so is_mem_insn() returns true for
> an instruction that performs no memory access at all.
>
> Every Spectre v1/v4 mitigation patch in do_misc_fixups() inserts such a
> barrier through bpf_patch_insn_data(), so on any program that requires a
> nospec barrier the barrier slot is given non_stack_access = true by the
> new marking code in adjust_insn_aux_data().
>
> On x86-64 the consumer added later in the series handles case BPF_ST |
> BPF_NOSPEC in a separate switch arm that emits an LFENCE and never reaches
> the load/store instrumentation path, so there is no functional
> misbehaviour on x86 today.
>
> However, the helper's stated contract ('is this a memory instruction') is
> wrong, the flag is set on non-memory slots, and any future or other-arch
> consumer of non_stack_access that dispatches on class/mode alone will
> inherit the confusion.
>
> Should the helper exclude BPF_ST | BPF_NOSPEC explicitly, for example by
> testing insn_is_nospec() or handling the BPF_ST case separately?
True. The omission does not affect the final instrumentation, but I'll
add it for correctness.
>>
>> /* single env->prog->insni[off] instruction was replaced with the range
>> * insni[off, off + cnt). Adjust corresponding insn_aux_data by copying
>> * [0, off) and [off, end) to new locations, so the patched range stays zero
>> */
>> static void adjust_insn_aux_data(struct bpf_verifier_env *env,
>> - struct bpf_prog *new_prog, u32 off, u32 cnt)
>> + struct bpf_prog *new_prog, u32 off, u32 cnt,
>> + struct bpf_insn *original_insn)
>> {
>> struct bpf_insn_aux_data *data = env->insn_aux_data;
>> struct bpf_insn *insn = new_prog->insnsi;
>> @@ -227,8 +240,15 @@ static void adjust_insn_aux_data(struct
>> bpf_verifier_env *env,
>> */
>> data[off].zext_dst = bpf_insn_def32(new_prog, insn + off + cnt - 1) >=
>> 0;
>>
>> - if (cnt == 1)
>> + if (cnt == 1) {
>> + /*
>> + * A non-memory accessing insn could have been replaced by a
>> + * memory accessing insn, systematically mark it for non-stack
>> + * access
>> + */
>> + data[off].non_stack_access = is_mem_insn(insn + off);
>> return;
>> + }
>> prog_len = new_prog->len;
>> env->insn_aux_data_len = prog_len;
>>
>> @@ -239,8 +259,25 @@ static void adjust_insn_aux_data(struct
>> bpf_verifier_env *env,
>> /* Expand insni[off]'s seen count to the patched range. */
>> data[i].seen = old_seen;
>> data[i].zext_dst = bpf_insn_def32(new_prog, insn + i) >= 0;
>> + if (!memcmp(insn + i, original_insn, sizeof(struct bpf_insn))) {
>> + data[i].non_stack_access =
>> + data[off + cnt - 1].non_stack_access;
>> + data[off + cnt - 1].non_stack_access = false;
>> + } else if (is_mem_insn(insn + i)) {
>> + data[i].non_stack_access = true;
>> + }
>> }
>>
>> + /*
>> + * Last slot instruction could be a newly generated
>> + * BPF_ST/BPF_LDX/BPF_STX, systematically mark it for non-stack access
>> + * if it is not the original instruction, otherwise keep the
>> + * original marking
>> + */
>> + if (is_mem_insn(insn + off + cnt - 1) &&
>> + memcmp(insn + off + cnt - 1, original_insn, sizeof(struct
>> bpf_insn)))
>> + data[off + cnt - 1].non_stack_access = true;
>
> The 'inherit the original marking' logic is driven purely by memcmp() of
> the raw instruction bytes against original_insn, which makes the result
> depend on how many copies of an identical encoding a patch buffer happens
> to contain. A subsystem pattern flags this as potentially concerning:
>
> If a patch buffer legitimately contains the original instruction twice (or
> contains a newly generated instruction that happens to encode identically
> to the original, which is easy for a repeated BPF_LDX_MEM/BPF_STX_MEM with
> the same regs/offset/size), the first match consumes the marking and
> clears data[off + cnt - 1].non_stack_access. Every later identical copy
> falls into the else if (is_mem_insn()) arm and is set to true, but the
> memcmp arm is actually taken again for those slots and now copies an
> already-cleared value, silently under-marking a real non-stack access.
I am not sure to understand how multiple consecutive BPF_LDX/BPF_STX
could confuse the patching logic, as each individual instruction will
have its own patch.
>
> Conversely, a genuinely new instruction that coincidentally matches
> original_insn byte-for-byte is exempted from the 'systematically mark new
> memory insns' rule by the trailing memcmp.
>
> Neither case is reachable via a patch site in this tree (all current
> multi-instruction patches place the original instruction exactly once,
> either first or last), so this is latent fragility rather than an
> observable bug today.
I could actually be somewhere in the middle of the newly generated patch
as well, as observed in the previous revisions.
> Should the identity of an instruction be tracked by position instead of by
> content comparison, since the patch sites all know where they put the
> original?
And so no, all the patch sites do no systematically know where the
original instruction is, sometimes the concept of "original instruction"
even becomes irrelevant, depending on the inserted patch. I've tried
this "tracking by position" in previous revisions, and it has proved to
be way too invasive, imprecise, and error-prone. So this memcmp may
indeed not be bulletproof, but that's a best effort. And again, the
"risk" of missing this stack marking is about inserting an undesired
stack check: this is just a slight overhead (which may be negligeable
compared to the whole kasan instrumentation overhead), not a potential
crash.
> [ ... ]
>
> Verifier-generated instructions that provably access the program stack are
> marked as non-stack accesses, which is precisely the case the feature
> exists to exclude, and the over-marking is systematic rather than
> incidental. A subsystem pattern flags this as potentially concerning:
>
> Concrete reachable instances:
>
> (1) The ARG_PTR_TO_CTX save inserted by do_misc_fixups():
>
> kernel/bpf/fixups.c:do_misc_fixups() {
> insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_FP, BPF_REG_1,
> -subprogs[0].stack_depth);
>
> This is a frame-pointer-relative store, and since it is not the original
> instruction it is marked non_stack_access = true by the loop above.
>
> (2) The matching frame-pointer load emitted by gen_epilogue callbacks,
> for example:
>
> net/sched/bpf_qdisc.c:bpf_qdisc_gen_epilogue() {
> BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_FP, ctx_stack_off)
>
> This gets the same treatment when the epilogue is patched in, so every
> struct_ops qdisc reset/destroy program is affected.
>
> (3) bpf_jit_blind_constants() in kernel/bpf/core.c rewrites a BPF_ST |
> BPF_MEM store into {MOV AX, XOR AX, BPF_STX_MEM(...)}, so with
> bpf_jit_harden enabled every blinded stack store loses its stack-only
> marking.
>
> The commit message acknowledges over-instrumentation of 'a few memory
> accessing instructions' as an accepted trade-off, but these cases are
> trivially detectable (dst_reg/src_reg == BPF_REG_10) and, for the
> KASAN-in-JIT consumer added later in the series, they mean pointless
> instrumentation on the hottest prologue/epilogue accesses.
>
> Would filtering on BPF_REG_FP in is_mem_insn() or adjust_insn_aux_data()
> remove this class of false positives?
I can possibly skip kasan instrumentation when dst_reg == BPF_REG_FP (or
src_reg, if LDX) directly in JIT compiler, similarly to what I have
proposed for BPF_REG_PARAMS in the review on patch 4/8.
>> @@ -306,6 +343,7 @@ struct bpf_prog *bpf_patch_insn_data(struct
>> bpf_verifier_env *env, u32 off,
>> {
>> struct bpf_prog *new_prog;
>> struct bpf_insn_aux_data *new_data = NULL;
>> + struct bpf_insn original_insn;
>>
>> if (len > 1) {
>> new_data = vrealloc(env->insn_aux_data,
>> @@ -318,6 +356,7 @@ struct bpf_prog *bpf_patch_insn_data(struct
>> bpf_verifier_env *env, u32 off,
>> env->insn_aux_data = new_data;
>> }
>>
>> + memcpy(&original_insn, env->prog->insnsi + off, sizeof(struct
>> bpf_insn));
>> new_prog = bpf_patch_insn_single(env->prog, off, patch, len);
>> if (IS_ERR(new_prog)) {
>> if (PTR_ERR(new_prog) == -ERANGE)
>> @@ -326,7 +365,7 @@ struct bpf_prog *bpf_patch_insn_data(struct
>> bpf_verifier_env *env, u32 off,
>> env->insn_aux_data[off].orig_idx);
>> return NULL;
>> }
>> - adjust_insn_aux_data(env, new_prog, off, len);
>> + adjust_insn_aux_data(env, new_prog, off, len, &original_insn);
>> adjust_subprog_starts(env, off, len);
>> adjust_insn_arrays(env, off, len);
>> adjust_poke_descs(new_prog, off, len);
>
> [ ... ]
>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index e036ae20bf6b9..364332eb41859 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -3272,6 +3272,11 @@ static void mark_indirect_target(struct
>> bpf_verifier_env *env, int idx)
>> env->insn_aux_data[idx].indirect_target = true;
>> }
>>
>> +static void mark_non_stack_access(struct bpf_verifier_env *env, int idx)
>> +{
>> + env->insn_aux_data[idx].non_stack_access = true;
>> +}
>> +
>
> [ ... ]
>
>> @@ -6416,6 +6421,7 @@ static int check_mem_access(struct bpf_verifier_env
>> *env, int insn_idx, struct b
>> int value_regno, bool strict_alignment_once, bool
>> is_ldsx)
>> {
>> struct bpf_reg_state *regs = cur_regs(env);
>> + enum bpf_reg_type ptr_type = reg->type;
>> int size, err = 0;
>>
>> size = bpf_size_to_bytes(bpf_size);
>
> [ ... ]
>
>> @@ -6654,6 +6660,10 @@ static int check_mem_access(struct bpf_verifier_env
>> *env, int insn_idx, struct b
>> clear_scalar_id(®s[value_regno]);
>> }
>> }
>> +
>> + if (!err && ptr_type != PTR_TO_STACK)
>> + mark_non_stack_access(env, insn_idx);
>> +
>> return err;
>> }
>
> Does the unconditional marking at the tail of check_mem_access() handle
> callers that are not load/store instructions?
>
> check_mem_access() is called with the call instruction's index from
> process_dynptr_func(), process_iter_arg(), check_helper_call(), and
> process_irq_flag(), where it is used to validate a helper/kfunc argument
> pointer rather than an actual BPF memory instruction.
>
> Because those argument pointers are typically PTR_TO_MAP_VALUE, PTR_TO_MEM,
> PTR_TO_BTF_ID etc. (i.e. != PTR_TO_STACK), the aux entry of the call
> instruction is marked. The flag then no longer means 'this load/store
> instruction may touch non-stack memory' for those slots; it means nothing,
> since a call performs no direct memory access.
>
> Consumers that only look at load/store opcodes are unaffected, but the
> marking is semantically wrong and, combined with the BPF_ST_NOSPEC issue
> above, means non_stack_access can be true for aux entries whose
> instruction is not a memory access.
>
> Should the marking be gated on the instruction class actually being
> BPF_LDX/BPF_STX/BPF_ST, or should the mark be done by the load/store
> callers rather than inside check_mem_access()?
Hmmm, I missed this specificity. And I guess is_mem_insn could be useful
here as well, then.
Alexis
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
