KCOV fixes

common_interrupt() and friends call kvm_set_cpu_l1tf_flush_l1d(), which is not marked as noinstr nor __always_inline.
So compiler outlines it and adds instrumentation to it. Since the call is inside of instrumentation_begin/end(), objtool does not warn about it.

The manifestation is that KCOV produces spurious coverage in kvm_set_cpu_l1tf_flush_l1d() in random places because the call happens when preempt count is not yet updated to say that we are in an interrupt.

Mark kvm_set_cpu_l1tf_flush_l1d() as __always_inline and move out of instrumentation_begin/end() section.
It only calls __this_cpu_write() which is already safe to call in noinstr contexts.

Signed-off-by Dmitry Vyukov <[email protected]>

Reviewed-by Alexander Potapenko <[email protected]>

Fixes 6368558c3710 ("x86/entry: Provide IDTENTRY_SYSVEC")

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 arch/x86/include/asm/hardirq.h  | 8 ++++++--
 arch/x86/include/asm/idtentry.h | 6 +++---
 2 files changed, 9 insertions(+), 5 deletions(-)

arch/x86/include/asm/hardirq.h +6 -2

index c67fa6ad098a..6ffa8b75f4cd 100644
@@ -69,7 +69,11 @@ extern u64 arch_irq_stat(void);
 #define local_softirq_pending_ref       pcpu_hot.softirq_pending
 
 #if IS_ENABLED(CONFIG_KVM_INTEL)
-static inline void kvm_set_cpu_l1tf_flush_l1d(void)
+/*
+ * This function is called from noinstr interrupt contexts
+ * and must be inlined to not get instrumentation.
+ */
+static __always_inline void kvm_set_cpu_l1tf_flush_l1d(void)
 {
 	__this_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, 1);
 }

@@ -84,7 +88,7 @@ static __always_inline bool kvm_get_cpu_l1tf_flush_l1d(void)
 	return __this_cpu_read(irq_stat.kvm_cpu_l1tf_flush_l1d);
 }
 #else /* !IS_ENABLED(CONFIG_KVM_INTEL) */
-static inline void kvm_set_cpu_l1tf_flush_l1d(void) { }
+static __always_inline void kvm_set_cpu_l1tf_flush_l1d(void) { }
 #endif /* IS_ENABLED(CONFIG_KVM_INTEL) */
 
 #endif /* _ASM_X86_HARDIRQ_H */

arch/x86/include/asm/idtentry.h +3 -3

index d4f24499b256..ad5c68f0509d 100644
@@ -212,8 +212,8 @@ __visible noinstr void func(struct pt_regs *regs,			\
 	irqentry_state_t state = irqentry_enter(regs);			\
 	u32 vector = (u32)(u8)error_code;				\
 									\
+	kvm_set_cpu_l1tf_flush_l1d();                                   \
 	instrumentation_begin();					\
-	kvm_set_cpu_l1tf_flush_l1d();					\
 	run_irq_on_irqstack_cond(__##func, regs, vector);		\
 	instrumentation_end();						\
 	irqentry_exit(regs, state);					\

@@ -250,7 +250,6 @@ static void __##func(struct pt_regs *regs);				\
 									\
 static __always_inline void instr_##func(struct pt_regs *regs)		\
 {									\
-	kvm_set_cpu_l1tf_flush_l1d();					\
 	run_sysvec_on_irqstack_cond(__##func, regs);			\
 }									\
 									\

@@ -258,6 +257,7 @@ __visible noinstr void func(struct pt_regs *regs)			\
 {									\
 	irqentry_state_t state = irqentry_enter(regs);			\
 									\
+	kvm_set_cpu_l1tf_flush_l1d();                                   \
 	instrumentation_begin();					\
 	instr_##func (regs);						\
 	instrumentation_end();						\

@@ -288,7 +288,6 @@ static __always_inline void __##func(struct pt_regs *regs);		\
 static __always_inline void instr_##func(struct pt_regs *regs)		\
 {									\
 	__irq_enter_raw();						\
-	kvm_set_cpu_l1tf_flush_l1d();					\
 	__##func (regs);						\
 	__irq_exit_raw();						\
 }									\

@@ -297,6 +296,7 @@ __visible noinstr void func(struct pt_regs *regs)			\
 {									\
 	irqentry_state_t state = irqentry_enter(regs);			\
 									\
+	kvm_set_cpu_l1tf_flush_l1d();                                   \
 	instrumentation_begin();					\
 	instr_##func (regs);						\
 	instrumentation_end();						\

Signature · 1 line

2.45.2.505.gda0bf45e8d-goog

Quoted 6 lines · Dmitry Vyukov

common_interrupt() and friends call kvm_set_cpu_l1tf_flush_l1d(), which is not marked as noinstr nor __always_inline.
So compiler outlines it and adds instrumentation to it. Since the call is inside of instrumentation_begin/end(), objtool does not warn about it.

Quoted 4 lines

The manifestation is that KCOV produces spurious coverage in kvm_set_cpu_l1tf_flush_l1d() in random places because the call happens when preempt count is not yet updated to say that we are in an interrupt.

So I'm reading spurious here, but the next patch trips BUG, them very much not the same thing. Which is it?

Quoted 11 lines

Mark kvm_set_cpu_l1tf_flush_l1d() as __always_inline and move out of instrumentation_begin/end() section.
It only calls __this_cpu_write() which is already safe to call in noinstr contexts.

Signed-off-by Dmitry Vyukov <[email protected]>

Reviewed-by Alexander Potapenko <[email protected]>

Fixes 6368558c3710 ("x86/entry: Provide IDTENTRY_SYSVEC")

Cc [email protected]

Cc [email protected]

Cc [email protected]

Anyway, the patch is fine,

Acked-by Peter Zijlstra (Intel) <[email protected]>

Quoted 78 lines

---
 arch/x86/include/asm/hardirq.h  | 8 ++++++--
 arch/x86/include/asm/idtentry.h | 6 +++---
 2 files changed, 9 insertions(+), 5 deletions(-)

arch/x86/include/asm/hardirq.h +6 -2

index c67fa6ad098a..6ffa8b75f4cd 100644
@@ -69,7 +69,11 @@ extern u64 arch_irq_stat(void);
 #define local_softirq_pending_ref       pcpu_hot.softirq_pending
 
 #if IS_ENABLED(CONFIG_KVM_INTEL)
-static inline void kvm_set_cpu_l1tf_flush_l1d(void)
+/*
+ * This function is called from noinstr interrupt contexts
+ * and must be inlined to not get instrumentation.
+ */
+static __always_inline void kvm_set_cpu_l1tf_flush_l1d(void)
 {
      __this_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, 1);
 }

@@ -84,7 +88,7 @@ static __always_inline bool kvm_get_cpu_l1tf_flush_l1d(void)
      return __this_cpu_read(irq_stat.kvm_cpu_l1tf_flush_l1d);
 }
 #else /* !IS_ENABLED(CONFIG_KVM_INTEL) */
-static inline void kvm_set_cpu_l1tf_flush_l1d(void) { }
+static __always_inline void kvm_set_cpu_l1tf_flush_l1d(void) { }
 #endif /* IS_ENABLED(CONFIG_KVM_INTEL) */
 
 #endif /* _ASM_X86_HARDIRQ_H */

arch/x86/include/asm/idtentry.h +3 -4

index d4f24499b256..ad5c68f0509d 100644
@@ -212,8 +212,8 @@ __visible noinstr void func(struct pt_regs *regs,                 \
      irqentry_state_t state = irqentry_enter(regs);                  \
      u32 vector = (u32)(u8)error_code;                               \
                                                                      \
+     kvm_set_cpu_l1tf_flush_l1d();                                   \
      instrumentation_begin();                                        \
-     kvm_set_cpu_l1tf_flush_l1d();                                   \
      run_irq_on_irqstack_cond(__##func, regs, vector);               \
      instrumentation_end();                                          \
      irqentry_exit(regs, state);                                     \

@@ -250,7 +250,6 @@ static void __##func(struct pt_regs *regs);                               \
                                                                      \
 static __always_inline void instr_##func(struct pt_regs *regs)               \
 {                                                                    \
-     kvm_set_cpu_l1tf_flush_l1d();                                   \
      run_sysvec_on_irqstack_cond(__##func, regs);                    \
 }                                                                    \
                                                                      \

@@ -258,6 +257,7 @@ __visible noinstr void func(struct pt_regs *regs)                 \
 {                                                                    \
      irqentry_state_t state = irqentry_enter(regs);                  \
                                                                      \
+     kvm_set_cpu_l1tf_flush_l1d();                                   \
      instrumentation_begin();                                        \
      instr_##func (regs);                                            \
      instrumentation_end();                                          \

@@ -288,7 +288,6 @@ static __always_inline void __##func(struct pt_regs *regs);               \
 static __always_inline void instr_##func(struct pt_regs *regs)               \
 {                                                                    \
      __irq_enter_raw();                                              \
-     kvm_set_cpu_l1tf_flush_l1d();                                   \
      __##func (regs);                                                \
      __irq_exit_raw();                                               \
 }                                                                    \

@@ -297,6 +296,7 @@ __visible noinstr void func(struct pt_regs *regs)                 \
 {                                                                    \
      irqentry_state_t state = irqentry_enter(regs);                  \
                                                                      \
+     kvm_set_cpu_l1tf_flush_l1d();                                   \
      instrumentation_begin();                                        \
      instr_##func (regs);                                            \
      instrumentation_end();                                          \
--

Quoted 2 lines

2.45.2.505.gda0bf45e8d-goog

--
You received this message because you are subscribed to the Google Groups "syzkaller" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20240619111936.GK31592%40noisy.programming.kicks-ass.net.

Add a boot self test that can catch sprious coverage from interrupts. The coverage callback filters out interrupt code, but only after the handler updates preempt count. Some code periodically leaks out of that section and leads to spurious coverage.
Add a best-effort (but simple) test that is likely to catch such bugs. If the test is enabled on CI systems that use KCOV, they should catch any issues fast.

Signed-off-by Dmitry Vyukov <[email protected]>

Reviewed-by Alexander Potapenko <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---

Changed since v1:
- renamed KCOV_TEST to KCOV_SELFTEST
- improved the config description
- loop for exactly 300ms in the test

In my local testing w/o the previous fix,
it immidiatly produced the following splat:

kcov: running selftest

BUG: TASK stack guard page was hit at ffffc90000147ff8
Oops: stack guard page: 0000 [#1] PREEMPT SMP KASAN PTI
...
 kvm_set_cpu_l1tf_flush_l1d+0x5/0x20
 sysvec_call_function+0x15/0xb0
 asm_sysvec_call_function+0x1a/0x20
 kcov_init+0xe4/0x130
 do_one_initcall+0xbc/0x470
 kernel_init_freeable+0x4fc/0x930
 kernel_init+0x1c/0x2b0
---
 kernel/kcov.c     | 31 +++++++++++++++++++++++++++++++
 lib/Kconfig.debug |  8 ++++++++
 2 files changed, 39 insertions(+)

kernel/kcov.c +31 -0

index c3124f6d5536..72a5bf55107f 100644
@@ -11,6 +11,7 @@
 #include <linux/fs.h>
 #include <linux/hashtable.h>
 #include <linux/init.h>
+#include <linux/jiffies.h>
 #include <linux/kmsan-checks.h>
 #include <linux/mm.h>
 #include <linux/preempt.h>

@@ -1057,6 +1058,32 @@ u64 kcov_common_handle(void)
 }
 EXPORT_SYMBOL(kcov_common_handle);
 
+#ifdef CONFIG_KCOV_SELFTEST
+static void __init selftest(void)
+{
+       unsigned long start;
+
+       pr_err("running self test\n");
+       /*
+        * Test that interrupts don't produce spurious coverage.
+        * The coverage callback filters out interrupt code, but only
+        * after the handler updates preempt count. Some code periodically
+        * leaks out of that section and leads to spurious coverage.
+        * It's hard to call the actual interrupt handler directly,
+        * so we just loop here for a bit waiting for a timer interrupt.
+        * We set kcov_mode to enable tracing, but don't setup the area,
+        * so any attempt to trace will crash. Note: we must not call any
+        * potentially traced functions in this region.
+        */
+       start = jiffies;
+       current->kcov_mode = KCOV_MODE_TRACE_PC;
+       while ((jiffies - start) * MSEC_PER_SEC / HZ < 300)
+               ;
+       current->kcov_mode = 0;
+       pr_err("done running self test\n");
+}
+#endif
+
 static int __init kcov_init(void)
 {
        int cpu;

@@ -1076,6 +1103,10 @@ static int __init kcov_init(void)
         */
        debugfs_create_file_unsafe("kcov", 0600, NULL, NULL, &kcov_fops);
 
+#ifdef CONFIG_KCOV_SELFTEST
+       selftest();
+#endif
+
        return 0;
 }
 

lib/Kconfig.debug +8 -1

index 59b6765d86b8..695a437a52d9 100644
@@ -2171,6 +2171,14 @@ config KCOV_IRQ_AREA_SIZE
          soft interrupts. This specifies the size of those areas in the
          number of unsigned long words.
 
+config KCOV_SELFTEST
+       bool "Perform short selftests on boot"
+       depends on KCOV
+       help
+         Run short KCOV coverage collection selftests on boot.
+         On test failure, causes the kernel to panic. Recommended to be
+         enabled, ensuring critical functionality works as intended.
+
 menuconfig RUNTIME_TESTING_MENU
        bool "Runtime Testing"
        default y
--

2.45.2.505.gda0bf45e8d-goog

Add a boot self test that can catch sprious coverage from interrupts. The coverage callback filters out interrupt code, but only after the handler updates preempt count. Some code periodically leaks out of that section and leads to spurious coverage.
Add a best-effort (but simple) test that is likely to catch such bugs. If the test is enabled on CI systems that use KCOV, they should catch any issues fast.

Signed-off-by Dmitry Vyukov <[email protected]>

Reviewed-by Alexander Potapenko <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---

Changed since v1:
- renamed KCOV_TEST to KCOV_SELFTEST
- improved the config description
- loop for exactly 300ms in the test

In my local testing w/o the previous fix,
it immidiatly produced the following splat:

kcov: running selftest

BUG: TASK stack guard page was hit at ffffc90000147ff8
Oops: stack guard page: 0000 [#1] PREEMPT SMP KASAN PTI
...
 kvm_set_cpu_l1tf_flush_l1d+0x5/0x20
 sysvec_call_function+0x15/0xb0
 asm_sysvec_call_function+0x1a/0x20
 kcov_init+0xe4/0x130
 do_one_initcall+0xbc/0x470
 kernel_init_freeable+0x4fc/0x930
 kernel_init+0x1c/0x2b0
---
 kernel/kcov.c     | 31 +++++++++++++++++++++++++++++++
 lib/Kconfig.debug |  8 ++++++++
 2 files changed, 39 insertions(+)

kernel/kcov.c +31 -0

index c3124f6d5536..72a5bf55107f 100644
@@ -11,6 +11,7 @@
 #include <linux/fs.h>
 #include <linux/hashtable.h>
 #include <linux/init.h>
+#include <linux/jiffies.h>
 #include <linux/kmsan-checks.h>
 #include <linux/mm.h>
 #include <linux/preempt.h>

@@ -1057,6 +1058,32 @@ u64 kcov_common_handle(void)
 }
 EXPORT_SYMBOL(kcov_common_handle);
 
+#ifdef CONFIG_KCOV_SELFTEST
+static void __init selftest(void)
+{
+       unsigned long start;
+
+       pr_err("running self test\n");
+       /*
+        * Test that interrupts don't produce spurious coverage.
+        * The coverage callback filters out interrupt code, but only
+        * after the handler updates preempt count. Some code periodically
+        * leaks out of that section and leads to spurious coverage.
+        * It's hard to call the actual interrupt handler directly,
+        * so we just loop here for a bit waiting for a timer interrupt.
+        * We set kcov_mode to enable tracing, but don't setup the area,
+        * so any attempt to trace will crash. Note: we must not call any
+        * potentially traced functions in this region.
+        */
+       start = jiffies;
+       current->kcov_mode = KCOV_MODE_TRACE_PC;
+       while ((jiffies - start) * MSEC_PER_SEC / HZ < 300)
+               ;
+       current->kcov_mode = 0;
+       pr_err("done running self test\n");
+}
+#endif
+
 static int __init kcov_init(void)
 {
        int cpu;

@@ -1076,6 +1103,10 @@ static int __init kcov_init(void)
         */
        debugfs_create_file_unsafe("kcov", 0600, NULL, NULL, &kcov_fops);
 
+#ifdef CONFIG_KCOV_SELFTEST
+       selftest();
+#endif
+
        return 0;
 }
 

lib/Kconfig.debug +0 -0

index 59b6765d86b8..695a437a52d9 100644

lib/Kconfig.debug +2 -1

+         enabled, ensuring critical functionality works as intended.
+
 menuconfig RUNTIME_TESTING_MENU
        bool "Runtime Testing"
        default y
--

2.45.2.505.gda0bf45e8d-goog

Add a boot self test that can catch sprious coverage from interrupts. The coverage callback filters out interrupt code, but only after the handler updates preempt count. Some code periodically leaks out of that section and leads to spurious coverage.
Add a best-effort (but simple) test that is likely to catch such bugs. If the test is enabled on CI systems that use KCOV, they should catch any issues fast.

Signed-off-by Dmitry Vyukov <[email protected]>

Reviewed-by Alexander Potapenko <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---

Changed since v1:
- renamed KCOV_TEST to KCOV_SELFTEST
- improved the config description
- loop for exactly 300ms in the test

In my local testing w/o the previous fix,
it immidiatly produced the following splat:

kcov: running selftest

BUG: TASK stack guard page was hit at ffffc90000147ff8
Oops: stack guard page: 0000 [#1] PREEMPT SMP KASAN PTI
...
 kvm_set_cpu_l1tf_flush_l1d+0x5/0x20
 sysvec_call_function+0x15/0xb0
 asm_sysvec_call_function+0x1a/0x20
 kcov_init+0xe4/0x130
 do_one_initcall+0xbc/0x470
 kernel_init_freeable+0x4fc/0x930
 kernel_init+0x1c/0x2b0

---
 kernel/kcov.c     | 31 +++++++++++++++++++++++++++++++
 lib/Kconfig.debug |  8 ++++++++
 2 files changed, 39 insertions(+)

kernel/kcov.c +10 -0

index c3124f6d5536..72a5bf55107f 100644
@@ -11,6 +11,7 @@
 #include <linux/fs.h>
 #include <linux/hashtable.h>
 #include <linux/init.h>
+#include <linux/jiffies.h>
 #include <linux/kmsan-checks.h>
 #include <linux/mm.h>
 #include <linux/preempt.h>

@@ -1057,6 +1058,32 @@ u64 kcov_common_handle(void)
 }
 EXPORT_SYMBOL(kcov_common_handle);
 
+#ifdef CONFIG_KCOV_SELFTEST
+static void __init selftest(void)
+{
+	unsigned long start;
+
+	pr_err("running self test\n");
+	/*
+	 * Test that interrupts don't produce spurious coverage.
+	 * The coverage callback filters out interrupt code, but only

kernel/kcov.c +4 -0

+	current->kcov_mode = 0;
+	pr_err("done running self test\n");
+}
+#endif

Quoted 25 lines · Dmitry Vyukov

Add a boot self test that can catch sprious coverage from interrupts. The coverage callback filters out interrupt code, but only after the handler updates preempt count. Some code periodically leaks out of that section and leads to spurious coverage.
Add a best-effort (but simple) test that is likely to catch such bugs. If the test is enabled on CI systems that use KCOV, they should catch any issues fast.

Signed-off-by Dmitry Vyukov <[email protected]>

Reviewed-by Alexander Potapenko <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---

Changed since v1:
- renamed KCOV_TEST to KCOV_SELFTEST
- improved the config description
- loop for exactly 300ms in the test

In my local testing w/o the previous fix,
it immidiatly produced the following splat:

kcov: running selftest

Quoted 10 lines

BUG: TASK stack guard page was hit at ffffc90000147ff8
Oops: stack guard page: 0000 [#1] PREEMPT SMP KASAN PTI
...
 kvm_set_cpu_l1tf_flush_l1d+0x5/0x20
 sysvec_call_function+0x15/0xb0
 asm_sysvec_call_function+0x1a/0x20
 kcov_init+0xe4/0x130
 do_one_initcall+0xbc/0x470
 kernel_init_freeable+0x4fc/0x930
 kernel_init+0x1c/0x2b0

So I'm not entirely sure how the above BUG comes about, nor how this selftest tickles it. Could you elaborate?

I've found check_kcov_mode() which has this !in_task() clause, but I'm not entirely sure how failing that leads to the above mentioned failure.

Quoted 30 lines

---
 kernel/kcov.c     | 31 +++++++++++++++++++++++++++++++
 lib/Kconfig.debug |  8 ++++++++
 2 files changed, 39 insertions(+)

kernel/kcov.c +10 -0

index c3124f6d5536..72a5bf55107f 100644
@@ -11,6 +11,7 @@
 #include <linux/fs.h>
 #include <linux/hashtable.h>
 #include <linux/init.h>
+#include <linux/jiffies.h>
 #include <linux/kmsan-checks.h>
 #include <linux/mm.h>
 #include <linux/preempt.h>

@@ -1057,6 +1058,32 @@ u64 kcov_common_handle(void)
 }
 EXPORT_SYMBOL(kcov_common_handle);
 
+#ifdef CONFIG_KCOV_SELFTEST
+static void __init selftest(void)
+{
+     unsigned long start;
+
+     pr_err("running self test\n");
+     /*
+      * Test that interrupts don't produce spurious coverage.
+      * The coverage callback filters out interrupt code, but only

kernel/kcov.c u64 kcov_common_handle(void) · line 1079

Quoted 10 lines

kernel/kcov.c +10 -0

+      * after the handler updates preempt count. Some code periodically
+      * leaks out of that section and leads to spurious coverage.
+      * It's hard to call the actual interrupt handler directly,
+      * so we just loop here for a bit waiting for a timer interrupt.
+      * We set kcov_mode to enable tracing, but don't setup the area,
+      * so any attempt to trace will crash. Note: we must not call any
+      * potentially traced functions in this region.
+      */
+     start = jiffies;
+     current->kcov_mode = KCOV_MODE_TRACE_PC;

        barrier();

kernel/kcov.c u64 kcov_common_handle(void) · line 1081

Quoted 2 lines

kernel/kcov.c +2 -0

+     while ((jiffies - start) * MSEC_PER_SEC / HZ < 300)
+             ;

        barrier();

kernel/kcov.c u64 kcov_common_handle(void) · line 1085

Quoted 4 lines

kernel/kcov.c +4 -0

+     current->kcov_mode = 0;
+     pr_err("done running self test\n");
+}
+#endif

--
You received this message because you are subscribed to the Google Groups "syzkaller" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20240619111309.GJ31592%40noisy.programming.kicks-ass.net.

Quoted 38 lines · Peter Zijlstra

Quoted 25 lines · Dmitry Vyukov

Add a boot self test that can catch sprious coverage from interrupts. The coverage callback filters out interrupt code, but only after the handler updates preempt count. Some code periodically leaks out of that section and leads to spurious coverage.
Add a best-effort (but simple) test that is likely to catch such bugs. If the test is enabled on CI systems that use KCOV, they should catch any issues fast.

Signed-off-by Dmitry Vyukov <[email protected]>

Reviewed-by Alexander Potapenko <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---

Changed since v1:
- renamed KCOV_TEST to KCOV_SELFTEST
- improved the config description
- loop for exactly 300ms in the test

In my local testing w/o the previous fix,
it immidiatly produced the following splat:

kcov: running selftest

Quoted 10 lines

BUG: TASK stack guard page was hit at ffffc90000147ff8
Oops: stack guard page: 0000 [#1] PREEMPT SMP KASAN PTI
...
 kvm_set_cpu_l1tf_flush_l1d+0x5/0x20
 sysvec_call_function+0x15/0xb0
 asm_sysvec_call_function+0x1a/0x20
 kcov_init+0xe4/0x130
 do_one_initcall+0xbc/0x470
 kernel_init_freeable+0x4fc/0x930
 kernel_init+0x1c/0x2b0

Quoted 5 lines

So I'm not entirely sure how the above BUG comes about, nor how this selftest tickles it. Could you elaborate?

I've found check_kcov_mode() which has this !in_task() clause, but I'm not entirely sure how failing that leads to the above mentioned failure.

I've tried to explain it in the test comment, maybe I need to improve it:

Basically, we setup current task kcov in a way that any attempt to trace in __sanitizer_cov_trace_pc() will crash, and then just loop waiting for interrupts.

A more legit way to achieve the same would be to properly setup kcov for tracing from within the kernel, then call outermost interrupt entry function, then check we traced nothing. But that would require a non-trivial amount of new complex code, and e.g. the top-most interrupt entry function is not exported and is arch-specific.

Module.c was renamed to main.c, but the Makefile directive was copy-pasted verbatim with the old file name. Fix up the file name.

Signed-off-by Dmitry Vyukov <[email protected]>

Reviewed-by Alexander Potapenko <[email protected]>

Fixes cfc1d277891e ("module: Move all into module/")

Cc Aaron Tomlin <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 kernel/module/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/module/Makefile +1 -2

index a10b2b9a6fdf..50ffcc413b54 100644
@@ -5,7 +5,7 @@
 
 # These are called from save_stack_trace() on slub debug path,
 # and produce insane amounts of uninteresting coverage.
-KCOV_INSTRUMENT_module.o := n
+KCOV_INSTRUMENT_main.o := n
 
 obj-y += main.o
 obj-y += strict_rwx.o
--

2.45.2.505.gda0bf45e8d-goog

Module.c was renamed to main.c, but the Makefile directive was copy-pasted verbatim with the old file name. Fix up the file name.

Signed-off-by Dmitry Vyukov <[email protected]>

Reviewed-by Alexander Potapenko <[email protected]>

Fixes cfc1d277891e ("module: Move all into module/")

Cc Aaron Tomlin <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 kernel/module/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/module/Makefile +1 -2

index a10b2b9a6fdf..50ffcc413b54 100644
@@ -5,7 +5,7 @@
 
 # These are called from save_stack_trace() on slub debug path,
 # and produce insane amounts of uninteresting coverage.
-KCOV_INSTRUMENT_module.o := n
+KCOV_INSTRUMENT_main.o := n
 
 obj-y += main.o
 obj-y += strict_rwx.o
--

2.45.2.505.gda0bf45e8d-goog

Stack unwinding produces large amounts of uninteresting coverage. It's called from KASAN kmalloc/kfree hooks, fault injection, etc. It's not particularly useful and is not a function of system call args. Ignore that code.

Signed-off-by Dmitry Vyukov <[email protected]>

Reviewed-by Alexander Potapenko <[email protected]>

Reviewed-by Marco Elver <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 arch/x86/kernel/Makefile | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/x86/kernel/Makefile +2 -0

index 20a0dd51700a..cd49ebfae984 100644
@@ -39,6 +39,14 @@ KMSAN_SANITIZE_sev.o                                 := n
 # first second.
 KCOV_INSTRUMENT_head$(BITS).o                          := n
 KCOV_INSTRUMENT_sev.o                                  := n
+# These are called from save_stack_trace() on debug paths,
+# and produce large amounts of uninteresting coverage.

Stack unwinding produces large amounts of uninteresting coverage. It's called from KASAN kmalloc/kfree hooks, fault injection, etc. It's not particularly useful and is not a function of system call args. Ignore that code.

Signed-off-by Dmitry Vyukov <[email protected]>

Reviewed-by Alexander Potapenko <[email protected]>

Reviewed-by Marco Elver <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 arch/x86/kernel/Makefile | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/x86/kernel/Makefile +8 -0

index 20a0dd51700a..cd49ebfae984 100644
@@ -39,6 +39,14 @@ KMSAN_SANITIZE_sev.o					:= n
 # first second.
 KCOV_INSTRUMENT_head$(BITS).o				:= n
 KCOV_INSTRUMENT_sev.o					:= n
+# These are called from save_stack_trace() on debug paths,
+# and produce large amounts of uninteresting coverage.
+KCOV_INSTRUMENT_stacktrace.o				:= n
+KCOV_INSTRUMENT_dumpstack.o				:= n
+KCOV_INSTRUMENT_dumpstack_$(BITS).o			:= n
+KCOV_INSTRUMENT_unwind_orc.o				:= n
+KCOV_INSTRUMENT_unwind_frame.o				:= n
+KCOV_INSTRUMENT_unwind_guess.o				:= n
 
 CFLAGS_irq.o := -I $(src)/../include/asm/trace
 

Signature · 1 line

2.45.2.505.gda0bf45e8d-goog

Quoted 4 lines · Dmitry Vyukov

Stack unwinding produces large amounts of uninteresting coverage. It's called from KASAN kmalloc/kfree hooks, fault injection, etc. It's not particularly useful and is not a function of system call args. Ignore that code.

This stems from KCOV's purpose being guiding syzkaller as opposed to it being a more general coverage tool, right?

Is that spelled out anywhere?

Anyway,

Acked-by Peter Zijlstra (Intel) <[email protected]>

Quoted 30 lines

Signed-off-by Dmitry Vyukov <[email protected]>

Reviewed-by Alexander Potapenko <[email protected]>

Reviewed-by Marco Elver <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 arch/x86/kernel/Makefile | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/x86/kernel/Makefile +8 -1

index 20a0dd51700a..cd49ebfae984 100644
@@ -39,6 +39,14 @@ KMSAN_SANITIZE_sev.o                                       := n
 # first second.
 KCOV_INSTRUMENT_head$(BITS).o                                := n
 KCOV_INSTRUMENT_sev.o                                        := n
+# These are called from save_stack_trace() on debug paths,
+# and produce large amounts of uninteresting coverage.
+KCOV_INSTRUMENT_stacktrace.o                         := n
+KCOV_INSTRUMENT_dumpstack.o                          := n
+KCOV_INSTRUMENT_dumpstack_$(BITS).o                  := n
+KCOV_INSTRUMENT_unwind_orc.o                         := n
+KCOV_INSTRUMENT_unwind_frame.o                               := n
+KCOV_INSTRUMENT_unwind_guess.o                               := n
 
 CFLAGS_irq.o := -I $(src)/../include/asm/trace
 
--

Quoted 2 lines

2.45.2.505.gda0bf45e8d-goog

--
You received this message because you are subscribed to the Google Groups "syzkaller" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20240619112332.GL31592%40noisy.programming.kicks-ass.net.

Fix spurious KCOV coverage from interrupts and add a test. Ignore some additional files that lead to large amounts of uninteresting coverage.
As a reference point, tracing a simple open system call produces ~10K PCs with these changes instead of ~45K PCs.

Dmitry Vyukov (4):
x86/entry: Remove unwanted instrumentation in common_interrupt()
kcov: add interrupt handling self test
module: Fix KCOV-ignored file name
x86: Ignore stack unwinding in KCOV

 arch/x86/include/asm/hardirq.h  |  8 ++++++--
 arch/x86/include/asm/idtentry.h |  6 +++---
 arch/x86/kernel/Makefile        |  8 ++++++++
 kernel/kcov.c                   | 31 +++++++++++++++++++++++++++++++
 kernel/module/Makefile          |  2 +-
 lib/Kconfig.debug               |  8 ++++++++
 6 files changed, 57 insertions(+), 6 deletions(-)

base-commit: 83a7eefedc9b56fe7bfeff13b6c7356688ffa670 --
2.45.2.505.gda0bf45e8d-goog

Quoted 21 lines

Fix spurious KCOV coverage from interrupts and add a test. Ignore some additional files that lead to large amounts of uninteresting coverage.
As a reference point, tracing a simple open system call produces ~10K PCs with these changes instead of ~45K PCs.

Dmitry Vyukov (4):
x86/entry: Remove unwanted instrumentation in common_interrupt()
kcov: add interrupt handling self test
module: Fix KCOV-ignored file name
x86: Ignore stack unwinding in KCOV

 arch/x86/include/asm/hardirq.h  |  8 ++++++--
 arch/x86/include/asm/idtentry.h |  6 +++---
 arch/x86/kernel/Makefile        |  8 ++++++++
 kernel/kcov.c                   | 31 +++++++++++++++++++++++++++++++
 kernel/module/Makefile          |  2 +-
 lib/Kconfig.debug               |  8 ++++++++
 6 files changed, 57 insertions(+), 6 deletions(-)

Quoted 3 lines

base-commit: 83a7eefedc9b56fe7bfeff13b6c7356688ffa670 --
2.45.2.505.gda0bf45e8d-goog

Thomas, Ingo, Borislav, Dave,

Can you take this via x86 tree please?

Or is it OK to take this via mm tree (where KCOV changes usually go)?

Quoted 1 line · Dmitry Vyukov

Or is it OK to take this via mm tree (where KCOV changes usually go)?

Be patient, pls, you're on the TODO list.

Quoted 5 lines · Borislav Petkov

Quoted 1 line · Dmitry Vyukov

Or is it OK to take this via mm tree (where KCOV changes usually go)?

Be patient, pls, you're on the TODO list.

Hi Borislav,

I was wondering what's the status of these patches? They didn't make it into 6.11 and I also still don't see them in linux-next.