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

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

[Sagi Maimon]

axienet_dma_bd_release() walks the RX ring to unmap and free every
receive buffer before releasing it, but frees the TX descriptor ring
with dma_free_coherent() alone.  Any descriptor that
axienet_free_tx_chain() had not yet reclaimed still holds its skb and
its streaming DMA mapping, and both are lost.

axienet_stop() disables TX NAPI and stops the DMA engine before calling
it, so nothing reclaims those descriptors afterwards.  Bringing the
interface down while frames are in flight therefore leaks up to
lp->tx_bd_num skbs and mappings each time.

Walk the TX ring the way axienet_dma_err_handler() already does: unmap
every descriptor whose cntrl is still set - axienet_free_tx_chain()
clears it on reclaim - and free any skb still attached.  The DMA engine
has been stopped by then, so the hardware no longer references the
buffers.  On the axienet_dma_bd_init() error path the TX ring has just
been allocated zeroed, so the walk does nothing.

This was reported by the Sashiko AI review bot.

Tested on an AXI Ethernet MAC behind a PCIe endpoint: traffic passes,
and after each of ten down/up cycles and five module reloads, all made
with traffic running and each running axienet_dma_bd_release(), traffic
resumes and nothing is logged.  The leak itself was not measured.

Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
Assisted-by: LLM sparse
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---

Notes:
    Found by the Sashiko review of v2 of "net: axienet: bound TX completion
    cleanup by the NAPI budget":
    https://lore.kernel.org/netdev/20260917115657.20697-1-maimon.sagi@gmail.com/
    It is independent of that patch and applies on its own.

 .../net/ethernet/xilinx/xilinx_axienet_main.c  | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 1722b7038f34..02bcb89d1bbe 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) {
+			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);
+		}
+		if (cur_p->skb)
+			dev_kfree_skb(cur_p->skb);
+	}
+
 	dma_free_coherent(lp->dev,
 			  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
 			  lp->tx_bd_v,

base-commit: a7bfaba4823e3c165bb2004c74eff7c096672bc7
-- 
2.47.0

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

[Jacob Keller]

On 9/27/2026 1:10 AM, Sagi Maimon wrote:
> axienet_dma_bd_release() walks the RX ring to unmap and free every
> receive buffer before releasing it, but frees the TX descriptor ring
> with dma_free_coherent() alone.  Any descriptor that
> axienet_free_tx_chain() had not yet reclaimed still holds its skb and
> its streaming DMA mapping, and both are lost.
> 
> axienet_stop() disables TX NAPI and stops the DMA engine before calling
> it, so nothing reclaims those descriptors afterwards.  Bringing the
> interface down while frames are in flight therefore leaks up to
> lp->tx_bd_num skbs and mappings each time.
> 
> Walk the TX ring the way axienet_dma_err_handler() already does: unmap
> every descriptor whose cntrl is still set - axienet_free_tx_chain()
> clears it on reclaim - and free any skb still attached.  The DMA engine
> has been stopped by then, so the hardware no longer references the
> buffers.  On the axienet_dma_bd_init() error path the TX ring has just
> been allocated zeroed, so the walk does nothing.
> 
> This was reported by the Sashiko AI review bot.
> 
> Tested on an AXI Ethernet MAC behind a PCIe endpoint: traffic passes,
> and after each of ten down/up cycles and five module reloads, all made
> with traffic running and each running axienet_dma_bd_release(), traffic
> resumes and nothing is logged.  The leak itself was not measured.
> 
> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
> Assisted-by: LLM sparse
> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
> ---
> 
> Notes:
>     Found by the Sashiko review of v2 of "net: axienet: bound TX completion
>     cleanup by the NAPI budget":
>     https://lore.kernel.org/netdev/20260917115657.20697-1-maimon.sagi@gmail.com/
>     It is independent of that patch and applies on its own.
> 

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  .../net/ethernet/xilinx/xilinx_axienet_main.c  | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 1722b7038f34..02bcb89d1bbe 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) {
> +			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);
> +		}
> +		if (cur_p->skb)
> +			dev_kfree_skb(cur_p->skb);
> +	}
> +
>  	dma_free_coherent(lp->dev,
>  			  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
>  			  lp->tx_bd_v,
> 
> base-commit: a7bfaba4823e3c165bb2004c74eff7c096672bc7

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

[Joe Damato]

On Sun, Sep 27, 2026 at 11:10:34AM +0300, Sagi Maimon wrote:

[...]
 
> Walk the TX ring the way axienet_dma_err_handler() already does: unmap
> every descriptor whose cntrl is still set - axienet_free_tx_chain()
> clears it on reclaim - and free any skb still attached.

The code added in this patch and in axienet_dma_err_handler are almost
identical (as your commit message suggests). I am wondering if it's possible
to factor this code out into a helper and instead call it from both places
instead of repeating the same code?

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

[Sagi Maimon]

On Tue, Sep 29, 2026 at 3:12 AM Joe Damato <joe@dama.to> wrote:
>
> On Sun, Sep 27, 2026 at 11:10:34AM +0300, Sagi Maimon wrote:
>
> [...]
>
> > Walk the TX ring the way axienet_dma_err_handler() already does: unmap
> > every descriptor whose cntrl is still set - axienet_free_tx_chain()
> > clears it on reclaim - and free any skb still attached.
>
> The code added in this patch and in axienet_dma_err_handler are almost
> identical (as your commit message suggests). I am wondering if it's possible
> to factor this code out into a helper and instead call it from both places
> instead of repeating the same code?

Yes, the unmap-and-free part is the same; the error handler also clears
each descriptor for reuse, and frees with dev_kfree_skb_irq() where this
uses dev_kfree_skb(), so a shared helper would use dev_kfree_skb_any().
Since this goes to net with a Fixes: tag I kept it to the minimal
change.  I will send the helper to net-next once this fix has reached
it, if that works for you.
Thanks,
Sagi

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

[netdev-bot+sashiko]

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