[PATCH 1/4] x86/entry: Remove unwanted instrumentation in common_interrupt()

[PATCH 1/4] x86/entry: Remove unwanted instrumentation in common_interrupt()

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

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 <dvyukov@google.com>
Fixes: 6368558c3710 ("x86/entry: Provide IDTENTRY_SYSVEC")
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: syzkaller@googlegroups.com
---
 arch/x86/include/asm/hardirq.h  | 8 ++++++--
 arch/x86/include/asm/idtentry.h | 6 +++---
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
index c67fa6ad098a..6ffa8b75f4cd 100644
--- a/arch/x86/include/asm/hardirq.h
+++ b/arch/x86/include/asm/hardirq.h
@@ -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 */
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index d4f24499b256..ad5c68f0509d 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -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();						\
-- 
2.45.1.467.gbab1589fc0-goog

Re: [PATCH 1/4] x86/entry: Remove unwanted instrumentation in common_interrupt()

[Alexander Potapenko]

On Tue, Jun 4, 2024 at 3:45 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> 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 <dvyukov@google.com>
Reviewed-by: Alexander Potapenko <glider@google.com>

Re: [PATCH 1/4] x86/entry: Remove unwanted instrumentation in common_interrupt()

[Dave Hansen]

On 6/4/24 06:45, Dmitry Vyukov wrote:
> 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.

I've internalized the main rules around noinstr to basically be: Only
call noinstr functions before begin_instrumentation().  Second, try to
minimize the amount of noinstr code.

This patch seems to be adding another rule which is that all code before
preempt_count manipulation needs to be noinstr.

_Is_ that a new rule, or was it something I was missing?

Re: [PATCH 1/4] x86/entry: Remove unwanted instrumentation in common_interrupt()

[Dmitry Vyukov]

On Tue, 4 Jun 2024 at 18:01, Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 6/4/24 06:45, Dmitry Vyukov wrote:
> > 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.
>
> I've internalized the main rules around noinstr to basically be: Only
> call noinstr functions before begin_instrumentation().  Second, try to
> minimize the amount of noinstr code.
>
> This patch seems to be adding another rule which is that all code before
> preempt_count manipulation needs to be noinstr.
>
> _Is_ that a new rule, or was it something I was missing?

Hi Dave,

This is an old rule. KCOV was always intended to not trace interrupts:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5c9a8750a6409c63a0f01d51a9024861022f6593

+void __sanitizer_cov_trace_pc(void)
+{
+         /*
+         * We are interested in code coverage as a function of a
syscall inputs,
+         * so we ignore code executed in interrupts.
+         */
+         if (!t || in_interrupt())
+                 return;

At the time the entry code was in asm and wasn't instrumented by the
compiler, so the in_interrupt() check was enough to avoid all
problems.

Re: [PATCH 1/4] x86/entry: Remove unwanted instrumentation in common_interrupt()

[Peter Zijlstra]

On Tue, Jun 04, 2024 at 09:00:59AM -0700, Dave Hansen wrote:
> On 6/4/24 06:45, Dmitry Vyukov wrote:
> > 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.
> 
> I've internalized the main rules around noinstr to basically be: Only
> call noinstr functions before begin_instrumentation().  Second, try to
> minimize the amount of noinstr code.
> 
> This patch seems to be adding another rule which is that all code before
> preempt_count manipulation needs to be noinstr.
> 
> _Is_ that a new rule, or was it something I was missing?

Specifically, the problem here appears to be that the instrumentation
cannot correctly identify the context because the HARDIRQ_MASK bits
aren't yet set in preempt_count.

So the preempt_count manipulations as such are not the problem, but the
fact that we call into instrumentation code that seems to rely on using
the preempt_count to determine context is.

Does that clarify?

[PATCH 2/4] kcov: add interrupt handling self test

[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 <dvyukov@google.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: syzkaller@googlegroups.com

---

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     | 28 ++++++++++++++++++++++++++++
 lib/Kconfig.debug |  9 +++++++++
 2 files changed, 37 insertions(+)

diff --git a/kernel/kcov.c b/kernel/kcov.c
index c3124f6d5536..04136f80042f 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -1057,6 +1057,30 @@ u64 kcov_common_handle(void)
 }
 EXPORT_SYMBOL(kcov_common_handle);
 
+#ifdef CONFIG_KCOV_TEST
+static void __init selftest(void)
+{
+	volatile int i;
+
+	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 ~400 ms 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.
+	 */
+	current->kcov_mode = KCOV_MODE_TRACE_PC;
+	for (i = 0; i < (1 << 28); i++)
+		;
+	current->kcov_mode = 0;
+	pr_err("done running self test\n");
+}
+#endif
+
 static int __init kcov_init(void)
 {
 	int cpu;
@@ -1076,6 +1100,10 @@ static int __init kcov_init(void)
 	 */
 	debugfs_create_file_unsafe("kcov", 0600, NULL, NULL, &kcov_fops);
 
+#ifdef CONFIG_KCOV_TEST
+	selftest();
+#endif
+
 	return 0;
 }
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 59b6765d86b8..79836a15b6cb 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2171,6 +2171,15 @@ config KCOV_IRQ_AREA_SIZE
 	  soft interrupts. This specifies the size of those areas in the
 	  number of unsigned long words.
 
+config KCOV_TEST
+	bool "Test CONFIG_KCOV feature"
+	depends on KCOV
+	help
+	  Sanity check for KCOV coverage collection.
+	  Runs built-in self test on boot to detect some common issues.
+
+	  If unsure, say N.
+
 menuconfig RUNTIME_TESTING_MENU
 	bool "Runtime Testing"
 	default y
-- 
2.45.1.467.gbab1589fc0-goog

Re: [PATCH 2/4] kcov: add interrupt handling self test

[Alexander Potapenko]

On Tue, Jun 4, 2024 at 3:45 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> 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 <dvyukov@google.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: syzkaller@googlegroups.com
Reviewed-by: Alexander Potapenko <glider@google.com>

Re: [PATCH 2/4] kcov: add interrupt handling self test

[Marco Elver]

On Tue, 4 Jun 2024 at 15:45, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> 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 <dvyukov@google.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: syzkaller@googlegroups.com
>
> ---
>
> 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     | 28 ++++++++++++++++++++++++++++
>  lib/Kconfig.debug |  9 +++++++++
>  2 files changed, 37 insertions(+)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index c3124f6d5536..04136f80042f 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -1057,6 +1057,30 @@ u64 kcov_common_handle(void)
>  }
>  EXPORT_SYMBOL(kcov_common_handle);
>
> +#ifdef CONFIG_KCOV_TEST
> +static void __init selftest(void)
> +{
> +       volatile int i;
> +
> +       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 ~400 ms waiting for a timer interrupt.

Where do the 400 ms come from? I only see that it loops a long time,
but that the timing is entirely dependent on how fast the CPU executes
the loop.

> +        * We set kcov_mode to enable tracing, but don't setup the area,
> +        * so any attempt to trace will crash.
> +        */
> +       current->kcov_mode = KCOV_MODE_TRACE_PC;
> +       for (i = 0; i < (1 << 28); i++)
> +               ;

Can't you check jiffies, and e.g. check that actual ~100-500ms have elapsed?

timeout = jiffies + msecs_to_jiffies(300);
while (!time_after(jiffies, timeout)) {
  cpu_relax();
}

> +       current->kcov_mode = 0;
> +       pr_err("done running self test\n");
> +}
> +#endif
> +
>  static int __init kcov_init(void)
>  {
>         int cpu;
> @@ -1076,6 +1100,10 @@ static int __init kcov_init(void)
>          */
>         debugfs_create_file_unsafe("kcov", 0600, NULL, NULL, &kcov_fops);
>
> +#ifdef CONFIG_KCOV_TEST
> +       selftest();
> +#endif
> +
>         return 0;
>  }
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 59b6765d86b8..79836a15b6cb 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2171,6 +2171,15 @@ config KCOV_IRQ_AREA_SIZE
>           soft interrupts. This specifies the size of those areas in the
>           number of unsigned long words.
>
> +config KCOV_TEST

s/TEST/SELFTEST/

It may be confused with a longer standalone test.

> +       bool "Test CONFIG_KCOV feature"

Maybe "Perform short selftests on boot" (similar to CONFIG_KCSAN_SELFTEST).

> +       depends on KCOV
> +       help
> +         Sanity check for KCOV coverage collection.
> +         Runs built-in self test on boot to detect some common issues.
> +
> +         If unsure, say N.
> +
>  menuconfig RUNTIME_TESTING_MENU
>         bool "Runtime Testing"
>         default y
> --
> 2.45.1.467.gbab1589fc0-goog
>

Re: [PATCH 2/4] kcov: add interrupt handling self test

[Dmitry Vyukov]

On Wed, 5 Jun 2024 at 11:10, Marco Elver <elver@google.com> wrote:
>
> > 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 <dvyukov@google.com>
> > Cc: x86@kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: syzkaller@googlegroups.com
> >
> > ---
> >
> > 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     | 28 ++++++++++++++++++++++++++++
> >  lib/Kconfig.debug |  9 +++++++++
> >  2 files changed, 37 insertions(+)
> >
> > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > index c3124f6d5536..04136f80042f 100644
> > --- a/kernel/kcov.c
> > +++ b/kernel/kcov.c
> > @@ -1057,6 +1057,30 @@ u64 kcov_common_handle(void)
> >  }
> >  EXPORT_SYMBOL(kcov_common_handle);
> >
> > +#ifdef CONFIG_KCOV_TEST
> > +static void __init selftest(void)
> > +{
> > +       volatile int i;
> > +
> > +       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 ~400 ms waiting for a timer interrupt.
>
> Where do the 400 ms come from? I only see that it loops a long time,
> but that the timing is entirely dependent on how fast the CPU executes
> the loop.
>
> > +        * We set kcov_mode to enable tracing, but don't setup the area,
> > +        * so any attempt to trace will crash.
> > +        */
> > +       current->kcov_mode = KCOV_MODE_TRACE_PC;
> > +       for (i = 0; i < (1 << 28); i++)
> > +               ;
>
> Can't you check jiffies, and e.g. check that actual ~100-500ms have elapsed?
>
> timeout = jiffies + msecs_to_jiffies(300);
> while (!time_after(jiffies, timeout)) {
>   cpu_relax();
> }

We can't call any functions. If anything is instrumented, the kernel crashes.

But just reading jiffies should be fine, so we can do:

unsigned long start = jiffies;
while ((jiffies - start) * MSEC_PER_SEC / HZ < 500)
;

> > +       current->kcov_mode = 0;
> > +       pr_err("done running self test\n");
> > +}
> > +#endif
> > +
> >  static int __init kcov_init(void)
> >  {
> >         int cpu;
> > @@ -1076,6 +1100,10 @@ static int __init kcov_init(void)
> >          */
> >         debugfs_create_file_unsafe("kcov", 0600, NULL, NULL, &kcov_fops);
> >
> > +#ifdef CONFIG_KCOV_TEST
> > +       selftest();
> > +#endif
> > +
> >         return 0;
> >  }
> >
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 59b6765d86b8..79836a15b6cb 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -2171,6 +2171,15 @@ config KCOV_IRQ_AREA_SIZE
> >           soft interrupts. This specifies the size of those areas in the
> >           number of unsigned long words.
> >
> > +config KCOV_TEST
>
> s/TEST/SELFTEST/
>
> It may be confused with a longer standalone test.
>
> > +       bool "Test CONFIG_KCOV feature"
>
> Maybe "Perform short selftests on boot" (similar to CONFIG_KCSAN_SELFTEST).
>
> > +       depends on KCOV
> > +       help
> > +         Sanity check for KCOV coverage collection.
> > +         Runs built-in self test on boot to detect some common issues.
> > +
> > +         If unsure, say N.
> > +
> >  menuconfig RUNTIME_TESTING_MENU
> >         bool "Runtime Testing"
> >         default y
> > --
> > 2.45.1.467.gbab1589fc0-goog
> >

Re: [PATCH 2/4] kcov: add interrupt handling self test

[Marco Elver]

On Wed, 5 Jun 2024 at 11:18, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Wed, 5 Jun 2024 at 11:10, Marco Elver <elver@google.com> wrote:
> >
> > > 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 <dvyukov@google.com>
> > > Cc: x86@kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: syzkaller@googlegroups.com
> > >
> > > ---
> > >
> > > 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     | 28 ++++++++++++++++++++++++++++
> > >  lib/Kconfig.debug |  9 +++++++++
> > >  2 files changed, 37 insertions(+)
> > >
> > > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > > index c3124f6d5536..04136f80042f 100644
> > > --- a/kernel/kcov.c
> > > +++ b/kernel/kcov.c
> > > @@ -1057,6 +1057,30 @@ u64 kcov_common_handle(void)
> > >  }
> > >  EXPORT_SYMBOL(kcov_common_handle);
> > >
> > > +#ifdef CONFIG_KCOV_TEST
> > > +static void __init selftest(void)
> > > +{
> > > +       volatile int i;
> > > +
> > > +       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 ~400 ms waiting for a timer interrupt.
> >
> > Where do the 400 ms come from? I only see that it loops a long time,
> > but that the timing is entirely dependent on how fast the CPU executes
> > the loop.
> >
> > > +        * We set kcov_mode to enable tracing, but don't setup the area,
> > > +        * so any attempt to trace will crash.
> > > +        */
> > > +       current->kcov_mode = KCOV_MODE_TRACE_PC;
> > > +       for (i = 0; i < (1 << 28); i++)
> > > +               ;
> >
> > Can't you check jiffies, and e.g. check that actual ~100-500ms have elapsed?
> >
> > timeout = jiffies + msecs_to_jiffies(300);
> > while (!time_after(jiffies, timeout)) {
> >   cpu_relax();
> > }
>
> We can't call any functions. If anything is instrumented, the kernel crashes.
>
> But just reading jiffies should be fine, so we can do:
>
> unsigned long start = jiffies;
> while ((jiffies - start) * MSEC_PER_SEC / HZ < 500)
> ;

I'm quite sure that those helpers are macros, but who knows if that
will ever change.

The above open-coded version looks reasonable.

Re: [PATCH 2/4] kcov: add interrupt handling self test

[Dmitry Vyukov]

On Wed, 5 Jun 2024 at 11:33, Marco Elver <elver@google.com> wrote:
>
> On Wed, 5 Jun 2024 at 11:18, Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> > On Wed, 5 Jun 2024 at 11:10, Marco Elver <elver@google.com> wrote:
> > >
> > > > 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 <dvyukov@google.com>
> > > > Cc: x86@kernel.org
> > > > Cc: linux-kernel@vger.kernel.org
> > > > Cc: syzkaller@googlegroups.com
> > > >
> > > > ---
> > > >
> > > > 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     | 28 ++++++++++++++++++++++++++++
> > > >  lib/Kconfig.debug |  9 +++++++++
> > > >  2 files changed, 37 insertions(+)
> > > >
> > > > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > > > index c3124f6d5536..04136f80042f 100644
> > > > --- a/kernel/kcov.c
> > > > +++ b/kernel/kcov.c
> > > > @@ -1057,6 +1057,30 @@ u64 kcov_common_handle(void)
> > > >  }
> > > >  EXPORT_SYMBOL(kcov_common_handle);
> > > >
> > > > +#ifdef CONFIG_KCOV_TEST
> > > > +static void __init selftest(void)
> > > > +{
> > > > +       volatile int i;
> > > > +
> > > > +       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 ~400 ms waiting for a timer interrupt.
> > >
> > > Where do the 400 ms come from? I only see that it loops a long time,
> > > but that the timing is entirely dependent on how fast the CPU executes
> > > the loop.
> > >
> > > > +        * We set kcov_mode to enable tracing, but don't setup the area,
> > > > +        * so any attempt to trace will crash.
> > > > +        */
> > > > +       current->kcov_mode = KCOV_MODE_TRACE_PC;
> > > > +       for (i = 0; i < (1 << 28); i++)
> > > > +               ;
> > >
> > > Can't you check jiffies, and e.g. check that actual ~100-500ms have elapsed?
> > >
> > > timeout = jiffies + msecs_to_jiffies(300);
> > > while (!time_after(jiffies, timeout)) {
> > >   cpu_relax();
> > > }
> >
> > We can't call any functions. If anything is instrumented, the kernel crashes.
> >
> > But just reading jiffies should be fine, so we can do:
> >
> > unsigned long start = jiffies;
> > while ((jiffies - start) * MSEC_PER_SEC / HZ < 500)
> > ;
>
> I'm quite sure that those helpers are macros, but who knows if that
> will ever change.
>
> The above open-coded version looks reasonable.

Sent v2 with fixes, PTAL.
https://lore.kernel.org/all/cover.1718092070.git.dvyukov@google.com/T/#t

[PATCH 3/4] module: Fix KCOV-ignored file name

[Dmitry Vyukov]

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 <dvyukov@google.com>
Fixes: cfc1d277891e ("module: Move all into module/")
Cc: Aaron Tomlin <atomlin@redhat.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: syzkaller@googlegroups.com
---
 kernel/module/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module/Makefile b/kernel/module/Makefile
index a10b2b9a6fdf..50ffcc413b54 100644
--- a/kernel/module/Makefile
+++ b/kernel/module/Makefile
@@ -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.1.467.gbab1589fc0-goog

Re: [PATCH 3/4] module: Fix KCOV-ignored file name

[Alexander Potapenko]

On Tue, Jun 4, 2024 at 3:45 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> 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 <dvyukov@google.com>
> Fixes: cfc1d277891e ("module: Move all into module/")
> Cc: Aaron Tomlin <atomlin@redhat.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: syzkaller@googlegroups.com
Reviewed-by: Alexander Potapenko <glider@google.com>

[PATCH 4/4] x86: Ignore stack unwinding in KCOV

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

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: syzkaller@googlegroups.com
---
 arch/x86/kernel/Makefile | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 20a0dd51700a..cd49ebfae984 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -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
 
-- 
2.45.1.467.gbab1589fc0-goog

Re: [PATCH 4/4] x86: Ignore stack unwinding in KCOV

[Alexander Potapenko]

On Tue, Jun 4, 2024 at 3:45 PM 'Dmitry Vyukov' via syzkaller
<syzkaller@googlegroups.com> wrote:
>
> 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 <dvyukov@google.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: syzkaller@googlegroups.com
Reviewed-by: Alexander Potapenko <glider@google.com>

Re: [PATCH 4/4] x86: Ignore stack unwinding in KCOV

[Marco Elver]

On Tue, 4 Jun 2024 at 15:45, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> 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 <dvyukov@google.com>

Reviewed-by: Marco Elver <elver@google.com>

> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: syzkaller@googlegroups.com
> ---
>  arch/x86/kernel/Makefile | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 20a0dd51700a..cd49ebfae984 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -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
>
> --
> 2.45.1.467.gbab1589fc0-goog
>