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

[Andy Shevchenko]

On Wed, Mar 25, 2026 at 06:22:33PM +0800, Felix Gu wrote:
> On Wed, Mar 25, 2026 at 5:54 PM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> >
> > > 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?
> 
> I got the thoughts from an AI review from sashiko and  verified it on my own.
> https://sashiko.dev/#/patchset/20260322-rockchip-v1-1-fac3f0c6dad8%40gmail.com

You must have said that in the first place. And have you found a real bug
when verified on your own? Or is it just theoretical possibility? If 'yes'
is the answer to the last question, then theoretically we have thousands
of ways in the kernel to shoot ourselves in the foot.

-- 
With Best Regards,
Andy Shevchenko