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

[Jakub Kicinski <kuba@kernel.org>]

Let's fix what AI is pointing out in another patch in the same series?
Or please explain in the commit msg why the issue is not real,
I'm not very familiar with the DMA engine API, TBH.

AI review:
---
net: xilinx: axienet: Free outstanding DMA buffers on dmaengine stop

This adds clearing of the ring slot skb pointer in the TX and RX dmaengine
completion callbacks, and makes axienet_stop() walk the TX and RX rings
after terminating the channels to unmap and free any slot that still has a
non-NULL skb, instead of only kfree()ing the ring shells.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 1722b7038f340..7828fbb09dc87 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c

[ ... ]

> @@ -1171,6 +1172,7 @@ static void axienet_dma_rx_cb(void *data, const struct dmaengine_result *result)
>  						       &meta_max_len);
>  	dma_unmap_single(lp->dev, skbuf_dma->dma_address, lp->max_frm_size,
>  			 DMA_FROM_DEVICE);
> +	skbuf_dma->skb = NULL;
>  
>  	if (IS_ERR(app_metadata)) {
>  		if (net_ratelimit())

> @@ -1752,16 +1754,40 @@ static int axienet_stop(struct net_device *ndev)
>  		free_irq(lp->rx_irq, ndev);
>  		axienet_dma_bd_release(ndev);
>  	} else {
> +		struct skbuf_dma_descriptor *skbuf_dma;
> +
>  		dmaengine_terminate_sync(lp->tx_chan);
>  		dmaengine_synchronize(lp->tx_chan);
>  		dmaengine_terminate_sync(lp->rx_chan);
>  		dmaengine_synchronize(lp->rx_chan);
>  
> -		for (i = 0; i < TX_BD_NUM_MAX; i++)
> -			kfree(lp->tx_skb_ring[i]);
> +		/* dmaengine_terminate_sync() aborts the descriptors still owned
> +		 * by the DMA engine without running their completion callbacks.
> +		 * A ring slot owns a live, DMA-mapped SKB iff its skb pointer is
> +		 * non-NULL (the callbacks clear it on completion), so unmap and
> +		 * free those here. Otherwise every outstanding TX/RX SKB and its
> +		 * DMA mapping is leaked on ifdown.
> +		 */

[ ... TX ring loop snipped ... ]

> +
> +		for (i = 0; i < RX_BUF_NUM_DEFAULT; i++) {
> +			skbuf_dma = lp->rx_skb_ring[i];
> +			if (skbuf_dma && skbuf_dma->skb) {
> +				dma_unmap_single(lp->dev, skbuf_dma->dma_address,
> +						 lp->max_frm_size, DMA_FROM_DEVICE);
> +				dev_kfree_skb_any(skbuf_dma->skb);
> +			}
> +			kfree(skbuf_dma);
> +		}
>  		kfree(lp->rx_skb_ring);
>  
>  		dma_release_channel(lp->rx_chan);

[Severity: High]

Is the invariant described in the comment, "A ring slot owns a live,
DMA-mapped SKB iff its skb pointer is non-NULL", guaranteed to hold once
the terminate calls have returned?

axienet_dma_rx_cb() re-arms the RX channel unconditionally at the end of
every completion, with no check against a stopping flag or
netif_running():

drivers/net/ethernet/xilinx/xilinx_axienet_main.c:axienet_dma_rx_cb() {
	...
rx_submit:
	for (i = 0; i < CIRC_SPACE(lp->rx_ring_head, lp->rx_ring_tail,
				   RX_BUF_NUM_DEFAULT); i++)
		axienet_rx_submit_desc(lp->ndev);
	dma_async_issue_pending(lp->rx_chan);
}

Note lp->stopping is only set in the !lp->use_dmaengine branch of
axienet_stop(), so it does not fence this path.

The callback runs from the provider tasklet with chan->lock dropped, so it
can run on another CPU while axienet_stop() is between the terminate and
the free loop:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_chan_desc_cleanup() {
	...
	spin_unlock_irqrestore(&chan->lock, flags);
	dmaengine_desc_get_callback_invoke(&desc->async_tx, &result);
	spin_lock_irqsave(&chan->lock, flags);
}

If the callback wins that race, axienet_rx_submit_desc() maps a fresh
buffer, installs it in the slot and submits it:

	skbuf_dma->skb = skb;
	skbuf_dma->dma_address = sg_dma_address(skbuf_dma->sgl);
	skbuf_dma->desc = dma_rx_desc;
	...
	lp->rx_ring_head++;
	dmaengine_submit(dma_rx_desc);

and xilinx_dma_tx_submit() queues it and clears the provider's guard:

	append_desc_queue(chan, desc);
	...
	chan->terminating = false;

The following dma_async_issue_pending() then restarts the channel.
dmaengine_synchronize() is only tasklet_kill() here:

drivers/dma/xilinx/xilinx_dma.c:
static void xilinx_dma_synchronize(struct dma_chan *dchan)
{
	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);

	tasklet_kill(&chan->tasklet);
}

so it waits for the callback to return but does not abort the descriptor the
callback just submitted.

In that case the new RX loop sees skbuf_dma->skb != NULL for that slot and
performs dma_unmap_single() plus dev_kfree_skb_any() on a buffer the engine
still owns.  Can this let the hardware write into a freed, unmapped skb, or
trigger an IOMMU fault?

There is also the ordering of kfree(lp->rx_skb_ring) before
dma_release_channel(lp->rx_chan).  If the late descriptor completes in that
window, axienet_dma_rx_cb() does:

	skbuf_dma = axienet_get_rx_desc(lp, lp->rx_ring_tail++);
	skb = skbuf_dma->skb;

Is that a use-after-free of lp->rx_skb_ring?

Before this change the same window only leaked the buffer.  Would it make
sense to fence resubmission first, for example by setting the stopping flag
for the dmaengine path before the terminate calls and checking it in
axienet_dma_rx_cb() before the rx_submit loop, and to release the channels
before freeing the rings?

Note also that dmaengine_terminate_sync() already ends with
dmaengine_synchronize(), so the explicit dmaengine_synchronize() calls that
follow it here are redundant, though harmless.
-- 
pw-bot: cr