Re: [PATCH v5 08/12] leds: trigger: Do not attach trigger to a removing LED

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch attempts to fix a data race on `led_cdev->flags` by wrapping the `LED_UNREGISTERING` update in `trigger_lock`. However, this is an incomplete fix because `flags` is an `int` modified via non-atomic bitwise operations (`|=`, `&= ~`), and other modifiers (such as `led_classdev_suspend` and `led_sysfs_disable`) do not hold `trigger_lock`. A concurrent update from these paths will still cause a data race that can drop the `LED_UNREGISTERING` flag, exposing the same Use-After-Free bug.
- [High] `led_classdev_unregister()` flushes the brightness workqueue before unregistering the sysfs device, creating a race condition where concurrent sysfs writes can queue new work that executes after the LED is freed.
--

--- Patch [8]: [PATCH v5 08/12] leds: trigger: Do not attach trigger to a removing LED ---
commit 9ab2868504159e735891fdf50fc3354c0c88c1c8
Author: Rong Zhang <[email protected]>

leds: trigger: Do not attach trigger to a removing LED

This patch attempts to fix a race condition where a trigger may be attached
to a removing LED by setting the LED_UNREGISTERING flag under the
trigger_lock in led_classdev_unregister().

> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 2460fcf0c4690..771a6e6c659be 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -633,12 +633,13 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
>  
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	down_write(&led_cdev->trigger_lock);
> +	led_cdev->flags |= LED_UNREGISTERING;

[Severity: High]
This is a pre-existing issue, but does this fully resolve the data race on
the flags bitmask?

While this update is now protected by trigger_lock, other paths like PM
suspend or V4L2 flash release modify led_cdev->flags using non-atomic
bitwise operations without holding trigger_lock.

If a concurrent update occurs without the lock, could the non-atomic
read-modify-write drop the LED_UNREGISTERING flag?

If the flag is lost, a subsequent sysfs trigger write might see it unset
and attach a trigger to the unregistering LED, leading to a use-after-free
when the trigger later fires.

[ ... ]

>  	/* Stop blinking */
>  	led_stop_software_blink(led_cdev);
> 
> 	if (!(led_cdev->flags & LED_RETAIN_AT_SHUTDOWN))
> 		led_set_brightness(led_cdev, LED_OFF);
> 
> 	flush_work(&led_cdev->set_brightness_work);

[Severity: High]
This is also a pre-existing issue, but is there a race condition between
flushing the workqueue and unregistering the device?

In led_classdev_unregister(), flush_work() is called before
device_unregister() removes the device from sysfs.

Could a user-space thread writing to the sysfs brightness attribute in the
window between these two calls queue new work?

sysfs brightness_store() -> led_set_brightness_nopm() -> queue_work()

Since device_unregister() hasn't been called yet, the sysfs write can
succeed, leaving pending work that will execute after the device memory is
freed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8