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

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

[Nicolai Buchwitz]

Frames for which macb_pad_and_fcs() supplies the FCS have four FCS
bytes appended, and TX completion then accounts the grown skb->len.
tx_bytes is defined to exclude the FCS, so these frames are reported
four bytes too large.

Track only the number of FCS bytes appended in software, 0 or
ETH_FCS_LEN, and subtract that from skb->len at completion. skb->len
already reflects the padded length by then, so there is nothing else
to store. macb_pad_and_fcs() already returns 0 on every non-error
path. Return the FCS length from there instead, rather than
recomputing the same check in the caller. 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>
---
Originally patch 1/2 of "net: macb: fix zero UDPv4 checksum on
transmit" [1], split out into its own patch since it's a pre-existing,
partly related issue, as suggested by Paolo and David.

[1] https://lore.kernel.org/all/20260824134703.766708-1-nb@tipi-net.de/

 drivers/net/ethernet/cadence/macb.h      |  3 +++
 drivers/net/ethernet/cadence/macb_main.c | 21 +++++++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 1e1f52285a39..d6931c41f39d 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
+ * @fcs_len: FCS bytes appended in software, 0 or ETH_FCS_LEN, 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;
+	u8			fcs_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..b1939da4c95a 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 += skb->len - tx_skb->fcs_len;
+				queue->stats.tx_bytes += skb->len - tx_skb->fcs_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 += skb->len - tx_skb->fcs_len;
+				queue->stats.tx_bytes += skb->len - tx_skb->fcs_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,
+				u8 fcs_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->fcs_len = fcs_len;
 
 	/* Update TX ring: update buffer descriptors in reverse order
 	 * to avoid race condition
@@ -2417,6 +2419,7 @@ static inline int macb_clear_csum(struct sk_buff *skb)
 	return 0;
 }
 
+/* Returns a negative errno, or the FCS bytes appended (0 or ETH_FCS_LEN). */
 static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *netdev)
 {
 	bool cloned = skb_cloned(*skb) || skb_header_cloned(*skb) ||
@@ -2465,7 +2468,7 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *netdev)
 	skb_put_u8(*skb, (fcs >> 16)	& 0xff);
 	skb_put_u8(*skb, (fcs >> 24)	& 0xff);
 
-	return 0;
+	return ETH_FCS_LEN;
 }
 
 static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
@@ -2478,6 +2481,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
 	netdev_tx_t ret = NETDEV_TX_OK;
 	unsigned int hdrlen;
 	unsigned long flags;
+	int fcs_len;
 	bool is_lso;
 
 	if (macb_clear_csum(skb)) {
@@ -2485,7 +2489,8 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
 		return ret;
 	}
 
-	if (macb_pad_and_fcs(&skb, netdev)) {
+	fcs_len = macb_pad_and_fcs(&skb, netdev);
+	if (fcs_len < 0) {
 		dev_kfree_skb_any(skb);
 		return ret;
 	}
@@ -2548,7 +2553,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, fcs_len)) {
 		dev_kfree_skb_any(skb);
 		goto unlock;
 	}
-- 
2.53.0

Re: [net] net: macb: exclude software FCS from TX byte statistics

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 (1) · High: 0 · Medium: 1 (1) · Low: 0

- [Medium] Incomplete fix: the software FCS can be appended twice to the
  same skb, so tx_bytes is still over-reported by four bytes and the new…

Pre-existing issues:
- [Critical] Use-after-free / double-free of an skb plus leak of the
  replacement skb: macb_pad_and_fcs()…
- [Medium] macb_pad_and_fcs() writes the four software FCS bytes into a
  cloned skb's shared data allocation without unsharing it.

--

net: macb: exclude software FCS from TX byte statistics

The patch records how many FCS bytes macb_pad_and_fcs() appended in
software into a new macb_tx_skb.fcs_len field, and subtracts that value
from skb->len when TX completion updates tx_bytes.  BQL accounting keeps
using the padded skb->len.

Referenced series: https://lore.kernel.org/all/20260824134703.766708-1-nb@tipi-net.de/

> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 1e1f52285a39..d6931c41f39d 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
> + * @fcs_len: FCS bytes appended in software, 0 or ETH_FCS_LEN, 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;
> +	u8			fcs_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..b1939da4c95a 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -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 += skb->len - tx_skb->fcs_len;
> +				queue->stats.tx_bytes += skb->len - tx_skb->fcs_len;
>  				packets++;
>  				bytes += skb->len;
>  			}

[ ... ]

> @@ -2417,6 +2419,7 @@ static inline int macb_clear_csum(struct sk_buff *skb)
>  	return 0;
>  }
>  
> +/* Returns a negative errno, or the FCS bytes appended (0 or ETH_FCS_LEN). */
>  static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *netdev)
>  {
>  	bool cloned = skb_cloned(*skb) || skb_header_cloned(*skb) ||
> @@ -2465,7 +2468,7 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *netdev)
>  	skb_put_u8(*skb, (fcs >> 16)	& 0xff);
>  	skb_put_u8(*skb, (fcs >> 24)	& 0xff);
>  
> -	return 0;
> +	return ETH_FCS_LEN;
>  }

[Severity: Medium]
This is a pre-existing issue, not something introduced here, but can these
four skb_put_u8() writes land in data shared with another sk_buff?

macb_pad_and_fcs() computes:

	bool cloned = skb_cloned(*skb) || skb_header_cloned(*skb) ||
		      skb_is_nonlinear(*skb);

and only copies via skb_copy_expand() under "if (cloned || tailroom <
padlen)".  For a linear skb with skb->len >= ETH_ZLEN and skb_tailroom()
>= ETH_FCS_LEN the earlier

	if (padlen <= 0) {
		/* FCS could be appeded to tailroom. */
		if (tailroom >= ETH_FCS_LEN)
			goto add_fcs;

jumps past that copy, so the FCS is written into the possibly shared data
area.

One case where the written bytes are still inside a clone's valid range:
packet_rcv() clones the skb for a tap, then ip_rcv_core() calls
pskb_trim_rcsum(), which only moves the original's tail.  Forwarding that
skb out a macb port then overwrites four bytes that the clone (and
userspace) still considers payload.

Would using skb_unshare()/skb_cow() before the "goto add_fcs" fast path be
appropriate here?

>  
>  static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
> @@ -2478,6 +2481,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
>  	netdev_tx_t ret = NETDEV_TX_OK;
>  	unsigned int hdrlen;
>  	unsigned long flags;
> +	int fcs_len;
>  	bool is_lso;
>  
>  	if (macb_clear_csum(skb)) {
> @@ -2485,7 +2489,8 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
>  		return ret;
>  	}
>  
> -	if (macb_pad_and_fcs(&skb, netdev)) {
> +	fcs_len = macb_pad_and_fcs(&skb, netdev);
> +	if (fcs_len < 0) {
>  		dev_kfree_skb_any(skb);
>  		return ret;
>  	}

[Severity: Medium]
Does the requeue path still report tx_bytes four bytes too large?

macb_pad_and_fcs() runs here, before the descriptor space check later in
macb_start_xmit():

	if (CIRC_SPACE(queue->tx_head, queue->tx_tail,
		       bp->tx_ring_size) < desc_cnt) {
		netif_stop_subqueue(netdev, q);
		...
		ret = NETDEV_TX_BUSY;
		goto unlock;
	}

On NETDEV_TX_BUSY the core hands the same skb back:

sch_direct_xmit()
    if (!dev_xmit_complete(ret)) {
        /* Driver returned NETDEV_TX_BUSY - requeue skb */
        dev_requeue_skb(skb, q);

so macb_start_xmit() -> macb_pad_and_fcs() runs on that skb a second time.
The early-exit predicate (features, ip_summed, gso_size, ptp state) is
unchanged, padlen = ETH_ZLEN - skb->len is now <= 0, so the code reaches
add_fcs again and appends a second four-byte CRC computed over
payload+FCS1.

macb_tx_map() then stores fcs_len = ETH_FCS_LEN while skb->len contains
2 * ETH_FCS_LEN software bytes, so macb_tx_complete() and
macb_tx_error_task() still add four bytes too many:

	bp->netdev->stats.tx_bytes += skb->len - tx_skb->fcs_len;

and the frame on the wire carries the stale first FCS as trailing payload.

Reaching the NETDEV_TX_BUSY branch needs desc_cnt >= 2, i.e. a
software-FCS frame longer than bp->max_tx_length.  In-tree configurations
allow that: mpfs_config has .max_tx_length = 4040 with .jumbo_max_len =
4040, and pic64hpsc_config has .jumbo_max_len = 16383 against
GEM_MAX_TX_LEN of 16320, so with jumbo MTU a frame grown by the software
FCS spans two descriptors and CIRC_SPACE == 1 yields NETDEV_TX_BUSY.

Would moving the macb_pad_and_fcs() call after the CIRC_SPACE check (or
otherwise making the append idempotent) address this?  Since the chosen
representation records only the length of the last append, should the
changelog also mention the requeue interaction?

[Severity: Critical]
This isn't a bug introduced by this patch, but the same ordering looks like
it can free the caller's skb and then tell the core to reuse it.

macb_pad_and_fcs() substitutes the skb only through the caller's local
pointer:

	if (cloned || tailroom < padlen) {
		nskb = skb_copy_expand(*skb, 0, padlen, GFP_ATOMIC);
		if (!nskb)
			return -ENOMEM;

		dev_consume_skb_any(*skb);
		*skb = nskb;
	}

If macb_start_xmit() afterwards takes the

		ret = NETDEV_TX_BUSY;
		goto unlock;

path, nskb is dropped on the floor and the core continues with the
original pointer:

dev_hard_start_xmit()
    rc = xmit_one(skb, dev, txq, next != NULL);
    if (unlikely(!dev_xmit_complete(rc))) {
            skb->next = next;
            goto out;

That is a write into the already freed sk_buff, and sch_direct_xmit() then
calls dev_requeue_skb() on it, so it is dequeued, transmitted and freed a
second time, while nskb leaks.

The preconditions coexist: a forwarded or bridged nonlinear frame has
skb_tailroom() == 0 and cloned == true, so the skb_copy_expand() /
dev_consume_skb_any() path is always taken, and on mpfs_config
(.max_tx_length = 4040, .jumbo_max_len = 4040) or pic64hpsc_config
(GEM_MAX_TX_LEN 16320 against .jumbo_max_len 16383) the frame plus the
four FCS bytes needs two descriptors, so CIRC_SPACE == 1 returns
NETDEV_TX_BUSY.

Should the space check happen before any skb is consumed or replaced?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831113128.1678674-1-nb%40tipi-net.de

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 31 Aug 2026 13:31:28 +0200 you wrote:
> Frames for which macb_pad_and_fcs() supplies the FCS have four FCS
> bytes appended, and TX completion then accounts the grown skb->len.
> tx_bytes is defined to exclude the FCS, so these frames are reported
> four bytes too large.
> 
> Track only the number of FCS bytes appended in software, 0 or
> ETH_FCS_LEN, and subtract that from skb->len at completion. skb->len
> already reflects the padded length by then, so there is nothing else
> to store. macb_pad_and_fcs() already returns 0 on every non-error
> path. Return the FCS length from there instead, rather than
> recomputing the same check in the caller. BQL stays on the padded
> skb->len that netdev_tx_sent_queue() saw.
> 
> [...]

Here is the summary with links:
  - [net] net: macb: exclude software FCS from TX byte statistics
    https://git.kernel.org/netdev/net/c/d85f521a9afb

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html