x86/entry: Remove unwanted instrumentation in common_interrupt()

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

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

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

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

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.

Quoted 9 lines · Dmitry Vyukov

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?

Quoted 9 lines · Dmitry Vyukov

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?

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

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

Cc [email protected]

Cc [email protected]

Cc [email protected]

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

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

Cc [email protected]

Cc [email protected]

Cc [email protected]

---

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

kernel/kcov.c +3 -0

index c3124f6d5536..04136f80042f 100644
@@ -1057,6 +1057,30 @@ u64 kcov_common_handle(void)
 }
 EXPORT_SYMBOL(kcov_common_handle);
 
+#ifdef CONFIG_KCOV_TEST
+static void __init selftest(void)
+{

kernel/kcov.c +9 -0

+       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;
 }
 

lib/Kconfig.debug +0 -0

index 59b6765d86b8..79836a15b6cb 100644

lib/Kconfig.debug +7 -1

+       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

Quoted 46 lines

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

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

Cc [email protected]

Cc [email protected]

Cc [email protected]

---

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

kernel/kcov.c +3 -0

index c3124f6d5536..04136f80042f 100644
@@ -1057,6 +1057,30 @@ u64 kcov_common_handle(void)
 }
 EXPORT_SYMBOL(kcov_common_handle);
 
+#ifdef CONFIG_KCOV_TEST
+static void __init selftest(void)
+{

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

Quoted 10 lines

kernel/kcov.c +10 -0

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

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

Quoted 6 lines

kernel/kcov.c +6 -0

+        * 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();
}

Quoted 24 lines

kernel/kcov.c +9 -0

+       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;
 }
 

lib/Kconfig.debug +0 -0

index 59b6765d86b8..79836a15b6cb 100644

lib/Kconfig.debug config KCOV_IRQ_AREA_SIZE · line 2174

Quoted 4 lines

lib/Kconfig.debug +1 -0

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

lib/Kconfig.debug config KCOV_IRQ_AREA_SIZE · line 2175

Quoted 1 line

lib/Kconfig.debug +1 -0

+       bool "Test CONFIG_KCOV feature"

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

Quoted 13 lines

lib/Kconfig.debug +7 -1

+       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

Quoted 68 lines · Marco Elver

Quoted 46 lines

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

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

Cc [email protected]

Cc [email protected]

Cc [email protected]

---

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

kernel/kcov.c +3 -0

index c3124f6d5536..04136f80042f 100644
@@ -1057,6 +1057,30 @@ u64 kcov_common_handle(void)
 }
 EXPORT_SYMBOL(kcov_common_handle);
 
+#ifdef CONFIG_KCOV_TEST
+static void __init selftest(void)
+{

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

Quoted 10 lines

kernel/kcov.c +10 -0

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

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

Quoted 6 lines

kernel/kcov.c +6 -0

+        * 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++)
+               ;

Quoted 6 lines

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) ;

Quoted 81 lines · Dmitry Vyukov

Quoted 68 lines · Marco Elver

Quoted 46 lines

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

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

Cc [email protected]

Cc [email protected]

Cc [email protected]

---

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

kernel/kcov.c +3 -0

index c3124f6d5536..04136f80042f 100644
@@ -1057,6 +1057,30 @@ u64 kcov_common_handle(void)
 }
 EXPORT_SYMBOL(kcov_common_handle);
 
+#ifdef CONFIG_KCOV_TEST
+static void __init selftest(void)
+{

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

Quoted 10 lines

kernel/kcov.c +10 -0

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

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

Quoted 6 lines

kernel/kcov.c +6 -0

+        * 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++)
+               ;

Quoted 6 lines

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:

Quoted 3 lines

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.

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

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

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

Cc Aaron Tomlin <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

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

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

Cc [email protected]

Cc [email protected]

Cc [email protected]

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

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

Cc [email protected]

Cc [email protected]

Cc [email protected]

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

arch/x86/kernel/Makefile +8 -1

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

2.45.1.467.gbab1589fc0-goog