| Message ID | dfe2eb3d6ad3204079df63ae123b82d49b0c90e2.1687545312.git.ecree.xilinx@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cf60ed469629927fe43c2f4b4ef28a563d991935 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: fix unaligned access in loopback selftests | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | warning | WARNING: quoted string split across lines |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Jun 23, 2023, at 20:38, edward.cree@amd.com wrote: > From: Edward Cree <ecree.xilinx@gmail.com> > > Add two bytes of padding to the start of struct efx_loopback_payload, > which are not sent on the wire. This ensures the 'ip' member is > 4-byte aligned, preventing the following W=1 warning: > net/ethernet/sfc/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; > > Reported-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Edward Cree <ecree.xilinx@gmail.com> > --- > drivers/net/ethernet/sfc/selftest.c | 47 +++++++++++++++++------------ > 1 file changed, 28 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/selftest.c > b/drivers/net/ethernet/sfc/selftest.c > index 3c5227afd497..96d856b9043c 100644 > --- a/drivers/net/ethernet/sfc/selftest.c > +++ b/drivers/net/ethernet/sfc/selftest.c > @@ -42,12 +42,16 @@ > * Falcon only performs RSS on TCP/UDP packets. > */ > struct efx_loopback_payload { > + char pad[2]; /* Ensures ip is 4-byte aligned */ > struct ethhdr header; > struct iphdr ip; > struct udphdr udp; > __be16 iteration; > char msg[64]; > -} __packed; > +} __packed __aligned(4); Unfortunately, the same warning now came back after commit 55c1528f9b97f ("sfc: fix field-spanning memcpy in selftest"), which does: struct efx_loopback_payload { char pad[2]; /* Ensures ip is 4-byte aligned */ - struct ethhdr header; - struct iphdr ip; - struct udphdr udp; - __be16 iteration; - char msg[64]; + struct_group_attr(packet, __packed, + struct ethhdr header; + struct iphdr ip; + struct udphdr udp; + __be16 iteration; + char msg[64]; + ); } __packed __aligned(4); I'm out of ideas for how to fix both warnings at the same time, with the struct group we get the iphdr at an invalid offset from the start of the inner structure, but without it the memcpy find the field overflow. My original patch would probably fix it, but as you pointed out that was rather ugly. Arnd
On 12/08/2023 09:23, Arnd Bergmann wrote: > On Fri, Jun 23, 2023, at 20:38, edward.cree@amd.com wrote: > Unfortunately, the same warning now came back after commit > 55c1528f9b97f ("sfc: fix field-spanning memcpy in selftest") ... > I'm out of ideas for how to fix both warnings at the same time, > with the struct group we get the iphdr at an invalid offset from > the start of the inner structure, But the actual address of the iphdr is properly aligned now, right? AFAICT the concept of the offset per se being 'valid' or not is not even meaningful; it's the access address that must be aligned, not the difference from random addresses used to construct it. In which case arguably the warning is just bogus and it's a compiler fix we need at this point. -e
On Mon, Aug 14, 2023, at 12:06, Edward Cree wrote: > On 12/08/2023 09:23, Arnd Bergmann wrote: >> On Fri, Jun 23, 2023, at 20:38, edward.cree@amd.com wrote: >> Unfortunately, the same warning now came back after commit >> 55c1528f9b97f ("sfc: fix field-spanning memcpy in selftest") > ... >> I'm out of ideas for how to fix both warnings at the same time, >> with the struct group we get the iphdr at an invalid offset from >> the start of the inner structure, > > But the actual address of the iphdr is properly aligned now, right? Yes > AFAICT the concept of the offset per se being 'valid' or not is not > even meaningful; it's the access address that must be aligned, not > the difference from random addresses used to construct it. > In which case arguably the warning is just bogus and it's a compiler > fix we need at this point. I think overall this is still a useful warning, in the sense that it can spot incorrect code elsewhere. The reasoning seems to be that the behavior of __packed is GNU specific and incompatible with the C23 _Alignas() annotation that is specified as 5 The combined effect of all alignment specifiers in a declaration shall not specify an alignment that is less strict than the alignment that would otherwise be required for the type of the object or member being declared. ... When multiple alignment specifiers occur in a declaration, the effective alignment requirement is the strictest specified alignment. I tried the same code using the standard C notation, which turns the warning into an error in clang. Arnd
On 14/08/2023 14:45, Arnd Bergmann wrote: > I think overall this is still a useful warning, in the sense that > it can spot incorrect code elsewhere. It's a valid concept for a warning, but it's badly implemented, because it fires on 'defining a type' rather than 'declaring an object'. At no point is an object of the inner (anonymous) struct type declared (or a pointer to such constructed) without being (4n+2)-aligned, but the compiler isn't smart enough to figure that out. And as Linus once said[1]: If you cannot distinguish it from incorrect uses, you shouldn't be warning the user, because the compiler obviously doesn't know enough to make a sufficiently educated guess. (among other remarks on a theme of 'warnings with false positives are worse than useless'. Especially when there's no way to shut them up without making the code objectively worse). -e [1]: https://yarchive.net/comp/linux/gcc.html#11
diff --git a/drivers/net/ethernet/sfc/selftest.c b/drivers/net/ethernet/sfc/selftest.c index 3c5227afd497..96d856b9043c 100644 --- a/drivers/net/ethernet/sfc/selftest.c +++ b/drivers/net/ethernet/sfc/selftest.c @@ -42,12 +42,16 @@ * Falcon only performs RSS on TCP/UDP packets. */ struct efx_loopback_payload { + char pad[2]; /* Ensures ip is 4-byte aligned */ struct ethhdr header; struct iphdr ip; struct udphdr udp; __be16 iteration; char msg[64]; -} __packed; +} __packed __aligned(4); +#define EFX_LOOPBACK_PAYLOAD_LEN (sizeof(struct efx_loopback_payload) - \ + offsetof(struct efx_loopback_payload, \ + header)) /* Loopback test source MAC address */ static const u8 payload_source[ETH_ALEN] __aligned(2) = { @@ -282,7 +286,7 @@ void efx_loopback_rx_packet(struct efx_nic *efx, const char *buf_ptr, int pkt_len) { struct efx_loopback_state *state = efx->loopback_selftest; - struct efx_loopback_payload *received; + struct efx_loopback_payload received; struct efx_loopback_payload *payload; BUG_ON(!buf_ptr); @@ -293,13 +297,14 @@ void efx_loopback_rx_packet(struct efx_nic *efx, payload = &state->payload; - received = (struct efx_loopback_payload *) buf_ptr; - received->ip.saddr = payload->ip.saddr; + memcpy(&received.header, buf_ptr, + min_t(int, pkt_len, EFX_LOOPBACK_PAYLOAD_LEN)); + received.ip.saddr = payload->ip.saddr; if (state->offload_csum) - received->ip.check = payload->ip.check; + received.ip.check = payload->ip.check; /* Check that header exists */ - if (pkt_len < sizeof(received->header)) { + if (pkt_len < sizeof(received.header)) { netif_err(efx, drv, efx->net_dev, "saw runt RX packet (length %d) in %s loopback " "test\n", pkt_len, LOOPBACK_MODE(efx)); @@ -307,7 +312,7 @@ void efx_loopback_rx_packet(struct efx_nic *efx, } /* Check that the ethernet header exists */ - if (memcmp(&received->header, &payload->header, ETH_HLEN) != 0) { + if (memcmp(&received.header, &payload->header, ETH_HLEN) != 0) { netif_err(efx, drv, efx->net_dev, "saw non-loopback RX packet in %s loopback test\n", LOOPBACK_MODE(efx)); @@ -315,16 +320,16 @@ void efx_loopback_rx_packet(struct efx_nic *efx, } /* Check packet length */ - if (pkt_len != sizeof(*payload)) { + if (pkt_len != EFX_LOOPBACK_PAYLOAD_LEN) { netif_err(efx, drv, efx->net_dev, "saw incorrect RX packet length %d (wanted %d) in " - "%s loopback test\n", pkt_len, (int)sizeof(*payload), - LOOPBACK_MODE(efx)); + "%s loopback test\n", pkt_len, + (int)EFX_LOOPBACK_PAYLOAD_LEN, LOOPBACK_MODE(efx)); goto err; } /* Check that IP header matches */ - if (memcmp(&received->ip, &payload->ip, sizeof(payload->ip)) != 0) { + if (memcmp(&received.ip, &payload->ip, sizeof(payload->ip)) != 0) { netif_err(efx, drv, efx->net_dev, "saw corrupted IP header in %s loopback test\n", LOOPBACK_MODE(efx)); @@ -332,7 +337,7 @@ void efx_loopback_rx_packet(struct efx_nic *efx, } /* Check that msg and padding matches */ - if (memcmp(&received->msg, &payload->msg, sizeof(received->msg)) != 0) { + if (memcmp(&received.msg, &payload->msg, sizeof(received.msg)) != 0) { netif_err(efx, drv, efx->net_dev, "saw corrupted RX packet in %s loopback test\n", LOOPBACK_MODE(efx)); @@ -340,10 +345,10 @@ void efx_loopback_rx_packet(struct efx_nic *efx, } /* Check that iteration matches */ - if (received->iteration != payload->iteration) { + if (received.iteration != payload->iteration) { netif_err(efx, drv, efx->net_dev, "saw RX packet from iteration %d (wanted %d) in " - "%s loopback test\n", ntohs(received->iteration), + "%s loopback test\n", ntohs(received.iteration), ntohs(payload->iteration), LOOPBACK_MODE(efx)); goto err; } @@ -363,7 +368,8 @@ void efx_loopback_rx_packet(struct efx_nic *efx, buf_ptr, pkt_len, 0); netif_err(efx, drv, efx->net_dev, "expected packet:\n"); print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 0x10, 1, - &state->payload, sizeof(state->payload), 0); + &state->payload.header, EFX_LOOPBACK_PAYLOAD_LEN, + 0); } #endif atomic_inc(&state->rx_bad); @@ -385,14 +391,15 @@ static void efx_iterate_state(struct efx_nic *efx) payload->ip.daddr = htonl(INADDR_LOOPBACK); payload->ip.ihl = 5; payload->ip.check = (__force __sum16) htons(0xdead); - payload->ip.tot_len = htons(sizeof(*payload) - sizeof(struct ethhdr)); + payload->ip.tot_len = htons(sizeof(*payload) - + offsetof(struct efx_loopback_payload, ip)); payload->ip.version = IPVERSION; payload->ip.protocol = IPPROTO_UDP; /* Initialise udp header */ payload->udp.source = 0; - payload->udp.len = htons(sizeof(*payload) - sizeof(struct ethhdr) - - sizeof(struct iphdr)); + payload->udp.len = htons(sizeof(*payload) - + offsetof(struct efx_loopback_payload, udp)); payload->udp.check = 0; /* checksum ignored */ /* Fill out payload */ @@ -418,7 +425,7 @@ static int efx_begin_loopback(struct efx_tx_queue *tx_queue) for (i = 0; i < state->packet_count; i++) { /* Allocate an skb, holding an extra reference for * transmit completion counting */ - skb = alloc_skb(sizeof(state->payload), GFP_KERNEL); + skb = alloc_skb(EFX_LOOPBACK_PAYLOAD_LEN, GFP_KERNEL); if (!skb) return -ENOMEM; state->skbs[i] = skb; @@ -429,6 +436,8 @@ static int efx_begin_loopback(struct efx_tx_queue *tx_queue) payload = skb_put(skb, sizeof(state->payload)); memcpy(payload, &state->payload, sizeof(state->payload)); payload->ip.saddr = htonl(INADDR_LOOPBACK | (i << 2)); + /* Strip off the leading padding */ + skb_pull(skb, offsetof(struct efx_loopback_payload, header)); /* Ensure everything we've written is visible to the * interrupt handler. */