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

[Kurt Kanzenbach <kurt@linutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 6688 bytes --]

On Thu Oct 08 2026, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 (1) · Medium: 0 (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] Baseline decision: false.
> - [High] This patch reorders stmmac_xdp_release() so that
>   hrtimer_cancel() on every tx_q->txtimer now runs before…
>
> Pre-existing issues:
> - [High] This problem was not introduced by this patch, but it is a use-
>   after-free in the same teardown sequence, and moving…
> - [Medium] Baseline decision: false.
>
> --
>
> net: stmmac: Disable NAPI before stopping Tx queues in stmmac_xdp_release()
>
> This commit reorders stmmac_xdp_release() so that NAPI is disabled and the
> Tx coalescing timers are cancelled before netif_tx_disable() is called.
> The goal is to stop an in-flight Tx NAPI poll from waking the Tx queue
> again while free_dma_desc_resources() releases the descriptor rings.
>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 9741f97fa37a2..796817caf7af0 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -7178,15 +7178,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]
> Now that netif_tx_disable() runs after the hrtimer_cancel() loop, can
> stmmac_xmit() re-arm tx_q->txtimer after it has been cancelled?
>
> Before this change, netif_tx_disable() took every txq lock first. Any
> running stmmac_xmit() had finished and the queues were stopped before the
> timers were cancelled.
>
> Now stmmac_xmit() can run at the same time as stmmac_disable_all_queues()
> and the cancel loop. It always ends by calling stmmac_tx_timer_arm():
>
> 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);
>     ...
> }
>
> One possible interleaving:
>
> CPU1 (stmmac_xmit)                  CPU0 (stmmac_xdp_release)
> stmmac_tx_timer_arm()
>   napi_is_scheduled() == false
>                                     stmmac_disable_all_queues()
>                                       napi_disable() sets SCHED
>                                     hrtimer_cancel(&tx_q->txtimer)
>                                       timer not armed, returns 0
>   hrtimer_start(&tx_q->txtimer)
>                                     netif_tx_disable(dev)
>
> The timer then stays queued while the rings are freed. The timeout is 5ms
> by default and up to 100ms through ethtool -C tx-usecs.
>
> stmmac_xdp_set_prog() then calls stmmac_xdp_open(), which does:
>
> 	hrtimer_setup(&tx_q->txtimer, stmmac_tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>
> __hrtimer_setup() memsets the timer while it is still linked in the
> hrtimer base timerqueue.
>
> Could that corrupt the timerqueue rbtree when the timer expires or is
> started again?
>
> Would this order avoid the problem: disable NAPI, then netif_tx_disable(),
> then hrtimer_cancel()?
>
> __stmmac_release() is cited in the commit message as the reference order,
> but it also cancels the timers before netif_tx_disable(). It may have the
> same window.

Looks like a possible issue even though I did not observe this. The
common cleanup code between stmmac_xdp_release() and __stmmac_release()
can be moved into a helper function in a later patch. This issue can be
addressed then.

>
> [Severity: High]
> The reproducer in the commit message opens an AF_XDP/ZC socket. When an
> XDP program is already attached, that goes through stmmac_xdp_setup_pool()
> -> stmmac_xdp_enable_pool() in stmmac_xdp.c, not stmmac_xdp_release():
>
> stmmac_xdp_enable_pool() {
>     ...
> 	if (need_update) {
> 		napi_disable(&ch->rx_napi);
> 		napi_disable(&ch->tx_napi);
> 		stmmac_disable_rx_queue(priv, queue);
> 		stmmac_disable_tx_queue(priv, queue);
> 	}
>     ...
> }
>
> stmmac_disable_tx_queue() frees the ring with stmmac_stop_tx_dma() and
> __free_dma_tx_desc_resources(). Nothing stops the netdev Tx queue.
> stmmac_xdp_disable_pool() follows the same pattern with ch->rxtx_napi.
>
> At this commit, can stmmac_xmit() keep writing into tx_q->dma_tx while the
> ring is freed and reallocated on these paths?
>
> The next patch in the series, "net: stmmac: Stop Tx queue when
> (en|dis)abling XSK pools", appears to address this. It adds
> stmmac_xdp_stop_tx_queue() after napi_disable() and netif_tx_wake_queue()
> after the queues are re-enabled, which closes the gap by the end of the
> series.

Yes, that's fixed in the second patch.

>
>> +
>>  	/* Free the IRQ lines */
>>  	stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the TX/DMA IRQ handlers
> stay registered after netif_tx_disable() until stmmac_free_irq() runs.
> STMMAC_DOWN is not set on this path.
>
> If a TX hard error arrives in that window, stmmac_msi_intr_tx() or
> stmmac_dma_interrupt() calls stmmac_tx_err(), which ends with:
>
> stmmac_tx_err() {
>     ...
> 	priv->xstats.tx_errors++;
> 	netif_tx_wake_queue(netdev_get_tx_queue(priv->dev, chan));
> }
>
> Can that reopen the queue so that stmmac_xmit() runs on rings that
> free_dma_desc_resources() is about to release?
>
> The window existed before this patch and was larger then. Hitting it needs
> a hardware DMA TX fatal error during XDP attach or detach.

Yes, i did not observe this while testing.

>
> [Severity: High]
> This is a pre-existing issue, not introduced by this patch. Moving
> netif_tx_disable() does not change it.

Yes, that's a separate issue.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]