netdev - Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200602230720.hf2ysnlssg67cpmw@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 2 Jun 2020 16:07:20 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Michael Forney <mforney@...rney.org>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Kernel Team <kernel-team@...com>, Yonghong Song <yhs@...com>
Subject: Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants
 used from BPF program side to enums

On Tue, Jun 02, 2020 at 02:40:52PM -0700, Michael Forney wrote:
> On 2020-06-02, Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> > It's possible, but I'm not sure what it will fix.
> > Your example is a bit misleading, since it's talking about B
> > which doesn't have type specifier, whereas enums in bpf.h have ULL
> > suffix where necessary.
> > And the one you pointed out BPF_F_CTXLEN_MASK has sizeof == 8 in all cases.
> 
> Apologies if I wasn't clear, I was just trying to explain why this C
> extension can have confusing semantics where the type of an enum
> constant depends on where it is used. You're right that it doesn't
> happen in this particular case.
> 
> The breakage appears with my C compiler, which as I mentioned, only
> implements the extension when the enum constants fit into unsigned int
> to avoid these problems.
> 
> $ cproc -x c -c - -o /dev/null <<EOF
> > #include <linux/bpf.h>
> > EOF
> <stdin>:420:41: error: enumerator 'BPF_F_CTXLEN_MASK' value cannot be
> represented as 'int' or 'unsigned int'
> cproc: compile: process 3772 exited with status 1
> cproc: preprocess: process signaled: Terminated
> cproc: assemble: process signaled: Terminated
> $
> 
> Since the Linux UAPI headers may be used with a variety of compilers,
> I think it's important to stick to the standard as much as possible.
> BPF_F_CTXLEN_MASK is the only enum constant I've encountered in the
> Linux UAPI that has a value outside the range of unsigned int.

the enum definition of BPF_F_CTXLEN_MASK is certainly within standard.
I don't think kernel should adjust its headers because some compiler
is failing to understand C standard.

> > Also when B is properly annotated like 0x80000000ULL it will have size 8
> > as well.
> 
> Even with a suffixed integer literal, it still may be the case that an
> annotated constant has a different type inside and outside the enum.
> 
> For example, in
> 
> 	enum {
> 		A = 0x80000000ULL,
> 		S1 = sizeof(A),
> 	};
> 	enum {
> 		S2 = sizeof(A),
> 	};
> 
> we have S1 == 8 and S2 == 4.

correct, because enum needs to fit all of its constants.
It's not at all related to size.
sizeof() is an expression that is being evulated in the context and
affects the size of enum.
It could have been any other math operation on constants known
at compile time.

> 
> >> Also, I'm not sure if it was considered, but using enums also changes
> >> the signedness of these constants. Many of the previous macro
> >> expressions had type unsigned long long, and now they have type int
> >> (the type of the expression specifying the constant value does not
> >> matter). I could see this causing problems if these constants are used
> >> in expressions involving shifts or implicit conversions.
> >
> > It would have been if the enums were not annotated. But that's not the case.
> 
> The type of the expression has no relation to the type of the constant
> outside the enum. Take this example from bpf.h:
> 
> 	enum {
> 		BPF_DEVCG_DEV_BLOCK     = (1ULL << 0),
> 	 	BPF_DEVCG_DEV_CHAR      = (1ULL << 1),
> 	};
> 
> Previously, with the defines, they had type unsigned long long. Now,
> they have type int. sizeof(BPF_DEVCG_DEV_BLOCK) == 4 and
> -BPF_DEVCG_DEV_BLOCK < 0 == 1.

and I still don't see how it breaks anything.
If it was #define and .h switched its definition from 1 to 1ULL
it would have had the same effect. That is the point of the constant in .h.
Same effect regardless whether it was done via #define and via enum.
The size and type of the constant may change. It's not uapi breakage.