Re: [PATCH net-next v3 07/15] net: macb: introduce macb_context struct for buffer management

[Théo Lebrun]

Hello Paolo's LLM,

On Wed Jul 8, 2026 at 12:27 PM CEST, Paolo Abeni wrote:
> From: AI Reviewer <ai@xxxxxxxxxxx>
>
> 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/
> ---
> net: macb: introduce macb_context struct for buffer management
>
> This patch moves per-queue ring buffers and ring-size parameters out of
> struct macb / struct macb_queue into a new struct macb_context that is
> allocated on open and freed on close, so future patches can swap in a
> new context while the interface is running.
>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 951a7f080225..dfb1d49d841c 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>
> [ ... ]
>
>> @@ -1257,6 +1288,7 @@ static void macb_tx_error_task(struct work_struct *work)
>>  	struct macb_queue *queue = container_of(work, struct macb_queue,
>>  						tx_error_task);
>>  	unsigned int q = queue - queue->bp->queues;
>> +	struct macb_txq *txq = macb_txq(queue);
>>  	struct macb *bp = queue->bp;
>
> [High]
> Can macb_tx_error_task() dereference a freed context?
>
> macb_tx_error_task is scheduled from macb_interrupt():
>
>     schedule_work(&queue->tx_error_task);
>
> After this patch, the very first thing the worker does is:
>
>     struct macb_txq *txq = macb_txq(queue);
>
> which expands to &bp->ctx->txq[q] and dereferences bp->ctx.
>
> Looking at macb_close():
>
>     macb_reset_hw(bp);
>     ...
>     macb_free(bp);
>     kfree(bp->ctx);
>     bp->ctx = NULL;
>
> There is no cancel_work_sync(&queue->tx_error_task), and macb_reset_hw()
> does not drain the workqueue. If an interrupt scheduled the work just
> before close, or the work was already pending, the worker can run after
> kfree(bp->ctx) and hit a NULL dereference (and a use-after-free if
> macb_open() re-allocates ctx concurrently via ethtool ring reconfig).
>
> The same reasoning applies to bp->hresp_err_bh_work, which is queued
> from macb_interrupt() and whose handler calls
> mog_init_rings()/macb_init_buffers() that read bp->ctx->tx_ring_size and
> bp->ctx->{tx,rx}q[q]. cancel_work_sync(&bp->hresp_err_bh_work) is only
> called from macb_remove(), not macb_close().

All of this is a valid concern.

> Pre-patch these fields lived inside the embedded struct macb_queue so a
> late worker would touch stale-but-addressable memory; post-patch the
> first indirection through bp->ctx faults.

This pre-series analysis is wrong however. Current upstream would crash:
macb_tx_error_task() restarts the HW and it even assigns
queue->tx_ring_dma which still contains the freed DMA address
(macb_free()/macb_close() don't reset that value).

Summary: this patch turns potential memory corruption into a NULL
pointer dereference.

> Would adding cancel_work_sync(&queue->tx_error_task) per queue and
> cancel_work_sync(&bp->hresp_err_bh_work) before kfree(bp->ctx) in
> macb_close() close this window?

Yes that is the way forward. It will probably be separate and target
net/main. Not specifically because I want it backported (it'd be nice
but issue is super unlikely and no one reported it) but mostly because
this series is already 15 patches long.

>> @@ -3690,8 +3778,8 @@ static int macb_set_ringparam(struct net_device *netdev,
>>  		macb_close(bp->netdev);
>>  	}
>>  
>> -	bp->rx_ring_size = new_rx_size;
>> -	bp->tx_ring_size = new_tx_size;
>> +	bp->configured_rx_ring_size = new_rx_size;
>> +	bp->configured_tx_ring_size = new_tx_size;
>>  
>>  	if (reset)
>>  		macb_open(bp->netdev);
>
> [Medium]
> Should the return value of macb_open() be checked here?
>
> This patch adds a new failure point inside macb_open():
>
>     bp->ctx = kzalloc_obj(*bp->ctx);
>     if (!bp->ctx) {
>         err = -ENOMEM;
>         goto pm_exit;
>     }
>
> On failure, bp->ctx remains NULL and the interface stays down, but
> macb_set_ringparam() ignores the return value and reports success to
> userspace. The commit message says the motivation is to improve
> reliability under memory pressure, but this caller silently swallows the
> new allocation failure.

No need, this series ends with context swapping and the
macb_context_swap_end() operation, which replaces the macb_open()
highlighted here, cannot fail.

>> @@ -5039,6 +5125,12 @@ static int at91ether_open(struct net_device *netdev)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> +	bp->ctx = kzalloc_obj(*bp->ctx);
>> +	if (!bp->ctx) {
>> +		ret = -ENOMEM;
>> +		goto pm_exit;
>> +	}
>> +
>>  	/* Clear internal statistics */
>>  	ctl = macb_readl(bp, NCR);
>>  	macb_writel(bp, NCR, ctl | MACB_BIT(CLRSTAT));
>
> [Low]
> Should bp->ctx->rx_ring_size, tx_ring_size and rx_buffer_size be
> initialized on this path?
>
> macb_open() sets them from configured_{rx,tx}_ring_size and calls
> macb_init_rx_buffer_size(), but at91ether_open() leaves them at zero
> after kzalloc_obj().
>
> Helpers such as macb_rx_ring_wrap() compute:
>
>     return index & (bp->ctx->rx_ring_size - 1);
>
> which becomes index & 0xFFFFFFFF when rx_ring_size is 0. It happens to
> be harmless today because the at91ether path constrains indices to
> [0, AT91ETHER_MAX_RX_DESCR-1] independently and macb_get_regs() reports
> zeroed head/tail for the tx-less at91ether, but pre-patch macb_init_dflt()
> unconditionally set bp->rx_ring_size = DEFAULT_RX_RING_SIZE.
>
> Any future shared helper that used tx_ring_size as a stride or divisor
> would misbehave on at91ether.

Yes. Null impact (but it would be better in theory).

>> @@ -5076,6 +5171,9 @@ static int at91ether_close(struct net_device *netdev)
>>  
>>  	at91ether_stop(bp);
>>  
>> +	kfree(bp->ctx);
>> +	bp->ctx = NULL;
>> +
>>  	pm_runtime_put(&bp->pdev->dev);
>>  
>>  	return 0;
>
> [High]
> Is there IRQ synchronization missing before freeing bp->ctx on this
> path?
>
> at91ether_stop() disables MAC RX/TX and writes IDR to mask further
> interrupts, but does not synchronize with a handler already running or
> already dispatched on another CPU. at91ether_interrupt() calls
> at91ether_rx(), which after this patch does:
>
>     struct macb_rxq *rxq = &bp->ctx->rxq[0];
>     ...
>     desc = macb_rx_desc(queue, rxq->tail);
>     ...
>     p_recv = rxq->buffers + rxq->tail * AT91ETHER_MAX_RBUFF_SZ;
>
> If the handler is in flight when kfree(bp->ctx); bp->ctx = NULL runs,
> the next access through bp->ctx faults. The macb_open()/macb_close()
> path is somewhat protected by napi_disable() before macb_free(), but
> at91ether does not use NAPI and has no equivalent barrier.
>
> Would a synchronize_irq(bp->netdev->irq) between at91ether_stop() and
> kfree(bp->ctx) close this?

Ah at91ether_interrupt() doesn't grab bp->lock as macb_interrupt() does.
Indeed synchronize_irq() would solve this.

Thanks,

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