Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4] kcov, usb: specify contexts for remote coverage sections
@ 2020-10-12 17:17 Andrey Konovalov
  2020-10-13  6:46 ` Dmitry Vyukov
  0 siblings, 1 reply; 5+ messages in thread
From: Andrey Konovalov @ 2020-10-12 17:17 UTC (permalink / raw)
  To: Dmitry Vyukov, Andrew Morton
  Cc: linux-usb, linux-kernel, Greg Kroah-Hartman, Alan Stern,
	Shuah Khan, Alexander Potapenko, Marco Elver, Aleksandr Nogikh,
	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.

The approach this patch takes is to add another set of kcov_remote_*()
callbacks that specify the context they are supposed to be executed in.
If the current context doesn't match the mask provided to a callback,
that callback is ignored. KCOV currently only supports collecting remote
coverage in two contexts: task and softirq. This patch constraints KCOV to
only collect coverage from __usb_hcd_giveback_urb() when it's executed in
softirq context.

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:
- Drop unnecessary returns from kcov callbacks.

---
 Documentation/dev-tools/kcov.rst |  6 ++++++
 drivers/usb/core/hcd.c           |  4 ++--
 include/linux/kcov.h             | 31 +++++++++++++++++++++++++++++--
 kernel/kcov.c                    | 26 +++++++++++++++++++-------
 4 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
index 8548b0b04e43..2c0f58988512 100644
--- a/Documentation/dev-tools/kcov.rst
+++ b/Documentation/dev-tools/kcov.rst
@@ -235,6 +235,12 @@ saved to the kcov_handle field in the current task_struct and needs to be
 passed to the newly spawned threads via custom annotations. Those threads
 should in turn be annotated with kcov_remote_start()/kcov_remote_stop().
 
+Besides the annotations that only accept a handle, there are also
+kcov_remote_start_context()/kcov_remote_stop_context() that accept a
+context mask. This mask describes the contexts in which these annotations
+should be applied. E.g. specifying KCOV_CONTEXT_SOFTIRQ will result in the
+corresponding annotations being ignored in any context other than softirq.
+
 Internally kcov stores handles as u64 integers. The top byte of a handle
 is used to denote the id of a subsystem that this handle belongs to, and
 the lower 4 bytes are used to denote the id of a thread instance within
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index a33b849e8beb..ea93d9ebcb2e 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1646,9 +1646,9 @@ 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);
+	kcov_remote_start_usb_softirq((u64)urb->dev->bus->busnum);
 	urb->complete(urb);
-	kcov_remote_stop();
+	kcov_remote_stop_softirq();
 
 	usb_anchor_resume_wakeups(anchor);
 	atomic_dec(&urb->use_count);
diff --git a/include/linux/kcov.h b/include/linux/kcov.h
index a10e84707d82..a9c025c3e1df 100644
--- a/include/linux/kcov.h
+++ b/include/linux/kcov.h
@@ -22,6 +22,10 @@ enum kcov_mode {
 	KCOV_MODE_TRACE_CMP = 3,
 };
 
+#define KCOV_CONTEXT_TASK	(1u << 0)
+#define KCOV_CONTEXT_SOFTIRQ	(1u << 1)
+#define KCOV_CONTEXT_MASK	(KCOV_CONTEXT_TASK | KCOV_CONTEXT_SOFTIRQ)
+
 #define KCOV_IN_CTXSW	(1 << 30)
 
 void kcov_task_init(struct task_struct *t);
@@ -38,10 +42,21 @@ do {						\
 } while (0)
 
 /* See Documentation/dev-tools/kcov.rst for usage details. */
-void kcov_remote_start(u64 handle);
-void kcov_remote_stop(void);
+
+void kcov_remote_start_context(u64 handle, unsigned int context);
+void kcov_remote_stop_context(unsigned int context);
 u64 kcov_common_handle(void);
 
+static inline void kcov_remote_start(u64 handle)
+{
+	kcov_remote_start_context(handle, KCOV_CONTEXT_MASK);
+}
+
+static inline void kcov_remote_stop(void)
+{
+	kcov_remote_stop_context(KCOV_CONTEXT_MASK);
+}
+
 static inline void kcov_remote_start_common(u64 id)
 {
 	kcov_remote_start(kcov_remote_handle(KCOV_SUBSYSTEM_COMMON, id));
@@ -52,6 +67,16 @@ static inline void kcov_remote_start_usb(u64 id)
 	kcov_remote_start(kcov_remote_handle(KCOV_SUBSYSTEM_USB, id));
 }
 
+static inline void kcov_remote_start_usb_softirq(u64 id)
+{
+	kcov_remote_start_context(kcov_remote_handle(KCOV_SUBSYSTEM_USB, id), KCOV_CONTEXT_SOFTIRQ);
+}
+
+static inline void kcov_remote_stop_softirq(void)
+{
+	kcov_remote_stop_context(KCOV_CONTEXT_SOFTIRQ);
+}
+
 #else
 
 static inline void kcov_task_init(struct task_struct *t) {}
@@ -66,6 +91,8 @@ static inline u64 kcov_common_handle(void)
 }
 static inline void kcov_remote_start_common(u64 id) {}
 static inline void kcov_remote_start_usb(u64 id) {}
+static inline void kcov_remote_start_usb_softirq(u64 id) {}
+static inline void kcov_remote_stop_softirq(void) {}
 
 #endif /* CONFIG_KCOV */
 #endif /* _LINUX_KCOV_H */
diff --git a/kernel/kcov.c b/kernel/kcov.c
index 6b8368be89c8..3ccdbe060f47 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -808,7 +808,8 @@ static void kcov_remote_softirq_stop(struct task_struct *t)
 	}
 }
 
-void kcov_remote_start(u64 handle)
+/* Also see kcov_remote_start() defined in include/linux/kcov.h. */
+void kcov_remote_start_context(u64 handle, unsigned int context)
 {
 	struct task_struct *t = current;
 	struct kcov_remote *remote;
@@ -821,7 +822,11 @@ void kcov_remote_start(u64 handle)
 
 	if (WARN_ON(!kcov_check_handle(handle, true, true, true)))
 		return;
-	if (!in_task() && !in_serving_softirq())
+	if (WARN_ON((context & ~KCOV_CONTEXT_MASK) || !context))
+		return;
+	if (in_task() && !(context & KCOV_CONTEXT_TASK))
+		return;
+	if (in_serving_softirq() && !(context & KCOV_CONTEXT_SOFTIRQ))
 		return;
 
 	local_irq_save(flags);
@@ -894,7 +899,7 @@ void kcov_remote_start(u64 handle)
 	local_irq_restore(flags);
 
 }
-EXPORT_SYMBOL(kcov_remote_start);
+EXPORT_SYMBOL(kcov_remote_start_context);
 
 static void kcov_move_area(enum kcov_mode mode, void *dst_area,
 				unsigned int dst_area_size, void *src_area)
@@ -951,8 +956,11 @@ static void kcov_move_area(enum kcov_mode mode, void *dst_area,
 	}
 }
 
-/* See the comment before kcov_remote_start() for usage details. */
-void kcov_remote_stop(void)
+/*
+ * Also see kcov_remote_stop() defined in include/linux/kcov.h.
+ * See the comment before kcov_remote_start_context() for usage details.
+ */
+void kcov_remote_stop_context(unsigned int context)
 {
 	struct task_struct *t = current;
 	struct kcov *kcov;
@@ -962,7 +970,11 @@ void kcov_remote_stop(void)
 	int sequence;
 	unsigned long flags;
 
-	if (!in_task() && !in_serving_softirq())
+	if (WARN_ON((context & ~KCOV_CONTEXT_MASK) || !context))
+		return;
+	if (in_task() && !(context & KCOV_CONTEXT_TASK))
+		return;
+	if (in_serving_softirq() && !(context & KCOV_CONTEXT_SOFTIRQ))
 		return;
 
 	local_irq_save(flags);
@@ -1018,7 +1030,7 @@ void kcov_remote_stop(void)
 	/* Get in kcov_remote_start(). */
 	kcov_put(kcov);
 }
-EXPORT_SYMBOL(kcov_remote_stop);
+EXPORT_SYMBOL(kcov_remote_stop_context);
 
 /* See the comment before kcov_remote_start() for usage details. */
 u64 kcov_common_handle(void)
-- 
2.28.0.1011.ga647a8990f-goog


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

* Re: [PATCH v4] kcov, usb: specify contexts for remote coverage sections
  2020-10-12 17:17 [PATCH v4] kcov, usb: specify contexts for remote coverage sections Andrey Konovalov
@ 2020-10-13  6:46 ` Dmitry Vyukov
  2020-10-13 13:58   ` Andrey Konovalov
  0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Vyukov @ 2020-10-13  6:46 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Andrew Morton, USB list, LKML, Greg Kroah-Hartman, Alan Stern,
	Shuah Khan, Alexander Potapenko, Marco Elver, Aleksandr Nogikh

On Mon, Oct 12, 2020 at 7:17 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?

Or is there recursion between task context and softirq context? 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?
Is this API supposed to be final? Or it only puts down fire re the warning?

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.
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.
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?



> The approach this patch takes is to add another set of kcov_remote_*()
> callbacks that specify the context they are supposed to be executed in.
> If the current context doesn't match the mask provided to a callback,
> that callback is ignored. KCOV currently only supports collecting remote
> coverage in two contexts: task and softirq. This patch constraints KCOV to
> only collect coverage from __usb_hcd_giveback_urb() when it's executed in
> softirq context.
>
> 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:
> - Drop unnecessary returns from kcov callbacks.
>
> ---
>  Documentation/dev-tools/kcov.rst |  6 ++++++
>  drivers/usb/core/hcd.c           |  4 ++--
>  include/linux/kcov.h             | 31 +++++++++++++++++++++++++++++--
>  kernel/kcov.c                    | 26 +++++++++++++++++++-------
>  4 files changed, 56 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
> index 8548b0b04e43..2c0f58988512 100644
> --- a/Documentation/dev-tools/kcov.rst
> +++ b/Documentation/dev-tools/kcov.rst
> @@ -235,6 +235,12 @@ saved to the kcov_handle field in the current task_struct and needs to be
>  passed to the newly spawned threads via custom annotations. Those threads
>  should in turn be annotated with kcov_remote_start()/kcov_remote_stop().
>
> +Besides the annotations that only accept a handle, there are also
> +kcov_remote_start_context()/kcov_remote_stop_context() that accept a
> +context mask. This mask describes the contexts in which these annotations
> +should be applied. E.g. specifying KCOV_CONTEXT_SOFTIRQ will result in the
> +corresponding annotations being ignored in any context other than softirq.
> +
>  Internally kcov stores handles as u64 integers. The top byte of a handle
>  is used to denote the id of a subsystem that this handle belongs to, and
>  the lower 4 bytes are used to denote the id of a thread instance within
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index a33b849e8beb..ea93d9ebcb2e 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1646,9 +1646,9 @@ 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);
> +       kcov_remote_start_usb_softirq((u64)urb->dev->bus->busnum);
>         urb->complete(urb);
> -       kcov_remote_stop();
> +       kcov_remote_stop_softirq();
>
>         usb_anchor_resume_wakeups(anchor);
>         atomic_dec(&urb->use_count);
> diff --git a/include/linux/kcov.h b/include/linux/kcov.h
> index a10e84707d82..a9c025c3e1df 100644
> --- a/include/linux/kcov.h
> +++ b/include/linux/kcov.h
> @@ -22,6 +22,10 @@ enum kcov_mode {
>         KCOV_MODE_TRACE_CMP = 3,
>  };
>
> +#define KCOV_CONTEXT_TASK      (1u << 0)
> +#define KCOV_CONTEXT_SOFTIRQ   (1u << 1)
> +#define KCOV_CONTEXT_MASK      (KCOV_CONTEXT_TASK | KCOV_CONTEXT_SOFTIRQ)
> +
>  #define KCOV_IN_CTXSW  (1 << 30)
>
>  void kcov_task_init(struct task_struct *t);
> @@ -38,10 +42,21 @@ do {                                                \
>  } while (0)
>
>  /* See Documentation/dev-tools/kcov.rst for usage details. */
> -void kcov_remote_start(u64 handle);
> -void kcov_remote_stop(void);
> +
> +void kcov_remote_start_context(u64 handle, unsigned int context);
> +void kcov_remote_stop_context(unsigned int context);
>  u64 kcov_common_handle(void);
>
> +static inline void kcov_remote_start(u64 handle)
> +{
> +       kcov_remote_start_context(handle, KCOV_CONTEXT_MASK);
> +}
> +
> +static inline void kcov_remote_stop(void)
> +{
> +       kcov_remote_stop_context(KCOV_CONTEXT_MASK);
> +}
> +
>  static inline void kcov_remote_start_common(u64 id)
>  {
>         kcov_remote_start(kcov_remote_handle(KCOV_SUBSYSTEM_COMMON, id));
> @@ -52,6 +67,16 @@ static inline void kcov_remote_start_usb(u64 id)
>         kcov_remote_start(kcov_remote_handle(KCOV_SUBSYSTEM_USB, id));
>  }
>
> +static inline void kcov_remote_start_usb_softirq(u64 id)
> +{
> +       kcov_remote_start_context(kcov_remote_handle(KCOV_SUBSYSTEM_USB, id), KCOV_CONTEXT_SOFTIRQ);
> +}
> +
> +static inline void kcov_remote_stop_softirq(void)
> +{
> +       kcov_remote_stop_context(KCOV_CONTEXT_SOFTIRQ);
> +}
> +
>  #else
>
>  static inline void kcov_task_init(struct task_struct *t) {}
> @@ -66,6 +91,8 @@ static inline u64 kcov_common_handle(void)
>  }
>  static inline void kcov_remote_start_common(u64 id) {}
>  static inline void kcov_remote_start_usb(u64 id) {}
> +static inline void kcov_remote_start_usb_softirq(u64 id) {}
> +static inline void kcov_remote_stop_softirq(void) {}
>
>  #endif /* CONFIG_KCOV */
>  #endif /* _LINUX_KCOV_H */
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 6b8368be89c8..3ccdbe060f47 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -808,7 +808,8 @@ static void kcov_remote_softirq_stop(struct task_struct *t)
>         }
>  }
>
> -void kcov_remote_start(u64 handle)
> +/* Also see kcov_remote_start() defined in include/linux/kcov.h. */
> +void kcov_remote_start_context(u64 handle, unsigned int context)
>  {
>         struct task_struct *t = current;
>         struct kcov_remote *remote;
> @@ -821,7 +822,11 @@ void kcov_remote_start(u64 handle)
>
>         if (WARN_ON(!kcov_check_handle(handle, true, true, true)))
>                 return;
> -       if (!in_task() && !in_serving_softirq())
> +       if (WARN_ON((context & ~KCOV_CONTEXT_MASK) || !context))
> +               return;
> +       if (in_task() && !(context & KCOV_CONTEXT_TASK))
> +               return;
> +       if (in_serving_softirq() && !(context & KCOV_CONTEXT_SOFTIRQ))
>                 return;
>
>         local_irq_save(flags);
> @@ -894,7 +899,7 @@ void kcov_remote_start(u64 handle)
>         local_irq_restore(flags);
>
>  }
> -EXPORT_SYMBOL(kcov_remote_start);
> +EXPORT_SYMBOL(kcov_remote_start_context);
>
>  static void kcov_move_area(enum kcov_mode mode, void *dst_area,
>                                 unsigned int dst_area_size, void *src_area)
> @@ -951,8 +956,11 @@ static void kcov_move_area(enum kcov_mode mode, void *dst_area,
>         }
>  }
>
> -/* See the comment before kcov_remote_start() for usage details. */
> -void kcov_remote_stop(void)
> +/*
> + * Also see kcov_remote_stop() defined in include/linux/kcov.h.
> + * See the comment before kcov_remote_start_context() for usage details.
> + */
> +void kcov_remote_stop_context(unsigned int context)
>  {
>         struct task_struct *t = current;
>         struct kcov *kcov;
> @@ -962,7 +970,11 @@ void kcov_remote_stop(void)
>         int sequence;
>         unsigned long flags;
>
> -       if (!in_task() && !in_serving_softirq())
> +       if (WARN_ON((context & ~KCOV_CONTEXT_MASK) || !context))
> +               return;
> +       if (in_task() && !(context & KCOV_CONTEXT_TASK))
> +               return;
> +       if (in_serving_softirq() && !(context & KCOV_CONTEXT_SOFTIRQ))
>                 return;
>
>         local_irq_save(flags);
> @@ -1018,7 +1030,7 @@ void kcov_remote_stop(void)
>         /* Get in kcov_remote_start(). */
>         kcov_put(kcov);
>  }
> -EXPORT_SYMBOL(kcov_remote_stop);
> +EXPORT_SYMBOL(kcov_remote_stop_context);
>
>  /* See the comment before kcov_remote_start() for usage details. */
>  u64 kcov_common_handle(void)
> --
> 2.28.0.1011.ga647a8990f-goog
>

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

* Re: [PATCH v4] kcov, usb: specify contexts for remote coverage sections
  2020-10-13  6:46 ` Dmitry Vyukov
@ 2020-10-13 13:58   ` Andrey Konovalov
  2020-10-13 14:15     ` Dmitry Vyukov
  0 siblings, 1 reply; 5+ messages in thread
From: Andrey Konovalov @ 2020-10-13 13:58 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Andrew Morton, USB list, LKML, Greg Kroah-Hartman, Alan Stern,
	Shuah Khan, Alexander Potapenko, Marco Elver, Aleksandr Nogikh

On Tue, Oct 13, 2020 at 8:46 AM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Mon, Oct 12, 2020 at 7:17 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.

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

* Re: [PATCH v4] kcov, usb: specify contexts for remote coverage sections
  2020-10-13 13:58   ` Andrey Konovalov
@ 2020-10-13 14:15     ` Dmitry Vyukov
  2020-10-13 14:38       ` Andrey Konovalov
  0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Vyukov @ 2020-10-13 14:15 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Andrew Morton, USB list, LKML, Greg Kroah-Hartman, Alan Stern,
	Shuah Khan, Alexander Potapenko, Marco Elver, Aleksandr Nogikh

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);

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

* Re: [PATCH v4] kcov, usb: specify contexts for remote coverage sections
  2020-10-13 14:15     ` Dmitry Vyukov
@ 2020-10-13 14:38       ` Andrey Konovalov
  0 siblings, 0 replies; 5+ messages in thread
From: Andrey Konovalov @ 2020-10-13 14:38 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Andrew Morton, USB list, LKML, Greg Kroah-Hartman, Alan Stern,
	Shuah Khan, Alexander Potapenko, Marco Elver, Aleksandr Nogikh

On Tue, Oct 13, 2020 at 4:15 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> 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);

Hmm, this actually makes a lot of sense. I wonder why I haven't
thought of that :) Will do in v4.

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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-12 17:17 [PATCH v4] kcov, usb: specify contexts for remote coverage sections Andrey Konovalov
2020-10-13  6:46 ` Dmitry Vyukov
2020-10-13 13:58   ` Andrey Konovalov
2020-10-13 14:15     ` Dmitry Vyukov
2020-10-13 14:38       ` 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