[PATCH v11 bpf-next 5/7] bpf: Use btf_kfunc_id_set.remap logic for bpf_dynptr_from_skb

[Alexei Starovoitov]

On Thu, Jan 30, 2025 at 9:49 AM Song Liu <songliubraving at meta.com> wrote:
>
> Hi Alexei,
>
> Thanks for the review!
>
> > On Jan 29, 2025, at 6:32 PM, Alexei Starovoitov <alexei.starovoitov at gmail.com> wrote:
>
> [...]
>
> >>
> >> +BTF_ID_LIST(bpf_dynptr_from_skb_list)
> >> +BTF_ID(func, bpf_dynptr_from_skb)
> >> +BTF_ID(func, bpf_dynptr_from_skb_rdonly)
> >> +
> >> +static u32 bpf_kfunc_set_skb_remap(const struct bpf_prog *prog, u32 kfunc_id)
> >> +{
> >> +       if (kfunc_id != bpf_dynptr_from_skb_list[0])
> >> +               return 0;
> >> +
> >> +       switch (resolve_prog_type(prog)) {
> >> +       /* Program types only with direct read access go here! */
> >> +       case BPF_PROG_TYPE_LWT_IN:
> >> +       case BPF_PROG_TYPE_LWT_OUT:
> >> +       case BPF_PROG_TYPE_LWT_SEG6LOCAL:
> >> +       case BPF_PROG_TYPE_SK_REUSEPORT:
> >> +       case BPF_PROG_TYPE_FLOW_DISSECTOR:
> >> +       case BPF_PROG_TYPE_CGROUP_SKB:
> >
> > This copy pastes the logic from may_access_direct_pkt_data(),
> > so any future change to that helper would need to update
> > this one as well.
>
> We can probably improve this with some helpers/macros.
>
> >
> >> +               return bpf_dynptr_from_skb_list[1];
> >
> > The [0] and [1] stuff is quite error prone.
> >
> >> +
> >> +       /* Program types with direct read + write access go here! */
> >> +       case BPF_PROG_TYPE_SCHED_CLS:
> >> +       case BPF_PROG_TYPE_SCHED_ACT:
> >> +       case BPF_PROG_TYPE_XDP:
> >> +       case BPF_PROG_TYPE_LWT_XMIT:
> >> +       case BPF_PROG_TYPE_SK_SKB:
> >> +       case BPF_PROG_TYPE_SK_MSG:
> >> +       case BPF_PROG_TYPE_CGROUP_SOCKOPT:
> >> +               return kfunc_id;
> >> +
> >> +       default:
> >> +               break;
> >> +       }
> >> +       return bpf_dynptr_from_skb_list[1];
> >> +}
> >> +
> >> static const struct btf_kfunc_id_set bpf_kfunc_set_skb = {
> >>        .owner = THIS_MODULE,
> >>        .set = &bpf_kfunc_check_set_skb,
> >> +       .hidden_set = &bpf_kfunc_check_hidden_set_skb,
> >
> > If I'm reading it correctly the hidden_set serves no additional purpose.
> > It splits the set into two, but patch 4 just adds them together.
>
> hidden_set does not have BTF_SET8_KFUNCS, so pahole will not export
> these kfuncs to vmlinux.h.
>
> >
> >> +       .remap = &bpf_kfunc_set_skb_remap,
> >
> > I'm not a fan of callbacks in general.
> > The makes everything harder to follow.
>
> This motivation here is to move polymorphism logic from verifier
> core to kfuncs owners. I guess we will need some callback to
> achieve this goal. Of course, we don't have to do it in this set.
>
>
> > For all these reasons I don't like this approach.
> > This "generality" doesn't make it cleaner or easier to extend.
> > For the patch 6... just repeat what specialize_kfunc()
> > currently does for dynptr ?
>
> Yes, specialize_kfunc() can handle this. But we will need to use
> d_inode_locked_hooks from 6/7 in specialize_kfunc(). It works,
> but it is not clean (to me).

I'm missing why that would be necessary to cross the layers
so much. I guess the code will tell.
Pls send an rfc to illustrate the unclean part.

> I will revise this set so that the polymorphism logic in handled
> in specialize_kfunc(). For longer term, maybe we should discuss
> "move some logic from verifier core to kfuncs" in the upcoming
> LSF/MM/BPF?

imo such topic is too narrow and detail oriented.
There is not much to gain from discussing it at lsfmm.
email works well for such discussions.