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

[Théo Lebrun]

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

> commit 275a99a81c17ff48dfcda588314d331d16fe6efa
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
> net: macb: introduce macb_context struct for buffer management
>
> This commit introduces the macb_context structure to own all queue buffers
> and their associated parameters. To improve reliability under memory pressure,
> it restructures the driver to support allocating new buffers before freeing
> old ones. Various fields are migrated from macb and macb_queue structs to
> macb_context, macb_txq, and macb_rxq.
>
> [ ... ]
>
> > @@ -3701,8 +3788,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;
> >  	}
> > @@ -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);
> >  
> >  	return 0;
> >  }
>
> [Severity: High]
> Is there a risk of a NULL pointer dereference if macb_open() fails here?
>
> If macb_open() fails (for example, due to a memory allocation failure), it
> returns an error, but this function ignores the return value and always returns
> 0. Because the interface state wasn't updated via the core dev_close(),
> netif_running(netdev) remains true, while bp->ctx is left as NULL.
>
> If the system later suspends and resumes, macb_resume() will see that
> netif_running(netdev) is true and call macb_init_buffers():
>
> static int __maybe_unused macb_resume(struct device *dev)
> {
>         ...
>         if (netif_running(netdev)) {
>                 ...
>                 if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC))
>                         macb_init_buffers(bp);
>
> This would unconditionally dereference the NULL bp->ctx pointer. Should the
> return value of macb_open() be checked and propagated here?

Two replies:

 - This is a pre-existing issue, but not flagged as such.

 - We fix it at the end of the series, but using context swapping. It's
   basically the whole point of the series. I'm surprised Sashiko
   doesn't pick that up by itself, nor does it pick up that I already
   told it the same thing on the same patch on previous revisions.

Thanks,

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