Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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. 



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux