LinuxLists.cc - [PATCH] kcov, usb: disable interrupts in kcov_remote_start_usb

2024-05-20 20:59:16

Subject: [PATCH] kcov, usb: disable interrupts in kcov_remote_start_usb_softirq

From: Andrey Konovalov <[email protected]>

After commit 8fea0c8fda30 ("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 <[email protected]>
Closes: https://lore.kernel.org/linux-usb/[email protected]/
Closes: https://syzkaller.appspot.com/bug?extid=0438378d6f157baae1a2
Suggested-by: Alan Stern <[email protected]>
Fixes: 8fea0c8fda30 ("usb: core: hcd: Convert from tasklet to BH workqueue")
Signed-off-by: Andrey Konovalov <[email protected]>
---
drivers/usb/core/hcd.c | 12 +++++++-----
include/linux/kcov.h | 44 +++++++++++++++++++++++++++++++++---------
2 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index c0e005670d67..fb1aa0d4fc28 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1623,6 +1623,7 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
struct usb_hcd *hcd = bus_to_hcd(urb->dev->bus);
struct usb_anchor *anchor = urb->anchor;
int status = urb->unlinked;
+ unsigned long flags;

urb->hcpriv = NULL;
if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) &&
@@ -1640,13 +1641,14 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
/* pass ownership to the completion handler */
urb->status = status;
/*
- * 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.
+ * Only collect coverage in the softirq context and disable interrupts
+ * to avoid scenarios with nested remote coverage collection sections
+ * that KCOV does not support.
+ * See the comment next to kcov_remote_start_usb_softirq() for details.
*/
- kcov_remote_start_usb_softirq((u64)urb->dev->bus->busnum);
+ flags = kcov_remote_start_usb_softirq((u64)urb->dev->bus->busnum);
urb->complete(urb);
- kcov_remote_stop_softirq();
+ kcov_remote_stop_softirq(flags);

usb_anchor_resume_wakeups(anchor);
atomic_dec(&urb->use_count);
diff --git a/include/linux/kcov.h b/include/linux/kcov.h
index b851ba415e03..ebcfc271aee3 100644
--- a/include/linux/kcov.h
+++ b/include/linux/kcov.h
@@ -55,21 +55,47 @@ static inline void kcov_remote_start_usb(u64 id)

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

-static inline void kcov_remote_start_usb_softirq(u64 id)
+static inline unsigned long kcov_remote_start_usb_softirq(u64 id)
{
- if (in_serving_softirq())
+ unsigned long flags = 0;
+
+ if (in_serving_softirq()) {
+ local_irq_save(flags);
kcov_remote_start_usb(id);
+ }
+
+ return flags;
}

-static inline void kcov_remote_stop_softirq(void)
+static inline void kcov_remote_stop_softirq(unsigned long flags)
{
- if (in_serving_softirq())
+ if (in_serving_softirq()) {
kcov_remote_stop();
+ local_irq_restore(flags);
+ }
}

#ifdef CONFIG_64BIT
@@ -103,8 +129,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) {}
+static inline unsigned long kcov_remote_start_usb_softirq(u64 id) {}
+static inline void kcov_remote_stop_softirq(unsigned long flags) {}

#endif /* CONFIG_KCOV */
#endif /* _LINUX_KCOV_H */
--
2.25.1

2024-05-21 01:26:37

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] kcov, usb: disable interrupts in kcov_remote_start_usb_softirq

2024-05-21 01:57:28

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] kcov, usb: disable interrupts in kcov_remote_start_usb_softirq

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus westeri-thunderbolt/next linus/master v6.9 next-20240520]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/andrey-konovalov-linux-dev/kcov-usb-disable-interrupts-in-kcov_remote_start_usb_softirq/20240521-050030
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/20240520205856.162910-1-andrey.konovalov%40linux.dev
patch subject: [PATCH] kcov, usb: disable interrupts in kcov_remote_start_usb_softirq
config: s390-allnoconfig (https://download.01.org/0day-ci/archive/20240521/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project fa9b1be45088dce1e4b602d451f118128b94237b)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240521/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

In file included from kernel/fork.c:30:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:173:
In file included from arch/s390/include/asm/mmu_context.h:11:
In file included from arch/s390/include/asm/pgalloc.h:18:
In file included from include/linux/mm.h:2210:
include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
In file included from kernel/fork.c:46:
include/linux/mm_inline.h:47:41: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
47 | __mod_lruvec_state(lruvec, NR_LRU_BASE + lru, nr_pages);
| ~~~~~~~~~~~ ^ ~~~
include/linux/mm_inline.h:49:22: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
49 | NR_ZONE_LRU_BASE + lru, nr_pages);
| ~~~~~~~~~~~~~~~~ ^ ~~~
In file included from kernel/fork.c:79:
In file included from include/linux/tty.h:11:
In file included from include/linux/tty_port.h:5:
In file included from include/linux/kfifo.h:42:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:78:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
| ^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
| ^
In file included from kernel/fork.c:79:
In file included from include/linux/tty.h:11:
In file included from include/linux/tty_port.h:5:
In file included from include/linux/kfifo.h:42:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:78:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
| ^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
| ^
In file included from kernel/fork.c:79:
In file included from include/linux/tty.h:11:
In file included from include/linux/tty_port.h:5:
In file included from include/linux/kfifo.h:42:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:78:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
692 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
700 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
708 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
717 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
726 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
735 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
In file included from kernel/fork.c:92:
>> include/linux/kcov.h:132:68: warning: non-void function does not return a value [-Wreturn-type]
132 | static inline unsigned long kcov_remote_start_usb_softirq(u64 id) {}
| ^
16 warnings generated.
--
In file included from kernel/exit.c:8:
In file included from include/linux/mm.h:2210:
include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
In file included from kernel/exit.c:21:
In file included from include/linux/tty.h:11:
In file included from include/linux/tty_port.h:5:
In file included from include/linux/kfifo.h:42:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:78:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
| ^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
| ^
In file included from kernel/exit.c:21:
In file included from include/linux/tty.h:11:
In file included from include/linux/tty_port.h:5:
In file included from include/linux/kfifo.h:42:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:78:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
| ^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
| ^
In file included from kernel/exit.c:21:
In file included from include/linux/tty.h:11:
In file included from include/linux/tty_port.h:5:
In file included from include/linux/kfifo.h:42:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:78:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
692 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
700 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
708 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
717 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
726 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
735 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
In file included from kernel/exit.c:62:
>> include/linux/kcov.h:132:68: warning: non-void function does not return a value [-Wreturn-type]
132 | static inline unsigned long kcov_remote_start_usb_softirq(u64 id) {}
| ^
14 warnings generated.
--
In file included from kernel/sched/core.c:9:
In file included from include/linux/highmem.h:10:
In file included from include/linux/mm.h:2210:
include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
In file included from kernel/sched/core.c:33:
In file included from include/linux/sched/isolation.h:7:
In file included from include/linux/tick.h:8:
In file included from include/linux/clockchips.h:14:
In file included from include/linux/clocksource.h:22:
In file included from arch/s390/include/asm/io.h:78:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
| ^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
| ^
In file included from kernel/sched/core.c:33:
In file included from include/linux/sched/isolation.h:7:
In file included from include/linux/tick.h:8:
In file included from include/linux/clockchips.h:14:
In file included from include/linux/clocksource.h:22:
In file included from arch/s390/include/asm/io.h:78:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
| ^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
| ^
In file included from kernel/sched/core.c:33:
In file included from include/linux/sched/isolation.h:7:
In file included from include/linux/tick.h:8:
In file included from include/linux/clockchips.h:14:
In file included from include/linux/clocksource.h:22:
In file included from arch/s390/include/asm/io.h:78:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
692 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
700 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
708 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
717 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
726 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
735 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
In file included from kernel/sched/core.c:48:
>> include/linux/kcov.h:132:68: warning: non-void function does not return a value [-Wreturn-type]
132 | static inline unsigned long kcov_remote_start_usb_softirq(u64 id) {}
| ^
kernel/sched/core.c:6548:20: warning: unused function 'sched_core_cpu_deactivate' [-Wunused-function]
6548 | static inline void sched_core_cpu_deactivate(unsigned int cpu) {}
| ^~~~~~~~~~~~~~~~~~~~~~~~~
15 warnings generated.

vim +132 include/linux/kcov.h

119
120 static inline void kcov_task_init(struct task_struct *t) {}
121 static inline void kcov_task_exit(struct task_struct *t) {}
122 static inline void kcov_prepare_switch(struct task_struct *t) {}
123 static inline void kcov_finish_switch(struct task_struct *t) {}
124 static inline void kcov_remote_start(u64 handle) {}
125 static inline void kcov_remote_stop(void) {}
126 static inline u64 kcov_common_handle(void)
127 {
128 return 0;
129 }
130 static inline void kcov_remote_start_common(u64 id) {}
131 static inline void kcov_remote_start_usb(u64 id) {}
> 132 static inline unsigned long kcov_remote_start_usb_softirq(u64 id) {}
133 static inline void kcov_remote_stop_softirq(unsigned long flags) {}
134

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2024-05-21 04:36:09

by Dmitry Vyukov

[permalink] [raw]

Subject: Re: [PATCH] kcov, usb: disable interrupts in kcov_remote_start_usb_softirq

On Mon, 20 May 2024 at 22:59, <[email protected]> wrote:
>
> From: Andrey Konovalov <[email protected]>
>
> After commit 8fea0c8fda30 ("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).

Besides the issue pointed by the test robot:

Acked-by: Dmitry Vyukov <[email protected]>

Thanks for fixing this.

This section of code does not rely on reentrancy, right? E.g. one
callback won't wait for completion of another callback?

At some point we started seeing lots of "remote cover enable write
trace failed (errno 17)" errors while running syzkaller. Can these
errors be caused by this issue?

> Reported-by: Tetsuo Handa <[email protected]>
> Closes: https://lore.kernel.org/linux-usb/[email protected]/
> Closes: https://syzkaller.appspot.com/bug?extid=0438378d6f157baae1a2
> Suggested-by: Alan Stern <[email protected]>
> Fixes: 8fea0c8fda30 ("usb: core: hcd: Convert from tasklet to BH workqueue")
> Signed-off-by: Andrey Konovalov <[email protected]>
> ---
> drivers/usb/core/hcd.c | 12 +++++++-----
> include/linux/kcov.h | 44 +++++++++++++++++++++++++++++++++---------
> 2 files changed, 42 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index c0e005670d67..fb1aa0d4fc28 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1623,6 +1623,7 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
> struct usb_hcd *hcd = bus_to_hcd(urb->dev->bus);
> struct usb_anchor *anchor = urb->anchor;
> int status = urb->unlinked;
> + unsigned long flags;
>
> urb->hcpriv = NULL;
> if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) &&
> @@ -1640,13 +1641,14 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
> /* pass ownership to the completion handler */
> urb->status = status;
> /*
> - * 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.
> + * Only collect coverage in the softirq context and disable interrupts
> + * to avoid scenarios with nested remote coverage collection sections
> + * that KCOV does not support.
> + * See the comment next to kcov_remote_start_usb_softirq() for details.
> */
> - kcov_remote_start_usb_softirq((u64)urb->dev->bus->busnum);
> + flags = kcov_remote_start_usb_softirq((u64)urb->dev->bus->busnum);
> urb->complete(urb);
> - kcov_remote_stop_softirq();
> + kcov_remote_stop_softirq(flags);
>
> usb_anchor_resume_wakeups(anchor);
> atomic_dec(&urb->use_count);
> diff --git a/include/linux/kcov.h b/include/linux/kcov.h
> index b851ba415e03..ebcfc271aee3 100644
> --- a/include/linux/kcov.h
> +++ b/include/linux/kcov.h
> @@ -55,21 +55,47 @@ static inline void kcov_remote_start_usb(u64 id)
>
> /*
> * The softirq flavor of kcov_remote_*() functions is introduced as a temporary
> - * work around for kcov's lack of nested remote coverage sections support in
> - * task context. Adding support for nested sections is tracked in:
> - * https://bugzilla.kernel.org/show_bug.cgi?id=210337
> + * workaround for KCOV's lack of nested remote coverage sections support.
> + *
> + * Adding support is tracked in https://bugzilla.kernel.org/show_bug.cgi?id=210337.
> + *
> + * kcov_remote_start_usb_softirq():
> + *
> + * 1. Only collects coverage when called in the softirq context. This allows
> + * avoiding nested remote coverage collection sections in the task context.
> + * For example, USB/IP calls usb_hcd_giveback_urb() in the task context
> + * within an existing remote coverage collection section. Thus, KCOV should
> + * not attempt to start collecting coverage within the coverage collection
> + * section in __usb_hcd_giveback_urb() in this case.
> + *
> + * 2. Disables interrupts for the duration of the coverage collection section.
> + * This allows avoiding nested remote coverage collection sections in the
> + * softirq context (a softirq might occur during the execution of a work in
> + * the BH workqueue, which runs with in_serving_softirq() > 0).
> + * For example, usb_giveback_urb_bh() runs in the BH workqueue with
> + * interrupts enabled, so __usb_hcd_giveback_urb() might be interrupted in
> + * the middle of its remote coverage collection section, and the interrupt
> + * handler might invoke __usb_hcd_giveback_urb() again.
> */
>
> -static inline void kcov_remote_start_usb_softirq(u64 id)
> +static inline unsigned long kcov_remote_start_usb_softirq(u64 id)
> {
> - if (in_serving_softirq())
> + unsigned long flags = 0;
> +
> + if (in_serving_softirq()) {
> + local_irq_save(flags);
> kcov_remote_start_usb(id);
> + }
> +
> + return flags;
> }
>
> -static inline void kcov_remote_stop_softirq(void)
> +static inline void kcov_remote_stop_softirq(unsigned long flags)
> {
> - if (in_serving_softirq())
> + if (in_serving_softirq()) {
> kcov_remote_stop();
> + local_irq_restore(flags);
> + }
> }
>
> #ifdef CONFIG_64BIT
> @@ -103,8 +129,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) {}
> +static inline unsigned long kcov_remote_start_usb_softirq(u64 id) {}
> +static inline void kcov_remote_stop_softirq(unsigned long flags) {}
>
> #endif /* CONFIG_KCOV */
> #endif /* _LINUX_KCOV_H */
> --
> 2.25.1
>

2024-05-21 20:47:06

by Andrey Konovalov

[permalink] [raw]

Subject: Re: [PATCH] kcov, usb: disable interrupts in kcov_remote_start_usb_softirq

On Tue, May 21, 2024 at 6:35 AM Dmitry Vyukov <[email protected]> wrote:
>
> On Mon, 20 May 2024 at 22:59, <[email protected]> wrote:
> >
> > From: Andrey Konovalov <[email protected]>
> >
> > After commit 8fea0c8fda30 ("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).
>
> Besides the issue pointed by the test robot:
>
> Acked-by: Dmitry Vyukov <[email protected]>
>
> Thanks for fixing this.

Thanks for the ack!

> This section of code does not rely on reentrancy, right? E.g. one
> callback won't wait for completion of another callback?

I think all should be good. Before the BH workqueue change, the code
ran with interrupts disabled.

> At some point we started seeing lots of "remote cover enable write
> trace failed (errno 17)" errors while running syzkaller. Can these
> errors be caused by this issue?

This looks like a different issue. I also noticed this when I tried
running a log with a bunch of USB programs via syz-execprog. Not sure
why this happens, but I still see it with this patch applied.

Thanks!

2024-06-11 13:38:49

by Aleksandr Nogikh

[permalink] [raw]

Subject: Re: [PATCH] kcov, usb: disable interrupts in kcov_remote_start_usb_softirq

On Tue, May 21, 2024 at 10:46 PM Andrey Konovalov <[email protected]> wrote:
>
> On Tue, May 21, 2024 at 6:35 AM Dmitry Vyukov <[email protected]> wrote:
> >
> > On Mon, 20 May 2024 at 22:59, <[email protected]> wrote:
> > >
> > > From: Andrey Konovalov <[email protected]>
> > >
> > > After commit 8fea0c8fda30 ("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).
> >
> > Besides the issue pointed by the test robot:
> >
> > Acked-by: Dmitry Vyukov <[email protected]>
> >
> > Thanks for fixing this.
>
> Thanks for the ack!
>
> > This section of code does not rely on reentrancy, right? E.g. one
> > callback won't wait for completion of another callback?
>
> I think all should be good. Before the BH workqueue change, the code
> ran with interrupts disabled.
>
> > At some point we started seeing lots of "remote cover enable write
> > trace failed (errno 17)" errors while running syzkaller. Can these
> > errors be caused by this issue?
>
> This looks like a different issue. I also noticed this when I tried
> running a log with a bunch of USB programs via syz-execprog. Not sure
> why this happens, but I still see it with this patch applied.

For the record:
https://lore.kernel.org/all/[email protected]/
should address that problem.

--
Aleksandr

>
> Thanks!
>