On Mon, Jun 01, 2020 at 10:31:34PM -0700, Michael Forney wrote:
> Hi,
>
> On 2020-03-04, Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> > I was about to push the series out, but agree that there may be a risk for
> > #ifndefs
> > in the BPF C code. If we want to be on safe side, #define FOO FOO would be
> > needed.
>
> I did indeed hit some breakage due to this change, but not for the
> anticipated reason.
>
> The C standard requires that enumeration constants be representable as
> an int, and have type int. While it is a common extension to allow
> constants that exceed the limits of int, and this is required
> elsewhere in Linux UAPI headers, this is the first case I've
> encountered where the constant is not representable as unsigned int
> either:
>
> enum {
> BPF_F_CTXLEN_MASK = (0xfffffULL << 32),
> };
>
> To see why this can be problematic, consider the following program:
>
> #include <stdio.h>
>
> enum {
> A = 1,
> B = 0x80000000,
> C = 1ULL << 32,
>
> A1 = sizeof(A),
> B1 = sizeof(B),
> };
>
> enum {
> A2 = sizeof(A),
> B2 = sizeof(B),
> };
>
> int main(void) {
> printf("sizeof(A) = %d, %d\n", (int)A1, (int)A2);
> printf("sizeof(B) = %d, %d\n", (int)B1, (int)B2);
> }
>
> You might be surprised by the output:
>
> sizeof(A) = 4, 4
> sizeof(B) = 4, 8
>
> This is because the type of B is different inside and outside the
> enum. In my C compiler, I have implemented the extension only for
> constants that fit in unsigned int to avoid these confusing semantics.
>
> Since BPF_F_CTXLEN_MASK is the only offending constant, is it possible
> to restore its definition as a macro?
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.
Also when B is properly annotated like 0x80000000ULL it will have size 8
as well.
#include <stdio.h>
enum {
A = 1,
B = 0x80000000,
C = 1ULL << 32,
D = 0x80000000ULL,
A1 = sizeof(A),
B1 = sizeof(B),
C1 = sizeof(C),
D1 = sizeof(D),
};
enum {
A2 = sizeof(A),
B2 = sizeof(B),
C2 = sizeof(C),
D2 = sizeof(D),
};
int main(void) {
printf("sizeof(A) = %d, %d\n", (int)A1, (int)A2);
printf("sizeof(B) = %d, %d\n", (int)B1, (int)B2);
printf("sizeof(C) = %d, %d\n", (int)C1, (int)C2);
printf("sizeof(D) = %d, %d\n", (int)D1, (int)D2);
}
sizeof(A) = 4, 4
sizeof(B) = 4, 8
sizeof(C) = 8, 8
sizeof(D) = 8, 8
So the problem is only with non-annotated enums that are mixed
in a enum with some values <32bit and others >32 bit.
bpf.h has only one such enum:
enum {
BPF_F_INDEX_MASK = 0xffffffffULL,
BPF_F_CURRENT_CPU = BPF_F_INDEX_MASK,
BPF_F_CTXLEN_MASK = (0xfffffULL << 32),
};
and all values are annotated with ULL.
So I really don't see a problem.
> 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.
