Re: [PATCH net-next v9 08/17] net: macb: introduce macb_context struct for buffer management

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

Replying to non-net sashiko
https://sashiko.dev/#/patchset/20260812-macb-context-v9-0-7ddbf5f715e0%40bootlin.com

> commit 64775f904ae680bc33819f27120daf167d287348
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
> Subject: net: macb: introduce macb_context struct for buffer management
>
> Whenever an operation requires buffer realloc, we close the interface,
> update parameters and reopen. This commit introduces the macb_context
> struct to own all queue buffers and parameters, paving the way for
> supporting multiple contexts in parallel to improve reliability under
> memory pressure.
>
> > 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
>
> [ ... ]
>
> > @@ -1257,6 +1288,7 @@ static void macb_tx_error_task(struct work_struct *work)
> >  	struct macb_queue *queue = container_of(work, struct macb_queue,
> >  						tx_error_task);
> >  	unsigned int q = queue - queue->bp->queues;
> > +	struct macb_txq *txq = macb_txq(queue);
> >  	struct macb *bp = queue->bp;
> >  	struct macb_tx_skb *tx_skb;
> >  	struct macb_dma_desc *desc;
>
> [Severity: High]
> Does this asynchronously executing work item dereference the bp->ctx
> pointer unconditionally via the macb_txq() helper?
>
> If the interface is brought down, macb_close() is called, which frees
> bp->ctx and sets it to NULL. If a hardware error interrupt had just fired
> and scheduled queue->tx_error_task, it appears this task could continue to
> execute because it is not cancelled via cancel_work_sync() in macb_close().
> Could this result in a NULL pointer dereference or use-after-free?

Yes! This is all unrelated to the context swapping series but as the
context swapping codepath does it properly we can fix the macb_close
codepath along the way.

The sequence will be something like:
 - quiesce IRQs to make sure they cannot re-schedule our BH tasks
 - synchronize_irq(bp->queues[i].irq) for all i
 - drain all BH tasks (in the right order!)
    - bp->hresp_err_bh_work
    - queue->tx_error_task
    - queue->napi_rx
    - queue->napi_tx
    - bp->tx_lpi_work

Here is my "what schedules what?" table which explains the ordering:

|               |   enable/disable   |           schedule            |
|               |----|-------|-------|------|----|----|--------|-----|
|               |IRQs|napi_tx|napi_rx|tx_lpi|napi|napi|tx_error|hresp|
| Context       |    |       |       | task | rx | tx |  task  |task |
|===============|====|=======|=======|======|====|====|========|=====|
| open          | X  |   X   |   X   |      |    |    |        |     |
| link_up       | X  |       |       |      |    |    |        |     |
| link_down     | X  |       |       |      |    |    |        |     |
| close         | X  |   X   |   X   |      |    |    |        |     |
| enable_tx_lpi |    |       |       |  X   |    |    |        |     |
| swap          | X  |   X   |   X   |  X   |    |    |        |     |
| suspend       | X  |   X   |   X   |      |    |    |        |     |
| resume        | X  |   X   |   X   |      |    |    |        |     |
|---------------|----|-------|-------|------|----|----|--------|-----|
| irq & netpoll | X  |       |       |      | X  | X  |   X    | X   |
|---------------|----|-------|-------|------|----|----|--------|-----|
| napi_rx       | X  |       |       |      | X  |    |        |     |
| napi_tx       | X  |       |       |  X   |    | X  |        |     |
|---------------|----|-------|-------|------|----|----|--------|-----|
| tx_error_task | X  |   X   |       |      |    |    |        |     |
| hresp task    | X  |       |       |      |    |    |        |     |


>
> [ ... ]
>
> > @@ -3222,6 +3300,8 @@ static int macb_close(struct net_device *netdev)
> >  	spin_unlock_irqrestore(&bp->lock, flags);
> >  
> >  	macb_free(bp);
> > +	kfree(bp->ctx);
> > +	bp->ctx = NULL;
>
> [Severity: High]
> Is it possible that queue->tx_error_task or bp->hresp_err_bh_work
> are still running or pending when we reach this point?

Yes, see above.

[...]

> [Severity: High]
> This is a pre-existing issue, but does macb_close() need to call
> synchronize_irq() before freeing resources?

Yes, see above.

[...]

>
> >
> >  	if (bp->ptp_info)
> >  		bp->ptp_info->ptp_remove(netdev);
>
> [ ... ]
>
> [Severity: High]
> This is a pre-existing issue, but does macb_interrupt() safely handle
> shared interrupts when the device is runtime suspended?
>
> drivers/net/ethernet/cadence/macb_main.c:macb_interrupt() {
>     ...
>     spin_lock(&bp->lock);
>
>     status = queue_readl(queue, ISR);
>     if (unlikely(!status)) {
>         spin_unlock(&bp->lock);
>         return IRQ_NONE;
>     ...
> }
>
> If the device enters runtime suspend and its clocks are disabled, and
> another device sharing the same IRQ line triggers an interrupt, this
> handler executes and performs queue_readl(queue, ISR) without verifying
> the device's power management state. Could this trigger a synchronous
> external abort (bus fault)?
>
> Using pm_runtime_get_if_active() before accessing hardware registers in
> shared IRQ handlers is typically required to prevent accessing powered-off
> hardware.

Too unrelated to context swapping. Also it only crashes if shared lanes
are with separate interfaces. I hope no hardware like that exists in
the wild. We supported shared interrupts only because some HW uses a
single lane for all queues.

Long term I'd like to have a single handler for those IP blocks.
Goal being to reduce overhead and remove that pesky IRQF_SHARED flag.

---

Replying to net sashiko
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260812-macb-context-v9-0-7ddbf5f715e0%40bootlin.com

> commit d54f7e79d0f10ea8e363d1bb3711374a51ae9c90
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
>
> net: macb: introduce macb_context struct for buffer management
>
> This moves the per-queue ring/buffer pointers and the ring size
> parameters out of struct macb / struct macb_queue into a new
> heap-allocated struct macb_context, allocated in macb_open() and
> at91ether_open() and freed in the close paths. All accessors are
> converted to reach the state through bp->ctx.
>
> > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> > index 1e1f52285a39..95cb494190c5 100644
> > --- a/drivers/net/ethernet/cadence/macb.h
> > +++ b/drivers/net/ethernet/cadence/macb.h
>
> [ ... ]
>
> > @@ -1295,6 +1284,32 @@ struct ethtool_rx_fs_list {
> >  	unsigned int count;
> >  };
> >  
> > +struct macb_rxq {
> > +	struct macb_dma_desc	*ring;		/* MACB & GEM */
> > +	dma_addr_t		ring_dma;	/* MACB & GEM */
> > +	unsigned int		tail;		/* MACB & GEM */
> > +	unsigned int		prepared_head;	/* GEM */
> > +	struct sk_buff		**skbuff;	/* GEM */
> > +	dma_addr_t		buffers_dma;	/* MACB */
> > +	void			*buffers;	/* MACB */
> > +};
> > +
> > +struct macb_txq {
> > +	unsigned int		head;
> > +	unsigned int		tail;
> > +	struct macb_dma_desc	*ring;
> > +	dma_addr_t		ring_dma;
> > +	struct macb_tx_skb	*skb;
> > +};
> > +
> > +struct macb_context {
> > +	unsigned int		rx_buffer_size;
> > +	unsigned int		rx_ring_size;
> > +	unsigned int		tx_ring_size;
> > +	struct macb_rxq		rxq[MACB_MAX_QUEUES];
> > +	struct macb_txq		txq[MACB_MAX_QUEUES];
> > +};
> > +
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 1476bce77f34..380ee72e26ac 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
>
> [ ... ]
>
> > @@ -1257,6 +1288,7 @@ static void macb_tx_error_task(struct work_struct *work)
> >  	struct macb_queue *queue = container_of(work, struct macb_queue,
> >  						tx_error_task);
> >  	unsigned int q = queue - queue->bp->queues;
> > +	struct macb_txq *txq = macb_txq(queue);
> >  	struct macb *bp = queue->bp;
> >  	struct macb_tx_skb *tx_skb;
> >  	struct macb_dma_desc *desc;
>
> [ ... ]
>
> > @@ -1353,10 +1385,10 @@ static void macb_tx_error_task(struct work_struct *work)
> >  	wmb();
> >  
> >  	/* Reinitialize the TX desc queue */
> > -	queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
> > +	queue_writel(queue, TBQP, lower_32_bits(txq->ring_dma));
> >  	/* Make TX ring reflect state of hardware */
> > -	queue->tx_head = 0;
> > -	queue->tx_tail = 0;
> > +	txq->head = 0;
> > +	txq->tail = 0;
>
> [ ... ]
>
> > @@ -2615,60 +2663,71 @@ static void gem_free_rx_buffers(struct macb *bp)
>
> [ ... ]
>
> >  static void macb_free(struct macb *bp)
> >  {
> >  	struct device *dev = &bp->pdev->dev;
> >  	struct macb_queue *queue;
> > +	struct macb_txq *txq;
> > +	struct macb_rxq *rxq;
> >  	unsigned int q;
> >  	size_t size;
> >  
> >  	bp->macbgem_ops.mog_free_rx_buffers(bp);
> >  
> > +	txq = &bp->ctx->txq[0];
> >  	size = bp->num_queues * macb_tx_ring_size_per_queue(bp);
> > -	dma_free_coherent(dev, size, bp->queues[0].tx_ring, bp->queues[0].tx_ring_dma);
> > +	dma_free_coherent(dev, size, txq->ring, txq->ring_dma);
> >  
> > +	rxq = &bp->ctx->rxq[0];
> >  	size = bp->num_queues * macb_rx_ring_size_per_queue(bp);
> > -	dma_free_coherent(dev, size, bp->queues[0].rx_ring, bp->queues[0].rx_ring_dma);
> > +	dma_free_coherent(dev, size, rxq->ring, rxq->ring_dma);
>
> [ ... ]
>
> > @@ -2728,7 +2786,8 @@ static int macb_alloc(struct macb *bp)
> >  {
> >  	struct device *dev = &bp->pdev->dev;
> >  	dma_addr_t tx_dma, rx_dma;
> > -	struct macb_queue *queue;
> > +	struct macb_txq *txq;
> > +	struct macb_rxq *rxq;
> >  	unsigned int q;
> >  	void *tx, *rx;
> >  	size_t size;
> > @@ -2754,16 +2813,19 @@ static int macb_alloc(struct macb *bp)
> >  	netdev_dbg(bp->netdev, "Allocated %zu bytes for %u RX rings at %08lx (mapped %p)\n",
> >  		   size, bp->num_queues, (unsigned long)rx_dma, rx);
> >  
> > -	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> > -		queue->tx_ring = tx + macb_tx_ring_size_per_queue(bp) * q;
> > -		queue->tx_ring_dma = tx_dma + macb_tx_ring_size_per_queue(bp) * q;
> > +	for (q = 0; q < bp->num_queues; ++q) {
> > +		txq = &bp->ctx->txq[q];
> > +		rxq = &bp->ctx->rxq[q];
> > +
> > +		txq->ring = tx + macb_tx_ring_size_per_queue(bp) * q;
> > +		txq->ring_dma = tx_dma + macb_tx_ring_size_per_queue(bp) * q;
> >  
> > -		queue->rx_ring = rx + macb_rx_ring_size_per_queue(bp) * q;
> > -		queue->rx_ring_dma = rx_dma + macb_rx_ring_size_per_queue(bp) * q;
> > +		rxq->ring = rx + macb_rx_ring_size_per_queue(bp) * q;
> > +		rxq->ring_dma = rx_dma + macb_rx_ring_size_per_queue(bp) * q;
>
> [Severity: Medium]
> [Finding: gpt-5-6-sol-4-12]
> [Sources: gpt-5-6-sol, opus-5]
> This isn't a bug introduced by this patch, but while macb_alloc() is being
> reshaped here, does the tx coherent block get leaked on the early error
> paths?

Yes indeed. I've got a fix for that incoming. But completely unrelated.

[...]

> > @@ -3140,14 +3207,22 @@ static int macb_open(struct net_device *netdev)
> >  	if (err < 0)
> >  		return err;
> >  
> > +	bp->ctx = kzalloc_obj(*bp->ctx);
> > +	if (!bp->ctx) {
> > +		err = -ENOMEM;
> > +		goto pm_exit;
> > +	}
> > +
> >  	/* RX buffers initialization */
> >  	macb_init_rx_buffer_size(bp, bufsz);
> > +	bp->ctx->rx_ring_size = bp->configured_rx_ring_size;
> > +	bp->ctx->tx_ring_size = bp->configured_tx_ring_size;
> >  
> >  	err = macb_alloc(bp);
> >  	if (err) {
> >  		netdev_err(netdev, "Unable to allocate DMA memory (error %d)\n",
> >  			   err);
> > -		goto pm_exit;
> > +		goto free_ctx;
> >  	}
> >  
> >  	bp->macbgem_ops.mog_init_rings(bp);
> > @@ -3189,6 +3264,9 @@ static int macb_open(struct net_device *netdev)
> >  		napi_disable(&queue->napi_tx);
> >  	}
> >  	macb_free(bp);
> > +free_ctx:
> > +	kfree(bp->ctx);
> > +	bp->ctx = NULL;
> >  pm_exit:
> >  	pm_runtime_put_sync(&bp->pdev->dev);
> >  	return err;
> > @@ -3222,6 +3300,8 @@ static int macb_close(struct net_device *netdev)
> >  	spin_unlock_irqrestore(&bp->lock, flags);
> >  
> >  	macb_free(bp);
> > +	kfree(bp->ctx);
> > +	bp->ctx = NULL;
>
> [Severity: High]
> [Finding: gpt-5-6-sol-4-11]
> [Sources: gpt-5-6-sol, opus-5]
> Can a pending queue->tx_error_task still reference the context after this
> kfree()?

Yes, see above.

[...]

> > @@ -3712,8 +3799,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);
>
> [Severity: High]
> [Finding: gpt-5-6-sol-3-7]
> [Sources: gpt-5-6-sol, opus-5]
> What happens here if macb_open() fails now that it can fail at the new
> kzalloc_obj() of bp->ctx?

We ignore this error path because at the end of the series this gets
turned into a context swap operation and the context_swap_end cannot
fail!

[...]

> > @@ -5061,6 +5149,12 @@ static int at91ether_open(struct net_device *netdev)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	bp->ctx = kzalloc_obj(*bp->ctx);
> > +	if (!bp->ctx) {
> > +		ret = -ENOMEM;
> > +		goto pm_exit;
> > +	}
> > +
>
> [Severity: Low]
> [Finding: gpt-5-6-sol-2-3]
> [Sources: gpt-5-6-sol, opus-5]
> This isn't a bug today, but should at91ether_open() also initialise the
> context parameters the way macb_open() does?

No because "configured_*_ring_size" does not mean anything on at91ether
which has 1-sized tx ring or 4-sized rx ring. We refuse the ring size
config ops, as expected.

[...]

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