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

[netdev-bot+sashiko@kernel.org]

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

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

- [High] The new @stopping check in axienet_dma_rx_cb() does not actually
  fence RX resubmission against the terminate in axienet_stop(), so the…

Pre-existing issues:
- [High] TX ring head/tail desynchronization causes a NULL pointer
  dereference in axienet_dma_tx_cb().

--

net: xilinx: axienet: Free outstanding DMA buffers on dmaengine stop

In the dmaengine path the driver clears the ring slot's skb pointer in the
TX and RX completion callbacks, so a non-NULL skb marks a slot that still
owns a live DMA-mapped buffer, and axienet_stop() unmaps and frees every
such buffer instead of only kfree()ing the ring shells. It also sets
@stopping before dmaengine_terminate_sync() and checks it in
axienet_dma_rx_cb() to fence RX resubmission, and releases the channels
before freeing the rings.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 1722b7038f340..fc314f67f157c 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -881,6 +881,7 @@ static void axienet_dma_tx_cb(void *data, const struct dmaengine_result *result)
>  	u64_stats_update_end(&lp->tx_stat_sync);
>  	dma_unmap_sg(lp->dev, skbuf_dma->sgl, skbuf_dma->sg_len, DMA_TO_DEVICE);
>  	dev_consume_skb_any(skbuf_dma->skb);
> +	skbuf_dma->skb = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but can clearing the slot's skb
here turn an existing TX ring desynchronization into a NULL pointer
dereference?

axienet_start_xmit_dmaengine() advances the head before the fallible steps:

	lp->tx_ring_head++;
	sg_init_table(skbuf_dma->sgl, sg_len);
	ret = skb_to_sgvec(skb, skbuf_dma->sgl, 0, skb->len);
	if (ret < 0)
		goto xmit_error_drop_skb;

	ret = dma_map_sg(lp->dev, skbuf_dma->sgl, sg_len, DMA_TO_DEVICE);
	if (!ret)
		goto xmit_error_drop_skb;

None of the xmit_error_* labels rewind lp->tx_ring_head, and they return
NETDEV_TX_OK without ever calling dmaengine_submit(), so that slot is
consumed with no descriptor behind it and skbuf_dma->skb is never assigned.

axienet_dma_tx_cb() pairs completions with slots positionally:

	skbuf_dma = axienet_get_tx_desc(lp, lp->tx_ring_tail++);
	len = skbuf_dma->skb->len;
	txq = skb_get_tx_queue(lp->ndev, skbuf_dma->skb);

so after one skipped slot the head/tail pairing is off by one for the life of
the interface and the next completion lands on the skipped slot.

Before this patch that read a stale, already consumed skb pointer. With
skbuf_dma->skb = NULL set on every completion, the skipped slot's skb is
deterministically NULL, so the dereference above runs on NULL from the DMA
channel tasklet.

The triggering condition is a single dma_map_sg() failure, i.e. the same
IOMMU aperture exhaustion the commit message describes as motivation. Would
it be reasonable to advance lp->tx_ring_head only after
device_prep_slave_sg() succeeds, or unwind it on the pre-submit error paths,
as part of this series?

> @@ -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())
> @@ -1193,6 +1195,12 @@ static void axienet_dma_rx_cb(void *data, const struct dmaengine_result *result)
>  	u64_stats_update_end(&lp->rx_stat_sync);
>  
>  rx_submit:
> +	/* Do not re-arm the RX ring while a stop is in progress, or the
> +	 * teardown could free a buffer still handed to the engine.
> +	 */
> +	if (READ_ONCE(lp->stopping))
> +		return;
> +
>  	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);

[Severity: High]
Does this check actually fence resubmission against the terminate?

axienet_dma_rx_cb() runs from the xilinx DMA channel tasklet, which drops
chan->lock around dmaengine_desc_get_callback_invoke() in
xilinx_dma_chan_desc_cleanup(), concurrently with axienet_stop() in process
context. READ_ONCE()/WRITE_ONCE() only prevent tearing, they do not make the
check-then-submit sequence atomic:

CPU0 (DMA tasklet)                     CPU1 (axienet_stop)
axienet_dma_rx_cb()
  READ_ONCE(lp->stopping) == false
                                       WRITE_ONCE(lp->stopping, true);
                                       dmaengine_terminate_sync(rx_chan);
  axienet_rx_submit_desc()
    skbuf_dma->skb = skb;
    dmaengine_submit(dma_rx_desc);
  dma_async_issue_pending(lp->rx_chan);

Note that dmaengine_terminate_sync() ends in dmaengine_synchronize(), which
for this provider is xilinx_dma_synchronize() -> tasklet_kill(): it waits for
the running tasklet to finish rather than discarding what that tasklet
submits.

Does the late submit re-arm the hardware? xilinx_dma_tx_submit() queues the
descriptor and clears the provider's terminate state:

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

and xilinx_dma_terminate_all() left the channel idle:

	chan->terminating = true;
	xilinx_dma_free_descriptors(chan);
	chan->idle = true;

so the racing dma_async_issue_pending() reaches start_transfer() and
reprograms CURDESC/TAILDESC with DMACR.RUNSTOP set.

Is anything after that point stopping the channel again? dma_release_channel()
-> dma_chan_put() only does:

	if (!chan->client_count && chan->device->device_free_chan_resources) {
		/* Make sure all operations have completed */
		dmaengine_synchronize(chan);
		chan->device->device_free_chan_resources(chan);
	}

and xilinx_dma_free_chan_resources() dma_free_coherent()s chan->seg_v without
a stop_transfer() or reset, so moving the release ahead of the ring free does
not appear to close the window.

The new teardown loop then sees the resubmitted slot:

> +		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);
> +			}

Can this unmap and free a buffer the engine is still writing into, turning
the former mapping leak into DMA write-after-free?

Documentation/driver-api/dmaengine/client.rst also states:

   The behavior of this function is undefined if dma_async_issue_pending() has
   been called between dmaengine_terminate_async() and this function.

which is exactly what the racing callback does inside the window, and this
patch removes the extra dmaengine_synchronize() calls that followed the
terminates.

There is a second placement question: the ring is dereferenced before the new
check,

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

so a completion from a re-armed channel arriving after tasklet_kill() and
after kfree(lp->rx_skb_ring) touches freed memory before the @stopping test is
reached.

The commit message says "Set @stopping before terminating and check it in the
callback to fence resubmission, and release the channels before freeing the
rings so a late completion cannot touch a freed ring." Given the above, does
the check-submit-issue sequence need real serialization against the stop
transition, or a second dmaengine_terminate_sync() once all callbacks are
known to be drained?

> @@ -1752,20 +1761,43 @@ 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;
> +
> +		WRITE_ONCE(lp->stopping, true);
>  		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]);
> -		kfree(lp->tx_skb_ring);
> -		for (i = 0; i < RX_BUF_NUM_DEFAULT; i++)
> -			kfree(lp->rx_skb_ring[i]);
> -		kfree(lp->rx_skb_ring);
>  
> +		/* Release the channels before freeing the rings, so the DMA is
> +		 * fully torn down before the memory its descriptors reference is
> +		 * freed.
> +		 */
>  		dma_release_channel(lp->rx_chan);
>  		dma_release_channel(lp->tx_chan);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917100525.250952-1-suraj.gupta2%40amd.com