Re: [PATCH net v3] net: xilinx: axienet: Free outstanding DMA buffers on dmaengine stop

[Simon Horman]

On Thu, Sep 24, 2026 at 02:24:41PM +0530, Suraj Gupta wrote:
> In the dmaengine path the driver pre-submits RX buffers and holds
> in-flight TX buffers whose SKBs are DMA-mapped by the driver and freed
> only in the completion callbacks. On ndo_stop() dmaengine_terminate_sync()
> aborts these descriptors without running their callbacks, and the driver
> then frees only the ring shells, leaking every SKB still owned by the
> engine and its DMA mapping on each ifdown. With 128 RX buffers pre-posted
> per channel, the mapping leak can eventually exhaust a limited IOMMU
> aperture.
> 
> Clear the slot's skb in the TX and RX callbacks so a non-NULL skb marks a
> slot that still owns a live, DMA-mapped buffer, and on stop unmap and free
> every such buffer.
> 
> axienet_dma_rx_cb() runs from the DMA tasklet and re-arms the RX ring on
> each completion, so it can race axienet_stop(): a completion may submit a
> fresh buffer after dmaengine_terminate_sync() has returned, leaving the
> channel armed with a buffer the teardown then frees while the engine may
> still write into it (dma_release_channel() does not stop it either). Add a
> lock that axienet_dma_rx_cb() holds across the @stopping check and the
> resubmit, and axienet_stop() holds to set @stopping before terminating.
> Once @stopping is set no callback can arm a new buffer, and any armed just
> before is aborted by the terminate, so teardown only frees buffers the
> engine no longer owns.
> 
> Fixes: 6a91b846af85 ("net: axienet: Introduce dmaengine support")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Suraj Gupta <suraj.gupta2@xxxxxxx>
> ---
> Changes in v3:
> - Serialize RX descriptor resubmission in axienet_dma_rx_cb() against the
>   stop with a dedicated rx_submit_lock, replacing the bare
>   READ_ONCE/WRITE_ONCE @stopping fence. (reported by the netdev Sashiko
>   AI bot).
> - Update the commit message to describe the race and its fix.

The use of a spinlock looks correct to me.
But, if so, I think that plain acecsses to stopping should be used:
READ_ONCE/WRITE_ONCE wrappers should be removed.

I mean, like this:

	spin_lock(&lp->rx_submit_lock);
	if (lp->stopping) {
		spin_unlock(&lp->rx_submit_lock);
		return;
	}
	...
	spin_unlock(&lp->rx_submit_lock);

And:

		spin_lock_bh(&lp->rx_submit_lock);
		lp->stopping = true;
		spin_unlock_bh(&lp->rx_submit_lock);

> v2: https://lore.kernel.org/netdev/20260917100525.250952-1-suraj.gupta2@xxxxxxx/
> 
> Changes in v2:
> - Free outstanding TX/RX buffers on the dmaengine stop path (the original
>   fix), and drop the redundant dmaengine_synchronize() calls that
>   followed dmaengine_terminate_sync() (Jakub Kicinski).
> v1: https://lore.kernel.org/netdev/20260910141946.3017164-1-suraj.gupta2@xxxxxxx/

-- 
pw-bot: changes-requested