[PATCH net] net: bcmgenet: stop Tx NAPI before disabling the queues

[PATCH net] net: bcmgenet: stop Tx NAPI before disabling the queues

[Nicolai Buchwitz]

bcmgenet_netif_stop() disables the Tx queues first and stops Tx NAPI
several steps later. A completion in flight calls netif_tx_wake_queue() in
between, so a transmit can reach the rings after bcmgenet_fini_dma() has
freed them.

Close is safe because dev_deactivate_many() stops the qdisc first.
bcmgenet_suspend() does not, so stop Tx NAPI before the queues.

KASAN on a Raspberry Pi CM4, driven from an MTU change because suspend
freezes user space before the callback runs:

  BUG: KASAN: use-after-free in bcmgenet_xmit+0x17f8/0x2258
  Write of size 8 at addr ffffff8055844a68 by task ksoftirqd/0/14
   bcmgenet_xmit+0x17f8/0x2258
   dev_hard_start_xmit+0x13c/0x588
   sch_direct_xmit+0x108/0x340
   __dev_queue_xmit+0x1190/0x3848

Fixes: 254f3239dd07 ("net: bcmgenet: revise suspend/resume")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
Found this, while I was preparing my jumbo frame series for genet.
The MTU change in my test comes from that series and is not in tree
yet.

 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index b916080f4ff1..0789b60e664a 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3441,6 +3441,8 @@ static void bcmgenet_netif_stop(struct net_device *dev, bool stop_phy)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 
+	/* Stop completion polling before it can wake a stopped queue */
+	bcmgenet_disable_tx_napi(priv);
 	netif_tx_disable(dev);
 
 	/* Disable MAC receive */
@@ -3455,7 +3457,6 @@ static void bcmgenet_netif_stop(struct net_device *dev, bool stop_phy)
 	/* Disable MAC transmit. TX DMA disabled must be done before this */
 	umac_enable_set(priv, CMD_TX_EN, false);
 
-	bcmgenet_disable_tx_napi(priv);
 	bcmgenet_disable_rx_napi(priv);
 	bcmgenet_intr_disable(priv);
 
-- 
2.53.0

Re: [PATCH net] net: bcmgenet: stop Tx NAPI before disabling the queues

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] Incomplete fix: the Wake-on-LAN branch of bcmgenet_suspend()
  keeps the exact queue-stop-before-Tx-NAPI-stop ordering that this…

--

net: bcmgenet: stop Tx NAPI before disabling the queues

This moves bcmgenet_disable_tx_napi() ahead of netif_tx_disable() in
bcmgenet_netif_stop(), so an in-flight Tx completion can no longer call
netif_tx_wake_queue() after the queues have been stopped and before
bcmgenet_fini_dma() frees the rings.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index b916080f4ff17..0789b60e664ad 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -3441,6 +3441,8 @@ static void bcmgenet_netif_stop(struct net_device *dev, bool stop_phy)
>  {
>  	struct bcmgenet_priv *priv = netdev_priv(dev);
>  
> +	/* Stop completion polling before it can wake a stopped queue */
> +	bcmgenet_disable_tx_napi(priv);
>  	netif_tx_disable(dev);

[Severity: High]

The commit message says:

  Close is safe because dev_deactivate_many() stops the qdisc first.
  bcmgenet_suspend() does not, so stop Tx NAPI before the queues.

bcmgenet_suspend() only reaches bcmgenet_netif_stop() in the non-WoL else
branch. Does the Wake-on-LAN branch still have the ordering this patch
declares unsafe?

drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_suspend() {
	if (device_may_wakeup(d) && priv->wolopts) {
		netif_tx_disable(dev);
		...
		/* HFB programming, interrupt clearing */
		...
		if (-ETIMEDOUT == bcmgenet_tdma_disable(priv))
			netdev_warn(priv->dev,
				    "Timed out while disabling TX DMA\n");

		bcmgenet_disable_tx_napi(priv);
		bcmgenet_disable_rx_napi(priv);
		disable_irq(priv->irq1);
		bcmgenet_tx_reclaim_all(dev);
		bcmgenet_fini_tx_napi(priv);
	} else {
		/* Teardown the interface */
		bcmgenet_netif_stop(dev, true);
	}
}

Between netif_tx_disable() and bcmgenet_disable_tx_napi() here, priv->irq1
is still enabled and the Tx NAPI is still live, and bcmgenet_tdma_disable()
spins in udelay, so the window is not short. Can bcmgenet_tx_poll() run in
that window and undo the stop?

drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_tx_poll() {
	work_done = __bcmgenet_tx_reclaim(ring->priv->dev, ring);
	if (ring->free_bds > (MAX_SKB_FRAGS + 1)) {
		txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
		netif_tx_wake_queue(txq);
	}
}

Nothing later in the WoL branch re-stops the queue, and suspend does not
call dev_deactivate(), so the qdisc stays attached. sch_direct_xmit() gates
only on the per-txq stop bit, not on netif_device_present(), so the
netif_device_detach() done earlier in bcmgenet_suspend() does not block a
transmit:

net/sched/sch_generic.c:sch_direct_xmit() {
	HARD_TX_LOCK(dev, txq, smp_processor_id());
	if (!netif_xmit_frozen_or_stopped(txq))
		skb = dev_hard_start_xmit(skb, dev, txq, &ret);
}

A kernel-generated transmit is enough to hit this (ARP/ND reply, TCP
retransmit, forwarding), so freezing user space does not close it.

Two follow-on questions about that path:

Can this leak the skb and its DMA mapping? A transmit arriving after
bcmgenet_tx_reclaim_all() dma_map_single()s the skb into ring->cbs while
TDMA is already disabled, so no completion reclaims it. On the WoL-success
resume path, bcmgenet_resume() -> bcmgenet_init_tx_queues() ->
bcmgenet_init_tx_ring() resets the software and hardware indices without
walking cbs:

drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_init_tx_ring() {
	ring->cbs = priv->tx_cbs + start_ptr;
	ring->size = size;
	ring->clean_ptr = start_ptr;
	ring->c_index = 0;
	ring->free_bds = size;
	ring->write_ptr = start_ptr;
	ring->cb_ptr = start_ptr;
	ring->prod_index = 0;
}

Neither that function nor the resume path calls netdev_tx_reset_queue(), so
does the netdev_tx_sent_queue() accounting from the stray xmit leave BQL
skewed as well?

And on the WoL-failure resume path, can this reproduce the same
use-after-free quoted in the commit message?

drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_resume() {
		/* MAC was reset so complete bcmgenet_netif_stop() */
		umac_enable_set(priv, CMD_RX_EN | CMD_TX_EN, false);
		bcmgenet_rdma_disable(priv);
		bcmgenet_intr_disable(priv);
		bcmgenet_fini_dma(priv);
}

bcmgenet_fini_dma() kfree()s priv->tx_cbs while the Tx queue is still
runnable, and bcmgenet_init_dma() only reallocates it after init_umac(),
phy_init_hw() and genphy_config_aneg(), so bcmgenet_xmit() ->
bcmgenet_get_txcb() would write through ring->cbs into freed memory.

Would it make sense to reorder the WoL branch as well, moving
bcmgenet_disable_tx_napi() ahead of netif_tx_disable() there?

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

Re: [PATCH net] net: bcmgenet: stop Tx NAPI before disabling the queues

[Nicolai Buchwitz]

Hi (S|Cl)ashiko

On 22.9.2026 14:47, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · 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).
> 
> - [High] Incomplete fix: the Wake-on-LAN branch of bcmgenet_suspend()
>   keeps the exact queue-stop-before-Tx-NAPI-stop ordering that this…
> 
> --
> 
> net: bcmgenet: stop Tx NAPI before disabling the queues
> 
> This moves bcmgenet_disable_tx_napi() ahead of netif_tx_disable() in
> bcmgenet_netif_stop(), so an in-flight Tx completion can no longer call
> netif_tx_wake_queue() after the queues have been stopped and before
> bcmgenet_fini_dma() frees the rings.
> 
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index b916080f4ff17..0789b60e664ad 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -3441,6 +3441,8 @@ static void bcmgenet_netif_stop(struct 
>> net_device *dev, bool stop_phy)
>>  {
>>  	struct bcmgenet_priv *priv = netdev_priv(dev);
>> 
>> +	/* Stop completion polling before it can wake a stopped queue */
>> +	bcmgenet_disable_tx_napi(priv);
>>  	netif_tx_disable(dev);
> 
> [Severity: High]
> 
> The commit message says:
> 
>   Close is safe because dev_deactivate_many() stops the qdisc first.
>   bcmgenet_suspend() does not, so stop Tx NAPI before the queues.
> 
> bcmgenet_suspend() only reaches bcmgenet_netif_stop() in the non-WoL 
> else
> branch. Does the Wake-on-LAN branch still have the ordering this patch
> declares unsafe?

Yes, and the window is even wider: bcmgenet_tdma_disable() spins up to
5 ms (DMA_TIMEOUT_VAL), and nothing re-stops the queue afterwards.

The reordering makes sense to me, so I will send a v2.

> 
> drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_suspend() {
> 	if (device_may_wakeup(d) && priv->wolopts) {
> 		netif_tx_disable(dev);
> 		...
> 		/* HFB programming, interrupt clearing */
> 		...
> 		if (-ETIMEDOUT == bcmgenet_tdma_disable(priv))
> 			netdev_warn(priv->dev,
> 				    "Timed out while disabling TX DMA\n");
> 
> 		bcmgenet_disable_tx_napi(priv);
> 		bcmgenet_disable_rx_napi(priv);
> 		disable_irq(priv->irq1);
> 		bcmgenet_tx_reclaim_all(dev);
> 		bcmgenet_fini_tx_napi(priv);
> 	} else {
> 		/* Teardown the interface */
> 		bcmgenet_netif_stop(dev, true);
> 	}
> }
> 
> Between netif_tx_disable() and bcmgenet_disable_tx_napi() here, 
> priv->irq1
> is still enabled and the Tx NAPI is still live, and 
> bcmgenet_tdma_disable()
> spins in udelay, so the window is not short. Can bcmgenet_tx_poll() run 
> in
> that window and undo the stop?
> 
> drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_tx_poll() {
> 	work_done = __bcmgenet_tx_reclaim(ring->priv->dev, ring);
> 	if (ring->free_bds > (MAX_SKB_FRAGS + 1)) {
> 		txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
> 		netif_tx_wake_queue(txq);
> 	}
> }
> 
> Nothing later in the WoL branch re-stops the queue, and suspend does 
> not
> call dev_deactivate(), so the qdisc stays attached. sch_direct_xmit() 
> gates
> only on the per-txq stop bit, not on netif_device_present(), so the
> netif_device_detach() done earlier in bcmgenet_suspend() does not block 
> a
> transmit:
> 
> net/sched/sch_generic.c:sch_direct_xmit() {
> 	HARD_TX_LOCK(dev, txq, smp_processor_id());
> 	if (!netif_xmit_frozen_or_stopped(txq))
> 		skb = dev_hard_start_xmit(skb, dev, txq, &ret);
> }
> 
> A kernel-generated transmit is enough to hit this (ARP/ND reply, TCP
> retransmit, forwarding), so freezing user space does not close it.
> 
> Two follow-on questions about that path:
> 
> Can this leak the skb and its DMA mapping? A transmit arriving after
> bcmgenet_tx_reclaim_all() dma_map_single()s the skb into ring->cbs 
> while
> TDMA is already disabled, so no completion reclaims it. On the 
> WoL-success
> resume path, bcmgenet_resume() -> bcmgenet_init_tx_queues() ->
> bcmgenet_init_tx_ring() resets the software and hardware indices 
> without
> walking cbs:
> 
> drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_init_tx_ring() 
> {
> 	ring->cbs = priv->tx_cbs + start_ptr;
> 	ring->size = size;
> 	ring->clean_ptr = start_ptr;
> 	ring->c_index = 0;
> 	ring->free_bds = size;
> 	ring->write_ptr = start_ptr;
> 	ring->cb_ptr = start_ptr;
> 	ring->prod_index = 0;
> }
> 
> Neither that function nor the resume path calls 
> netdev_tx_reset_queue(), so
> does the netdev_tx_sent_queue() accounting from the stray xmit leave 
> BQL
> skewed as well?

AFAIU this should be fixed with the reordering suggest above.

> 
> And on the WoL-failure resume path, can this reproduce the same
> use-after-free quoted in the commit message?
> 
> drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_resume() {
> 		/* MAC was reset so complete bcmgenet_netif_stop() */
> 		umac_enable_set(priv, CMD_RX_EN | CMD_TX_EN, false);
> 		bcmgenet_rdma_disable(priv);
> 		bcmgenet_intr_disable(priv);
> 		bcmgenet_fini_dma(priv);
> }
> 
> bcmgenet_fini_dma() kfree()s priv->tx_cbs while the Tx queue is still
> runnable, and bcmgenet_init_dma() only reallocates it after 
> init_umac(),
> phy_init_hw() and genphy_config_aneg(), so bcmgenet_xmit() ->
> bcmgenet_get_txcb() would write through ring->cbs into freed memory.

Same as above.

> 
> Would it make sense to reorder the WoL branch as well, moving
> bcmgenet_disable_tx_napi() ahead of netif_tx_disable() there?

Yes, makes sense and should fix all of the above.

pw-bot: cr