netdev - Re: [PATCH v2 bpf-next 1/3] bpf: switch BPF UAPI #define constants to enums

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: <CAEf4BzYWCyLBNzH9ns-jP7SFeOpGfLbypr7VRhDPSTOMA0nyjA@mail.gmail.com>
Date:   Mon, 2 Mar 2020 15:14:10 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 1/3] bpf: switch BPF UAPI #define constants to enums

On Mon, Mar 2, 2020 at 2:37 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Sat, Feb 29, 2020 at 10:24:03PM -0800, Andrii Nakryiko wrote:
> > Switch BPF UAPI constants, previously defined as #define macro, to anonymous
> > enum values. This preserves constants values and behavior in expressions, but
> > has added advantaged of being captured as part of DWARF and, subsequently, BTF
> > type info. Which, in turn, greatly improves usefulness of generated vmlinux.h
> > for BPF applications, as it will not require BPF users to copy/paste various
> > flags and constants, which are frequently used with BPF helpers.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@...com>
> > ---
> >  include/uapi/linux/bpf.h              | 272 +++++++++++++++----------
> >  include/uapi/linux/bpf_common.h       |  86 ++++----
> >  include/uapi/linux/btf.h              |  60 +++---
> >  tools/include/uapi/linux/bpf.h        | 274 ++++++++++++++++----------
> >  tools/include/uapi/linux/bpf_common.h |  86 ++++----
> >  tools/include/uapi/linux/btf.h        |  60 +++---
> >  6 files changed, 497 insertions(+), 341 deletions(-)
>
> I see two reasons why converting #define to enum is useful:
> 1. bpf progs can use them from vmlinux.h as evident in patch 3.
> 2. "bpftool feature probe" can be replaced with
>   bpftool btf dump file /sys/kernel/btf/vmlinux |grep BPF_CGROUP_SETSOCKOPT
>
> The second use case is already possible, since bpf_prog_type,
> bpf_attach_type, bpf_cmd, bpf_func_id are all enums.
> So kernel is already self describing most bpf features.
> Does kernel support bpf_probe_read_user() ? Answer is:
> bpftool btf dump file /sys/kernel/btf/vmlinux | grep BPF_FUNC_probe_read_user
>
> The only bit missing is supported kernel flags and instructions.

Yep, my motivation was primarily the former, but I can see benefits
from the latter as well.

>
> I think for now I would only convert flags that are going to be
> used from bpf program and see whether 1st use case works well.
> Later we can convert flags that are used out of user space too.
>
> In other words:
>
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 8e98ced0963b..03e08f256bd1 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -14,34 +14,36 @@
> >  /* Extended instruction set based on top of classic BPF */
> >
> >  /* instruction classes */
> > -#define BPF_JMP32    0x06    /* jmp mode in word width */
> > -#define BPF_ALU64    0x07    /* alu mode in double word width */
> > +enum {
> > +     BPF_JMP32       = 0x06, /* jmp mode in word width */
> > +     BPF_ALU64       = 0x07, /* alu mode in double word width */
>
> not those.

makes sense

>
> > -#define BPF_F_ALLOW_OVERRIDE (1U << 0)
> > -#define BPF_F_ALLOW_MULTI    (1U << 1)
> > -#define BPF_F_REPLACE                (1U << 2)
> > +enum {
> > +     BPF_F_ALLOW_OVERRIDE    = (1U << 0),
> > +     BPF_F_ALLOW_MULTI       = (1U << 1),
> > +     BPF_F_REPLACE           = (1U << 2),
> > +};
>
> not those either. These are the flags for user space. Not for the prog.

yep...

>
> >  /* flags for BPF_MAP_UPDATE_ELEM command */
> > -#define BPF_ANY              0 /* create new element or update existing */
> > -#define BPF_NOEXIST  1 /* create new element if it didn't exist */
> > -#define BPF_EXIST    2 /* update existing element */
> > -#define BPF_F_LOCK   4 /* spin_lock-ed map_lookup/map_update */
> > +enum {
> > +     BPF_ANY         = 0, /* create new element or update existing */
> > +     BPF_NOEXIST     = 1, /* create new element if it didn't exist */
> > +     BPF_EXIST       = 2, /* update existing element */
> > +     BPF_F_LOCK      = 4, /* spin_lock-ed map_lookup/map_update */
> > +};
>
> yes to these.

yep, these and below are the most important ones...

[...]

>
> In all such cases I don't think we need #define FOO FOO
> trick. These are the flags used within bpf program.
> I don't think any user is doing #ifdef logic there.
> I cannot come up with a use case of anything useful this way.

Sounds good, I'll revert non-BPF helper flags cases and will post v2, thanks!