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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAEf4BzYfEAUsywfGLnnZ_wZC40icq6viYhgGywq8eKm9DMqSYA@mail.gmail.com>
Date: Tue, 14 Jan 2025 15:40:52 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Song Liu <songliubraving@...a.com>
Cc: Song Liu <song@...nel.org>, "bpf@...r.kernel.org" <bpf@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"linux-security-module@...r.kernel.org" <linux-security-module@...r.kernel.org>, Kernel Team <kernel-team@...a.com>, 
	"andrii@...nel.org" <andrii@...nel.org>, "ast@...nel.org" <ast@...nel.org>, 
	"daniel@...earbox.net" <daniel@...earbox.net>, "martin.lau@...ux.dev" <martin.lau@...ux.dev>, 
	"kpsingh@...nel.org" <kpsingh@...nel.org>, "mattbobrowski@...gle.com" <mattbobrowski@...gle.com>, 
	"paul@...l-moore.com" <paul@...l-moore.com>, "jmorris@...ei.org" <jmorris@...ei.org>, 
	"serge@...lyn.com" <serge@...lyn.com>, "memxor@...il.com" <memxor@...il.com>
Subject: Re: [PATCH v8 bpf-next 5/7] bpf: Use btf_kfunc_id_set.remap logic for bpf_dynptr_from_skb

On Tue, Jan 14, 2025 at 3:03 PM Song Liu <songliubraving@...a.com> wrote:
>
>
>
> > On Jan 14, 2025, at 2:37 PM, Andrii Nakryiko <andrii.nakryiko@...il.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.
>

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