Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3505 bytes --]

> On Fri, Jan 14, 2022 at 10:55 AM Zvi Effron <zeffron@riotgames.com> wrote:
> >
> > On Fri, Jan 14, 2022 at 8:50 AM Jesper Dangaard Brouer
> > <jbrouer@redhat.com> wrote:
> > >
> > >
> > >
> > > On 14/01/2022 03.09, Alexei Starovoitov wrote:
> > > > On Thu, Jan 13, 2022 at 3:58 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > > >>>
> > > >>> Btw "xdp_cpumap" should be cleaned up.
> > > >>> xdp_cpumap is an attach type. It's not prog type.
> > > >>> Probably it should be "xdp/cpumap" to align with "cgroup/bind[46]" ?
> > > >>
> > > >> so for xdp "mb" or xdp "frags" it will be xdp/cpumap.mb (xdp/devmap.mb) or
> > > >> xdp/cpumap.frags (xdp/devmap.frags), right?
> > > >
> > > > xdp.frags/cpumap
> > > > xdp.frags/devmap
> > > >
> > > > The current de-facto standard for SEC("") in libbpf:
> > > > prog_type.prog_flags/attach_place
> > >
> > > Ups, did we make a mistake with SEC("xdp_devmap/")
> > >
> > > and can we correct without breaking existing programs?
> > >
> >
> > We can (at the very least) add the correct sections, even if we leave the
> > current incorrect ones as well. Ideally we'd mark the incorrect ones deprecated
> > and either remove them before libbpf 1.0 or as part of 2.0?
> >
> 
> Correct, those would need to be new aliases. We can also deprecate old
> ones, if we have consensus on that. We can teach libbpf to emit
> warnings (through logs, of course) for such uses of to-be-removed
> sections aliases. We still have probably a few months before the final
> 1.0 release, should hopefully be plenty of time to people to adapt.

If we all agree on old cpumap/devmap sec deprecation and replace them with
xdp/cpumap and xdp/devmap, would it be ok something like the patch below or
would be necessary something different?

Regards,
Lorenzo

--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6575,6 +6575,12 @@ static int libbpf_preload_prog(struct bpf_program *prog,
 	if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_FRAGS))
 		opts->prog_flags |= BPF_F_XDP_HAS_FRAGS;
 
+	if (strstr(prog->sec_name, "xdp_devmap") ||
+	    strstr(prog->sec_name, "xdp_cpumap")) {
+		pr_warn("sec_name '%s' is deprecated, use xdp/devmap or xdp_cpumap instead\n",
+			prog->sec_name);
+	}
+
 	if ((prog->type == BPF_PROG_TYPE_TRACING ||
 	     prog->type == BPF_PROG_TYPE_LSM ||
 	     prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {
@@ -8618,8 +8624,10 @@ static const struct bpf_sec_def section_defs[] = {
 	SEC_DEF("iter.s/",		TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_iter),
 	SEC_DEF("syscall",		SYSCALL, 0, SEC_SLEEPABLE),
 	SEC_DEF("xdp.frags/devmap",	XDP, BPF_XDP_DEVMAP, SEC_XDP_FRAGS),
+	SEC_DEF("xdp/devmap",		XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
 	SEC_DEF("xdp_devmap/",		XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
 	SEC_DEF("xdp.frags/cpumap",	XDP, BPF_XDP_CPUMAP, SEC_XDP_FRAGS),
+	SEC_DEF("xdp/cpumap",		XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
 	SEC_DEF("xdp_cpumap/",		XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
 	SEC_DEF("xdp.frags",		XDP, BPF_XDP, SEC_XDP_FRAGS),
 	SEC_DEF("xdp",			XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),


> 
> > --Zvi
> >
> > > > "attach_place" is either function_name for fentry/, tp/, lsm/, etc.
> > > > or attach_type/hook/target for cgroup/bind4, cgroup_skb/egress.
> > > >
> > > > lsm.s/socket_bind -> prog_type = LSM, flags = SLEEPABLE
> > > > lsm/socket_bind -> prog_type = LSM, non sleepable.
> > > >
> > >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]