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

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

On Thu Jul 16, 2026 at 8:17 AM CEST, Alexis Lothoré wrote:

[...]

>>> 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.

So this new logic indeed let some "unwanted" cases being instrumented,
like the ldx_patched_on_stack (testing the rnd_hi32 case, so involving
bpf_opt_subreg_zext_lo32_rnd_hi32):

0000000000000710 <ldx_patched_on_stack>:
     226:       bf a6 00 00 00 00 00 00 r6 = r10
     227:       07 06 00 00 f0 ff ff ff r6 += -0x10
     228:       bf 61 00 00 00 00 00 00 r1 = r6
     229:       b4 02 00 00 10 00 00 00 w2 = 0x10
     230:       85 10 00 00 ff ff ff ff call -0x1
     231:       61 a1 f4 ff 00 00 00 00 w1 = *(u32 *)(r10 - 0xc)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                Is instrumented, shouldn't
     232:       63 1a f4 ff 00 00 00 00 *(u32 *)(r10 - 0xc) = w1
     233:       bf 61 00 00 00 00 00 00 r1 = r6
     234:       b4 02 00 00 10 00 00 00 w2 = 0x10
     235:       85 10 00 00 ff ff ff ff call -0x1
     236:       b4 00 00 00 00 00 00 00 w0 = 0x0
     237:       95 00 00 00 00 00 00 00 exit

While taking a further look at it, I thought about an intermediate
solution that would prevent the unwanted instrumentation, without
spreading the insn_off_in_patch value I initially proposed. What if:
- in bpf_patch_insn_data, we copy the original instruction being patched
  (the one at "off") in a temp var
- we pass this saved insn to adjust_insn_aux_data
- in the loop going over the patched zone, if the current insn matches
  the saved insn, we apply back the original non_stack_access flag,
  otherwise we systematically mark the insn as "non stack accessing" if
  this is a mem insn

Assuming that memcp'ing a struct bpf_insn is enough/correct to check
whether we are processing the original insn or a new one. Some initial
tests in my env correctly reduce the amount of instrumentation for those
stack accesses, without having to edit all bpf_patch_insn_data sites.

Alexis

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