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

[Nicolai Buchwitz <nb@tipi-net.de>]

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