On Thu, Jan 30, 2025 at 9:49 AM Song Liu [off-list ref] wrote:

Hi Alexei,

Thanks for the review!

quoted

On Jan 29, 2025, at 6:32 PM, Alexei Starovoitov [off-list ref] wrote:

[...]

quoted

quoted

+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.

quoted

quoted

+               return bpf_dynptr_from_skb_list[1];

The [0] and [1] stuff is quite error prone.

quoted

+
+       /* 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.

quoted

quoted

+       .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.

quoted

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.