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

[Song Liu <songliubraving@meta.com>]

> On Jan 14, 2025, at 2:37 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 

[...]

>> 
>>        if (bpf_dev_bound_kfunc_id(func_id)) {
>>                xdp_kfunc = bpf_dev_bound_resolve_kfunc(prog, func_id);
>> @@ -20833,22 +20836,6 @@ static void specialize_kfunc(struct bpf_verifier_env *env,
>>                }
>>                /* 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)
>> -                       *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;
> 
> is it safe to remove this special seen_direct_write part of logic?

We need to save and restore seen_direct_write because 
may_access_direct_pkt_data() mutates it. If we do not call 
may_access_direct_pkt_data() here, as after this patch, we don't need to 
save and restore seen_direct_write. 

> 
>> -       }
>> }
>> 
>> static void __fixup_collection_insert_kfunc(struct bpf_insn_aux_data *insn_aux,
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 21131ec25f24..f12bcc1b21d1 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -12047,10 +12047,8 @@ __bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct __sk_buff *s, struct sock *sk,
>> #endif
>> }
>> 
>> -__bpf_kfunc_end_defs();
>> -
>> -int bpf_dynptr_from_skb_rdonly(struct __sk_buff *skb, u64 flags,
>> -                              struct bpf_dynptr *ptr__uninit)
>> +__bpf_kfunc int bpf_dynptr_from_skb_rdonly(struct __sk_buff *skb, u64 flags,
>> +                                          struct bpf_dynptr *ptr__uninit)
>> {
>>        struct bpf_dynptr_kern *ptr = (struct bpf_dynptr_kern *)ptr__uninit;
>>        int err;
>> @@ -12064,10 +12062,16 @@ int bpf_dynptr_from_skb_rdonly(struct __sk_buff *skb, u64 flags,
>>        return 0;
>> }

[...]

>> +
>> +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:
>> +               return bpf_dynptr_from_skb_list[1];
>> +
>> +       /* 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];
>> +}
> 
> I'd personally prefer the approach we have with BPF helpers, where
> each program type has a function that handles all helpers (identified
> by its ID), and then we can use C code sharing to minimize duplication
> of code.

Different hooks of the same program type, especially struct_ops, may 
not have same access to different kfuncs. Therefore, I am not sure 
whether the approach with helpers can scale in the long term. At the
moment, we use special_kfunc_[type|set|list] to handle special cases. 
But I am afraid this approach cannot work well with more struct_ops
and kfuncs. 

> 
> With this approach it seems like we'll have more duplication and we'll
> need to repeat these program type-based large switches for various
> small sets of kfuncs, no?

The motivation is to make the verification of kfuncs more modular, so 
that each set of kfuncs handle their verification as much as possible.

I think the code duplication here (bpf_kfunc_set_skb_remap) is not a
common problem. And we can actually reduce duplication with some 
simple helpers. 

Does this make sense?

Thanks,
Song