Re: [PATCH 0/6] leds: fix broken devres usage

[Andy Shevchenko]

On Mon, Jun 1, 2020 at 5:29 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> On Mon, Jun 01, 2020 at 05:08:40PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 1, 2020 at 5:01 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> > > On Mon, Jun 01, 2020 at 04:51:01PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Jun 1, 2020 at 4:42 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> > > > >
> > > > > Several MFD child drivers register their class devices directly under
> > > > > the parent device (about half of the MFD LED drivers do so).
> > > > >
> > > > > This means you cannot blindly do devres conversions so that
> > > > > deregistration ends up being tied to the parent device, something which
> > > > > leads to use-after-free on driver unbind when the class device is
> > > > > released while still being registered (and, for example, oopses on later
> > > > > parent MFD driver unbind or LED class callbacks, or resource leaks and
> > > > > name clashes on child driver reload).
> > > >
> > > > Shouldn't MFD take reference count for their children?
> > >
> > > That's not the issue here. The child driver is allocating memory for the
> > > class device (for example using devres), and that will end up being
> > > freed on unbind while said device is still registered. The child driver
> > > may then even be unloaded. No extra reference can fix this.
> >
> > Okay, I didn't still get how dropping devres will help here.
> >
> > Say, we have
> >
> > ->probe()
> > {
> >  return devm_foo_register();
> > }
> >
> > and no ->remove()
> >
> > vs.
> >
> > ->probe()
> > {
> >   return foo_register();
> > }
> >
> > ->remove()
> > {
> >  foo_unregister();
> > }
> >
> > So, basically what you seem to workaround is that ->remove() is not
> > getting called?
>
> Any driver which frees a resource before making sure it's no longer used
> it is just plain broken. Unfortunately, devres makes this harder to
> reason about and people get it wrong. This is roughly the current
> situation with these drivers:
>
>         drv->probe(dev)
>           foo = devm_kzalloc(dev);

>           devm_foo_register(dev->parent, foo);  // NOTE: dev->parent

A-ha! Thanks for this detail.
But why are they doing so?

>         drv->remove(dev)
>         devres_release_all(dev)
>           kfree(foo);                           // foo still registered

> but foo remains registered until the parent driver is unbound.

Since the last fixes against kobject elimination, shouldn't be this
simple fixed by not supplying dev->parent above?

-- 
With Best Regards,
Andy Shevchenko