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
[...]
