On Tue, Oct 13, 2020 at 3:58 PM Andrey Konovalov [off-list ref] wrote:

quoted

quoted

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

quoted

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.

quoted

But
this should not happen if softirq's disabled around
usb_hcd_giveback_urb call in task context...

[...]

quoted

We do want to collect coverage from usb_hcd_giveback_urb in the task
context eventually, right?

Ideally, eventually, yes.

quoted

Is this API supposed to be final? Or it only puts down fire re the warning?

Only puts down the fire.

quoted

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.

quoted

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.

quoted

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);