Re: [PATCH RFC bpf-next 1/1] bpf: Support 64-bit pointers to kfuncs

[Stanislav Fomichev <sdf@google.com>]

On 02/15, Ilya Leoshkevich wrote:
> On Tue, 2023-02-14 at 15:58 -0800, Stanislav Fomichev wrote:
> > On 02/14, Ilya Leoshkevich wrote:
> > > test_ksyms_module fails to emit a kfunc call targeting a module on
> > > s390x, because the verifier stores the difference between kfunc
> > > address and __bpf_call_base in bpf_insn.imm, which is s32, and
> > > modules
> > > are roughly (1 << 42) bytes away from the kernel on s390x.
> >
> > > Fix by keeping BTF id in bpf_insn.imm for BPF_PSEUDO_KFUNC_CALLs,
> > > and storing the absolute address in bpf_kfunc_desc, which JITs
> > > retrieve
> > > as usual by calling bpf_jit_get_func_addr().
> >
> > > This also fixes the problem with XDP metadata functions outlined in
> > > the description of commit 63d7b53ab59f ("s390/bpf: Implement
> > > bpf_jit_supports_kfunc_call()") by replacing address lookups with
> > > BTF
> > > id lookups. This eliminates the inconsistency between "abstract"
> > > XDP
> > > metadata functions' BTF ids and their concrete addresses.
> >
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >   include/linux/bpf.h   |  2 ++
> > >   kernel/bpf/core.c     | 23 ++++++++++---
> > >   kernel/bpf/verifier.c | 79 +++++++++++++-------------------------
> > > -----
> > >   3 files changed, 45 insertions(+), 59 deletions(-)
> >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index be34f7deb6c3..83ce94d11484 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -2227,6 +2227,8 @@ bool bpf_prog_has_kfunc_call(const struct
> > > bpf_prog 
> > > *prog);
> > >   const struct btf_func_model *
> > >   bpf_jit_find_kfunc_model(const struct bpf_prog *prog,
> > >                          const struct bpf_insn *insn);
> > > +int bpf_get_kfunc_addr(const struct bpf_prog *prog, u32 func_id,
> > > u16 
> > > offset,
> > > +                      u8 **func_addr);
> > >   struct bpf_core_ctx {
> > >         struct bpf_verifier_log *log;
> > >         const struct btf *btf;
> > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > > index 3390961c4e10..a42382afe333 100644
> > > --- a/kernel/bpf/core.c
> > > +++ b/kernel/bpf/core.c
> > > @@ -1185,10 +1185,12 @@ int bpf_jit_get_func_addr(const struct
> > > bpf_prog 
> > > *prog,
> > >   {
> > >         s16 off = insn->off;
> > >         s32 imm = insn->imm;
> > > +       bool fixed;
> > >         u8 *addr;
> > > +       int err;
> >
> > > -       *func_addr_fixed = insn->src_reg != BPF_PSEUDO_CALL;
> > > -       if (!*func_addr_fixed) {
> > > +       switch (insn->src_reg) {
> > > +       case BPF_PSEUDO_CALL:
> > >                 /* Place-holder address till the last pass has
> > > collected
> > >                  * all addresses for JITed subprograms in which
> > > case we
> > >                  * can pick them up from prog->aux.
> > > @@ -1200,16 +1202,29 @@ int bpf_jit_get_func_addr(const struct
> > > bpf_prog 
> > > *prog,
> > >                         addr = (u8 *)prog->aux->func[off]-
> > > >bpf_func;
> > >                 else
> > >                         return -EINVAL;
> > > -       } else {
> > > +               fixed = false;
> > > +               break;
> >
> > [..]
> >
> > > +       case 0:
> >
> > nit: Should we define BPF_HELPER_CALL here for consistency?

> I think this would be good, the verifier currently uses 0 for this
> purpose; having a symbolic constant would surely improve readability.

> > >                 /* Address of a BPF helper call. Since part of the
> > > core
> > >                  * kernel, it's always at a fixed location.
> > > __bpf_call_base
> > >                  * and the helper with imm relative to it are both
> > > in core
> > >                  * kernel.
> > >                  */
> > >                 addr = (u8 *)__bpf_call_base + imm;
> > > +               fixed = true;
> > > +               break;
> > > +       case BPF_PSEUDO_KFUNC_CALL:
> > > +               err = bpf_get_kfunc_addr(prog, imm, off, &addr);
> > > +               if (err)
> > > +                       return err;
> > > +               fixed = true;
> > > +               break;
> > > +       default:
> > > +               return -EINVAL;
> > >         }
> >
> > > -       *func_addr = (unsigned long)addr;
> > > +       *func_addr_fixed = fixed;
> > > +       *func_addr = addr;
> > >         return 0;
> > >   }
> >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 21e08c111702..aea59974f0d6 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -2115,8 +2115,8 @@ static int add_subprog(struct
> > > bpf_verifier_env 
> > > *env, int off)
> > >   struct bpf_kfunc_desc {
> > >         struct btf_func_model func_model;
> > >         u32 func_id;
> > > -       s32 imm;
> > >         u16 offset;
> > > +       unsigned long addr;
> > >   };
> >
> > >   struct bpf_kfunc_btf {
> > > @@ -2166,6 +2166,19 @@ find_kfunc_desc(const struct bpf_prog *prog,
> > > u32 
> > > func_id, u16 offset)
> > >                        sizeof(tab->descs[0]),
> > > kfunc_desc_cmp_by_id_off);
> > >   }
> >
> >
> > [..]
> >
> > > +int bpf_get_kfunc_addr(const struct bpf_prog *prog, u32 func_id,
> > > u16 
> > > offset,
> > > +                      u8 **func_addr)
> > > +{
> > > +       const struct bpf_kfunc_desc *desc;
> > > +
> > > +       desc = find_kfunc_desc(prog, func_id, offset);
> > > +       if (WARN_ON_ONCE(!desc))
> > > +               return -EINVAL;
> > > +
> > > +       *func_addr = (u8 *)desc->addr;
> > > +       return 0;
> > > +}
> >
> > This function isn't doing much and has a single caller. Should we
> > just
> > export find_kfunc_desc?

> We would have to export struct bpf_kfunc_desc as well; I thought it's
> better to add an extra function so that we could keep hiding the
> struct.

Ah, good point. In this case seems ok to have this extra wrapper.
On that note: what's the purpose of WARN_ON_ONCE here?


> > > +
> > >   static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env
> > > *env,
> > >                                          s16 offset)
> > >   {
> > > @@ -2261,8 +2274,8 @@ static int add_kfunc_call(struct
> > > bpf_verifier_env 
> > > *env, u32 func_id, s16 offset)
> > >         struct bpf_kfunc_desc *desc;
> > >         const char *func_name;
> > >         struct btf *desc_btf;
> > > -       unsigned long call_imm;
> > >         unsigned long addr;
> > > +       void *xdp_kfunc;
> > >         int err;
> >
> > >         prog_aux = env->prog->aux;
> > > @@ -2346,24 +2359,21 @@ static int add_kfunc_call(struct
> > > bpf_verifier_env 
> > > *env, u32 func_id, s16 offset)
> > >                 return -EINVAL;
> > >         }
> >
> > > -       call_imm = BPF_CALL_IMM(addr);
> > > -       /* Check whether or not the relative offset overflows desc-
> > > >imm */
> > > -       if ((unsigned long)(s32)call_imm != call_imm) {
> > > -               verbose(env, "address of kernel function %s is out
> > > of range\n",
> > > -                       func_name);
> > > -               return -EINVAL;
> > > -       }
> > > -
> > >         if (bpf_dev_bound_kfunc_id(func_id)) {
> > >                 err = bpf_dev_bound_kfunc_check(&env->log,
> > > prog_aux);
> > >                 if (err)
> > >                         return err;
> > > +
> > > +               xdp_kfunc = bpf_dev_bound_resolve_kfunc(env->prog,
> > > func_id);
> > > +               if (xdp_kfunc)
> > > +                       addr = (unsigned long)xdp_kfunc;
> > > +               /* fallback to default kfunc when not supported by
> > > netdev */
> > >         }
> >
> > >         desc = &tab->descs[tab->nr_descs++];
> > >         desc->func_id = func_id;
> > > -       desc->imm = call_imm;
> > >         desc->offset = offset;
> > > +       desc->addr = addr;
> > >         err = btf_distill_func_proto(&env->log, desc_btf,
> > >                                      func_proto, func_name,
> > >                                      &desc->func_model);
> > > @@ -2373,30 +2383,6 @@ static int add_kfunc_call(struct
> > > bpf_verifier_env 
> > > *env, u32 func_id, s16 offset)
> > >         return err;
> > >   }
> >
> > > -static int kfunc_desc_cmp_by_imm(const void *a, const void *b)
> > > -{
> > > -       const struct bpf_kfunc_desc *d0 = a;
> > > -       const struct bpf_kfunc_desc *d1 = b;
> > > -
> > > -       if (d0->imm > d1->imm)
> > > -               return 1;
> > > -       else if (d0->imm < d1->imm)
> > > -               return -1;
> > > -       return 0;
> > > -}
> > > -
> > > -static void sort_kfunc_descs_by_imm(struct bpf_prog *prog)
> > > -{
> > > -       struct bpf_kfunc_desc_tab *tab;
> > > -
> > > -       tab = prog->aux->kfunc_tab;
> > > -       if (!tab)
> > > -               return;
> > > -
> > > -       sort(tab->descs, tab->nr_descs, sizeof(tab->descs[0]),
> > > -            kfunc_desc_cmp_by_imm, NULL);
> > > -}
> > > -
> > >   bool bpf_prog_has_kfunc_call(const struct bpf_prog *prog)
> > >   {
> > >         return !!prog->aux->kfunc_tab;
> > > @@ -2407,14 +2393,15 @@ bpf_jit_find_kfunc_model(const struct
> > > bpf_prog 
> > > *prog,
> > >                          const struct bpf_insn *insn)
> > >   {
> > >         const struct bpf_kfunc_desc desc = {
> > > -               .imm = insn->imm,
> > > +               .func_id = insn->imm,
> > > +               .offset = insn->off,
> > >         };
> > >         const struct bpf_kfunc_desc *res;
> > >         struct bpf_kfunc_desc_tab *tab;
> >
> > >         tab = prog->aux->kfunc_tab;
> > >         res = bsearch(&desc, tab->descs, tab->nr_descs,
> > > -                     sizeof(tab->descs[0]),
> > > kfunc_desc_cmp_by_imm);
> > > +                     sizeof(tab->descs[0]),
> > > kfunc_desc_cmp_by_id_off);
> >
> > >         return res ? &res->func_model : NULL;
> > >   }
> > > @@ -16251,7 +16238,6 @@ static int fixup_kfunc_call(struct 
> > > bpf_verifier_env *env, struct bpf_insn *insn,
> > >                             struct bpf_insn *insn_buf, int
> > > insn_idx, int *cnt)
> > >   {
> > >         const struct bpf_kfunc_desc *desc;
> > > -       void *xdp_kfunc;
> >
> > >         if (!insn->imm) {
> > >                 verbose(env, "invalid kernel function call not
> > > eliminated in verifier 
> > > pass\n");
> > > @@ -16259,20 +16245,6 @@ static int fixup_kfunc_call(struct 
> > > bpf_verifier_env *env, struct bpf_insn *insn,
> > >         }
> >
> > >         *cnt = 0;
> > > -
> > > -       if (bpf_dev_bound_kfunc_id(insn->imm)) {
> > > -               xdp_kfunc = bpf_dev_bound_resolve_kfunc(env->prog,
> > > insn->imm);
> > > -               if (xdp_kfunc) {
> > > -                       insn->imm = BPF_CALL_IMM(xdp_kfunc);
> > > -                       return 0;
> > > -               }
> > > -
> > > -               /* fallback to default kfunc when not supported by
> > > netdev */
> > > -       }
> > > -
> > > -       /* insn->imm has the btf func_id. Replace it with
> > > -        * an address (relative to __bpf_call_base).
> > > -        */
> > >         desc = find_kfunc_desc(env->prog, insn->imm, insn->off);
> > >         if (!desc) {
> > >                 verbose(env, "verifier internal error: kernel
> > > function descriptor not 
> > > found for func_id %u\n",
> > > @@ -16280,7 +16252,6 @@ static int fixup_kfunc_call(struct 
> > > bpf_verifier_env *env, struct bpf_insn *insn,
> > >                 return -EFAULT;
> > >         }
> >
> > > -       insn->imm = desc->imm;
> > >         if (insn->off)
> > >                 return 0;
> > >         if (desc->func_id ==
> > > special_kfunc_list[KF_bpf_obj_new_impl]) {
> > > @@ -16834,8 +16805,6 @@ static int do_misc_fixups(struct
> > > bpf_verifier_env 
> > > *env)
> > >                 }
> > >         }
> >
> >
> > [..]
> >
> > > -       sort_kfunc_descs_by_imm(env->prog);
> >
> > If we are not doing sorting here, how does the bsearch work?

> add_kfunc_call() already makes sure that kfuncs are sorted in the
> kfunc_desc_cmp_by_id_off() order. fixup_kfunc_call() used to put
> addresses into imms and re-sort based on that, however, after this
> patch it's not needed anymore: the code (e.g.
> bpf_jit_find_kfunc_model()) continues to do lookups in
> the kfunc_desc_cmp_by_id_off() order.

Makes sense, thank you 👍

> > > -
> > >         return 0;
> > >   }
> >
> > > --
> > > 2.39.1
> >