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

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Nov 29, 2023 at 10:10:22AM -0800, Stanislav Fomichev wrote:
> On Wed, Nov 29, 2023 at 6:55 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Wed, Nov 29, 2023 at 03:05:21PM +0100, Jiri Olsa wrote:
> > > 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
> >
> > looks like below change would be enough, I'll test and send new version
> 
> sg. I'm still not 100% sure why it's x86 only, I was (probably
> wrongly?) assuming that at least arm64 jit is mostly on par with x86
> :-)

AFAICS the direct tail calls are on x86, CI also seems to be ok with
that change.. I'll send it as formal patch

jirka