Skip to content

Commit f85d39d

Browse files
xairygregkh
authored andcommitted
kcov, usb: disable interrupts in kcov_remote_start_usb_softirq
After commit 8fea0c8 ("usb: core: hcd: Convert from tasklet to BH workqueue"), usb_giveback_urb_bh() runs in the BH workqueue with interrupts enabled. Thus, the remote coverage collection section in usb_giveback_urb_bh()-> __usb_hcd_giveback_urb() might be interrupted, and the interrupt handler might invoke __usb_hcd_giveback_urb() again. This breaks KCOV, as it does not support nested remote coverage collection sections within the same context (neither in task nor in softirq). Update kcov_remote_start/stop_usb_softirq() to disable interrupts for the duration of the coverage collection section to avoid nested sections in the softirq context (in addition to such in the task context, which are already handled). Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> Closes: https://lore.kernel.org/linux-usb/0f4d1964-7397-485b-bc48-11c01e2fcbca@I-love.SAKURA.ne.jp/ Closes: https://syzkaller.appspot.com/bug?extid=0438378d6f157baae1a2 Suggested-by: Alan Stern <stern@rowland.harvard.edu> Fixes: 8fea0c8 ("usb: core: hcd: Convert from tasklet to BH workqueue") Cc: stable@vger.kernel.org Acked-by: Dmitry Vyukov <dvyukov@google.com> Signed-off-by: Andrey Konovalov <andreyknvl@gmail.com> Link: https://lore.kernel.org/r/20240527173538.4989-1-andrey.konovalov@linux.dev Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent e4228cf commit f85d39d

File tree

2 files changed

+45
-14
lines changed

2 files changed

+45
-14
lines changed

drivers/usb/core/hcd.c

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1623,6 +1623,7 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
16231623
struct usb_hcd *hcd = bus_to_hcd(urb->dev->bus);
16241624
struct usb_anchor *anchor = urb->anchor;
16251625
int status = urb->unlinked;
1626+
unsigned long flags;
16261627

16271628
urb->hcpriv = NULL;
16281629
if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) &&
@@ -1640,13 +1641,14 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
16401641
/* pass ownership to the completion handler */
16411642
urb->status = status;
16421643
/*
1643-
* This function can be called in task context inside another remote
1644-
* coverage collection section, but kcov doesn't support that kind of
1645-
* recursion yet. Only collect coverage in softirq context for now.
1644+
* Only collect coverage in the softirq context and disable interrupts
1645+
* to avoid scenarios with nested remote coverage collection sections
1646+
* that KCOV does not support.
1647+
* See the comment next to kcov_remote_start_usb_softirq() for details.
16461648
*/
1647-
kcov_remote_start_usb_softirq((u64)urb->dev->bus->busnum);
1649+
flags = kcov_remote_start_usb_softirq((u64)urb->dev->bus->busnum);
16481650
urb->complete(urb);
1649-
kcov_remote_stop_softirq();
1651+
kcov_remote_stop_softirq(flags);
16501652

16511653
usb_anchor_resume_wakeups(anchor);
16521654
atomic_dec(&urb->use_count);

include/linux/kcov.h

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,21 +55,47 @@ static inline void kcov_remote_start_usb(u64 id)
5555

5656
/*
5757
* The softirq flavor of kcov_remote_*() functions is introduced as a temporary
58-
* work around for kcov's lack of nested remote coverage sections support in
59-
* task context. Adding support for nested sections is tracked in:
60-
* https://bugzilla.kernel.org/show_bug.cgi?id=210337
58+
* workaround for KCOV's lack of nested remote coverage sections support.
59+
*
60+
* Adding support is tracked in https://bugzilla.kernel.org/show_bug.cgi?id=210337.
61+
*
62+
* kcov_remote_start_usb_softirq():
63+
*
64+
* 1. Only collects coverage when called in the softirq context. This allows
65+
* avoiding nested remote coverage collection sections in the task context.
66+
* For example, USB/IP calls usb_hcd_giveback_urb() in the task context
67+
* within an existing remote coverage collection section. Thus, KCOV should
68+
* not attempt to start collecting coverage within the coverage collection
69+
* section in __usb_hcd_giveback_urb() in this case.
70+
*
71+
* 2. Disables interrupts for the duration of the coverage collection section.
72+
* This allows avoiding nested remote coverage collection sections in the
73+
* softirq context (a softirq might occur during the execution of a work in
74+
* the BH workqueue, which runs with in_serving_softirq() > 0).
75+
* For example, usb_giveback_urb_bh() runs in the BH workqueue with
76+
* interrupts enabled, so __usb_hcd_giveback_urb() might be interrupted in
77+
* the middle of its remote coverage collection section, and the interrupt
78+
* handler might invoke __usb_hcd_giveback_urb() again.
6179
*/
6280

63-
static inline void kcov_remote_start_usb_softirq(u64 id)
81+
static inline unsigned long kcov_remote_start_usb_softirq(u64 id)
6482
{
65-
if (in_serving_softirq())
83+
unsigned long flags = 0;
84+
85+
if (in_serving_softirq()) {
86+
local_irq_save(flags);
6687
kcov_remote_start_usb(id);
88+
}
89+
90+
return flags;
6791
}
6892

69-
static inline void kcov_remote_stop_softirq(void)
93+
static inline void kcov_remote_stop_softirq(unsigned long flags)
7094
{
71-
if (in_serving_softirq())
95+
if (in_serving_softirq()) {
7296
kcov_remote_stop();
97+
local_irq_restore(flags);
98+
}
7399
}
74100

75101
#ifdef CONFIG_64BIT
@@ -103,8 +129,11 @@ static inline u64 kcov_common_handle(void)
103129
}
104130
static inline void kcov_remote_start_common(u64 id) {}
105131
static inline void kcov_remote_start_usb(u64 id) {}
106-
static inline void kcov_remote_start_usb_softirq(u64 id) {}
107-
static inline void kcov_remote_stop_softirq(void) {}
132+
static inline unsigned long kcov_remote_start_usb_softirq(u64 id)
133+
{
134+
return 0;
135+
}
136+
static inline void kcov_remote_stop_softirq(unsigned long flags) {}
108137

109138
#endif /* CONFIG_KCOV */
110139
#endif /* _LINUX_KCOV_H */

0 commit comments

Comments
 (0)