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-googOn 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
>
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.
© 2016 - 2026 Red Hat, Inc.