[PATCH net v4] net: axienet: free outstanding TX buffers in axienet_dma_bd_release()
Sagi Maimon
maimon.sagi at gmail.com
Sat Oct 10 22:00:30 PDT 2026
On Thu, Oct 8, 2026 at 6:47 PM <netdev-bot+sashiko at kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 (3) · Medium: 1 (3) · 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] net: axienet: RX buffer leak in axienet_dma_bd_release
Real, but not introduced here: the RX walk is unchanged by this patch.
It stops at the first descriptor with a NULL skb, so after a failed
refill in axienet_rx_poll() the buffers behind that slot leak, and it
frees each skb before unmapping it. The commit message overstates it
when it says the RX walk frees every receive buffer. I will send a
separate fix for the RX walk once I can test it on hardware.
> - [Medium] axienet_free_tx_bufs() loops over the current lp->tx_bd_num
> entries of lp->tx_bd_v, reading cntrl/skb and storing zeros into every…
This needs axienet_open() to ignore a failed reset after the ring was
resized, a path on which dma_free_coherent() already uses the wrong
size. The fix is for axienet_open() to check the reset result and
unwind, which is a separate, pre-existing problem.
The remaining pre-existing items (the unchecked reset in
axienet_open(), the error handler not stopping the TX queue, the open
error paths, ll_temac) are outside this patch. For the error handler,
the loop is moved unchanged; dev_kfree_skb_any() in its process context
frees at once instead of at the next softirq on the same CPU, which
neither opens nor closes that race.
> - [Low] axienet_free_tx_bufs() walks the TX ring in index order
> 0..tx_bd_num-1 and, at each descriptor that has an skb attached, frees…
> - [Low] axienet_dma_stop() ignores both the timeouts of its HALT polls
> and the return value of __axienet_device_reset(), which is -ETIMEDOUT…
>
> Pre-existing issues:
> - [High] axienet_open() ignores the result of axienet_device_reset().
> - [High] In the RX walk of axienet_dma_bd_release(), each RX skb is freed
> with dev_kfree_skb() (line 254) before its DMA_FROM_DEVICE streaming…
> - [High] axienet_dma_err_handler() runs from a workqueue and does not
> stop or lock the TX queue: there is no netif_tx_disable(),…
> - [Medium] In the sibling Xilinx driver ll_temac, temac_dma_bd_release()
> unmaps and frees the RX skbs but frees the TX descriptor ring with…
> - [Medium] The RX walk in axienet_dma_bd_release() stops (`break`) at the
> first descriptor with a NULL skb, on the assumption in its comment…
> - [Medium] If phylink_of_phy_connect() or axienet_init_legacy_dma() fails
> after axienet_device_reset() has succeeded, axienet_open() returns…
>
> --
>
> net: axienet: free outstanding TX buffers in axienet_dma_bd_release()
>
> This patch moves the TX ring walk out of axienet_dma_err_handler() into a
> new helper, axienet_free_tx_bufs(). axienet_dma_bd_release() now calls it,
> so TX skbs still in flight and their DMA mappings are freed when the
> interface goes down. The patch also clears tx_bd_v and rx_bd_v once their
> rings are freed, skips the TX walk when tx_bd_v is NULL, and frees skbs
> with dev_kfree_skb_any() instead of dev_kfree_skb_irq().
>
> > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > index 09443623a3e2a..318ff03b04e69 100644
> > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > @@ -173,6 +173,46 @@ static dma_addr_t desc_get_phys_addr(struct axienet_local *lp,
>
> [ ... ]
>
> > +static void axienet_free_tx_bufs(struct axienet_local *lp)
> > +{
> > + struct axidma_bd *cur_p;
> > + u32 i;
> > +
> > + for (i = 0; i < lp->tx_bd_num; i++) {
> > + cur_p = &lp->tx_bd_v[i];
>
> [Severity: Medium]
> Can this loop run past the end of the allocated ring? The loop bound is
> the current lp->tx_bd_num. The only guard in axienet_dma_bd_release() is
> the NULL check on lp->tx_bd_v, and that says nothing about how many
> entries the ring was allocated with.
>
> This sequence seems to reach that state:
>
> axienet_open()
> axienet_device_reset() succeeds, axienet_dma_bd_init() allocates N entries
> phylink_of_phy_connect() or axienet_init_legacy_dma() fails
> returns without axienet_dma_bd_release(), so tx_bd_v stays non-NULL
>
> axienet_ethtools_set_ringparam()
> the interface is down, so lp->tx_bd_num = M with M > N is accepted
>
> axienet_open()
> __axienet_device_reset() times out and axienet_dma_bd_init() is skipped
> the failure is ignored and open returns 0
>
> axienet_stop()->axienet_dma_bd_release()->axienet_free_tx_bufs()
> walks M entries of an N-entry ring
>
> For i >= N, the loop reads cntrl and skb from beyond the coherent buffer.
> It may call dma_unmap_single() or dev_kfree_skb_any() on garbage values,
> and it writes zeros past the end of the allocation.
>
> On this path the following dma_free_coherent() already passes the wrong
> size, so the path itself isn't new. The walk adds out-of-bounds writes to
> it. Fixing the unchecked reset and the missing unwind in axienet_open()
> would cover this too.
>
> > + 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);
> > + }
> > + /* not reclaimed by axienet_free_tx_chain(), so a drop */
> > + if (cur_p->skb)
> > + dev_kfree_skb_any(cur_p->skb);
>
> [Severity: Low]
> Is the skb freed here before all of its mappings have been unmapped?
>
> axienet_start_xmit() puts the head at orig_tail_ptr and the fragments at
> the following indices, wrapping to 0. Only the last descriptor gets
> cur_p->skb:
>
> for (ii = 0; ii < num_frag; ii++) {
> if (++new_tail_ptr >= lp->tx_bd_num)
> new_tail_ptr = 0;
> ...
> cur_p->skb = skb;
>
> Take an in-flight packet whose head is at tx_bd_num - 1 and whose last
> fragment wrapped to index 0. This loop frees the skb at index 0. It only
> calls dma_unmap_single() on the head and fragment descriptors at the top
> of the ring later in the walk.
>
> With DMA_TO_DEVICE and the engine stopped there may be no visible effect.
> Would unmapping every descriptor first and then freeing the skbs, or
> walking from tx_bd_ci in chain order, keep the usual unmap-before-free
> order? The error handler loop had the same order, and this patch adds it
> to normal teardown.
>
> > + cur_p->phys = 0;
>
> [ ... ]
>
> > @@ -186,11 +226,18 @@ 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, and is cleared below once the ring is freed;
> > + * dma_free_coherent() accepts NULL.
> > + */
> > + if (lp->tx_bd_v)
> > + axienet_free_tx_bufs(lp);
>
> [Severity: Low]
> This code assumes the DMA engine has stopped. axienet_dma_stop() ignores
> the HALT poll timeouts and also the return value of the reset:
>
> /* Do a reset to ensure DMA is really stopped */
> axienet_lock_mii(lp);
> __axienet_device_reset(lp);
> axienet_unlock_mii(lp);
>
> Suppose __axienet_device_reset() returns -ETIMEDOUT ("DMA reset timeout!")
> while an AXI read is still outstanding. Could the device then read TX
> buffers after they were unmapped and freed here, for example causing an
> IOMMU fault?
>
> axienet_stop() clears TXEN before this point, so freed data shouldn't
> reach the wire, and teardown has to proceed anyway. This is more a
> hardening question about faulty hardware.
>
> > +
> > dma_free_coherent(lp->dev,
> > sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
> > lp->tx_bd_v,
> > lp->tx_bd_p);
> > + lp->tx_bd_v = NULL;
>
> [Severity: High]
> This isn't a bug introduced by this patch. The commit message notes that
> axienet_open() does not check the reset result, and the transmit path
> still uses the ring when the reset fails:
>
> axienet_open() {
> ...
> ret = axienet_device_reset(ndev);
> axienet_unlock_mii(lp);
>
> ret = phylink_of_phy_connect(lp->phylink, lp->dev->of_node, 0);
> ...
> }
>
> The reset can fail in two ways: __axienet_device_reset() times out, or
> axienet_dma_bd_init() fails with -ENOMEM and calls
> axienet_dma_bd_release(). Either way, open still runs
> axienet_init_legacy_dma() and returns 0 with no TX or RX ring.
>
> The first transmit then does this:
>
> axienet_start_xmit()
> cur_p = &lp->tx_bd_v[orig_tail_ptr];
> ...
> axienet_check_tx_bd_space()
> cur_p = &lp->tx_bd_v[(READ_ONCE(lp->tx_bd_tail) + num_frag) %
> lp->tx_bd_num];
> if (cur_p->cntrl)
>
> Now that this patch clears tx_bd_v, that is a NULL pointer dereference.
> Before the patch, after an earlier close, it was a write into the freed
> coherent ring.
>
> axienet_dma_err_handler() also calls axienet_free_tx_bufs() and walks
> rx_bd_v, without the NULL check that axienet_dma_bd_release() now has.
>
> Should axienet_open() check the result of axienet_device_reset() and
> unwind on failure?
>
> [Severity: Medium]
> This is a pre-existing issue. The axienet_open() error exits that come
> after a successful reset never release the rings or stop the DMA engine:
>
> ret = phylink_of_phy_connect(lp->phylink, lp->dev->of_node, 0);
> if (ret) {
> dev_err(lp->dev, "phylink_of_phy_connect() failed: %d\n", ret);
> return ret;
> }
>
> The err_phy path, taken when axienet_init_legacy_dma() fails, does the
> same.
>
> By that point axienet_dma_bd_init() has allocated both coherent rings,
> allocated and mapped rx_bd_num RX skbs, and started the DMA engine. Do
> these leak, with the DMA engine still running?
>
> The next open's axienet_dma_bd_init() then overwrites tx_bd_v and rx_bd_v
> without freeing the old allocations.
>
> >
> > if (!lp->rx_bd_v)
> > return;
>
> [Severity: High]
> This isn't a bug introduced by this patch, but the RX walk just below
> leaks in the same way the TX side did. It stops at the first
> descriptor with a NULL skb:
>
> if (!lp->rx_bd_v[i].skb)
> break;
>
> That only works if the populated descriptors form a prefix of the
> ring. That holds on the axienet_dma_bd_init() error path, but not once
> the ring has been running.
>
> axienet_rx_poll() takes the skb from a completed descriptor and sets
> cur_p->skb to NULL before refilling it. If napi_alloc_skb() or
> dma_map_single() then fails, the poll loop breaks. That leaves a NULL
> slot at rx_bd_ci in the middle of an otherwise populated ring.
>
> If the interface is closed before a later poll refills that slot,
> axienet_stop()->axienet_dma_bd_release() stops there. The skb and
> DMA_FROM_DEVICE mapping of every descriptor after it are then leaked.
> Depending on where rx_bd_ci sits, that can be most of the ring. A
> system under memory pressure is the one most likely to hit this.
>
> It also contradicts the commit message, which says this walk frees
> every receive buffer.
>
> Would continue instead of break be enough? A slot emptied by
> axienet_rx_poll() still has a non-zero cntrl, and its phys points at
> the buffer that was already unmapped. So the skip has to cover the
> unmap as well, which a continue at that point does. The zeroed,
> never-filled slots on the axienet_dma_bd_init() error path would be
> skipped the same way.
>
> This patch is about making axienet_dma_bd_release() release everything
> the rings still hold. Could the RX side be fixed here as well, or in a
> separate patch?
>
> [Severity: High]
> This isn't a bug introduced by this patch. The RX walk below frees each
> skb before it unmaps the skb's DMA_FROM_DEVICE mapping:
>
> dev_kfree_skb(lp->rx_bd_v[i].skb);
> ...
> if (lp->rx_bd_v[i].cntrl) {
> phys = desc_get_phys_addr(lp, &lp->rx_bd_v[i]);
> dma_unmap_single(lp->dev, phys,
> lp->max_frm_size, DMA_FROM_DEVICE);
> }
>
> With swiotlb bouncing, can the unmap copy up to max_frm_size bytes back
> into the skb data that was just freed? On non-coherent platforms the
> unmap also invalidates cache lines of memory that may already belong to a
> new owner.
>
> Bouncing looks realistic here. Probe uses a 32-bit DMA mask unless 64-bit
> DMA is detected, so a 32-bit AXI DMA on a host with RAM above 4 GiB would
> bounce, as could the PCIe card from the commit message.
>
> The new TX helper unmaps before it frees. Would swapping the order in
> this loop make the two walks consistent?
>
> [Severity: Medium]
> This is also pre-existing. The same RX walk stops at the first NULL skb:
>
> /* A NULL skb means this descriptor has not been initialised
> * at all.
> */
> if (!lp->rx_bd_v[i].skb)
> break;
>
> axienet_rx_poll() sets cur_p->skb = NULL and exits its loop if
> napi_alloc_skb() or dma_map_single() fails. That leaves a NULL slot at
> rx_bd_ci in the middle of a populated ring.
>
> Would a close after such a refill failure leak every RX skb and mapping
> after that slot? That would also contradict the commit message, which
> says the RX walk frees "every receive buffer".
>
> > @@ -221,6 +268,7 @@ static void axienet_dma_bd_release(struct net_device *ndev)
> > sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
> > lp->rx_bd_v,
> > lp->rx_bd_p);
> > + lp->rx_bd_v = NULL;
> > }
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the sibling ll_temac
> driver seems to have the same TX leak. temac_dma_bd_release() in
> drivers/net/ethernet/xilinx/ll_temac_main.c frees only the TX ring:
>
> if (lp->tx_bd_v)
> dma_free_coherent(ndev->dev.parent,
> sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
> lp->tx_bd_v, lp->tx_bd_p);
>
> temac_start_xmit() attaches the skb to the last TX BD via ptr_to_txbd(),
> and only temac_start_xmit_done() reclaims it.
>
> Does temac_stop()->temac_dma_bd_release() leak in-flight TX skbs and
> their DMA_TO_DEVICE mappings? It may be worth a separate fix.
>
> [ ... ]
>
> > @@ -2749,29 +2797,7 @@ static void axienet_dma_err_handler(struct work_struct *work)
> > axienet_dma_stop(lp);
> > netdev_reset_queue(ndev);
> >
> > - for (i = 0; i < lp->tx_bd_num; i++) {
>
> [ ... ]
>
> > - }
> > + axienet_free_tx_bufs(lp);
>
> [Severity: High]
> This is a pre-existing issue, since the loop was moved rather than newly
> written. What stops axienet_start_xmit() from running at the same time as
> this code?
>
> axienet_dma_err_handler() runs from a workqueue. It disables NAPI, the
> MAC TX/RX enables and the DMA engine, but it never calls
> netif_tx_disable(), netif_tx_lock() or netif_stop_queue(). The TX queue
> stays awake for the up to ~250 ms spent in axienet_dma_stop().
>
> Meanwhile, on another CPU:
>
> axienet_start_xmit()
> cur_p->skb = skb;
> ...
> WRITE_ONCE(lp->tx_bd_tail, new_tail_ptr);
> netdev_sent_queue(ndev, skb->len);
>
> If axienet_free_tx_bufs() frees that skb between the store and the read
> of skb->len, can start_xmit read a freed skb?
>
> The two paths also race on tx_bd_tail, which the handler later resets to
> 0, on BQL state (netdev_reset_queue() against netdev_sent_queue()), and
> on descriptor contents. The restarted DMA could therefore fetch zeroed
> descriptors.
>
> dev_kfree_skb_irq() deferred the free to NET_TX softirq. With
> dev_kfree_skb_any(), this process-context worker now frees the skb
> immediately, so the freed memory can be reused sooner.
>
> The helper's kernel-doc lists only "The DMA engine must be stopped" as a
> precondition. Should it also require the TX queue to be quiesced?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007034620.1360542-1-maimon.sagi%40gmail.com
More information about the linux-arm-kernel
mailing list