> On Jan 14, 2025, at 2:37 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> 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
