Re: [PATCH bpf-next v6] bpf: Support 64-bit pointers to kfuncs

[Ilya Leoshkevich <iii@linux.ibm.com>]

On Thu, 2023-04-06 at 11:44 +0200, Jiri Olsa wrote:
> On Wed, Apr 05, 2023 at 11:34:53PM +0200, Ilya Leoshkevich wrote:
> 
> SNIP
> 
> >  
> > +int bpf_get_kfunc_addr(const struct bpf_prog *prog, u32 func_id,
> > +                      u16 btf_fd_idx, u8 **func_addr)
> > +{
> > +       const struct bpf_kfunc_desc *desc;
> > +
> > +       desc = find_kfunc_desc(prog, func_id, btf_fd_idx);
> > +       if (!desc)
> > +               return -EFAULT;
> > +
> > +       *func_addr = (u8 *)desc->addr;
> > +       return 0;
> > +}
> > +
> >  static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env
> > *env,
> >                                          s16 offset)
> >  {
> > @@ -2672,14 +2691,19 @@ 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_jit_supports_far_kfunc_call()) {
> > +               call_imm = func_id;
> > +       } else {
> > +               call_imm = BPF_CALL_IMM(addr);
> 
> we compute call_imm again in fixup_kfunc_call, seems like we could
> store
> the address and the func_id in desc and have fixup_kfunc_call do the
> insn->imm setup

We can drop this diff in fixup_kfunc_call():

-       insn->imm = desc->imm;
+       if (!bpf_jit_supports_far_kfunc_call())
+               insn->imm = BPF_CALL_IMM(desc->addr);

in order to avoid duplicating the imm calculation logic, but I'm not
sure if we want to move the entire desc->imm setup there.

For example, fixup_kfunc_call() considers kfunc_tab const, which is a
nice property that I think is worth keeping.

Another option would be to drop desc->imm, but having it is very
convenient for doing lookups the same way on all architectures. 

> > +               /* Check whether 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;
> > +               }
> >         }
> >  
> > +
> 
> nit, extra line

Ouch. Thanks for spotting this.

> 
> >         if (bpf_dev_bound_kfunc_id(func_id)) {
> >                 err = bpf_dev_bound_kfunc_check(&env->log,
> > prog_aux);
> >                 if (err)
> > @@ -2690,6 +2714,7 @@ static int add_kfunc_call(struct
> > bpf_verifier_env *env, u32 func_id, s16 offset)
> >         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);
> > @@ -2699,19 +2724,19 @@ 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)
> > +static int kfunc_desc_cmp_by_imm_off(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;
> > +       if (d0->imm != d1->imm)
> > +               return d0->imm < d1->imm ? -1 : 1;
> > +       if (d0->offset != d1->offset)
> > +               return d0->offset < d1->offset ? -1 : 1;
> >         return 0;
> >  }
> >  
> 
> SNIP
> 
> > +/* replace a generic kfunc with a specialized version if necessary
> > */
> > +static void fixup_kfunc_desc(struct bpf_verifier_env *env,
> > +                            struct bpf_kfunc_desc *desc)
> > +{
> > +       struct bpf_prog *prog = env->prog;
> > +       u32 func_id = desc->func_id;
> > +       u16 offset = desc->offset;
> > +       bool seen_direct_write;
> > +       void *xdp_kfunc;
> > +       bool is_rdonly;
> > +
> > +       if (bpf_dev_bound_kfunc_id(func_id)) {
> > +               xdp_kfunc = bpf_dev_bound_resolve_kfunc(prog,
> > func_id);
> > +               if (xdp_kfunc) {
> > +                       desc->addr = (unsigned long)xdp_kfunc;
> > +                       return;
> > +               }
> > +               /* fallback to default kfunc when not supported by
> > netdev */
> > +       }
> > +
> > +       if (offset)
> > +               return;
> > +
> > +       if (func_id == special_kfunc_list[KF_bpf_dynptr_from_skb])
> > {
> > +               seen_direct_write = env->seen_direct_write;
> > +               is_rdonly = !may_access_direct_pkt_data(env, NULL,
> > BPF_WRITE);
> > +
> > +               if (is_rdonly)
> > +                       desc->addr = (unsigned
> > long)bpf_dynptr_from_skb_rdonly;
> > +
> > +               /* restore env->seen_direct_write to its original
> > value, since
> > +                * may_access_direct_pkt_data mutates it
> > +                */
> > +               env->seen_direct_write = seen_direct_write;
> > +       }
> 
> could we do this directly in add_kfunc_call?

Initially I thought that it wasn't possible, because
may_access_direct_pkt_data() may depend on data gathered during
verification. But on a second look that's simply not the case, so this
code can indeed be moved to add_kfunc_call().

> 
> thanks,
> jirka

[...]