| 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
| ||
|
Message-ID: <CAC1LvL0CeTw+YKjO6r0f68Ly3tK4qhDyjV0ak82e0PpHURVQOw@mail.gmail.com>
Date: Wed, 12 Jan 2022 12:04:36 -0800
From: Zvi Effron <zeffron@...tgames.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Andrii Nakryiko <andrii.nakryiko@...il.com>,
Lorenzo Bianconi <lorenzo@...nel.org>,
bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Shay Agroskin <shayagr@...zon.com>,
john fastabend <john.fastabend@...il.com>,
David Ahern <dsahern@...nel.org>,
Jesper Dangaard Brouer <brouer@...hat.com>,
Eelco Chaudron <echaudro@...hat.com>,
Jason Wang <jasowang@...hat.com>,
Alexander Duyck <alexander.duyck@...il.com>,
Saeed Mahameed <saeed@...nel.org>,
Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
Magnus Karlsson <magnus.karlsson@...el.com>,
tirthendu.sarkar@...el.com,
Toke Høiland-Jørgensen <toke@...hat.com>
Subject: Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs
On Wed, Jan 12, 2022 at 11:47 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Wed, Jan 12, 2022 at 11:21 AM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Wed, Jan 12, 2022 at 11:17 AM Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> > >
> > > On Wed, Jan 12, 2022 at 10:24 AM Andrii Nakryiko
> > > <andrii.nakryiko@...il.com> wrote:
> > > >
> > > > On Wed, Jan 12, 2022 at 10:18 AM Lorenzo Bianconi <lorenzo@...nel.org> wrote:
> > > > >
> > > > > > On Sun, Jan 9, 2022 at 7:05 AM Lorenzo Bianconi <lorenzo@...nel.org> wrote:
> > > > > > >
> > > > > > > Introduce support for the following SEC entries for XDP multi-buff
> > > > > > > property:
> > > > > > > - SEC("xdp_mb/")
> > > > > > > - SEC("xdp_devmap_mb/")
> > > > > > > - SEC("xdp_cpumap_mb/")
> > > > > >
> > > > > > Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for
> > > > > > sleepable, seems like we'll have kprobe.multi or something along
> > > > > > those lines as well), so let's stay consistent and call this "xdp_mb",
> > > > > > "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that
> > > > > > recognizable? would ".multibuf" be too verbose?). Also, why the "/"
> > > > > > part? Also it shouldn't be "sloppy" either. Neither expected attach
> > > > > > type should be optional. Also not sure SEC_ATTACHABLE is needed. So
> > > > > > at most it should be SEC_XDP_MB, probably.
> > > > >
> > > > > ack, I fine with it. Something like:
> > > > >
> > > > > SEC_DEF("lsm.s/", LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
> > > > > SEC_DEF("iter/", TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
> > > > > SEC_DEF("syscall", SYSCALL, 0, SEC_SLEEPABLE),
> > > > > + SEC_DEF("xdp_devmap.multibuf", XDP, BPF_XDP_DEVMAP, 0),
> > > > > SEC_DEF("xdp_devmap/", XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
> > > > > + SEC_DEF("xdp_cpumap.multibuf", XDP, BPF_XDP_CPUMAP, 0),
> > > > > SEC_DEF("xdp_cpumap/", XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
> > > > > + SEC_DEF("xdp.multibuf", XDP, BPF_XDP, 0),
> > > >
> > > > yep, but please use SEC_NONE instead of zero
> > > >
> > > > > SEC_DEF("xdp", XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
> > > > > SEC_DEF("perf_event", PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
> > > > > SEC_DEF("lwt_in", LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Acked-by: Toke Hoiland-Jorgensen <toke@...hat.com>
> > > > > > > Acked-by: John Fastabend <john.fastabend@...il.com>
> > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> > > > > > > ---
> > > > > > > tools/lib/bpf/libbpf.c | 8 ++++++++
> > > > > > > 1 file changed, 8 insertions(+)
> > > > > > >
> > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > > > > index 7f10dd501a52..c93f6afef96c 100644
> > > > > > > --- a/tools/lib/bpf/libbpf.c
> > > > > > > +++ b/tools/lib/bpf/libbpf.c
> > > > > > > @@ -235,6 +235,8 @@ enum sec_def_flags {
> > > > > > > SEC_SLEEPABLE = 8,
> > > > > > > /* allow non-strict prefix matching */
> > > > > > > SEC_SLOPPY_PFX = 16,
> > > > > > > + /* BPF program support XDP multi-buff */
> > > > > > > + SEC_XDP_MB = 32,
> > > > > > > };
> > > > > > >
> > > > > > > struct bpf_sec_def {
> > > > > > > @@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
> > > > > > > if (def & SEC_SLEEPABLE)
> > > > > > > opts->prog_flags |= BPF_F_SLEEPABLE;
> > > > > > >
> > > > > > > + if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
> > > > > > > + opts->prog_flags |= BPF_F_XDP_MB;
> > > > > >
> > > > > > I'd say you don't even need SEC_XDP_MB flag at all, you can just check
> > > > > > that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or
> > > > > > "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic
> > > > > > enough to warrant a flag.
> > > > >
> > > > > ack, something like:
> > > > >
> > > > > + if (prog->type == BPF_PROG_TYPE_XDP &&
> > > > > + (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
> > > > > + !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||
> > > > > + !strcmp(prog->sec_name, "xdp.multibuf")))
> > > > > + opts->prog_flags |= BPF_F_XDP_MB;
> > > >
> > > > yep, can also simplify it a bit with strstr(prog->sec_name,
> > > > ".multibuf") instead of three strcmp
> > >
> > > Maybe ".mb" ?
> > > ".multibuf" is too verbose.
> > > We're fine with ".s" for sleepable :)
> >
> >
> > I had reservations about "mb" because the first and strong association
> > is "megabyte", not "multibuf". And it's not like anyone would have
> > tens of those programs in a single file so that ".multibuf" becomes
> > way too verbose. But I don't feel too strongly about this, if the
> > consensus is on ".mb".
>
> The rest of the patches are using _mb everywhere.
> I would keep libbpf consistent.
Should the rest of the patches maybe use "multibuf" instead of "mb"? I've been
following this patch series closely and excitedly, and I keep having to remind
myself that "mb" is "multibuff" and not "megabyte". If I'm having to correct
myself while following the patch series, I'm wondering if future confusion is
inevitable?
But, is it enough confusion to be worth updating many other patches? I'm not
sure.
I agree consistency is more important than the specific term we're consistent
on.
--Zvi
Powered by blists - more mailing lists