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