On Tue, Oct 13, 2020 at 4:15 PM Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote: > > On Tue, Oct 13, 2020 at 3:58 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. > > > I see. > The following is simpler option that does what this patch achieves but > in a more direct way: > > // Because ... > if (in_serving_softirq()) > kcov_remote_start_usb((u64)urb->dev->bus->busnum); Hmm, this actually makes a lot of sense. I wonder why I haven't thought of that :) Will do in v4.
