sound/core/control_led.c | 62 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 5 deletions(-)
snd_ctl_led_disconnect() removes the ctl-led sysfs files while
holding card->controls_rwsem. device_unregister() waits for running
sysfs callbacks which can deadlock when a concurrent write causes
the attach or detach callback to wait for controls_rwsem.
Break kernfs active protection in the attach, detach and list
callbacks and hold a module reference instead.
Fixes: a135dfb5de15 ("ALSA: led control - add sysfs kcontrol LED marking layer")
Signed-off-by: Ali Ahmet Memiş <aliamemis@disroot.org>
---
sound/core/control_led.c | 62 ++++++++++++++++++++++++++++++++++++----
1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index ec0e6c7ad..501387287 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -563,12 +563,43 @@ static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, si
return count;
}
+/*
+ * snd_ctl_led_disconnect() removes these files with card->controls_rwsem
+ * held. so don't let the removal wait for a callback that takes it.
+ */
+static struct kernfs_node *snd_ctl_led_sysfs_enter(struct device *dev,
+ struct device_attribute *attr)
+{
+ struct kernfs_node *kn;
+
+ if (!try_module_get(THIS_MODULE))
+ return NULL;
+ kn = sysfs_break_active_protection(&dev->kobj, &attr->attr);
+ if (!kn)
+ module_put(THIS_MODULE);
+ return kn;
+}
+
+static void snd_ctl_led_sysfs_exit(struct kernfs_node *kn)
+{
+ sysfs_unbreak_active_protection(kn);
+ module_put(THIS_MODULE);
+}
+
static ssize_t attach_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
- return set_led_id(led_card, buf, count, true);
+ struct kernfs_node *kn;
+ ssize_t ret;
+
+ kn = snd_ctl_led_sysfs_enter(dev, attr);
+ if (!kn)
+ return -ENODEV;
+ ret = set_led_id(led_card, buf, count, true);
+ snd_ctl_led_sysfs_exit(kn);
+ return ret;
}
static ssize_t detach_store(struct device *dev,
@@ -576,7 +607,15 @@ static ssize_t detach_store(struct device *dev,
const char *buf, size_t count)
{
struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
- return set_led_id(led_card, buf, count, false);
+ struct kernfs_node *kn;
+ ssize_t ret;
+
+ kn = snd_ctl_led_sysfs_enter(dev, attr);
+ if (!kn)
+ return -ENODEV;
+ ret = set_led_id(led_card, buf, count, false);
+ snd_ctl_led_sysfs_exit(kn);
+ return ret;
}
static ssize_t reset_store(struct device *dev,
@@ -594,10 +633,8 @@ static ssize_t reset_store(struct device *dev,
return count;
}
-static ssize_t list_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+static ssize_t snd_ctl_led_list(struct snd_ctl_led_card *led_card, char *buf)
{
- struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
struct snd_ctl_led_ctl *lctl;
size_t l = 0;
struct snd_card *card __free(snd_card_unref) =
@@ -620,6 +657,21 @@ static ssize_t list_show(struct device *dev,
return l;
}
+static ssize_t list_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
+ struct kernfs_node *kn;
+ ssize_t ret;
+
+ kn = snd_ctl_led_sysfs_enter(dev, attr);
+ if (!kn)
+ return -ENODEV;
+ ret = snd_ctl_led_list(led_card, buf);
+ snd_ctl_led_sysfs_exit(kn);
+ return ret;
+}
+
static DEVICE_ATTR_WO(attach);
static DEVICE_ATTR_WO(detach);
static DEVICE_ATTR_WO(reset);
--
2.55.0
On Sun, 11 Oct 2026 20:06:17 +0200,
Ali Ahmet Memiş wrote:
>
> snd_ctl_led_disconnect() removes the ctl-led sysfs files while
> holding card->controls_rwsem.
Hm, it's not clear to me how this happens: I see no controls_rwsem
down around snd_ctl_led_disconnect() call. Could you elaborate about
it a bit more?
thanks,
Takashi
> device_unregister() waits for running
> sysfs callbacks which can deadlock when a concurrent write causes
> the attach or detach callback to wait for controls_rwsem.
>
> Break kernfs active protection in the attach, detach and list
> callbacks and hold a module reference instead.
>
> Fixes: a135dfb5de15 ("ALSA: led control - add sysfs kcontrol LED marking layer")
> Signed-off-by: Ali Ahmet Memiş <aliamemis@disroot.org>
> ---
> sound/core/control_led.c | 62 ++++++++++++++++++++++++++++++++++++----
> 1 file changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index ec0e6c7ad..501387287 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -563,12 +563,43 @@ static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, si
> return count;
> }
>
> +/*
> + * snd_ctl_led_disconnect() removes these files with card->controls_rwsem
> + * held. so don't let the removal wait for a callback that takes it.
> + */
> +static struct kernfs_node *snd_ctl_led_sysfs_enter(struct device *dev,
> + struct device_attribute *attr)
> +{
> + struct kernfs_node *kn;
> +
> + if (!try_module_get(THIS_MODULE))
> + return NULL;
> + kn = sysfs_break_active_protection(&dev->kobj, &attr->attr);
> + if (!kn)
> + module_put(THIS_MODULE);
> + return kn;
> +}
> +
> +static void snd_ctl_led_sysfs_exit(struct kernfs_node *kn)
> +{
> + sysfs_unbreak_active_protection(kn);
> + module_put(THIS_MODULE);
> +}
> +
> static ssize_t attach_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> - return set_led_id(led_card, buf, count, true);
> + struct kernfs_node *kn;
> + ssize_t ret;
> +
> + kn = snd_ctl_led_sysfs_enter(dev, attr);
> + if (!kn)
> + return -ENODEV;
> + ret = set_led_id(led_card, buf, count, true);
> + snd_ctl_led_sysfs_exit(kn);
> + return ret;
> }
>
> static ssize_t detach_store(struct device *dev,
> @@ -576,7 +607,15 @@ static ssize_t detach_store(struct device *dev,
> const char *buf, size_t count)
> {
> struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> - return set_led_id(led_card, buf, count, false);
> + struct kernfs_node *kn;
> + ssize_t ret;
> +
> + kn = snd_ctl_led_sysfs_enter(dev, attr);
> + if (!kn)
> + return -ENODEV;
> + ret = set_led_id(led_card, buf, count, false);
> + snd_ctl_led_sysfs_exit(kn);
> + return ret;
> }
>
> static ssize_t reset_store(struct device *dev,
> @@ -594,10 +633,8 @@ static ssize_t reset_store(struct device *dev,
> return count;
> }
>
> -static ssize_t list_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> +static ssize_t snd_ctl_led_list(struct snd_ctl_led_card *led_card, char *buf)
> {
> - struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> struct snd_ctl_led_ctl *lctl;
> size_t l = 0;
> struct snd_card *card __free(snd_card_unref) =
> @@ -620,6 +657,21 @@ static ssize_t list_show(struct device *dev,
> return l;
> }
>
> +static ssize_t list_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> + struct kernfs_node *kn;
> + ssize_t ret;
> +
> + kn = snd_ctl_led_sysfs_enter(dev, attr);
> + if (!kn)
> + return -ENODEV;
> + ret = snd_ctl_led_list(led_card, buf);
> + snd_ctl_led_sysfs_exit(kn);
> + return ret;
> +}
> +
> static DEVICE_ATTR_WO(attach);
> static DEVICE_ATTR_WO(detach);
> static DEVICE_ATTR_WO(reset);
> --
> 2.55.0
>
On Mon, 12 Oct 2026 09:24:26 +0200, Takashi Iwai wrote:
> Hm, it's not clear to me how this happens: I see no controls_rwsem
> down around snd_ctl_led_disconnect() call. Could you elaborate about
> it a bit more?
It's taken in the call_snd_ctl_lops() macro in control.c:
guard(rwsem_read)(&(_card)->controls_rwsem);
guard(rwsem_read)(&snd_ctl_layer_rwsem);
so snd_ctl_dev_disconnect() calls ->ldisconnect() with controls_rwsem
held for read. snd_ctl_led_sysfs_remove() then waits in kernfs_drain()
for the attach write which is blocked on down_write(controls_rwsem) in
snd_ctl_led_set_id().
I hit it in QEMU with snd-dummy writing to attach in a loop while
unloading the module. It hung on the third iteration:
rmmod:
[<0>] kernfs_drain+0x2eb/0x510
[<0>] __kernfs_remove+0x2e6/0x890
[<0>] kernfs_remove_by_name_ns+0x7d/0x100
[<0>] remove_files+0x8d/0x1b0
[<0>] sysfs_remove_group+0x71/0xd0
[<0>] sysfs_remove_groups+0x67/0xd0
[<0>] device_remove_attrs+0x181/0x270
[<0>] device_del+0x31f/0x8d0
[<0>] device_unregister+0x12/0x30
[<0>] snd_ctl_led_sysfs_remove+0x161/0x2f0
[<0>] snd_ctl_led_disconnect+0x16/0xd0
[<0>] snd_ctl_dev_disconnect+0x151/0x210
[<0>] snd_device_disconnect_all+0xf0/0x180
[<0>] snd_card_disconnect.part.0+0x25d/0x4c0
attach writer:
[<0>] set_led_id+0x2d9/0xc60
[<0>] kernfs_fop_write_iter+0x33c/0x510
[<0>] vfs_write+0x516/0x1340
and lockdep reports it as well:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
rlock(snd_ctl_layer_rwsem);
lock(&card->controls_rwsem);
lock(snd_ctl_layer_rwsem);
lock(kn->active#37);
#1: ffff88800768a1f0 (&card->controls_rwsem){++++}-{4:4}, at: snd_ctl_dev_disconnect+0x10e/0x210
#2: ffffffff881fdf28 (snd_ctl_layer_rwsem){++++}-{4:4}, at: snd_ctl_dev_disconnect+0x11a/0x210
On Mon, 12 Oct 2026 16:12:35 +0200, Ali Ahmet Memiş wrote: > > On Mon, 12 Oct 2026 09:24:26 +0200, Takashi Iwai wrote: > > Hm, it's not clear to me how this happens: I see no controls_rwsem > > down around snd_ctl_led_disconnect() call. Could you elaborate about > > it a bit more? > > It's taken in the call_snd_ctl_lops() macro in control.c: > > guard(rwsem_read)(&(_card)->controls_rwsem); > guard(rwsem_read)(&snd_ctl_layer_rwsem); > > so snd_ctl_dev_disconnect() calls ->ldisconnect() with controls_rwsem > held for read. snd_ctl_led_sysfs_remove() then waits in kernfs_drain() > for the attach write which is blocked on down_write(controls_rwsem) in > snd_ctl_led_set_id(). Thanks, now it's clearer. The patch is applied to for-next branch now. thanks, Takashi
© 2016 - 2026 Red Hat, Inc.