Lots of drivers use devm_led_classdev_register() to register their led objects
and let the kernel free those leds at the driver's remove stage.
It can lead to a problem due to led_classdev_unregister()
implementation calls led_set_brightness() to turn off the led.
led_set_brightness() may call one of the module's brightness_set callbacks.
If that callback uses module's resources allocated without using devm funcs()
then those resources will be already freed at module's remove() callback and
we may have use-after-free situation.
Here is an example:
module_probe()
{
devm_led_classdev_register(module_brightness_set_cb);
mutex_init(&mutex);
}
module_brightness_set_cb()
{
mutex_lock(&mutex);
do_set_brightness();
mutex_unlock(&mutex);
}
module_remove()
{
mutex_destroy(&mutex);
}
at rmmod:
module_remove()
->mutex_destroy(&mutex);
devres_release_all()
->led_classdev_unregister();
->led_set_brightness();
->module_brightness_set_cb();
->mutex_lock(&mutex); /* use-after-free */
I think it's an architectural issue and should be discussed thoroughly.
Some thoughts about fixing it as a start:
1) drivers can use devm_led_classdev_unregister() to explicitly free leds before
dependend resources are freed. devm_led_classdev_register() remains being useful
to simplify probe implementation.
As a proof of concept I examined all drivers from drivers/leds and prepared
patches where it's needed. Sometimes it was not as clean as just calling
devm_led_classdev_unregister() because several drivers do not track
their leds object at all - they can call devm_led_classdev_register() and drop the
returned pointer. In that case I used devres group API.
Drivers outside drivers/leds should be checked too after discussion.
2) remove led_set_brightness from led_classdev_unregister() and force the drivers
to turn leds off at shutdown. May be add check that led's brightness is 0
at led_classdev_unregister() and put a warning to dmesg if it's not.
Actually in many cases it doesn't really need to turn off the leds manually one-by-one
if driver shutdowns whole led controller. For the last case to disable the warning
new flag can be brought in e.g LED_AUTO_OFF_AT_SHUTDOWN (similar to LED_RETAIN_AT_SHUTDOWN).
George Stark (8):
leds: powernv: explicitly unregister LEDs at module's shutdown
leds: nic78bx: explicitly unregister LEDs at module's shutdown
leds: an30259a: explicitly unregister LEDs at module's shutdown
leds: mlxreg: explicitly unregister LEDs at module's shutdown
leds: aw200xx: explicitly unregister LEDs at module's shutdown
leds: aw2013: explicitly unregister LEDs at module's shutdown
leds: lp3952: explicitly unregister LEDs at module's shutdown
leds: lm3532: explicitly unregister LEDs at module's shutdown
drivers/leds/leds-an30259a.c | 4 ++++
drivers/leds/leds-aw200xx.c | 4 ++++
drivers/leds/leds-aw2013.c | 4 ++++
drivers/leds/leds-lm3532.c | 6 ++++++
drivers/leds/leds-lp3952.c | 5 +++++
drivers/leds/leds-mlxreg.c | 12 +++++++++++-
drivers/leds/leds-nic78bx.c | 4 ++++
drivers/leds/leds-powernv.c | 7 +++++++
8 files changed, 45 insertions(+), 1 deletion(-)
--
2.38.4
LEDs are registered using devm_led_classdev_register() and automatically
unregistered after module's remove(). led_classdev_unregister() calls
led_set_brightness() to turn off the LEDs and module's appropriate callback
uses resources those were destroyed already in module's remove().
So explicitly unregister LEDs at module shutdown.
Signed-off-by: George Stark <[email protected]>
---
drivers/leds/leds-powernv.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
index 743e2cdd0891..7c7f696c8265 100644
--- a/drivers/leds/leds-powernv.c
+++ b/drivers/leds/leds-powernv.c
@@ -302,7 +302,12 @@ static int powernv_led_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, powernv_led_common);
+ if (!devres_open_group(&pdev->dev, priv, GFP_KERNEL))
+ return -ENOMEM;
+
rc = powernv_led_classdev(pdev, led_node, powernv_led_common);
+ if (rc)
+ devres_remove_group(dev, priv);
out:
of_node_put(led_node);
return rc;
@@ -313,6 +318,8 @@ static int powernv_led_remove(struct platform_device *pdev)
{
struct powernv_led_common *powernv_led_common;
+ devres_release_group(&pdev->dev, powernv_led_common);
+
/* Disable LED operation */
powernv_led_common = platform_get_drvdata(pdev);
powernv_led_common->led_disabled = true;
--
2.38.4
LEDs are registered using devm_led_classdev_register() and automatically
unregistered after module's remove(). led_classdev_unregister() calls
led_set_brightness() to turn off the LEDs and module's appropriate callback
uses resources those were destroyed already in module's remove().
So explicitly unregister LEDs at module shutdown.
Signed-off-by: George Stark <[email protected]>
---
drivers/leds/leds-aw200xx.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 96979b8e09b7..3da0923507ec 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -539,6 +539,10 @@ static int aw200xx_probe(struct i2c_client *client)
static void aw200xx_remove(struct i2c_client *client)
{
struct aw200xx *chip = i2c_get_clientdata(client);
+ int i;
+
+ for (i = 0; i < chip->num_leds; i++)
+ devm_led_classdev_unregister(&client->dev, &chip->leds[i].cdev);
aw200xx_chip_reset(chip);
mutex_destroy(&chip->mutex);
--
2.38.4
LEDs are registered using devm_led_classdev_register() and automatically
unregistered after module's remove(). led_classdev_unregister() calls
led_set_brightness() to turn off the LEDs and module's appropriate callback
uses resources those were destroyed already in module's remove().
So explicitly unregister LEDs at module shutdown.
Signed-off-by: George Stark <[email protected]>
---
drivers/leds/leds-lp3952.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c
index 3bd55652a706..2de49192011a 100644
--- a/drivers/leds/leds-lp3952.c
+++ b/drivers/leds/leds-lp3952.c
@@ -257,8 +257,13 @@ static int lp3952_probe(struct i2c_client *client)
static void lp3952_remove(struct i2c_client *client)
{
struct lp3952_led_array *priv;
+ int i;
priv = i2c_get_clientdata(client);
+ for (i = 0; i < LP3952_LED_ALL; i++)
+ if (priv->leds[i].priv)
+ devm_led_classdev_unregister(&client->dev,
+ &priv->leds[i].cdev);
lp3952_on_off(priv, LP3952_LED_ALL, false);
gpiod_set_value(priv->enable_gpio, 0);
}
--
2.38.4
Hello Andy
Could you please take a look at this patch series?
I've just found your post on habr about devres API misusing and I think
this is just another case.
On 10/25/23 16:07, George Stark wrote:
> Lots of drivers use devm_led_classdev_register() to register their led objects
> and let the kernel free those leds at the driver's remove stage.
> It can lead to a problem due to led_classdev_unregister()
> implementation calls led_set_brightness() to turn off the led.
> led_set_brightness() may call one of the module's brightness_set callbacks.
> If that callback uses module's resources allocated without using devm funcs()
> then those resources will be already freed at module's remove() callback and
> we may have use-after-free situation.
>
> Here is an example:
>
> module_probe()
> {
> devm_led_classdev_register(module_brightness_set_cb);
> mutex_init(&mutex);
> }
>
> module_brightness_set_cb()
> {
> mutex_lock(&mutex);
> do_set_brightness();
> mutex_unlock(&mutex);
> }
>
> module_remove()
> {
> mutex_destroy(&mutex);
> }
>
> at rmmod:
> module_remove()
> ->mutex_destroy(&mutex);
> devres_release_all()
> ->led_classdev_unregister();
> ->led_set_brightness();
> ->module_brightness_set_cb();
> ->mutex_lock(&mutex); /* use-after-free */
>
> I think it's an architectural issue and should be discussed thoroughly.
> Some thoughts about fixing it as a start:
> 1) drivers can use devm_led_classdev_unregister() to explicitly free leds before
> dependend resources are freed. devm_led_classdev_register() remains being useful
> to simplify probe implementation.
> As a proof of concept I examined all drivers from drivers/leds and prepared
> patches where it's needed. Sometimes it was not as clean as just calling
> devm_led_classdev_unregister() because several drivers do not track
> their leds object at all - they can call devm_led_classdev_register() and drop the
> returned pointer. In that case I used devres group API.
>
> Drivers outside drivers/leds should be checked too after discussion.
>
> 2) remove led_set_brightness from led_classdev_unregister() and force the drivers
> to turn leds off at shutdown. May be add check that led's brightness is 0
> at led_classdev_unregister() and put a warning to dmesg if it's not.
> Actually in many cases it doesn't really need to turn off the leds manually one-by-one
> if driver shutdowns whole led controller. For the last case to disable the warning
> new flag can be brought in e.g LED_AUTO_OFF_AT_SHUTDOWN (similar to LED_RETAIN_AT_SHUTDOWN).
>
> George Stark (8):
> leds: powernv: explicitly unregister LEDs at module's shutdown
> leds: nic78bx: explicitly unregister LEDs at module's shutdown
> leds: an30259a: explicitly unregister LEDs at module's shutdown
> leds: mlxreg: explicitly unregister LEDs at module's shutdown
> leds: aw200xx: explicitly unregister LEDs at module's shutdown
> leds: aw2013: explicitly unregister LEDs at module's shutdown
> leds: lp3952: explicitly unregister LEDs at module's shutdown
> leds: lm3532: explicitly unregister LEDs at module's shutdown
>
> drivers/leds/leds-an30259a.c | 4 ++++
> drivers/leds/leds-aw200xx.c | 4 ++++
> drivers/leds/leds-aw2013.c | 4 ++++
> drivers/leds/leds-lm3532.c | 6 ++++++
> drivers/leds/leds-lp3952.c | 5 +++++
> drivers/leds/leds-mlxreg.c | 12 +++++++++++-
> drivers/leds/leds-nic78bx.c | 4 ++++
> drivers/leds/leds-powernv.c | 7 +++++++
> 8 files changed, 45 insertions(+), 1 deletion(-)
>
--
Best regards
George
Le 25/10/2023 à 15:07, George Stark a écrit :
> Lots of drivers use devm_led_classdev_register() to register their led objects
> and let the kernel free those leds at the driver's remove stage.
> It can lead to a problem due to led_classdev_unregister()
> implementation calls led_set_brightness() to turn off the led.
> led_set_brightness() may call one of the module's brightness_set callbacks.
> If that callback uses module's resources allocated without using devm funcs()
> then those resources will be already freed at module's remove() callback and
> we may have use-after-free situation.
>
> Here is an example:
>
> module_probe()
> {
> devm_led_classdev_register(module_brightness_set_cb);
> mutex_init(&mutex);
> }
>
> module_brightness_set_cb()
> {
> mutex_lock(&mutex);
> do_set_brightness();
> mutex_unlock(&mutex);
> }
>
> module_remove()
> {
> mutex_destroy(&mutex);
> }
>
> at rmmod:
> module_remove()
> ->mutex_destroy(&mutex);
> devres_release_all()
> ->led_classdev_unregister();
> ->led_set_brightness();
> ->module_brightness_set_cb();
> ->mutex_lock(&mutex); /* use-after-free */
>
> I think it's an architectural issue and should be discussed thoroughly.
> Some thoughts about fixing it as a start:
> 1) drivers can use devm_led_classdev_unregister() to explicitly free leds before
> dependend resources are freed. devm_led_classdev_register() remains being useful
> to simplify probe implementation.
> As a proof of concept I examined all drivers from drivers/leds and prepared
> patches where it's needed. Sometimes it was not as clean as just calling
> devm_led_classdev_unregister() because several drivers do not track
> their leds object at all - they can call devm_led_classdev_register() and drop the
> returned pointer. In that case I used devres group API.
I see no point in using a device managed function if you have to call
the unregister function. All the purpose of device managed functions is
to avoid having to call an unregister function at the end.
>
> Drivers outside drivers/leds should be checked too after discussion.
>
> 2) remove led_set_brightness from led_classdev_unregister() and force the drivers
> to turn leds off at shutdown. May be add check that led's brightness is 0
> at led_classdev_unregister() and put a warning to dmesg if it's not.
> Actually in many cases it doesn't really need to turn off the leds manually one-by-one
> if driver shutdowns whole led controller. For the last case to disable the warning
> new flag can be brought in e.g LED_AUTO_OFF_AT_SHUTDOWN (similar to LED_RETAIN_AT_SHUTDOWN).
>
> George Stark (8):
> leds: powernv: explicitly unregister LEDs at module's shutdown
> leds: nic78bx: explicitly unregister LEDs at module's shutdown
> leds: an30259a: explicitly unregister LEDs at module's shutdown
> leds: mlxreg: explicitly unregister LEDs at module's shutdown
> leds: aw200xx: explicitly unregister LEDs at module's shutdown
> leds: aw2013: explicitly unregister LEDs at module's shutdown
> leds: lp3952: explicitly unregister LEDs at module's shutdown
> leds: lm3532: explicitly unregister LEDs at module's shutdown
>
> drivers/leds/leds-an30259a.c | 4 ++++
> drivers/leds/leds-aw200xx.c | 4 ++++
> drivers/leds/leds-aw2013.c | 4 ++++
> drivers/leds/leds-lm3532.c | 6 ++++++
> drivers/leds/leds-lp3952.c | 5 +++++
> drivers/leds/leds-mlxreg.c | 12 +++++++++++-
> drivers/leds/leds-nic78bx.c | 4 ++++
> drivers/leds/leds-powernv.c | 7 +++++++
> 8 files changed, 45 insertions(+), 1 deletion(-)
>
On Wed, Oct 25, 2023 at 04:07:29PM +0300, George Stark wrote:
> Lots of drivers use devm_led_classdev_register() to register their led objects
> and let the kernel free those leds at the driver's remove stage.
> It can lead to a problem due to led_classdev_unregister()
> implementation calls led_set_brightness() to turn off the led.
> led_set_brightness() may call one of the module's brightness_set callbacks.
> If that callback uses module's resources allocated without using devm funcs()
> then those resources will be already freed at module's remove() callback and
> we may have use-after-free situation.
>
> Here is an example:
>
> module_probe()
> {
> devm_led_classdev_register(module_brightness_set_cb);
> mutex_init(&mutex);
> }
>
> module_brightness_set_cb()
> {
> mutex_lock(&mutex);
> do_set_brightness();
> mutex_unlock(&mutex);
> }
>
> module_remove()
> {
> mutex_destroy(&mutex);
> }
>
> at rmmod:
> module_remove()
> ->mutex_destroy(&mutex);
> devres_release_all()
> ->led_classdev_unregister();
> ->led_set_brightness();
> ->module_brightness_set_cb();
> ->mutex_lock(&mutex); /* use-after-free */
>
> I think it's an architectural issue and should be discussed thoroughly.
> Some thoughts about fixing it as a start:
> 1) drivers can use devm_led_classdev_unregister() to explicitly free leds before
> dependend resources are freed. devm_led_classdev_register() remains being useful
> to simplify probe implementation.
> As a proof of concept I examined all drivers from drivers/leds and prepared
> patches where it's needed. Sometimes it was not as clean as just calling
> devm_led_classdev_unregister() because several drivers do not track
> their leds object at all - they can call devm_led_classdev_register() and drop the
> returned pointer. In that case I used devres group API.
>
> Drivers outside drivers/leds should be checked too after discussion.
>
> 2) remove led_set_brightness from led_classdev_unregister() and force the drivers
> to turn leds off at shutdown. May be add check that led's brightness is 0
> at led_classdev_unregister() and put a warning to dmesg if it's not.
> Actually in many cases it doesn't really need to turn off the leds manually one-by-one
> if driver shutdowns whole led controller. For the last case to disable the warning
> new flag can be brought in e.g LED_AUTO_OFF_AT_SHUTDOWN (similar to LED_RETAIN_AT_SHUTDOWN).
NAK.
Just fix the drivers by wrapping mutex_destroy() into devm, There are many
doing so. You may be brave enough to introduce devm_mutex_init() somewhere
in include/linux/device*
--
With Best Regards,
Andy Shevchenko
On Sat, Nov 4, 2023 at 9:17 AM George Stark <[email protected]> wrote:
>
> Hello Andy
>
> Could you please take a look at this patch series?
>
> I've just found your post on habr about devres API misusing and I think
> this is just another case.
Just had a look, sorry for the delay.
By quickly reading it seems to be a wrong approach (or wrong end to
start solving the issue from).
--
With Best Regards,
Andy Shevchenko
Hello Andy
Thanks for the review.
On 11/24/23 18:28, Andy Shevchenko wrote:
> On Wed, Oct 25, 2023 at 04:07:29PM +0300, George Stark wrote:
>> Lots of drivers use devm_led_classdev_register() to register their led objects
>> and let the kernel free those leds at the driver's remove stage.
>> It can lead to a problem due to led_classdev_unregister()
>> implementation calls led_set_brightness() to turn off the led.
>> led_set_brightness() may call one of the module's brightness_set callbacks.
>> If that callback uses module's resources allocated without using devm funcs()
>> then those resources will be already freed at module's remove() callback and
>> we may have use-after-free situation.
>>
>> Here is an example:
>>
>> module_probe()
>> {
>> devm_led_classdev_register(module_brightness_set_cb);
>> mutex_init(&mutex);
>> }
>>
>> module_brightness_set_cb()
>> {
>> mutex_lock(&mutex);
>> do_set_brightness();
>> mutex_unlock(&mutex);
>> }
>>
>> module_remove()
>> {
>> mutex_destroy(&mutex);
>> }
>>
>> at rmmod:
>> module_remove()
>> ->mutex_destroy(&mutex);
>> devres_release_all()
>> ->led_classdev_unregister();
>> ->led_set_brightness();
>> ->module_brightness_set_cb();
>> ->mutex_lock(&mutex); /* use-after-free */
>>
>> I think it's an architectural issue and should be discussed thoroughly.
>> Some thoughts about fixing it as a start:
>> 1) drivers can use devm_led_classdev_unregister() to explicitly free leds before
>> dependend resources are freed. devm_led_classdev_register() remains being useful
>> to simplify probe implementation.
>> As a proof of concept I examined all drivers from drivers/leds and prepared
>> patches where it's needed. Sometimes it was not as clean as just calling
>> devm_led_classdev_unregister() because several drivers do not track
>> their leds object at all - they can call devm_led_classdev_register() and drop the
>> returned pointer. In that case I used devres group API.
>>
>> Drivers outside drivers/leds should be checked too after discussion.
>>
>> 2) remove led_set_brightness from led_classdev_unregister() and force the drivers
>> to turn leds off at shutdown. May be add check that led's brightness is 0
>> at led_classdev_unregister() and put a warning to dmesg if it's not.
>> Actually in many cases it doesn't really need to turn off the leds manually one-by-one
>> if driver shutdowns whole led controller. For the last case to disable the warning
>> new flag can be brought in e.g LED_AUTO_OFF_AT_SHUTDOWN (similar to LED_RETAIN_AT_SHUTDOWN).
>
> NAK.
>
> Just fix the drivers by wrapping mutex_destroy() into devm, There are many
> doing so. You may be brave enough to introduce devm_mutex_init() somewhere
> in include/linux/device*
>
Just one thing about mutex_destroy(). It seems like there's no single
opinion on should it be called in 100% cases e.g. in remove() paths.
For example in iio subsystem Jonathan suggests it can be dropped in
simple cases: https://www.spinics.net/lists/linux-iio/msg73423.html
So the question is can we just drop mutex_destroy() in module's remove()
callback here if that mutex is needed for devm subsequent callbacks?
--
Best regards
George
On Sat, 25 Nov 2023 03:47:41 +0300
George Stark <[email protected]> wrote:
> Hello Andy
>
> Thanks for the review.
>
> On 11/24/23 18:28, Andy Shevchenko wrote:
> > On Wed, Oct 25, 2023 at 04:07:29PM +0300, George Stark wrote:
> >> Lots of drivers use devm_led_classdev_register() to register their led objects
> >> and let the kernel free those leds at the driver's remove stage.
> >> It can lead to a problem due to led_classdev_unregister()
> >> implementation calls led_set_brightness() to turn off the led.
> >> led_set_brightness() may call one of the module's brightness_set callbacks.
> >> If that callback uses module's resources allocated without using devm funcs()
> >> then those resources will be already freed at module's remove() callback and
> >> we may have use-after-free situation.
> >>
> >> Here is an example:
> >>
> >> module_probe()
> >> {
> >> devm_led_classdev_register(module_brightness_set_cb);
> >> mutex_init(&mutex);
> >> }
> >>
> >> module_brightness_set_cb()
> >> {
> >> mutex_lock(&mutex);
> >> do_set_brightness();
> >> mutex_unlock(&mutex);
> >> }
> >>
> >> module_remove()
> >> {
> >> mutex_destroy(&mutex);
> >> }
> >>
> >> at rmmod:
> >> module_remove()
> >> ->mutex_destroy(&mutex);
> >> devres_release_all()
> >> ->led_classdev_unregister();
> >> ->led_set_brightness();
> >> ->module_brightness_set_cb();
> >> ->mutex_lock(&mutex); /* use-after-free */
> >>
> >> I think it's an architectural issue and should be discussed thoroughly.
> >> Some thoughts about fixing it as a start:
> >> 1) drivers can use devm_led_classdev_unregister() to explicitly free leds before
> >> dependend resources are freed. devm_led_classdev_register() remains being useful
> >> to simplify probe implementation.
> >> As a proof of concept I examined all drivers from drivers/leds and prepared
> >> patches where it's needed. Sometimes it was not as clean as just calling
> >> devm_led_classdev_unregister() because several drivers do not track
> >> their leds object at all - they can call devm_led_classdev_register() and drop the
> >> returned pointer. In that case I used devres group API.
> >>
> >> Drivers outside drivers/leds should be checked too after discussion.
> >>
> >> 2) remove led_set_brightness from led_classdev_unregister() and force the drivers
> >> to turn leds off at shutdown. May be add check that led's brightness is 0
> >> at led_classdev_unregister() and put a warning to dmesg if it's not.
> >> Actually in many cases it doesn't really need to turn off the leds manually one-by-one
> >> if driver shutdowns whole led controller. For the last case to disable the warning
> >> new flag can be brought in e.g LED_AUTO_OFF_AT_SHUTDOWN (similar to LED_RETAIN_AT_SHUTDOWN).
> >
> > NAK.
> >
> > Just fix the drivers by wrapping mutex_destroy() into devm, There are many
> > doing so. You may be brave enough to introduce devm_mutex_init() somewhere
> > in include/linux/device*
> >
>
> Just one thing about mutex_destroy(). It seems like there's no single
> opinion on should it be called in 100% cases e.g. in remove() paths.
> For example in iio subsystem Jonathan suggests it can be dropped in
> simple cases: https://www.spinics.net/lists/linux-iio/msg73423.html
>
> So the question is can we just drop mutex_destroy() in module's remove()
> callback here if that mutex is needed for devm subsequent callbacks?
I've never considered it remotely critical. The way IIO works means that things
have gone pretty horribly wrong in the core if you managed to access a mutex after
the unwind of devm_iio_device_register() has completed but sure, add a
devm_mutex_init() and I'd happily see that adopted in IIO for consistency
and to avoid answering questions on whether it is necessary to call mutex_destroy()
My arguement has always eben that if line after(ish) a mutex_destroy() is going to
either free the memory it's in, or make it otherwise inaccessible (IIO is proxying
accesses via chardevs if there are open so should ensure they never hit the driver)
then it's pointless and messy to call mutex_destroy(). devm_mutex_init() gets rid
of that mess..
Jonathan
>
On Sat, Nov 25, 2023 at 03:47:41AM +0300, George Stark wrote:
> On 11/24/23 18:28, Andy Shevchenko wrote:
> > On Wed, Oct 25, 2023 at 04:07:29PM +0300, George Stark wrote:
> > > Lots of drivers use devm_led_classdev_register() to register their led objects
> > > and let the kernel free those leds at the driver's remove stage.
> > > It can lead to a problem due to led_classdev_unregister()
> > > implementation calls led_set_brightness() to turn off the led.
> > > led_set_brightness() may call one of the module's brightness_set callbacks.
> > > If that callback uses module's resources allocated without using devm funcs()
> > > then those resources will be already freed at module's remove() callback and
> > > we may have use-after-free situation.
> > >
> > > Here is an example:
> > >
> > > module_probe()
> > > {
> > > devm_led_classdev_register(module_brightness_set_cb);
> > > mutex_init(&mutex);
> > > }
> > >
> > > module_brightness_set_cb()
> > > {
> > > mutex_lock(&mutex);
> > > do_set_brightness();
> > > mutex_unlock(&mutex);
> > > }
> > >
> > > module_remove()
> > > {
> > > mutex_destroy(&mutex);
> > > }
> > >
> > > at rmmod:
> > > module_remove()
> > > ->mutex_destroy(&mutex);
> > > devres_release_all()
> > > ->led_classdev_unregister();
> > > ->led_set_brightness();
> > > ->module_brightness_set_cb();
> > > ->mutex_lock(&mutex); /* use-after-free */
> > >
> > > I think it's an architectural issue and should be discussed thoroughly.
> > > Some thoughts about fixing it as a start:
> > > 1) drivers can use devm_led_classdev_unregister() to explicitly free leds before
> > > dependend resources are freed. devm_led_classdev_register() remains being useful
> > > to simplify probe implementation.
> > > As a proof of concept I examined all drivers from drivers/leds and prepared
> > > patches where it's needed. Sometimes it was not as clean as just calling
> > > devm_led_classdev_unregister() because several drivers do not track
> > > their leds object at all - they can call devm_led_classdev_register() and drop the
> > > returned pointer. In that case I used devres group API.
> > >
> > > Drivers outside drivers/leds should be checked too after discussion.
> > >
> > > 2) remove led_set_brightness from led_classdev_unregister() and force the drivers
> > > to turn leds off at shutdown. May be add check that led's brightness is 0
> > > at led_classdev_unregister() and put a warning to dmesg if it's not.
> > > Actually in many cases it doesn't really need to turn off the leds manually one-by-one
> > > if driver shutdowns whole led controller. For the last case to disable the warning
> > > new flag can be brought in e.g LED_AUTO_OFF_AT_SHUTDOWN (similar to LED_RETAIN_AT_SHUTDOWN).
> >
> > NAK.
> >
> > Just fix the drivers by wrapping mutex_destroy() into devm, There are many
> > doing so. You may be brave enough to introduce devm_mutex_init() somewhere
> > in include/linux/device*
>
> Just one thing about mutex_destroy(). It seems like there's no single
> opinion on should it be called in 100% cases e.g. in remove() paths.
> For example in iio subsystem Jonathan suggests it can be dropped in simple
> cases: https://www.spinics.net/lists/linux-iio/msg73423.html
>
> So the question is can we just drop mutex_destroy() in module's remove()
> callback here if that mutex is needed for devm subsequent callbacks?
mutex_destroy() makes sense when debugging mutexes. It's harmless to drop,
but will make life harder to one who is trying to debug something there...
--
With Best Regards,
Andy Shevchenko