* Is usb_hcd_giveback_urb() allowed in task context? @ 2020-10-05 15:08 Andrey Konovalov 2020-10-05 15:18 ` Greg Kroah-Hartman 2020-10-05 15:22 ` Alan Stern 0 siblings, 2 replies; 9+ messages in thread From: Andrey Konovalov @ 2020-10-05 15:08 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman, Alan Stern Cc: USB list, LKML, Dmitry Vyukov, Nazime Hande Harputluoglu, syzkaller Dear USB and USB/IP maintainers, While fuzzing the USB/IP stack with syzkaller we've stumbled upon an issue. Currently kcov (the subsystem that is used for coverage collection) USB-related callbacks assume that usb_hcd_giveback_urb() can only be called from interrupt context, as indicated by the comment before the function definition. In the USB/IP code, however, it's called from the task context (see the stack trace below). Is this something that is allowed and we need to fix kcov? Or is this a bug in USB/IP? Thank you! ------------[ cut here ]------------ WARNING: CPU: 2 PID: 57 at kernel/kcov.c:834 kcov_remote_start+0xa7/0x400 kernel/kcov.c:834 Kernel panic - not syncing: panic_on_warn set ... CPU: 2 PID: 57 Comm: kworker/2:1 Not tainted 5.9.0-rc7+ #45 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 04/01/2014 Workqueue: usb_hub_wq hub_event Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x14b/0x19d lib/dump_stack.c:118 panic+0x319/0x765 kernel/panic.c:231 __warn.cold+0x2f/0x2f kernel/panic.c:600 report_bug+0x273/0x300 lib/bug.c:198 handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234 exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254 asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536 RIP: 0010:kcov_remote_start+0xa7/0x400 kernel/kcov.c:834 Code: 84 26 03 00 00 fa 66 0f 1f 44 00 00 65 8b 05 50 13 93 7e a9 00 01 ff 00 41 8b 94 24 50 0a 00 00 75 1a 81 e2 ff ff ff bf 74 12 <0f> 0b 48 83 3d 17 c4 26 08 00 0f 85 62 01 00 00 0f 0b 65 8b 05 20 RSP: 0018:ffffc9000030f600 EFLAGS: 00010002 RAX: 0000000080000000 RBX: 0100000000000003 RCX: ffffc90014cd1000 RDX: 0000000000000002 RSI: ffffffff85199fcc RDI: 0100000000000003 RBP: 0000000000000282 R08: ffff88806d594640 R09: fffff52000061eca R10: 0000000000000003 R11: fffff52000061ec9 R12: ffff88806d594640 R13: 0000000000000000 R14: 0100000000000003 R15: 0000000000000000 kcov_remote_start_usb include/linux/kcov.h:52 [inline] __usb_hcd_giveback_urb+0x284/0x4b0 drivers/usb/core/hcd.c:1649 usb_hcd_giveback_urb+0x367/0x410 drivers/usb/core/hcd.c:1716 vhci_urb_enqueue.cold+0x37f/0x4c5 drivers/usb/usbip/vhci_hcd.c:801 usb_hcd_submit_urb+0x2b1/0x20d0 drivers/usb/core/hcd.c:1547 usb_submit_urb+0x6e5/0x13b0 drivers/usb/core/urb.c:570 usb_start_wait_urb+0x10f/0x2c0 drivers/usb/core/message.c:58 usb_internal_control_msg drivers/usb/core/message.c:102 [inline] usb_control_msg+0x31c/0x4a0 drivers/usb/core/message.c:153 hub_set_address drivers/usb/core/hub.c:4472 [inline] hub_port_init+0x23f6/0x2d20 drivers/usb/core/hub.c:4748 hub_port_connect drivers/usb/core/hub.c:5140 [inline] hub_port_connect_change drivers/usb/core/hub.c:5348 [inline] port_event drivers/usb/core/hub.c:5494 [inline] hub_event+0x1cc9/0x38d0 drivers/usb/core/hub.c:5576 process_one_work+0x7b6/0x1190 kernel/workqueue.c:2269 worker_thread+0x94/0xdc0 kernel/workqueue.c:2415 kthread+0x372/0x450 kernel/kthread.c:292 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 Dumping ftrace buffer: (ftrace buffer empty) Kernel Offset: disabled Rebooting in 1 seconds.. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Is usb_hcd_giveback_urb() allowed in task context? 2020-10-05 15:08 Is usb_hcd_giveback_urb() allowed in task context? Andrey Konovalov @ 2020-10-05 15:18 ` Greg Kroah-Hartman 2020-10-05 15:21 ` Andrey Konovalov 2020-10-05 15:22 ` Alan Stern 1 sibling, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2020-10-05 15:18 UTC (permalink / raw) To: Andrey Konovalov Cc: Valentina Manea, Shuah Khan, Alan Stern, USB list, LKML, Dmitry Vyukov, Nazime Hande Harputluoglu, syzkaller On Mon, Oct 05, 2020 at 05:08:11PM +0200, Andrey Konovalov wrote: > Dear USB and USB/IP maintainers, > > While fuzzing the USB/IP stack with syzkaller we've stumbled upon an issue. > > Currently kcov (the subsystem that is used for coverage collection) > USB-related callbacks assume that usb_hcd_giveback_urb() can only be > called from interrupt context, as indicated by the comment before the > function definition. In the USB/IP code, however, it's called from the > task context (see the stack trace below). > > Is this something that is allowed and we need to fix kcov? Or is this > a bug in USB/IP? It's a bug in kcov, and is not true as you have found out :) thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Is usb_hcd_giveback_urb() allowed in task context? 2020-10-05 15:18 ` Greg Kroah-Hartman @ 2020-10-05 15:21 ` Andrey Konovalov 2020-10-05 15:25 ` Alan Stern 0 siblings, 1 reply; 9+ messages in thread From: Andrey Konovalov @ 2020-10-05 15:21 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Valentina Manea, Shuah Khan, Alan Stern, USB list, LKML, Dmitry Vyukov, Nazime Hande Harputluoglu, syzkaller On Mon, Oct 5, 2020 at 5:18 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, Oct 05, 2020 at 05:08:11PM +0200, Andrey Konovalov wrote: > > Dear USB and USB/IP maintainers, > > > > While fuzzing the USB/IP stack with syzkaller we've stumbled upon an issue. > > > > Currently kcov (the subsystem that is used for coverage collection) > > USB-related callbacks assume that usb_hcd_giveback_urb() can only be > > called from interrupt context, as indicated by the comment before the > > function definition. In the USB/IP code, however, it's called from the > > task context (see the stack trace below). > > > > Is this something that is allowed and we need to fix kcov? Or is this > > a bug in USB/IP? > > It's a bug in kcov, and is not true as you have found out :) OK, I see, I'll work on a fix, thanks! Should I also update the comment above usb_hcd_giveback_urb() to mention that it can be called in_task()? Or is this redundant and is assumed in general? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Is usb_hcd_giveback_urb() allowed in task context? 2020-10-05 15:21 ` Andrey Konovalov @ 2020-10-05 15:25 ` Alan Stern 2020-10-05 23:38 ` Shuah Khan 0 siblings, 1 reply; 9+ messages in thread From: Alan Stern @ 2020-10-05 15:25 UTC (permalink / raw) To: Andrey Konovalov Cc: Greg Kroah-Hartman, Valentina Manea, Shuah Khan, USB list, LKML, Dmitry Vyukov, Nazime Hande Harputluoglu, syzkaller On Mon, Oct 05, 2020 at 05:21:30PM +0200, Andrey Konovalov wrote: > On Mon, Oct 5, 2020 at 5:18 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Oct 05, 2020 at 05:08:11PM +0200, Andrey Konovalov wrote: > > > Dear USB and USB/IP maintainers, > > > > > > While fuzzing the USB/IP stack with syzkaller we've stumbled upon an issue. > > > > > > Currently kcov (the subsystem that is used for coverage collection) > > > USB-related callbacks assume that usb_hcd_giveback_urb() can only be > > > called from interrupt context, as indicated by the comment before the > > > function definition. In the USB/IP code, however, it's called from the > > > task context (see the stack trace below). > > > > > > Is this something that is allowed and we need to fix kcov? Or is this > > > a bug in USB/IP? > > > > It's a bug in kcov, and is not true as you have found out :) > > OK, I see, I'll work on a fix, thanks! > > Should I also update the comment above usb_hcd_giveback_urb() to > mention that it can be called in_task()? Or is this redundant and is > assumed in general? No, no -- it won't work right if it's called in process context. Not only do the spinlock calls leave the interrupt flag unchanged, also the driver callback routines may expect to be invoked with interrupts disabled. (We have tried to fix this, but I'm not at all certain that all the cases have been updated.) Alan Stern ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Is usb_hcd_giveback_urb() allowed in task context? 2020-10-05 15:25 ` Alan Stern @ 2020-10-05 23:38 ` Shuah Khan 2020-10-06 1:23 ` Alan Stern 0 siblings, 1 reply; 9+ messages in thread From: Shuah Khan @ 2020-10-05 23:38 UTC (permalink / raw) To: Alan Stern, Andrey Konovalov Cc: Greg Kroah-Hartman, Valentina Manea, Shuah Khan, USB list, LKML, Dmitry Vyukov, Nazime Hande Harputluoglu, syzkaller, Shuah Khan On 10/5/20 9:25 AM, Alan Stern wrote: > On Mon, Oct 05, 2020 at 05:21:30PM +0200, Andrey Konovalov wrote: >> On Mon, Oct 5, 2020 at 5:18 PM Greg Kroah-Hartman >> <gregkh@linuxfoundation.org> wrote: >>> >>> On Mon, Oct 05, 2020 at 05:08:11PM +0200, Andrey Konovalov wrote: >>>> Dear USB and USB/IP maintainers, >>>> >>>> While fuzzing the USB/IP stack with syzkaller we've stumbled upon an issue. >>>> >>>> Currently kcov (the subsystem that is used for coverage collection) >>>> USB-related callbacks assume that usb_hcd_giveback_urb() can only be >>>> called from interrupt context, as indicated by the comment before the >>>> function definition. In the USB/IP code, however, it's called from the >>>> task context (see the stack trace below). >>>> >>>> Is this something that is allowed and we need to fix kcov? Or is this >>>> a bug in USB/IP? >>> >>> It's a bug in kcov, and is not true as you have found out :) >> >> OK, I see, I'll work on a fix, thanks! >> >> Should I also update the comment above usb_hcd_giveback_urb() to >> mention that it can be called in_task()? Or is this redundant and is >> assumed in general? > > No, no -- it won't work right if it's called in process context. Not > only do the spinlock calls leave the interrupt flag unchanged, also the > driver callback routines may expect to be invoked with interrupts > disabled. (We have tried to fix this, but I'm not at all certain that > all the cases have been updated.) > In the case of vhci case, usb_hcd_giveback_urb() is called from vhci's urb_enqueue, when it determines it doesn't need to xmit the urb and can give it back. This path runs in task context. Do you have any recommendation on how this case can be handled? thanks, -- Shuah ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Is usb_hcd_giveback_urb() allowed in task context? 2020-10-05 23:38 ` Shuah Khan @ 2020-10-06 1:23 ` Alan Stern 2020-10-06 15:44 ` Andrey Konovalov 0 siblings, 1 reply; 9+ messages in thread From: Alan Stern @ 2020-10-06 1:23 UTC (permalink / raw) To: Shuah Khan Cc: Andrey Konovalov, Greg Kroah-Hartman, Valentina Manea, Shuah Khan, USB list, LKML, Dmitry Vyukov, Nazime Hande Harputluoglu, syzkaller On Mon, Oct 05, 2020 at 05:38:22PM -0600, Shuah Khan wrote: > On 10/5/20 9:25 AM, Alan Stern wrote: > > On Mon, Oct 05, 2020 at 05:21:30PM +0200, Andrey Konovalov wrote: > > No, no -- it won't work right if it's called in process context. Not > > only do the spinlock calls leave the interrupt flag unchanged, also the > > driver callback routines may expect to be invoked with interrupts > > disabled. (We have tried to fix this, but I'm not at all certain that > > all the cases have been updated.) > > > > In the case of vhci case, usb_hcd_giveback_urb() is called from vhci's > urb_enqueue, when it determines it doesn't need to xmit the urb and can give > it back. This path runs in task context. > > Do you have any recommendation on how this case can be handled? Just call local_irq_disable() before usb_hcd_giveback_urb(), and local_irq_enable() afterward. Alan Stern ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Is usb_hcd_giveback_urb() allowed in task context? 2020-10-06 1:23 ` Alan Stern @ 2020-10-06 15:44 ` Andrey Konovalov 0 siblings, 0 replies; 9+ messages in thread From: Andrey Konovalov @ 2020-10-06 15:44 UTC (permalink / raw) To: Alan Stern Cc: Shuah Khan, Greg Kroah-Hartman, Valentina Manea, Shuah Khan, USB list, LKML, Dmitry Vyukov, Nazime Hande Harputluoglu, syzkaller On Tue, Oct 6, 2020 at 3:23 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, Oct 05, 2020 at 05:38:22PM -0600, Shuah Khan wrote: > > On 10/5/20 9:25 AM, Alan Stern wrote: > > > On Mon, Oct 05, 2020 at 05:21:30PM +0200, Andrey Konovalov wrote: > > > No, no -- it won't work right if it's called in process context. Not > > > only do the spinlock calls leave the interrupt flag unchanged, also the > > > driver callback routines may expect to be invoked with interrupts > > > disabled. (We have tried to fix this, but I'm not at all certain that > > > all the cases have been updated.) > > > > > > > In the case of vhci case, usb_hcd_giveback_urb() is called from vhci's > > urb_enqueue, when it determines it doesn't need to xmit the urb and can give > > it back. This path runs in task context. > > > > Do you have any recommendation on how this case can be handled? > > Just call local_irq_disable() before usb_hcd_giveback_urb(), and > local_irq_enable() afterward. OK, so overall it's possible to call usb_hcd_giveback_urb() in task context, but only with irqs disabled. This means we do need a fix for kcov as well, thank you! ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Is usb_hcd_giveback_urb() allowed in task context? 2020-10-05 15:08 Is usb_hcd_giveback_urb() allowed in task context? Andrey Konovalov 2020-10-05 15:18 ` Greg Kroah-Hartman @ 2020-10-05 15:22 ` Alan Stern 2020-10-05 15:28 ` Greg Kroah-Hartman 1 sibling, 1 reply; 9+ messages in thread From: Alan Stern @ 2020-10-05 15:22 UTC (permalink / raw) To: Andrey Konovalov Cc: Valentina Manea, Shuah Khan, Greg Kroah-Hartman, USB list, LKML, Dmitry Vyukov, Nazime Hande Harputluoglu, syzkaller On Mon, Oct 05, 2020 at 05:08:11PM +0200, Andrey Konovalov wrote: > Dear USB and USB/IP maintainers, > > While fuzzing the USB/IP stack with syzkaller we've stumbled upon an issue. > > Currently kcov (the subsystem that is used for coverage collection) > USB-related callbacks assume that usb_hcd_giveback_urb() can only be > called from interrupt context, as indicated by the comment before the > function definition. The primary reason for this restriction (as far as I'm aware) is because the routine uses spin_lock/spin_unlock rather than the _irqsave/_irqrestore variants. There's also a small efficiency issue: In the vast majority of cases involving real host controllers, the routine _will_ be called in interrupt context. So we optimized for that case. > In the USB/IP code, however, it's called from the > task context (see the stack trace below). > > Is this something that is allowed and we need to fix kcov? Or is this > a bug in USB/IP? It's a bug in USB/IP. Interrupts should be disabled when it calls usb_hcd_giveback_urb(). Alan Stern ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Is usb_hcd_giveback_urb() allowed in task context? 2020-10-05 15:22 ` Alan Stern @ 2020-10-05 15:28 ` Greg Kroah-Hartman 0 siblings, 0 replies; 9+ messages in thread From: Greg Kroah-Hartman @ 2020-10-05 15:28 UTC (permalink / raw) To: Alan Stern Cc: Andrey Konovalov, Valentina Manea, Shuah Khan, USB list, LKML, Dmitry Vyukov, Nazime Hande Harputluoglu, syzkaller On Mon, Oct 05, 2020 at 11:22:18AM -0400, Alan Stern wrote: > On Mon, Oct 05, 2020 at 05:08:11PM +0200, Andrey Konovalov wrote: > > Dear USB and USB/IP maintainers, > > > > While fuzzing the USB/IP stack with syzkaller we've stumbled upon an issue. > > > > Currently kcov (the subsystem that is used for coverage collection) > > USB-related callbacks assume that usb_hcd_giveback_urb() can only be > > called from interrupt context, as indicated by the comment before the > > function definition. > > The primary reason for this restriction (as far as I'm aware) is because > the routine uses spin_lock/spin_unlock rather than the > _irqsave/_irqrestore variants. There's also a small efficiency issue: > In the vast majority of cases involving real host controllers, the > routine _will_ be called in interrupt context. So we optimized for that > case. > > > In the USB/IP code, however, it's called from the > > task context (see the stack trace below). > > > > Is this something that is allowed and we need to fix kcov? Or is this > > a bug in USB/IP? > > It's a bug in USB/IP. Interrupts should be disabled when it calls > usb_hcd_giveback_urb(). But that's not always the case when we have host controllers running with threaded interrupts, right? Or do they still disable interrupts? thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-06 15:44 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-05 15:08 Is usb_hcd_giveback_urb() allowed in task context? Andrey Konovalov 2020-10-05 15:18 ` Greg Kroah-Hartman 2020-10-05 15:21 ` Andrey Konovalov 2020-10-05 15:25 ` Alan Stern 2020-10-05 23:38 ` Shuah Khan 2020-10-06 1:23 ` Alan Stern 2020-10-06 15:44 ` Andrey Konovalov 2020-10-05 15:22 ` Alan Stern 2020-10-05 15:28 ` Greg Kroah-Hartman
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).