* possible deadlock in open_rio @ 2019-08-01 15:28 syzbot 2019-08-02 20:51 ` Alan Stern 2019-08-06 19:13 ` Alan Stern 0 siblings, 2 replies; 14+ messages in thread From: syzbot @ 2019-08-01 15:28 UTC (permalink / raw) To: andreyknvl, gregkh, linux-kernel, linux-usb, miquel, rio500-users, syzkaller-bugs Hello, syzbot found the following crash on: HEAD commit: 7f7867ff 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=136b6aec600000 kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a compiler: gcc (GCC) 9.0.0 20181231 (experimental) Unfortunately, I don't have any reproducer for this crash yet. IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com ====================================================== WARNING: possible circular locking dependency detected 5.3.0-rc2+ #23 Not tainted ------------------------------------------------------ syz-executor.2/20386 is trying to acquire lock: 00000000772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0 drivers/usb/misc/rio500.c:64 but task is already holding lock: 00000000d3e8f4b9 (minor_rwsem){++++}, at: usb_open+0x23/0x270 drivers/usb/core/file.c:39 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (minor_rwsem){++++}: down_write+0x92/0x150 kernel/locking/rwsem.c:1500 usb_register_dev drivers/usb/core/file.c:187 [inline] usb_register_dev+0x131/0x6a0 drivers/usb/core/file.c:156 probe_rio.cold+0x53/0x21d drivers/usb/misc/rio500.c:468 usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361 really_probe+0x281/0x650 drivers/base/dd.c:548 driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709 __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816 bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454 __device_attach+0x217/0x360 drivers/base/dd.c:882 bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514 device_add+0xae6/0x16f0 drivers/base/core.c:2114 usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023 generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210 usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266 really_probe+0x281/0x650 drivers/base/dd.c:548 driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709 __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816 bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454 __device_attach+0x217/0x360 drivers/base/dd.c:882 bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514 device_add+0xae6/0x16f0 drivers/base/core.c:2114 usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536 hub_port_connect drivers/usb/core/hub.c:5098 [inline] hub_port_connect_change drivers/usb/core/hub.c:5213 [inline] port_event drivers/usb/core/hub.c:5359 [inline] hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441 process_one_work+0x92b/0x1530 kernel/workqueue.c:2269 worker_thread+0x96/0xe20 kernel/workqueue.c:2415 kthread+0x318/0x420 kernel/kthread.c:255 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352 -> #0 (rio500_mutex){+.+.}: check_prev_add kernel/locking/lockdep.c:2405 [inline] check_prevs_add kernel/locking/lockdep.c:2507 [inline] validate_chain kernel/locking/lockdep.c:2897 [inline] __lock_acquire+0x1f7c/0x3b50 kernel/locking/lockdep.c:3880 lock_acquire+0x127/0x320 kernel/locking/lockdep.c:4412 __mutex_lock_common kernel/locking/mutex.c:930 [inline] __mutex_lock+0x158/0x1360 kernel/locking/mutex.c:1077 open_rio+0x16/0xc0 drivers/usb/misc/rio500.c:64 usb_open+0x1df/0x270 drivers/usb/core/file.c:48 chrdev_open+0x219/0x5c0 fs/char_dev.c:414 do_dentry_open+0x494/0x1120 fs/open.c:797 do_last fs/namei.c:3416 [inline] path_openat+0x1430/0x3f50 fs/namei.c:3533 do_filp_open+0x1a1/0x280 fs/namei.c:3563 do_sys_open+0x3c0/0x580 fs/open.c:1089 do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296 entry_SYSCALL_64_after_hwframe+0x49/0xbe other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(minor_rwsem); lock(rio500_mutex); lock(minor_rwsem); lock(rio500_mutex); *** DEADLOCK *** 1 lock held by syz-executor.2/20386: #0: 00000000d3e8f4b9 (minor_rwsem){++++}, at: usb_open+0x23/0x270 drivers/usb/core/file.c:39 stack backtrace: CPU: 1 PID: 20386 Comm: syz-executor.2 Not tainted 5.3.0-rc2+ #23 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0xca/0x13e lib/dump_stack.c:113 check_noncircular+0x345/0x3e0 kernel/locking/lockdep.c:1741 check_prev_add kernel/locking/lockdep.c:2405 [inline] check_prevs_add kernel/locking/lockdep.c:2507 [inline] validate_chain kernel/locking/lockdep.c:2897 [inline] __lock_acquire+0x1f7c/0x3b50 kernel/locking/lockdep.c:3880 lock_acquire+0x127/0x320 kernel/locking/lockdep.c:4412 __mutex_lock_common kernel/locking/mutex.c:930 [inline] __mutex_lock+0x158/0x1360 kernel/locking/mutex.c:1077 open_rio+0x16/0xc0 drivers/usb/misc/rio500.c:64 usb_open+0x1df/0x270 drivers/usb/core/file.c:48 chrdev_open+0x219/0x5c0 fs/char_dev.c:414 do_dentry_open+0x494/0x1120 fs/open.c:797 do_last fs/namei.c:3416 [inline] path_openat+0x1430/0x3f50 fs/namei.c:3533 do_filp_open+0x1a1/0x280 fs/namei.c:3563 do_sys_open+0x3c0/0x580 fs/open.c:1089 do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x413711 Code: 75 14 b8 02 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 04 19 00 00 c3 48 83 ec 08 e8 0a fa ff ff 48 89 04 24 b8 02 00 00 00 0f 05 <48> 8b 3c 24 48 89 c2 e8 53 fa ff ff 48 89 d0 48 83 c4 08 48 3d 01 RSP: 002b:00007f26383357a0 EFLAGS: 00000293 ORIG_RAX: 0000000000000002 RAX: ffffffffffffffda RBX: 6666666666666667 RCX: 0000000000413711 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00007f2638335850 RBP: 000000000075bf20 R08: 000000000000000f R09: 0000000000000000 R10: ffffffffffffffff R11: 0000000000000293 R12: 00007f26383366d4 R13: 00000000004c8bee R14: 00000000004dfa68 R15: 00000000ffffffff usb 5-1: Rio opened. --- This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this bug report. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: possible deadlock in open_rio 2019-08-01 15:28 possible deadlock in open_rio syzbot @ 2019-08-02 20:51 ` Alan Stern 2019-08-06 19:13 ` Alan Stern 1 sibling, 0 replies; 14+ messages in thread From: Alan Stern @ 2019-08-02 20:51 UTC (permalink / raw) To: Oliver Neukum, syzbot Cc: andreyknvl, gregkh, Kernel development list, USB list, miquel, rio500-users, syzkaller-bugs On Thu, 1 Aug 2019, syzbot wrote: > Hello, > > syzbot found the following crash on: > > HEAD commit: 7f7867ff 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=136b6aec600000 > kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 > dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > Unfortunately, I don't have any reproducer for this crash yet. > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com > > ====================================================== > WARNING: possible circular locking dependency detected > 5.3.0-rc2+ #23 Not tainted > ------------------------------------------------------ > syz-executor.2/20386 is trying to acquire lock: > 00000000772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0 > drivers/usb/misc/rio500.c:64 > > but task is already holding lock: > 00000000d3e8f4b9 (minor_rwsem){++++}, at: usb_open+0x23/0x270 > drivers/usb/core/file.c:39 > > which lock already depends on the new lock. > > > the existing dependency chain (in reverse order) is: > > -> #1 (minor_rwsem){++++}: > down_write+0x92/0x150 kernel/locking/rwsem.c:1500 > usb_register_dev drivers/usb/core/file.c:187 [inline] > usb_register_dev+0x131/0x6a0 drivers/usb/core/file.c:156 > probe_rio.cold+0x53/0x21d drivers/usb/misc/rio500.c:468 This was caused by Oliver's commit 3864d33943b4 ("USB: rio500: refuse more than one device at a time"). It added mutex_lock(&rio500_mutex); to probe_rio(). I guess it will be necessary to add another mutex to fix this. Alan Stern ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: possible deadlock in open_rio 2019-08-01 15:28 possible deadlock in open_rio syzbot 2019-08-02 20:51 ` Alan Stern @ 2019-08-06 19:13 ` Alan Stern 2019-08-07 13:37 ` Oliver Neukum 2019-08-07 13:53 ` Andrey Konovalov 1 sibling, 2 replies; 14+ messages in thread From: Alan Stern @ 2019-08-06 19:13 UTC (permalink / raw) To: syzbot Cc: andreyknvl, gregkh, linux-kernel, linux-usb, miquel, rio500-users, syzkaller-bugs On Thu, 1 Aug 2019, syzbot wrote: > Hello, > > syzbot found the following crash on: > > HEAD commit: 7f7867ff 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=136b6aec600000 > kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 > dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > Unfortunately, I don't have any reproducer for this crash yet. > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com > > ====================================================== > WARNING: possible circular locking dependency detected > 5.3.0-rc2+ #23 Not tainted > ------------------------------------------------------ Andrey: This should be completely reproducible, since it's a simple ABBA locking violation. Maybe just introducing a time delay (to avoid races and give the open() call time to run) between the gadget creation and gadget removal would be enough to do it. Is there any way you can test this? Alan Stern ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: possible deadlock in open_rio 2019-08-06 19:13 ` Alan Stern @ 2019-08-07 13:37 ` Oliver Neukum 2019-08-07 14:07 ` Alan Stern 2019-08-07 13:53 ` Andrey Konovalov 1 sibling, 1 reply; 14+ messages in thread From: Oliver Neukum @ 2019-08-07 13:37 UTC (permalink / raw) To: Alan Stern, syzbot Cc: miquel, andreyknvl, syzkaller-bugs, gregkh, rio500-users, linux-kernel, linux-usb Am Dienstag, den 06.08.2019, 15:13 -0400 schrieb Alan Stern: > On Thu, 1 Aug 2019, syzbot wrote: > > > Hello, > > > > syzbot found the following crash on: > > > > HEAD commit: 7f7867ff 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=136b6aec600000 > > kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 > > dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com > > > > ====================================================== > > WARNING: possible circular locking dependency detected > > 5.3.0-rc2+ #23 Not tainted > > ------------------------------------------------------ > > Andrey: > > This should be completely reproducible, since it's a simple ABBA > locking violation. Maybe just introducing a time delay (to avoid races > and give the open() call time to run) between the gadget creation and > gadget removal would be enough to do it. Hi, technically yes. However in practical terms the straight revert I sent out yesterday should fix it. Regards Oliver ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: possible deadlock in open_rio 2019-08-07 13:37 ` Oliver Neukum @ 2019-08-07 14:07 ` Alan Stern 0 siblings, 0 replies; 14+ messages in thread From: Alan Stern @ 2019-08-07 14:07 UTC (permalink / raw) To: Oliver Neukum Cc: syzbot, miquel, andreyknvl, syzkaller-bugs, gregkh, rio500-users, linux-kernel, linux-usb On Wed, 7 Aug 2019, Oliver Neukum wrote: > Am Dienstag, den 06.08.2019, 15:13 -0400 schrieb Alan Stern: > > On Thu, 1 Aug 2019, syzbot wrote: > > > > > Hello, > > > > > > syzbot found the following crash on: > > > > > > HEAD commit: 7f7867ff 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=136b6aec600000 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 > > > dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com > > > > > > ====================================================== > > > WARNING: possible circular locking dependency detected > > > 5.3.0-rc2+ #23 Not tainted > > > ------------------------------------------------------ > > > > Andrey: > > > > This should be completely reproducible, since it's a simple ABBA > > locking violation. Maybe just introducing a time delay (to avoid races > > and give the open() call time to run) between the gadget creation and > > gadget removal would be enough to do it. > > Hi, > > technically yes. However in practical terms the straight revert I sent > out yesterday should fix it. I didn't see the revert, and it doesn't appear to have reached the mailing list archive. Can you post it again? Alan Stern PS: syzbot reported a similar lock inversion problem (involving two mutexes rather than just one) in drivers/usb/misc/iowarrior.c. Probably the two drivers need similar fixes. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: possible deadlock in open_rio 2019-08-06 19:13 ` Alan Stern 2019-08-07 13:37 ` Oliver Neukum @ 2019-08-07 13:53 ` Andrey Konovalov 2019-08-07 14:01 ` Alan Stern 1 sibling, 1 reply; 14+ messages in thread From: Andrey Konovalov @ 2019-08-07 13:53 UTC (permalink / raw) To: Alan Stern Cc: syzbot, Greg Kroah-Hartman, LKML, USB list, Cesar Miquel, rio500-users, syzkaller-bugs On Tue, Aug 6, 2019 at 9:13 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, 1 Aug 2019, syzbot wrote: > > > Hello, > > > > syzbot found the following crash on: > > > > HEAD commit: 7f7867ff 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=136b6aec600000 > > kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 > > dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com > > > > ====================================================== > > WARNING: possible circular locking dependency detected > > 5.3.0-rc2+ #23 Not tainted > > ------------------------------------------------------ > > Andrey: > > This should be completely reproducible, since it's a simple ABBA > locking violation. Maybe just introducing a time delay (to avoid races > and give the open() call time to run) between the gadget creation and > gadget removal would be enough to do it. I've tried some simple approaches to reproducing this, but failed. Should this require two rio500 devices to trigger? > > Is there any way you can test this? Not yet. > > Alan Stern > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: possible deadlock in open_rio 2019-08-07 13:53 ` Andrey Konovalov @ 2019-08-07 14:01 ` Alan Stern 2019-08-07 14:24 ` Andrey Konovalov 0 siblings, 1 reply; 14+ messages in thread From: Alan Stern @ 2019-08-07 14:01 UTC (permalink / raw) To: Andrey Konovalov Cc: syzbot, Greg Kroah-Hartman, LKML, USB list, Cesar Miquel, rio500-users, syzkaller-bugs On Wed, 7 Aug 2019, Andrey Konovalov wrote: > On Tue, Aug 6, 2019 at 9:13 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Thu, 1 Aug 2019, syzbot wrote: > > > > > Hello, > > > > > > syzbot found the following crash on: > > > > > > HEAD commit: 7f7867ff 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=136b6aec600000 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 > > > dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com > > > > > > ====================================================== > > > WARNING: possible circular locking dependency detected > > > 5.3.0-rc2+ #23 Not tainted > > > ------------------------------------------------------ > > > > Andrey: > > > > This should be completely reproducible, since it's a simple ABBA > > locking violation. Maybe just introducing a time delay (to avoid races > > and give the open() call time to run) between the gadget creation and > > gadget removal would be enough to do it. > > I've tried some simple approaches to reproducing this, but failed. > Should this require two rio500 devices to trigger? No, one device should be enough. Just plug it in and then try to open the character device file. Alan Stern > > Is there any way you can test this? > > Not yet. > > > > > Alan Stern ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: possible deadlock in open_rio 2019-08-07 14:01 ` Alan Stern @ 2019-08-07 14:24 ` Andrey Konovalov 2019-08-07 14:34 ` Andrey Konovalov 2019-08-07 14:39 ` Alan Stern 0 siblings, 2 replies; 14+ messages in thread From: Andrey Konovalov @ 2019-08-07 14:24 UTC (permalink / raw) To: Alan Stern Cc: syzbot, Greg Kroah-Hartman, LKML, USB list, Cesar Miquel, rio500-users, syzkaller-bugs On Wed, Aug 7, 2019 at 4:01 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, 7 Aug 2019, Andrey Konovalov wrote: > > > On Tue, Aug 6, 2019 at 9:13 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Thu, 1 Aug 2019, syzbot wrote: > > > > > > > Hello, > > > > > > > > syzbot found the following crash on: > > > > > > > > HEAD commit: 7f7867ff 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=136b6aec600000 > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a > > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com > > > > > > > > ====================================================== > > > > WARNING: possible circular locking dependency detected > > > > 5.3.0-rc2+ #23 Not tainted > > > > ------------------------------------------------------ > > > > > > Andrey: > > > > > > This should be completely reproducible, since it's a simple ABBA > > > locking violation. Maybe just introducing a time delay (to avoid races > > > and give the open() call time to run) between the gadget creation and > > > gadget removal would be enough to do it. > > > > I've tried some simple approaches to reproducing this, but failed. > > Should this require two rio500 devices to trigger? > > No, one device should be enough. Just plug it in and then try to open > the character device file. OK, I've reproduced it, so I can test a patch manually. The reason syzbot couldn't do that, is because it doesn't open character devices. Right now the USB fuzzing instance only opens /dev/input*, /dev/hidraw* and /dev/usb/hiddev* (only the devices that are created by USB HID devices as I've been working on adding USB HID targeted fuzzing support lately). I guess we should open /dev/chr/* as well. The problem is that there 300+ devices there even without connecting USB devices and opening them blindly probably won't work. Is there a way to know which character devices are created by USB devices? Maybe they are exposed over /sys/bus/usb or via some other way? > > Alan Stern > > > > Is there any way you can test this? > > > > Not yet. > > > > > > > > Alan Stern > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: possible deadlock in open_rio 2019-08-07 14:24 ` Andrey Konovalov @ 2019-08-07 14:34 ` Andrey Konovalov 2019-08-07 14:38 ` Andrey Konovalov 2019-08-07 14:39 ` Alan Stern 1 sibling, 1 reply; 14+ messages in thread From: Andrey Konovalov @ 2019-08-07 14:34 UTC (permalink / raw) To: Alan Stern Cc: syzbot, Greg Kroah-Hartman, LKML, USB list, Cesar Miquel, rio500-users, syzkaller-bugs On Wed, Aug 7, 2019 at 4:24 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > On Wed, Aug 7, 2019 at 4:01 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Wed, 7 Aug 2019, Andrey Konovalov wrote: > > > > > On Tue, Aug 6, 2019 at 9:13 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > > > On Thu, 1 Aug 2019, syzbot wrote: > > > > > > > > > Hello, > > > > > > > > > > syzbot found the following crash on: > > > > > > > > > > HEAD commit: 7f7867ff 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=136b6aec600000 > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a > > > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > > > > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com > > > > > > > > > > ====================================================== > > > > > WARNING: possible circular locking dependency detected > > > > > 5.3.0-rc2+ #23 Not tainted > > > > > ------------------------------------------------------ > > > > > > > > Andrey: > > > > > > > > This should be completely reproducible, since it's a simple ABBA > > > > locking violation. Maybe just introducing a time delay (to avoid races > > > > and give the open() call time to run) between the gadget creation and > > > > gadget removal would be enough to do it. > > > > > > I've tried some simple approaches to reproducing this, but failed. > > > Should this require two rio500 devices to trigger? > > > > No, one device should be enough. Just plug it in and then try to open > > the character device file. > > OK, I've reproduced it, so I can test a patch manually. The reason > syzbot couldn't do that, is because it doesn't open character devices. > Right now the USB fuzzing instance only opens /dev/input*, > /dev/hidraw* and /dev/usb/hiddev* (only the devices that are created > by USB HID devices as I've been working on adding USB HID targeted > fuzzing support lately). > > I guess we should open /dev/chr/* as well. The problem is that there > 300+ devices there even without connecting USB devices and opening > them blindly probably won't work. Is there a way to know which > character devices are created by USB devices? Maybe they are exposed > over /sys/bus/usb or via some other way? Ah, OK, I see that it's also exposed as /dev/rio500 for this particular driver. This doesn't really help, as these names will differ for different drivers, and this will require custom syzkaller descriptions for each driver. I'm planning to add them for some widely-used (i.e. enabled on Android) drivers at some point, but it's too much work to do it for all the drivers enabled on e.g. Ubuntu. > > > > > Alan Stern > > > > > > Is there any way you can test this? > > > > > > Not yet. > > > > > > > > > > > Alan Stern > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: possible deadlock in open_rio 2019-08-07 14:34 ` Andrey Konovalov @ 2019-08-07 14:38 ` Andrey Konovalov 0 siblings, 0 replies; 14+ messages in thread From: Andrey Konovalov @ 2019-08-07 14:38 UTC (permalink / raw) To: Alan Stern Cc: syzbot, Greg Kroah-Hartman, LKML, USB list, Cesar Miquel, rio500-users, syzkaller-bugs On Wed, Aug 7, 2019 at 4:34 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > On Wed, Aug 7, 2019 at 4:24 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > > > On Wed, Aug 7, 2019 at 4:01 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Wed, 7 Aug 2019, Andrey Konovalov wrote: > > > > > > > On Tue, Aug 6, 2019 at 9:13 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > > > > > On Thu, 1 Aug 2019, syzbot wrote: > > > > > > > > > > > Hello, > > > > > > > > > > > > syzbot found the following crash on: > > > > > > > > > > > > HEAD commit: 7f7867ff 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=136b6aec600000 > > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 > > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a > > > > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > > > > > > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > > > > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > > > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com > > > > > > > > > > > > ====================================================== > > > > > > WARNING: possible circular locking dependency detected > > > > > > 5.3.0-rc2+ #23 Not tainted > > > > > > ------------------------------------------------------ > > > > > > > > > > Andrey: > > > > > > > > > > This should be completely reproducible, since it's a simple ABBA > > > > > locking violation. Maybe just introducing a time delay (to avoid races > > > > > and give the open() call time to run) between the gadget creation and > > > > > gadget removal would be enough to do it. > > > > > > > > I've tried some simple approaches to reproducing this, but failed. > > > > Should this require two rio500 devices to trigger? > > > > > > No, one device should be enough. Just plug it in and then try to open > > > the character device file. > > > > OK, I've reproduced it, so I can test a patch manually. The reason > > syzbot couldn't do that, is because it doesn't open character devices. > > Right now the USB fuzzing instance only opens /dev/input*, > > /dev/hidraw* and /dev/usb/hiddev* (only the devices that are created > > by USB HID devices as I've been working on adding USB HID targeted > > fuzzing support lately). > > > > I guess we should open /dev/chr/* as well. The problem is that there > > 300+ devices there even without connecting USB devices and opening > > them blindly probably won't work. Is there a way to know which > > character devices are created by USB devices? Maybe they are exposed > > over /sys/bus/usb or via some other way? > > Ah, OK, I see that it's also exposed as /dev/rio500 for this > particular driver. This doesn't really help, as these names will > differ for different drivers, and this will require custom syzkaller > descriptions for each driver. I'm planning to add them for some > widely-used (i.e. enabled on Android) drivers at some point, but it's > too much work to do it for all the drivers enabled on e.g. Ubuntu. BTW, the deadlock report is actually followed by another one, which looks like a different bug: usercopy: Kernel memory exposure attempt detected from wrapped address (offset 0, size 184466! ------------[ cut here ]------------ kernel BUG at mm/usercopy.c:98! invalid opcode: 0000 [#1] SMP KASAN CPU: 1 PID: 2287 Comm: cat Not tainted 5.3.0-rc2+ #126 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 RIP: 0010:usercopy_abort+0xb9/0xbb mm/usercopy.c:86 Code: e8 b1 f5 d6 ff 49 89 d9 4d 89 e8 4c 89 e1 41 56 48 89 ee 48 c7 c7 20 f4 cd 85 ff 74 24 1 RSP: 0018:ffff88806655fc60 EFLAGS: 00010282 RAX: 000000000000006d RBX: ffffffff85cdf140 RCX: 0000000000000000 RDX: 0000000000000000 RSI: ffffffff8128a0fd RDI: ffffed100ccabf7e RBP: ffffffff85cdf300 R08: 000000000000006d R09: ffffed100d965d60 R10: ffffed100d965d5f R11: ffff88806cb2eaff R12: ffffffff85cdf4a0 R13: ffffffff85cdf140 R14: ffff887feae14e00 R15: ffffffff85cdf140 FS: 00007f4ab703f700(0000) GS:ffff88806cb00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000008cd068 CR3: 0000000065ffa000 CR4: 00000000000006e0 Call Trace: check_bogus_address mm/usercopy.c:151 __check_object_size mm/usercopy.c:260 __check_object_size.cold+0xb2/0xba mm/usercopy.c:250 check_object_size ./include/linux/thread_info.h:119 check_copy_size ./include/linux/thread_info.h:150 copy_to_user ./include/linux/uaccess.h:151 read_rio+0x223/0x480 drivers/usb/misc/rio500.c:423 __vfs_read+0x76/0x100 fs/read_write.c:425 vfs_read+0x1ea/0x430 fs/read_write.c:461 ksys_read+0x127/0x250 fs/read_write.c:587 do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296 entry_SYSCALL_64_after_hwframe+0x49/0xbe arch/x86/entry/entry_64.S:175 RIP: 0033:0x7f4ab6b6d310 Code: 73 01 c3 48 8b 0d 28 4b 2b 00 31 d2 48 29 c2 64 89 11 48 83 c8 ff eb ea 90 90 83 3d e5 4 RSP: 002b:00007fff2ba3e448 EFLAGS: 00000246 ORIG_RAX: 0000000000000000 RAX: ffffffffffffffda RBX: 0000000000008000 RCX: 00007f4ab6b6d310 RDX: 0000000000008000 RSI: 00000000008c5000 RDI: 0000000000000003 RBP: 0000000000008000 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 00000000008c5000 R13: 0000000000000003 R14: 0000000000000000 R15: 0000000000008000 Modules linked in: ---[ end trace 01dee08b337d41c2 ]--- RIP: 0010:usercopy_abort+0xb9/0xbb mm/usercopy.c:86 Code: e8 b1 f5 d6 ff 49 89 d9 4d 89 e8 4c 89 e1 41 56 48 89 ee 48 c7 c7 20 f4 cd 85 ff 74 24 1 RSP: 0018:ffff88806655fc60 EFLAGS: 00010282 RAX: 000000000000006d RBX: ffffffff85cdf140 RCX: 0000000000000000 RDX: 0000000000000000 RSI: ffffffff8128a0fd RDI: ffffed100ccabf7e RBP: ffffffff85cdf300 R08: 000000000000006d R09: ffffed100d965d60 R10: ffffed100d965d5f R11: ffff88806cb2eaff R12: ffffffff85cdf4a0 R13: ffffffff85cdf140 R14: ffff887feae14e00 R15: ffffffff85cdf140 FS: 00007f4ab703f700(0000) GS:ffff88806cb00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000008cd068 CR3: 0000000065ffa000 CR4: 00000000000006e0 usb 1-1: USB disconnect, device number 3 > > > > > > > > > Alan Stern > > > > > > > > Is there any way you can test this? > > > > > > > > Not yet. > > > > > > > > > > > > > > Alan Stern > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: possible deadlock in open_rio 2019-08-07 14:24 ` Andrey Konovalov 2019-08-07 14:34 ` Andrey Konovalov @ 2019-08-07 14:39 ` Alan Stern 2019-08-07 15:08 ` Andrey Konovalov 1 sibling, 1 reply; 14+ messages in thread From: Alan Stern @ 2019-08-07 14:39 UTC (permalink / raw) To: Andrey Konovalov Cc: syzbot, Greg Kroah-Hartman, LKML, USB list, Cesar Miquel, rio500-users, syzkaller-bugs On Wed, 7 Aug 2019, Andrey Konovalov wrote: > On Wed, Aug 7, 2019 at 4:01 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Wed, 7 Aug 2019, Andrey Konovalov wrote: > > > > > On Tue, Aug 6, 2019 at 9:13 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > > > On Thu, 1 Aug 2019, syzbot wrote: > > > > > > > > > Hello, > > > > > > > > > > syzbot found the following crash on: > > > > > > > > > > HEAD commit: 7f7867ff 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=136b6aec600000 > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a > > > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > > > > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com > > > > > > > > > > ====================================================== > > > > > WARNING: possible circular locking dependency detected > > > > > 5.3.0-rc2+ #23 Not tainted > > > > > ------------------------------------------------------ > > > > > > > > Andrey: > > > > > > > > This should be completely reproducible, since it's a simple ABBA > > > > locking violation. Maybe just introducing a time delay (to avoid races > > > > and give the open() call time to run) between the gadget creation and > > > > gadget removal would be enough to do it. > > > > > > I've tried some simple approaches to reproducing this, but failed. > > > Should this require two rio500 devices to trigger? > > > > No, one device should be enough. Just plug it in and then try to open > > the character device file. > > OK, I've reproduced it, so I can test a patch manually. The reason > syzbot couldn't do that, is because it doesn't open character devices. > Right now the USB fuzzing instance only opens /dev/input*, > /dev/hidraw* and /dev/usb/hiddev* (only the devices that are created > by USB HID devices as I've been working on adding USB HID targeted > fuzzing support lately). > > I guess we should open /dev/chr/* as well. The problem is that there > 300+ devices there even without connecting USB devices and opening > them blindly probably won't work. Is there a way to know which > character devices are created by USB devices? Maybe they are exposed > over /sys/bus/usb or via some other way? I don't have any devices that use this API, so I can't be certain. However, I believe the devices do get registered under /sys/class/usb/. (Note that this directory doesn't exist when there aren't any USB class files.) In any case, the USB character device files all have their major numbers set to 180 (USB_MAJOR defined in include/linux/usb.h), so you can identify them that way. Alan Stern ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: possible deadlock in open_rio 2019-08-07 14:39 ` Alan Stern @ 2019-08-07 15:08 ` Andrey Konovalov 0 siblings, 0 replies; 14+ messages in thread From: Andrey Konovalov @ 2019-08-07 15:08 UTC (permalink / raw) To: Alan Stern Cc: syzbot, Greg Kroah-Hartman, LKML, USB list, Cesar Miquel, rio500-users, syzkaller-bugs On Wed, Aug 7, 2019 at 4:39 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, 7 Aug 2019, Andrey Konovalov wrote: > > > On Wed, Aug 7, 2019 at 4:01 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Wed, 7 Aug 2019, Andrey Konovalov wrote: > > > > > > > On Tue, Aug 6, 2019 at 9:13 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > > > > > On Thu, 1 Aug 2019, syzbot wrote: > > > > > > > > > > > Hello, > > > > > > > > > > > > syzbot found the following crash on: > > > > > > > > > > > > HEAD commit: 7f7867ff 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=136b6aec600000 > > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 > > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a > > > > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > > > > > > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > > > > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > > > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com > > > > > > > > > > > > ====================================================== > > > > > > WARNING: possible circular locking dependency detected > > > > > > 5.3.0-rc2+ #23 Not tainted > > > > > > ------------------------------------------------------ > > > > > > > > > > Andrey: > > > > > > > > > > This should be completely reproducible, since it's a simple ABBA > > > > > locking violation. Maybe just introducing a time delay (to avoid races > > > > > and give the open() call time to run) between the gadget creation and > > > > > gadget removal would be enough to do it. > > > > > > > > I've tried some simple approaches to reproducing this, but failed. > > > > Should this require two rio500 devices to trigger? > > > > > > No, one device should be enough. Just plug it in and then try to open > > > the character device file. > > > > OK, I've reproduced it, so I can test a patch manually. The reason > > syzbot couldn't do that, is because it doesn't open character devices. > > Right now the USB fuzzing instance only opens /dev/input*, > > /dev/hidraw* and /dev/usb/hiddev* (only the devices that are created > > by USB HID devices as I've been working on adding USB HID targeted > > fuzzing support lately). > > > > I guess we should open /dev/chr/* as well. The problem is that there > > 300+ devices there even without connecting USB devices and opening > > them blindly probably won't work. Is there a way to know which > > character devices are created by USB devices? Maybe they are exposed > > over /sys/bus/usb or via some other way? > > I don't have any devices that use this API, so I can't be certain. > However, I believe the devices do get registered under /sys/class/usb/. > (Note that this directory doesn't exist when there aren't any USB class > files.) > > In any case, the USB character device files all have their major > numbers set to 180 (USB_MAJOR defined in include/linux/usb.h), so you > can identify them that way. This should work! I'll enable fuzzing of /dev/char/180:*, thanks! > > Alan Stern > ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <1565187142.15973.3.camel@neukum.org>]
* Re: possible deadlock in open_rio [not found] <1565187142.15973.3.camel@neukum.org> @ 2019-08-08 14:33 ` Alan Stern 2019-08-08 14:44 ` Andrey Konovalov 0 siblings, 1 reply; 14+ messages in thread From: Alan Stern @ 2019-08-08 14:33 UTC (permalink / raw) To: Oliver Neukum Cc: andreyknvl, syzkaller-bugs, gregkh, syzbot, Kernel development list, USB list On Wed, 7 Aug 2019, Oliver Neukum wrote: > Am Mittwoch, den 07.08.2019, 10:07 -0400 schrieb Alan Stern: > > On Wed, 7 Aug 2019, Oliver Neukum wrote: > > > technically yes. However in practical terms the straight revert I sent > > > out yesterday should fix it. > > > > I didn't see the revert, and it doesn't appear to have reached the > > mailing list archive. Can you post it again? > > As soon as our VPN server is back up again. The revert may not be necessay; a little fix should get rid of the locking violation. The key is to avoid calling the registration or deregistration routines while holding the rio500_mutex, and to recognize that the probe and disconnect routines are both protected by the device mutex. How does this patch look? Alan Stern #syz test: https://github.com/google/kasan.git 7f7867ff Index: usb-devel/drivers/usb/misc/rio500.c =================================================================== --- usb-devel.orig/drivers/usb/misc/rio500.c +++ usb-devel/drivers/usb/misc/rio500.c @@ -454,52 +454,54 @@ static int probe_rio(struct usb_interfac { struct usb_device *dev = interface_to_usbdev(intf); struct rio_usb_data *rio = &rio_instance; - int retval = 0; + int retval; + char *ibuf, *obuf; - mutex_lock(&rio500_mutex); if (rio->present) { dev_info(&intf->dev, "Second USB Rio at address %d refused\n", dev->devnum); - retval = -EBUSY; - goto bail_out; - } else { - dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum); + return -EBUSY; } + dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum); retval = usb_register_dev(intf, &usb_rio_class); if (retval) { dev_err(&dev->dev, "Not able to get a minor for this device.\n"); - retval = -ENOMEM; - goto bail_out; + goto err_register; } - rio->rio_dev = dev; - - if (!(rio->obuf = kmalloc(OBUF_SIZE, GFP_KERNEL))) { + obuf = kmalloc(OBUF_SIZE, GFP_KERNEL); + if (!obuf) { dev_err(&dev->dev, "probe_rio: Not enough memory for the output buffer\n"); - usb_deregister_dev(intf, &usb_rio_class); - retval = -ENOMEM; - goto bail_out; + goto err_obuf; } - dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf); + dev_dbg(&intf->dev, "obuf address: %p\n", obuf); - if (!(rio->ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL))) { + ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL); + if (!ibuf) { dev_err(&dev->dev, "probe_rio: Not enough memory for the input buffer\n"); - usb_deregister_dev(intf, &usb_rio_class); - kfree(rio->obuf); - retval = -ENOMEM; - goto bail_out; + goto err_ibuf; } - dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf); + dev_dbg(&intf->dev, "ibuf address: %p\n", ibuf); + mutex_lock(&rio500_mutex); + rio->rio_dev = dev; + rio->ibuf = ibuf; + rio->obuf = obuf; usb_set_intfdata (intf, rio); rio->present = 1; -bail_out: mutex_unlock(&rio500_mutex); return retval; + + err_ibuf: + kfree(obuf); + err_obuf: + usb_deregister_dev(intf, &usb_rio_class); + err_register: + return -ENOMEM; } static void disconnect_rio(struct usb_interface *intf) @@ -507,10 +509,10 @@ static void disconnect_rio(struct usb_in struct rio_usb_data *rio = usb_get_intfdata (intf); usb_set_intfdata (intf, NULL); - mutex_lock(&rio500_mutex); if (rio) { usb_deregister_dev(intf, &usb_rio_class); + mutex_lock(&rio500_mutex); if (rio->isopen) { rio->isopen = 0; /* better let it finish - the release will do whats needed */ @@ -524,8 +526,8 @@ static void disconnect_rio(struct usb_in dev_info(&intf->dev, "USB Rio disconnected.\n"); rio->present = 0; + mutex_unlock(&rio500_mutex); } - mutex_unlock(&rio500_mutex); } static const struct usb_device_id rio_table[] = { ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: possible deadlock in open_rio 2019-08-08 14:33 ` Alan Stern @ 2019-08-08 14:44 ` Andrey Konovalov 0 siblings, 0 replies; 14+ messages in thread From: Andrey Konovalov @ 2019-08-08 14:44 UTC (permalink / raw) To: Alan Stern Cc: Oliver Neukum, syzkaller-bugs, Greg Kroah-Hartman, syzbot, Kernel development list, USB list On Thu, Aug 8, 2019 at 4:33 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, 7 Aug 2019, Oliver Neukum wrote: > > > Am Mittwoch, den 07.08.2019, 10:07 -0400 schrieb Alan Stern: > > > On Wed, 7 Aug 2019, Oliver Neukum wrote: > > > > > technically yes. However in practical terms the straight revert I sent > > > > out yesterday should fix it. > > > > > > I didn't see the revert, and it doesn't appear to have reached the > > > mailing list archive. Can you post it again? > > > > As soon as our VPN server is back up again. > > The revert may not be necessay; a little fix should get rid of the > locking violation. The key is to avoid calling the registration or > deregistration routines while holding the rio500_mutex, and to > recognize that the probe and disconnect routines are both protected by > the device mutex. > > How does this patch look? > > Alan Stern > > > #syz test: https://github.com/google/kasan.git 7f7867ff There's no reproducer yet (it should appear at some point, I've enabled fuzzing of USB char devices). I've tested your patch manually and the deadlock report is gone. Thanks! Tested-by: Andrey Konovalov <andreyknvl@google.com> > > Index: usb-devel/drivers/usb/misc/rio500.c > =================================================================== > --- usb-devel.orig/drivers/usb/misc/rio500.c > +++ usb-devel/drivers/usb/misc/rio500.c > @@ -454,52 +454,54 @@ static int probe_rio(struct usb_interfac > { > struct usb_device *dev = interface_to_usbdev(intf); > struct rio_usb_data *rio = &rio_instance; > - int retval = 0; > + int retval; > + char *ibuf, *obuf; > > - mutex_lock(&rio500_mutex); > if (rio->present) { > dev_info(&intf->dev, "Second USB Rio at address %d refused\n", dev->devnum); > - retval = -EBUSY; > - goto bail_out; > - } else { > - dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum); > + return -EBUSY; > } > + dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum); > > retval = usb_register_dev(intf, &usb_rio_class); > if (retval) { > dev_err(&dev->dev, > "Not able to get a minor for this device.\n"); > - retval = -ENOMEM; > - goto bail_out; > + goto err_register; > } > > - rio->rio_dev = dev; > - > - if (!(rio->obuf = kmalloc(OBUF_SIZE, GFP_KERNEL))) { > + obuf = kmalloc(OBUF_SIZE, GFP_KERNEL); > + if (!obuf) { > dev_err(&dev->dev, > "probe_rio: Not enough memory for the output buffer\n"); > - usb_deregister_dev(intf, &usb_rio_class); > - retval = -ENOMEM; > - goto bail_out; > + goto err_obuf; > } > - dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf); > + dev_dbg(&intf->dev, "obuf address: %p\n", obuf); > > - if (!(rio->ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL))) { > + ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL); > + if (!ibuf) { > dev_err(&dev->dev, > "probe_rio: Not enough memory for the input buffer\n"); > - usb_deregister_dev(intf, &usb_rio_class); > - kfree(rio->obuf); > - retval = -ENOMEM; > - goto bail_out; > + goto err_ibuf; > } > - dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf); > + dev_dbg(&intf->dev, "ibuf address: %p\n", ibuf); > > + mutex_lock(&rio500_mutex); > + rio->rio_dev = dev; > + rio->ibuf = ibuf; > + rio->obuf = obuf; > usb_set_intfdata (intf, rio); > rio->present = 1; > -bail_out: > mutex_unlock(&rio500_mutex); > > return retval; > + > + err_ibuf: > + kfree(obuf); > + err_obuf: > + usb_deregister_dev(intf, &usb_rio_class); > + err_register: > + return -ENOMEM; > } > > static void disconnect_rio(struct usb_interface *intf) > @@ -507,10 +509,10 @@ static void disconnect_rio(struct usb_in > struct rio_usb_data *rio = usb_get_intfdata (intf); > > usb_set_intfdata (intf, NULL); > - mutex_lock(&rio500_mutex); > if (rio) { > usb_deregister_dev(intf, &usb_rio_class); > > + mutex_lock(&rio500_mutex); > if (rio->isopen) { > rio->isopen = 0; > /* better let it finish - the release will do whats needed */ > @@ -524,8 +526,8 @@ static void disconnect_rio(struct usb_in > dev_info(&intf->dev, "USB Rio disconnected.\n"); > > rio->present = 0; > + mutex_unlock(&rio500_mutex); > } > - mutex_unlock(&rio500_mutex); > } > > static const struct usb_device_id rio_table[] = { > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-08-08 14:44 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-01 15:28 possible deadlock in open_rio syzbot 2019-08-02 20:51 ` Alan Stern 2019-08-06 19:13 ` Alan Stern 2019-08-07 13:37 ` Oliver Neukum 2019-08-07 14:07 ` Alan Stern 2019-08-07 13:53 ` Andrey Konovalov 2019-08-07 14:01 ` Alan Stern 2019-08-07 14:24 ` Andrey Konovalov 2019-08-07 14:34 ` Andrey Konovalov 2019-08-07 14:38 ` Andrey Konovalov 2019-08-07 14:39 ` Alan Stern 2019-08-07 15:08 ` Andrey Konovalov [not found] <1565187142.15973.3.camel@neukum.org> 2019-08-08 14:33 ` Alan Stern 2019-08-08 14:44 ` Andrey Konovalov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).