Re: [PATCHv2 bpf 1/2] bpf: Add checkip argument to bpf_arch_text_poke

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Nov 28, 2023 at 01:24:14PM -0800, Stanislav Fomichev wrote:
> On 11/28, Jiri Olsa wrote:
> > We need to be able to skip ip address check for caller in following
> > changes. Adding checkip argument to allow that.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
> >  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
> >  arch/s390/net/bpf_jit_comp.c    |  3 ++-
> >  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
> >  include/linux/bpf.h             |  2 +-
> >  kernel/bpf/arraymap.c           |  8 ++++----
> >  kernel/bpf/core.c               |  2 +-
> >  kernel/bpf/trampoline.c         | 12 ++++++------
> >  8 files changed, 32 insertions(+), 27 deletions(-)
> > 
> > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > index 7d4af64e3982..b52549d18730 100644
> > --- a/arch/arm64/net/bpf_jit_comp.c
> > +++ b/arch/arm64/net/bpf_jit_comp.c
> > @@ -2167,7 +2167,8 @@ static int gen_branch_or_nop(enum aarch64_insn_branch_type type, void *ip,
> >   * locations during the patching process, making the patching process easier.
> >   */
> >  int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type,
> > -		       void *old_addr, void *new_addr)
> > +		       void *old_addr, void *new_addr,
> 
> [..]
> 
> > +		       bool checkip __maybe_unused)
> 
> Any idea why only riscv and x86 do this check?

so arm does the check as well, but needs the data from the lookup
to patch things properly.. but IIUC it does not suffer the same
issue because it does not implement direct tail calls [1] which
is used only on x86

> 
> Asking because maybe it makes sense to move this check into some
> new generic bpf_text_poke and call it in the places where you currently
> call checkip=true (and keep using bpf_arch_text_poke for checkip=false
> case).
> 
> (don't see any issues with the current approach btw, just interested..)

I tried to add new function for that, but it did not look good for arm
because it needs to do the lookup anyway

hm maybe we could use new arch function that would cover the single
tail call 'text poke' update in prog_array_map_poke_run and would be
implemented only on x86 ... using __bpf_arch_text_poke directly

jirka


[1] 428d5df1fa4f bpf, x86: Emit patchable direct jump as tail call