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

[Théo Lebrun]

Replying to sashiko
https://sashiko.dev/#/patchset/20260805-macb-context-v8-0-bc302ffd1174%40bootlin.com

> commit 370786abcd6adf66f9c2032c12fec77c99f4d627
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>
> net: macb: use context swapping in .ndo_change_mtu()
>
> Use newly introduced context buffer management to implement
> .ndo_change_mtu() as a context swap: allocate new context ->
> reconfigure HW -> free old context.
>
> This resists memory pressure well by failing without closing the
> interface and it is much faster by avoiding PHY reinit.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 4e004a5345b39..b3d84a65078f8 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -3533,11 +3533,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;
> > +	struct macb *bp = netdev_priv(netdev);
> > +	bool running = netif_running(netdev);
> > +	struct macb_context *new_ctx;
> > +
> > +	if (running) {
> > +		/* Context swapping is not supported for AT91. */
> > +		if (bp->caps & MACB_CAPS_MACB_IS_EMAC)
> > +			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 the watchdog timer cause a use-after-free by concurrently accessing the
> old context pointer while macb_context_swap_end() is freeing it?
>
> If the interface is idle when the MTU is changed, the trans_start timestamp
> may be older than the watchdog timeout.  macb_context_swap_start() calls
> netif_tx_disable() to stop the tx queues, but does not update trans_start.
>
> This can allow the asynchronous dev_watchdog timer to conclude a tx timeout
> has occurred and trigger macb_tx_timeout() -> macb_tx_restart(), which reads
> the context:
>
> static void macb_tx_restart(struct macb_queue *queue)
> {
> 	struct macb_context *ctx = queue->bp->ctx;
> 	struct macb_txq *txq = macb_txq(queue);
>
> At the same time, macb_context_swap_end() replaces the context pointer
> and frees the old context:
>
> 	macb_free(old_ctx);
> 	kfree(old_ctx);
>
> Could this race cause memory corruption if macb_tx_restart() accesses the old
> context after it is freed?

Well, this is a fresh report for once! But it is completely unrelated to
our series at hand. We implement context swapping.

Let's investigate anyway.

 - First I checked and nothing serialises all those call together at the
   subsystem layer. ndo_tx_timeout is only under netdev->tx_global_lock.

 - ndo_tx_timeout is in softirq context so no bp->mac_cfg_lock mutex to
   save us, that would have been the easy solution.

 - The proper solution is therefore bp->lock with a bp->ctx_swap check
   inside. As queue->tx_ptr_lock is also involved we must make sure to
   respect the ordering done elsewhere in the driver, so something like
   the following in macb_tx_timeout() should fix it:

   spin_lock_irqsave(&queue->tx_ptr_lock, flags);
   spin_lock(&bp->lock);
   if (bp->ctx_swap)
      goto out;
   ...
   spin_unlock(&bp->lock);
   spin_unlock_irqrestore(&queue->tx_ptr_lock, flags);

This is all out of scope and series is way too large already, it'll have
to wait.

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