Re: [PATCH net] net: axienet: free outstanding TX buffers in axienet_dma_bd_release()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (2) · Low: 2

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).

- [Medium] The patch adds an unguarded walk of lp->tx_bd_v[] to
  axienet_dma_bd_release()…
- [Low] The new TX walk frees never-transmitted, in-flight skbs with
  dev_kfree_skb(), which include/linux/skbuff.h:1479 defines as…
- [Low] The commit message justifies the new unmap/free by saying 'The
  DMA engine has been stopped by then, so the hardware no longer…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the same
  interface-down leak remains in the dmaengine configuration…
- [Medium] This problem wasn't introduced by this patch, but
  axienet_open() allocates the BD rings and maps the RX skbs via…

--

net: axienet: free outstanding TX buffers in axienet_dma_bd_release()

This patch makes axienet_dma_bd_release() walk the legacy DMA TX
descriptor ring before freeing it. It unmaps every descriptor whose cntrl
is still set and frees any skb still attached. Frames that are in flight
when the interface goes down are then no longer leaked.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 1722b7038f340..02bcb89d1bbef 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -187,6 +187,24 @@ static void axienet_dma_bd_release(struct net_device *ndev)
>  	struct axienet_local *lp = netdev_priv(ndev);
>  
>  	/* If we end up here, tx_bd_v must have been DMA allocated. */
> +	for (i = 0; i < lp->tx_bd_num; i++) {
> +		struct axidma_bd *cur_p = &lp->tx_bd_v[i];
> +
> +		/* axienet_free_tx_chain() clears cntrl when it reclaims a
> +		 * descriptor, so a non-zero value means the mapping is live.
> +		 */
> +		if (cur_p->cntrl) {

[Severity: Medium]
Can lp->tx_bd_v be NULL or already freed when axienet_stop() gets to this
loop?

The comment above says tx_bd_v must have been DMA allocated. However,
axienet_open() throws away the result of axienet_device_reset():

axienet_open() {
	...
	ret = axienet_device_reset(ndev);
	axienet_unlock_mii(lp);

	ret = phylink_of_phy_connect(lp->phylink, lp->dev->of_node, 0);
	...
}

So open still succeeds in each of these cases:

- __axienet_device_reset() times out and axienet_dma_bd_init() never runs.
  On the first open tx_bd_v is still NULL. On a later open it still points
  at the ring that the previous stop freed.

- dma_alloc_coherent() for the TX ring fails in axienet_dma_bd_init(), which
  leaves tx_bd_v NULL.

- The RX allocation or mapping fails. The out: label then calls
  axienet_dma_bd_release(), which frees both rings but leaves tx_bd_v and
  rx_bd_v pointing at the freed memory.

On the next ifdown, axienet_stop()->axienet_dma_bd_release() reads
cur_p->cntrl and cur_p->skb through that pointer. If the pointer is NULL
this oopses. If the ring was freed, garbage can be passed to
dma_unmap_single() and dev_kfree_skb().

Before this patch the TX side only called dma_free_coherent(), and
dma_free_attrs() returns early for a NULL cpu_addr. So the NULL cases did
no harm when no traffic had been sent.

The RX half of this function already has a guard:

	if (!lp->rx_bd_v)
		return;

Should the TX walk have a similar check? It might also help to clear
tx_bd_v and rx_bd_v after freeing them, and to check the
axienet_device_reset() return value in axienet_open().

> +			dma_addr_t addr = desc_get_phys_addr(lp, cur_p);
> +
> +			dma_unmap_single(lp->dev, addr,
> +					 (cur_p->cntrl &
> +					  XAXIDMA_BD_CTRL_LENGTH_MASK),
> +					 DMA_TO_DEVICE);

[Severity: Low]
The commit message says:

  "The DMA engine has been stopped by then, so the hardware no longer
  references the buffers."

Is that guaranteed? axienet_dma_stop() checks for HALT at most five times
and carries on without any message if HALT never appears. It then ignores
the return value of __axienet_device_reset():

axienet_dma_stop() {
	...
	sr = axienet_dma_in32(lp, XAXIDMA_TX_SR_OFFSET);
	for (count = 0; !(sr & XAXIDMA_SR_HALT_MASK) && count < 5; ++count) {
		msleep(20);
		sr = axienet_dma_in32(lp, XAXIDMA_TX_SR_OFFSET);
	}

	/* Do a reset to ensure DMA is really stopped */
	axienet_lock_mii(lp);
	__axienet_device_reset(lp);
	axienet_unlock_mii(lp);
}

__axienet_device_reset() returns -ETIMEDOUT and logs "DMA reset timeout!"
if the reset bit never clears.

If MM2S is still running at that point, could this loop unmap live TX
mappings and free skb data that the engine is still reading?

The RX half of this function and axienet_dma_err_handler() already rely on
the same assumption, so this patch adds little new exposure. Before the
patch, though, the TX buffers were leaked, which meant they stayed valid
in this failure case.

> +		}
> +		if (cur_p->skb)
> +			dev_kfree_skb(cur_p->skb);

[Severity: Low]
Should these frames be freed as drops? dev_kfree_skb() is consume_skb(),
which marks frames that were never transmitted as successfully consumed.

The commit message says this mirrors axienet_dma_err_handler(). That
function frees the same descriptors with:

		if (cur_p->skb)
			dev_kfree_skb_irq(cur_p->skb);

which reports them as drops. With consume_skb(), frames thrown away at
ifdown do not show up in drop_monitor or the kfree_skb tracepoint.
Something like dev_kfree_skb_any() would keep the two paths consistent.

> +	}
> +
>  	dma_free_coherent(lp->dev,
>  			  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
>  			  lp->tx_bd_v,

[Severity: Medium]
This is a pre-existing issue that this patch did not introduce. The
dmaengine configuration (lp->use_dmaengine) seems to have the same leak on
interface down. In axienet_stop():

	} else {
		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);

xilinx_dma_terminate_all()->xilinx_dma_free_descriptors() frees the done,
active and pending descriptors without running their callbacks.

The TX skb and its dma_map_sg() mapping stored by
axienet_start_xmit_dmaengine() are only released in axienet_dma_tx_cb().
The RX skb and its dma_map_single() mapping stored by
axienet_rx_submit_desc() are only released in axienet_dma_rx_cb().

Does each down cycle therefore leak every posted RX skb and any in-flight
TX skbs, along with their DMA mappings?

[Severity: Medium]
This is also a pre-existing issue that this patch did not introduce.
axienet_open() allocates the BD rings and maps the RX skbs through
axienet_device_reset()->axienet_dma_bd_init() before it calls
axienet_init_legacy_dma(). If a request_irq() fails there, the unwind is:

err_tx_irq:
	napi_disable(&lp->napi_tx);
	napi_disable(&lp->napi_rx);
	cancel_work_sync(&lp->dma_err_task);
	dev_err(lp->dev, "request_irq() failed\n");
	return ret;

axienet_open() then jumps to err_phy, which only undoes phylink and the
stats work. Neither path calls axienet_dma_stop() or
axienet_dma_bd_release().

Are the coherent rings, the RX skbs and their mappings leaked here, with
the DMA engine left running? The next open overwrites tx_bd_v and rx_bd_v.

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