Re: [PATCH] leds: core: Disable hardware control when writing brightness

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] TOCTOU race in brightness_store() can cause unintended removal of a newly registered trigger.
--

--- Patch [1]: [PATCH] leds: core: Disable hardware control when writing brightness ---
commit 86a852e7c49262f7479b1653712185d4edd86c24
Author: Rong Zhang <[email protected]>

leds: core: Disable hardware control when writing brightness

This commit modifies the brightness sysfs attribute to disable hardware control
when written. It addresses an issue where the brightness attribute becomes
confusingly write-only while hardware control remains active.

> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index a51b0ed538864..3e25e5fb63dc8 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -72,7 +72,7 @@ static ssize_t brightness_store(struct device *dev,
>  	if (ret)
>  		goto unlock;
>  
> -	if (state == LED_OFF)
> +	if (state == LED_OFF || led_trigger_is_hw_controlled(led_cdev))
>  		led_trigger_remove(led_cdev);

[Severity: Medium]
Could this introduce a race condition that removes an unrelated trigger?

led_trigger_is_hw_controlled() takes and drops the read-side of trigger_lock:

drivers/leds/led-class.c:led_trigger_is_hw_controlled() {
	...
	guard(rwsem_read)(&led_cdev->trigger_lock);
	return led_cdev->trigger && led_cdev->trigger->trigger_type;
}

Since the lock is dropped before led_trigger_remove() acquires the write-side
lock, is it possible for a concurrent thread to unregister the hardware
trigger and attach a new default trigger in the window between the check and
the removal? 

If that sequence occurs, led_trigger_remove() might inadvertently remove the
newly attached trigger instead of the intended hardware trigger.

>  	led_set_brightness(led_cdev, state);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-leds-hw-control-brightness-set-v1-1-1de593b09d26@rong.moe?part=1