[PATCH 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 | 28 ++++++++++++++++++++++++++++
kernel/module/Makefile | 2 +-
lib/Kconfig.debug | 8 ++++++++
6 files changed, 54 insertions(+), 6 deletions(-)


base-commit: 2ab79514109578fc4b6df90633d500cf281eb689
--
2.45.1.467.gbab1589fc0-goog

[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

[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 <dvy...@google.com>
Reviewed-by: Alexander Potapenko <gli...@google.com>
Fixes: 6368558c3710 ("x86/entry: Provide IDTENTRY_SYSVEC")
Cc: x...@kernel.org
Cc: linux-...@vger.kernel.org
Cc: syzk...@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

[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 <dvy...@google.com>
Reviewed-by: Alexander Potapenko <gli...@google.com>

Cc: x...@kernel.org
Cc: linux-...@vger.kernel.org
Cc: syzk...@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

[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 <dvy...@google.com>
Reviewed-by: Alexander Potapenko <gli...@google.com>

Fixes: cfc1d277891e ("module: Move all into module/")
Cc: Aaron Tomlin <ato...@redhat.com>

Cc: x...@kernel.org
Cc: linux-...@vger.kernel.org
Cc: syzk...@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

[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 <dvy...@google.com>
Reviewed-by: Alexander Potapenko <gli...@google.com>

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

Cc: x...@kernel.org
Cc: linux-...@vger.kernel.org
Cc: syzk...@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

[Marco Elver]

On Tue, 11 Jun 2024 at 09:50, Dmitry Vyukov <dvy...@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 <dvy...@google.com>
> Reviewed-by: Alexander Potapenko <gli...@google.com>
> Cc: x...@kernel.org
> Cc: linux-...@vger.kernel.org
> Cc: syzk...@googlegroups.com

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

[Marco Elver]

On Tue, 11 Jun 2024 at 09:50, Dmitry Vyukov <dvy...@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 <dvy...@google.com>
> Reviewed-by: Alexander Potapenko <gli...@google.com>
> Fixes: cfc1d277891e ("module: Move all into module/")
> Cc: Aaron Tomlin <ato...@redhat.com>
> Cc: x...@kernel.org
> Cc: linux-...@vger.kernel.org
> Cc: syzk...@googlegroups.com

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

[Dmitry Vyukov]

On Tue, 11 Jun 2024 at 09:50, Dmitry Vyukov <dvy...@google.com> wrote:
>

Thomas, Ingo, Borislav, Dave,

Can you take this via x86 tree please?

[Andrey Konovalov]

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

[Andrey Konovalov]

On Tue, Jun 11, 2024 at 9:50 AM 'Dmitry Vyukov' via syzkaller
<syzk...@googlegroups.com> wrote:
>

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

[Andrey Konovalov]

On Tue, Jun 11, 2024 at 9:50 AM 'Dmitry Vyukov' via syzkaller
<syzk...@googlegroups.com> wrote:
>

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 <andre...@gmail.com>

[Dmitry Vyukov]

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

[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

[Peter Zijlstra]

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.

barrier();

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

barrier();

[Dmitry Vyukov]

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.

> --
> 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+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20240619111309.GJ31592%40noisy.programming.kicks-ass.net.

[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 <dvy...@google.com>
> Reviewed-by: Alexander Potapenko <gli...@google.com>
> Fixes: 6368558c3710 ("x86/entry: Provide IDTENTRY_SYSVEC")
> Cc: x...@kernel.org
> Cc: linux-...@vger.kernel.org
> Cc: syzk...@googlegroups.com

Anyway, the patch is fine,

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

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

[Peter Zijlstra]

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.

[Dmitry Vyukov]

On Wed, 19 Jun 2024 at 13:19, Peter Zijlstra <pet...@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.

> --
> 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+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20240619111936.GK31592%40noisy.programming.kicks-ass.net.

[Dmitry Vyukov]

On Wed, 19 Jun 2024 at 13:23, Peter Zijlstra <pet...@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) <pet...@infradead.org>

Thanks for your reviews, Peter!

> --
> 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+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20240619112332.GL31592%40noisy.programming.kicks-ass.net.

[Andrey Konovalov]

On Wed, Jun 19, 2024 at 10:31 AM Borislav Petkov <b...@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!

[Thomas Gleixner]

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