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

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

[muhammad.nazim.amirul.nazle.asmade]

From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>

stmmac_xdp_release() calls netif_tx_disable() while NAPI is still
running. A TX completion handled in NAPI context can then restart the
queue through netif_tx_wake_queue() in stmmac_tx_clean(), so the stack
may call ndo_start_xmit() while the DMA rings are being torn down,
leading to a kernel panic.

Fix this by disabling NAPI with stmmac_disable_all_queues() and
cancelling the TX timers before calling netif_tx_disable(). This
matches the teardown order already used in __stmmac_release() since
commit 7028471edb64 ("net: ethernet: stmicro: stmmac: first disable all
queues and disconnect in release"), so both release paths stop NAPI,
the TX timers and the TX queues in the same way.

Once NAPI is disabled, NAPI_STATE_SCHED stays set, so
stmmac_tx_timer_arm() takes the hrtimer_try_to_cancel() path and new
transmissions cannot re-arm the TX timers after hrtimer_cancel().

Fixes: 77711683a504 ("net: stmmac: ensure tx function is not running in stmmac_xdp_release()")
Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
---
Changes in v2:
- Fix the Fixes: tag to point to the commit that introduced
  netif_tx_disable() in stmmac_xdp_release()
- Describe the queue wake-up race in the commit message, and why the
  TX timers cannot be re-armed after hrtimer_cancel() (Sashiko review)
- The other two issues reported by Sashiko (ndo_xdp_xmit() racing with
  stmmac_xdp_release(), and stmmac_xdp_set_prog() ignoring the
  stmmac_xdp_open() return value) are pre-existing and not affected by
  this change; they will be looked at separately.
- Rebase on latest net/main

v1: https://lore.kernel.org/netdev/20260727030645.13280-1-muhammad.nazim.amirul.nazle.asmade@altera.com/

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7ffed295eb14..7d1497392b8b 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);
+
 	/* Free the IRQ lines */
 	stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
 
-- 
2.43.7

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

[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:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - 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 v2] net: stmmac: Disable queues before netif_tx_disable in xdp_release

[netdev-bot+sashiko]

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

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

- [High] The patch moves the hrtimer_cancel() loop on tx_q->txtimer ahead
  of netif_tx_disable().
- [Medium] The reorder only blocks one of the two places that can wake a
  TX queue after netif_tx_disable() in stmmac_xdp_release().

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a use-
  after-free in the same teardown.

--

net: stmmac: Disable queues before netif_tx_disable in xdp_release

This commit reorders stmmac_xdp_release(). NAPI is now disabled and the TX
coalescing hrtimers are cancelled before netif_tx_disable() is called. The
goal is to stop stmmac_tx_clean() from waking a TX queue while the DMA
rings are being torn down, using the same order as __stmmac_release().

> 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: High]
hrtimer_cancel() now runs before netif_tx_disable(). Can a stmmac_xmit()
or stmmac_tso_xmit() that is already in flight re-arm tx_q->txtimer after
the cancel has returned?

stmmac_tx_timer_arm() checks the NAPI state and then acts on it:

stmmac_main.c: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);
	} else {
    ...
}

One possible interleaving:

CPU1 (stmmac_xmit, txq lock held)
stmmac_tx_timer_arm()
  napi_is_scheduled(napi) returns false
  <hardirq, or preempted on PREEMPT_RT>

CPU0 (stmmac_xdp_release)
stmmac_disable_all_queues()      /* napi_disable() sets SCHED */
hrtimer_cancel(&tx_q->txtimer)   /* nothing queued, returns */

CPU1
  hrtimer_start(&tx_q->txtimer, ...)

CPU0
netif_tx_disable()   /* waits for the txq lock, timer stays queued */

stmmac_xdp_set_prog() then calls stmmac_xdp_open() straight away, and
stmmac_xdp_open() does:

	hrtimer_setup(&tx_q->txtimer, stmmac_tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);

The coalesce timer defaults to 5ms and ethtool can raise it to 100ms, so
the stray timer is likely still queued at that point. Would re-initializing
a queued hrtimer corrupt the per-CPU hrtimer base?

Before this patch, netif_tx_disable() ran first, so in-flight xmits were
drained before the timers were cancelled.

The commit message says:

  "Once NAPI is disabled, NAPI_STATE_SCHED stays set, so
  stmmac_tx_timer_arm() takes the hrtimer_try_to_cancel() path and new
  transmissions cannot re-arm the TX timers after hrtimer_cancel()."

This seems to hold only for transmissions that check the NAPI state after
napi_disable(). Would the order stmmac_disable_all_queues() ->
netif_tx_disable() -> hrtimer_cancel() close both this window and the
stmmac_tx_clean() one?

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

[Severity: Medium]
Disabling NAPI first stops the wake in stmmac_tx_clean(). Is there still a
second waker in the window between netif_tx_disable() and
stmmac_free_irq()?

stmmac_tx_err() also calls netif_tx_wake_queue(). It runs from the hard
IRQ handlers stmmac_dma_interrupt() and stmmac_msi_intr_tx() when the
status is tx_hard_error:

stmmac_main.c:stmmac_tx_err() {
    ...
	stmmac_start_tx_dma(priv, chan);

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

The IRQ lines stay requested until stmmac_free_irq(), which now runs after
netif_tx_disable(). stmmac_xdp_release() never sets STMMAC_DOWN, so the
STMMAC_DOWN check in stmmac_msi_intr_tx() does not filter this either.

Suppose the DMA reports TPS or FBE in that window. The queue is woken
again and TX DMA is restarted. Could the NET_TX softirq then call
stmmac_xmit() while stmmac_stop_all_dma() and free_dma_desc_resources()
are freeing the descriptor ring and tx_skbuff/tx_skbuff_dma?

The trigger needs a fatal bus error or a transmit-process-stopped status in
this window, so it is narrow. __stmmac_release() has the same
netif_tx_disable() -> stmmac_free_irq() window. It is still the same
failure the commit message describes: the queue restarts after
netif_tx_disable() and ndo_start_xmit() runs during ring teardown.

[Severity: High]
This isn't a bug introduced by this patch, but it is the ndo_xdp_xmit()
race the v2 changelog mentions. Can stmmac_xdp_xmit() write into the TX
rings that free_dma_desc_resources() is freeing here?

On program removal, stmmac_xdp_set_prog() does:

	if (!prog)
		xdp_features_clear_redirect_target(dev);

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

Nothing waits for a grace period between these two calls.
stmmac_disable_all_queues() only calls synchronize_rcu() when an RX queue
has an xsk_pool.

Another device's NAPI may already have passed the feature check in
__xdp_enqueue() and queued frames in a devmap bulk queue. When it flushes
through bq_xmit_all() -> stmmac_xdp_xmit(), the only gate is:

	if (unlikely(test_bit(STMMAC_DOWN, &priv->state)))
		return -ENETDOWN;

stmmac_xdp_release() never sets STMMAC_DOWN. stmmac_xdp_xmit() also takes
__netif_tx_lock() without checking whether the queue is stopped, so
netif_tx_disable() does not fence it.

stmmac_xdp_xmit_xdpf() then writes into tx_q->dma_tx and
tx_q->tx_skbuff_dma. __free_dma_tx_desc_resources() releases that memory
with dma_free_coherent() and kfree().

The v2 changelog already says this will be handled separately.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105053916.22353-1-muhammad.nazim.amirul.nazle.asmade%40altera.com