Re: [PATCH] leds: led-class: Only Add LED to leds_list when it is fully ready

[Hans de Goede <johannes.goede@oss.qualcomm.com>]

Hi Lee,

On 8-Jan-26 13:11, Lee Jones wrote:
> On Fri, 12 Dec 2025, Hans de Goede wrote:
> 
>> Hi,
>>
>> On 12-Dec-25 07:49, Sebastian Reichel wrote:
>>> Hi,
>>>
>>> On Thu, Dec 11, 2025 at 05:37:27PM +0100, Hans de Goede wrote:
>>>> Before this change the LED was added to leds_list before led_init_core()
>>>> gets called adding it the list before led_classdev.set_brightness_work gets
>>>> initialized.
>>>>
>>>> This leaves a window where led_trigger_register() of a LED's default
>>>> trigger will call led_trigger_set() which calls led_set_brightness()
>>>> which in turn will end up queueing the *uninitialized*
>>>> led_classdev.set_brightness_work.
>>>>
>>>> This race gets hit by the lenovo-thinkpad-t14s EC driver which registers
>>>> 2 LEDs with a default trigger provided by snd_ctl_led.ko in quick
>>>> succession. The first led_classdev_register() causes an async modprobe of
>>>> snd_ctl_led to run and that async modprobe manages to exactly hit
>>>> the window where the second LED is on the leds_list without led_init_core()
>>>> being called for it, resulting in:
>>>>
>>>>  ------------[ cut here ]------------
>>>>  WARNING: CPU: 11 PID: 5608 at kernel/workqueue.c:4234 __flush_work+0x344/0x390
>>>>  Hardware name: LENOVO 21N2S01F0B/21N2S01F0B, BIOS N42ET93W (2.23 ) 09/01/2025
>>>>  ...
>>>>  Call trace:
>>>>   __flush_work+0x344/0x390 (P)
>>>>   flush_work+0x2c/0x50
>>>>   led_trigger_set+0x1c8/0x340
>>>>   led_trigger_register+0x17c/0x1c0
>>>>   led_trigger_register_simple+0x84/0xe8
>>>>   snd_ctl_led_init+0x40/0xf88 [snd_ctl_led]
>>>>   do_one_initcall+0x5c/0x318
>>>>   do_init_module+0x9c/0x2b8
>>>>   load_module+0x7e0/0x998
>>>>
>>>> Close the race window by moving the adding of the LED to leds_list to
>>>> after the led_init_core() call.
>>>>
>>>> Cc: Sebastian Reichel <sre@kernel.org>
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
>>>> ---
>>>
>>> heh, I've never hit this. But I guess that is not too surprising
>>> considering it is a race condition. The change looks good to me:
>>>
>>> Reviewed-by: Sebastian Reichel <sre@kernel.org>
>>
>> Thx.
>>  
>>>> Note no Fixes tag as this problem has been around for a long long time,
>>>> so I could not really find a good commit for the Fixes tag.
>>>
>>> My suggestion would be:
>>>
>>> Fixes: d23a22a74fde ("leds: delay led_set_brightness if stopping soft-blink")
>>
>> Ack, that works for me.
>>
>> Lee can you add this Fixes tag while merging ?
>>
>> Also (in case it is not obvious) this is a bugfix so it would be
>> nice if this could go in a fixes pull-request for 6.19.
> 
> Yes, I can add the Fixes: tag and no, I have no plans to send this for
> -fixes.  As you rightly mentioned, this issue has been around for a long
> time already.  I tend to only send -fixes pull-requests for things that
> broke in -rc1 of the same release.

Even though this has been around for a long time, it would be good
to get this in as a fix for 6.19-rc# because as described in the commit
msg the lenovo-thinkpad-t14s embedded-controller driver, which is new in
6.19-rc1 manages to reliably trigger the race (for me, with a Fedora
kernel distconfig).

I was surprised I could hit the race pretty reliably, but it did make
debugging this easier.

Hitting the race also leads to a crash due to a NULL ptr deref after
the WARN(). I did not elaborate on this in the commit msg, because
the WARN() is the first sign of trying to use uninitialized mem.

IMHO having a reproducable race which causes a crash is
a good reason to submit this as a fix for 6.19 .

Regards,

Hans