Re: [PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Johan Hovold <johan@kernel.org>]

On Tue, Mar 24, 2026 at 10:55:48PM +0800, Felix Gu wrote:
> >  /**
> > @@ -3398,22 +3395,14 @@ static void devm_spi_unregister(struct device *dev, void *res)
> >  int devm_spi_register_controller(struct device *dev,
> >  				 struct spi_controller *ctlr)
> >  {
> > -	struct spi_controller **ptr;
> >  	int ret;
> >  
> > -	ptr = devres_alloc(devm_spi_unregister, sizeof(*ptr), GFP_KERNEL);
> > -	if (!ptr)
> > -		return -ENOMEM;
> > -
> >  	ret = spi_register_controller(ctlr);
> > -	if (!ret) {
> > -		*ptr = ctlr;
> > -		devres_add(dev, ptr);
> > -	} else {
> > -		devres_free(ptr);
> > -	}
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_add_action_or_reset(dev, devm_spi_unregister_controller, ctlr);
> >  
> > -	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(devm_spi_register_controller);

> It seems has a potential corner case in this commit.
> 
> The issue is that devm_add_action_or_reset() triggers its callback 
> immediately if the internal devres allocation fails. This creates the 
> following sequence:
> 1. spi_register_controller(ctlr) succeeds.
> 2. devm_add_action_or_reset() is called but fails.
> 3. The "reset" triggers devm_spi_unregister_controller(ctlr) immediately.
> 4. This calls spi_unregister_controller(ctlr).
> 5. For controllers allocated via spi_alloc_host() (where 
>    ctlr->devm_allocated is false), spi_unregister_controller() calls 
>    put_device(&ctlr->dev).
> 6. This drops the reference count to zero and frees the ctlr structure.
> 
> However, the driver still holds the ctlr pointer and will typically 
> attempt its own cleanup, leading to a use-after-free or double-free.

Indeed, you're right. I stumbled over this change as well recently an
flagged it as something that's likely broken, but when I had another
quick look at it I somehow convinced myself that my instinct had been
wrong.

I just sent a patch to fix this here:

	https://lore.kernel.org/all/20260325145319.1132072-1-johan@kernel.org/

I think handling this explicitly is preferred over reverting as
otherwise someone will just send the same conversion again later.

Johan