[PATCH net] net: macb: drop in-flight Tx SKBs on close

[PATCH net] net: macb: drop in-flight Tx SKBs on close

[Théo Lebrun]

The MACB driver has since forever leaked the outgoing SKBs that have not
yet been marked as completed. They live in queue->tx_skb which gets
freed without remorse nor checking.

macb_free_consistent() gets called in a few codepaths, but only
close will trigger the added expressions. In macb_open() and
macb_alloc_consistent() failure cases, tx_skb just got allocated
and is empty.

Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index a12aa21244e8..3ffd60b852f8 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2649,6 +2649,7 @@ static unsigned int macb_rx_ring_size_per_queue(struct macb *bp)
 static void macb_free_consistent(struct macb *bp)
 {
 	struct device *dev = &bp->pdev->dev;
+	unsigned int dropped, tail;
 	struct macb_queue *queue;
 	unsigned int q;
 	size_t size;
@@ -2668,6 +2669,14 @@ static void macb_free_consistent(struct macb *bp)
 	dma_free_coherent(dev, size, bp->queues[0].rx_ring, bp->queues[0].rx_ring_dma);
 
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
+		dropped = CIRC_CNT(queue->tx_head, queue->tx_tail,
+				   bp->tx_ring_size);
+		queue->stats.tx_dropped += dropped;
+		bp->dev->stats.tx_dropped += dropped;
+
+		for (tail = queue->tx_tail; tail != queue->tx_head; tail++)
+			macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0);
+
 		kfree(queue->tx_skb);
 		queue->tx_skb = NULL;
 		queue->tx_ring = NULL;

---
base-commit: 41517b5a932a35c6f59a997ab3b7fa7746c273bd
change-id: 20260423-macb-drop-tx-f9ce72720d05

Best regards,
--  
Théo Lebrun <theo.lebrun@bootlin.com>

Re: [PATCH net] net: macb: drop in-flight Tx SKBs on close

[Nicolai Buchwitz]

Hi Théo,

thanks for your patch.

On 24.4.2026 12:01, Théo Lebrun wrote:
> [...]

> 
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> +		dropped = CIRC_CNT(queue->tx_head, queue->tx_tail,
> +				   bp->tx_ring_size);
> +		queue->stats.tx_dropped += dropped;
> +		bp->dev->stats.tx_dropped += dropped;

AFAIUI CIRC_CNT counts descriptor slots, not packets. A fragmented
skb uses multiple slots so tx_dropped would be overcounted?

> +
> +		for (tail = queue->tx_tail; tail != queue->tx_head; tail++)
> +			macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0);

I might be missing something, but couldn't this crash on the
macb_alloc_consistent() -> out_err path after a previous close
with in-flight frames?

1. First close: the new loop runs and frees skbs, but tx_head and
    tx_tail are not reset. kfree(tx_skb) sets it to NULL.
2. Second open: macb_alloc_consistent() fails early (e.g. the tx
    dma_alloc_coherent on the first line) and jumps to out_err.
3. macb_free_consistent() runs again. CIRC_CNT is non-zero (stale
    from previous session). macb_tx_skb() dereferences queue->tx_skb
    which is NULL.

Or if the failure happens later, the loop would iterate over a
freshly kmalloc'd (uninitialized) tx_skb array and macb_tx_unmap()
would read garbage mapping/skb pointers.

Maybe reset tx_head = tx_tail = 0 after the loop, or guard with
if (queue->tx_skb)?

> +
>  		kfree(queue->tx_skb);
>  		queue->tx_skb = NULL;
>  		queue->tx_ring = NULL;
> 

> [...]

Thanks,
Nicolai

Re: [PATCH net] net: macb: drop in-flight Tx SKBs on close

[Théo Lebrun]

Hello Nicolai,

On Fri Apr 24, 2026 at 12:30 PM CEST, Nicolai Buchwitz wrote:
> On 24.4.2026 12:01, Théo Lebrun wrote:
>> [...]
>
>> 
>>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>> +		dropped = CIRC_CNT(queue->tx_head, queue->tx_tail,
>> +				   bp->tx_ring_size);
>> +		queue->stats.tx_dropped += dropped;
>> +		bp->dev->stats.tx_dropped += dropped;
>
> AFAIUI CIRC_CNT counts descriptor slots, not packets. A fragmented
> skb uses multiple slots so tx_dropped would be overcounted?

Ah yes, agreed. dropped is the count of !!macb_tx_skb(queue, tail)->skb.

>> +		for (tail = queue->tx_tail; tail != queue->tx_head; tail++)
>> +			macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0);
>
> I might be missing something, but couldn't this crash on the
> macb_alloc_consistent() -> out_err path after a previous close
> with in-flight frames?
>
> 1. First close: the new loop runs and frees skbs, but tx_head and
>     tx_tail are not reset. kfree(tx_skb) sets it to NULL.
> 2. Second open: macb_alloc_consistent() fails early (e.g. the tx
>     dma_alloc_coherent on the first line) and jumps to out_err.
> 3. macb_free_consistent() runs again. CIRC_CNT is non-zero (stale
>     from previous session). macb_tx_skb() dereferences queue->tx_skb
>     which is NULL.
>
> Or if the failure happens later, the loop would iterate over a
> freshly kmalloc'd (uninitialized) tx_skb array and macb_tx_unmap()
> would read garbage mapping/skb pointers.
>
> Maybe reset tx_head = tx_tail = 0 after the loop, or guard with
> if (queue->tx_skb)?

Ah yes, guarding on queue->tx_skb sounds good for the error case of
macb_alloc_coherent(). I didn't notice we could land in free with
queue->tx_skb NULL. But it is also surprising to keep stall head/tail
cursors. I'll probably reset them as well for V2.

So along the lines of:

static void macb_free_consistent(struct macb *bp)
{
	// ... as before ...

	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
		if (queue->tx_skb) {
			for (tail = queue->tx_tail; tail != queue->tx_head; tail++) {
				if (macb_tx_skb(queue, tail)->skb)
					dropped++;
				macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0);
			}

			queue->stats.tx_dropped += dropped;
			bp->dev->stats.tx_dropped += dropped;

		}

		kfree(queue->tx_skb);
		queue->tx_skb = NULL;
		queue->tx_head = 0;
		queue->tx_tail = 0;
		queue->tx_ring = NULL;
		queue->rx_ring = NULL;
	}
}

Thanks for the swift review!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com