Re: [PATCH v3] powerpc/pseries/mce: Avoid instrumentation in realmode

["Nicholas Piggin" <npiggin@gmail.com>]

On Mon Sep 26, 2022 at 4:18 PM AEST, Ganesh Goudar wrote:
> Part of machine check error handling is done in realmode,
> As of now instrumentation is not possible for any code that
> runs in realmode.
> When MCE is injected on KASAN enabled kernel, crash is
> observed, Hence force inline or mark no instrumentation
> for functions which can run in realmode, to avoid KASAN
> instrumentation.
>
> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
> ---
> v2: Force inline few more functions.
>
> v3: Adding noinstr to few functions instead of __always_inline.

I would still like to consider doing a realmode annotation, but
as a minimal fix for the next merge window I suppose this is okay.
There's still no indication for why the annotation exists on the
functions which is a bit annoying, maybe not fundamentally worse
than notrace was, but the scope of reasons why it's there gets
bigger.


> ---
>  arch/powerpc/include/asm/hw_irq.h    | 8 ++++----
>  arch/powerpc/include/asm/interrupt.h | 2 +-
>  arch/powerpc/include/asm/rtas.h      | 4 ++--
>  arch/powerpc/kernel/rtas.c           | 4 ++--
>  4 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> index 983551859891..c4d542b4a623 100644
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -111,7 +111,7 @@ static inline void __hard_RI_enable(void)
>  #ifdef CONFIG_PPC64
>  #include <asm/paca.h>
>  
> -static inline notrace unsigned long irq_soft_mask_return(void)
> +noinstr static unsigned long irq_soft_mask_return(void)
>  {
>  	unsigned long flags;

Don't uninline the ones in headers.

> @@ -128,7 +128,7 @@ static inline notrace unsigned long irq_soft_mask_return(void)
>   * for the critical section and as a clobber because
>   * we changed paca->irq_soft_mask
>   */
> -static inline notrace void irq_soft_mask_set(unsigned long mask)
> +noinstr static void irq_soft_mask_set(unsigned long mask)
>  {
>  	/*
>  	 * The irq mask must always include the STD bit if any are set.
> @@ -155,7 +155,7 @@ static inline notrace void irq_soft_mask_set(unsigned long mask)
>  		: "memory");
>  }
>  
> -static inline notrace unsigned long irq_soft_mask_set_return(unsigned long mask)
> +noinstr static unsigned long irq_soft_mask_set_return(unsigned long mask)
>  {
>  	unsigned long flags;
>  
> @@ -191,7 +191,7 @@ static inline notrace unsigned long irq_soft_mask_or_return(unsigned long mask)
>  	return flags;
>  }
>  
> -static inline unsigned long arch_local_save_flags(void)
> +static __always_inline unsigned long arch_local_save_flags(void)
>  {
>  	return irq_soft_mask_return();
>  }

Can we instead add noinstr to this too, the the other ones that were
changed to always inline?

Thanks,
Nick

> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index 8069dbc4b8d1..090895051712 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -92,7 +92,7 @@ static inline bool is_implicit_soft_masked(struct pt_regs *regs)
>  	return search_kernel_soft_mask_table(regs->nip);
>  }
>  
> -static inline void srr_regs_clobbered(void)
> +static __always_inline void srr_regs_clobbered(void)
>  {
>  	local_paca->srr_valid = 0;
>  	local_paca->hsrr_valid = 0;
> diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
> index 00531af17ce0..52d29d664fdf 100644
> --- a/arch/powerpc/include/asm/rtas.h
> +++ b/arch/powerpc/include/asm/rtas.h
> @@ -201,13 +201,13 @@ inline uint32_t rtas_ext_event_company_id(struct rtas_ext_event_log_v6 *ext_log)
>  #define PSERIES_ELOG_SECT_ID_MCE		(('M' << 8) | 'C')
>  
>  static
> -inline uint16_t pseries_errorlog_id(struct pseries_errorlog *sect)
> +__always_inline uint16_t pseries_errorlog_id(struct pseries_errorlog *sect)
>  {
>  	return be16_to_cpu(sect->id);
>  }
>  
>  static
> -inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect)
> +__always_inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect)
>  {
>  	return be16_to_cpu(sect->length);
>  }
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 693133972294..f9d78245c0e8 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -48,7 +48,7 @@
>  /* This is here deliberately so it's only used in this file */
>  void enter_rtas(unsigned long);
>  
> -static inline void do_enter_rtas(unsigned long args)
> +static __always_inline void do_enter_rtas(unsigned long args)
>  {
>  	unsigned long msr;
>  
> @@ -435,7 +435,7 @@ static char *__fetch_rtas_last_error(char *altbuf)
>  #endif
>  
>  
> -static void
> +noinstr static void
>  va_rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret,
>  		      va_list list)
>  {
> -- 
> 2.37.1