Re: [PATCH net-next v11 0/9] net: macb: implement context swapping

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

Hello Théo,

Sashiko has 4 high comments, Clashiko has 3 low comments.

TL;DR: nothing worth a respin because it is all out of scope.

https://sashiko.dev/#/patchset/20261013-macb-context-v11-0-b27785167566%40bootlin.com
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-macb-context-v11-0-b27785167566%40bootlin.com

> commit 0eff5390c4748a085f669ce98d1bc433c0f36ace
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
> net: macb: introduce macb_context struct for buffer management
>
> This commit introduces a macb_context structure to own all queue
> buffers and parameters, paving the way for supporting multiple contexts
> in parallel during buffer reallocation. It updates ring parameters and
> moves related fields out of the main macb struct into the context struct.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> [ ... ]
> > @@ -2741,60 +2789,71 @@ static void gem_free_rx_buffers(struct macb *bp)
> [ ... ]
> >  static unsigned int macb_tx_ring_size_per_queue(struct macb *bp)
> >  {
> > - return macb_dma_desc_get_size(bp) * bp->tx_ring_size + bp->tx_bd_rd_prefetch;
> > + return macb_dma_desc_get_size(bp) * bp->ctx->tx_ring_size +
> > +    bp->tx_bd_rd_prefetch;
> >  }
>
> [Severity: High]
> Can this dereference a NULL pointer if the context was freed?
>
> If macb_open() fails, it now frees bp->ctx and leaves it NULL. Later, if
> the interface is closed and macb_free() calls this function:
>
> macb_close() -> macb_free() -> macb_tx_ring_size_per_queue()
>
> This will dereference bp->ctx.

Sashiko says there is an issue with a macb_open() failing followed by a
macb_close(). Indeed, but the interface won't be closed if its open
operation failed.

>
> [ ... ]
> > @@ -3856,8 +3946,8 @@ static int macb_set_ringparam(struct net_device *netdev,
> >      macb_close(bp->netdev);
> >   }
> >
> > - bp->rx_ring_size = new_rx_size;
> > - bp->tx_ring_size = new_tx_size;
> > + bp->configured_rx_ring_size = new_rx_size;
> > + bp->configured_tx_ring_size = new_tx_size;
> >
> >   if (reset)
> >      macb_open(bp->netdev);
> >
> >   return 0;
> >  }
>
> [Severity: High]
> Should we be checking the return value of macb_open() here?
>
> If macb_open() fails, it frees bp->ctx. However, this function
> unconditionally returns 0, masking the failure and leaving the device in
> an UP state in the network stack.
>
> A subsequent interface down event will trigger macb_close(), which calls
> macb_free(), resulting in the NULL pointer dereference in
> macb_tx_ring_size_per_queue() noted above.

We ignore that on purpose. Check out the last two commits. The whole
point of this series is to introduce context swapping, to fix the
set_ringparam operation (and others).

---

> commit b435a89b7b7a33cd5d0880d8a83b893da802f106
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
>
> net: macb: change function signatures to take contexts
>
> This commit changes many function signatures in the MACB driver to take a
> macb_context pointer instead of macb or macb_queue. This refactoring prepares
> the driver for supporting parallel MACB contexts while reducing the bug
> surface area by preventing the use of the wrong context.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> [ ... ]
> >   for (tail = txq->tail; tail != txq->head; tail++) {
> >      u32   ctrl;
> >
> > -    desc = macb_tx_desc(queue, tail);
> > +    desc = macb_tx_desc(ctx, q, tail);
>
> [Severity: High]
> Does this traversal and unmapping of the TX ring lack necessary synchronization?
>
> When macb_tx_error_task is triggered in a workqueue due to a TX error, it
> calls netif_tx_stop_all_queues(). This sets QUEUE_STATE_DRV_XOFF but does
> not wait for a concurrent macb_start_xmit on another CPU.

This is out of scope. We ignore suspend, the HRESP error task and the TX
error task on purpose.

[...]

> >      ctrl = desc->ctrl;
> >      tx_skb = macb_tx_skb(queue, tail);
> >      skb = tx_skb->skb;
> [ ... ]
> >   netif_tx_stop_all_queues(netdev);
> >   netif_carrier_off(netdev);
> >
> > - bp->macbgem_ops.mog_init_rings(bp);
> > + bp->macbgem_ops.mog_init_rings(ctx);
>
> [Severity: High]
> Could this ring reinitialization lead to a use-after-free or data corruption?
>
> When a hardware HRESP error triggers macb_hresp_error_task via system_bh_wq,
> mog_init_rings is called to reset ring pointers and reallocate SKB arrays.
> However, it does not appear to synchronize with software datapath operations
> using napi_disable() and spin_lock(&queue->tx_ptr_lock).

This is out of scope. We ignore suspend, the HRESP error task and the TX
error task on purpose.

[...]

---

> commit c7aa30e3b67cf259fbc73811fb17ffc27d9be6eb
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
>
>     net: macb: use context swapping in .set_ringparam()
>
> This patch introduces a context swapping mechanism for macb's set_ringparam
> ethtool operation. It avoids freeing buffers during interface close and
> reallocating them on open, instead pre-allocating a new context to prevent
> failures under memory pressure. The swap logic is synchronized against MAC
> callbacks and NAPI execution using a new mac_cfg_lock mutex and the quiesce
> state.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 32a24c91c045a..6bacf3e3caabb 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> [ ... ]
> > @@ -2010,29 +2038,37 @@ static int macb_rx_poll(struct napi_struct *napi, int budget)
> >
> >  static void macb_tx_restart(struct macb_queue *queue)
> >  {
> > - struct macb_context *ctx = queue->bp->ctx;
> > - struct macb_txq *txq = macb_txq(queue);
> >   struct macb *bp = queue->bp;
> >   unsigned int head_idx, tbqp;
> > + struct macb_context *ctx;
> > + struct macb_txq *txq;
> >   unsigned long flags;
> >
> >   spin_lock_irqsave(&queue->tx_ptr_lock, flags);
> >
> > + spin_lock(&bp->lock);
> > +
> > + /* context swap ongoing? */
> > + if (bp->irq_quiesced)
> > +    goto out_unlock;
> > +
> > + ctx = queue->bp->ctx;
> > + txq = macb_txq(queue);
> > +
> >   if (txq->head == txq->tail)
> > -    goto out_tx_ptr_unlock;
> > +    goto out_unlock;
>
> [Severity: High]
> Does macb_tx_error_task() in drivers/net/ethernet/cadence/macb_main.c also
> need to acquire queue->tx_ptr_lock when updating txq->head and txq->tail?

This is out of scope. We ignore suspend, the HRESP error task and the TX
error task on purpose.

[...]

---

> commit 59119f040d96bc1d81d6e0708a9c738cad768fcd
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
>
> net: macb: use context swapping in .set_ringparam()
>
> The old close/reopen sequence in macb_set_ringparam() is replaced. A new
> context with the requested ring sizes is now allocated before the
> interface is stopped, and it is swapped in with the new helpers
> macb_context_swap_start() and macb_context_swap_end(). A new mac_cfg_lock
> mutex serializes the swap against the phylink MAC callbacks, and a new
> bp->link_up boolean tracks link state under that lock.
>
> > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> > index ec2e2fbc1146a..b7b4b6a382980 100644
> > --- a/drivers/net/ethernet/cadence/macb.h
> > +++ b/drivers/net/ethernet/cadence/macb.h
>
> [ ... ]
>
> > @@ -1419,10 +1421,13 @@ struct macb {
> >   u32         tx_lpi_timer;
> >
> >   /* ISR must not drive NAPI & BH mechanisms. True when the interface
> > -  * is closed. Protected by bp->lock.
> > +  * is closed or during context-swap. Protected by bp->lock.
> >    */
> >   bool        irq_quiesced;
> >
> > + /* Redundant to netif_carrier_ok(), but set under bp->mac_cfg_lock. */
> > + bool        link_up;
>
> [Severity: Low]
> [Finding: main-2-3]
> [Sources: opus-5-5]
> Is the "redundant to netif_carrier_ok()" claim accurate?
> macb_hresp_error_task() changes the carrier and the hardware enable bits
> without taking mac_cfg_lock, and it never reads or updates bp->link_up:

Technically the "Redundant to netif_carrier_ok()" phrase is wrong yes
but I hope it is evident that if it was actually redundant, we wouldn't
have introduced a separate flag.

Fixing the many known races inbetween the HRESP task and the rest of the
driver execution is out of scope of this series.

[...]

> > @@ -3995,16 +4140,20 @@ static int macb_set_ringparam(struct net_device *netdev,
> >      return 0;
> >   }
> >
> > - if (netif_running(bp->netdev)) {
> > -    reset = 1;
> > -    macb_close(bp->netdev);
> > + if (running) {
> > +    new_ctx = macb_context_alloc(bp, netdev->mtu,
> > +                  new_rx_size, new_tx_size);
> > +    if (IS_ERR(new_ctx))
> > +       return PTR_ERR(new_ctx);
> > +
> > +    macb_context_swap_start(bp);
> >   }
> >
> >   bp->configured_rx_ring_size = new_rx_size;
> >   bp->configured_tx_ring_size = new_tx_size;
>

[... pre existing finding ...]

>
> >
> > - if (reset)
> > -    macb_open(bp->netdev);
> > + if (running)
> > +    macb_context_swap_end(bp, new_ctx);
> >
> >   return 0;
> >  }
>
> [Severity: Low]
> [Finding: gpt-6-astra-1-2]
> [Sources: gpt-6-astra, opus-5-5]
> The code change looks correct, but the commit message describes the old
> behaviour only as:
>
>   Under memory pressure this does not fly: we free our buffers at close
>   and cannot reallocate new ones at open.

I've cut the rambling. The point here is that the commit message is
somewhat underselling what we gain from context swapping. I couldn't
agree more, context swapping is great!

Clashiko wants to point out all the advantages and add a Fixes trailer.
I consider all that useless and see this patch as a new feature rather
than a bugfix.

[...]

---

> commit 9410fd54d1f3a19a62c932c210f14be133012c06
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
>
> net: macb: use context swapping in .ndo_change_mtu()
>
> This patch makes macb_change_mtu() handle a running interface by swapping
> contexts: it allocates a new context sized for the new MTU, quiesces and
> reconfigures the hardware, then frees the old context. Before this, the
> function returned -EBUSY. AT91 EMAC devices are rejected defensively, since
> they use a separate ndo struct and should never reach this path.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 6bacf3e3caabb..f087f278f9e56 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -3658,11 +3658,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;
>
> [Severity: Low]
> [Finding: main-2-0]
> [Sources: opus-5-5]
> This isn't a bug, but should this comment say "AT91 EMAC" instead of
> "AT91"?
>
> MACB_CAPS_MACB_IS_EMAC is set only by emac_config. Other AT91-family
> configs, such as at91sam9260_config, sama5d2_config and sama5d3_config,
> don't set it. Those parts use macb_netdev_ops, so they do get context
> swapping on a live MTU change. The commit message already says "AT91 EMAC".

On this one, I agree: AT91 is confusing, the proper way to describe the
old IP we support would be 'EMAC' or 'RM9200' (only board we support
that uses that IP revision).

I won't respin just for this comment tweak. If we wanted to fully remove
the confusion, we would also rename all functions from at91ether_* to
something else, like macb_emac_* or something of the sort. This feels
out of scope.

Thanks,

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