Re: [PATCH net-next v10 7/8] net: macb: use context swapping in .set_ringparam()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Use-after-free in the new live context swap.

--

net: macb: use context swapping in .set_ringparam()

This patch reworks macb_set_ringparam(). It now allocates a new context
before stopping the interface, swaps it in and then frees the old one,
instead of doing a full close/open. The generic swap logic is split into
macb_context_swap_start() and macb_context_swap_end(), and a new
mac_cfg_lock mutex serialises the swap against the phylink MAC callbacks.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 69873966a9890..93049b399d783 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -3217,6 +3255,112 @@ static void macb_configure_dma(struct macb *bp)
>  	}
>  }
>  
> +static void macb_context_swap_start(struct macb *bp)
> +{
> +	struct macb_queue *queue;
> +	unsigned long flags;
> +	unsigned int q;
> +	u32 ctrl;
> +
> +	mutex_lock(&bp->mac_cfg_lock);
> +
> +	macb_quiesce_start(bp);

[ ... ]

> +static void macb_context_swap_end(struct macb *bp,
> +				  struct macb_context *new_ctx)
> +{

[ ... ]

> +	old_ctx = bp->ctx;
> +	WRITE_ONCE(bp->ctx, new_ctx);
> +	wmb(); /* ensure IRQ enabled below read the new context */
> +	macb_init_buffers(bp);

[ ... ]

> +	mutex_unlock(&bp->mac_cfg_lock);
> +
> +	/* Free old context. */
> +
> +	macb_free(old_ctx);
> +	kfree(old_ctx);
> +}

[Severity: High]
Can a NAPI poll still be reading old_ctx when it gets freed here?

The swap depends on macb_quiesce_start() calling napi_disable() to drain
NAPI. However, napi_disable() only waits for NAPIF_STATE_SCHED and
NAPIF_STATE_NPSVC to clear, and napi_complete_done() clears SCHED before
the poll callback returns. Documentation/networking/napi.rst notes that
napi_disable() only waits for the ownership of the instance to be
released, not for the poll method to exit.

Both poll functions keep reading the context after napi_complete_done()
succeeds. In macb_rx_poll():

	if (work_done < budget && napi_complete_done(napi, work_done)) {
		queue_writel(queue, IER, bp->rx_intr_mask);
		...
		if (macb_rx_pending(queue)) {

and macb_rx_pending() does:

	struct macb_context *ctx = queue->bp->ctx;
	struct macb_rxq *rxq = macb_rxq(queue);
	...
	entry = macb_rx_ring_wrap(ctx, rxq->tail);
	desc = macb_rx_desc(ctx, q, entry);
	...
	return (desc->addr & MACB_BIT(RX_USED)) != 0;

macb_tx_poll() does the same through macb_tx_complete_pending(). That
function reads queue->bp->ctx and the TX ring under tx_ptr_lock. The swap
and free path takes neither tx_ptr_lock nor any other lock or RCU grace
period that these helpers respect.

This looks like a possible interleaving:

poll CPU
macb_rx_poll()
  napi_complete_done()   /* clears SCHED */
  macb_rx_pending()
    ctx = queue->bp->ctx;   /* old ctx */
    <delayed>

ethtool CPU
macb_set_ringparam()
  macb_context_swap_start()
    macb_quiesce_start()
      napi_disable()   /* returns, SCHED already clear */
  macb_context_swap_end()
    WRITE_ONCE(bp->ctx, new_ctx);
    ...
    macb_free(old_ctx);   /* rings NULLed and dma_free_coherent()ed */
    kfree(old_ctx);

poll CPU
    desc = macb_rx_desc(ctx, q, entry);   /* freed memory */

Here the poll CPU would dereference the kfree()d context, a NULL ring
pointer, or freed coherent DMA memory. The delay could come from threaded
NAPI, softirq preemption on PREEMPT_RT, interrupts or vCPU preemption.
This can be triggered with ethtool -G while traffic is flowing.

The new irq_quiesced check in macb_tx_restart() does not seem to cover
this. That helper runs before napi_complete_done() and is already
protected by NAPI ownership.

macb_close() uses the same napi_disable-then-free pattern. This patch adds
a new free-while-running site that is reachable during normal operation.
The later patch "net: macb: use context swapping in .ndo_change_mtu()"
reuses these helpers without adding any extra synchronization.

Would a synchronize_net() (or an equivalent grace period) before
macb_free(old_ctx) be enough to close this window? Alternatively, the
post-completion helpers could be made to check for an ongoing swap.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001-macb-context-v10-0-beb541bbb7df%40bootlin.com