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
