[PATCH net 0/2] net: macb: fix zero UDPv4 checksum on transmit

[PATCH net 0/2] net: macb: fix zero UDPv4 checksum on transmit

[Nicolai Buchwitz]

When a UDPv4 checksum computes to zero, RFC 768 wants 0xffff on the
wire, but the GEM sends the raw 0x0000, which receivers read as "no
checksum". Raspberry Pi confirmed in a simulation of the Cadence IP
that the engine skips the substitution for UDPv4. UDPv6 and TCP are
fine, and the IP changelog suggests all GEM revisions could be affected.
Reported and analyzed on the Raspberry Pi 5:
https://github.com/raspberrypi/linux/issues/7550

Patch 2 does UDPv4 checksums in software and lets the existing pad
and FCS path keep the hardware off the frame, UDPv6 and TCP keep
the offload. That path miscounts tx_bytes by the software FCS, so
patch 1 fixes the accounting first.

Tested on CM5. To reproduce, send bulk UDP to another host and capture
zero checksums on the receiving side:

  iperf3 -c <host> -u -b 200M -t 30
  tcpdump -ni <iface> 'src host <dut> and udp[6:2] == 0'

Without the fix a packet shows up every few seconds:

  12:46:20.002739 IP <dut>.36309 > <host>.5201: UDP, length 1448
  12:46:23.773968 IP <dut>.36309 > <host>.5201: UDP, length 1448
  12:46:40.227460 IP <dut>.36309 > <host>.5201: UDP, length 1448

With the fix the capture showed none.

Some numbers (udpgso_bench):
- 18-byte UDP throughput dropped by ~ 8%
- 256-byte UDP throughput dropped by ~ 10%
- MTU-sized UDP and UDP GSO were unchanged at the 116 MiB/s link limit

Unfortunately I don't see a cheaper fix, the zero only shows up after
computing the checksum, the MAC inserts it in flight, and AFAIU there is
no register or descriptor bit for the substitution.

I'd appreciate testing on other silicon.

Nicolai Buchwitz (2):
  net: macb: exclude software FCS from TX byte statistics
  net: macb: fix zero UDPv4 checksum on transmit

 drivers/net/ethernet/cadence/macb.h      |  3 +++
 drivers/net/ethernet/cadence/macb_main.c | 28 +++++++++++++++++++++-------
 2 files changed, 24 insertions(+), 7 deletions(-)

-- 
2.53.0

[PATCH net 1/2] net: macb: exclude software FCS from TX byte statistics

[Nicolai Buchwitz]

Frames going through macb_pad_and_fcs() get padded and four FCS
bytes appended, and TX completion then accounts the grown skb->len.
tx_bytes is supposed to exclude the FCS, and frames padded by the
hardware are counted without the padding anyway, so these frames
show up too large in the statistics.

Remember the length the stack handed over and use that for the byte
counters. BQL stays on the padded skb->len that
netdev_tx_sent_queue() saw.

Fixes: 653e92a9175e ("net: macb: add support for padding and fcs computation")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/cadence/macb.h      |  3 +++
 drivers/net/ethernet/cadence/macb_main.c | 17 ++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 1e1f52285a39..802971c9d344 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -968,6 +968,8 @@ struct macb_dma_desc_ptp {
  *       of the frame
  * @mapping: DMA address of the skb's fragment buffer
  * @size: size of the DMA mapped buffer
+ * @skb_len: skb->len as handed over by the stack, before padding and
+ *           software FCS, only set for the last buffer of the frame
  * @mapped_as_page: true when buffer was mapped with skb_frag_dma_map(),
  *                  false when buffer was mapped with dma_map_single()
  */
@@ -975,6 +977,7 @@ struct macb_tx_skb {
 	struct sk_buff		*skb;
 	dma_addr_t		mapping;
 	size_t			size;
+	unsigned int		skb_len;
 	bool			mapped_as_page;
 };
 
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 76ee4f506033..623d072a271d 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1322,8 +1322,8 @@ static void macb_tx_error_task(struct work_struct *work)
 				bp->netdev->stats.tx_packets++;
 				queue->stats.tx_packets++;
 				packets++;
-				bp->netdev->stats.tx_bytes += skb->len;
-				queue->stats.tx_bytes += skb->len;
+				bp->netdev->stats.tx_bytes += tx_skb->skb_len;
+				queue->stats.tx_bytes += tx_skb->skb_len;
 				bytes += skb->len;
 			}
 		} else {
@@ -1450,8 +1450,8 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
 					    skb->data);
 				bp->netdev->stats.tx_packets++;
 				queue->stats.tx_packets++;
-				bp->netdev->stats.tx_bytes += skb->len;
-				queue->stats.tx_bytes += skb->len;
+				bp->netdev->stats.tx_bytes += tx_skb->skb_len;
+				queue->stats.tx_bytes += tx_skb->skb_len;
 				packets++;
 				bytes += skb->len;
 			}
@@ -2199,7 +2199,8 @@ static void macb_poll_controller(struct net_device *netdev)
 static unsigned int macb_tx_map(struct macb *bp,
 				struct macb_queue *queue,
 				struct sk_buff *skb,
-				unsigned int hdrlen)
+				unsigned int hdrlen,
+				unsigned int skb_len)
 {
 	unsigned int f, nr_frags = skb_shinfo(skb)->nr_frags;
 	unsigned int len, i, tx_head = queue->tx_head;
@@ -2284,6 +2285,7 @@ static unsigned int macb_tx_map(struct macb *bp,
 
 	/* This is the last buffer of the frame: save socket buffer */
 	tx_skb->skb = skb;
+	tx_skb->skb_len = skb_len;
 
 	/* Update TX ring: update buffer descriptors in reverse order
 	 * to avoid race condition
@@ -2476,7 +2478,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
 	unsigned int desc_cnt, nr_frags, frag_size, f;
 	struct macb_queue *queue = &bp->queues[q];
 	netdev_tx_t ret = NETDEV_TX_OK;
-	unsigned int hdrlen;
+	unsigned int hdrlen, skb_len;
 	unsigned long flags;
 	bool is_lso;
 
@@ -2485,6 +2487,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
 		return ret;
 	}
 
+	skb_len = skb->len;
 	if (macb_pad_and_fcs(&skb, netdev)) {
 		dev_kfree_skb_any(skb);
 		return ret;
@@ -2548,7 +2551,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
 	}
 
 	/* Map socket buffer for DMA transfer */
-	if (macb_tx_map(bp, queue, skb, hdrlen)) {
+	if (macb_tx_map(bp, queue, skb, hdrlen, skb_len)) {
 		dev_kfree_skb_any(skb);
 		goto unlock;
 	}
-- 
2.53.0

[PATCH net 2/2] net: macb: fix zero UDPv4 checksum on transmit

[Nicolai Buchwitz]

The GEM checksum engine writes its raw result into the UDP checksum
field, so a UDPv4 checksum that computes to zero goes out as 0x0000.
RFC 768 requires:

  If the computed checksum is zero, it is transmitted as all ones
  (the equivalent in one's complement arithmetic). An all zero
  transmitted checksum value means that the transmitter generated
  no checksum (for debugging or for higher level protocols that
  don't care).

Raspberry Pi confirmed in a simulation of the Cadence IP [1] that
the engine skips the final substitution exactly for UDP over IPv4.
UDPv6 and TCP come out correct. The IP changelog shows no related
change, so probably all GEM revisions have the same bug.

Clear the checksum offload features for UDPv4 frames in
macb_features_check(), the core then completes the checksum in
software. With ip_summed cleared, macb_pad_and_fcs() appends the
FCS and TX_NOCRC keeps the hardware off the frame. TCP and UDPv6
keep the offload.

One-step PTP sync packets stay on the hardware path, the MAC
rewrites their timestamp during transmit and would invalidate a
software checksum.

Link: https://github.com/raspberrypi/linux/issues/7550#issuecomment-5343018065 [1]
Fixes: 85ff3d87bf2e ("net/macb: add TX checksum offload feature")
Cc: Jonathan Bell <jonathan@raspberrypi.com>
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/cadence/macb_main.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 623d072a271d..f3548475f945 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -14,6 +14,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/etherdevice.h>
 #include <linux/firmware/xlnx-zynqmp.h>
+#include <linux/if_vlan.h>
 #include <linux/inetdevice.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -2373,6 +2374,16 @@ static netdev_features_t macb_features_check(struct sk_buff *skb,
 	unsigned int nr_frags, f;
 	unsigned int hdrlen;
 
+	/* The GEM skips the RFC 768 substitution of 0xffff for a zero
+	 * UDPv4 checksum, do those in software. One-step PTP sync frames
+	 * stay on hardware, the MAC rewrites their timestamp in flight.
+	 */
+	if (skb->ip_summed == CHECKSUM_PARTIAL &&
+	    vlan_get_protocol(skb) == htons(ETH_P_IP) &&
+	    skb->csum_offset == offsetof(struct udphdr, check) &&
+	    !ptp_one_step_sync(skb))
+		features &= ~NETIF_F_CSUM_MASK;
+
 	/* Validate LSO compatibility */
 
 	/* there is only one buffer or protocol is not UDP */
-- 
2.53.0

Re: [PATCH net 1/2] net: macb: exclude software FCS from TX byte statistics

[Nicolai Buchwitz]

Replying to netdev's sashiko
https://netdev-ai.bots.linux.dev/sashiko/#/message/20260824134703.766708-2-nb%40tipi-net.de:

> --- Patch [1]: [PATCH net 1/2] net: macb: exclude software FCS from TX 
> byte statistics ---
> commit 24acee751440b1a44c9e16688554df0848ab621a
> Author: Nicolai Buchwitz <nb@tipi-net.de>
> 
>     net: macb: exclude software FCS from TX byte statistics
> 
>     Frames going through macb_pad_and_fcs() get padded and four FCS
>     bytes appended, and TX completion then accounts the grown skb->len.
>     tx_bytes is supposed to exclude the FCS, and frames padded by the
>     hardware are counted without the padding anyway, so these frames
>     show up too large in the statistics.
> 
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 76ee4f5060335..623d072a271d0 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -1322,8 +1322,8 @@ static void macb_tx_error_task(struct work_struct *work)
> >  				bp->netdev->stats.tx_packets++;
> >  				queue->stats.tx_packets++;
> >  				packets++;
> > -				bp->netdev->stats.tx_bytes += skb->len;
> > -				queue->stats.tx_bytes += skb->len;
> > +				bp->netdev->stats.tx_bytes += tx_skb->skb_len;
> > +				queue->stats.tx_bytes += tx_skb->skb_len;
> >  				bytes += skb->len;
> >  			}
> >  		} else {
> 
> This is a pre-existing issue, but does macb_tx_error_task() leak BQL 
> bytes for
> dropped pending packets?
> 
> When recovering from a TX error, macb_tx_error_task() iterates through 
> the TX
> ring. For pending descriptors that did not successfully transmit, the 
> code calls
> macb_tx_unmap(bp, tx_skb, 0) which frees the SKB.
> 
> However, it appears to fail to accumulate the freed SKB's length into 
> the bytes
> accumulator. When netdev_tx_completed_queue() is subsequently called, 
> these
> leaked bytes are never reported to BQL, which could permanently retain 
> these
> charged bytes and stall the TX queue.

Looks like it. The dropped frames never get a completion credit, and DQL
only gets reset on close. This is independent of the statistics change,
so I would address it in a follow-up fix. Objections?

> This is another pre-existing issue, but does macb_tx_error_task() 
> modify the TX
> ring pointers without holding the appropriate lock?
> 
> macb_tx_error_task() acquires bp->lock and issues a non-blocking stop 
> to the
> queues via netif_tx_stop_all_queues(). It then manipulates the ring 
> pointers
> queue->tx_head and queue->tx_tail.
> 
> However, the fast-path macb_start_xmit() protects these identical 
> pointers
> using queue->tx_ptr_lock.
> 
> Since netif_tx_stop_all_queues() does not wait for an already-running
> macb_start_xmit() to complete, could macb_tx_error_task() zero out the 
> ring
> pointers while macb_start_xmit() is concurrently populating descriptors 
> on
> another CPU?

AFAIU yes, stopping the queues doesn't wait for a start_xmit that's
already running. Taking tx_ptr_lock there before bp->lock, in the same
order as start_xmit, should fix that. I would handle this together with 
the
BQL issue in a follow-up series.

> [ ... ]
> > @@ -2485,6 +2487,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
> >  		return ret;
> >  	}
> >
> > +	skb_len = skb->len;
> > 	if (macb_pad_and_fcs(&skb, netdev)) {
> 
> This isn't a bug introduced by this patch, but does macb_pad_and_fcs() 
> append
> an FCS before verifying TX ring space, which could corrupt the SKB upon 
> retry?
> 
> [ ... ]
> 
> Because the network stack will subsequently retry transmitting this 
> identical
> SKB, and its length is now larger, will macb_pad_and_fcs() compute a 
> new CRC
> over the old FCS and append a second, duplicate FCS, eventually 
> corrupting the
> payload once tailroom is exhausted?

Yes, and the copy path looks worse: the stack can requeue an skb the 
driver already
consumed. I'll do the ring space check before touching the skb at all. 
After that
point the driver never returns NETDEV_TX_BUSY, so swapping the skb 
should be fine.

> This is also a pre-existing issue, but does macb_pad_and_fcs() modify 
> shared SKB
> data for cloned packets and break nonlinear SKB layouts?
> 
> If an SKB is cloned (e.g., multicast, TCP retransmissions) or nonlinear 
> (paged
> fragments), and its length is already >= ETH_ZLEN (60 bytes) so padlen 
> <= 0,
> and it has at least 4 bytes of tailroom, the code skips the 
> reallocation check:
> 
> 	if (padlen <= 0) {
> 		/* FCS could be appeded to tailroom. */
> 		if (tailroom >= ETH_FCS_LEN)
> 			goto add_fcs;
> 
> It then directly appends 4 bytes of FCS via skb_put_u8(). For cloned 
> SKBs, this
> modifies shared memory data. For nonlinear SKBs, skb_put() illegally 
> extends the
> linear part, which could trigger a panic in skb_panic().

The cloned part looks right. The nonlinear case IMHO can't trigger 
today:
skb_tailroom() is 0 for nonlinear skbs, so they go down 
skb_copy_expand(),
which linearizes them anyway.

I'll order the cloned check before the in-place FCS path.