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

Hi all,

On Fri, 2026-08-21 at 17:27 +0000, [email protected] wrote:
> 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.

Fixing them would need a huge series and is far beyond the scope of the
series.

Thank,
Rong

> --
> 
> --- 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.