Re: [PATCH] leds: Fix LED_OFF brightness race

[Remi Pommarel <repk@triplefau.lt>]

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 ?

Regards,

-- 
Remi