| Message ID | 20220905063811.16454-1-ganeshgr@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [PACTH,v2] powerpc/pseries/mce: Avoid instrumentation in realmode | expand |
> On 05-Sep-2022, at 12:08 PM, Ganesh Goudar <ganeshgr@linux.ibm.com> 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. Thanks for the patch. The test successfully ran to completion. Tested-by: Sachin Sant <sachinp@linux.ibm.com>
On Mon Sep 5, 2022 at 4:38 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. > --- > 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 26ede09c521d..3264991fe524 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) > +static __always_inline notrace unsigned long irq_soft_mask_return(void) > { > return READ_ONCE(local_paca->irq_soft_mask); > } > @@ -121,7 +121,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) > +static __always_inline notrace void irq_soft_mask_set(unsigned long mask) > { > /* > * The irq mask must always include the STD bit if any are set. This doesn't give a reason why it's __always_inline, and having the notrace attribute makes it possibly confusing. I think it would be easy for someone to break without realising. Could you add a noinstr to these instead / as well? What about adding a 'realmode' function annotation that includes noinstr? Thanks, Nick > @@ -144,7 +144,7 @@ static inline notrace void irq_soft_mask_set(unsigned long mask) > barrier(); > } > > -static inline notrace unsigned long irq_soft_mask_set_return(unsigned long mask) > +static __always_inline notrace unsigned long irq_soft_mask_set_return(unsigned long mask) > { > unsigned long flags = irq_soft_mask_return(); > > @@ -162,7 +162,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(); > } > 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
On 9/7/22 09:49, Nicholas Piggin wrote: > On Mon Sep 5, 2022 at 4:38 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. >> --- >> 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 26ede09c521d..3264991fe524 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) >> +static __always_inline notrace unsigned long irq_soft_mask_return(void) >> { >> return READ_ONCE(local_paca->irq_soft_mask); >> } >> @@ -121,7 +121,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) >> +static __always_inline notrace void irq_soft_mask_set(unsigned long mask) >> { >> /* >> * The irq mask must always include the STD bit if any are set. > This doesn't give a reason why it's __always_inline, and having the > notrace attribute makes it possibly confusing. I think it would be easy > for someone to break without realising. Could you add a noinstr to these > instead / as well? Yeah we can add noinstr. Missed to see your comment, Sorry for the delayed reply > > What about adding a 'realmode' function annotation that includes noinstr? You mean to define a new function annotation?
On Mon Sep 19, 2022 at 4:03 PM AEST, Ganesh wrote: > On 9/7/22 09:49, Nicholas Piggin wrote: > > > On Mon Sep 5, 2022 at 4:38 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. > >> --- > >> 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 26ede09c521d..3264991fe524 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) > >> +static __always_inline notrace unsigned long irq_soft_mask_return(void) > >> { > >> return READ_ONCE(local_paca->irq_soft_mask); > >> } > >> @@ -121,7 +121,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) > >> +static __always_inline notrace void irq_soft_mask_set(unsigned long mask) > >> { > >> /* > >> * The irq mask must always include the STD bit if any are set. > > This doesn't give a reason why it's __always_inline, and having the > > notrace attribute makes it possibly confusing. I think it would be easy > > for someone to break without realising. Could you add a noinstr to these > > instead / as well? > > Yeah we can add noinstr. Missed to see your comment, Sorry for the delayed reply Okay that would be good. I would prefer to avoid changing the inline-ness of things in a fix patch if possible. > > > > > What about adding a 'realmode' function annotation that includes noinstr? > > You mean to define a new function annotation? Yes, a powerpc specific one that has the necessary adjustments. I think it would be helpful documentation for the code and possibly something we could use to do additional debug checking with at some point too. Thanks, Nick
diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h index 26ede09c521d..3264991fe524 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) +static __always_inline notrace unsigned long irq_soft_mask_return(void) { return READ_ONCE(local_paca->irq_soft_mask); } @@ -121,7 +121,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) +static __always_inline notrace void irq_soft_mask_set(unsigned long mask) { /* * The irq mask must always include the STD bit if any are set. @@ -144,7 +144,7 @@ static inline notrace void irq_soft_mask_set(unsigned long mask) barrier(); } -static inline notrace unsigned long irq_soft_mask_set_return(unsigned long mask) +static __always_inline notrace unsigned long irq_soft_mask_set_return(unsigned long mask) { unsigned long flags = irq_soft_mask_return(); @@ -162,7 +162,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(); } 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) {
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. --- 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(-)