Hi Alexei,
Thanks for the review!
> On Jan 29, 2025, at 6:32 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> 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 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?
Thanks,
Song
