On Sat, 25 Nov 2023 03:47:41 +0300
George Stark [off-list ref] wrote:

Hello Andy

Thanks for the review.

On 11/24/23 18:28, Andy Shevchenko wrote:

quoted

On Wed, Oct 25, 2023 at 04:07:29PM +0300, George Stark wrote:  

quoted

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