[PATCH net v3] net: bgmac: fix TX DMA unmapping and slot cleanup

[PATCH net v3] net: bgmac: fix TX DMA unmapping and slot cleanup

[Rosen Penev]

When mapping a fragment fails, bgmac_dma_tx_add() unwinds with the
wrong slots. It passes the failed fragment's slot->dma_addr, which
holds the mapping error, to dma_unmap_single() for the head. The
fragment loop indexes from ring->end instead of ring->end + 1, so it
unmaps the head a second time as a page and leaves the last mapped
fragment mapped. None of the affected slots get their dma_addr cleared
either, which includes the head slot when mapping the head fails.

bgmac_dma_tx_ring_free() unmaps every slot with a non-zero dma_addr,
so tearing down the ring unmaps these addresses again. It also frees
the skbs of pending TX slots but leaves slot->skb and slot->dma_addr
set. The slots survive a down/up cycle, and bgmac_dma_tx_add() only
sets slot->skb on the last slot of a packet. If a slot with a stale
pointer is later used for the head or a fragment of a scatter/gather
packet, bgmac_dma_tx_free() frees the old skb a second time when that
slot completes.

bgmac_dma_tx_ring_free() also picks dma_unmap_single() or
dma_unmap_page() by looking at slot->skb, which is set on the last slot
of a packet, while the head was mapped with dma_map_single() and the
fragments with skb_frag_dma_map().

In the error path, unmap the head and each mapped fragment from their
own slots, and clear dma_addr on every slot the failed packet touched.
In bgmac_dma_tx_ring_free(), choose the unmap function by the SOF flag
of the descriptor like bgmac_dma_tx_free() does, unmap before freeing,
and clear both fields.

This was found through code inspection by an LLM while hitting other
bugs on an Asus RT-N16. It did not hit these ones specifically.

Fixes: 9cde94506eac ("bgmac: implement scatter/gather support")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v3: clarify how this was found
 v2: merge two patches into one
 drivers/net/ethernet/broadcom/bgmac.c | 41 ++++++++++++++++-----------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index e66856337d33..55d126c7d35c 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -205,19 +205,24 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
 	return NETDEV_TX_OK;
 
 err_dma:
-	dma_unmap_single(dma_dev, slot->dma_addr, skb_headlen(skb),
-			 DMA_TO_DEVICE);
+	slot->dma_addr = 0;
 
 	while (i-- > 0) {
-		int index = (ring->end + i) % BGMAC_TX_RING_SLOTS;
-		struct bgmac_slot_info *slot = &ring->slots[index];
-		u32 ctl1 = le32_to_cpu(ring->cpu_base[index].ctl1);
-		int len = ctl1 & BGMAC_DESC_CTL1_LEN;
-
-		dma_unmap_page(dma_dev, slot->dma_addr, len, DMA_TO_DEVICE);
+		index = (ring->end + 1 + i) % BGMAC_TX_RING_SLOTS;
+		slot = &ring->slots[index];
+		dma_unmap_page(dma_dev, slot->dma_addr,
+			       skb_frag_size(&skb_shinfo(skb)->frags[i]),
+			       DMA_TO_DEVICE);
+		slot->dma_addr = 0;
 	}
 
+	slot = &ring->slots[ring->end % BGMAC_TX_RING_SLOTS];
+	dma_unmap_single(dma_dev, slot->dma_addr, skb_headlen(skb),
+			 DMA_TO_DEVICE);
+
 err_dma_head:
+	slot->dma_addr = 0;
+
 	netdev_err(bgmac->net_dev, "Mapping error of skb on ring 0x%X\n",
 		   ring->mmio_base);
 
@@ -535,17 +540,19 @@ static void bgmac_dma_tx_ring_free(struct bgmac *bgmac,
 		unsigned int len = ctl1 & BGMAC_DESC_CTL1_LEN;
 
 		slot = &ring->slots[i];
-		dev_kfree_skb(slot->skb);
 
-		if (!slot->dma_addr)
-			continue;
+		if (slot->dma_addr) {
+			if (le32_to_cpu(dma_desc[i].ctl0) & BGMAC_DESC_CTL0_SOF)
+				dma_unmap_single(dma_dev, slot->dma_addr,
+						 len, DMA_TO_DEVICE);
+			else
+				dma_unmap_page(dma_dev, slot->dma_addr,
+					       len, DMA_TO_DEVICE);
+			slot->dma_addr = 0;
+		}
 
-		if (slot->skb)
-			dma_unmap_single(dma_dev, slot->dma_addr,
-					 len, DMA_TO_DEVICE);
-		else
-			dma_unmap_page(dma_dev, slot->dma_addr,
-				       len, DMA_TO_DEVICE);
+		dev_kfree_skb(slot->skb);
+		slot->skb = NULL;
 	}
 }
 
-- 
2.56.0

Re: [PATCH net v3] net: bgmac: fix TX DMA unmapping and slot cleanup

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022215235.3692083-1-rosenp@gmail.com?part=1
---
net: bgmac: fix TX DMA unmapping and slot cleanup

This patch fixes the bgmac_dma_tx_add() error path so that the head and
each mapped fragment are unmapped from their own slots. It also clears
dma_addr on every slot the failed packet touched. In
bgmac_dma_tx_ring_free() it picks the unmap function from the SOF flag
of the descriptor, unmaps before freeing, and clears slot->skb and
slot->dma_addr.

> This was found through code inspection by an LLM while hitting other
> bugs on an Asus RT-N16. It did not hit these ones specifically.
>
> Fixes: 9cde94506eac ("bgmac: implement scatter/gather support")
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

[Severity: Low]
This isn't a bug, but the commit message says an LLM found these
problems. Should the trailers include an Assisted-by: tag?

Documentation/process/submitting-patches.rst says:

  If you used any sort of advanced coding tool in the creation of your
  patch, you need to acknowledge that use by adding an Assisted-by tag.

Documentation/process/coding-assistants.rst gives the format as
"Assisted-by: LLM [TOOL1] [TOOL2]". Step 6 of its procedure for finding
and fixing bugs also asks for an Assisted-by tag.

> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 3e9c57196a397..d44ee8ed32459 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c

[ ... ]

> @@ -535,17 +540,19 @@ static void bgmac_dma_tx_ring_free(struct bgmac *bgmac,
>  		unsigned int len = ctl1 & BGMAC_DESC_CTL1_LEN;
>  
>  		slot = &ring->slots[i];
> -		dev_kfree_skb(slot->skb);
>  
> -		if (!slot->dma_addr)
> -			continue;
> +		if (slot->dma_addr) {
> +			if (le32_to_cpu(dma_desc[i].ctl0) & BGMAC_DESC_CTL0_SOF)
> +				dma_unmap_single(dma_dev, slot->dma_addr,
> +						 len, DMA_TO_DEVICE);
> +			else
> +				dma_unmap_page(dma_dev, slot->dma_addr,
> +					       len, DMA_TO_DEVICE);
> +			slot->dma_addr = 0;
> +		}
>  
> -		if (slot->skb)
> -			dma_unmap_single(dma_dev, slot->dma_addr,
> -					 len, DMA_TO_DEVICE);
> -		else
> -			dma_unmap_page(dma_dev, slot->dma_addr,
> -				       len, DMA_TO_DEVICE);
> +		dev_kfree_skb(slot->skb);
> +		slot->skb = NULL;

[Severity: Low]
If a pending packet wraps around the end of the ring, is the skb still
freed before all of its slots are unmapped?

The commit message says bgmac_dma_tx_ring_free() will "unmap before
freeing". That ordering only holds within a single slot, because the
loop walks slots in physical order from 0 to BGMAC_TX_RING_SLOTS - 1.

bgmac_dma_tx_add() puts fragments in consecutive slots modulo the ring
size, and sets slot->skb only on the last one:

drivers/net/ethernet/broadcom/bgmac.c:bgmac_dma_tx_add() {
    ...
		index = (index + 1) % BGMAC_TX_RING_SLOTS;
		slot = &ring->slots[index];
    ...
	slot->skb = skb;
    ...
}

Take a scatter/gather packet whose head (SOF) is in the last slot of the
ring and whose last fragment, which owns the skb, is in slot 0. The loop
reaches i == 0 first and calls dev_kfree_skb(), which releases skb->head
and the frag pages. Only later, at i == BGMAC_TX_RING_SLOTS - 1, does it
call dma_unmap_single() on the head buffer, which is already freed.

bgmac_stop() and the suspend path call bgmac_chip_reset() before
bgmac_dma_cleanup(), and the mappings are DMA_TO_DEVICE. So this may
have no visible effect, but it still breaks the DMA API rule that a
buffer is unmapped before it is freed.

bgmac_dma_tx_free() avoids this by walking from ring->start to
ring->end. Could the same be done here? Another option is to unmap every
slot in a first pass and free the skbs in a second pass.

>  	}
>  }

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. When
request_irq() fails in bgmac_open(), are the RX buffers freed while the
hardware is still running?

drivers/net/ethernet/broadcom/bgmac.c:bgmac_open() {
	bgmac_chip_reset(bgmac);

	err = bgmac_dma_init(bgmac);
	...
	bgmac_chip_init(bgmac);

	err = request_irq(bgmac->irq, bgmac_interrupt, IRQF_SHARED,
			  net_dev->name, net_dev);
	if (err < 0) {
		dev_err(bgmac->dev, "IRQ request error: %d!\n", err);
		bgmac_dma_cleanup(bgmac);
		return err;
	}
    ...
}

By the time request_irq() fails, bgmac_dma_init() has enabled the RX DMA
engine, attached a mapped page to every RX descriptor and written
RX_INDEX. bgmac_chip_init() has also unmasked device interrupts with
bgmac_chip_intrs_on() and enabled the MAC with bgmac_enable().

bgmac_dma_cleanup() then unmaps and frees every RX buffer through
bgmac_dma_rx_ring_free(), and nothing stops the engine first.

Could a frame received before the next bgmac_chip_reset() be DMA'd into
pages that are already freed? The device interrupt also stays unmasked
on an IRQF_SHARED line with no bgmac handler registered.

bgmac_stop() quiesces the hardware before cleanup:

drivers/net/ethernet/broadcom/bgmac.c:bgmac_stop() {
    ...
	bgmac_chip_intrs_off(bgmac);
	free_irq(bgmac->irq, net_dev);

	bgmac_chip_reset(bgmac);
	bgmac_dma_cleanup(bgmac);
    ...
}

Should the bgmac_open() error path also call bgmac_chip_intrs_off() and
bgmac_chip_reset() before bgmac_dma_cleanup()?

Re: [PATCH net v3] net: bgmac: fix TX DMA unmapping and slot cleanup

[Simon Horman]

On Tue, Oct 27, 2026 at 05:22:41PM +0000, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022215235.3692083-1-rosenp@gmail.com?part=1
> ---
> net: bgmac: fix TX DMA unmapping and slot cleanup
> 
> This patch fixes the bgmac_dma_tx_add() error path so that the head and
> each mapped fragment are unmapped from their own slots. It also clears
> dma_addr on every slot the failed packet touched. In
> bgmac_dma_tx_ring_free() it picks the unmap function from the SOF flag
> of the descriptor, unmaps before freeing, and clears slot->skb and
> slot->dma_addr.
> 
> > This was found through code inspection by an LLM while hitting other
> > bugs on an Asus RT-N16. It did not hit these ones specifically.
> >
> > Fixes: 9cde94506eac ("bgmac: implement scatter/gather support")
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> 
> [Severity: Low]
> This isn't a bug, but the commit message says an LLM found these
> problems. Should the trailers include an Assisted-by: tag?
> 
> Documentation/process/submitting-patches.rst says:
> 
>   If you used any sort of advanced coding tool in the creation of your
>   patch, you need to acknowledge that use by adding an Assisted-by tag.
> 
> Documentation/process/coding-assistants.rst gives the format as
> "Assisted-by: LLM [TOOL1] [TOOL2]". Step 6 of its procedure for finding
> and fixing bugs also asks for an Assisted-by tag.
> 
> > diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> > index 3e9c57196a397..d44ee8ed32459 100644
> > --- a/drivers/net/ethernet/broadcom/bgmac.c
> > +++ b/drivers/net/ethernet/broadcom/bgmac.c
> 
> [ ... ]
> 
> > @@ -535,17 +540,19 @@ static void bgmac_dma_tx_ring_free(struct bgmac *bgmac,
> >  		unsigned int len = ctl1 & BGMAC_DESC_CTL1_LEN;
> >  
> >  		slot = &ring->slots[i];
> > -		dev_kfree_skb(slot->skb);
> >  
> > -		if (!slot->dma_addr)
> > -			continue;
> > +		if (slot->dma_addr) {
> > +			if (le32_to_cpu(dma_desc[i].ctl0) & BGMAC_DESC_CTL0_SOF)
> > +				dma_unmap_single(dma_dev, slot->dma_addr,
> > +						 len, DMA_TO_DEVICE);
> > +			else
> > +				dma_unmap_page(dma_dev, slot->dma_addr,
> > +					       len, DMA_TO_DEVICE);
> > +			slot->dma_addr = 0;
> > +		}
> >  
> > -		if (slot->skb)
> > -			dma_unmap_single(dma_dev, slot->dma_addr,
> > -					 len, DMA_TO_DEVICE);
> > -		else
> > -			dma_unmap_page(dma_dev, slot->dma_addr,
> > -				       len, DMA_TO_DEVICE);
> > +		dev_kfree_skb(slot->skb);
> > +		slot->skb = NULL;
> 
> [Severity: Low]
> If a pending packet wraps around the end of the ring, is the skb still
> freed before all of its slots are unmapped?
> 
> The commit message says bgmac_dma_tx_ring_free() will "unmap before
> freeing". That ordering only holds within a single slot, because the
> loop walks slots in physical order from 0 to BGMAC_TX_RING_SLOTS - 1.
> 
> bgmac_dma_tx_add() puts fragments in consecutive slots modulo the ring
> size, and sets slot->skb only on the last one:
> 
> drivers/net/ethernet/broadcom/bgmac.c:bgmac_dma_tx_add() {
>     ...
> 		index = (index + 1) % BGMAC_TX_RING_SLOTS;
> 		slot = &ring->slots[index];
>     ...
> 	slot->skb = skb;
>     ...
> }
> 
> Take a scatter/gather packet whose head (SOF) is in the last slot of the
> ring and whose last fragment, which owns the skb, is in slot 0. The loop
> reaches i == 0 first and calls dev_kfree_skb(), which releases skb->head
> and the frag pages. Only later, at i == BGMAC_TX_RING_SLOTS - 1, does it
> call dma_unmap_single() on the head buffer, which is already freed.
> 
> bgmac_stop() and the suspend path call bgmac_chip_reset() before
> bgmac_dma_cleanup(), and the mappings are DMA_TO_DEVICE. So this may
> have no visible effect, but it still breaks the DMA API rule that a
> buffer is unmapped before it is freed.
> 
> bgmac_dma_tx_free() avoids this by walking from ring->start to
> ring->end. Could the same be done here? Another option is to unmap every
> slot in a first pass and free the skbs in a second pass.

Hi Rosen,

The issue flagged above does seem somewhat concerning.
What is your take on it?

...