Re: [PATCH net-next v4 1/2] net: stmmac: Disable NAPI before stopping Tx queues in stmmac_xdp_release()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 (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] stmmac_xdp_release() now runs napi_disable (via
  stmmac_disable_all_queues()), then hrtimer_cancel() on every…
- [High] The commit says the Tx queue must not be woken again after
  netif_tx_disable() in stmmac_xdp_release(), and it fixes this only for…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  stmmac_xdp_set_prog() discards the return value of stmmac_xdp_open()…

--

net: stmmac: Disable NAPI before stopping Tx queues in stmmac_xdp_release()

This patch reorders stmmac_xdp_release() so that NAPI is disabled and the
Tx coalescing timers are cancelled before netif_tx_disable(). The goal is
to stop stmmac_tx_clean() from waking the Tx queue after it was stopped,
which let stmmac_xmit() run on descriptor rings freed by
free_dma_desc_resources().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index fb53b1f17714..27c4a249e560 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7222,15 +7222,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: High]
With this order, ndo_start_xmit is still live while the hrtimer_cancel()
loop runs. Can stmmac_xmit() or stmmac_tso_xmit() re-arm txtimer after it
has been cancelled?

Both of them end with stmmac_tx_timer_arm(), which does:

stmmac_tx_timer_arm() {
    ...
	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);
    ...
}

Nothing makes this check-then-start sequence atomic with respect to
napi_disable() and hrtimer_cancel(). One possible interleaving:

CPU1 (stmmac_xmit, holding txq lock)
  napi_is_scheduled(napi) -> false
                                CPU0 (stmmac_xdp_release)
                                stmmac_disable_all_queues()
                                hrtimer_cancel(&tx_q->txtimer)
                                  timer not armed, returns at once
  hrtimer_start(&tx_q->txtimer, ...)
                                netif_tx_disable(dev)
                                  only waits for CPU1 to drop txq lock

Nothing later in stmmac_xdp_release() cancels the timer. It stays queued
for tx_coal_timer, which is 5ms by default and up to 100ms via ethtool -C.
A hardirq on CPU1 widens the window. On PREEMPT_RT it can be arbitrarily
long.

stmmac_xdp_set_prog() then calls stmmac_xdp_open() right away, and that
function does:

stmmac_xdp_open() {
    ...
		hrtimer_setup(&tx_q->txtimer, stmmac_tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
    ...
}

Would this re-initialize an hrtimer that is still linked in the per-CPU
timerqueue and corrupt the hrtimer base?

Before this patch, netif_tx_disable() ran first and serialized against
in-flight xmit callers. Only NAPI could arm the timer, and NAPI was
disabled before the cancel.

Would the order napi disable, then netif_tx_disable(), then the
hrtimer_cancel() loop avoid this?

>  	/* Free the IRQ lines */
>  	stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
>  

[Severity: High]
The commit message says the problem is the Tx queue being woken again
after netif_tx_disable(), and this patch covers the stmmac_tx_clean()
waker. Is there still a waker in IRQ context?

netif_tx_disable() still runs before stmmac_free_irq() and
stmmac_stop_all_dma(). At that point the DMA interrupt handlers are still
installed and Tx DMA is still running.

stmmac_xdp_release() never sets STMMAC_DOWN, so the STMMAC_DOWN check in
stmmac_interrupt() and stmmac_msi_intr_tx() does not stop the handlers.

On a Tx fatal bus error (tx_hard_error), stmmac_dma_interrupt() or
stmmac_msi_intr_tx() calls stmmac_tx_err(), which ends with an
unconditional wake:

stmmac_tx_err() {
    ...
	priv->xstats.tx_errors++;
	netif_tx_wake_queue(netdev_get_tx_queue(priv->dev, chan));
}

The qdisc is still active in the XDP reconfigure path. In ndo_stop,
dev_deactivate_many() has already run, but not here. So the wake can
start stmmac_xmit() again:

stmmac_xdp_release()
  netif_tx_disable(dev)
                            stmmac_dma_interrupt()
                              stmmac_tx_err()
                                netif_tx_wake_queue()
  stmmac_free_irq()
  stmmac_stop_all_dma()
  free_dma_desc_resources()
                            stmmac_xmit()
                              writes into tx_q->dma_tx / dma_etx

free_irq() only waits for handlers that are already running. It does not
undo the wake, and nothing stops the queue again after it.

Could stmmac_xmit() then write descriptors into rings that
free_dma_desc_resources() has already freed?

Moving netif_tx_disable() after stmmac_free_irq() might close this path.
Another option is a teardown flag that stmmac_tx_err() and the ISRs
check. The last patch of the series ("net: stmmac: Stop Tx queue when
(en|dis)abling XSK pools") does not seem to cover this case.

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. It is
on the same release/reopen path, though. stmmac_xdp_set_prog() ignores
the return value of stmmac_xdp_open():

drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
    ...
	if (if_running && need_update)
		stmmac_xdp_open(dev);
    ...
}

stmmac_xdp_open() can fail in alloc_dma_desc_resources(),
init_dma_desc_rings() or stmmac_request_irq(). On failure it frees the
descriptor resources and returns before stmmac_enable_all_queues(),
netif_carrier_on() and netif_tx_start_all_queues(). Userspace is still
told the XDP update succeeded.

What happens on the next ifdown in that state? The call chain
stmmac_release()->__stmmac_release()->stmmac_disable_all_queues() calls
napi_disable() on NAPI instances that stmmac_xdp_release() has already
disabled. In napi_disable_locked():

	val = READ_ONCE(n->state);
	do {
		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
			usleep_range(20, 200);
			val = READ_ONCE(n->state);
		}

Both bits are already set, so wouldn't this loop forever while holding
rtnl_lock and the netdev instance lock? Another XDP program change would
take the same path through stmmac_xdp_release().

Should stmmac_xdp_set_prog() propagate the error, and should the device
be left in a state that a later close can handle?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-stmmac_xsk_crashes-v4-0-15c0c750bb96%40linutronix.de