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