Re: [PATCH net-next v4 2/2] net: stmmac: Stop Tx queue when (en|dis)abling XSK pools

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 2 · 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_enable_pool() (stmmac_xdp.c:59) and
  stmmac_xdp_disable_pool() (stmmac_xdp.c:102) now call…
- [Medium] The patch only works if __QUEUE_STATE_DRV_XOFF stays set from
  stmmac_xdp_stop_tx_queue() until the ring is rebuilt.
- [Medium] In stmmac_xdp_xmit_xdpf(), first_entry and entry are read from
  tx_q->cur_tx when the locals are declared (stmmac_main.c:5263-5264).

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but if
  __alloc_dma_tx_desc_resources() fails during an XSK pool switch, tx_q…

--

net: stmmac: Stop Tx queue when (en|dis)abling XSK pools

This patch stops the netdev Tx queue while an XSK pool is enabled or
disabled on a running interface. It does this because
stmmac_disable_tx_queue() and stmmac_enable_tx_queue() free and reallocate
the Tx DMA ring. The queue is woken once the ring is rebuilt, and
stmmac_xdp_xmit_xdpf() now drops XDP frames while the queue is stopped.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 27c4a249e560..2083520179a6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5270,6 +5270,10 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
>  	if (unlikely(xdp_frame_has_frags(xdpf)))
>  		num_frames += sinfo->nr_frags;
>  
> +	/* Ring may be torn down for an XSK pool switch */
> +	if (netif_tx_queue_stopped(netdev_get_tx_queue(priv->dev, queue)))
> +		return STMMAC_XDP_CONSUMED;
> +

[Severity: Medium]
Is this check early enough? first_entry and entry have already been read
from tx_q->cur_tx at the top of stmmac_xdp_xmit_xdpf():

	unsigned int first_entry = tx_q->cur_tx;
	unsigned int entry = tx_q->cur_tx;

The pool switch path does not hold the Tx lock while it rebuilds the ring
or wakes the queue. stmmac_enable_tx_queue()->stmmac_reset_tx_queue() sets
cur_tx and dirty_tx to 0. After that, netif_tx_wake_queue() clears XOFF
without taking the lock.

Take an XDP sender (stmmac_xdp_xmit_back() or stmmac_xdp_xmit()) that:

1. takes __netif_tx_lock() while the queue is stopped
2. loads the old cur_tx == N
3. is delayed past the whole rebuild and wake, for example by being
   preempted on PREEMPT_RT
4. then finds the queue running and passes stmmac_tx_avail()

Could that sender fill and OWN-mark descriptors at the stale index N on
the new ring and then set cur_tx = N + 1?

DMA restarts at descriptor 0, which nobody owns. That looks like it would
stall Tx. stmmac_tx_clean() would then walk entries 0..N-1 as if they had
completed.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> index 9af9e0cb366c..14445683a5c3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> @@ -6,6 +6,15 @@
>  #include "stmmac.h"
>  #include "stmmac_xdp.h"
>  
> +static void stmmac_xdp_stop_tx_queue(struct stmmac_priv *priv, u16 queue)
> +{
> +	struct netdev_queue *nq = netdev_get_tx_queue(priv->dev, queue);
> +
> +	__netif_tx_lock_bh(nq);
> +	netif_tx_stop_queue(nq);
> +	__netif_tx_unlock_bh(nq);
> +}

[Severity: Medium]
Can stmmac_tx_err() undo this stop while the pool switch is still running?

stmmac_tx_err() runs in hardirq context from stmmac_dma_interrupt() and
stmmac_msi_intr_tx() when the channel status is tx_hard_error. It takes no
lock and always finishes by waking the queue:

stmmac_tx_err() {
	...
	stmmac_stop_tx_dma(priv, chan);
	dma_free_tx_skbufs(priv, &priv->dma_conf, chan);
	stmmac_clear_tx_descriptors(priv, &priv->dma_conf, chan);
	stmmac_reset_tx_queue(priv, chan);
	stmmac_init_tx_chan(priv, priv->ioaddr, priv->plat->dma_cfg,
			    tx_q->dma_tx_phy, chan);
	stmmac_start_tx_dma(priv, chan);

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

The switch does not seem to keep it out. dwmac4_disable_dma_irq() clears
only RIE and TIE, so AIE, RPS, RBU and FBE stay enabled. There is also no
synchronize_irq().

Stopping RX DMA during the switch raises RPS, and with it AIS.
dwmac4_dma_interrupt() decodes AIS together with TPS or FBE as
tx_hard_error.

TPS is never acknowledged, because only the enabled bits get written back:

	writel(intr_status & intr_en,
	       ioaddr + DMA_CHAN_STATUS(dwmac4_addrs, chan));

That means TPS can stay latched from an earlier Tx DMA stop.

Suppose tx_hard_error is decoded inside the stop-to-wake window. Wouldn't
stmmac_tx_err() then work on the freed or half-rebuilt ring and restart
DMA on freed coherent memory? It would also reopen the queue for
stmmac_xmit() and the XDP Tx paths.

The source alone does not show whether the hardware reports exactly
tx_hard_error in that window. For example, a latched RI bit would change
the status value.

[ ... ]

> @@ -46,6 +56,7 @@ static int stmmac_xdp_enable_pool(struct stmmac_priv *priv,
>  		stmmac_enable_rx_queue(priv, queue);
>  		stmmac_enable_tx_queue(priv, queue);
>  		napi_enable(&ch->rxtx_napi);
> +		netif_tx_wake_queue(netdev_get_tx_queue(priv->dev, queue));

[Severity: High]
What happens here if stmmac_enable_tx_queue() failed? It returns void.
When an allocation fails, it only logs a message and returns:

stmmac_enable_tx_queue() {
	...
	ret = __alloc_dma_tx_desc_resources(priv, &priv->dma_conf, queue);
	if (ret) {
		netdev_err(priv->dev, "Failed to alloc TX desc.\n");
		return;
	}
	...
}

The state left behind depends on which allocation in
__alloc_dma_tx_desc_resources() failed:

- tx_q->tx_skbuff_dma or tx_q->tx_skbuff is NULL, or
- dma_tx/dma_etx/dma_entx still point at the coherent ring that
  __free_dma_tx_desc_resources() just freed. That function never clears
  those pointers.

stmmac_reset_tx_queue() is skipped as well, so cur_tx and dirty_tx keep
their old values.

Once this unconditional wake runs, stmmac_xmit() can execute:

	WARN_ON(tx_q->tx_skbuff[first_entry]);

It would then write tx_skbuff/tx_skbuff_dma entries, and descriptors via
stmmac_get_tx_desc(), into NULL or freed memory. The new
netif_tx_queue_stopped() check in stmmac_xdp_xmit_xdpf() is open again
too.

stmmac_xdp_disable_pool() gets the same unconditional wake.

The commit message says the crash comes from Tx touching the ring while it
is freed and reallocated. On this error path, that still seems reachable.

Could stmmac_enable_tx_queue() return the error, so the queue stays
stopped (or the switch is unwound) instead of being woken?

[ ... ]

> @@ -87,6 +99,7 @@ static int stmmac_xdp_disable_pool(struct stmmac_priv *priv, u16 queue)
>  		stmmac_enable_tx_queue(priv, queue);
>  		napi_enable(&ch->rx_napi);
>  		napi_enable(&ch->tx_napi);
> +		netif_tx_wake_queue(netdev_get_tx_queue(priv->dev, queue));
>  	}
>  
>  	return 0;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. After
stmmac_enable_tx_queue() fails, tx_q looks set up for a double free on the
next teardown.

__alloc_dma_tx_desc_resources() does no unwind. It returns -ENOMEM at the
first step that fails:

	tx_q->tx_skbuff_dma = kzalloc_objs(*tx_q->tx_skbuff_dma,
					   dma_conf->dma_tx_size);
	if (!tx_q->tx_skbuff_dma)
		return -ENOMEM;

	tx_q->tx_skbuff = kzalloc_objs(struct sk_buff *, dma_conf->dma_tx_size);
	if (!tx_q->tx_skbuff)
		return -ENOMEM;
	...
	addr = dma_alloc_coherent(priv->device, size,
				  &tx_q->dma_tx_phy, GFP_KERNEL);
	if (!addr)
		return -ENOMEM;

__free_dma_tx_desc_resources() never clears dma_tx/dma_etx/dma_entx,
tx_skbuff or tx_skbuff_dma. The next teardown therefore calls it again on
the stale pointers. That teardown can be stmmac_disable_tx_queue() during
another pool switch, or free_dma_tx_desc_resources() from stmmac_release().

Wouldn't that call dma_free_coherent() a second time on the ring that was
already freed?

If the tx_skbuff_dma allocation was the one that failed, two more things
happen:

- dma_free_tx_skbufs() dereferences the NULL tx_skbuff_dma in
  stmmac_free_tx_buffer()
- kfree(tx_q->tx_skbuff) frees the old array a second time

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