Skip to contentSkip to conversation

sfc: fix unaligned access in loopback selftests

PATCH Applied [email protected] 4 participants 9 messages Jun 23, 2023 – Aug 14, 2023

Subsystems (3)
Networking Network drivers Sfc network driver
lore
Share & tools
lore
author
#1 lore raw

PATCH v2 1/3 sfc: use padding to fix alignment in loopback test

In reply to [email protected]

From: Edward Cree <[email protected]>

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 <[email protected]>
Signed-off-by Edward Cree <[email protected]>
---
 drivers/net/ethernet/sfc/selftest.c | 47 +++++++++++++++++------------
 1 file changed, 28 insertions(+), 19 deletions(-)
drivers/net/ethernet/sfc/selftest.c +28 -19
index 3c5227afd497..96d856b9043c 100644
@@ -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. */

[email protected]

author
#0 lore raw

v2 cover sfc: fix unaligned access in loopback selftests

From: Edward Cree <[email protected]>

Arnd reported that the sfc drivers each define a packed loopback_payload
structure with an ethernet header followed by an IP header, whereas the kernel definition of iphdr specifies that this is 4-byte aligned, causing a W=1 warning.
Fix this in each case by adding two bytes of leading padding to the
struct, taking care that these are not sent on the wire.
Tested on EF10; build-tested on Siena and Falcon.

Changed in v2:
* added __aligned(4) to payload struct definitions (Arnd)
* fixed dodgy whitespace (checkpatch)

Edward Cree (3):
sfc: use padding to fix alignment in loopback test
sfc: siena: use padding to fix alignment in loopback test
sfc: falcon: use padding to fix alignment in loopback test

 drivers/net/ethernet/sfc/falcon/selftest.c | 47 +++++++++++++---------
 drivers/net/ethernet/sfc/selftest.c        | 47 +++++++++++++---------
 drivers/net/ethernet/sfc/siena/selftest.c  | 47 +++++++++++++---------
 3 files changed, 84 insertions(+), 57 deletions(-)
author
#6 lore raw

PATCH v2 2/3 sfc: siena: use padding to fix alignment in loopback test

From: Edward Cree <[email protected]>

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/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;
Reported-by Arnd Bergmann <[email protected]>
Signed-off-by Edward Cree <[email protected]>
---
 drivers/net/ethernet/sfc/siena/selftest.c | 47 ++++++++++++++---------
 1 file changed, 28 insertions(+), 19 deletions(-)
drivers/net/ethernet/sfc/siena/selftest.c +28 -19
index 07715a3d6bea..111ac17194a5 100644
@@ -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_siena_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_siena_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_siena_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_siena_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_siena_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_siena_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_siena_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. */
author
#7 lore raw

PATCH v2 3/3 sfc: falcon: use padding to fix alignment in loopback test

In reply to [email protected]

From: Edward Cree <[email protected]>

Add two bytes of padding to the start of struct ef4_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/falcon/selftest.c:43:15: error: field ip within 'struct ef4_loopback_payload' is less aligned than 'struct iphdr' and is usually due to 'struct ef4_loopback_payload' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]

        struct iphdr ip;
Reported-by Arnd Bergmann <[email protected]>
Signed-off-by Edward Cree <[email protected]>
---
 drivers/net/ethernet/sfc/falcon/selftest.c | 47 +++++++++++++---------
 1 file changed, 28 insertions(+), 19 deletions(-)
drivers/net/ethernet/sfc/falcon/selftest.c +28 -19
index 6a454ac6f876..9e5ce2a13787 100644
@@ -39,12 +39,16 @@
  * Falcon only performs RSS on TCP/UDP packets.
  */
 struct ef4_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 EF4_LOOPBACK_PAYLOAD_LEN	(sizeof(struct ef4_loopback_payload) - \
+					 offsetof(struct ef4_loopback_payload, \
+						  header))
 
 /* Loopback test source MAC address */
 static const u8 payload_source[ETH_ALEN] __aligned(2) = {

@@ -284,7 +288,7 @@ void ef4_loopback_rx_packet(struct ef4_nic *efx,
 			    const char *buf_ptr, int pkt_len)
 {
 	struct ef4_loopback_state *state = efx->loopback_selftest;
-	struct ef4_loopback_payload *received;
+	struct ef4_loopback_payload received;
 	struct ef4_loopback_payload *payload;
 
 	BUG_ON(!buf_ptr);

@@ -295,13 +299,14 @@ void ef4_loopback_rx_packet(struct ef4_nic *efx,
 
 	payload = &state->payload;
 
-	received = (struct ef4_loopback_payload *) buf_ptr;
-	received->ip.saddr = payload->ip.saddr;
+	memcpy(&received.header, buf_ptr,
+	       min_t(int, pkt_len, EF4_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));

@@ -309,7 +314,7 @@ void ef4_loopback_rx_packet(struct ef4_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));

@@ -317,16 +322,16 @@ void ef4_loopback_rx_packet(struct ef4_nic *efx,
 	}
 
 	/* Check packet length */
-	if (pkt_len != sizeof(*payload)) {
+	if (pkt_len != EF4_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)EF4_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));

@@ -334,7 +339,7 @@ void ef4_loopback_rx_packet(struct ef4_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));

@@ -342,10 +347,10 @@ void ef4_loopback_rx_packet(struct ef4_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;
 	}

@@ -365,7 +370,8 @@ void ef4_loopback_rx_packet(struct ef4_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, EF4_LOOPBACK_PAYLOAD_LEN,
+			       0);
 	}
 #endif
 	atomic_inc(&state->rx_bad);

@@ -387,14 +393,15 @@ static void ef4_iterate_state(struct ef4_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 ef4_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 ef4_loopback_payload, udp));
 	payload->udp.check = 0;	/* checksum ignored */
 
 	/* Fill out payload */

@@ -420,7 +427,7 @@ static int ef4_begin_loopback(struct ef4_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(EF4_LOOPBACK_PAYLOAD_LEN, GFP_KERNEL);
 		if (!skb)
 			return -ENOMEM;
 		state->skbs[i] = skb;

@@ -431,6 +438,8 @@ static int ef4_begin_loopback(struct ef4_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 ef4_loopback_payload, header));
 
 		/* Ensure everything we've written is visible to the
 		 * interrupt handler. */
CI Applied
#8 lore raw
In reply to [email protected]
This series was applied to netdev/net-next.git (main) by David S. Miller <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (main) by David S. Miller <[email protected]>:

Quoted 11 lines · On Fri, 23 Jun 2023 19:38:03 +0100 you wrote:

From: Edward Cree <[email protected]>

Arnd reported that the sfc drivers each define a packed loopback_payload
structure with an ethernet header followed by an IP header, whereas the kernel definition of iphdr specifies that this is 4-byte aligned, causing a W=1 warning.
Fix this in each case by adding two bytes of leading padding to the
struct, taking care that these are not sent on the wire.
Tested on EF10; build-tested on Siena and Falcon.

[...]

Here is the summary with links:
- [v2,net-next,1/3] sfc: use padding to fix alignment in loopback test

    https://git.kernel.org/netdev/net-next/c/cf60ed469629
  - [v2,net-next,2/3] sfc: siena: use padding to fix alignment in loopback test
    https://git.kernel.org/netdev/net-next/c/30c24dd87f3f
  - [v2,net-next,3/3] sfc: falcon: use padding to fix alignment in loopback test
    https://git.kernel.org/netdev/net-next/c/1186c6b31ee1

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
#2 lore raw

Re: [PATCH v2 net-next 1/3] sfc: use padding to fix alignment in loopback test

In reply to [email protected]
Quoted 24 lines · On Fri, Jun 23, 2023, at 20:38, [email protected] wrote:

From: Edward Cree <[email protected]>

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 <[email protected]>
Signed-off-by Edward Cree <[email protected]>
---
 drivers/net/ethernet/sfc/selftest.c | 47 +++++++++++++++++------------
 1 file changed, 28 insertions(+), 19 deletions(-)
drivers/net/ethernet/sfc/selftest.c +0 -0
index 3c5227afd497..96d856b9043c 100644
@@ -42,12 +42,16 @@
  * Falcon only performs RSS on TCP/UDP packets.

drivers/net/ethernet/sfc/selftest.c line 51

Quoted 10 lines
drivers/net/ethernet/sfc/selftest.c +2 -1
  */
 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

maintainer
#3 lore raw

Re: [PATCH v2 net-next 1/3] sfc: use padding to fix alignment in loopback test

Quoted 3 lines · Arnd Bergmann

On Fri, Jun 23, 2023, at 20:38, [email protected] wrote:
Unfortunately, the same warning now came back after commit 55c1528f9b97f ("sfc: fix field-spanning memcpy in selftest")

...

Quoted 3 lines

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

#4 lore raw

Re: [PATCH v2 net-next 1/3] sfc: use padding to fix alignment in loopback test

Quoted 10 lines · Edward Cree
Quoted 3 lines · Arnd Bergmann

On Fri, Jun 23, 2023, at 20:38, [email protected] wrote:
Unfortunately, the same warning now came back after commit 55c1528f9b97f ("sfc: fix field-spanning memcpy in selftest")

...

Quoted 3 lines

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

Quoted 5 lines

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

maintainer
#5 lore raw

Re: [PATCH v2 net-next 1/3] sfc: use padding to fix alignment in loopback test

Quoted 2 lines · Arnd Bergmann

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