Re: [PATCH net] net: stmmac: Clear DMA errors for multi-channel interrupts

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 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] The new common-MSI error path runs stmmac_tx_err() without any
  synchronization against TX NAPI or the transmit path for the same…
- [Medium] In multi-MSI mode the patch adds new callers of
  stmmac_tx_err() for a channel, and nothing serializes them.
- [Medium] Use-after-free of the TX ring during AF_XDP pool
  setup/teardown while the device is up.

--

net: stmmac: Clear DMA errors for multi-channel interrupts

On dwmac4, the multi-MSI MAC interrupt handler now clears the per-channel
DMA error status bits (AIS, FBE, RPS, RBU) through a new DMA_DIR_ERR
direction. This avoids interrupt storms. The TX error handling is also
factored out into stmmac_dma_tx_error(), and the new
stmmac_dma_err_interrupt() path calls it as well.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9ded31150fbe6..2068b57df8d52 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -3165,13 +3175,24 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)

[ ... ]

> +static void stmmac_dma_err_interrupt(struct stmmac_priv *priv)
> +{
> +	u8 tx_channel_count = priv->plat->tx_queues_to_use;
> +	u8 channels_to_check = max(priv->plat->rx_queues_to_use,
> +				   tx_channel_count);
> +	int status;
> +	u8 chan;
> +
> +	for (chan = 0; chan < channels_to_check; chan++) {
> +		status = stmmac_dma_interrupt_status(priv, priv->ioaddr,
> +						     &priv->xstats, chan,
> +						     DMA_DIR_ERR);
> +		if (chan < tx_channel_count)
> +			stmmac_dma_tx_error(priv, chan, status);

[Severity: High]
Can this race with TX NAPI or stmmac_xmit() on the same queue?

DMA_DIR_ERR masks out RI/TI/TBU. A latched FBE with AIS therefore makes
dwmac4_dma_interrupt() return exactly tx_hard_error, and the MAC IRQ goes
straight into the recovery path:

stmmac_mac_interrupt()
  stmmac_dma_err_interrupt()
    stmmac_dma_tx_error()
      stmmac_tx_err()
        netif_tx_stop_queue()
        stmmac_stop_tx_dma()
        dma_free_tx_skbufs()
        stmmac_clear_tx_descriptors()
        stmmac_reset_tx_queue()

stmmac_tx_err() does not take the txq lock and does not quiesce NAPI. At
the same time, TX NAPI for that queue may be in stmmac_tx_clean() on
another CPU, walking and freeing the same tx_skbuff[] entries under:

	__netif_tx_lock_bh(netdev_get_tx_queue(priv->dev, queue));

While NAPI runs, dwmac4_disable_dma_irq() only clears
DMA_CHAN_INTR_ENA_TIE, so the AIE/FBE-driven MAC line is not blocked.
The TX MSI affinity is set with cpumask_of(i % num_online_cpus()), so
NAPI for queue i will usually run on a different CPU from the MAC IRQ.

netif_tx_stop_queue() also does not wait for a stmmac_xmit() that is
already filling the ring.

Could this cause a double free or use-after-free of skbs and xdp_frames,
duplicate DMA unmaps, or corrupted cur_tx/dirty_tx?

Before this patch, stmmac_tx_err() could only be reached in multi-MSI
mode from stmmac_msi_intr_tx(). That handler is TI-driven, and its source
is masked while NAPI is active.

The single-IRQ path has the same unsynchronized stmmac_tx_err(). Taking
the BH-only txq lock from hardirq context would deadlock. Would it be
better to defer this recovery, for example to the service task or a
reset?

[Severity: Medium]
What happens here if an AF_XDP pool change on a running interface has
freed the TX queue's ring?

stmmac_xdp_enable_pool() and stmmac_xdp_disable_pool() call
stmmac_disable_tx_queue():

stmmac_disable_tx_queue() {
	spin_lock_irqsave(&ch->lock, flags);
	stmmac_disable_dma_irq(priv, priv->ioaddr, queue, 0, 1);
	spin_unlock_irqrestore(&ch->lock, flags);

	stmmac_stop_tx_dma(priv, queue);
	__free_dma_tx_desc_resources(priv, &priv->dma_conf, queue);
}

Only TIE is masked. __free_dma_tx_desc_resources() then frees the
descriptor ring and does the following, without clearing the pointers:

	kfree(tx_q->tx_skbuff_dma);
	kfree(tx_q->tx_skbuff);

During this window the MAC IRQ stays requested, AIE/FBE stay enabled,
and STMMAC_DOWN is not set. stmmac_dma_err_interrupt() walks every
channel below tx_queues_to_use without checking whether its ring is
allocated.

Suppose FBE is latched on that channel before stmmac_enable_tx_queue()
reallocates the ring. Would stmmac_tx_err() then read tx_skbuff_dma[i]
and tx_skbuff[i] from freed memory in stmmac_free_tx_buffer()? And would
stmmac_clear_tx_descriptors() write into freed coherent DMA memory?

Before this patch, no IRQ path touched the TX ring in this window in
multi-MSI mode.

>  	}
>  }

[ ... ]

> @@ -6466,6 +6487,9 @@ static irqreturn_t stmmac_mac_interrupt(int irq, void *dev_id)
>  	/* To handle Common interrupts */
>  	stmmac_common_interrupt(priv);
>  
> +	if (priv->plat->core_type == DWMAC_CORE_GMAC4)
> +		stmmac_dma_err_interrupt(priv);
> +

[Severity: Medium]
Can stmmac_tx_err() now run on the same channel from two CPUs at once?

In multi-MSI mode, stmmac_request_irq_multi_msi() installs
stmmac_mac_interrupt() on dev->irq. When wol_irq is a different line, it
installs the same handler there too:

	if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) {
		...
		ret = request_irq(priv->wol_irq,
				  stmmac_mac_interrupt,
				  0, int_name, dev);

dwmac-intel, for example, sets res->wol_irq to a separate MSI vector.

stmmac_msi_intr_tx() can also reach tx_hard_error on the same channel,
because FBE and AIS are part of DMA_CHAN_STATUS_MSK_TX through
DMA_CHAN_STATUS_MSK_COMMON.

dwmac4_dma_interrupt() reads the status first and clears it later:

	u32 intr_status = readl(ioaddr + DMA_CHAN_STATUS(dwmac4_addrs, chan));
	...
	writel(intr_status & intr_en,
	       ioaddr + DMA_CHAN_STATUS(dwmac4_addrs, chan));

Two of these handlers could both see FBE before either one clears it,
and both would then call stmmac_tx_err() on the same tx_q.
stmmac_free_tx_buffer() does an unlocked check-then-free:

	if (tx_q->tx_skbuff[i] &&
	    tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_SKB) {
		dev_kfree_skb_any(tx_q->tx_skbuff[i]);
		tx_q->tx_skbuff[i] = NULL;
	}

Could this double free the skb or xdp_frame and double unmap
tx_skbuff_dma[i].buf? Could it also race on descriptor clearing, ring
reset and TX DMA channel reprogramming?

In INTM=1 the TX MSI normally fires with TI set. That gives
tx_hard_error | handle_tx, which fails the == tx_hard_error check, so the
TX MSI case probably needs a status snapshot without TI. The MAC plus
WoL case needs the WoL vector to fire at the same moment at runtime.

The single-IRQ path already has a similar MAC plus WoL race, since
stmmac_interrupt() is also requested on wol_irq there. In multi-MSI mode,
this patch is what introduces it.

>  	return IRQ_HANDLED;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030102251.208929-1-maxime.chevallier%40bootlin.com