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

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

commit 370786abcd6adf66f9c2032c12fec77c99f4d627
Author: Théo Lebrun [off-list ref]

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.

quoted

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);

[...]

quoted

+		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