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

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Dec 01, 2023 at 03:36:26PM +0100, Ilya Leoshkevich wrote:
> On Tue, 2023-11-28 at 10:28 +0100, 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(-)
> 
> [...]
> 
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -435,19 +435,21 @@ static int __bpf_arch_text_poke(void *ip, enum
> > bpf_text_poke_type t,
> >  }
> >  
> >  int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
> > -                      void *old_addr, void *new_addr)
> > +                      void *old_addr, void *new_addr, bool checkip)
> >  {
> > -       if (!is_kernel_text((long)ip) &&
> > -           !is_bpf_text_address((long)ip))
> > -               /* BPF poking in modules is not supported */
> > -               return -EINVAL;
> > +       if (checkip) {
> > +               if (!is_kernel_text((long)ip) &&
> > +                   !is_bpf_text_address((long)ip))
> > +                       /* BPF poking in modules is not supported */
> > +                       return -EINVAL;
> >  
> > -       /*
> > -        * See emit_prologue(), for IBT builds the trampoline hook is
> > preceded
> > -        * with an ENDBR instruction.
> > -        */
> > -       if (is_endbr(*(u32 *)ip))
> > -               ip += ENDBR_INSN_SIZE;
> > +               /*
> > +                * See emit_prologue(), for IBT builds the trampoline
> > hook is preceded
> > +                * with an ENDBR instruction.
> > +                */
> > +               if (is_endbr(*(u32 *)ip))
> > +                       ip += ENDBR_INSN_SIZE;
> 
> Do we really want to skip the IP adjustment too?

the idea was that with __bpf_arch_text_poke you are aware of what you
are updating, so there's no need for extra checking

anyway this version got deprecated and I just sent v3 which is bit
different without this change

thanks,
jirka

> 
> > +       }
> >  
> >         return __bpf_arch_text_poke(ip, t, old_addr, new_addr);
> >  }
> 
> [...]