Re: [PATCH AUTOSEL 6.18] leds: core: Fix race condition for software blink

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing `smp_mb__before_atomic()` before `set_bit()` allows the CPU or compiler to reorder the memory write, leading to lost brightness updates.
- [High] The newly added early-return path bypasses max_brightness clamping and state updates, exposing hardware drivers to arbitrary values and causing software state inconsistency.

Pre-existing issues:
- [Critical] A race condition during LED device unregistration can lead to a Use-After-Free of the `led_classdev` structure because background tasks are flushed before the sysfs interface is removed.
- [High] Incomplete fix for software blink race condition leaves a TOCTOU window where brightness changes are permanently lost during `led_stop_software_blink()`.
--

commit 27c103cf78e9f98bb6d9bdb9ae011c1befedc295
Author: Craig McQueen <craig@mcqueen.au>

leds: core: Fix race condition for software blink

led_set_brightness() function: Change handling of software blink to
avoid race conditions when stopping blink and setting brightness.

Triggers may call led_set_brightness(LED_OFF),
led_set_brightness(LED_FULL) in quick succession to disable blinking and
turn the LED on. If the delayed work task has not yet disabled blinking
by the time the second call occurs, then the brightness also needs to be
changed in the delayed work task.

Link: https://patch.msgid.link/20260423113638.2079302-1-craig@mcqueen.au

> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 59473f286b31f..eeb36e50c285c 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -304,24 +304,31 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);
>  
>  void led_set_brightness(struct led_classdev *led_cdev, unsigned int brightness)
>  {
> -	/*
> -	 * If software blink is active, delay brightness setting
> -	 * until the next timer tick.
> -	 */
> -	if (test_bit(LED_BLINK_SW, &led_cdev->work_flags)) {
> +	if (brightness) {
>  		/*
> -		 * If we need to disable soft blinking delegate this to the
> -		 * work queue task to avoid problems in case we are called
> -		 * from hard irq context.
> +		 * If software blink disable is pending, also queue brightness setting.
> +		 * If software blink is active, delay brightness setting
> +		 * until the next timer tick.
>  		 */
> -		if (!brightness) {
> -			set_bit(LED_BLINK_DISABLE, &led_cdev->work_flags);
> +		if (test_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags) ||
> +		    test_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) {
> +			led_cdev->delayed_set_value = brightness;
> +			set_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);

[Severity: High]
Is it possible for the CPU or compiler to reorder this non-atomic write
and the atomic set_bit() operation?

If a worker thread executes test_and_clear_bit(LED_SET_BRIGHTNESS) after
the bit is set but before the data store to delayed_set_value is committed,
could it read a stale delayed_set_value? A subsequent execution of the
worker triggered by the queue_work() would then see the bit cleared and do
nothing, permanently discarding the brightness update.

Would it be better to add an smp_mb__before_atomic() before setting the bit?

[Severity: High]
Does this newly added early return path bypass the clamping of the brightness
value?

By directly storing the unvalidated brightness into delayed_set_value and
returning, this seems to bypass the min(value, led_cdev->max_brightness) check
normally performed in led_set_brightness_nosleep().

If the background worker passes this unvalidated delayed_set_value directly
into the hardware driver's brightness_set callback, could this expose backend
hardware drivers to out-of-bounds values up to ULONG_MAX?

>  			queue_work(led_cdev->wq, &led_cdev->set_brightness_work);
> -		} else {
> -			set_bit(LED_BLINK_BRIGHTNESS_CHANGE,
> -				&led_cdev->work_flags);
> +			return;
> +		} else if (test_bit(LED_BLINK_SW, &led_cdev->work_flags)) {
>  			led_cdev->new_blink_brightness = brightness;
> +			set_bit(LED_BLINK_BRIGHTNESS_CHANGE, &led_cdev->work_flags);

[Severity: High]
Would this also require an smp_mb__before_atomic() before setting the bit
for the same reordering reasons?

[Severity: High]
This is a pre-existing issue, but does this incomplete fix leave a TOCTOU
window where brightness changes can be permanently lost?

In set_brightness_delayed(), if test_and_clear_bit() clears LED_BLINK_DISABLE
before led_stop_software_blink() completes and clears LED_BLINK_SW, could
a concurrent led_set_brightness(LED_FULL) see LED_BLINK_DISABLE as cleared
but LED_BLINK_SW as still set?

If so, it would fall into this branch, recording the new brightness into
new_blink_brightness and setting LED_BLINK_BRIGHTNESS_CHANGE. But since the
timer is actively being deleted by the worker, it will never fire again,
permanently losing the brightness request.

> +			return;
>  		}

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free race condition
during LED device unregistration?

Looking at led_classdev_unregister() in drivers/leds/led-class.c, it appears
flush_work(&led_cdev->set_brightness_work) is called before
device_unregister(led_cdev->dev).

Because device_unregister() is what removes the sysfs attributes, the
brightness sysfs interface remains fully active during and after flush_work().
Could a concurrent sysfs write execute led_set_brightness() and queue the
worker again on the global leds_wq?

When device_unregister() completes and led_cdev is freed, the previously
queued work would execute and access the freed memory. Should this use
cancel_work_sync() after the sysfs node has been fully removed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-161-sashal@kernel.org?part=1