[PATCH] staging: greybus: light: initialize channel lock before registration

[PATCH] staging: greybus: light: initialize channel lock before registration

[Runyu Xiao]

gb_lights_light_register() registers each channel before initializing
light->channels[i].lock. Channel registration exposes the LED class
device and its brightness callback, which later takes this mutex. A
concurrent brightness update can therefore hit an uninitialized lock.

Initialize the channel mutex before registering the channel so the
callback always sees a valid lock.

Fixes: cc43368a3cde ("greybus: lights: Control runtime pm suspend/resume on AP side")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/staging/greybus/light.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
index 38c233a706c4..056ef6b0276e 100644
--- a/drivers/staging/greybus/light.c
+++ b/drivers/staging/greybus/light.c
@@ -1043,11 +1043,11 @@ static int gb_lights_light_register(struct gb_light *light)
 	 * found.
 	 */
 	for (i = 0; i < light->channels_count; i++) {
+		mutex_init(&light->channels[i].lock);
+
 		ret = gb_lights_channel_register(&light->channels[i]);
 		if (ret < 0)
 			return ret;
-
-		mutex_init(&light->channels[i].lock);
 	}
 
 	light->ready = true;
-- 
2.34.1

Re: [PATCH] staging: greybus: light: initialize channel lock before registration

[Greg Kroah-Hartman]

On Tue, Aug 18, 2026 at 10:20:20PM +0800, Runyu Xiao wrote:
> gb_lights_light_register() registers each channel before initializing
> light->channels[i].lock. Channel registration exposes the LED class
> device and its brightness callback, which later takes this mutex. A
> concurrent brightness update can therefore hit an uninitialized lock.
> 
> Initialize the channel mutex before registering the channel so the
> callback always sees a valid lock.
> 
> Fixes: cc43368a3cde ("greybus: lights: Control runtime pm suspend/resume on AP side")
> Cc: stable@vger.kernel.org
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
> ---
>  drivers/staging/greybus/light.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index 38c233a706c4..056ef6b0276e 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -1043,11 +1043,11 @@ static int gb_lights_light_register(struct gb_light *light)
>  	 * found.
>  	 */
>  	for (i = 0; i < light->channels_count; i++) {
> +		mutex_init(&light->channels[i].lock);
> +
>  		ret = gb_lights_channel_register(&light->channels[i]);
>  		if (ret < 0)
>  			return ret;
> -
> -		mutex_init(&light->channels[i].lock);
>  	}
>  
>  	light->ready = true;

How was this found and tested?

thanks,

greg k-h

Re: [PATCH] staging: greybus: light: initialize channel lock before registration

[Greg Kroah-Hartman]

On Wed, Aug 19, 2026 at 08:32:45PM +0800, Runyu Xiao wrote:
> On Tue, Aug 18, 2026 at 20:23:10 +0200, Greg Kroah-Hartman wrote:
> > How was this found and tested?
> 
> 
> Found by code inspection: gb_lights_channel_register() publishes the
> LED class device and its brightness callback before
> mutex_init(&light->channels[i].lock) runs, so a concurrent brightness
> update can take an uninitialized mutex.

<snip>

Don't send html email with tracking images in it to a public mailing
list, they are rejected and generally considered bad-form.

greg k-h

Re: [PATCH] staging: greybus: light: initialize channel lock before registration

[Greg Kroah-Hartman]

On Wed, Aug 19, 2026 at 08:32:45PM +0800, Runyu Xiao wrote:
> On Tue, Aug 18, 2026 at 20:23:10 +0200, Greg Kroah-Hartman wrote:
> > How was this found and tested?
> 
> 
> Found by code inspection: gb_lights_channel_register() publishes the
> LED class device and its brightness callback before
> mutex_init(&light->channels[i].lock) runs, so a concurrent brightness
> update can take an uninitialized mutex.

What tool did this inspection?  A LLM?

> Tested by reproducing that code shape in a minimal out-of-tree module
> under QEMU (kernel 6.1.66 with CONFIG_DEBUG_MUTEXES=y and
> CONFIG_DEBUG_LOCK_ALLOC=y), since the real driver needs greybus/LED
> hardware I don't have. The module kzalloc's a channel, registers
> (publishes) it, and runs the brightness callback, which takes the
> embedded mutex:
> 
> 
>  - before the fix, mutex_init() runs after register, so the callback
>    locks the still-zeroed mutex and trips
> 
> 
>        DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>        WARNING: CPU: 1 PID: 188 at kernel/locking/mutex.c:582 __mutex_lock+0x712/0xd20
> 
> 
>  - after the fix, mutex_init() runs before register and the same
>    trigger path produces no warning.

But that isn't testing anything at all.

And why 6.1.66?  That's not what new patches need to be made against, as
it's years old.

> The real driver hits this as a race (a concurrent brightness update),
> whereas the reproducer runs the callback synchronously during registration
> to make it deterministic; both leave the callback observing an
> uninitialized lock.

What "reproducer"?  Again, was this done by a LLM?  Where is the
"reproducer"?

thanks,

greg k-h

Re: [PATCH] staging: greybus: light: initialize channel lock before registration

[Runyu Xiao]

You're right, and "tested" was the wrong word in my reply.

The finding here is from code inspection, not from a runtime test. The
path I checked is:

  gb_lights_channel_register()
    -> led_classdev_register()
       publishes cdev->brightness_set_blocking
       (= gb_brightness_set()
          -> __gb_lights_brightness_set()
          -> __gb_lights_led_brightness_set()
          -> mutex_lock(&channel->lock))

but mutex_init(&channel->lock) only runs after
gb_lights_channel_register() returns, and that is the only place where
channel->lock is initialized. So the lock is published before it is
initialized.

The QEMU module I mentioned does not exercise this driver. It only shows
that taking an uninitialized mutex triggers the expected
DEBUG_LOCKS_WARN_ON(lock->magic != lock), so I should not have described
it as driver testing or as a reproducer for this driver.

And yes, 6.1.66 was only the kernel version in that local QEMU setup. The
patch itself was made and compile-checked against current mainline, and
I'll keep validation against mainline for future submissions.

The initial report came from PatchProof, a static-analysis pipeline I am
building for lock-API misuse. It flagged this publish-before-init ordering,
and I then manually verified the call path above. I did use an LLM as an
assistant during the workflow, but the finding itself was from the checker
and the patch I sent was based on my manual review of the code.

Thanks,
Runyu Xiao

Re: [PATCH] staging: greybus: light: initialize channel lock before registration

[Greg Kroah-Hartman]

On Wed, Aug 19, 2026 at 09:21:08PM +0800, Runyu Xiao wrote:
> You're right, and "tested" was the wrong word in my reply.

Sorry, I have no context :(

Remember, some of us get thousands of emails a day.

> The finding here is from code inspection, not from a runtime test. The
> path I checked is:
> 
>   gb_lights_channel_register()
>     -> led_classdev_register()
>        publishes cdev->brightness_set_blocking
>        (= gb_brightness_set()
>           -> __gb_lights_brightness_set()
>           -> __gb_lights_led_brightness_set()
>           -> mutex_lock(&channel->lock))
> 
> but mutex_init(&channel->lock) only runs after
> gb_lights_channel_register() returns, and that is the only place where
> channel->lock is initialized. So the lock is published before it is
> initialized.
> 
> The QEMU module I mentioned does not exercise this driver. It only shows
> that taking an uninitialized mutex triggers the expected
> DEBUG_LOCKS_WARN_ON(lock->magic != lock), so I should not have described
> it as driver testing or as a reproducer for this driver.
> 
> And yes, 6.1.66 was only the kernel version in that local QEMU setup. The
> patch itself was made and compile-checked against current mainline, and
> I'll keep validation against mainline for future submissions.

Just build testing against a many-year-old kernel is a sure way to
ensure that nothing will actually work at all :(

> The initial report came from PatchProof, a static-analysis pipeline I am
> building for lock-API misuse. It flagged this publish-before-init ordering,
> and I then manually verified the call path above. I did use an LLM as an
> assistant during the workflow, but the finding itself was from the checker
> and the patch I sent was based on my manual review of the code.

Please read the archives for why we don't take LLM patches for
drivers/staging/ unless you can test the change on real hardware.

thanks,

greg k-h

Re: [PATCH] staging: greybus: light: initialize channel lock before registration

[Runyu Xiao]

On Wed, Aug 19, 2026 at 09:29:11PM +0800, Greg Kroah-Hartman wrote:
> Please read the archives for why we don't take LLM patches for
> drivers/staging/ unless you can test the change on real hardware.

Understood — I'll drop this patch, and only submit changes I can build
and test against current mainline (and on real hardware where possible)
going forward. Thanks for the guidance.

Thanks,
Runyu Xiao