| Message ID | 20240520170349.2417900-1-xu.yang_2@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: usb-audio: fix potential use after free issue when remove module snd-usb-audio | expand |
On Mon, 20 May 2024 19:03:49 +0200, Xu Yang wrote: > > When remove module snd-usb-audio, snd_card_free_when_closed() will not > release the card resource if the card_dev refcount > 0 and > usb_audio_disconnect() will return directly, finally kernel will release > the module resource. Then if the userspace continue to cleanup sound card > resources, such as close controlC0, the closing path will touch this > modules' code, unfortunately it just got released and the kernel will > dump below error: > > [ 183.450073] Internal error: Oops: 0000000086000007 [#1] PREEMPT SMP > [ 183.456345] Modules linked in: snd_usbmidi_lib snd_hwdep [last unloaded: snd_usb_audio] > [ 183.464373] CPU: 0 PID: 537 Comm: wireplumber Not tainted 6.6.23-06215-gc5317d88b3ec #708 > [ 183.472552] Hardware name: NXP i.MX93 11X11 EVK board (DT) > [ 183.478039] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 183.485004] pc : 0xffff80007c19a3b0 > [ 183.488507] lr : snd_pcm_dev_free+0x3c/0x70 > [ 183.492708] sp : ffff800085c77af0 > [ 183.496030] x29: ffff800085c77af0 x28: dead000000000122 x27: ffff0000079f8090 > [ 183.503188] x26: ffff0000079f8090 x25: ffff00000e78c270 x24: ffff00000e78c000 > [ 183.510336] x23: ffff00000e78c1a8 x22: ffff00000e78c000 x21: ffff00000b6a2718 > [ 183.517486] x20: ffff800082608180 x19: ffff00000d2d7400 x18: 0000000000000000 > [ 183.524635] x17: 0000000000000000 x16: 0000000000000000 x15: 0000ffffa0000b90 > [ 183.531786] x14: 0000000000000000 x13: 0000000000000000 x12: ffff700010b8ef49 > [ 183.538939] x11: 1ffff00010b8ef48 x10: ffff700010b8ef48 x9 : 0000000000000000 > [ 183.546086] x8 : ffff800085c77a50 x7 : ffff00006ccd77b0 x6 : 0000000000000003 > [ 183.553236] x5 : 00000000000000c0 x4 : ffff00000ea57000 x3 : dfff800000000000 > [ 183.560386] x2 : 0000000000000007 x1 : ffff80007c19a3b0 x0 : ffff00000d2d7400 > [ 183.567539] Call trace: > [ 183.569992] 0xffff80007c19a3b0 > [ 183.573134] __snd_device_free+0x94/0x16c > [ 183.577156] snd_device_free_all+0x70/0xe8 > [ 183.581258] release_card_device+0x30/0xc0 > [ 183.585363] device_release+0x50/0x10c > [ 183.589124] kobject_put+0xe0/0x184 > [ 183.592634] put_device+0x14/0x24 > [ 183.595954] snd_card_file_remove+0x158/0x22c > [ 183.600322] snd_ctl_release+0x174/0x194 > [ 183.604256] snd_disconnect_release+0x128/0x178 > [ 183.608798] __fput+0x160/0x3d0 > [ 183.611955] __fput_sync+0x74/0x84 > [ 183.615370] __arm64_sys_close+0x4c/0x8c > [ 183.619302] invoke_syscall+0x60/0x184 > [ 183.623072] el0_svc_common.constprop.0+0x114/0x13c > [ 183.627960] do_el0_svc+0x30/0x40 > [ 183.631288] el0_svc+0x38/0x70 > [ 183.634356] el0t_64_sync_handler+0x120/0x12c > [ 183.638724] el0t_64_sync+0x190/0x194 > [ 183.642403] Code: ???????? ???????? ???????? ???????? (????????) > [ 183.648497] ---[ end trace 0000000000000000 ]--- > > To fix the issue, use snd_card_free() to release all resources (including > all files attached to the card_dev) instead snd_card_free_when_closed(). > Then, even the userspace trying to cleanup the resources, kernel will not > touch the released code memory. Hm, it's an interesting report. Could you verify whether it's really hitting a module unload race? The module refcount should have been non-zero when the device is still in use, and it should have prevented the module unloading. Practically seen, replacing snd_card_free_when_closed() with snd_card_free() shouldn't be a big problem, and it'll work in most cases. But there are always some corner cases that might lead to unexpected behavior. So, let's try to analyze more exactly what's happening there at first. thanks, Takashi > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > --- > sound/usb/card.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/usb/card.c b/sound/usb/card.c > index 1b2edc0fd2e9..5e799f147eb5 100644 > --- a/sound/usb/card.c > +++ b/sound/usb/card.c > @@ -992,7 +992,7 @@ static void usb_audio_disconnect(struct usb_interface *intf) > if (chip->num_interfaces <= 0) { > usb_chip[chip->index] = NULL; > mutex_unlock(®ister_mutex); > - snd_card_free_when_closed(card); > + snd_card_free(card); > } else { > mutex_unlock(®ister_mutex); > } > -- > 2.34.1 >
On Mon, May 20, 2024 at 12:29:15PM +0200, Takashi Iwai wrote: > On Mon, 20 May 2024 19:03:49 +0200, > Xu Yang wrote: > > > > When remove module snd-usb-audio, snd_card_free_when_closed() will not > > release the card resource if the card_dev refcount > 0 and [...] > > Then, even the userspace trying to cleanup the resources, kernel will not > > touch the released code memory. > > Hm, it's an interesting report. Could you verify whether it's really > hitting a module unload race? The module refcount should have been > non-zero when the device is still in use, and it should have prevented > the module unloading. Yes, the race does exist. I enable trace and got below output: It seems that snd_usb_audio module refcnt is 0 after insmod completed. So it can continue to be removed even it's still in use. # echo 1 > /sys/kernel/tracing/events/module/enable # cat /sys/kernel/tracing/trace_pipe & [1] 522 # insmod snd-hwdep.ko <...>-527 [000] ..... 42.147850: module_load: snd_hwdep insmod-527 [000] ..... 42.148106: module_put: snd_hwdep call_site=do_init_module refcnt=1 # insmod snd-usbmidi-lib.ko <...>-529 [000] ..... 46.723871: module_load: snd_usbmidi_lib insmod-529 [000] ..... 46.724105: module_put: snd_usbmidi_lib call_site=do_init_module refcnt=1 # insmod snd-usb-audio.ko; sleep 2; rmmod snd-usb-audio.ko <...>-531 [000] ..... 59.720852: module_get: snd_usbmidi_lib call_site=resolve_symbol refcnt=2 <...>-531 [000] ..... 59.720922: module_get: snd_hwdep call_site=resolve_symbol refcnt=2 <...>-531 [000] ..... 59.722334: module_load: snd_usb_audio insmod-531 [000] ..... 59.893096: module_put: snd_usb_audio call_site=do_init_module refcnt=1 rmmod-541 [000] ..... 61.911391: module_free: snd_usb_audio rmmod-541 [000] ..... 61.912291: module_put: snd_hwdep call_site=module_unload_free refcnt=1 rmmod-541 [000] ..... 61.912315: module_put: snd_usbmidi_lib call_site=module_unload_free refcnt=1 [ 61.927058] Internal error: Oops: 0000000086000007 [#1] PREEMPT SMP [ 61.933320] Modules linked in: snd_usbmidi_lib snd_hwdep [last unloaded: snd_usb_audio] [ 61.941320] CPU: 1 PID: 476 Comm: wireplumber Not tainted 6.6.23-06215-gc5317d88b3ec #719 [ 61.949480] Hardware name: NXP i.MX93 11X11 EVK board (DT) [ 61.954950] pstate: 20400009 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 61.961900] pc : 0xffff80007a85fb88 [ 61.965384] lr : snd_pcm_dev_free+0x28/0x5c [ 61.969567] sp : ffff800083f9bbf0 [ 61.972869] x29: ffff800083f9bbf0 x28: ffff00000a0b8000 x27: 0000000000000000 [ 61.979993] x26: dead000000000122 x25: dead000000000100 x24: ffff00000a2dea70 [ 61.987117] x23: ffff800082819ce8 x22: 0000000000000000 x21: ffff00000a2de800 [ 61.994241] x20: ffff00000a2de9a0 x19: ffff00000751ba00 x18: 0000000000000000 [ 62.001368] x17: 0000000000000000 x16: 0000000000000000 x15: 0111289e3bb3ce7e [ 62.008489] x14: 000044678c0b24b6 x13: 00000000000000ab x12: ffff000007b23200 [ 62.015613] x11: 0000000000000000 x10: 0000000000000a70 x9 : ffff800083f9b9e0 [ 62.022737] x8 : ffff800083f9bba0 x7 : 0000000000000000 x6 : ffff00000a2ded98 [ 62.029861] x5 : ffff8000811bedc4 x4 : dead000000000122 x3 : ffff00000b52dc80 [ 62.036985] x2 : ffff000007f36240 x1 : ffff80007a85fb88 x0 : ffff00000751ba00 [ 62.044112] Call trace: [ 62.046548] 0xffff80007a85fb88 [ 62.049683] __snd_device_free+0x50/0xd0 [ 62.053602] snd_device_free_all+0x4c/0x9c [ 62.057690] release_card_device+0x24/0x90 [ 62.061772] device_release+0x34/0x90 [ 62.065432] kobject_put+0xa4/0x120 [ 62.068913] put_device+0x14/0x24 [ 62.072224] snd_card_file_remove+0xe4/0x178 [ 62.076479] snd_ctl_release+0xfc/0x114 [ 62.080313] snd_disconnect_release+0xcc/0x114 [ 62.084747] __fput+0xb4/0x274 [ 62.087798] __fput_sync+0x50/0x5c [ 62.091195] __arm64_sys_close+0x38/0x7c [ 62.095113] invoke_syscall+0x48/0x110 [ 62.098857] el0_svc_common.constprop.0+0xc0/0xe0 [ 62.103554] do_el0_svc+0x1c/0x28 [ 62.106865] el0_svc+0x40/0xe4 [ 62.109918] el0t_64_sync_handler+0x120/0x12c [ 62.114266] el0t_64_sync+0x190/0x194 [ 62.117933] Code: ???????? ???????? ???????? ???????? (????????) [ 62.124011] ---[ end trace 0000000000000000 ]--- Then I take some time to check why snd_usb_audio module refcnt is 0 even though the card_dev is in use. Finally I got below finding: I build kernel and module with below configuration: CONFIG_SOUND=y CONFIG_SND=y CONFIG_SND_USB=y CONFIG_SND_USB_AUDIO=m Then GCC will add -DMODULE when build snd-usb-audio as module, but will not add -DMODULE when build sound/core/*.c. When insmod snd-usb-audio.ko, it will create a snd card device and call: snd_card_init() // sound/core/init.c #ifdef MODULE WARN_ON(!module); card->module = module; #endif However, MODULE is not defined for sound/core/init.c, then card->module will keep NULL pointer. With this results, snd-usb-audio module refcnt will not be a non-zero value. > > Practically seen, replacing snd_card_free_when_closed() with > snd_card_free() shouldn't be a big problem, and it'll work in most > cases. But there are always some corner cases that might lead to > unexpected behavior. So, let's try to analyze more exactly what's > happening there at first. With above finding, we needn't to replace snd_card_free_when_closed() with snd_card_free(). We need to find a way to correctly handle module refcnt since this should be a normal usecase. Thanks, Xu Yang > > > thanks, > > Takashi >
On Tue, 21 May 2024 12:56:05 +0200, Xu Yang wrote: > > On Mon, May 20, 2024 at 12:29:15PM +0200, Takashi Iwai wrote: > > On Mon, 20 May 2024 19:03:49 +0200, > > Xu Yang wrote: > > > > > > When remove module snd-usb-audio, snd_card_free_when_closed() will not > > > release the card resource if the card_dev refcount > 0 and > > [...] > > > > Then, even the userspace trying to cleanup the resources, kernel will not > > > touch the released code memory. > > > > Hm, it's an interesting report. Could you verify whether it's really > > hitting a module unload race? The module refcount should have been > > non-zero when the device is still in use, and it should have prevented > > the module unloading. > > Yes, the race does exist. I enable trace and got below output: > It seems that snd_usb_audio module refcnt is 0 after insmod completed. So > it can continue to be removed even it's still in use. If no device is opened, it's not really "used", and the driver module can be unloaded at any time. That's the intended behavior. (snip) > Then I take some time to check why snd_usb_audio module refcnt is 0 > even though the card_dev is in use. Finally I got below finding: > > I build kernel and module with below configuration: > > CONFIG_SOUND=y > CONFIG_SND=y > CONFIG_SND_USB=y > CONFIG_SND_USB_AUDIO=m > > Then GCC will add -DMODULE when build snd-usb-audio as module, but will > not add -DMODULE when build sound/core/*.c. > > When insmod snd-usb-audio.ko, it will create a snd card device and call: > > snd_card_init() // sound/core/init.c > > #ifdef MODULE > WARN_ON(!module); > card->module = module; > #endif > > However, MODULE is not defined for sound/core/init.c, then card->module > will keep NULL pointer. With this results, snd-usb-audio module refcnt > will not be a non-zero value. Ah, it's a good finding! That explains. > > Practically seen, replacing snd_card_free_when_closed() with > > snd_card_free() shouldn't be a big problem, and it'll work in most > > cases. But there are always some corner cases that might lead to > > unexpected behavior. So, let's try to analyze more exactly what's > > happening there at first. > > With above finding, we needn't to replace snd_card_free_when_closed() > with snd_card_free(). We need to find a way to correctly handle module > refcnt since this should be a normal usecase. Right, I guess a simple fix below to replace '#ifdef MODULE' with '#ifdef CONFIG_MODULES' should work instead? thanks, Takashi -- 8< -- --- a/sound/core/init.c +++ b/sound/core/init.c @@ -50,7 +50,7 @@ MODULE_PARM_DESC(slots, "Module names assigned to the slots."); static int module_slot_match(struct module *module, int idx) { int match = 1; -#ifdef MODULE +#ifdef CONFIG_MODULES const char *s1, *s2; if (!module || !*module->name || !slots[idx]) @@ -77,7 +77,7 @@ static int module_slot_match(struct module *module, int idx) if (!c1) break; } -#endif /* MODULE */ +#endif /* CONFIG_MODULES */ return match; } @@ -311,7 +311,7 @@ static int snd_card_init(struct snd_card *card, struct device *parent, } card->dev = parent; card->number = idx; -#ifdef MODULE +#ifdef CONFIG_MODULES WARN_ON(!module); card->module = module; #endif @@ -969,7 +969,7 @@ void snd_card_info_read_oss(struct snd_info_buffer *buffer) #endif -#ifdef MODULE +#ifdef CONFIG_MODULES static void snd_card_module_info_read(struct snd_info_entry *entry, struct snd_info_buffer *buffer) { @@ -997,7 +997,7 @@ int __init snd_card_info_init(void) if (snd_info_register(entry) < 0) return -ENOMEM; /* freed in error path */ -#ifdef MODULE +#ifdef CONFIG_MODULES entry = snd_info_create_module_entry(THIS_MODULE, "modules", NULL); if (!entry) return -ENOMEM;
On Tue, May 21, 2024 at 01:32:37PM +0200, Takashi Iwai wrote: > On Tue, 21 May 2024 12:56:05 +0200, > Xu Yang wrote: > > > > On Mon, May 20, 2024 at 12:29:15PM +0200, Takashi Iwai wrote: > > > On Mon, 20 May 2024 19:03:49 +0200, > > > Xu Yang wrote: > > > > > > > > When remove module snd-usb-audio, snd_card_free_when_closed() will not > > > > release the card resource if the card_dev refcount > 0 and > > > > [...] > > > > > > Then, even the userspace trying to cleanup the resources, kernel will not > > > > touch the released code memory. > > > > > > Hm, it's an interesting report. Could you verify whether it's really > > > hitting a module unload race? The module refcount should have been > > > non-zero when the device is still in use, and it should have prevented > > > the module unloading. > > > > Yes, the race does exist. I enable trace and got below output: > > It seems that snd_usb_audio module refcnt is 0 after insmod completed. So > > it can continue to be removed even it's still in use. > > If no device is opened, it's not really "used", and the driver module > can be unloaded at any time. That's the intended behavior. Hh, I see wireplumber did open the card_dev when it scan card devices. But wireplumber didn't close the card_dev when the scan process completed. > > (snip) > > Then I take some time to check why snd_usb_audio module refcnt is 0 > > even though the card_dev is in use. Finally I got below finding: > > > > I build kernel and module with below configuration: > > > > CONFIG_SOUND=y > > CONFIG_SND=y > > CONFIG_SND_USB=y > > CONFIG_SND_USB_AUDIO=m > > > > Then GCC will add -DMODULE when build snd-usb-audio as module, but will > > not add -DMODULE when build sound/core/*.c. > > > > When insmod snd-usb-audio.ko, it will create a snd card device and call: > > > > snd_card_init() // sound/core/init.c > > > > #ifdef MODULE > > WARN_ON(!module); > > card->module = module; > > #endif > > > > However, MODULE is not defined for sound/core/init.c, then card->module > > will keep NULL pointer. With this results, snd-usb-audio module refcnt > > will not be a non-zero value. > > Ah, it's a good finding! That explains. > > > > Practically seen, replacing snd_card_free_when_closed() with > > > snd_card_free() shouldn't be a big problem, and it'll work in most > > > cases. But there are always some corner cases that might lead to > > > unexpected behavior. So, let's try to analyze more exactly what's > > > happening there at first. > > > > With above finding, we needn't to replace snd_card_free_when_closed() > > with snd_card_free(). We need to find a way to correctly handle module > > refcnt since this should be a normal usecase. > > Right, I guess a simple fix below to replace '#ifdef MODULE' with > '#ifdef CONFIG_MODULES' should work instead? Yeah, it works for me. Will you send a fix for the issue or suggest me send it? ^_^ Thanks, Xu Yang
On Wed, 22 May 2024 05:34:25 +0200, Xu Yang wrote: > > On Tue, May 21, 2024 at 01:32:37PM +0200, Takashi Iwai wrote: > > On Tue, 21 May 2024 12:56:05 +0200, > > Xu Yang wrote: > > > > > > On Mon, May 20, 2024 at 12:29:15PM +0200, Takashi Iwai wrote: > > > > On Mon, 20 May 2024 19:03:49 +0200, > > > > Xu Yang wrote: > > > > > > > > > > When remove module snd-usb-audio, snd_card_free_when_closed() will not > > > > > release the card resource if the card_dev refcount > 0 and > > > > > > [...] > > > > > > > > Then, even the userspace trying to cleanup the resources, kernel will not > > > > > touch the released code memory. > > > > > > > > Hm, it's an interesting report. Could you verify whether it's really > > > > hitting a module unload race? The module refcount should have been > > > > non-zero when the device is still in use, and it should have prevented > > > > the module unloading. > > > > > > Yes, the race does exist. I enable trace and got below output: > > > It seems that snd_usb_audio module refcnt is 0 after insmod completed. So > > > it can continue to be removed even it's still in use. > > > > If no device is opened, it's not really "used", and the driver module > > can be unloaded at any time. That's the intended behavior. > > Hh, I see wireplumber did open the card_dev when it scan card devices. > But wireplumber didn't close the card_dev when the scan process completed. Then rmmod shouldn't have been allowed, it's a consequence of the NULL module pointer, I suppose. > > (snip) > > > Then I take some time to check why snd_usb_audio module refcnt is 0 > > > even though the card_dev is in use. Finally I got below finding: > > > > > > I build kernel and module with below configuration: > > > > > > CONFIG_SOUND=y > > > CONFIG_SND=y > > > CONFIG_SND_USB=y > > > CONFIG_SND_USB_AUDIO=m > > > > > > Then GCC will add -DMODULE when build snd-usb-audio as module, but will > > > not add -DMODULE when build sound/core/*.c. > > > > > > When insmod snd-usb-audio.ko, it will create a snd card device and call: > > > > > > snd_card_init() // sound/core/init.c > > > > > > #ifdef MODULE > > > WARN_ON(!module); > > > card->module = module; > > > #endif > > > > > > However, MODULE is not defined for sound/core/init.c, then card->module > > > will keep NULL pointer. With this results, snd-usb-audio module refcnt > > > will not be a non-zero value. > > > > Ah, it's a good finding! That explains. > > > > > > Practically seen, replacing snd_card_free_when_closed() with > > > > snd_card_free() shouldn't be a big problem, and it'll work in most > > > > cases. But there are always some corner cases that might lead to > > > > unexpected behavior. So, let's try to analyze more exactly what's > > > > happening there at first. > > > > > > With above finding, we needn't to replace snd_card_free_when_closed() > > > with snd_card_free(). We need to find a way to correctly handle module > > > refcnt since this should be a normal usecase. > > > > Right, I guess a simple fix below to replace '#ifdef MODULE' with > > '#ifdef CONFIG_MODULES' should work instead? > > Yeah, it works for me. > Will you send a fix for the issue or suggest me send it? ^_^ I'm going to submit a proper fix patch later. thanks, Takashi
On Wed, 22 May 2024 07:57:24 +0200, Takashi Iwai wrote: > > On Wed, 22 May 2024 05:34:25 +0200, > Xu Yang wrote: > > > > On Tue, May 21, 2024 at 01:32:37PM +0200, Takashi Iwai wrote: > > > On Tue, 21 May 2024 12:56:05 +0200, > > > Xu Yang wrote: > > > > > > > > On Mon, May 20, 2024 at 12:29:15PM +0200, Takashi Iwai wrote: > > > > > On Mon, 20 May 2024 19:03:49 +0200, > > > > > Xu Yang wrote: > > > > > > > > > > > > When remove module snd-usb-audio, snd_card_free_when_closed() will not > > > > > > release the card resource if the card_dev refcount > 0 and > > > > > > > > [...] > > > > > > > > > > Then, even the userspace trying to cleanup the resources, kernel will not > > > > > > touch the released code memory. > > > > > > > > > > Hm, it's an interesting report. Could you verify whether it's really > > > > > hitting a module unload race? The module refcount should have been > > > > > non-zero when the device is still in use, and it should have prevented > > > > > the module unloading. > > > > > > > > Yes, the race does exist. I enable trace and got below output: > > > > It seems that snd_usb_audio module refcnt is 0 after insmod completed. So > > > > it can continue to be removed even it's still in use. > > > > > > If no device is opened, it's not really "used", and the driver module > > > can be unloaded at any time. That's the intended behavior. > > > > Hh, I see wireplumber did open the card_dev when it scan card devices. > > But wireplumber didn't close the card_dev when the scan process completed. > > Then rmmod shouldn't have been allowed, it's a consequence of the NULL > module pointer, I suppose. > > > > (snip) > > > > Then I take some time to check why snd_usb_audio module refcnt is 0 > > > > even though the card_dev is in use. Finally I got below finding: > > > > > > > > I build kernel and module with below configuration: > > > > > > > > CONFIG_SOUND=y > > > > CONFIG_SND=y > > > > CONFIG_SND_USB=y > > > > CONFIG_SND_USB_AUDIO=m > > > > > > > > Then GCC will add -DMODULE when build snd-usb-audio as module, but will > > > > not add -DMODULE when build sound/core/*.c. > > > > > > > > When insmod snd-usb-audio.ko, it will create a snd card device and call: > > > > > > > > snd_card_init() // sound/core/init.c > > > > > > > > #ifdef MODULE > > > > WARN_ON(!module); > > > > card->module = module; > > > > #endif > > > > > > > > However, MODULE is not defined for sound/core/init.c, then card->module > > > > will keep NULL pointer. With this results, snd-usb-audio module refcnt > > > > will not be a non-zero value. > > > > > > Ah, it's a good finding! That explains. > > > > > > > > Practically seen, replacing snd_card_free_when_closed() with > > > > > snd_card_free() shouldn't be a big problem, and it'll work in most > > > > > cases. But there are always some corner cases that might lead to > > > > > unexpected behavior. So, let's try to analyze more exactly what's > > > > > happening there at first. > > > > > > > > With above finding, we needn't to replace snd_card_free_when_closed() > > > > with snd_card_free(). We need to find a way to correctly handle module > > > > refcnt since this should be a normal usecase. > > > > > > Right, I guess a simple fix below to replace '#ifdef MODULE' with > > > '#ifdef CONFIG_MODULES' should work instead? > > > > Yeah, it works for me. > > Will you send a fix for the issue or suggest me send it? ^_^ > > I'm going to submit a proper fix patch later. Looking at the git log again, I noticed that it's rather a regression in commit 81033c6b584b. So at best we should address only this regression by the patch below. Then the rest '#ifdef MODULE' can be replaced with '#ifdef CONFIG_MODULES' in another patch as a different fix / cleanup. The latter changes the behavior slightly (now the proc entry appears even if you build the sound core into kernel). Takashi -- 8< -- Subject: [PATCH] ALSA: core: Fix NULL module pointer assignment at card init The commit 81033c6b584b ("ALSA: core: Warn on empty module") introduced a WARN_ON() for a NULL module pointer passed at snd_card object creation, and it also wraps the code around it with '#ifdef MODULE'. This works in most cases, but the devils are always in details. "MODULE" is defined when the target code (i.e. the sound core) is built as a module; but this doesn't mean that the caller is also built-in or not. Namely, when only the sound core is built-in (CONFIG_SND=y) while the driver is a module (e.g. CONFIG_SND_USB_AUDIO=m), the passed module pointer is ignored even if it's non-NULL and NULL is kept in card->module as consequence. This resulted in the missing module reference up/down at the device open/close, leading to a race with the code execution after the module removal. For addressing the bug, move the assignment of card->module again out of ifdef. The WARN_ON() is still wrapped with ifdef, because the module can be really NULL when all sound drivers are built-in. Note that a NULL module pointer won't caught by ifdef MODULE, but it's only for special kconfigs and this warning is only for debugging, hence it shouldn't be any serious problem. Fixes: 81033c6b584b ("ALSA: core: Warn on empty module") Reported-by: Xu Yang <xu.yang_2@nxp.com> Closes: https://lore.kernel.org/r/20240520170349.2417900-1-xu.yang_2@nxp.com Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/core/init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/core/init.c b/sound/core/init.c index 6b127864a1a3..ac072614d1ea 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -313,8 +313,8 @@ static int snd_card_init(struct snd_card *card, struct device *parent, card->number = idx; #ifdef MODULE WARN_ON(!module); - card->module = module; #endif + card->module = module; INIT_LIST_HEAD(&card->devices); init_rwsem(&card->controls_rwsem); rwlock_init(&card->ctl_files_rwlock);
On Wed, May 22, 2024 at 08:21:24AM +0200, Takashi Iwai wrote: > On Wed, 22 May 2024 07:57:24 +0200, > Takashi Iwai wrote: > > > > On Wed, 22 May 2024 05:34:25 +0200, > > Xu Yang wrote: > > > > > > On Tue, May 21, 2024 at 01:32:37PM +0200, Takashi Iwai wrote: > > > > On Tue, 21 May 2024 12:56:05 +0200, > > > > Xu Yang wrote: > > > > > > > > > > On Mon, May 20, 2024 at 12:29:15PM +0200, Takashi Iwai wrote: > > > > > > On Mon, 20 May 2024 19:03:49 +0200, > > > > > > Xu Yang wrote: > > > > > > > > > > > > > > When remove module snd-usb-audio, snd_card_free_when_closed() will not > > > > > > > release the card resource if the card_dev refcount > 0 and > > > > > > > > > > [...] > > > > > > > > > > > > Then, even the userspace trying to cleanup the resources, kernel will not > > > > > > > touch the released code memory. > > > > > > > > > > > > Hm, it's an interesting report. Could you verify whether it's really > > > > > > hitting a module unload race? The module refcount should have been > > > > > > non-zero when the device is still in use, and it should have prevented > > > > > > the module unloading. > > > > > > > > > > Yes, the race does exist. I enable trace and got below output: > > > > > It seems that snd_usb_audio module refcnt is 0 after insmod completed. So > > > > > it can continue to be removed even it's still in use. > > > > > > > > If no device is opened, it's not really "used", and the driver module > > > > can be unloaded at any time. That's the intended behavior. > > > > > > Hh, I see wireplumber did open the card_dev when it scan card devices. > > > But wireplumber didn't close the card_dev when the scan process completed. > > > > Then rmmod shouldn't have been allowed, it's a consequence of the NULL > > module pointer, I suppose. Yes. > > > > > > (snip) > > > > > Then I take some time to check why snd_usb_audio module refcnt is 0 > > > > > even though the card_dev is in use. Finally I got below finding: > > > > > > > > > > I build kernel and module with below configuration: > > > > > > > > > > CONFIG_SOUND=y > > > > > CONFIG_SND=y > > > > > CONFIG_SND_USB=y > > > > > CONFIG_SND_USB_AUDIO=m > > > > > > > > > > Then GCC will add -DMODULE when build snd-usb-audio as module, but will > > > > > not add -DMODULE when build sound/core/*.c. > > > > > > > > > > When insmod snd-usb-audio.ko, it will create a snd card device and call: > > > > > > > > > > snd_card_init() // sound/core/init.c > > > > > > > > > > #ifdef MODULE > > > > > WARN_ON(!module); > > > > > card->module = module; > > > > > #endif > > > > > > > > > > However, MODULE is not defined for sound/core/init.c, then card->module > > > > > will keep NULL pointer. With this results, snd-usb-audio module refcnt > > > > > will not be a non-zero value. > > > > > > > > Ah, it's a good finding! That explains. > > > > > > > > > > Practically seen, replacing snd_card_free_when_closed() with > > > > > > snd_card_free() shouldn't be a big problem, and it'll work in most > > > > > > cases. But there are always some corner cases that might lead to > > > > > > unexpected behavior. So, let's try to analyze more exactly what's > > > > > > happening there at first. > > > > > > > > > > With above finding, we needn't to replace snd_card_free_when_closed() > > > > > with snd_card_free(). We need to find a way to correctly handle module > > > > > refcnt since this should be a normal usecase. > > > > > > > > Right, I guess a simple fix below to replace '#ifdef MODULE' with > > > > '#ifdef CONFIG_MODULES' should work instead? > > > > > > Yeah, it works for me. > > > Will you send a fix for the issue or suggest me send it? ^_^ > > > > I'm going to submit a proper fix patch later. Thanks in advance. > > Looking at the git log again, I noticed that it's rather a regression > in commit 81033c6b584b. So at best we should address only this > regression by the patch below. Then the rest '#ifdef MODULE' can be > replaced with '#ifdef CONFIG_MODULES' in another patch as a different > fix / cleanup. The latter changes the behavior slightly (now the proc > entry appears even if you build the sound core into kernel). Yes, indeed true. I agree with you. > > > Takashi > > -- 8< -- > Subject: [PATCH] ALSA: core: Fix NULL module pointer assignment at card init > > The commit 81033c6b584b ("ALSA: core: Warn on empty module") > introduced a WARN_ON() for a NULL module pointer passed at snd_card > object creation, and it also wraps the code around it with '#ifdef > MODULE'. This works in most cases, but the devils are always in > details. "MODULE" is defined when the target code (i.e. the sound > core) is built as a module; but this doesn't mean that the caller is > also built-in or not. Namely, when only the sound core is built-in > (CONFIG_SND=y) while the driver is a module > (e.g. CONFIG_SND_USB_AUDIO=m), the passed module pointer is ignored > even if it's non-NULL and NULL is kept in card->module as consequence. > This resulted in the missing module reference up/down at the device > open/close, leading to a race with the code execution after the module > removal. > > For addressing the bug, move the assignment of card->module again out > of ifdef. The WARN_ON() is still wrapped with ifdef, because the > module can be really NULL when all sound drivers are built-in. > > Note that a NULL module pointer won't caught by ifdef MODULE, but it's > only for special kconfigs and this warning is only for debugging, > hence it shouldn't be any serious problem. > > Fixes: 81033c6b584b ("ALSA: core: Warn on empty module") > Reported-by: Xu Yang <xu.yang_2@nxp.com> > Closes: https://lore.kernel.org/r/20240520170349.2417900-1-xu.yang_2@nxp.com > Cc: <stable@vger.kernel.org> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/core/init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/core/init.c b/sound/core/init.c > index 6b127864a1a3..ac072614d1ea 100644 > --- a/sound/core/init.c > +++ b/sound/core/init.c > @@ -313,8 +313,8 @@ static int snd_card_init(struct snd_card *card, struct device *parent, > card->number = idx; > #ifdef MODULE > WARN_ON(!module); > - card->module = module; > #endif > + card->module = module; > INIT_LIST_HEAD(&card->devices); > init_rwsem(&card->controls_rwsem); > rwlock_init(&card->ctl_files_rwlock); It's okay for me. I'll give a Tested-by when receive a formal patch. Thanks, Xu Yang > -- > 2.43.0 >
diff --git a/sound/usb/card.c b/sound/usb/card.c index 1b2edc0fd2e9..5e799f147eb5 100644 --- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -992,7 +992,7 @@ static void usb_audio_disconnect(struct usb_interface *intf) if (chip->num_interfaces <= 0) { usb_chip[chip->index] = NULL; mutex_unlock(®ister_mutex); - snd_card_free_when_closed(card); + snd_card_free(card); } else { mutex_unlock(®ister_mutex); }
When remove module snd-usb-audio, snd_card_free_when_closed() will not release the card resource if the card_dev refcount > 0 and usb_audio_disconnect() will return directly, finally kernel will release the module resource. Then if the userspace continue to cleanup sound card resources, such as close controlC0, the closing path will touch this modules' code, unfortunately it just got released and the kernel will dump below error: [ 183.450073] Internal error: Oops: 0000000086000007 [#1] PREEMPT SMP [ 183.456345] Modules linked in: snd_usbmidi_lib snd_hwdep [last unloaded: snd_usb_audio] [ 183.464373] CPU: 0 PID: 537 Comm: wireplumber Not tainted 6.6.23-06215-gc5317d88b3ec #708 [ 183.472552] Hardware name: NXP i.MX93 11X11 EVK board (DT) [ 183.478039] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 183.485004] pc : 0xffff80007c19a3b0 [ 183.488507] lr : snd_pcm_dev_free+0x3c/0x70 [ 183.492708] sp : ffff800085c77af0 [ 183.496030] x29: ffff800085c77af0 x28: dead000000000122 x27: ffff0000079f8090 [ 183.503188] x26: ffff0000079f8090 x25: ffff00000e78c270 x24: ffff00000e78c000 [ 183.510336] x23: ffff00000e78c1a8 x22: ffff00000e78c000 x21: ffff00000b6a2718 [ 183.517486] x20: ffff800082608180 x19: ffff00000d2d7400 x18: 0000000000000000 [ 183.524635] x17: 0000000000000000 x16: 0000000000000000 x15: 0000ffffa0000b90 [ 183.531786] x14: 0000000000000000 x13: 0000000000000000 x12: ffff700010b8ef49 [ 183.538939] x11: 1ffff00010b8ef48 x10: ffff700010b8ef48 x9 : 0000000000000000 [ 183.546086] x8 : ffff800085c77a50 x7 : ffff00006ccd77b0 x6 : 0000000000000003 [ 183.553236] x5 : 00000000000000c0 x4 : ffff00000ea57000 x3 : dfff800000000000 [ 183.560386] x2 : 0000000000000007 x1 : ffff80007c19a3b0 x0 : ffff00000d2d7400 [ 183.567539] Call trace: [ 183.569992] 0xffff80007c19a3b0 [ 183.573134] __snd_device_free+0x94/0x16c [ 183.577156] snd_device_free_all+0x70/0xe8 [ 183.581258] release_card_device+0x30/0xc0 [ 183.585363] device_release+0x50/0x10c [ 183.589124] kobject_put+0xe0/0x184 [ 183.592634] put_device+0x14/0x24 [ 183.595954] snd_card_file_remove+0x158/0x22c [ 183.600322] snd_ctl_release+0x174/0x194 [ 183.604256] snd_disconnect_release+0x128/0x178 [ 183.608798] __fput+0x160/0x3d0 [ 183.611955] __fput_sync+0x74/0x84 [ 183.615370] __arm64_sys_close+0x4c/0x8c [ 183.619302] invoke_syscall+0x60/0x184 [ 183.623072] el0_svc_common.constprop.0+0x114/0x13c [ 183.627960] do_el0_svc+0x30/0x40 [ 183.631288] el0_svc+0x38/0x70 [ 183.634356] el0t_64_sync_handler+0x120/0x12c [ 183.638724] el0t_64_sync+0x190/0x194 [ 183.642403] Code: ???????? ???????? ???????? ???????? (????????) [ 183.648497] ---[ end trace 0000000000000000 ]--- To fix the issue, use snd_card_free() to release all resources (including all files attached to the card_dev) instead snd_card_free_when_closed(). Then, even the userspace trying to cleanup the resources, kernel will not touch the released code memory. Signed-off-by: Xu Yang <xu.yang_2@nxp.com> --- sound/usb/card.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)