* [PATCH] ALSA: info: Serialize freeing with recursive registration
@ 2026-10-17 6:58 Cen Zhang
2026-10-17 8:25 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Cen Zhang @ 2026-10-17 6:58 UTC (permalink / raw)
To: perex, tiwai; +Cc: linux-sound, linux-kernel, baijiaju1990, jjzuming, zzzccc427
An snd_info_entry must remain alive while it is reachable from a proc
node. snd_info_free_entry() clears entry->p under info_mutex, but drops
the mutex before unlinking and freeing the entry. The recursive
snd_info_register() walk can still find it and publish another node.
With a live card and a PCM OSS proc entry, unloading snd-pcm-oss can
therefore overlap a card owner's repeated snd_card_register(). The OSS
notifier removal holds register_mutex, while the already-registered card
path holds snd_card_mutex. Neither excludes the other path:
PCM OSS removal Card registration
snd_pcm_oss_proc_done()
snd_info_free_entry()
proc_remove(entry->p)
lock info_mutex
clear entry->p
unlock info_mutex
snd_info_card_register()
snd_info_register()
find linked OSS entry
lock info_mutex
publish new proc node
unlock info_mutex
unlink entry
kfree(entry)
A subsequent open of the new node passes its stale pde_data pointer to
snd_info_text_entry_open(). alloc_info_private() then reads entry->p
from freed storage.
Hold info_mutex across the entire recursive registration walk and across
clearing, unlinking and freeing the entry tree. Use internal recursive
helpers so the mutex is acquired only once. Registration then finishes
before the entry is cleared, or walks the tree after the entry has been
unlinked. Keep proc_remove() outside info_mutex: proc open takes the same
mutex, and removal waits for pending proc operations to finish.
KASAN report as below:
BUG: KASAN: slab-use-after-free in alloc_info_private+0x12f/0x1a0
Read of size 8 at addr ffff888115c84250 by task python3/426
CPU: 3 UID: 0 PID: 426 Comm: python3 Tainted: G O 7.2.0-rc6-pmb-bt-functional-v1+ #1 PREEMPT(lazy)
Tainted: [O]=OOT_MODULE
Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
Call Trace:
<TASK>
dump_stack_lvl+0x93/0xd0
print_report+0xce/0x630
? alloc_info_private+0x12f/0x1a0
? srso_alias_return_thunk+0x5/0xfbef5
? __virt_addr_valid+0x20d/0x410
? alloc_info_private+0x12f/0x1a0
kasan_report+0xe0/0x110
? alloc_info_private+0x12f/0x1a0
alloc_info_private+0x12f/0x1a0
snd_info_text_entry_open+0xbd/0x2d0
? __kasan_slab_alloc+0x89/0x90
? __pfx_snd_info_text_entry_open+0x10/0x10
? srso_alias_return_thunk+0x5/0xfbef5
? proc_reg_open+0x20e/0x590
? __pfx_snd_info_text_entry_open+0x10/0x10
proc_reg_open+0x27d/0x590
do_dentry_open+0x5c8/0x1300
? __pfx_proc_reg_open+0x10/0x10
? security_inode_permission+0x5d/0xb0
vfs_open+0x87/0x390
? srso_alias_return_thunk+0x5/0xfbef5
path_openat+0x2460/0x3cd0
? __pfx_path_openat+0x10/0x10
do_file_open+0x21e/0x470
? __pfx_do_file_open+0x10/0x10
? _raw_spin_unlock+0x23/0x40
? srso_alias_return_thunk+0x5/0xfbef5
? alloc_fd+0x3a6/0x6b0
do_sys_openat2+0xf4/0x1b0
? __pfx_do_sys_openat2+0x10/0x10
__x64_sys_openat+0x136/0x1e0
? __pfx___x64_sys_openat+0x10/0x10
? irqentry_exit+0xbe/0x820
? srso_alias_return_thunk+0x5/0xfbef5
do_syscall_64+0x115/0x6a0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f4e79f32687
Code: 48 89 fa 4c 89 df e8 58 b3 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44 24 10 0f 05 <5b> c3 0f 1f 80 00 00 00 00 83 e2 39 83 fa 08 75 de e8 23 ff ff ff
RSP: 002b:00007ffdd084f450 EFLAGS: 00000202 ORIG_RAX: 0000000000000101
RAX: ffffffffffffffda RBX: 00007f4e79e9e780 RCX: 00007f4e79f32687
RDX: 0000000000080000 RSI: 00007f4e79c84d90 RDI: ffffffffffffff9c
RBP: 00007f4e79cdc1a0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000202 R12: 00000000000001ff
R13: 00007f4e79e0b140 R14: 00007ffdd084f530 R15: 00007f4e79c84d90
</TASK>
Allocated by task 421:
kasan_save_stack+0x33/0x60
kasan_save_track+0x14/0x30
__kasan_kmalloc+0xaa/0xb0
__kmalloc_cache_noprof+0x251/0x610
snd_info_create_entry+0x56/0x400
0xffffffffc0401b64
snd_pcm_notify+0x34e/0x470
0xffffffffc041b19b
do_one_initcall+0xf3/0x4e0
do_init_module+0x281/0x8e0
load_module+0x658c/0x7cc0
init_module_from_file+0x15b/0x180
idempotent_init_module+0x22d/0x770
__x64_sys_finit_module+0xd2/0x160
do_syscall_64+0x115/0x6a0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
Freed by task 424:
kasan_save_stack+0x33/0x60
kasan_save_track+0x14/0x30
kasan_save_free_info+0x3b/0x60
__kasan_slab_free+0x5f/0x80
kfree+0x307/0x580
snd_info_free_entry+0x3d4/0x4d0
0xffffffffc0400d80
snd_pcm_notify+0x216/0x470
__do_sys_delete_module+0x338/0x530
do_syscall_64+0x115/0x6a0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
The buggy address belongs to the object at ffff888115c84200
which belongs to the cache kmalloc-256 of size 256
The buggy address is located 80 bytes inside of
freed 256-byte region [ffff888115c84200, ffff888115c84300)
The buggy address belongs to the physical page:
page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x115c84
head: order:1 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
flags: 0x200000000000040(head|node=0|zone=2)
page_type: f5(slab)
raw: 0200000000000040 ffff888100042b40 dead000000000100 dead000000000122
raw: 0000000000000000 0000000000100010 00000000f5000000 0000000000000000
head: 0200000000000040 ffff888100042b40 dead000000000100 dead000000000122
head: 0000000000000000 0000000000100010 00000000f5000000 0000000000000000
head: 0200000000000001 ffffffffffffff81 00000000ffffffff 00000000ffffffff
head: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff888115c84100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff888115c84180: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff888115c84200: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff888115c84280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff888115c84300: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================
Fixes: 2471b6c80a70 ("ALSA: info: Register proc entries recursively, too")
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
Assisted-by: LLM
---
diff --git a/sound/core/info.c b/sound/core/info.c
index c4d57d0809adff80732fdb741832c92b9b68f387..9b801caa65caab96ee4aa83a3141ea4306084868 100644
--- a/sound/core/info.c
+++ b/sound/core/info.c
@@ -737,27 +737,13 @@ static void snd_info_clear_entries(struct snd_info_entry *entry)
entry->p = NULL;
}
-/**
- * snd_info_free_entry - release the info entry
- * @entry: the info entry
- *
- * Releases the info entry.
- */
-void snd_info_free_entry(struct snd_info_entry * entry)
+static void __snd_info_free_entry(struct snd_info_entry *entry)
{
struct snd_info_entry *p, *n;
- if (!entry)
- return;
- if (entry->p) {
- proc_remove(entry->p);
- guard(mutex)(&info_mutex);
- snd_info_clear_entries(entry);
- }
-
/* free all children at first */
list_for_each_entry_safe(p, n, &entry->children, list)
- snd_info_free_entry(p);
+ __snd_info_free_entry(p);
p = entry->parent;
if (p) {
@@ -769,6 +755,25 @@ void snd_info_free_entry(struct snd_info_entry * entry)
entry->private_free(entry);
kfree(entry);
}
+
+/**
+ * snd_info_free_entry - release the info entry
+ * @entry: the info entry
+ *
+ * Releases the info entry.
+ */
+void snd_info_free_entry(struct snd_info_entry *entry)
+{
+ if (!entry)
+ return;
+ if (entry->p)
+ proc_remove(entry->p);
+
+ /* Serialize clearing and unlinking with recursive registration. */
+ guard(mutex)(&info_mutex);
+ snd_info_clear_entries(entry);
+ __snd_info_free_entry(entry);
+}
EXPORT_SYMBOL(snd_info_free_entry);
static int __snd_info_register(struct snd_info_entry *entry)
@@ -778,7 +783,6 @@ static int __snd_info_register(struct snd_info_entry *entry)
if (snd_BUG_ON(!entry))
return -ENXIO;
root = entry->parent == NULL ? snd_proc_root->p : entry->parent->p;
- guard(mutex)(&info_mutex);
if (entry->p || !root)
return 0;
if (S_ISDIR(entry->mode)) {
@@ -801,16 +805,7 @@ static int __snd_info_register(struct snd_info_entry *entry)
return 0;
}
-/**
- * snd_info_register - register the info entry
- * @entry: the info entry
- *
- * Registers the proc info entry.
- * The all children entries are registered recursively.
- *
- * Return: Zero if successful, or a negative error code on failure.
- */
-int snd_info_register(struct snd_info_entry *entry)
+static int snd_info_register_recursive(struct snd_info_entry *entry)
{
struct snd_info_entry *p;
int err;
@@ -822,13 +817,28 @@ int snd_info_register(struct snd_info_entry *entry)
}
list_for_each_entry(p, &entry->children, list) {
- err = snd_info_register(p);
+ err = snd_info_register_recursive(p);
if (err < 0)
return err;
}
return 0;
}
+
+/**
+ * snd_info_register - register the info entry
+ * @entry: the info entry
+ *
+ * Registers the proc info entry.
+ * The all children entries are registered recursively.
+ *
+ * Return: Zero if successful, or a negative error code on failure.
+ */
+int snd_info_register(struct snd_info_entry *entry)
+{
+ guard(mutex)(&info_mutex);
+ return snd_info_register_recursive(entry);
+}
EXPORT_SYMBOL(snd_info_register);
/**
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ALSA: info: Serialize freeing with recursive registration
2026-10-17 6:58 [PATCH] ALSA: info: Serialize freeing with recursive registration Cen Zhang
@ 2026-10-17 8:25 ` Takashi Iwai
2026-10-17 9:27 ` Cen Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2026-10-17 8:25 UTC (permalink / raw)
To: Cen Zhang; +Cc: perex, tiwai, linux-sound, linux-kernel, baijiaju1990, jjzuming
On Sat, 17 Oct 2026 08:58:31 +0200,
Cen Zhang wrote:
>
> An snd_info_entry must remain alive while it is reachable from a proc
> node. snd_info_free_entry() clears entry->p under info_mutex, but drops
> the mutex before unlinking and freeing the entry. The recursive
> snd_info_register() walk can still find it and publish another node.
>
> With a live card and a PCM OSS proc entry, unloading snd-pcm-oss can
> therefore overlap a card owner's repeated snd_card_register(). The OSS
> notifier removal holds register_mutex, while the already-registered card
> path holds snd_card_mutex. Neither excludes the other path:
>
> PCM OSS removal Card registration
> snd_pcm_oss_proc_done()
> snd_info_free_entry()
> proc_remove(entry->p)
> lock info_mutex
> clear entry->p
> unlock info_mutex
> snd_info_card_register()
> snd_info_register()
> find linked OSS entry
> lock info_mutex
> publish new proc node
> unlock info_mutex
> unlink entry
> kfree(entry)
>
> A subsequent open of the new node passes its stale pde_data pointer to
> snd_info_text_entry_open(). alloc_info_private() then reads entry->p
> from freed storage.
>
> Hold info_mutex across the entire recursive registration walk and across
> clearing, unlinking and freeing the entry tree. Use internal recursive
> helpers so the mutex is acquired only once. Registration then finishes
> before the entry is cleared, or walks the tree after the entry has been
> unlinked. Keep proc_remove() outside info_mutex: proc open takes the same
> mutex, and removal waits for pending proc operations to finish.
>
> KASAN report as below:
>
> BUG: KASAN: slab-use-after-free in alloc_info_private+0x12f/0x1a0
> Read of size 8 at addr ffff888115c84250 by task python3/426
>
> CPU: 3 UID: 0 PID: 426 Comm: python3 Tainted: G O 7.2.0-rc6-pmb-bt-functional-v1+ #1 PREEMPT(lazy)
> Tainted: [O]=OOT_MODULE
> Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> Call Trace:
> <TASK>
> dump_stack_lvl+0x93/0xd0
> print_report+0xce/0x630
> ? alloc_info_private+0x12f/0x1a0
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? __virt_addr_valid+0x20d/0x410
> ? alloc_info_private+0x12f/0x1a0
> kasan_report+0xe0/0x110
> ? alloc_info_private+0x12f/0x1a0
> alloc_info_private+0x12f/0x1a0
> snd_info_text_entry_open+0xbd/0x2d0
> ? __kasan_slab_alloc+0x89/0x90
> ? __pfx_snd_info_text_entry_open+0x10/0x10
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? proc_reg_open+0x20e/0x590
> ? __pfx_snd_info_text_entry_open+0x10/0x10
> proc_reg_open+0x27d/0x590
> do_dentry_open+0x5c8/0x1300
> ? __pfx_proc_reg_open+0x10/0x10
> ? security_inode_permission+0x5d/0xb0
> vfs_open+0x87/0x390
> ? srso_alias_return_thunk+0x5/0xfbef5
> path_openat+0x2460/0x3cd0
> ? __pfx_path_openat+0x10/0x10
> do_file_open+0x21e/0x470
> ? __pfx_do_file_open+0x10/0x10
> ? _raw_spin_unlock+0x23/0x40
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? alloc_fd+0x3a6/0x6b0
> do_sys_openat2+0xf4/0x1b0
> ? __pfx_do_sys_openat2+0x10/0x10
> __x64_sys_openat+0x136/0x1e0
> ? __pfx___x64_sys_openat+0x10/0x10
> ? irqentry_exit+0xbe/0x820
> ? srso_alias_return_thunk+0x5/0xfbef5
> do_syscall_64+0x115/0x6a0
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f4e79f32687
> Code: 48 89 fa 4c 89 df e8 58 b3 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44 24 10 0f 05 <5b> c3 0f 1f 80 00 00 00 00 83 e2 39 83 fa 08 75 de e8 23 ff ff ff
> RSP: 002b:00007ffdd084f450 EFLAGS: 00000202 ORIG_RAX: 0000000000000101
> RAX: ffffffffffffffda RBX: 00007f4e79e9e780 RCX: 00007f4e79f32687
> RDX: 0000000000080000 RSI: 00007f4e79c84d90 RDI: ffffffffffffff9c
> RBP: 00007f4e79cdc1a0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000202 R12: 00000000000001ff
> R13: 00007f4e79e0b140 R14: 00007ffdd084f530 R15: 00007f4e79c84d90
> </TASK>
>
> Allocated by task 421:
> kasan_save_stack+0x33/0x60
> kasan_save_track+0x14/0x30
> __kasan_kmalloc+0xaa/0xb0
> __kmalloc_cache_noprof+0x251/0x610
> snd_info_create_entry+0x56/0x400
> 0xffffffffc0401b64
> snd_pcm_notify+0x34e/0x470
> 0xffffffffc041b19b
> do_one_initcall+0xf3/0x4e0
> do_init_module+0x281/0x8e0
> load_module+0x658c/0x7cc0
> init_module_from_file+0x15b/0x180
> idempotent_init_module+0x22d/0x770
> __x64_sys_finit_module+0xd2/0x160
> do_syscall_64+0x115/0x6a0
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
>
> Freed by task 424:
> kasan_save_stack+0x33/0x60
> kasan_save_track+0x14/0x30
> kasan_save_free_info+0x3b/0x60
> __kasan_slab_free+0x5f/0x80
> kfree+0x307/0x580
> snd_info_free_entry+0x3d4/0x4d0
> 0xffffffffc0400d80
> snd_pcm_notify+0x216/0x470
> __do_sys_delete_module+0x338/0x530
> do_syscall_64+0x115/0x6a0
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
>
> The buggy address belongs to the object at ffff888115c84200
> which belongs to the cache kmalloc-256 of size 256
> The buggy address is located 80 bytes inside of
> freed 256-byte region [ffff888115c84200, ffff888115c84300)
>
> The buggy address belongs to the physical page:
> page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x115c84
> head: order:1 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
> flags: 0x200000000000040(head|node=0|zone=2)
> page_type: f5(slab)
> raw: 0200000000000040 ffff888100042b40 dead000000000100 dead000000000122
> raw: 0000000000000000 0000000000100010 00000000f5000000 0000000000000000
> head: 0200000000000040 ffff888100042b40 dead000000000100 dead000000000122
> head: 0000000000000000 0000000000100010 00000000f5000000 0000000000000000
> head: 0200000000000001 ffffffffffffff81 00000000ffffffff 00000000ffffffff
> head: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff888115c84100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff888115c84180: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >ffff888115c84200: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ^
> ffff888115c84280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff888115c84300: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ==================================================================
>
> Fixes: 2471b6c80a70 ("ALSA: info: Register proc entries recursively, too")
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> Assisted-by: LLM
Does the above crash really happen with unmodified code?
The PCM oss proc unregister is per card, hence there can't be race
between register and unregister for the same card.
The hardening itself in the proc code is good, per se (although it's
doubtful whether we really need a global mutex from the performance
POV), though.
thanks,
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ALSA: info: Serialize freeing with recursive registration
2026-10-17 8:25 ` Takashi Iwai
@ 2026-10-17 9:27 ` Cen Zhang
2026-10-17 10:31 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Cen Zhang @ 2026-10-17 9:27 UTC (permalink / raw)
To: Takashi Iwai
Cc: perex, tiwai, linux-sound, linux-kernel, baijiaju1990, jjzuming
Hi Takashi,
>
> Does the above crash really happen with unmodified code?
> The PCM oss proc unregister is per card, hence there can't be race
> between register and unregister for the same card.
>
> The hardening itself in the proc code is good, per se (although it's
> doubtful whether we really need a global mutex from the performance
> POV), though.
>
>
> thanks,
>
> Takashi
Thank you for the review. The quoted KASAN report used scheduling
instrumentation
and a small test module that called snd_card_register() on an already registered
dummy card.
But reviewing the source, I found a concrete scenario involving
existing in-tree callers that appears to allow this race.
Consider a registered card with PCM A and its published OSS proc
entry. A later bind of another codec B on the same card can reach
hda_codec_driver_probe(), which calls snd_card_register() when
!bus_probing && card->registered.
At the same time, snd-pcm-oss can be unloaded while the card remains
registered, provided no active OSS references prevent the unload.
alsa_pcm_oss_exit() calls snd_pcm_notify(..., 1), which invokes
n_unregister() for the registered PCM devices and frees their OSS
proc entries.
The interleaving I believe the source permits is:
HDA codec B probe snd-pcm-oss module exit
----------------- -----------------------
snd_card_register(card)
snd_device_register_all(card)
/* PCM register_mutex released */
lock(register_mutex)
n_unregister(PCM A)
snd_info_free_entry(entry)
proc_remove(entry->p)
lock(info_mutex)
entry->p = NULL
unlock(info_mutex)
/* entry still linked */
lock(snd_card_mutex)
snd_info_card_register(card)
snd_info_register(card->proc_root)
find PCM A's linked entry
lock(info_mutex)
recreate proc node with
entry as private data
unlock(info_mutex)
unlock(snd_card_mutex)
unlink entry
kfree(entry)
unlock(register_mutex)
The card-info traversal does not hold the PCM register_mutex, and
OSS notifier removal does not take snd_card_mutex. The parent proc
directories also remain present because the card and PCM A have
not been removed.
This appears to allow the new proc node to retain a pointer to the
freed entry. A subsequent open could then reach alloc_info_private()
with that stale pointer. Please let me know if I have overlooked
another serialization point in this path.
Your point about the global mutex deserves careful consideration. Do you
have any suggestions for a better way to synchronize these paths?
Best regards,
Cen Zhang
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ALSA: info: Serialize freeing with recursive registration
2026-10-17 9:27 ` Cen Zhang
@ 2026-10-17 10:31 ` Takashi Iwai
2026-10-17 10:47 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2026-10-17 10:31 UTC (permalink / raw)
To: Cen Zhang
Cc: Takashi Iwai, perex, tiwai, linux-sound, linux-kernel,
baijiaju1990, jjzuming
On Sat, 17 Oct 2026 11:27:54 +0200,
Cen Zhang wrote:
>
> Hi Takashi,
>
> >
> > Does the above crash really happen with unmodified code?
> > The PCM oss proc unregister is per card, hence there can't be race
> > between register and unregister for the same card.
> >
> > The hardening itself in the proc code is good, per se (although it's
> > doubtful whether we really need a global mutex from the performance
> > POV), though.
> >
> >
> > thanks,
> >
> > Takashi
>
>
> Thank you for the review. The quoted KASAN report used scheduling
> instrumentation
> and a small test module that called snd_card_register() on an already registered
> dummy card.
>
> But reviewing the source, I found a concrete scenario involving
> existing in-tree callers that appears to allow this race.
>
> Consider a registered card with PCM A and its published OSS proc
> entry. A later bind of another codec B on the same card can reach
> hda_codec_driver_probe(), which calls snd_card_register() when
> !bus_probing && card->registered.
>
> At the same time, snd-pcm-oss can be unloaded while the card remains
> registered, provided no active OSS references prevent the unload.
> alsa_pcm_oss_exit() calls snd_pcm_notify(..., 1), which invokes
> n_unregister() for the registered PCM devices and frees their OSS
> proc entries.
>
> The interleaving I believe the source permits is:
>
> HDA codec B probe snd-pcm-oss module exit
> ----------------- -----------------------
> snd_card_register(card)
> snd_device_register_all(card)
> /* PCM register_mutex released */
>
> lock(register_mutex)
> n_unregister(PCM A)
> snd_info_free_entry(entry)
> proc_remove(entry->p)
> lock(info_mutex)
> entry->p = NULL
> unlock(info_mutex)
> /* entry still linked */
>
> lock(snd_card_mutex)
> snd_info_card_register(card)
> snd_info_register(card->proc_root)
> find PCM A's linked entry
> lock(info_mutex)
> recreate proc node with
> entry as private data
> unlock(info_mutex)
> unlock(snd_card_mutex)
>
> unlink entry
> kfree(entry)
> unlock(register_mutex)
>
> The card-info traversal does not hold the PCM register_mutex, and
> OSS notifier removal does not take snd_card_mutex. The parent proc
> directories also remain present because the card and PCM A have
> not been removed.
>
> This appears to allow the new proc node to retain a pointer to the
> freed entry. A subsequent open could then reach alloc_info_private()
> with that stale pointer. Please let me know if I have overlooked
> another serialization point in this path.
OK, the arbitrary OSS removal is the problem child.
IMO, instead of hardening piece-by-piece in ad hoc ways, we can pin
the OSS PCM and mixer module when any card is added and unpin at the
card removal, so that we can't unload OSS modules while there are any
actually used cards.
The cases with the partial HDA codec probe/remove is a different
problem, though. This has to be addressed somehow better.
thanks,
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: info: Serialize freeing with recursive registration
2026-10-17 10:31 ` Takashi Iwai
@ 2026-10-17 10:47 ` Takashi Iwai
2026-10-17 15:14 ` Cen Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2026-10-17 10:47 UTC (permalink / raw)
To: Cen Zhang; +Cc: perex, tiwai, linux-sound, linux-kernel, baijiaju1990, jjzuming
On Sat, 17 Oct 2026 12:31:54 +0200,
Takashi Iwai wrote:
>
> On Sat, 17 Oct 2026 11:27:54 +0200,
> Cen Zhang wrote:
> >
> > Hi Takashi,
> >
> > >
> > > Does the above crash really happen with unmodified code?
> > > The PCM oss proc unregister is per card, hence there can't be race
> > > between register and unregister for the same card.
> > >
> > > The hardening itself in the proc code is good, per se (although it's
> > > doubtful whether we really need a global mutex from the performance
> > > POV), though.
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> >
> >
> > Thank you for the review. The quoted KASAN report used scheduling
> > instrumentation
> > and a small test module that called snd_card_register() on an already registered
> > dummy card.
> >
> > But reviewing the source, I found a concrete scenario involving
> > existing in-tree callers that appears to allow this race.
> >
> > Consider a registered card with PCM A and its published OSS proc
> > entry. A later bind of another codec B on the same card can reach
> > hda_codec_driver_probe(), which calls snd_card_register() when
> > !bus_probing && card->registered.
> >
> > At the same time, snd-pcm-oss can be unloaded while the card remains
> > registered, provided no active OSS references prevent the unload.
> > alsa_pcm_oss_exit() calls snd_pcm_notify(..., 1), which invokes
> > n_unregister() for the registered PCM devices and frees their OSS
> > proc entries.
> >
> > The interleaving I believe the source permits is:
> >
> > HDA codec B probe snd-pcm-oss module exit
> > ----------------- -----------------------
> > snd_card_register(card)
> > snd_device_register_all(card)
> > /* PCM register_mutex released */
> >
> > lock(register_mutex)
> > n_unregister(PCM A)
> > snd_info_free_entry(entry)
> > proc_remove(entry->p)
> > lock(info_mutex)
> > entry->p = NULL
> > unlock(info_mutex)
> > /* entry still linked */
> >
> > lock(snd_card_mutex)
> > snd_info_card_register(card)
> > snd_info_register(card->proc_root)
> > find PCM A's linked entry
> > lock(info_mutex)
> > recreate proc node with
> > entry as private data
> > unlock(info_mutex)
> > unlock(snd_card_mutex)
> >
> > unlink entry
> > kfree(entry)
> > unlock(register_mutex)
> >
> > The card-info traversal does not hold the PCM register_mutex, and
> > OSS notifier removal does not take snd_card_mutex. The parent proc
> > directories also remain present because the card and PCM A have
> > not been removed.
> >
> > This appears to allow the new proc node to retain a pointer to the
> > freed entry. A subsequent open could then reach alloc_info_private()
> > with that stale pointer. Please let me know if I have overlooked
> > another serialization point in this path.
>
> OK, the arbitrary OSS removal is the problem child.
>
> IMO, instead of hardening piece-by-piece in ad hoc ways, we can pin
> the OSS PCM and mixer module when any card is added and unpin at the
> card removal, so that we can't unload OSS modules while there are any
> actually used cards.
I meant something like below (totally untested)
Takashi
-- 8< --
diff --git a/sound/core/oss/mixer_oss.c b/sound/core/oss/mixer_oss.c
index f1fa42e634fc..2e686bd6c5f9 100644
--- a/sound/core/oss/mixer_oss.c
+++ b/sound/core/oss/mixer_oss.c
@@ -1309,13 +1309,18 @@ static int snd_mixer_oss_free1(void *private)
static int snd_mixer_oss_notify_handler(struct snd_card *card, int cmd)
{
struct snd_mixer_oss *mixer;
+ int err;
if (cmd == SND_MIXER_OSS_NOTIFY_REGISTER) {
- int idx, err;
+ int idx;
mixer = kzalloc_objs(*mixer, 2);
if (mixer == NULL)
return -ENOMEM;
+ if (!try_module_get(THIS_MODULE)) {
+ kfree(mixer);
+ return -ENODEV;
+ }
mutex_init(&mixer->reg_mutex);
err = snd_register_oss_device(SNDRV_OSS_DEVICE_TYPE_MIXER,
card, 0,
@@ -1325,6 +1330,7 @@ static int snd_mixer_oss_notify_handler(struct snd_card *card, int cmd)
"unable to register OSS mixer device %i:%i\n",
card->number, 0);
kfree(mixer);
+ module_put(THIS_MODULE);
return err;
}
mixer->oss_dev_alloc = 1;
@@ -1358,7 +1364,9 @@ static int snd_mixer_oss_notify_handler(struct snd_card *card, int cmd)
if (cmd == SND_MIXER_OSS_NOTIFY_DISCONNECT)
return 0;
snd_mixer_oss_proc_done(mixer);
- return snd_mixer_oss_free1(mixer);
+ err = snd_mixer_oss_free1(mixer);
+ module_put(THIS_MODULE);
+ return err;
}
return 0;
}
diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
index bd19328dca9e..3d35f9d2d675 100644
--- a/sound/core/oss/pcm_oss.c
+++ b/sound/core/oss/pcm_oss.c
@@ -3151,6 +3151,9 @@ static void register_oss_dsp(struct snd_pcm *pcm, int index)
static int snd_pcm_oss_register_minor(struct snd_pcm *pcm)
{
+ if (!try_module_get(THIS_MODULE))
+ return -ENODEV;
+
pcm->oss.reg = 0;
if (dsp_map[pcm->card->number] == (int)pcm->device) {
char name[128];
@@ -3207,6 +3210,7 @@ static int snd_pcm_oss_unregister_minor(struct snd_pcm *pcm)
{
snd_pcm_oss_disconnect_minor(pcm);
snd_pcm_oss_proc_done(pcm);
+ module_put(THIS_MODULE);
return 0;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ALSA: info: Serialize freeing with recursive registration
2026-10-17 10:47 ` Takashi Iwai
@ 2026-10-17 15:14 ` Cen Zhang
0 siblings, 0 replies; 6+ messages in thread
From: Cen Zhang @ 2026-10-17 15:14 UTC (permalink / raw)
To: Takashi Iwai
Cc: perex, tiwai, linux-sound, linux-kernel, baijiaju1990, jjzuming
Hi Takashi,
Takashi Iwai <tiwai@suse.de> 于2026年10月17日周六 18:47写道：
>
> I meant something like below (totally untested)
>
>
> Takashi
>
> -- 8< --
> diff --git a/sound/core/oss/mixer_oss.c b/sound/core/oss/mixer_oss.c
> index f1fa42e634fc..2e686bd6c5f9 100644
> --- a/sound/core/oss/mixer_oss.c
> +++ b/sound/core/oss/mixer_oss.c
> @@ -1309,13 +1309,18 @@ static int snd_mixer_oss_free1(void *private)
> static int snd_mixer_oss_notify_handler(struct snd_card *card, int cmd)
> {
> struct snd_mixer_oss *mixer;
> + int err;
>
> if (cmd == SND_MIXER_OSS_NOTIFY_REGISTER) {
> - int idx, err;
> + int idx;
>
> mixer = kzalloc_objs(*mixer, 2);
> if (mixer == NULL)
> return -ENOMEM;
> + if (!try_module_get(THIS_MODULE)) {
> + kfree(mixer);
> + return -ENODEV;
> + }
> mutex_init(&mixer->reg_mutex);
> err = snd_register_oss_device(SNDRV_OSS_DEVICE_TYPE_MIXER,
> card, 0,
> @@ -1325,6 +1330,7 @@ static int snd_mixer_oss_notify_handler(struct snd_card *card, int cmd)
> "unable to register OSS mixer device %i:%i\n",
> card->number, 0);
> kfree(mixer);
> + module_put(THIS_MODULE);
> return err;
> }
> mixer->oss_dev_alloc = 1;
> @@ -1358,7 +1364,9 @@ static int snd_mixer_oss_notify_handler(struct snd_card *card, int cmd)
> if (cmd == SND_MIXER_OSS_NOTIFY_DISCONNECT)
> return 0;
> snd_mixer_oss_proc_done(mixer);
> - return snd_mixer_oss_free1(mixer);
> + err = snd_mixer_oss_free1(mixer);
> + module_put(THIS_MODULE);
> + return err;
> }
> return 0;
> }
> diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
> index bd19328dca9e..3d35f9d2d675 100644
> --- a/sound/core/oss/pcm_oss.c
> +++ b/sound/core/oss/pcm_oss.c
> @@ -3151,6 +3151,9 @@ static void register_oss_dsp(struct snd_pcm *pcm, int index)
>
> static int snd_pcm_oss_register_minor(struct snd_pcm *pcm)
> {
> + if (!try_module_get(THIS_MODULE))
> + return -ENODEV;
> +
> pcm->oss.reg = 0;
> if (dsp_map[pcm->card->number] == (int)pcm->device) {
> char name[128];
> @@ -3207,6 +3210,7 @@ static int snd_pcm_oss_unregister_minor(struct snd_pcm *pcm)
> {
> snd_pcm_oss_disconnect_minor(pcm);
> snd_pcm_oss_proc_done(pcm);
> + module_put(THIS_MODULE);
> return 0;
> }
>
Thank you for the review and suggestions. This looks good to me.
While reviewing the draft, I noticed one detail we may need to account
for: snd_pcm_free() calls
n_unregister even for a PCM that never reached n_register, for example
during creation failure or
BUILD-state cleanup. We may therefore need per-PCM bookkeeping so that
unregister drops a
module reference only if one was successfully acquired.
I would also like to check the last-reference drop against callback
completion and notifier traversal,
to make sure module teardown cannot overtake either.
I will send a patch soon.
Thanks again.
Best regards,
Cen Zhang
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-10-17 15:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-17 6:58 [PATCH] ALSA: info: Serialize freeing with recursive registration Cen Zhang
2026-10-17 8:25 ` Takashi Iwai
2026-10-17 9:27 ` Cen Zhang
2026-10-17 10:31 ` Takashi Iwai
2026-10-17 10:47 ` Takashi Iwai
2026-10-17 15:14 ` Cen Zhang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.