On Fri, Oct 16, 2020 at 3:57 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. > > Change __usb_hcd_giveback_urb() to only call kcov_remote_*() callbacks > when it's being executed in a softirq. As the result, the coverage from > USB/IP related usb_hcd_giveback_urb() calls won't be collected, but the > WARNING is fixed. > > A potential future improvement would be to support nested remote coverage > collection sections, but this patch doesn't address that. > > Signed-off-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx> > Acked-by: Marco Elver <elver@xxxxxxxxxx> > --- > > Changes v3->v4: > - Don't make any kcov changes, do a softirq context check in usb code > instead. > > --- > drivers/usb/core/hcd.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index a33b849e8beb..2f6a39e09dc6 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -1646,9 +1646,16 @@ static void __usb_hcd_giveback_urb(struct urb *urb) > > /* pass ownership to the completion handler */ > urb->status = status; > - kcov_remote_start_usb((u64)urb->dev->bus->busnum); > + /* > + * This function can be called in task context inside another remote > + * coverage collection section, but KCOV doesn't support that kind of > + * recursion yet. Only collect coverage in softirq context for now. > + */ > + if (in_serving_softirq()) > + kcov_remote_start_usb((u64)urb->dev->bus->busnum); > urb->complete(urb); > - kcov_remote_stop(); > + if (in_serving_softirq()) > + kcov_remote_stop(); Reviewed-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx> Looks simpler :) > usb_anchor_resume_wakeups(anchor); > atomic_dec(&urb->use_count); > -- > 2.29.0.rc1.297.gfa9743e501-goog >
