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

[PATCH net v2] 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, as a drop since
it was never transmitted.  This relies on axienet_stop() having stopped
the DMA engine first, as the RX walk in the same function already does.

tx_bd_v is NULL when axienet_dma_bd_init() did not get as far as
allocating it: axienet_open() does not check the result of the reset
that runs it.  dma_free_coherent() accepts that, so skip the walk then
too, and drop the comment claiming the ring is always allocated.  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")
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Assisted-by: LLM sparse
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---

Notes:
    Changes in v2:
    - Skip the TX walk when tx_bd_v is NULL, which axienet_open() allows
      when the reset fails; v1 would have dereferenced it where the old
      dma_free_coherent() did not (Sashiko).  Drop the comment that claimed
      the ring is always allocated.
    - Free the skbs with dev_kfree_skb_any(), so they count as drops as in
      axienet_dma_err_handler(), rather than as consumed (Sashiko).
    - Say that the walk relies on axienet_stop() stopping the DMA engine
      first, rather than stating it as a guarantee (Sashiko).
    - Kept Jacob's Reviewed-by, as the changes are small; please say if
      that is not OK.
    - The hardware test is v1's.  The changes do not touch the path it
      exercised other than how the freed skbs are accounted.
    - v1: https://lore.kernel.org/netdev/20260927081034.350422-1-maimon.sagi@gmail.com/

 .../net/ethernet/xilinx/xilinx_axienet_main.c | 23 ++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 1722b7038f34..8bf27e20e103 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -186,7 +186,28 @@ static void axienet_dma_bd_release(struct net_device *ndev)
 	int i;
 	struct axienet_local *lp = netdev_priv(ndev);
 
-	/* If we end up here, tx_bd_v must have been DMA allocated. */
+	/* tx_bd_v is NULL if axienet_dma_bd_init() did not get as far as
+	 * allocating it; dma_free_coherent() below accepts that.
+	 */
+	for (i = 0; lp->tx_bd_v && 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);
+		}
+		/* never transmitted, so account it as a drop */
+		if (cur_p->skb)
+			dev_kfree_skb_any(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 v2] 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: 1 · Medium: 0 (1) · 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).

- [High] The new TX walk in axienet_dma_bd_release()
  (drivers/net/ethernet/xilinx/xilinx_axienet_main.c:192) only checks…
- [Low] The new walk in axienet_dma_bd_release() frees every leftover TX
  skb with dev_kfree_skb_any() (a drop with…
- [Low] The new TX walk relies on axienet_stop() having stopped the DMA
  engine, and the commit message says so.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the same
  kind of leak exists in dmaengine mode.

--

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

axienet_dma_bd_release() now walks the TX descriptor ring before freeing
it. For each descriptor whose cntrl is still set it unmaps the buffer, and
it frees any attached skb as a drop. The walk is skipped when tx_bd_v is
NULL.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 1722b7038f340..8bf27e20e103d 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -186,7 +186,28 @@ static void axienet_dma_bd_release(struct net_device *ndev)
>  	int i;
>  	struct axienet_local *lp = netdev_priv(ndev);
>
> -	/* If we end up here, tx_bd_v must have been DMA allocated. */
> +	/* tx_bd_v is NULL if axienet_dma_bd_init() did not get as far as
> +	 * allocating it; dma_free_coherent() below accepts that.
> +	 */
> +	for (i = 0; lp->tx_bd_v && i < lp->tx_bd_num; i++) {

[Severity: High]

Is lp->tx_bd_v ever set back to NULL? The only assignment seems to be the
dma_alloc_coherent() in axienet_dma_bd_init(). The dma_free_coherent()
below leaves the pointer in place, so the NULL check only holds on the
first open, while the private area is still zeroed.

Consider this sequence:

ifup (succeeds)
ifdown
  axienet_stop()
    axienet_dma_bd_release()
      dev_kfree_skb_any(cur_p->skb)      /* cur_p->skb not cleared */
      dma_free_coherent(..., lp->tx_bd_v, ...)  /* tx_bd_v left as is */
ifup
  axienet_open()
    axienet_device_reset()
      __axienet_device_reset() fails, returns before axienet_dma_bd_init()
    ret = phylink_of_phy_connect(...)    /* reset result overwritten */
ifdown
  axienet_stop()
    axienet_dma_bd_release()
      walks the already freed tx_bd_v ring

Can this call dev_kfree_skb_any() a second time on the skbs freed at the
previous close? It would also call dma_unmap_single() on addresses that
were already unmapped. If the coherent memory has been reused, arbitrary
values would go to kfree and the DMA unmap.

The out: label in axienet_dma_bd_init() looks like a second way in. If the
rx_bd_v allocation, netdev_alloc_skb_ip_align() or dma_map_single() fails,
the TX ring is freed there and tx_bd_v is not cleared. axienet_open() then
ignores the error, and the next axienet_stop() walks the freed ring.

Before this patch these paths already did a second dma_free_coherent() on
the stale pointer. With this patch, the freed descriptors are also read,
and the skb and DMA addresses stored in them are freed and unmapped.

The commit message says:

  tx_bd_v is NULL when axienet_dma_bd_init() did not get as far as
  allocating it: axienet_open() does not check the result of the reset
  that runs it.

Could lp->tx_bd_v (and lp->rx_bd_v) be set to NULL after each
dma_free_coherent()? Clearing cur_p->skb and cur_p->cntrl in the walk, or
having axienet_open() propagate the axienet_device_reset() error, would
also close this.

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

[Severity: Low]

This unmap assumes the MM2S engine has stopped. axienet_dma_stop() polls
the halt bit for at most 5 x 20 ms and does not act on a timeout. It also
discards the return value of __axienet_device_reset():

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

If the channel neither halts nor resets, could the engine still be reading
TX payloads after they are unmapped here?

The RX walk and the ring dma_free_coherent() in this function already
carry the same risk, and the commit message documents the dependency. The
new exposure is small and limited to faulty hardware.

> +		}
> +		/* never transmitted, so account it as a drop */
> +		if (cur_p->skb)
> +			dev_kfree_skb_any(cur_p->skb);

[Severity: Low]

Is "never transmitted" true for every descriptor that reaches this point?
axienet_stop() calls napi_disable(&lp->napi_tx) before phylink_stop(), the
TXEN clear and axienet_dma_stop(). The MM2S engine can keep completing
frames in that window, and nothing reclaims them.

axienet_free_tx_chain() treats descriptors with the COMPLETE bit set as
transmitted:

  if (cur_p->skb && (status & XAXIDMA_BD_STS_COMPLETE_MASK)) {
          ...
          napi_consume_skb(cur_p->skb, budget);
          packets++;
  }

Should this check cur_p->status for XAXIDMA_BD_STS_COMPLETE_MASK and use
dev_consume_skb_any() for those skbs? As written, frames that were
actually sent are reported as drops and are left out of tx_packets and
tx_bytes.

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

[Severity: Medium]

This isn't a bug introduced by this patch, but the dmaengine branch of
axienet_stop() seems to leak in the same way. When lp->use_dmaengine is
set, axienet_dma_bd_release() is never called. The branch does this:

  dmaengine_terminate_sync(lp->tx_chan);
  ...
  for (i = 0; i < TX_BD_NUM_MAX; i++)
          kfree(lp->tx_skb_ring[i]);

In-flight TX entries still hold an skb and the dma_map_sg() mapping set up
in axienet_start_xmit_dmaengine(). Only axienet_dma_tx_cb() releases
those.

xilinx_dma_terminate_all()->xilinx_dma_free_descriptors() frees the
descriptor lists without invoking client callbacks, so that callback never
runs for terminated transfers.

Does this leak the TX skbs and their scatterlist mappings on every ifdown
with traffic in flight? The RX buffers submitted by
axienet_rx_submit_desc() look like they are lost the same way.

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