On Tue, Oct 13, 2020 at 8:46 AM Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote: > > On Mon, Oct 12, 2020 at 7:17 PM Andrey Konovalov <andreyknvl@xxxxxxxxxx> wrote: > > > > Currently there's a KCOV remote coverage collection section in > > __usb_hcd_giveback_urb(). Initially that section was added based on the > > assumption that usb_hcd_giveback_urb() can only be called in interrupt > > context as indicated by a comment before it. This is what happens when > > syzkaller is fuzzing the USB stack via the dummy_hcd driver. > > > > As it turns out, it's actually valid to call usb_hcd_giveback_urb() in task > > context, provided that the caller turned off the interrupts; USB/IP does > > exactly that. This can lead to a nested KCOV remote coverage collection > > sections both trying to collect coverage in task context. This isn't > > supported by KCOV, and leads to a WARNING. > > How does this recursion happen? There is literal recursion in the task > context? A function starts a remote coverage section and calls another > function that also starts a remote coverage section? Yes, a literal recursion. Background thread for processing requests for USB/IP hub (which we collect coverage from) calls __usb_hcd_giveback_urb(). Here's the stack trace: 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 > Or is there recursion between task context and softirq context? No. This kind of recursion is actually supported by kcov right now. A softirq with a coverage collection section can come in the middle of a coverage collection section for a task. > But > this should not happen if softirq's disabled around > usb_hcd_giveback_urb call in task context... [...] > We do want to collect coverage from usb_hcd_giveback_urb in the task > context eventually, right? Ideally, eventually, yes. > Is this API supposed to be final? Or it only puts down fire re the warning? Only puts down the fire. > I don't understand how this API can be used in other contexts. > Let's say there is recursion in task context and we want to collect > coverage in task context (the function is only called in task > context). This API won't help. No, it won't. Full recursion support is required for this. > Let's say a function is called from both task and softirq context and > these can recurse (softirq arrive while in remote task section). This > API won't help. It will force to choose either task or softirq, but > let's say you can't make that choice because they are equally > important. This currently works, everything that happens in a softirq gets associated with softirq, everything else - with the task. This seems to be the only logical approach here, it makes no sense to associate what happens in a softirq with the task where the softirq happened. > The API helps to work around the unimplemented recursion in KCOV, but > it's also specific to this particular case. It's not necessary that > recursion is specific to one context only and it's not necessary that > a user can choose to sacrifice one of the contexts. > Also, if we support recursion in one way or another, we will never > want to use this API, right? Correct.
