Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4] kcov, usb: only collect coverage from __usb_hcd_giveback_urb in softirq
@ 2020-10-16 13:57 Andrey Konovalov
  2020-10-16 14:33 ` Dmitry Vyukov
  2020-11-13 12:30 ` Sebastian Andrzej Siewior
  0 siblings, 2 replies; 9+ messages in thread
From: Andrey Konovalov @ 2020-10-16 13:57 UTC (permalink / raw)
  To: Dmitry Vyukov, Greg Kroah-Hartman
  Cc: linux-usb, linux-kernel, Andrew Morton, Alan Stern, Shuah Khan,
	Alexander Potapenko, Marco Elver, Aleksandr Nogikh,
	Nazime Hande Harputluoglu, Andrey Konovalov

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@google.com>
Acked-by: Marco Elver <elver@google.com>
---

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();
 
 	usb_anchor_resume_wakeups(anchor);
 	atomic_dec(&urb->use_count);
-- 
2.29.0.rc1.297.gfa9743e501-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] kcov, usb: only collect coverage from __usb_hcd_giveback_urb in softirq
  2020-10-16 13:57 [PATCH v4] kcov, usb: only collect coverage from __usb_hcd_giveback_urb in softirq Andrey Konovalov
@ 2020-10-16 14:33 ` Dmitry Vyukov
  2020-11-13 12:30 ` Sebastian Andrzej Siewior
  1 sibling, 0 replies; 9+ messages in thread
From: Dmitry Vyukov @ 2020-10-16 14:33 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Greg Kroah-Hartman, USB list, LKML, Andrew Morton, Alan Stern,
	Shuah Khan, Alexander Potapenko, Marco Elver, Aleksandr Nogikh,
	Nazime Hande Harputluoglu

On Fri, Oct 16, 2020 at 3:57 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.
>
> 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@google.com>
> Acked-by: Marco Elver <elver@google.com>
> ---
>
> 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@google.com>

Looks simpler :)

>         usb_anchor_resume_wakeups(anchor);
>         atomic_dec(&urb->use_count);
> --
> 2.29.0.rc1.297.gfa9743e501-goog
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] kcov, usb: only collect coverage from __usb_hcd_giveback_urb in softirq
  2020-10-16 13:57 [PATCH v4] kcov, usb: only collect coverage from __usb_hcd_giveback_urb in softirq Andrey Konovalov
  2020-10-16 14:33 ` Dmitry Vyukov
@ 2020-11-13 12:30 ` Sebastian Andrzej Siewior
  2020-11-13 12:51   ` Andrey Konovalov
  1 sibling, 1 reply; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-11-13 12:30 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Dmitry Vyukov, Greg Kroah-Hartman, linux-usb, linux-kernel,
	Andrew Morton, Alan Stern, Shuah Khan, Alexander Potapenko,
	Marco Elver, Aleksandr Nogikh, Nazime Hande Harputluoglu, tglx

On 2020-10-16 15:57:45 [+0200], Andrey Konovalov wrote:
> --- 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())

Could this in_serving_softirq() usage be replaced, please?  

> +		kcov_remote_start_usb((u64)urb->dev->bus->busnum);
>  	urb->complete(urb);
> -	kcov_remote_stop();
> +	if (in_serving_softirq())
> +		kcov_remote_stop();
>  
>  	usb_anchor_resume_wakeups(anchor);
>  	atomic_dec(&urb->use_count);

Sebastian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] kcov, usb: only collect coverage from __usb_hcd_giveback_urb in softirq
  2020-11-13 12:30 ` Sebastian Andrzej Siewior
@ 2020-11-13 12:51   ` Andrey Konovalov
  2020-11-13 13:28     ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 9+ messages in thread
From: Andrey Konovalov @ 2020-11-13 12:51 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Dmitry Vyukov, Greg Kroah-Hartman, USB list, LKML, Andrew Morton,
	Alan Stern, Shuah Khan, Alexander Potapenko, Marco Elver,
	Aleksandr Nogikh, Nazime Hande Harputluoglu, Thomas Gleixner

On Fri, Nov 13, 2020 at 1:30 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> On 2020-10-16 15:57:45 [+0200], Andrey Konovalov wrote:
> > --- 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())
>
> Could this in_serving_softirq() usage be replaced, please?

Hi Sebastian,

Replaced with what and why?

Thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] kcov, usb: only collect coverage from __usb_hcd_giveback_urb in softirq
  2020-11-13 12:51   ` Andrey Konovalov
@ 2020-11-13 13:28     ` Sebastian Andrzej Siewior
  2020-11-13 13:42       ` Andrey Konovalov
  0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-11-13 13:28 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Dmitry Vyukov, Greg Kroah-Hartman, USB list, LKML, Andrew Morton,
	Alan Stern, Shuah Khan, Alexander Potapenko, Marco Elver,
	Aleksandr Nogikh, Nazime Hande Harputluoglu, Thomas Gleixner

On 2020-11-13 13:51:19 [+0100], Andrey Konovalov wrote:
> Hi Sebastian,

Hi Andrey,

> Replaced with what and why?

Linus requested in
	https://lkml.kernel.org/r/CAHk-=wht7kAeyR5xEW2ORj7m0hibVxZ3t+2ie8vNHLQfdbN2_g@mail.gmail.com/

that drivers should not change their behaviour on context magic like
in_atomic(), in_interrupt() and so on.
The USB bits were posted in
	https://lkml.kernel.org/r/20201019100629.419020859@linutronix.de

and merged (which is probably the same time as this patch).

I haven't look what this code should do or does but there are HCDs for
which this is never true like the UHCI/OHCI controller for instance.

> Thanks!

Sebastian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] kcov, usb: only collect coverage from __usb_hcd_giveback_urb in softirq
  2020-11-13 13:28     ` Sebastian Andrzej Siewior
@ 2020-11-13 13:42       ` Andrey Konovalov
  2020-11-13 15:32         ` Greg Kroah-Hartman
  2020-11-13 15:47         ` Marco Elver
  0 siblings, 2 replies; 9+ messages in thread
From: Andrey Konovalov @ 2020-11-13 13:42 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, Greg Kroah-Hartman
  Cc: Dmitry Vyukov, USB list, LKML, Andrew Morton, Alan Stern,
	Shuah Khan, Alexander Potapenko, Marco Elver, Aleksandr Nogikh,
	Nazime Hande Harputluoglu, Thomas Gleixner

On Fri, Nov 13, 2020 at 2:28 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> On 2020-11-13 13:51:19 [+0100], Andrey Konovalov wrote:
> > Hi Sebastian,
>
> Hi Andrey,
>
> > Replaced with what and why?
>
> Linus requested in
>         https://lkml.kernel.org/r/CAHk-=wht7kAeyR5xEW2ORj7m0hibVxZ3t+2ie8vNHLQfdbN2_g@mail.gmail.com/
>
> that drivers should not change their behaviour on context magic like
> in_atomic(), in_interrupt() and so on.
> The USB bits were posted in
>         https://lkml.kernel.org/r/20201019100629.419020859@linutronix.de
>
> and merged (which is probably the same time as this patch).
>
> I haven't look what this code should do or does but there are HCDs for
> which this is never true like the UHCI/OHCI controller for instance.

We could go back to adding softirq-specific kcov callbacks. Perhaps
with a simpler implementation than what we had before to only cover
this case. Something like kcov_remote_start_usb_softirq() and
kcov_remote_stop_softirq() that do the softirq check internally.

Greg, what would you prefer?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] kcov, usb: only collect coverage from __usb_hcd_giveback_urb in softirq
  2020-11-13 13:42       ` Andrey Konovalov
@ 2020-11-13 15:32         ` Greg Kroah-Hartman
  2020-11-13 15:47         ` Marco Elver
  1 sibling, 0 replies; 9+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-13 15:32 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Sebastian Andrzej Siewior, Dmitry Vyukov, USB list, LKML,
	Andrew Morton, Alan Stern, Shuah Khan, Alexander Potapenko,
	Marco Elver, Aleksandr Nogikh, Nazime Hande Harputluoglu,
	Thomas Gleixner

On Fri, Nov 13, 2020 at 02:42:44PM +0100, Andrey Konovalov wrote:
> On Fri, Nov 13, 2020 at 2:28 PM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> >
> > On 2020-11-13 13:51:19 [+0100], Andrey Konovalov wrote:
> > > Hi Sebastian,
> >
> > Hi Andrey,
> >
> > > Replaced with what and why?
> >
> > Linus requested in
> >         https://lkml.kernel.org/r/CAHk-=wht7kAeyR5xEW2ORj7m0hibVxZ3t+2ie8vNHLQfdbN2_g@mail.gmail.com/
> >
> > that drivers should not change their behaviour on context magic like
> > in_atomic(), in_interrupt() and so on.
> > The USB bits were posted in
> >         https://lkml.kernel.org/r/20201019100629.419020859@linutronix.de
> >
> > and merged (which is probably the same time as this patch).
> >
> > I haven't look what this code should do or does but there are HCDs for
> > which this is never true like the UHCI/OHCI controller for instance.
> 
> We could go back to adding softirq-specific kcov callbacks. Perhaps
> with a simpler implementation than what we had before to only cover
> this case. Something like kcov_remote_start_usb_softirq() and
> kcov_remote_stop_softirq() that do the softirq check internally.
> 
> Greg, what would you prefer?

I really have no idea, sorry.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] kcov, usb: only collect coverage from __usb_hcd_giveback_urb in softirq
  2020-11-13 13:42       ` Andrey Konovalov
  2020-11-13 15:32         ` Greg Kroah-Hartman
@ 2020-11-13 15:47         ` Marco Elver
  2020-11-23 23:42           ` Andrey Konovalov
  1 sibling, 1 reply; 9+ messages in thread
From: Marco Elver @ 2020-11-13 15:47 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Sebastian Andrzej Siewior, Greg Kroah-Hartman, Dmitry Vyukov,
	USB list, LKML, Andrew Morton, Alan Stern, Shuah Khan,
	Alexander Potapenko, Aleksandr Nogikh, Nazime Hande Harputluoglu,
	Thomas Gleixner

On Fri, 13 Nov 2020 at 14:42, Andrey Konovalov <andreyknvl@google.com> wrote:
> On Fri, Nov 13, 2020 at 2:28 PM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> >
> > On 2020-11-13 13:51:19 [+0100], Andrey Konovalov wrote:
> > > Hi Sebastian,
> >
> > Hi Andrey,
> >
> > > Replaced with what and why?
> >
> > Linus requested in
> >         https://lkml.kernel.org/r/CAHk-=wht7kAeyR5xEW2ORj7m0hibVxZ3t+2ie8vNHLQfdbN2_g@mail.gmail.com/
> >
> > that drivers should not change their behaviour on context magic like
> > in_atomic(), in_interrupt() and so on.
> > The USB bits were posted in
> >         https://lkml.kernel.org/r/20201019100629.419020859@linutronix.de

Arguably this patch is *not* changing "driver behaviour", it's only
changing how and when KCOV collects coverage, which is not related to
how the driver behaves.

> > and merged (which is probably the same time as this patch).
> >
> > I haven't look what this code should do or does but there are HCDs for
> > which this is never true like the UHCI/OHCI controller for instance.
>
> We could go back to adding softirq-specific kcov callbacks. Perhaps
> with a simpler implementation than what we had before to only cover
> this case. Something like kcov_remote_start_usb_softirq() and
> kcov_remote_stop_softirq() that do the softirq check internally.

Is this a matter of simply banning such functions entirely without
understanding their use? Because that sounds wrong. But if it is, we
probably have to just add some static inline functions in
include/linux/kcov.h that simply does the check.

Thanks,
-- Marco

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] kcov, usb: only collect coverage from __usb_hcd_giveback_urb in softirq
  2020-11-13 15:47         ` Marco Elver
@ 2020-11-23 23:42           ` Andrey Konovalov
  0 siblings, 0 replies; 9+ messages in thread
From: Andrey Konovalov @ 2020-11-23 23:42 UTC (permalink / raw)
  To: Marco Elver
  Cc: Sebastian Andrzej Siewior, Greg Kroah-Hartman, Dmitry Vyukov,
	USB list, LKML, Andrew Morton, Alan Stern, Shuah Khan,
	Alexander Potapenko, Aleksandr Nogikh, Nazime Hande Harputluoglu,
	Thomas Gleixner

On Fri, Nov 13, 2020 at 4:47 PM Marco Elver <elver@google.com> wrote:
>
> On Fri, 13 Nov 2020 at 14:42, Andrey Konovalov <andreyknvl@google.com> wrote:
> > On Fri, Nov 13, 2020 at 2:28 PM Sebastian Andrzej Siewior
> > <bigeasy@linutronix.de> wrote:
> > >
> > > On 2020-11-13 13:51:19 [+0100], Andrey Konovalov wrote:
> > > > Hi Sebastian,
> > >
> > > Hi Andrey,
> > >
> > > > Replaced with what and why?
> > >
> > > Linus requested in
> > >         https://lkml.kernel.org/r/CAHk-=wht7kAeyR5xEW2ORj7m0hibVxZ3t+2ie8vNHLQfdbN2_g@mail.gmail.com/
> > >
> > > that drivers should not change their behaviour on context magic like
> > > in_atomic(), in_interrupt() and so on.
> > > The USB bits were posted in
> > >         https://lkml.kernel.org/r/20201019100629.419020859@linutronix.de
>
> Arguably this patch is *not* changing "driver behaviour", it's only
> changing how and when KCOV collects coverage, which is not related to
> how the driver behaves.
>
> > > and merged (which is probably the same time as this patch).
> > >
> > > I haven't look what this code should do or does but there are HCDs for
> > > which this is never true like the UHCI/OHCI controller for instance.
> >
> > We could go back to adding softirq-specific kcov callbacks. Perhaps
> > with a simpler implementation than what we had before to only cover
> > this case. Something like kcov_remote_start_usb_softirq() and
> > kcov_remote_stop_softirq() that do the softirq check internally.
>
> Is this a matter of simply banning such functions entirely without
> understanding their use? Because that sounds wrong. But if it is, we
> probably have to just add some static inline functions in
> include/linux/kcov.h that simply does the check.

Yeah, this seems like a solution that will satisfy everyone. Will mail
a new version shortly.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16 13:57 [PATCH v4] kcov, usb: only collect coverage from __usb_hcd_giveback_urb in softirq Andrey Konovalov
2020-10-16 14:33 ` Dmitry Vyukov
2020-11-13 12:30 ` Sebastian Andrzej Siewior
2020-11-13 12:51   ` Andrey Konovalov
2020-11-13 13:28     ` Sebastian Andrzej Siewior
2020-11-13 13:42       ` Andrey Konovalov
2020-11-13 15:32         ` Greg Kroah-Hartman
2020-11-13 15:47         ` Marco Elver
2020-11-23 23:42           ` Andrey Konovalov

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git