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

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

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().

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.

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?

[ ... ]

> @@ -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.

[ ... ]

> @@ -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.

[ ... ]

> @@ -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?
-- 
This is an AI-generated review.