On Tue, Jan 14, 2025 at 3:03 PM Song Liu <songliubraving@xxxxxxxx> wrote:
>
>
>
> > 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.
>
ah, existing logic is quite convoluted (and that resolve_prog_type()
bit is another gotcha that's easy to miss), ok, so we used some
side-effecting function for simulating side effect-free check...
> >
> >> - }
> >> }
> >>
> >> 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.
>
I think we had discussion in the similar vein at last LSF/MM/BPF.
struct_ops is sort of a "meta program type", I don't consider it to be
sufficient by itself. For struct_ops you need to know which exact
struct_ops callback is being considered (that's what would identify
"BPF program type" in the pre-struct_ops world).
But anyways, somehow BPF helpers approach worked across lots of
program types, not sure I see why it wouldn't work for kfuncs
(especially taking into account extending struct_ops with more
detailed "which struct_ops callback" bit).
> >
> > 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?
see above, I'm a bit skeptical, buf proof is in the pudding ;)
>
> Thanks,
> Song
>
>
