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

[Andrey Konovalov]

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.