Re: [PATCH v2] ALSA: bcd2000: Wait for open files before freeing the MIDI URBs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: bcd2000: deadlock in bcd2000_disconnect() via snd_card_disconnect_sync()
--

commit b86f69d1ff1d65cb943f4d36efce05abcc0a498d
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

ALSA: bcd2000: Wait for open files before freeing the MIDI URBs

This commit replaces snd_card_disconnect() with snd_card_disconnect_sync() in
bcd2000_disconnect() so that open rawmidi files are closed before freeing the
MIDI URBs.

> diff --git a/sound/usb/bcd2000/bcd2000.c b/sound/usb/bcd2000/bcd2000.c
> index 2bd49bf827489..a5e8ea92532c5 100644
> --- a/sound/usb/bcd2000/bcd2000.c
> +++ b/sound/usb/bcd2000/bcd2000.c
> @@ -451,8 +451,13 @@ static void bcd2000_disconnect(struct usb_interface *interface)
>  
>  	guard(mutex)(&devices_mutex);
>  
> -	/* make sure that userspace cannot create new requests */
> -	snd_card_disconnect(bcd2k->card);
> +	/*
> +	 * Make sure that userspace cannot create new requests, and wait
> +	 * until all already-open files are closed so that the rawmidi
> +	 * close callbacks (which may kill the out URB) run while the
> +	 * URBs are still valid.
> +	 */
> +	snd_card_disconnect_sync(bcd2k->card);

[Severity: High]
Can calling snd_card_disconnect_sync() inside bcd2000_disconnect() deadlock or
stall USB disconnect processing?

In sound/core/init.c, snd_card_disconnect_sync() sleeps uninterruptibly until
all open file descriptors on the card are closed:

sound/core/init.c:snd_card_disconnect_sync() {
	snd_card_disconnect(card);

	guard(spinlock_irq)(&card->files_lock);
	wait_event_lock_irq(card->remove_sleep,
			    list_empty(&card->files_list),
			    card->files_lock);
}

However, bcd2000_disconnect() is invoked by usb_unbind_interface() with
device_lock(&interface->dev) held, and from usb_disconnect() ->
usb_disable_device() or usbfs ioctls (usbdev_do_ioctl()) with
usb_lock_device(udev) held, and then acquires devices_mutex.

If a process holding an open ALSA fd on bcd2k->card unbinds the driver via
sysfs (/sys/bus/usb/drivers/snd-bcd2000/unbind) or issues a usbfs ioctl that
unbinds the interface (such as USBDEVFS_DISCONNECT, USBDEVFS_DISCONNECT_CLAIM,
USBDEVFS_RESET, or USBDEVFS_SETCONFIGURATION), bcd2000_disconnect() runs
synchronously in that process's syscall context and sleeps in
snd_card_disconnect_sync() waiting for that same process to close its ALSA fd.

An ABBA deadlock with usb_lock_device(udev) is also possible during
usb_disconnect(): the hub worker holds usb_lock_device(udev) while waiting in
snd_card_disconnect_sync() for open ALSA fds to close. If a process holds both
a usbfs fd (/dev/bus/usb/...) with a lower fd number and an ALSA fd on the
same device and exits (do_exit() -> close_files()) or closes the usbfs fd
first, usbdev_release() blocks on usb_lock_device(udev) before the process can
close the ALSA fd.

Even on a normal physical unplug, waiting in bcd2000_disconnect() for
userspace to close all ALSA fds blocks hub_event() on hub_wq in
TASK_UNINTERRUPTIBLE while holding devices_mutex and before device_del() emits
remove uevents, defeating the asynchronous teardown intended by
snd_card_free_when_closed(bcd2k->card) at the end of bcd2000_disconnect().

Instead of blocking in snd_card_disconnect_sync(), would it be safer to keep
snd_card_disconnect() and Either serialize midi_out_urb in
bcd2000_midi_output_close() or defer usb_free_urb() to a card->private_free
callback that runs when snd_card_free_when_closed() finishes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031184405.15307-1-25031212351@stu.xidian.edu.cn?part=1