public inbox for stable@vger.kernel.org
 help / color / mirror / Atom feed
From: Lee Jones <lee@kernel.org>
To: Hans de Goede <johannes.goede@oss.qualcomm.com>
Cc: Sebastian Reichel <sre@kernel.org>,
	Pavel Machek <pavel@kernel.org>,
	linux-leds@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] leds: led-class: Only Add LED to leds_list when it is fully ready
Date: Thu, 22 Jan 2026 11:49:25 +0000	[thread overview]
Message-ID: <20260122114925.GF3831112@google.com> (raw)
In-Reply-To: <20260109093504.GA1118061@google.com>

On Fri, 09 Jan 2026, Lee Jones wrote:

> On Thu, 08 Jan 2026, Hans de Goede wrote:
> 
> > 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 .
> 
> Noted.  Leave it with me.

https://lore.kernel.org/all/20260122114749.GE3831112@google.com/T/#u

-- 
Lee Jones [李琼斯]

  reply	other threads:[~2026-01-22 11:49 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-12-11 16:37 [PATCH] leds: led-class: Only Add LED to leds_list when it is fully ready Hans de Goede
2025-12-12  6:49 ` Sebastian Reichel
2025-12-12  9:05   ` Hans de Goede
2026-01-08 12:11     ` Lee Jones
2026-01-08 12:17       ` Hans de Goede
2026-01-09  9:35         ` Lee Jones
2026-01-22 11:49           ` Lee Jones [this message]
2026-01-08 12:14 ` (subset) " Lee Jones

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260122114925.GF3831112@google.com \
    --to=lee@kernel.org \
    --cc=johannes.goede@oss.qualcomm.com \
    --cc=linux-leds@vger.kernel.org \
    --cc=pavel@kernel.org \
    --cc=sre@kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox