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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: stmmac: stmmac_xdp_xmit() flushes TX descriptors when queue is stopped
--

commit c900d4172ba56a5bcf30305f3561e2f9948f9892
Author: Kurt Kanzenbach <kurt@linutronix.de>

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

This commit stops the network device Tx queue while enabling or disabling
XSK pools so that concurrent transmissions do not access Tx DMA descriptors
while the Tx ring is being freed and reallocated. It also adds a check in
stmmac_xdp_xmit_xdpf() to bail out when the Tx queue is stopped.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 27c4a249e560f..2083520179a69 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;
> +
>  	if (stmmac_tx_avail(priv, queue) < max(num_frames, txq_thr))
>  		return STMMAC_XDP_CONSUMED;
>  

[Severity: High]
In drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c,
stmmac_xdp_stop_tx_queue() releases nq->_xmit_lock immediately after setting
__QUEUE_STATE_DRV_XOFF, so stmmac_disable_tx_queue() and
stmmac_enable_tx_queue() run without holding nq->_xmit_lock.

If stmmac_xdp_xmit() in drivers/net/ethernet/stmicro/stmmac/stmmac_main.c is
invoked concurrently with XDP_XMIT_FLUSH while the queue is stopped for an
XSK pool switch, stmmac_xdp_xmit_xdpf() returns STMMAC_XDP_CONSUMED and breaks
out of the frame loop, but stmmac_xdp_xmit() still executes the flush block:

	for (i = 0; i < num_frames; i++) {
		int res;

		res = stmmac_xdp_xmit_xdpf(priv, queue, frames[i], true);
		if (res == STMMAC_XDP_CONSUMED)
			break;

		nxmit++;
	}

	if (flags & XDP_XMIT_FLUSH) {
		stmmac_flush_tx_descriptors(priv, queue);
		stmmac_tx_timer_arm(priv, queue);
	}

stmmac_flush_tx_descriptors() reads tx_q->dma_tx_phy and tx_q->cur_tx and
writes to the hardware Tx tail pointer register via
stmmac_set_queue_tx_tail_ptr():

static void stmmac_flush_tx_descriptors(struct stmmac_priv *priv, int queue)
{
	struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
	...
	wmb();

	stmmac_set_queue_tx_tail_ptr(priv, tx_q, queue, tx_q->cur_tx);
}

Meanwhile, stmmac_disable_tx_queue() frees the old ring without resetting
tx_q->cur_tx, and stmmac_enable_tx_queue() updates tx_q->dma_tx_phy, resets
tx_q->cur_tx to 0, programs the tail pointer to 0, and starts Tx DMA before
netif_tx_wake_queue() is called.

Can a concurrent stmmac_xdp_xmit() call corrupt the hardware Tx tail pointer
register with a stale tx_q->cur_tx or arm tx_q->txtimer while the Tx queue is
being reconfigured, and should thestopped-queue check be performed in
stmmac_xdp_xmit() (or skip flushing when no frames were transmitted)?

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-stmmac_xsk_crashes-v4-0-15c0c750bb96@linutronix.de?part=2