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@xxxxxxxxxx>
> Reviewed-by: Alexander Potapenko <glider@xxxxxxxxxx>
> Fixes: 6368558c3710 ("x86/entry: Provide IDTENTRY_SYSVEC")
> Cc: x86@xxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: syzkaller@xxxxxxxxxxxxxxxx

Anyway, the patch is fine,

Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>

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