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] [day] [month] [year] [list]

Date:   Wed, 3 Jun 2020 14:22:42 -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:
> ISO C forbids zero-size arrays, unnamed struct/union, gcc extensions,
> empty unions, etc
> So ?

So their use should be avoided in UAPI headers whenever possible.
While the other extensions are simple and have clear semantics, enums
with out-of-range values do not. Even gcc and clang don't agree on
what the types of out-of-range constants are within the enum
specifier:

	enum { A = 0x80000000ULL, is_clang =
__builtin_types_compatible_p(__typeof__(A), unsigned long long) };

On gcc x86_64, is_clang == 0. On clang x86_64, is_clang == 1.

> #define BPF_F_CTXLEN_MASK BPF_F_CTXLEN_MASK
> will only work as workaround for _your_ compiler.
> We are not gonna add hacks like this for every compiler.

This doesn't solve the problem, which is that after preprocessing,
BPF_F_INDEX_MASK and BPF_F_CURRENT_CPU, and BPF_F_CTXLEN_MASK are enum
constants with values that can't be represented as int. Changing them
back to defines will. This is not a hack, it is following the C
standard.

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

My point is that something like

	unsigned long long x = BPF_DEVCG_DEV_BLOCK << 32;

used to be perfectly fine in Linux 5.6. Now, in 5.7 with the enum
definition, it is undefined behavior.

-Michael