Re: [PATCH net v3 1/2] net: nixge: free IRQs before killing the DMA error tasklet

[netdev-bot+sashiko@kernel.org]

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

Critical: 1 · High: 0 (3) · 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).

- [Critical] In this commit, nixge_stop() in
  drivers/net/ethernet/ni/nixge.c clears RUNSTOP on both channels (lines…

Pre-existing issues:
- [High] nixge_dma_err_handler() runs as a tasklet (softirq, atomic
  context).
- [High] nixge_hw_dma_bd_release() frees priv->rx_bd_v, its RX skbs,
  priv->tx_skb and priv->tx_bd_v, but never sets those pointers to NULL.
- [High] nixge_dma_err_handler() takes no lock and does not quiesce NAPI
  or the TX queue.

--

net: nixge: free IRQs before killing the DMA error tasklet

This patch reorders nixge_stop() so the TX and RX IRQs are freed before
tasklet_kill() runs on dma_err_tasklet. The aim is to stop the IRQ
handlers from rescheduling the error tasklet after it has been drained
and the descriptor rings have been released.

> diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
> index 230d5ff99dd7e..8697a76241394 100644
> --- a/drivers/net/ethernet/ni/nixge.c
> +++ b/drivers/net/ethernet/ni/nixge.c
> @@ -927,11 +927,12 @@ static int nixge_stop(struct net_device *ndev)
>  	nixge_dma_write_reg(priv, XAXIDMA_TX_CR_OFFSET,
>  			    cr & (~XAXIDMA_CR_RUNSTOP_MASK));
>  
> -	tasklet_kill(&priv->dma_err_tasklet);
> -
> +	/* Remove both producers before draining the error tasklet. */
>  	free_irq(priv->tx_irq, ndev);
>  	free_irq(priv->rx_irq, ndev);
>  
> +	tasklet_kill(&priv->dma_err_tasklet);
> +
>  	nixge_hw_dma_bd_release(ndev);

[Severity: Critical]
Can the error tasklet restart the DMA engine here, after RUNSTOP has
already been cleared above?

Suppose nixge_tx_irq() or nixge_rx_irq() scheduled dma_err_tasklet
before free_irq(). Then tasklet_kill() waits for it to run to
completion. nixge_dma_err_handler() resets the AXI DMA, re-enables
XAXIDMA_IRQ_ALL_MASK, and sets RUNSTOP on both channels again. The RX
channel ends up owning the whole RX ring:

nixge_dma_err_handler() {
    ...
	nixge_dma_write_desc_reg(lp, XAXIDMA_RX_CDESC_OFFSET, lp->rx_bd_p);
	cr = nixge_dma_read_reg(lp, XAXIDMA_RX_CR_OFFSET);
	nixge_dma_write_reg(lp, XAXIDMA_RX_CR_OFFSET,
			    cr | XAXIDMA_CR_RUNSTOP_MASK);
	nixge_dma_write_desc_reg(lp, XAXIDMA_RX_TDESC_OFFSET, lp->rx_bd_p +
			    (sizeof(*lp->rx_bd_v) * (RX_BD_NUM - 1)));
    ...
	nixge_dma_write_reg(lp, XAXIDMA_TX_CR_OFFSET,
			    cr | XAXIDMA_CR_RUNSTOP_MASK);
}

Nothing stops the engine again before nixge_hw_dma_bd_release() unmaps
and frees the RX skbs and calls dma_free_coherent() on both rings.

Could the device then DMA received frames into freed memory, or fetch
freed descriptors?

The old ordering (RUNSTOP clear, tasklet_kill(), free_irq()) had the
same restart window, so the reordering does not create it. The next
patch in this series, "net: nixge: wait for DMA reset before releasing
buffers", looks like it closes the window. After tasklet_kill() it
clears RUNSTOP and IRQ_ALL on both channels again and does a polled AXI
DMA reset before the release. If the reset fails, it keeps the buffers.

>  
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but nixge_stop() now relies
on draining dma_err_tasklet. Is it safe for that tasklet to call a
sleeping poll helper?

nixge_dma_err_handler() runs in softirq context and calls
__nixge_device_reset() for both channels, which does:

__nixge_device_reset() {
    ...
	err = nixge_dma_poll_timeout(priv, offset, status,
				     !(status & XAXIDMA_CR_RESET_MASK), 10,
				     1000);
    ...
}

nixge_dma_poll_timeout() wraps readl_poll_timeout(). With a nonzero
sleep_us, poll_timeout_us() calls might_sleep_if() and then
usleep_range() between polls.

Wouldn't every error recovery run trigger a "sleeping function called
from invalid context" warning? And if the reset bit has not cleared on
the first read, wouldn't it hit "scheduling while atomic"?

Using readl_poll_timeout_atomic(), or moving recovery into a work item,
would avoid this. The problem goes back to the driver's introduction.

[Severity: High]
This is a pre-existing issue, but nixge_hw_dma_bd_release(), called
from nixge_stop() above, frees priv->rx_bd_v, the RX skbs, priv->tx_skb
and priv->tx_bd_v. It never sets those pointers to NULL.

nixge_hw_dma_bd_init() assigns the pointers one at a time and calls
nixge_hw_dma_bd_release() on failure:

nixge_hw_dma_bd_init() {
    ...
	priv->tx_bd_v = dma_alloc_coherent(ndev->dev.parent,
					   sizeof(*priv->tx_bd_v) * TX_BD_NUM,
					   &priv->tx_bd_p, GFP_KERNEL);
	if (!priv->tx_bd_v)
		goto out;
    ...
out:
	nixge_hw_dma_bd_release(ndev);
	return -ENOMEM;
}

Consider an open/stop cycle followed by a second open where the tx_bd_v
or tx_skb allocation fails. At that point priv->rx_bd_v still points at
the ring that was already freed.

Would release then read freed descriptor memory, call
dma_unmap_single() on stale addresses, call dev_kfree_skb() on skbs
that were already freed, and call dma_free_coherent() on the RX ring a
second time?

A stale tx_skb would also reach devm_kfree() for a resource that no
longer exists, which triggers the WARN_ON in devres_destroy().

nixge_device_reset() only logs the -ENOMEM:

	if (nixge_hw_dma_bd_init(ndev))
		netdev_err(ndev, "%s: descriptor allocation failed\n",
			   __func__);

So nixge_open() carries on with tx_bd_v == NULL or with freed rings.
Couldn't nixge_start_xmit() then dereference priv->tx_bd_v, and the
next nixge_stop() release the same stale pointers again?

[Severity: High]
This isn't a bug introduced by this patch, but nixge_dma_err_handler()
takes no lock and does not quiesce NAPI or the TX queue while it walks
the rings:

nixge_dma_err_handler() {
    ...
	for (i = 0; i < TX_BD_NUM; i++) {
		cur_p = &lp->tx_bd_v[i];
		tx_skb = &lp->tx_skb[i];
		nixge_tx_skb_unmap(lp, tx_skb);
    ...
	lp->tx_bd_ci = 0;
	lp->tx_bd_tail = 0;
	lp->rx_bd_ci = 0;
    ...
}

TX and RX use separate IRQ lines. An RX error can schedule the tasklet
while nixge_tx_irq()->nixge_start_xmit_done() is running on another
CPU. In that case both sides call nixge_tx_skb_unmap() on the same
entries:

nixge_tx_skb_unmap() {
    ...
	if (tx_skb->skb) {
		dev_kfree_skb_any(tx_skb->skb);
		tx_skb->skb = NULL;
	}
}

Can both sides pass the tx_skb->skb check and free the same skb twice,
or unmap the same mapping twice? Clearing XAXIDMA_IRQ_ALL_MASK in the
error path only blocks future interrupts. It does not stop a handler
that is already running.

nixge_start_xmit() has the same exposure. Only the netdev TX lock
serializes it, so it can fill tx_skb[] and advance tx_bd_tail while the
tasklet clears and resets them. nixge_recv() under NAPI can also
advance rx_bd_ci while the tasklet resets it.

Could this free an skb that was just queued, or leave the ring indices
out of sync?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018020840.3851552-1-mhun512%40gmail.com