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

[Ihor Solodrai <ihor.solodrai@linux.dev>]

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.

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?

> 
> Add a new parameter to bpf_patch_insn_data and adjust_insn_aux_data to
> convey the info about the new location in the patch of the original
> instruction. This info does not always make sense depending on the
> generated patch (eg, some bpf helpers being inlined by the verifier, and
> so turned into multiple new instructions without any remaining BPF_CALL),
> the parameter can then be set to -1. It will be used in next patches to
> properly handle insn_aux_data adjustment for the new KASAN
> instrumentation
> 
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
> ---
> Changes in v5:
> - fix a few incorrect offset values
> 
> Changes in v4:
> - define insn_off_in_patch inside convert_ctx_accesses main loop to
>   avoid old value leakage
> 
> Changes in v3:
> - new patch
> ---
>  include/linux/filter.h |  10 +++--
>  kernel/bpf/core.c      |   2 +-
>  kernel/bpf/fixups.c    | 100 ++++++++++++++++++++++++++++++++-----------------
>  3 files changed, 74 insertions(+), 38 deletions(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 14acb2455746..1ebcd247ef4e 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1210,13 +1210,17 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
>  
>  #ifdef CONFIG_BPF_SYSCALL
>  struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 off,
> -				     const struct bpf_insn *patch, u32 len);
> +				     const struct bpf_insn *patch, u32 len,
> +				     s32 insn_off_in_patch);
>  struct bpf_insn_aux_data *bpf_dup_insn_aux_data(struct bpf_verifier_env *env);
>  void bpf_restore_insn_aux_data(struct bpf_verifier_env *env,
>  			       struct bpf_insn_aux_data *orig_insn_aux);
>  #else
> -static inline struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 off,
> -						   const struct bpf_insn *patch, u32 len)
> +static inline struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env,
> +						   u32 off,
> +						   const struct bpf_insn *patch,
> +						   u32 len,
> +						   s32 insn_off_in_patch)
>  {
>  	return ERR_PTR(-ENOTSUPP);
>  }
> [...]
>  
>