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

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello netdev-bot+sashiko,

On Mon Oct 5, 2026 at 1:33 AM CEST,  wrote:
> 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.

Technically this is possible. To get it in practice, you would have to
 - have no phylink attached
 - be running a PREEMPT_RT kernel would help a lot
 - be unlucky

I'm fixing it with the recommended synchronize_net(), after having
checked that we are not the first ones. A few commits do justify the
same bug for using synchronize_net().

Looked through the results from

   ⟩ git log -S synchronize_net net-next/main drivers/net/ethernet/

For example, here is the simplest precedent I could find:

commit eb41a264a3a576dc040ee37c3d9d6b7e2d9be968
Author: Carolina Jubran <cjubran@nvidia.com>
Date:   Thu Jul 10 16:53:43 2025 +0300

    net/mlx5e: Fix race between DIM disable and net_dim()

    There's a race between disabling DIM and NAPI callbacks using the dim
    pointer on the RQ or SQ.

    If NAPI checks the DIM state bit and sees it still set, it assumes
    `rq->dim` or `sq->dim` is valid. But if DIM gets disabled right after
    that check, the pointer might already be set to NULL, leading to a NULL
    pointer dereference in net_dim().

    Fix this by calling `synchronize_net()` before freeing the DIM context.
    This ensures all in-progress NAPI callbacks are finished before the
    pointer is cleared.

    Kernel log:

    BUG: kernel NULL pointer dereference, address: 0000000000000000
    [...]

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dim.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dim.c
index 298bb74ec5e9..d1d629697e28 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_dim.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dim.c
@@ -113,7 +113,7 @@ int mlx5e_dim_rx_change(struct mlx5e_rq *rq, bool enable)
      __set_bit(MLX5E_RQ_STATE_DIM, &rq->state);
   } else {
      __clear_bit(MLX5E_RQ_STATE_DIM, &rq->state);
-
+     synchronize_net();
      mlx5e_dim_disable(rq->dim);
      rq->dim = NULL;
   }
@@ -140,7 +140,7 @@ int mlx5e_dim_tx_change(struct mlx5e_txqsq *sq, bool enable)
      __set_bit(MLX5E_SQ_STATE_DIM, &sq->state);
   } else {
      __clear_bit(MLX5E_SQ_STATE_DIM, &sq->state);
-
+     synchronize_net();
      mlx5e_dim_disable(sq->dim);
      sq->dim = NULL;
   }

Thanks,

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