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
