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: <CAGw6cBstsD40MMoHg2dGUe7YvR5KdHD8BqQ5xeXoYKLCUFAudg@mail.gmail.com>
Date:   Tue, 2 Jun 2020 14:40:52 -0700
From:   Michael Forney <mforney@...rney.org>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
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 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.

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

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

-Michael