Re: [PATCH net-next v5 07/15] net: macb: introduce macb_context struct for buffer management

[Théo Lebrun]

This is a reply to both Sashikos comments.
https://sashiko.dev/#/patchset/20260724-macb-context-v5-0-569b1852bc7f%40bootlin.com
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260724-macb-context-v5-0-569b1852bc7f%40bootlin.com

sashiko.dev has comments on previous patches but they all match

> This is a pre-existing issue, but [...]

What remains:

> > @@ -3696,8 +3783,8 @@ static int macb_set_ringparam(struct net_device *netdev,
> >  			      MIN_TX_RING_SIZE, MAX_TX_RING_SIZE);
> >  	new_tx_size = roundup_pow_of_two(new_tx_size);
> >
> > -	if ((new_tx_size == bp->tx_ring_size) &&
> > -	    (new_rx_size == bp->rx_ring_size)) {
> > +	if (new_tx_size == bp->configured_tx_ring_size &&
> > +	    new_rx_size == bp->configured_rx_ring_size) {
> >  		/* nothing to do */
> >  		return 0;
> >  	}
> > @@ -3707,8 +3794,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: Critical]
> Does ignoring the return value of macb_open() here risk leaving the
> device in the UP state with a NULL bp->ctx?
>
> If a ring parameter update fails during macb_open() (for example, due to a
> DMA allocation failure), bp->ctx will be freed and left as NULL. Because
> the failure is ignored and 0 is returned, the network stack still
> considers the device to be UP. A subsequent interface down event will call
> macb_close() which will panic when it dereferences bp->ctx to free the
> queues.

This is also a pre-existing issue but it's wasn't flagged as one.
Anyway, it should be ignored because by the end of the series this
macb_open() disappears and we do context swapping.

---

For netdev sashiko now:

> > @@ -3138,14 +3205,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);
> > @@ -3187,6 +3262,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;
>
> [High]
> Once macb_init_hw() has enabled IRQs, can the hardware fire a TX error
> or HRESP interrupt whose handler schedules work before this error path
> runs?
>
> [...]

Same type of report. It sees a bug because now the race lands in a
bp->ctx null ptr deref, but the bug existed before and probably crashes
the system as well.

> > @@ -3220,6 +3298,8 @@ static int macb_close(struct net_device *netdev)
> >  	spin_unlock_irqrestore(&bp->lock, flags);
> >
> >  	macb_free(bp);
> > +	kfree(bp->ctx);
> > +	bp->ctx = NULL;
> >
> >  	if (bp->ptp_info)
> >  		bp->ptp_info->ptp_remove(netdev);
>
> [High, High]
> The same question applies here: after macb_reset_hw() masks the HW
> interrupts under bp->lock, can a handler that had already read ISR on
> another CPU still schedule tx_error_task or hresp_err_bh_work once
> bp->lock is dropped?
>
> [...]

Again, pre-existing issue.

> > @@ -5093,6 +5187,10 @@ static int at91ether_close(struct net_device *netdev)
> >
> >  	at91ether_stop(bp);
> >
> > +	synchronize_irq(bp->netdev->irq);
> > +	kfree(bp->ctx);
> > +	bp->ctx = NULL;
> > +
> >  	pm_runtime_put(&bp->pdev->dev);
> >
> >  	return 0;
>
> [High]
> Is the ordering here inverted?
>
> at91ether_stop() writes IDR to mask MAC interrupts and then calls
> at91ether_free_coherent(), which dma_free_coherent()s rxq->ring and
> rxq->buffers and sets both pointers to NULL. Only after that does
> synchronize_irq() run.
>
> If an at91ether_interrupt handler on another CPU had already read a
> set RCOMP bit before the IDR write became visible, it will proceed
> into at91ether_rx():

This is true: synchronize_irq() is too late. I'm pretty sure this is
moot anyway because AT91 is (old and) only single core. I expect a lot
of at91ether code would break if at91ether_*() functions were used on a
multicore SoC.

Anyway I'll fix in next revision.

Thanks,

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