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

[Dmitry Vyukov <dvyukov@google.com>]

On Tue, Oct 13, 2020 at 3:58 PM Andrey Konovalov <andreyknvl@google.com> 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);