[PATCH 0/8] devm_led_classdev_register() usage problem

[PATCH 0/8] devm_led_classdev_register() usage problem

[George Stark]

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

[PATCH 1/8] leds: powernv: explicitly unregister LEDs at module's shutdown

[George Stark]

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 <gnstark@salutedevices.com>
---
 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

[PATCH 2/8] leds: nic78bx: explicitly unregister LEDs at module's shutdown

[George Stark]

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 <gnstark@salutedevices.com>
---
 drivers/leds/leds-nic78bx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/leds/leds-nic78bx.c b/drivers/leds/leds-nic78bx.c
index f196f52eec1e..12b70fcad37f 100644
--- a/drivers/leds/leds-nic78bx.c
+++ b/drivers/leds/leds-nic78bx.c
@@ -170,6 +170,10 @@ static int nic78bx_probe(struct platform_device *pdev)
 static int nic78bx_remove(struct platform_device *pdev)
 {
 	struct nic78bx_led_data *led_data = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(nic78bx_leds); i++)
+		devm_led_classdev_unregister(&pdev->dev, &nic78bx_leds[i].cdev);
 
 	/* Lock LED register */
 	outb(NIC78BX_LOCK_VALUE,
-- 
2.38.4

Re: [PATCH 2/8] leds: nic78bx: explicitly unregister LEDs at module's shutdown

[Christophe Leroy]

Le 25/10/2023 à 15:07, George Stark a écrit :
> 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 <gnstark@salutedevices.com>
> ---
>   drivers/leds/leds-nic78bx.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/leds/leds-nic78bx.c b/drivers/leds/leds-nic78bx.c
> index f196f52eec1e..12b70fcad37f 100644
> --- a/drivers/leds/leds-nic78bx.c
> +++ b/drivers/leds/leds-nic78bx.c
> @@ -170,6 +170,10 @@ static int nic78bx_probe(struct platform_device *pdev)
>   static int nic78bx_remove(struct platform_device *pdev)
>   {
>   	struct nic78bx_led_data *led_data = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(nic78bx_leds); i++)
> +		devm_led_classdev_unregister(&pdev->dev, &nic78bx_leds[i].cdev);

The whole purpose of devm_ functions is that you don't need to call 
unregister when removing the driver as the dev core will do it for you. 
I understand your problem but I think this is not the solution.

>   
>   	/* Lock LED register */
>   	outb(NIC78BX_LOCK_VALUE,

Re: [PATCH 2/8] leds: nic78bx: explicitly unregister LEDs at module's shutdown

[George Stark]

Hello Christophe.

Thanks for the review.


On 11/6/23 11:13, Christophe Leroy wrote:
 >
 >
 > Le 25/10/2023 à 15:07, George Stark a écrit :
 >> 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 <gnstark@salutedevices.com>
 >> ---
 >>    drivers/leds/leds-nic78bx.c | 4 ++++
 >>    1 file changed, 4 insertions(+)
 >>
 >> diff --git a/drivers/leds/leds-nic78bx.c b/drivers/leds/leds-nic78bx.c
 >> index f196f52eec1e..12b70fcad37f 100644
 >> --- a/drivers/leds/leds-nic78bx.c
 >> +++ b/drivers/leds/leds-nic78bx.c
 >> @@ -170,6 +170,10 @@ static int nic78bx_probe(struct platform_device 
*pdev)
 >>    static int nic78bx_remove(struct platform_device *pdev)
 >>    {
 >>    	struct nic78bx_led_data *led_data = platform_get_drvdata(pdev);
 >> +	int i;
 >> +
 >> +	for (i = 0; i < ARRAY_SIZE(nic78bx_leds); i++)
 >> +		devm_led_classdev_unregister(&pdev->dev, &nic78bx_leds[i].cdev);
 >
 > The whole purpose of devm_ functions is that you don't need to call
 > unregister when removing the driver as the dev core will do it for you.
 > I understand your problem but I think this is not the solution.

I agree my solution is questionable although 
devm_led_classdev_unregister() is exists for some reason.

Probably it's not the best solution to remove led_set_brightness() from 
led_classdev_unregister() either.
Or we'll have to patch a lot of drivers which use led subsystem to call 
led_set_brightness() manually to keep leds' previous behavior.

Well if we can't easily unregister leds before module's remove() 
callback is completed may be we can get rid of remove() itself and 
manage all resources using devm API. In that case by the time 
led_set_brightness() is called from led_classdev_unregister() all 
dependent resources will be alive.
I'll try it in the next patch series.

 >
 >>
 >>    	/* Lock LED register */
 >>    	outb(NIC78BX_LOCK_VALUE,
-- 
Best regards
George

[PATCH 3/8] leds: an30259a: explicitly unregister LEDs at module's shutdown

[George Stark]

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 <gnstark@salutedevices.com>
---
 drivers/leds/leds-an30259a.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/leds/leds-an30259a.c b/drivers/leds/leds-an30259a.c
index 24b1041213c2..4209a407d802 100644
--- a/drivers/leds/leds-an30259a.c
+++ b/drivers/leds/leds-an30259a.c
@@ -324,6 +324,10 @@ static int an30259a_probe(struct i2c_client *client)
 static void an30259a_remove(struct i2c_client *client)
 {
 	struct an30259a *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);
 
 	mutex_destroy(&chip->mutex);
 }
-- 
2.38.4

[PATCH 4/8] leds: mlxreg: explicitly unregister LEDs at module's shutdown

[George Stark]

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 <gnstark@salutedevices.com>
---
 drivers/leds/leds-mlxreg.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-mlxreg.c b/drivers/leds/leds-mlxreg.c
index b7855c93bd72..6d65e39c3372 100644
--- a/drivers/leds/leds-mlxreg.c
+++ b/drivers/leds/leds-mlxreg.c
@@ -258,6 +258,7 @@ static int mlxreg_led_probe(struct platform_device *pdev)
 {
 	struct mlxreg_core_platform_data *led_pdata;
 	struct mlxreg_led_priv_data *priv;
+	int res;
 
 	led_pdata = dev_get_platdata(&pdev->dev);
 	if (!led_pdata) {
@@ -273,13 +274,22 @@ static int mlxreg_led_probe(struct platform_device *pdev)
 	priv->pdev = pdev;
 	priv->pdata = led_pdata;
 
-	return mlxreg_led_config(priv);
+	if (!devres_open_group(&pdev->dev, priv, GFP_KERNEL))
+		return -ENOMEM;
+
+	res = mlxreg_led_config(priv);
+	if (res)
+		devres_remove_group(&pdev->dev, priv);
+
+	return res;
 }
 
 static int mlxreg_led_remove(struct platform_device *pdev)
 {
 	struct mlxreg_led_priv_data *priv = dev_get_drvdata(&pdev->dev);
 
+	devres_release_group(&pdev->dev, priv);
+
 	mutex_destroy(&priv->access_lock);
 
 	return 0;
-- 
2.38.4

[PATCH 5/8] leds: aw200xx: explicitly unregister LEDs at module's shutdown

[George Stark]

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 <gnstark@salutedevices.com>
---
 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

[PATCH 6/8] leds: aw2013: explicitly unregister LEDs at module's shutdown

[George Stark]

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 <gnstark@salutedevices.com>
---
 drivers/leds/leds-aw2013.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 59765640b70f..bd233500aa2d 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -404,6 +404,10 @@ static int aw2013_probe(struct i2c_client *client)
 static void aw2013_remove(struct i2c_client *client)
 {
 	struct aw2013 *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);
 
 	aw2013_chip_disable(chip);
 
-- 
2.38.4

[PATCH 7/8] leds: lp3952: explicitly unregister LEDs at module's shutdown

[George Stark]

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 <gnstark@salutedevices.com>
---
 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

Re: [PATCH 0/8] devm_led_classdev_register() usage problem

[George Stark]

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

Re: [PATCH 0/8] devm_led_classdev_register() usage problem

[Andy Shevchenko]

On Sat, Nov 4, 2023 at 9:17 AM George Stark <gnstark@salutedevices.com> 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

Re: [PATCH 0/8] devm_led_classdev_register() usage problem

[Christophe Leroy]

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

Re: [PATCH 0/8] devm_led_classdev_register() usage problem

[Andy Shevchenko]

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

Re: [PATCH 0/8] devm_led_classdev_register() usage problem

[George Stark]

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

Re: [PATCH 0/8] devm_led_classdev_register() usage problem

[Jonathan Cameron]

On Sat, 25 Nov 2023 03:47:41 +0300
George Stark <gnstark@salutedevices.com> 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


> 

Re: [PATCH 0/8] devm_led_classdev_register() usage problem

[Andy Shevchenko]

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