| Message ID | tencent_9E3DBD3732961C37FC4AEC74E3763367E209@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: fix a task hung in snd_card_free | expand |
On Wed, 06 Nov 2024 03:15:49 +0100, Edward Adam Davis wrote: > > task 1: snd ctrl will add card_dev ref count and can't call close to dec it, > it is blocked waiting for task 2 to release the USB dev lock. > > task 2: usb dev lock has been locked by hung task (here is usb_disconnect), > it is hung waiting for task 1 to exit and release card_dev. > > Adjust the USB lock acquisition method to non-blocking in ioctl to avoid > hang when the USB connection is closed. I'm afraid that this change would break things too badly. i.e. changing the blocking behavior to non-blocking is no-go. > Reported-and-tested-by: syzbot+73582d08864d8268b6fd@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=73582d08864d8268b6fd This particular syzkaller entry can be fixed rather by replacing snd_card_free() in snd_usx2y_disconnect() with snd_card_free_when_closed() like other USB audio drivers, something like below. Judging from the git log, it had been with snd_card_free_in_thread(), but was switch to snd_card_free() around year 2005. Meanwhile the handling of async card release got improved, and it's very likely OK to use snd_card_free_when_closed() there with the recent kernel. thanks, Takashi -- 8< -- --- a/sound/usb/usx2y/usbusx2y.c +++ b/sound/usb/usx2y/usbusx2y.c @@ -422,7 +422,7 @@ static void snd_usx2y_disconnect(struct usb_interface *intf) } if (usx2y->us428ctls_sharedmem) wake_up(&usx2y->us428ctls_wait_queue_head); - snd_card_free(card); + snd_card_free_when_closed(card); } static int snd_usx2y_probe(struct usb_interface *intf,
On Tue, 12 Nov 2024 17:04:04 +0100, Takashi Iwai wrote: > On Wed, 06 Nov 2024 03:15:49 +0100, > Edward Adam Davis wrote: > > > > task 1: snd ctrl will add card_dev ref count and can't call close to dec it, > > it is blocked waiting for task 2 to release the USB dev lock. > > > > task 2: usb dev lock has been locked by hung task (here is usb_disconnect), > > it is hung waiting for task 1 to exit and release card_dev. > > > > Adjust the USB lock acquisition method to non-blocking in ioctl to avoid > > hang when the USB connection is closed. > > I'm afraid that this change would break things too badly. > i.e. changing the blocking behavior to non-blocking is no-go. > > > Reported-and-tested-by: syzbot+73582d08864d8268b6fd@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=73582d08864d8268b6fd > > This particular syzkaller entry can be fixed rather by replacing > snd_card_free() in snd_usx2y_disconnect() with > snd_card_free_when_closed() like other USB audio drivers, something > like below. > > Judging from the git log, it had been with snd_card_free_in_thread(), > but was switch to snd_card_free() around year 2005. Meanwhile the > handling of async card release got improved, and it's very likely OK > to use snd_card_free_when_closed() there with the recent kernel. The snd_card instance will be released in snd_card_do_free(). So, if snd_card_free_when_closed() is used to replace snd_card_free(), who will release the snd_card instance? BR, Edward > > > thanks, > > Takashi > > -- 8< -- > --- a/sound/usb/usx2y/usbusx2y.c > +++ b/sound/usb/usx2y/usbusx2y.c > @@ -422,7 +422,7 @@ static void snd_usx2y_disconnect(struct usb_interface *intf) > } > if (usx2y->us428ctls_sharedmem) > wake_up(&usx2y->us428ctls_wait_queue_head); > - snd_card_free(card); > + snd_card_free_when_closed(card); > } > > static int snd_usx2y_probe(struct usb_interface *intf,
On Wed, 13 Nov 2024 02:48:49 +0100, Edward Adam Davis wrote: > > On Tue, 12 Nov 2024 17:04:04 +0100, Takashi Iwai wrote: > > On Wed, 06 Nov 2024 03:15:49 +0100, > > Edward Adam Davis wrote: > > > > > > task 1: snd ctrl will add card_dev ref count and can't call close to dec it, > > > it is blocked waiting for task 2 to release the USB dev lock. > > > > > > task 2: usb dev lock has been locked by hung task (here is usb_disconnect), > > > it is hung waiting for task 1 to exit and release card_dev. > > > > > > Adjust the USB lock acquisition method to non-blocking in ioctl to avoid > > > hang when the USB connection is closed. > > > > I'm afraid that this change would break things too badly. > > i.e. changing the blocking behavior to non-blocking is no-go. > > > > > Reported-and-tested-by: syzbot+73582d08864d8268b6fd@syzkaller.appspotmail.com > > > Closes: https://syzkaller.appspot.com/bug?extid=73582d08864d8268b6fd > > > > This particular syzkaller entry can be fixed rather by replacing > > snd_card_free() in snd_usx2y_disconnect() with > > snd_card_free_when_closed() like other USB audio drivers, something > > like below. > > > > Judging from the git log, it had been with snd_card_free_in_thread(), > > but was switch to snd_card_free() around year 2005. Meanwhile the > > handling of async card release got improved, and it's very likely OK > > to use snd_card_free_when_closed() there with the recent kernel. > The snd_card instance will be released in snd_card_do_free(). > So, if snd_card_free_when_closed() is used to replace snd_card_free(), who will release the snd_card instance? Via the release callback of the card device object, which is triggered at the last close by refcounting. Takashi > > BR, > Edward > > > > > > thanks, > > > > Takashi > > > > -- 8< -- > > --- a/sound/usb/usx2y/usbusx2y.c > > +++ b/sound/usb/usx2y/usbusx2y.c > > @@ -422,7 +422,7 @@ static void snd_usx2y_disconnect(struct usb_interface *intf) > > } > > if (usx2y->us428ctls_sharedmem) > > wake_up(&usx2y->us428ctls_wait_queue_head); > > - snd_card_free(card); > > + snd_card_free_when_closed(card); > > } > > > > static int snd_usx2y_probe(struct usb_interface *intf, > >
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index 3beb6a862e80..dd037dc4cb37 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -2605,7 +2605,8 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd, if (!(file->f_mode & FMODE_WRITE)) return -EPERM; - usb_lock_device(dev); + if (!usb_trylock_device(dev)) + return -EBUSY; /* Reap operations are allowed even after disconnection */ switch (cmd) {
task 1: snd ctrl will add card_dev ref count and can't call close to dec it, it is blocked waiting for task 2 to release the USB dev lock. task 2: usb dev lock has been locked by hung task (here is usb_disconnect), it is hung waiting for task 1 to exit and release card_dev. Adjust the USB lock acquisition method to non-blocking in ioctl to avoid hang when the USB connection is closed. Reported-and-tested-by: syzbot+73582d08864d8268b6fd@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=73582d08864d8268b6fd Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- drivers/usb/core/devio.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)