[PATCH net] net: bcmgenet: unshare the skb before writing the control block

[PATCH net] net: bcmgenet: unshare the skb before writing the control block

[Nicolai Buchwitz]

bcmgenet stores first_cb, last_cb and bytes_sent in the skb control
block and reads them back on completion to free the skb and credit BQL.
ether_setup() leaves IFF_TX_SKB_SHARING set, so the stack can resubmit
a shared skb that is still in the ring. The next transmit overwrites
last_cb, so the earlier slots complete without crediting their bytes
and BQL stalls the queue until the watchdog resets it.

Unshare the skb before writing the control block.

Reproduce with pktgen in clone_skb mode, which hands the driver the same
skb while earlier copies are still in the ring:

  modprobe pktgen
  pg=/proc/net/pktgen
  echo "add_device eth0" > $pg/kpktgend_0
  echo "dst_mac AA:BB:CC:DD:EE:FF" > $pg/eth0
  echo "dst 192.0.2.1" > $pg/eth0
  echo "clone_skb 8" > $pg/eth0
  echo "count 0" > $pg/eth0
  echo start > $pg/pgctrl &
  sleep 5
  echo stop > $pg/pgctrl
  wait
  cat /sys/class/net/eth0/queues/tx-0/byte_queue_limits/inflight

inflight stays nonzero on the now idle queue. clone_skb 0 leaves it at
zero.

Fixes: f48bed16a756 ("net: bcmgenet: Free skb after last Tx frag")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
Another finding while testing the genet MTU series. Similar shared-skb
bug as the macb fixes already merged in net [1], here in bcmgenet's Tx
control block rather than the software FCS path.

[1] https://lore.kernel.org/netdev/20261006-nb-macb-shared-skb-net-v1-0-a80641479041@tipi-net.de/
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index e8908916558b..f4cfbff49a1b 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2153,6 +2153,16 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 		goto out;
 	}
 
+	/* We store Tx state in the control block, so the skb must not be
+	 * shared, but ether_setup() leaves IFF_TX_SKB_SHARING set.
+	 */
+	skb = skb_share_check(skb, GFP_ATOMIC);
+	if (!skb) {
+		BCMGENET_STATS64_INC((&ring->stats64), dropped);
+		ret = NETDEV_TX_OK;
+		goto out;
+	}
+
 	/* Retain how many bytes will be sent on the wire, without TSB inserted
 	 * by transmit checksum offload
 	 */

---
base-commit: af32da41b0327b9c6a37856ba82b6760d6c8d10e
change-id: 20261009-nb-genet-shared-skb-net-5747ee5df864

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

Re: [PATCH net] net: bcmgenet: unshare the skb before writing the control block

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: bcmgenet: unshare the skb before writing the control block

[Nicolai Buchwitz]

On 9.10.2026 12:14, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.

Raspberry Pi CM4

> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Re: [PATCH net] net: bcmgenet: unshare the skb before writing the control block

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new skb_share_check() failure branch in bcmgenet_xmit()
  (drivers/net/ethernet/broadcom/genet/bcmgenet.c:2159-2164) jumps to…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  bcmgenet_timeout()…

--

net: bcmgenet: unshare the skb before writing the control block

bcmgenet_xmit() keeps first_cb, last_cb and bytes_sent in the skb control
block and reads them back when the transmit completes. A shared skb that
is resubmitted while it is still in the ring overwrites that state, and
BQL then stalls the queue. The patch calls skb_share_check() before the
control block is written, so each transmit works on its own skb.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index e8908916558b1..f4cfbff49a1b8 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -2153,6 +2153,16 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>  		goto out;
>  	}
>  
> +	/* We store Tx state in the control block, so the skb must not be
> +	 * shared, but ether_setup() leaves IFF_TX_SKB_SHARING set.
> +	 */
> +	skb = skb_share_check(skb, GFP_ATOMIC);
> +	if (!skb) {
> +		BCMGENET_STATS64_INC((&ring->stats64), dropped);
> +		ret = NETDEV_TX_OK;
> +		goto out;

[Severity: Medium]
If the GFP_ATOMIC clone fails on the last skb of an xmit_more batch, do
the descriptors queued by the earlier calls in that batch get handed to
the hardware?

The only TDMA_PROD_INDEX doorbell write in bcmgenet_xmit() is on the
success path, and the out label comes right after it:

	if (!netdev_xmit_more() || netif_xmit_stopped(txq))
		/* Packets are ready, update producer index */
		bcmgenet_tdma_ring_writel(priv, ring->index,
					  ring->prod_index, TDMA_PROD_INDEX);
out:
	spin_unlock(&ring->lock);

The earlier calls in the batch already advanced ring->prod_index and
called netdev_tx_sent_queue(), and they left the doorbell write to the
last packet. When that last packet takes this goto out, the skb is freed,
NETDEV_TX_OK is returned, and the hardware is never told about the
earlier descriptors.

pktgen_xmit() in M_START_XMIT mode with burst > 1 can hit this path. The
skb is always shared there, so every call goes through the clone:

	if (skb_shared)
		refcount_add(burst, &pkt_dev->skb->users);

xmit_more:
	ret = netdev_start_xmit(pkt_dev->skb, odev, txq, --burst > 0);

The final call runs with more == false. pktgen counts NETDEV_TX_OK as
done and does not retry it.

Those frames then sit unsent in the ring, and BQL inflight stays
nonzero, until a later transmit on the same ring writes the doorbell. The
queue is not stopped in that state, so dev_watchdog will not recover it
either.

The existing exits for a bcmgenet_add_tsb() failure and a DMA map failure
have the same pattern. Would it make sense to write the doorbell on the
drop paths when !netdev_xmit_more()?

> +	}
> +
>  	/* Retain how many bytes will be sent on the wire, without TSB inserted
>  	 * by transmit checksum offload
>  	 */

[Severity: High]
This is a pre-existing issue, not one introduced by this patch, but it
involves the same ring->stats64.syncp that the new dropped increment
uses.

bcmgenet_timeout() updates ring->stats64 without holding ring->lock:

drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_timeout() {
    ...
	bcmgenet_tx_reclaim(dev, ring, true);

	/* Re-enable the TX interrupt for this ring */
	bcmgenet_intrl2_1_writel(priv, 1 << txqueue, INTRL2_CPU_MASK_CLEAR);

	txq_trans_cond_update(txq);

	BCMGENET_STATS64_INC((&ring->stats64), errors);
    ...
}

By that point bcmgenet_tx_reclaim() has already taken and dropped
spin_lock_bh(&ring->lock), and the ring's TX interrupt is unmasked again.

All the other writers of this syncp run under ring->lock. That includes
__bcmgenet_tx_reclaim() called from bcmgenet_tx_poll():

	spin_lock(&ring->lock);
	work_done = __bcmgenet_tx_reclaim(ring->priv->dev, ring);

which does:

	u64_stats_update_begin(&stats->syncp);
	u64_stats_add(&stats->packets, pkts_compl);
	u64_stats_add(&stats->bytes, bytes_compl);
	u64_stats_update_end(&stats->syncp);

The watchdog holds netif_tx_lock, which keeps ndo_start_xmit out. It does
not keep TX NAPI out, and NAPI is not disabled here.

On 32-bit SMP kernels, u64_stats_update_begin() does a plain,
non-atomic seqcount increment. If bcmgenet_timeout() and
bcmgenet_tx_poll() interleave on two CPUs, one of the increments can be
lost and the sequence stays odd for good.

If that happens, bcmgenet_get_stats64() would spin forever in
u64_stats_fetch_begin(). Stats readers that hold RTNL, such as
rtnl_fill_ifinfo() callers, would hang with it.

Could the errors increment be done under ring->lock? Either wrap it in
spin_lock_bh(&ring->lock) or move it inside bcmgenet_tx_reclaim().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-nb-genet-shared-skb-net-v1-1-c6d0a841977d%40tipi-net.de

Re: [PATCH net] net: bcmgenet: unshare the skb before writing the control block

[Nicolai Buchwitz]

Hi Clashiko

On 10.10.2026 13:10, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let 
> patchwork
> know by including "pw-bot: cr" as a separate line at the end of your 
> reply
> (one such reply per series is enough).
> 
> - [Medium] The new skb_share_check() failure branch in bcmgenet_xmit()
>   (drivers/net/ethernet/broadcom/genet/bcmgenet.c:2159-2164) jumps to…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but
>   bcmgenet_timeout()…
> 
> --
> 
> net: bcmgenet: unshare the skb before writing the control block
> 
> bcmgenet_xmit() keeps first_cb, last_cb and bytes_sent in the skb 
> control
> block and reads them back when the transmit completes. A shared skb 
> that
> is resubmitted while it is still in the ring overwrites that state, and
> BQL then stalls the queue. The patch calls skb_share_check() before the
> control block is written, so each transmit works on its own skb.
> 
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index e8908916558b1..f4cfbff49a1b8 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -2153,6 +2153,16 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff 
>> *skb, struct net_device *dev)
>>  		goto out;
>>  	}
>> 
>> +	/* We store Tx state in the control block, so the skb must not be
>> +	 * shared, but ether_setup() leaves IFF_TX_SKB_SHARING set.
>> +	 */
>> +	skb = skb_share_check(skb, GFP_ATOMIC);
>> +	if (!skb) {
>> +		BCMGENET_STATS64_INC((&ring->stats64), dropped);
>> +		ret = NETDEV_TX_OK;
>> +		goto out;
> 
> [Severity: Medium]
> If the GFP_ATOMIC clone fails on the last skb of an xmit_more batch, do
> the descriptors queued by the earlier calls in that batch get handed to
> the hardware?

No, but that is nothing new. The same goto out is taken by the existing
add_tsb and unmap drops, and the next xmit flushes them anyway. You only
get here on an alloc failure under pktgen, so real traffic probably 
never
hits it. Can be cleaned up separately, maybe even net-next?

> [...]

> [Severity: High]
> This is a pre-existing issue, not one introduced by this patch, but it
> involves the same ring->stats64.syncp that the new dropped increment
> uses.

Agreed, pre-existing. I can fix in another series together with above.

> [...]