Re: [PATCH net v2] net: stmmac: Disable queues before netif_tx_disable in xdp_release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Critical] net: stmmac: race between stmmac_tx_timer_arm() and
  hrtimer_cancel()
- [Medium] net: stmmac: live xmit and shrunk DMA drain window in
  stmmac_xdp_release()
--

commit 55148e7152341ae5e169618b10fed6a75cae5332
Author: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>

net: stmmac: Disable queues before netif_tx_disable in xdp_release

This commit moves netif_tx_disable() after stmmac_disable_all_queues() and
hrtimer_cancel() in stmmac_xdp_release() to prevent a NAPI TX completion from
waking a stopped TX queue during XDP ring teardown.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 7ffed295eb14a..7d1497392b8b5 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7264,15 +7264,15 @@ void stmmac_xdp_release(struct net_device *dev)
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	u8 chan;
>  
> -	/* Ensure tx function is not running */
> -	netif_tx_disable(dev);
> -
>  	/* Disable NAPI process */
>  	stmmac_disable_all_queues(priv);
>  
>  	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>  		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
>  
> +	/* Ensure tx function is not running */
> +	netif_tx_disable(dev);

[Severity: Critical]
As reported by Sashiko on v1 [1], can moving netif_tx_disable(dev) after
hrtimer_cancel() allow an in-flight stmmac_xmit() or stmmac_tso_xmit() to
re-arm tx_q->txtimer after hrtimer_cancel() has already returned?

In drivers/net/ethernet/stmicro/stmmac/stmmac_main.c, stmmac_tx_timer_arm()
checks !napi_is_scheduled(napi) without synchronization against
napi_disable():

    if (unlikely(!napi_is_scheduled(napi))) {
            if (unlikely(!(hrtimer_active(&tx_q->txtimer))))
                    hrtimer_start(&tx_q->txtimer,
                                  STMMAC_COAL_TIMER(tx_coal_timer),
                                  HRTIMER_MODE_REL);

When stmmac_xdp_set_prog() calls stmmac_xdp_release() on a running interface,
an in-flight transmit holding __netif_tx_lock on another CPU can evaluate
!napi_is_scheduled(napi) as true just before stmmac_disable_all_queues() sets
NAPIF_STATE_SCHED in napi_disable().

Because neither napi_disable() nor hrtimer_cancel() acquires __netif_tx_lock,
stmmac_xdp_release() can finish both stmmac_disable_all_queues() and
hrtimer_cancel() before the transmitting CPU calls hrtimer_start() on
tx_q->txtimer. When stmmac_xdp_open() subsequently calls hrtimer_setup() on
tx_q->txtimer, won't it zero the active timer with memset() while it is still
queued in the per-CPU hrtimer RB-tree?

Calling netif_tx_disable(dev) after stmmac_disable_all_queues(priv) and
before the hrtimer_cancel() loop would ensure all in-flight transmits have
exited __netif_tx_lock before the timers are cancelled.

[Severity: Medium]
As reported by Jakub Kicinski on v1 [2], does delaying netif_tx_disable(dev)
until after stmmac_disable_all_queues(priv) and hrtimer_cancel() also leave
ndo_start_xmit() active while TX completion NAPI is disabled, and shrink the
drain window before DMA teardown?

Unlike stmmac_release() (where ndo_stop runs after dev_deactivate_many() has
already stopped TX queues and quiesced qdiscs), stmmac_xdp_set_prog() in
drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c calls stmmac_xdp_release()
while the interface is up and qdiscs are active:

    need_update = !!priv->xdp_prog != !!prog;
    if (if_running && need_update)
            stmmac_xdp_release(dev);

While stmmac_disable_all_queues() (which may sleep in synchronize_rcu() and
napi_disable()) and the hrtimer_cancel() loop run, stmmac_xmit() and
stmmac_tso_xmit() on other CPUs can continue mapping SKBs, handing
descriptors to the DMA engine, and updating the TX tail pointer after TX
completion NAPI has been disabled.

Immediately after netif_tx_disable(dev), stmmac_xdp_release() calls
stmmac_free_irq(), stmmac_stop_all_dma(), and free_dma_desc_resources().
However, the hardware TX DMA stop callbacks such as dwmac4_dma_stop_tx() in
drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c only clear the start bit
without polling for an in-flight transfer to finish:

    value &= ~DMA_CONTROL_ST;
    writel(value, ioaddr + DMA_CHAN_TX_CONTROL(dwmac4_addrs, chan));

Can unmapping TX buffers and freeing descriptor rings in
free_dma_desc_resources() immediately after netif_tx_disable(dev) race with
an in-flight DMA transfer that was submitted just before netif_tx_disable()?
Also, note that the Signed-off-by chain in the commit message still lists
Rohan G Thomas before the commit author without a Co-developed-by tag, and
these v1 review points were not addressed or explained in v2.

[1] https://lore.kernel.org/r/20260727030645.13280-1-muhammad.nazim.amirul.nazle.asmade@altera.com
[2] https://lore.kernel.org/r/20260730005641.4191366-1-kuba@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105053916.22353-1-muhammad.nazim.amirul.nazle.asmade@altera.com?part=1