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

[sashiko-bot@kernel.org]

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