Re: [PATCH] leds: Fix LED_OFF brightness race

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 20-Feb-25 9:46 AM, Remi Pommarel wrote:
> On Wed, Feb 19, 2025 at 12:52:36PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 19-Feb-25 11:41 AM, Remi Pommarel wrote:
>>> While commit fa15d8c69238 ("leds: Fix set_brightness_delayed() race")
>>> successfully forces led_set_brightness() to be called with LED_OFF at
>>> least once when switching from blinking to LED on state so that
>>> hw-blinking can be disabled, another race remains. Indeed in
>>> led_set_brightness(LED_OFF) followed by led_set_brightness(any)
>>> scenario the following CPU scheduling can happen:
>>>
>>>     CPU0                                     CPU1
>>>     ----                                     ----
>>>  set_brightness_delayed() {
>>>    test_and_clear_bit(BRIGHTNESS_OFF)
>>>                                          led_set_brightness(LED_OFF) {
>>>                                            set_bit(BRIGHTNESS_OFF)
>>> 					   queue_work()
>>>                                          }
>>>                                          led_set_brightness(any) {
>>>                                            set_bit(BRIGHTNESS)
>>> 					   queue_work() //already queued
>>>                                          }
>>>    test_and_clear_bit(BRIGHTNESS)
>>>      /* LED set with brightness any */
>>>  }
>>>
>>>  /* From previous CPU1 queue_work() */
>>>  set_brightness_delayed() {
>>>    test_and_clear_bit(BRIGHTNESS_OFF)
>>>      /* LED turned off */
>>>    test_and_clear_bit(BRIGHTNESS)
>>>      /* Clear from previous run, LED remains off */
>>>
>>> In that case the led_set_brightness(LED_OFF)/led_set_brightness(any)
>>> sequence will be effectively executed in reverse order and LED will
>>> remain off.
>>>
>>> With the introduction of commit 32360bf6a5d4 ("leds: Introduce ordered
>>> workqueue for LEDs events instead of system_wq") the race is easier to
>>> trigger as sysfs brightness configuration does not wait for
>>> set_brightness_delayed() work to finish (flush_work() removal).
>>>
>>> Use delayed_set_value to optionnally re-configure brightness after a
>>> LED_OFF. That way a LED state could be configured more that once but
>>> final state will always be as expected.
>>>
>>> Fixes: fa15d8c69238 ("leds: Fix set_brightness_delayed() race")
>>> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
>>
>> Thanks, patch looks good to me:
>>
> 
> Actually two additionnal remarks here. The first one is that now more
> than before, delayed_set_value store should be seen before work_flags
> modification on other CPUs. That means that a smp_mb__before_atomic()
> is needed before the two set_bit().
> 
> The second one is that delayed_set_value can be bigger than a single
> byte, so theoretically store tearing can happen and
> set_brightness_delayed_set_brightness() could be called with an invalid
> value. WRITE_ONCE/READ_ONCE could prevent that but because the
> smp_mb__before_atomic() ensures that the "last" delayed_set_value is
> valid I don't mind having very seldom intermediate invalid values.
> 
> So I think a v2 with smp_mb__before_atomic() is needed here, what do you
> think ?

Doing a v2 with smp_mb__before_atomic() sounds good to me.

Regards,

Hans