Re: possible deadlock in usb_deregister_dev

[Andrey Konovalov <andreyknvl@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]

On Fri, Aug 9, 2019 at 9:32 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, 9 Aug 2019, syzbot wrote:
>
> > syzbot has found a reproducer for the following crash on:
> >
> > HEAD commit:    e96407b4 usb-fuzzer: main usb gadget fuzzer driver
> > git tree:       https://github.com/google/kasan.git usb-fuzzer
> > console output: https://syzkaller.appspot.com/x/log.txt?x=15bf780e600000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
> > dashboard link: https://syzkaller.appspot.com/bug?extid=a64a382964bf6c71a9c0
> > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16787574600000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=136cc4d2600000
> >
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+a64a382964bf6c71a9c0@syzkaller.appspotmail.com
> >
> > usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
> > usb 1-1: config 0 descriptor??
> > iowarrior 1-1:0.236: IOWarrior product=0x1501, serial= interface=236 now
> > attached to iowarrior0
> > usb 1-1: USB disconnect, device number 2
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 5.3.0-rc2+ #25 Not tainted
> > ------------------------------------------------------
> > kworker/0:1/12 is trying to acquire lock:
> > 00000000cd63e8f1 (minor_rwsem){++++}, at: usb_deregister_dev
> > drivers/usb/core/file.c:238 [inline]
> > 00000000cd63e8f1 (minor_rwsem){++++}, at: usb_deregister_dev+0x61/0x270
> > drivers/usb/core/file.c:230
> >
> > but task is already holding lock:
> > 000000001d1989ef (iowarrior_open_disc_lock){+.+.}, at:
> > iowarrior_disconnect+0x45/0x2c0 drivers/usb/misc/iowarrior.c:867
> >
> > which lock already depends on the new lock.
>
> https://syzkaller.appspot.com/bug?extid=ca52394faa436d8131df is
> undoubtedly a duplicate of this.

I've marked it as one, thanks!

Now that we have a reproducer, let's retry Oliver's fix:

#syz test: https://github.com/google/kasan.git e96407b4

[-- Attachment #2: iowarrior.patch --]
[-- Type: text/x-patch, Size: 952 bytes --]

diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index ba05dd80a020..f5bed9f29e56 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -866,19 +866,20 @@ static void iowarrior_disconnect(struct usb_interface *interface)
 	dev = usb_get_intfdata(interface);
 	mutex_lock(&iowarrior_open_disc_lock);
 	usb_set_intfdata(interface, NULL);
+	/* prevent device read, write and ioctl */
+	dev->present = 0;
 
 	minor = dev->minor;
+	mutex_unlock(&iowarrior_open_disc_lock);
+	/* give back our minor - this will call close() locks need to be dropped at this point*/
 
-	/* give back our minor */
 	usb_deregister_dev(interface, &iowarrior_class);
 
 	mutex_lock(&dev->mutex);
 
 	/* prevent device read, write and ioctl */
-	dev->present = 0;
 
 	mutex_unlock(&dev->mutex);
-	mutex_unlock(&iowarrior_open_disc_lock);
 
 	if (dev->opened) {
 		/* There is a process that holds a filedescriptor to the device ,