| Message ID | 20230619091215.2731541-3-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/3,v2] sfc: add CONFIG_INET dependency for TC offload | expand |
On 19/06/2023 10:12, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Three of the sfc drivers define a packed loopback_payload structure with an > ethernet header followed by an IP header. However, the kernel definition > of iphdr specifies that this is 4-byte aligned, causing a W=1 warning: > > net/ethernet/sfc/siena/selftest.c:46:15: error: field ip within 'struct efx_loopback_payload' is less aligned than 'struct iphdr' and is usually due to 'struct efx_loopback_payload' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access] > struct iphdr ip; > > As the iphdr packing is not easily changed without breaking other code, > change the three structures to use a local definition instead. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Duplicating the definition isn't the prettiest thing in the world; it'd do for a quick fix if needed but I assume W=1 warnings aren't blocking anyone, so maybe defer this one for now and I'll follow up soon with a rewrite that fixes this more cleanly? My idea is to drop the __packed from the containing struct, make efx_begin_loopback() copy the layers separately, and efx_loopback_rx_packet() similarly do something less direct than casting the packet data to the struct. But I don't insist on it; if you want this fix in immediately then I'm okay with that too. > --- > drivers/net/ethernet/sfc/falcon/selftest.c | 21 ++++++++++++++++++++- > drivers/net/ethernet/sfc/selftest.c | 21 ++++++++++++++++++++- > drivers/net/ethernet/sfc/siena/selftest.c | 21 ++++++++++++++++++++- > 3 files changed, 60 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/falcon/selftest.c b/drivers/net/ethernet/sfc/falcon/selftest.c > index 6a454ac6f8763..fb7fcd27a33a5 100644 > --- a/drivers/net/ethernet/sfc/falcon/selftest.c > +++ b/drivers/net/ethernet/sfc/falcon/selftest.c > @@ -40,7 +40,26 @@ > */ > struct ef4_loopback_payload { > struct ethhdr header; > - struct iphdr ip; > + struct { > +#if defined(__LITTLE_ENDIAN_BITFIELD) > + __u8 ihl:4, > + version:4; > +#elif defined (__BIG_ENDIAN_BITFIELD) > + __u8 version:4, > + ihl:4; > +#else > +#error "Please fix <asm/byteorder.h>" > +#endif > + __u8 tos; > + __be16 tot_len; > + __be16 id; > + __be16 frag_off; > + __u8 ttl; > + __u8 protocol; > + __sum16 check; > + __be32 saddr; > + __be32 daddr; > + } __packed ip; /* unaligned struct iphdr */ > struct udphdr udp; > __be16 iteration; > char msg[64]; > diff --git a/drivers/net/ethernet/sfc/selftest.c b/drivers/net/ethernet/sfc/selftest.c > index 3c5227afd4977..440a57953779c 100644 > --- a/drivers/net/ethernet/sfc/selftest.c > +++ b/drivers/net/ethernet/sfc/selftest.c > @@ -43,7 +43,26 @@ > */ > struct efx_loopback_payload { > struct ethhdr header; > - struct iphdr ip; > + struct { > +#if defined(__LITTLE_ENDIAN_BITFIELD) > + __u8 ihl:4, > + version:4; > +#elif defined (__BIG_ENDIAN_BITFIELD) > + __u8 version:4, > + ihl:4; > +#else > +#error "Please fix <asm/byteorder.h>" > +#endif > + __u8 tos; > + __be16 tot_len; > + __be16 id; > + __be16 frag_off; > + __u8 ttl; > + __u8 protocol; > + __sum16 check; > + __be32 saddr; > + __be32 daddr; > + } __packed ip; /* unaligned struct iphdr */ > struct udphdr udp; > __be16 iteration; > char msg[64]; > diff --git a/drivers/net/ethernet/sfc/siena/selftest.c b/drivers/net/ethernet/sfc/siena/selftest.c > index 07715a3d6beab..b8a8b0495f661 100644 > --- a/drivers/net/ethernet/sfc/siena/selftest.c > +++ b/drivers/net/ethernet/sfc/siena/selftest.c > @@ -43,7 +43,26 @@ > */ > struct efx_loopback_payload { > struct ethhdr header; > - struct iphdr ip; > + struct { > +#if defined(__LITTLE_ENDIAN_BITFIELD) > + __u8 ihl:4, > + version:4; > +#elif defined (__BIG_ENDIAN_BITFIELD) > + __u8 version:4, > + ihl:4; > +#else > +#error "Please fix <asm/byteorder.h>" > +#endif > + __u8 tos; > + __be16 tot_len; > + __be16 id; > + __be16 frag_off; > + __u8 ttl; > + __u8 protocol; > + __sum16 check; > + __be32 saddr; > + __be32 daddr; > + } __packed ip; /* unaligned struct iphdr */ > struct udphdr udp; > __be16 iteration; > char msg[64]; >
On Mon, Jun 19, 2023, at 12:25, Edward Cree wrote: > On 19/06/2023 10:12, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> Three of the sfc drivers define a packed loopback_payload structure with an >> ethernet header followed by an IP header. However, the kernel definition >> of iphdr specifies that this is 4-byte aligned, causing a W=1 warning: >> >> net/ethernet/sfc/siena/selftest.c:46:15: error: field ip within 'struct efx_loopback_payload' is less aligned than 'struct iphdr' and is usually due to 'struct efx_loopback_payload' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access] >> struct iphdr ip; >> >> As the iphdr packing is not easily changed without breaking other code, >> change the three structures to use a local definition instead. >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Duplicating the definition isn't the prettiest thing in the world; it'd > do for a quick fix if needed but I assume W=1 warnings aren't blocking > anyone, so maybe defer this one for now and I'll follow up soon with a > rewrite that fixes this more cleanly? My idea is to drop the __packed > from the containing struct, make efx_begin_loopback() copy the layers > separately, and efx_loopback_rx_packet() similarly do something less > direct than casting the packet data to the struct. > > But I don't insist on it; if you want this fix in immediately then I'm > okay with that too. > >> --- >> drivers/net/ethernet/sfc/falcon/selftest.c | 21 ++++++++++++++++++++- >> drivers/net/ethernet/sfc/selftest.c | 21 ++++++++++++++++++++- >> drivers/net/ethernet/sfc/siena/selftest.c | 21 ++++++++++++++++++++- >> 3 files changed, 60 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/sfc/falcon/selftest.c b/drivers/net/ethernet/sfc/falcon/selftest.c >> index 6a454ac6f8763..fb7fcd27a33a5 100644 >> --- a/drivers/net/ethernet/sfc/falcon/selftest.c >> +++ b/drivers/net/ethernet/sfc/falcon/selftest.c >> @@ -40,7 +40,26 @@ >> */ >> struct ef4_loopback_payload { >> struct ethhdr header; >> - struct iphdr ip; >> + struct { >> +#if defined(__LITTLE_ENDIAN_BITFIELD) >> + __u8 ihl:4, >> + version:4; >> +#elif defined (__BIG_ENDIAN_BITFIELD) >> + __u8 version:4, >> + ihl:4; >> +#else >> +#error "Please fix <asm/byteorder.h>" >> +#endif >> + __u8 tos; >> + __be16 tot_len; >> + __be16 id; >> + __be16 frag_off; >> + __u8 ttl; >> + __u8 protocol; >> + __sum16 check; >> + __be32 saddr; >> + __be32 daddr; >> + } __packed ip; /* unaligned struct iphdr */ >> struct udphdr udp; >> __be16 iteration; >> char msg[64]; >> diff --git a/drivers/net/ethernet/sfc/selftest.c b/drivers/net/ethernet/sfc/selftest.c >> index 3c5227afd4977..440a57953779c 100644 >> --- a/drivers/net/ethernet/sfc/selftest.c >> +++ b/drivers/net/ethernet/sfc/selftest.c >> @@ -43,7 +43,26 @@ >> */ >> struct efx_loopback_payload { >> struct ethhdr header; >> - struct iphdr ip; >> + struct { >> +#if defined(__LITTLE_ENDIAN_BITFIELD) >> + __u8 ihl:4, >> + version:4; >> +#elif defined (__BIG_ENDIAN_BITFIELD) >> + __u8 version:4, >> + ihl:4; >> +#else >> +#error "Please fix <asm/byteorder.h>" >> +#endif >> + __u8 tos; >> + __be16 tot_len; >> + __be16 id; >> + __be16 frag_off; >> + __u8 ttl; >> + __u8 protocol; >> + __sum16 check; >> + __be32 saddr; >> + __be32 daddr; >> + } __packed ip; /* unaligned struct iphdr */ >> struct udphdr udp; >> __be16 iteration; >> char msg[64]; >> diff --git a/drivers/net/ethernet/sfc/siena/selftest.c b/drivers/net/ethernet/sfc/siena/selftest.c >> index 07715a3d6beab..b8a8b0495f661 100644 >> --- a/drivers/net/ethernet/sfc/siena/selftest.c >> +++ b/drivers/net/ethernet/sfc/siena/selftest.c >> @@ -43,7 +43,26 @@ >> */ >> struct efx_loopback_payload { >> struct ethhdr header; >> - struct iphdr ip; >> + struct { >> +#if defined(__LITTLE_ENDIAN_BITFIELD) >> + __u8 ihl:4, >> + version:4; >> +#elif defined (__BIG_ENDIAN_BITFIELD) >> + __u8 version:4, >> + ihl:4; >> +#else >> +#error "Please fix <asm/byteorder.h>" >> +#endif >> + __u8 tos; >> + __be16 tot_len; >> + __be16 id; >> + __be16 frag_off; >> + __u8 ttl; >> + __u8 protocol; >> + __sum16 check; >> + __be32 saddr; >> + __be32 daddr; >> + } __packed ip; /* unaligned struct iphdr */ >> struct udphdr udp; >> __be16 iteration; >> char msg[64]; >>
On Mon, Jun 19, 2023, at 12:25, Edward Cree wrote: > On 19/06/2023 10:12, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> Three of the sfc drivers define a packed loopback_payload structure with an >> ethernet header followed by an IP header. However, the kernel definition >> of iphdr specifies that this is 4-byte aligned, causing a W=1 warning: >> >> net/ethernet/sfc/siena/selftest.c:46:15: error: field ip within 'struct efx_loopback_payload' is less aligned than 'struct iphdr' and is usually due to 'struct efx_loopback_payload' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access] >> struct iphdr ip; >> >> As the iphdr packing is not easily changed without breaking other code, >> change the three structures to use a local definition instead. >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Duplicating the definition isn't the prettiest thing in the world; it'd > do for a quick fix if needed but I assume W=1 warnings aren't blocking > anyone, so maybe defer this one for now and I'll follow up soon with a > rewrite that fixes this more cleanly? My idea is to drop the __packed > from the containing struct, make efx_begin_loopback() copy the layers > separately, and efx_loopback_rx_packet() similarly do something less > direct than casting the packet data to the struct. > > But I don't insist on it; if you want this fix in immediately then I'm > okay with that too. It's not urgent, if you can come up with a nicer solution, that is probably better than applying my patch now. I have a patch [1] that addresses -Wunaligned-access for all x86 and arm randconfig builds, and this currently affects 23 drivers. Most of the changes don't have changelog texts yet, and some need a more detailed analysis to come up with a correct patch. I'd probably aim for linux-6.6 or later to get them all done, at which point we could move the warning from W=1 to the default set. Arnd [1] https://pastebin.com/g6D9NTS4
... > Duplicating the definition isn't the prettiest thing in the world; it'd > do for a quick fix if needed but I assume W=1 warnings aren't blocking > anyone, so maybe defer this one for now and I'll follow up soon with a > rewrite that fixes this more cleanly? My idea is to drop the __packed > from the containing struct, make efx_begin_loopback() copy the layers > separately, and efx_loopback_rx_packet() similarly do something less > direct than casting the packet data to the struct. Maybe you can get away with adding a 16bit pad before the ethernet header so that the IP header is actually aligned. (Then fight all the stuff that stops you doing a memcpy() that runs into a second field of a structure.) Failing that maybe a single shared copy of the misaligned IP header. I also suspect you could just add __packed to the two 32bit address fields. That would generate better code on systems that care. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 23/06/2023 11:52, David Laight wrote: > Maybe you can get away with adding a 16bit pad before the ethernet > header so that the IP header is actually aligned. That's what I ended up doing, because my original idea was overcomplicated and turned out super ugly. See https://lore.kernel.org/netdev/6f87fdf5-1844-4633-b4fe-6b247bc6ab49@app.fastmail.com/T/ > (Then fight all the stuff that stops you doing a memcpy() > that runs into a second field of a structure.) Yeah, I don't know how you're meant to annotate that stuff. I guess I'll have to wait until Kees shouts at me and tells me what to do :S -ed
diff --git a/drivers/net/ethernet/sfc/falcon/selftest.c b/drivers/net/ethernet/sfc/falcon/selftest.c index 6a454ac6f8763..fb7fcd27a33a5 100644 --- a/drivers/net/ethernet/sfc/falcon/selftest.c +++ b/drivers/net/ethernet/sfc/falcon/selftest.c @@ -40,7 +40,26 @@ */ struct ef4_loopback_payload { struct ethhdr header; - struct iphdr ip; + struct { +#if defined(__LITTLE_ENDIAN_BITFIELD) + __u8 ihl:4, + version:4; +#elif defined (__BIG_ENDIAN_BITFIELD) + __u8 version:4, + ihl:4; +#else +#error "Please fix <asm/byteorder.h>" +#endif + __u8 tos; + __be16 tot_len; + __be16 id; + __be16 frag_off; + __u8 ttl; + __u8 protocol; + __sum16 check; + __be32 saddr; + __be32 daddr; + } __packed ip; /* unaligned struct iphdr */ struct udphdr udp; __be16 iteration; char msg[64]; diff --git a/drivers/net/ethernet/sfc/selftest.c b/drivers/net/ethernet/sfc/selftest.c index 3c5227afd4977..440a57953779c 100644 --- a/drivers/net/ethernet/sfc/selftest.c +++ b/drivers/net/ethernet/sfc/selftest.c @@ -43,7 +43,26 @@ */ struct efx_loopback_payload { struct ethhdr header; - struct iphdr ip; + struct { +#if defined(__LITTLE_ENDIAN_BITFIELD) + __u8 ihl:4, + version:4; +#elif defined (__BIG_ENDIAN_BITFIELD) + __u8 version:4, + ihl:4; +#else +#error "Please fix <asm/byteorder.h>" +#endif + __u8 tos; + __be16 tot_len; + __be16 id; + __be16 frag_off; + __u8 ttl; + __u8 protocol; + __sum16 check; + __be32 saddr; + __be32 daddr; + } __packed ip; /* unaligned struct iphdr */ struct udphdr udp; __be16 iteration; char msg[64]; diff --git a/drivers/net/ethernet/sfc/siena/selftest.c b/drivers/net/ethernet/sfc/siena/selftest.c index 07715a3d6beab..b8a8b0495f661 100644 --- a/drivers/net/ethernet/sfc/siena/selftest.c +++ b/drivers/net/ethernet/sfc/siena/selftest.c @@ -43,7 +43,26 @@ */ struct efx_loopback_payload { struct ethhdr header; - struct iphdr ip; + struct { +#if defined(__LITTLE_ENDIAN_BITFIELD) + __u8 ihl:4, + version:4; +#elif defined (__BIG_ENDIAN_BITFIELD) + __u8 version:4, + ihl:4; +#else +#error "Please fix <asm/byteorder.h>" +#endif + __u8 tos; + __be16 tot_len; + __be16 id; + __be16 frag_off; + __u8 ttl; + __u8 protocol; + __sum16 check; + __be32 saddr; + __be32 daddr; + } __packed ip; /* unaligned struct iphdr */ struct udphdr udp; __be16 iteration; char msg[64];