Re: [PATCH v4] kcov, usb: specify contexts for remote coverage sections

[Andrey Konovalov]

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.