[PATCH net 0/2] net: macb: fix software FCS handling of shared and requeued skbs

[PATCH net 0/2] net: macb: fix software FCS handling of shared and requeued skbs

[Nicolai Buchwitz]

While testing the genet MTU series I used a Raspberry Pi CM5 (RP1 GEM)
as pktgen source for the CM4. With clone_skb the CM5 rebooted after a
few seconds. Further investigation showed that macb_pad_and_fcs()
appends the FCS in place, so the shared skb grows with every transmit
until BQL completes more than was queued and dql_completed() hits its
BUG_ON.

The same code also modifies the skb before the TX ring check, so a
NETDEV_TX_BUSY retry gets an skb that was already replaced or grown.

Patch 1 checks the ring first, patch 2 copies shared skbs.

Tested on Raspberry CM5 with pktgen at 60/20000 bytes, clone_skb 0/
1000, burst 1/32.

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
Nicolai Buchwitz (2):
      net: macb: check TX ring before modifying skb
      net: macb: copy shared skbs before appending the FCS

 drivers/net/ethernet/cadence/macb_main.c | 80 +++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 31 deletions(-)
---
base-commit: d5a007b9b457c915ab1a53227e8939e4018aa97a
change-id: 20261004-nb-macb-shared-skb-net-13174995332b

Best regards,
-- 
Nicolai Buchwitz <nb@tipi-net.de>

[PATCH net 1/2] net: macb: check TX ring before modifying skb

[Nicolai Buchwitz]

macb_pad_and_fcs() replaces or extends the skb before the ring space
check. On NETDEV_TX_BUSY the stack requeues an skb that is already freed
or grown.

Check the ring first, using the padded length for the descriptor count.
Nonlinear skbs always take the copy path so the count can assume a
linear skb.

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_main.c | 76 +++++++++++++++++++-------------
 1 file changed, 46 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 8e5c034dc3a4..6082e63009a5 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2421,8 +2421,15 @@ static inline int macb_clear_csum(struct sk_buff *skb)
 	return 0;
 }
 
+static bool macb_needs_sw_fcs(struct sk_buff *skb, struct net_device *netdev)
+{
+	return netdev->features & NETIF_F_HW_CSUM &&
+	       skb->ip_summed != CHECKSUM_PARTIAL &&
+	       !skb_shinfo(skb)->gso_size && !ptp_one_step_sync(skb);
+}
+
 /* 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)
+static int macb_pad_and_fcs(struct sk_buff **skb, bool add_fcs)
 {
 	bool cloned = skb_cloned(*skb) || skb_header_cloned(*skb) ||
 		      skb_is_nonlinear(*skb);
@@ -2431,18 +2438,15 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *netdev)
 	struct sk_buff *nskb;
 	u32 fcs;
 
-	if (!(netdev->features & NETIF_F_HW_CSUM) ||
-	    !((*skb)->ip_summed != CHECKSUM_PARTIAL) ||
-	    skb_shinfo(*skb)->gso_size || ptp_one_step_sync(*skb))
+	if (!add_fcs)
 		return 0;
 
 	if (padlen <= 0) {
-		/* FCS could be appeded to tailroom. */
-		if (tailroom >= ETH_FCS_LEN)
+		/* FCS could be appended to tailroom. */
+		if (!skb_is_nonlinear(*skb) && tailroom >= ETH_FCS_LEN)
 			goto add_fcs;
-		/* No room for FCS, need to reallocate skb. */
-		else
-			padlen = ETH_FCS_LEN;
+		/* Reallocate with room for the FCS. */
+		padlen = ETH_FCS_LEN;
 	} else {
 		/* Add room for FCS. */
 		padlen += ETH_FCS_LEN;
@@ -2481,27 +2485,15 @@ 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, tx_len;
+	bool add_fcs, is_lso;
 	unsigned long flags;
 	int fcs_len;
-	bool is_lso;
-
-	if (macb_clear_csum(skb)) {
-		dev_kfree_skb_any(skb);
-		return ret;
-	}
-
-	fcs_len = macb_pad_and_fcs(&skb, netdev);
-	if (fcs_len < 0) {
-		dev_kfree_skb_any(skb);
-		return ret;
-	}
-
-	if (macb_dma_ptp(bp) &&
-	    (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
-		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 
+	add_fcs = macb_needs_sw_fcs(skb, netdev);
 	is_lso = (skb_shinfo(skb)->gso_size != 0);
+	tx_len = add_fcs ? max_t(unsigned int, skb->len, ETH_ZLEN) +
+			   ETH_FCS_LEN : skb->len;
 
 	if (is_lso) {
 		/* length of headers */
@@ -2515,8 +2507,11 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
 			/* if this is required, would need to copy to single buffer */
 			return NETDEV_TX_BUSY;
 		}
-	} else
+	} else if (add_fcs) {
+		hdrlen = umin(tx_len, bp->max_tx_length);
+	} else {
 		hdrlen = umin(skb_headlen(skb), bp->max_tx_length);
+	}
 
 #if defined(DEBUG) && defined(VERBOSE_DEBUG)
 	netdev_vdbg(bp->netdev,
@@ -2531,12 +2526,18 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
 	 * socket buffer: skb fragments of jumbo frames may need to be
 	 * split into many buffer descriptors.
 	 */
-	if (is_lso && (skb_headlen(skb) > hdrlen))
+	if (add_fcs) {
+		/* macb_pad_and_fcs() linearizes the skb before adding the FCS. */
+		desc_cnt = DIV_ROUND_UP(tx_len, bp->max_tx_length);
+		nr_frags = 0;
+	} else if (is_lso && (skb_headlen(skb) > hdrlen)) {
 		/* extra header descriptor if also payload in first buffer */
 		desc_cnt = DIV_ROUND_UP((skb_headlen(skb) - hdrlen), bp->max_tx_length) + 1;
-	else
+		nr_frags = skb_shinfo(skb)->nr_frags;
+	} else {
 		desc_cnt = DIV_ROUND_UP(skb_headlen(skb), bp->max_tx_length);
-	nr_frags = skb_shinfo(skb)->nr_frags;
+		nr_frags = skb_shinfo(skb)->nr_frags;
+	}
 	for (f = 0; f < nr_frags; f++) {
 		frag_size = skb_frag_size(&skb_shinfo(skb)->frags[f]);
 		desc_cnt += DIV_ROUND_UP(frag_size, bp->max_tx_length);
@@ -2554,6 +2555,21 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
 		goto unlock;
 	}
 
+	if (macb_clear_csum(skb)) {
+		dev_kfree_skb_any(skb);
+		goto unlock;
+	}
+
+	fcs_len = macb_pad_and_fcs(&skb, add_fcs);
+	if (fcs_len < 0) {
+		dev_kfree_skb_any(skb);
+		goto unlock;
+	}
+
+	if (macb_dma_ptp(bp) &&
+	    (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
+		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+
 	/* Map socket buffer for DMA transfer */
 	if (macb_tx_map(bp, queue, skb, hdrlen, fcs_len)) {
 		dev_kfree_skb_any(skb);

-- 
2.53.0

[PATCH net 2/2] net: macb: copy shared skbs before appending the FCS

[Nicolai Buchwitz]

macb_pad_and_fcs() appends the FCS in place when the skb has tailroom.
A shared skb, as pktgen sends in clone_skb mode, grows by one FCS per
transmit. BQL then completes more bytes than were queued and
dql_completed() hits its BUG_ON.

On a Raspberry Pi CM5 (RP1 GEM) pktgen with clone_skb 1000 burst 32 at
60 bytes kills the box within seconds.

Copy shared skbs before appending the FCS. Clearing IFF_TX_SKB_SHARING
would also fix it but makes pktgen refuse clone_skb on macb.

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_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 6082e63009a5..261a7e87520a 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2435,6 +2435,7 @@ static int macb_pad_and_fcs(struct sk_buff **skb, bool add_fcs)
 		      skb_is_nonlinear(*skb);
 	int padlen = ETH_ZLEN - (*skb)->len;
 	int tailroom = skb_tailroom(*skb);
+	bool shared = skb_shared(*skb);
 	struct sk_buff *nskb;
 	u32 fcs;
 
@@ -2443,7 +2444,8 @@ static int macb_pad_and_fcs(struct sk_buff **skb, bool add_fcs)
 
 	if (padlen <= 0) {
 		/* FCS could be appended to tailroom. */
-		if (!skb_is_nonlinear(*skb) && tailroom >= ETH_FCS_LEN)
+		if (!shared && !skb_is_nonlinear(*skb) &&
+		    tailroom >= ETH_FCS_LEN)
 			goto add_fcs;
 		/* Reallocate with room for the FCS. */
 		padlen = ETH_FCS_LEN;
@@ -2452,7 +2454,7 @@ static int macb_pad_and_fcs(struct sk_buff **skb, bool add_fcs)
 		padlen += ETH_FCS_LEN;
 	}
 
-	if (cloned || tailroom < padlen) {
+	if (shared || cloned || tailroom < padlen) {
 		nskb = skb_copy_expand(*skb, 0, padlen, GFP_ATOMIC);
 		if (!nskb)
 			return -ENOMEM;

-- 
2.53.0

Re: [PATCH net 0/2] net: macb: fix software FCS handling of shared and requeued skbs

[patchwork-bot+netdevbpf]

Hello:

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

On Tue, 06 Oct 2026 07:41:31 +0200 you wrote:
> While testing the genet MTU series I used a Raspberry Pi CM5 (RP1 GEM)
> as pktgen source for the CM4. With clone_skb the CM5 rebooted after a
> few seconds. Further investigation showed that macb_pad_and_fcs()
> appends the FCS in place, so the shared skb grows with every transmit
> until BQL completes more than was queued and dql_completed() hits its
> BUG_ON.
> 
> [...]

Here is the summary with links:
  - [net,1/2] net: macb: check TX ring before modifying skb
    https://git.kernel.org/netdev/net/c/6b48ed85ae0c
  - [net,2/2] net: macb: copy shared skbs before appending the FCS
    https://git.kernel.org/netdev/net/c/9151d6c42799

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