[PATCH] ALSA: control: Synchronize LED mode accesses

[PATCH] ALSA: control: Synchronize LED mode accesses

[Sreeraj S Kurup]

mode_store() updates led->mode while holding
snd_ctl_led_mutex.

However, snd_ctl_led_set_state() and mode_show() read this
field without holding the same mutex.

This can cause a data race when userspace changes the mode
while another path reads it.

Read the mode under snd_ctl_led_mutex in
snd_ctl_led_set_state() and use the local value after
releasing the mutex.

Also protect the mode read in mode_show() with the mutex.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 sound/core/control_led.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index 501387287ae6..044430b2f1a2 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -140,6 +140,7 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
 {
 	struct snd_ctl_led *led;
 	struct snd_ctl_led_ctl *lctl;
+	enum snd_ctl_led_mode mode;
 	int route;
 	bool found;
 
@@ -168,8 +169,9 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
 				UPDATE_ROUTE(route, snd_ctl_led_get(lctl));
 			}
 		}
+		mode = led->mode;
 	}
-	switch (led->mode) {
+	switch (mode) {
 	case MODE_OFF:		route = 1; break;
 	case MODE_ON:		route = 0; break;
 	case MODE_FOLLOW_ROUTE:	if (route >= 0) route ^= 1; break;
@@ -382,6 +384,8 @@ static ssize_t mode_show(struct device *dev,
 	struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev);
 	const char *str = NULL;
 
+	guard(mutex)(&snd_ctl_led_mutex);
+
 	switch (led->mode) {
 	case MODE_FOLLOW_MUTE:	str = "follow-mute"; break;
 	case MODE_FOLLOW_ROUTE:	str = "follow-route"; break;
-- 
2.55.0

Re: [PATCH] ALSA: control: Synchronize LED mode accesses

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] ALSA: control: missing Fixes trailer for led->mode data race fix
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] ALSA: control_led: data race in snd_ctl_led_reset() [1]
--

commit 2868f72034945ef393b8a3a1a9c6b9d772455395
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

> ALSA: control: Synchronize LED mode accesses
>
> mode_store() updates led->mode while holding
> snd_ctl_led_mutex.
>
> However, snd_ctl_led_set_state() and mode_show() read this
> field without holding the same mutex.
>
> This can cause a data race when userspace changes the mode
> while another path reads it.
>
> Read the mode under snd_ctl_led_mutex in
> snd_ctl_led_set_state() and use the local value after
> releasing the mutex.
>
> Also protect the mode read in mode_show() with the mutex.
>
> Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

[Severity: Low]
Since this fixes the unlocked reads of led->mode introduced when sysfs
support was added, should this include a Fixes tag?

  Fixes: cb17fe0045aa ("ALSA: control - add sysfs support to the LED trigger module")

> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index 501387287ae69..044430b2f1a22 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c

[ ... ]

> @@ -382,6 +384,8 @@ static ssize_t mode_show(struct device *dev,
>  	struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev);
>  	const char *str = NULL;
>  
> +	guard(mutex)(&snd_ctl_led_mutex);

[Severity: Medium]
This problem wasn't introduced by this patch, but can snd_ctl_led_reset()
and snd_ctl_led_set_id() race on vd->access [1]?

Concurrent sysfs writes to the reset and attach or detach attributes
modify vd->access under different locks: snd_ctl_led_reset() holds only
snd_ctl_led_mutex, whereas snd_ctl_led_set_id() holds only
card->controls_rwsem.

CPU0                                  CPU1
snd_ctl_led_reset()                   snd_ctl_led_set_id()
  scoped_guard(mutex,
               &snd_ctl_led_mutex)
                                        guard(rwsem_write)(
                                          &card->controls_rwsem)
                                        new_access = vd->access & ~mask;
    vd->access &= ~group_to_access();
                                        vd->access = new_access;
                                          <-- overwrites reset

The lost update leaves the control's LED access flags out of sync with the
LED group's control list.

[1] https://sashiko.dev/#/bug/linux-55c9a300-c6f3-4df1-9060-c4bb4df43d19

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106180812.5097-1-sreekuttan2156239@gmail.com?part=1

[PATCH v2] ALSA: control: Synchronize LED mode accesses

[Sreeraj S Kurup]

mode_store() updates led->mode while holding
snd_ctl_led_mutex.

However, snd_ctl_led_set_state() and mode_show() read this
field without holding the same mutex.

This can cause a data race when userspace changes the mode
while another path reads it.

Read the mode under snd_ctl_led_mutex in
snd_ctl_led_set_state() and use the local value after
releasing the mutex.

Also protect the mode read in mode_show() with the mutex.

Fixes: cb17fe0045aa ("ALSA: control - add sysfs support to the LED trigger module")
Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
v2:
 - Add the Fixes trailer.

 sound/core/control_led.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index 501387287ae6..044430b2f1a2 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -140,6 +140,7 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
 {
 	struct snd_ctl_led *led;
 	struct snd_ctl_led_ctl *lctl;
+	enum snd_ctl_led_mode mode;
 	int route;
 	bool found;
 
@@ -168,8 +169,9 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
 				UPDATE_ROUTE(route, snd_ctl_led_get(lctl));
 			}
 		}
+		mode = led->mode;
 	}
-	switch (led->mode) {
+	switch (mode) {
 	case MODE_OFF:		route = 1; break;
 	case MODE_ON:		route = 0; break;
 	case MODE_FOLLOW_ROUTE:	if (route >= 0) route ^= 1; break;
@@ -382,6 +384,8 @@ static ssize_t mode_show(struct device *dev,
 	struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev);
 	const char *str = NULL;
 
+	guard(mutex)(&snd_ctl_led_mutex);
+
 	switch (led->mode) {
 	case MODE_FOLLOW_MUTE:	str = "follow-mute"; break;
 	case MODE_FOLLOW_ROUTE:	str = "follow-route"; break;
-- 
2.55.0

Re: [PATCH v2] ALSA: control: Synchronize LED mode accesses

[Takashi Iwai]

On Fri, 06 Nov 2026 19:39:53 +0100,
Sreeraj S Kurup wrote:
> 
> mode_store() updates led->mode while holding
> snd_ctl_led_mutex.
> 
> However, snd_ctl_led_set_state() and mode_show() read this
> field without holding the same mutex.
> 
> This can cause a data race when userspace changes the mode
> while another path reads it.
> 
> Read the mode under snd_ctl_led_mutex in
> snd_ctl_led_set_state() and use the local value after
> releasing the mutex.
> 
> Also protect the mode read in mode_show() with the mutex.
> 
> Fixes: cb17fe0045aa ("ALSA: control - add sysfs support to the LED trigger module")
> Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

As this sort of data race for a single value has no much impact on the
actual behavior, it's more or less just for consistency.
So I applied to for-next branch.  Thanks.


Takashi

Re: [PATCH] ALSA: control: Synchronize LED mode accesses

[Sreeraj S Kurup]

Hi,

Thanks for applying the patch!

Best regards,
Sreeraj

Re: [PATCH v2] ALSA: control: Synchronize LED mode accesses

[Sreeraj S Kurup]

Hi,

Thanks for applying the patch!

Best regards,
Sreeraj

Re: [PATCH v2] ALSA: control: Synchronize LED mode accesses

[Sreeraj S Kurup]

Hi Takashi,

I would like to kindly follow up on my first patch.

Could you please let me know if you have had a chance to review it?

For reference, here is the patch link:
https://lore.kernel.org/all/20261106162357.3616-1-sreekuttan2156239@gmail.com/

I would appreciate your feedback whenever you have time.

Thank you for your time and consideration.

Best regards,
Sreeraj

Re: [PATCH v2] ALSA: control: Synchronize LED mode accesses

[Takashi Iwai]

On Sat, 07 Nov 2026 13:12:35 +0100,
Sreeraj S Kurup wrote:
> 
> Hi Takashi,
> 
> I would like to kindly follow up on my first patch.
> 
> Could you please let me know if you have had a chance to review it?
> 
> For reference, here is the patch link:
> https://lore.kernel.org/all/20261106162357.3616-1-sreekuttan2156239@gmail.com/

I'm still considering what's the best way.

Your patch replaces and adds some open-code, which isn't nice.
OTOH, it's a small change, so it's acceptable.

An alternative would be to use simple_strtoul() that can return the
end pointer.  Or, we may reorganize the whole code a bit, so that to
get the delimiter at first (instead of the head of the next word),
then clear and parse.  But not sure whether it'd be worth.


Takashi

Re: [PATCH v2] ALSA: control: Synchronize LED mode accesses

[Sreeraj S Kurup]

Hi Takashi,

I looked into using simple_strtoul(), but noticed that it is deprecated. 
kstrtoul() does not provide an end pointer either.

Would it be reasonable to retain the delimiter-first approach with 
kstrtouint(), as in v2?

Thanks,
Sreeraj