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

[Andy Shevchenko]

On Tue, Mar 24, 2026 at 10:55:48PM +0800, Felix Gu wrote:

...

> >  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;
> >  }

> 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.

Yes, it does.

> 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.

I am not sure I see how the sequence is different from the say this

	ret = spi_register_controller(...)
	if (ret)
		goto err;
	// ...if success do something else...
	ret = foo();
	if (ret) {
		spi_unregister_controller(...);
		goto err;
	}

So, each driver should be prepared for an error handling in which it should
release resources in the reversed order.

> However, the driver still holds the ctlr pointer and will typically 
> attempt its own cleanup, leading to a use-after-free or double-free.

You mean buggy driver? Any real example of the use case?

In the 5. you implied something like

	ret = spi_alloc_host();
	...
	ret = devm_spi_register_controller()

?

But this is against the rule how devm must be used.

> What are your thoughts on this? Does this analysis seem correct, or am
> I missing a detail in how the refcounting is handled here?

Is this analysis AI assisted?

-- 
With Best Regards,
Andy Shevchenko