Re: [PATCH net v2 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 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (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] The patch only stops the netdev Tx queue, and the driver's own
  XDP transmit paths ignore that.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the new wake-
  up makes it visible: if stmmac_enable_tx_queue() fails, the Tx queue…

--

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

This patch stops the netdev Tx queue in stmmac_xdp_enable_pool() and
stmmac_xdp_disable_pool() while the Tx DMA descriptor resources are freed
and reallocated. The queue is woken once the ring has been rebuilt. The goal
is to fix crashes in stmmac_xmit() when XSK pools are toggled while Tx
traffic is running.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> index d7e4db7224b0c..883bd3fe8089d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> @@ -6,6 +6,16 @@
>  #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);
> +	txq_trans_cond_update(nq);
> +	netif_tx_stop_queue(nq);
> +	__netif_tx_unlock_bh(nq);
> +}

[Severity: High]

Does setting __QUEUE_STATE_DRV_XOFF here keep the driver's own XDP
transmit paths away from the ring while it is freed and reallocated?

The Tx lock is released before stmmac_disable_tx_queue() calls
__free_dma_tx_desc_resources(). DRV_XOFF only stops callers that reach
ndo_start_xmit through the stack.

Both XDP Tx paths in stmmac_main.c choose the queue by CPU. Neither checks
whether that queue is stopped before calling stmmac_xdp_xmit_xdpf():

stmmac_xdp_xmit_back() {
    ...
	queue = stmmac_xdp_get_tx_queue(priv, cpu);
	nq = netdev_get_tx_queue(priv->dev, queue);

	__netif_tx_lock(nq, cpu);
    ...
	res = stmmac_xdp_xmit_xdpf(priv, queue, xdpf, zc);
    ...
}

stmmac_xdp_xmit() {
    ...
	if (unlikely(test_bit(STMMAC_DOWN, &priv->state)))
		return -ENETDOWN;
    ...
	__netif_tx_lock(nq, cpu);
    ...
		res = stmmac_xdp_xmit_xdpf(priv, queue, frames[i], true);
    ...
}

need_update requires stmmac_xdp_is_enabled(), so an XDP program is always
attached while this window is open. Only the NAPI instances of this one
channel are disabled.

Two producers can still reach this queue during the window:

- RX NAPI of another channel, running on a CPU that
  stmmac_xdp_get_tx_queue() maps to this queue, can hit XDP_TX.
- A redirect from another device on such a CPU can enter
  stmmac_xdp_xmit().

Either path then runs:

stmmac_xdp_xmit_xdpf()
    stmmac_get_tx_desc()
    stmmac_set_tx_dma_entry()
    tx_q->xdpf[entry] = ...
    stmmac_set_desc_addr()

These calls can write into the descriptor ring that was released with
dma_free_coherent() and into the kfree()'d tx_skbuff_dma array. They can
also race with __init_dma_tx_desc_rings() while the ring is being rebuilt.

In stmmac_xdp_disable_pool(), synchronize_rcu() only runs after the ring has
already been freed:

		stmmac_disable_tx_queue(priv, queue);
		synchronize_rcu();

so it cannot drain ndo_xdp_xmit callers that are in flight before the free.

Would this need a per-queue gate that the XDP Tx paths check under the Tx
lock, or a drain before the free, rather than DRV_XOFF alone?

[ ... ]

> @@ -46,6 +57,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]

This is a pre-existing issue that this patch did not introduce, but the
new wake-up makes it more visible. What happens here if
stmmac_enable_tx_queue() failed?

stmmac_enable_tx_queue() returns void and only logs the failure:

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

A failure in __init_dma_tx_desc_rings() is handled the same way. Either way
it skips stmmac_init_tx_chan(), stmmac_start_tx_dma() and
stmmac_enable_dma_irq().

__free_dma_tx_desc_resources() never clears tx_q->dma_tx/dma_etx/dma_entx,
tx_skbuff_dma or tx_skbuff. That leaves three problems:

- If dma_alloc_coherent() fails in __alloc_dma_tx_desc_resources(), the
  descriptor pointer still points at the freed coherent memory.
- If the tx_skbuff allocation fails, tx_skbuff is left NULL.
- The earlier partial allocations leak.

netif_tx_wake_queue() is called unconditionally here and in
stmmac_xdp_disable_pool(). The next stmmac_xmit() would then dereference
tx_q->tx_skbuff[first_entry]. It would also write descriptors through
stmmac_get_tx_desc() into the freed ring. That is the same
dwmac4_set_addr()/dwmac4_set_vlan_tag() crash described in the commit
message.

A later stmmac_release()->free_dma_desc_resources() could also free the
stale pointers a second time.

Before this patch the queue was never stopped, so traffic hit the broken
ring the same way.

Should the failure be returned to the callers, the partial allocations
unwound, and the queue kept stopped until a valid ring is in place?

>  
>  		err = stmmac_xsk_wakeup(priv->dev, queue, XDP_WAKEUP_RX);
>  		if (err)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005-stmmac_xsk_crashes-v2-0-46c60cba6421%40linutronix.de