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

[netdev-bot+sashiko]

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] A live MTU change can free the old context while a NAPI poll on
  another CPU is still reading it.

--

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

This patch changes .ndo_change_mtu() so that it works on a running
interface. It allocates a new context, quiesces and reconfigures the
hardware, then frees the old context. Before this patch the call returned
-EBUSY when the interface was up. AT91 EMAC still returns -EBUSY.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 93049b399d783..4b03f8a513722 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3624,11 +3624,29 @@ static int macb_close(struct net_device *netdev)
>  
>  static int macb_change_mtu(struct net_device *netdev, int new_mtu)
>  {
> -	if (netif_running(netdev))
> -		return -EBUSY;

[ ... ]

> +		new_ctx = macb_context_alloc(bp, new_mtu,
> +					     bp->configured_rx_ring_size,
> +					     bp->configured_tx_ring_size);
> +		if (IS_ERR(new_ctx))
> +			return PTR_ERR(new_ctx);
> +
> +		macb_context_swap_start(bp);
> +	}
>  
>  	WRITE_ONCE(netdev->mtu, new_mtu);
>  
> +	if (running)
> +		macb_context_swap_end(bp, new_ctx);

[Severity: High]
Can a live MTU change free the old context while a NAPI poll on another
CPU is still reading it?

Both poll functions keep using context memory after
napi_complete_done(). 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)) {

macb_rx_pending() then reads bp->ctx and rxq->tail/prepared_head, and
dereferences a descriptor in the rx ring:

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

macb_tx_poll() does the same thing. After napi_complete_done() it calls
macb_tx_complete_pending(), which reads:

    if (macb_tx_desc(ctx, q, txq->tail)->ctrl & MACB_BIT(TX_USED))

napi_complete_done() clears NAPIF_STATE_SCHED. napi_disable() only waits
for the instance's ownership bits to clear, not for the poll callback to
return. Documentation/networking/napi.rst says drivers should not touch
data structures after napi_complete_done() for this reason.

One possible interleaving:

CPU A (softirq)
macb_rx_poll()
    napi_complete_done()   /* SCHED cleared */

CPU B
macb_change_mtu()
    macb_context_swap_start()
        macb_quiesce_start()
            synchronize_irq() ...
            napi_disable()   /* returns immediately */
    macb_context_swap_end()
        WRITE_ONCE(bp->ctx, new_ctx);
        ...
        macb_free(old_ctx);   /* dma_free_coherent() of rings */
        kfree(old_ctx);

CPU A
    macb_rx_pending()
        reads the freed ctx and descriptor ring

As far as I can see, nothing on the swap path closes this window. There
is no synchronize_net() or synchronize_rcu(), and no tx_ptr_lock or
bp->lock handshake with the poll tail.

On non-coherent ARM/arm64 SoCs, coherent DMA memory is usually remapped
and unmapped on free, so this read could fault in softirq context. The
window is short on !PREEMPT_RT. It can be much longer under PREEMPT_RT,
threaded NAPI, or vCPU preemption.

Before this patch, macb_change_mtu() never reached the swap path on a
running interface. Any CAP_NET_ADMIN "ip link set mtu" while traffic is
flowing now gets there.

The same window also seems to exist through .set_ringparam(), which an
earlier patch in this series ("net: macb: use context swapping in
.set_ringparam()") converted to context swapping. Would a grace period
such as synchronize_net() before freeing old_ctx in
macb_context_swap_end() fix both paths? So would making the poll tails
avoid touching the context after napi_complete_done().

> +
>  	return 0;
>  }

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