Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

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

On Tue Aug 30, 2022 at 3:24 PM AEST, Christophe Leroy wrote:
>
>
> Le 30/08/2022 à 07:15, Nicholas Piggin a écrit :
> > On Wed Aug 24, 2022 at 2:39 AM AEST, Christophe Leroy wrote:
> >> In ppc, compiler based sanitizer will generate instrument instructions
> >> around statement WRITE_ONCE(local_paca->irq_soft_mask, mask):
> >>
>
> [...]
>
> >>
> >> If there is a context switch before "stb     r9,2354(r31)", r31 may
> >> not equal to r13, in such case, irq soft mask will not work.
> >>
> >> The same problem occurs in irq_soft_mask_return() with
> >> READ_ONCE(local_paca->irq_soft_mask).
> > 
> > WRITE_ONCE doesn't require address generation to be atomic with the
> > store so this is a bug without sanitizer too. I have seen gcc put r13
> > into a nvgpr before.
> > 
> > READ_ONCE maybe could be argued is safe in this case because data
> > could be stale when you use it anyway, but pointless and risky
> > in some cases (imagine cpu offline -> store poison value to irq soft
> > mask.
> > 
> >> This patch partially reverts commit ef5b570d3700 ("powerpc/irq: Don't
> >> open code irq_soft_mask helpers") with a more modern inline assembly.
> >>
> >> Reported-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
> >> Fixes: ef5b570d3700 ("powerpc/irq: Don't open code irq_soft_mask helpers")
> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> >> ---
> >> v2: Use =m constraint for stb instead of m constraint
> >> ---
> >>   arch/powerpc/include/asm/hw_irq.h | 9 ++++++---
> >>   1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> >> index 26ede09c521d..815420988ef3 100644
> >> --- a/arch/powerpc/include/asm/hw_irq.h
> >> +++ b/arch/powerpc/include/asm/hw_irq.h
> >> @@ -113,7 +113,11 @@ static inline void __hard_RI_enable(void)
> >>   
> >>   static inline notrace unsigned long irq_soft_mask_return(void)
> >>   {
> >> -	return READ_ONCE(local_paca->irq_soft_mask);
> >> +	unsigned long flags;
> >> +
> >> +	asm volatile("lbz%X1 %0,%1" : "=r" (flags) : "m" (local_paca->irq_soft_mask));
> >> +
> >> +	return flags;
> >>   }
> >>   
> >>   /*
> >> @@ -140,8 +144,7 @@ static inline notrace void irq_soft_mask_set(unsigned long mask)
> >>   	if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
> >>   		WARN_ON(mask && !(mask & IRQS_DISABLED));
> >>   
> >> -	WRITE_ONCE(local_paca->irq_soft_mask, mask);
> >> -	barrier();
> >> +	asm volatile("stb%X0 %1,%0" : "=m" (local_paca->irq_soft_mask) : "r" (mask) : "memory");
> > 
> > This is still slightly concerning to me. Is there any guarantee that the
> > compiler would not use a different sequence for the address here?
> > 
> > Maybe explicit r13 is required.
> > 
>
> local_paca is defined as:
>
> 	register struct paca_struct *local_paca asm("r13");
>
> Why would the compiler use another register ?

Hopefully it doesn't. Is it guaranteed that it won't?

> If so, do we also have an 
> issue with the use of current_stack_pointer in irq.c ?

What problems do you think it might have?  I think it may be okay
because we're only using it to check what stack we are using so doesn't
really matter what value it is when we sample it.

The overflow check similarly probably doesn't matter the exact value.

> Segher ?

I'm sure Segher will be delighted with the creative asm in __do_IRQ
and call_do_irq :) *Grabs popcorn*

Thanks,
Nick