Re: [PATCH bpf-next v5 01/10] bpf: propagate original instruction offset when patching program

["Alexis Lothoré" <alexis.lothore@bootlin.com>]

Hi Ihor,
thanks for the thorough review !

On Wed Jul 15, 2026 at 2:47 AM CEST, Ihor Solodrai wrote:
> On 7/9/26 3:01 AM, Alexis Lothoré (eBPF Foundation) wrote:
>> When the verifier patches an ebpf program with bpf_patch_insn_data, it
>> then calls adjust_insn_aux_data to make sure that insn_aux_data takes
>> into account the newly inserted patch. Some of the data offset is pretty
>> straightforward to deduce, it is for example the case for
>> indirect_target, as any patch affecting indirect calls will
>> systematically move the original instruction to the end of the new
>> patch.
>
> I think an additional KASAN-specific argument to adjust_insn_aux_data()
> is not a good idea. It's threaded through ~30 call sites, and it's
> been error-prone too: you had to fix the offsets a few times already.

Agree. I've indeed already made a few back and forth on it, it looks
like there are still more to do, and not in the good direction (I mean,
just passing -1 to ignore original insn offset), so that sounds more and
more like a hassle for no significant gain. This ends up being pretty
intrusive for a debug feature, and as you state below, the only downside
of not tracking too finely those stack accesses is about getting a few
unecessary KASAN checks.

> indirect_target needs no help from the callers, so it's not really the
> same pattern. Similar for the other aux fields: seen is broadcast and
> zext_dst is re-derived.
>
>> 
>> In order to introduce KASAN support for eBPF JIT, we need to mark any
>> load/store instruction that accesses non-stack memory, but updating this
>> new marking after a patch is not as straightforward as for indirect
>> calls: the original BPF_ST/BPF_STX/BPF_LDX can be at the beginning, at
>> the end or somewhere in the middle of the new patch: we then need some
>> additional info to properly update this marking.
>
> I don't think we need to track the exact offset here.
>
> It looks like .non_stack_access is set to is_mem_insn(insn + off) in
> every single case *except* for when a stack access happens to be in
> the middle of a patch.
>
> Given that the cost of getting the flag "wrong" is an unnecessary
> kasan check only for that case, I think it'll be cleaner to just
> unconditionally do:
>
>     data[off].non_stack_access = is_mem_insn(insn + off);
>
> in adjust_insn_aux_data()
>
> And then the code change can be folded in patch #2
>
> The only problem with this I can think of is that in reality *most*
> stack accesses go trough that special case, which would defeat the
> purpose of the .non_stack_access flag. This can be checked empirically
> however.

> Anything else I'm missing here?

Sounds good to me. I'll try to get a rough idea about how often the
flag, in this scenario, correctly triggers or not.

Thanks,

Alexis


-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com