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

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 10/28/26 09:48, Rosen Penev wrote:
> 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 every slot
> before freeing any skb, since a packet can wrap around the end of the
> ring with its skb on the last slot, 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>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

This version looks good to me, thanks!
-- 
Florian