On 4/3/24 10:09 PM, Arnd Bergmann wrote:
On Wed, Apr 3, 2024, at 14:33, Anton Protopopov wrote:
The struct bpf_fib_lookup is supposed to be of size 64. A recent commit
59b418c7063d ("bpf: Add a check for struct bpf_fib_lookup size") added
a static assertion to check this property so that future changes to the
structure will not accidentally break this assumption.
As it immediately turned out, on some 32-bit arm systems, when AEABI=n,
the total size of the structure was equal to 68, see [1]. This happened
because the bpf_fib_lookup structure contains a union of two 16-bit
fields:
union {
__u16 tot_len;
__u16 mtu_result;
};
This union was introduced three years ago, so fixing it now means
another incompatible ABI change. While you clearly change it back
to what it should have been the entire time, it's not obvious that
this is the correct thing to do after three years.
I do wonder to what degree we still want to care about OABI
at all. As I mentioned, I stopped testing it myself because it
is no longer relevant to most people, and there are probably
a number of other ABI issues.
which was supposed to compile to a 16-bit-aligned 16-bit field. On the
aforementioned setups it was instead both aligned and padded to 32-bits.
Declare this inner union as __attribute__((packed, aligned(2))) such
that it always is of size 2 and is aligned to 16 bits.
I think you probably want 32-bit alignment for the structure,
to keep the ABI unchanged on all other architectures.
Fwiw, on x86 nothing should change on this regard, see below pahole dump
before/after. I think similar might be true for other archs as otherwise
we should have seen a kbuild bot complaint on hitting the size assert.
So it seems at this point only for the reported case of AEABI=n ... if it
is no longer relevant to most people, I'd think the likelihood of such
users and users who utilize this specific helper is even smaller. But we
could queue it to bpf-next to give it some time.
Before patch:
struct bpf_fib_lookup {
__u8 family; /* 0 1 */
__u8 l4_protocol; /* 1 1 */
__be16 sport; /* 2 2 */
__be16 dport; /* 4 2 */
union {
__u16 tot_len; /* 6 2 */
__u16 mtu_result; /* 6 2 */
}; /* 6 2 */
__u32 ifindex; /* 8 4 */
union {
__u8 tos; /* 12 1 */
__be32 flowinfo; /* 12 4 */
__u32 rt_metric; /* 12 4 */
}; /* 12 4 */
union {
__be32 ipv4_src; /* 16 4 */
__u32 ipv6_src[4]; /* 16 16 */
}; /* 16 16 */
union {
__be32 ipv4_dst; /* 32 4 */
__u32 ipv6_dst[4]; /* 32 16 */
}; /* 32 16 */
union {
struct {
__be16 h_vlan_proto; /* 48 2 */
__be16 h_vlan_TCI; /* 50 2 */
}; /* 48 4 */
__u32 tbid; /* 48 4 */
}; /* 48 4 */
__u8 smac[6]; /* 52 6 */
__u8 dmac[6]; /* 58 6 */
/* size: 64, cachelines: 1, members: 12 */
};
After patch:
struct bpf_fib_lookup {
__u8 family; /* 0 1 */
__u8 l4_protocol; /* 1 1 */
__be16 sport; /* 2 2 */
__be16 dport; /* 4 2 */
union {
__u16 tot_len; /* 6 2 */
__u16 mtu_result; /* 6 2 */
} __attribute__((__aligned__(2))); /* 6 2 */
__u32 ifindex; /* 8 4 */
union {
__u8 tos; /* 12 1 */
__be32 flowinfo; /* 12 4 */
__u32 rt_metric; /* 12 4 */
}; /* 12 4 */
union {
__be32 ipv4_src; /* 16 4 */
__u32 ipv6_src[4]; /* 16 16 */
}; /* 16 16 */
union {
__be32 ipv4_dst; /* 32 4 */
__u32 ipv6_dst[4]; /* 32 16 */
}; /* 32 16 */
union {
struct {
__be16 h_vlan_proto; /* 48 2 */
__be16 h_vlan_TCI; /* 50 2 */
}; /* 48 4 */
__u32 tbid; /* 48 4 */
}; /* 48 4 */
__u8 smac[6]; /* 52 6 */
__u8 dmac[6]; /* 58 6 */
/* size: 64, cachelines: 1, members: 12 */
/* forced alignments: 1 */
} __attribute__((__aligned__(4)));
Thanks,
Daniel
