leds: fix broken devres usage

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

Quoted 4 lines · Johan Hovold

Several MFD child drivers register their class devices directly under the parent device (about half of the MFD LED drivers do so).

Quoted 6 lines

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?

Quoted 12 lines · Andy Shevchenko

Quoted 10 lines · Johan Hovold

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

Quoted 1 line

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.

Quoted 14 lines · Johan Hovold

Quoted 12 lines · Andy Shevchenko

Quoted 10 lines · Johan Hovold

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

Quoted 6 lines

Quoted 1 line

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?

Quoted 39 lines · Andy Shevchenko

Quoted 13 lines · Johan Hovold

Quoted 12 lines · Andy Shevchenko

Quoted 10 lines · Johan Hovold

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

Quoted 6 lines

Quoted 1 line

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

Quoted 7 lines

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

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

but foo remains registered until the parent driver is unbound.

Quoted 29 lines · Johan Hovold

Quoted 14 lines · Andy Shevchenko

Quoted 13 lines · Johan Hovold

Quoted 12 lines · Andy Shevchenko

Quoted 10 lines · Johan Hovold

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

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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.

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:

Quoted 4 lines

        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?

Quoted 5 lines

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?

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Johan Hovold

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Johan Hovold

On Mon, Jun 01, 2020 at 04:51:01PM +0300, Andy Shevchenko wrote:

Quoted 12 lines

Quoted 12 lines

Quoted 6 lines

Quoted 4 lines

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.

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:

Quoted 7 lines

Quoted 4 lines

        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?

As I mentioned in a commit message, we have quite a few MFD drivers that register class devices under the parent device directly and have been doing so for a very long time.

As this is reflected in sysfs and we may have users relying on the current topology, changing this shouldn't be taken too lightly (drivers may also depend on it). And in any case, it wouldn't be stable material to fix the regressions at hand.

Quoted 8 lines

Quoted 5 lines

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?

No, that's a separate issue as it also changes the device tree.

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

Included is also a clean up removing some pointless casts when registering class devices.

All but the lm3533 one have only been compile tested.