[PATCH v2 0/4] KCOV fixes

[PATCH v2 0/4] KCOV fixes

[Dmitry Vyukov]

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

[PATCH v2 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>
Reviewed-by: Alexander Potapenko <glider@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.2.505.gda0bf45e8d-goog

[PATCH v2 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>
Reviewed-by: Alexander Potapenko <glider@google.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: syzkaller@googlegroups.com

---

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(+)

diff --git a/kernel/kcov.c b/kernel/kcov.c
index c3124f6d5536..72a5bf55107f 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -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;
 }
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 59b6765d86b8..695a437a52d9 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -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

[PATCH v2 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>
Reviewed-by: Alexander Potapenko <glider@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.2.505.gda0bf45e8d-goog

[PATCH v2 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>
Reviewed-by: Alexander Potapenko <glider@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.2.505.gda0bf45e8d-goog

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

[Marco Elver]

On Tue, 11 Jun 2024 at 09:50, 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>
> Reviewed-by: Alexander Potapenko <glider@google.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: syzkaller@googlegroups.com

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

> ---
>
> 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(+)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index c3124f6d5536..72a5bf55107f 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -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;
>  }
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 59b6765d86b8..695a437a52d9 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -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
>

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

[Marco Elver]

On Tue, 11 Jun 2024 at 09:50, 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>
> Reviewed-by: Alexander Potapenko <glider@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: Marco Elver <elver@google.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.2.505.gda0bf45e8d-goog
>

Re: [PATCH v2 0/4] KCOV fixes

[Dmitry Vyukov]

On Tue, 11 Jun 2024 at 09:50, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> 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

Thomas, Ingo, Borislav, Dave,

Can you take this via x86 tree please?

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

[Andrey Konovalov]

On Tue, Jun 11, 2024 at 9:50 AM '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>
> Reviewed-by: Alexander Potapenko <glider@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
>
> --

Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>

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

[Andrey Konovalov]

On Tue, Jun 11, 2024 at 9:50 AM 'Dmitry Vyukov' via syzkaller
<syzkaller@googlegroups.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>
> Reviewed-by: Alexander Potapenko <glider@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.2.505.gda0bf45e8d-goog

Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>

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

[Andrey Konovalov]

On Tue, Jun 11, 2024 at 9:50 AM 'Dmitry Vyukov' via syzkaller
<syzkaller@googlegroups.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>
> Reviewed-by: Alexander Potapenko <glider@google.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: syzkaller@googlegroups.com
>
> ---
>
> 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(+)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index c3124f6d5536..72a5bf55107f 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -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;
>  }
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 59b6765d86b8..695a437a52d9 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -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

Nit: "causes the kernel to panic" => "causes a kernel panic" or "panic
the kernel"

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

Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>

Re: [PATCH v2 0/4] KCOV fixes

[Dmitry Vyukov]

On Tue, 11 Jun 2024 at 11:31, Dmitry Vyukov <dvyukov@google.com> wrote:
> > 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
>
> 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)?

Re: [PATCH v2 0/4] KCOV fixes

[Borislav Petkov]

On Wed, Jun 19, 2024 at 07:20:56AM +0200, Dmitry Vyukov wrote:
> Or is it OK to take this via mm tree (where KCOV changes usually go)?

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

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Peter Zijlstra]

On Tue, Jun 11, 2024 at 09:50:31AM +0200, Dmitry Vyukov 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>
> Reviewed-by: Alexander Potapenko <glider@google.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: syzkaller@googlegroups.com
> 
> ---
> 
> 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

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.

> ---
>  kernel/kcov.c     | 31 +++++++++++++++++++++++++++++++
>  lib/Kconfig.debug |  8 ++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index c3124f6d5536..72a5bf55107f 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -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;

	barrier();

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

	barrier();

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

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

[Dmitry Vyukov]

On Wed, 19 Jun 2024 at 13:13, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Jun 11, 2024 at 09:50:31AM +0200, Dmitry Vyukov 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>
> > Reviewed-by: Alexander Potapenko <glider@google.com>
> > Cc: x86@kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: syzkaller@googlegroups.com
> >
> > ---
> >
> > 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
>
> 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:

+        * 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.

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.



> > ---
> >  kernel/kcov.c     | 31 +++++++++++++++++++++++++++++++
> >  lib/Kconfig.debug |  8 ++++++++
> >  2 files changed, 39 insertions(+)
> >
> > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > index c3124f6d5536..72a5bf55107f 100644
> > --- a/kernel/kcov.c
> > +++ b/kernel/kcov.c
> > @@ -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;
>
>         barrier();
>
> > +     while ((jiffies - start) * MSEC_PER_SEC / HZ < 300)
> > +             ;
>
>         barrier();
>
> > +     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 syzkaller+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20240619111309.GJ31592%40noisy.programming.kicks-ass.net.

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

[Peter Zijlstra]

On Tue, Jun 11, 2024 at 09:50:30AM +0200, Dmitry Vyukov 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.

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

> 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>
> Fixes: 6368558c3710 ("x86/entry: Provide IDTENTRY_SYSVEC")
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: syzkaller@googlegroups.com

Anyway, the patch is fine,

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> ---
>  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.2.505.gda0bf45e8d-goog
> 

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

[Peter Zijlstra]

On Tue, Jun 11, 2024 at 09:50:33AM +0200, Dmitry Vyukov 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.

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) <peterz@infradead.org>

> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> Reviewed-by: Alexander Potapenko <glider@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.2.505.gda0bf45e8d-goog
> 

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

[Peter Zijlstra]

On Wed, Jun 19, 2024 at 01:18:52PM +0200, Dmitry Vyukov wrote:
> On Wed, 19 Jun 2024 at 13:13, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Tue, Jun 11, 2024 at 09:50:31AM +0200, Dmitry Vyukov 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>
> > > Reviewed-by: Alexander Potapenko <glider@google.com>
> > > Cc: x86@kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: syzkaller@googlegroups.com
> > >
> > > ---
> > >
> > > 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
> >
> > 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:
> 
> +        * 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.

Ah, I'm just slow today.. did not connect the dots. No this is fine.

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

Yeah, polling jiffies should be fine I suppose.

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

[Dmitry Vyukov]

On Wed, 19 Jun 2024 at 13:19, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Jun 11, 2024 at 09:50:30AM +0200, Dmitry Vyukov 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.
>
> So I'm reading spurious here, but the next patch trips BUG, them very
> much not the same thing. Which is it?

I am not sure I understand the question.
Currently kvm_set_cpu_l1tf_flush_l1d() is traced, so the added test
produces a BUG without this fix (and does not produce with this fix).
By "spurious" I meant that tracing of kvm_set_cpu_l1tf_flush_l1d() and
interrupts in general is parasitic/unwanted.

> > 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>
> > Fixes: 6368558c3710 ("x86/entry: Provide IDTENTRY_SYSVEC")
> > Cc: x86@kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: syzkaller@googlegroups.com
>
> Anyway, the patch is fine,
>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> > ---
> >  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.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 syzkaller+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20240619111936.GK31592%40noisy.programming.kicks-ass.net.

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

[Dmitry Vyukov]

On Wed, 19 Jun 2024 at 13:23, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Jun 11, 2024 at 09:50:33AM +0200, Dmitry Vyukov 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.
>
> 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?

It may be used for other similar purposes as well, e.g. collecting
unit test coverage (not the whole kernel, but a single specific test
provided that even other tests can run and collect their own coverage
in parallel).

It's spelled explicitly in the docs:

https://elixir.bootlin.com/linux/latest/source/Documentation/dev-tools/kcov.rst

"""
KCOV collects and exposes kernel code coverage information in a form suitable
for coverage-guided fuzzing .... Coverage collection is enabled on a
task basis, and
thus KCOV can capture precise coverage of a single system call.

Note that KCOV does not aim to collect as much coverage as possible. It aims
to collect more or less stable coverage that is a function of syscall inputs.
To achieve this goal, it does not collect coverage in soft/hard interrupts...
"""



> Anyway,
>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Thanks for your reviews, Peter!


> > Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> > Reviewed-by: Alexander Potapenko <glider@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.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 syzkaller+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20240619112332.GL31592%40noisy.programming.kicks-ass.net.

Re: [PATCH v2 0/4] KCOV fixes

[Andrey Konovalov]

On Wed, Jun 19, 2024 at 10:31 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Jun 19, 2024 at 07:20:56AM +0200, Dmitry Vyukov wrote:
> > 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.

Thank you!

Re: [PATCH v2 0/4] KCOV fixes

[Thomas Gleixner]

On Mon, Aug 05 2024 at 14:52, Andrey Konovalov wrote:
> On Wed, Jun 19, 2024 at 10:31 AM Borislav Petkov <bp@alien8.de> wrote:
>>
>> On Wed, Jun 19, 2024 at 07:20:56AM +0200, Dmitry Vyukov wrote:
>> > 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.

Sorry. That fell through the cracks. I'm picking it up now.

[tip: x86/build] x86: Ignore stack unwinding in KCOV

[tip-bot2 for Dmitry Vyukov]

The following commit has been merged into the x86/build branch of tip:

Commit-ID:     ae94b263f5f69c180347e795fbefa051b65aacc3
Gitweb:        https://git.kernel.org/tip/ae94b263f5f69c180347e795fbefa051b65aacc3
Author:        Dmitry Vyukov <dvyukov@google.com>
AuthorDate:    Tue, 11 Jun 2024 09:50:33 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 08 Aug 2024 17:36:35 +02:00

x86: Ignore stack unwinding in KCOV

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>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Alexander Potapenko <glider@google.com>
Reviewed-by: Marco Elver <elver@google.com>
Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/all/eaf54b8634970b73552dcd38bf9be6ef55238c10.1718092070.git.dvyukov@google.com

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

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index a847180..f791898 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -35,6 +35,14 @@ KMSAN_SANITIZE_nmi.o					:= n
 # If instrumentation of the following files is enabled, boot hangs during
 # first second.
 KCOV_INSTRUMENT_head$(BITS).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
 

[tip: x86/build] module: Fix KCOV-ignored file name

[tip-bot2 for Dmitry Vyukov]

The following commit has been merged into the x86/build branch of tip:

Commit-ID:     f34d086fb7102fec895fd58b9e816b981b284c17
Gitweb:        https://git.kernel.org/tip/f34d086fb7102fec895fd58b9e816b981b284c17
Author:        Dmitry Vyukov <dvyukov@google.com>
AuthorDate:    Tue, 11 Jun 2024 09:50:32 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 08 Aug 2024 17:36:35 +02:00

module: Fix KCOV-ignored file name

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.

Fixes: cfc1d277891e ("module: Move all into module/")
Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Alexander Potapenko <glider@google.com>
Reviewed-by: Marco Elver <elver@google.com>
Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/bc0cf790b4839c5e38e2fafc64271f620568a39e.1718092070.git.dvyukov@google.com

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

diff --git a/kernel/module/Makefile b/kernel/module/Makefile
index a10b2b9..50ffcc4 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

[tip: x86/build] kcov: Add interrupt handling self test

[tip-bot2 for Dmitry Vyukov]

The following commit has been merged into the x86/build branch of tip:

Commit-ID:     6cd0dd934b03d4ee4094ac474108723e2f2ed7d6
Gitweb:        https://git.kernel.org/tip/6cd0dd934b03d4ee4094ac474108723e2f2ed7d6
Author:        Dmitry Vyukov <dvyukov@google.com>
AuthorDate:    Tue, 11 Jun 2024 09:50:31 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 08 Aug 2024 17:36:35 +02:00

kcov: Add interrupt handling self test

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>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Alexander Potapenko <glider@google.com>
Reviewed-by: Marco Elver <elver@google.com>
Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
Link: https://lore.kernel.org/all/7662127c97e29da1a748ad1c1539dd7b65b737b2.1718092070.git.dvyukov@google.com


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

diff --git a/kernel/kcov.c b/kernel/kcov.c
index f0a69d4..d9d4a0c 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -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>
@@ -1058,6 +1059,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;
@@ -1077,6 +1104,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;
 }
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index a30c03a..270e367 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2173,6 +2173,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

[tip: x86/build] x86/entry: Remove unwanted instrumentation in common_interrupt()

[tip-bot2 for Dmitry Vyukov]

The following commit has been merged into the x86/build branch of tip:

Commit-ID:     477d81a1c47a1b79b9c08fc92b5dea3c5143800b
Gitweb:        https://git.kernel.org/tip/477d81a1c47a1b79b9c08fc92b5dea3c5143800b
Author:        Dmitry Vyukov <dvyukov@google.com>
AuthorDate:    Tue, 11 Jun 2024 09:50:30 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 08 Aug 2024 17:36:35 +02:00

x86/entry: Remove unwanted instrumentation in common_interrupt()

common_interrupt() and related variants call kvm_set_cpu_l1tf_flush_l1d(),
which is neither marked noinstr nor __always_inline.

So compiler puts it out of line 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 the kernel is in an interrupt.

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

Fixes: 6368558c3710 ("x86/entry: Provide IDTENTRY_SYSVEC")
Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Alexander Potapenko <glider@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/3f9a1de9e415fcb53d07dc9e19fa8481bb021b1b.1718092070.git.dvyukov@google.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 c67fa6a..6ffa8b7 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 d4f2449..ad5c68f 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();						\