1. Patchew
  2. linux
  3. View series

[PATCH] usb: fix a task hung in snd_card_free

Edward Adam Davis posted 1 patch 2 weeks, 4 days ago 
drivers/usb/core/devio.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] usb: fix a task hung in snd_card_free
Posted by Edward Adam Davis 2 weeks, 4 days ago 
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(-)

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) {
-- 
2.43.0
Re: [PATCH] usb: fix a task hung in snd_card_free
Posted by Takashi Iwai 1 week, 4 days ago 
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,
Re: [PATCH] usb: fix a task hung in snd_card_free
Posted by Edward Adam Davis 1 week, 4 days ago 
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,
Re: [PATCH] usb: fix a task hung in snd_card_free
Posted by Takashi Iwai 1 week, 3 days ago 
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,
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.