Messages in this thread | | | Date | Tue, 30 Aug 2022 15:15:03 +1000 | Subject | Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer | From | "Nicholas Piggin" <> |
| |
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): > > 0xc000000000295cb0 <+0>: addis r2,r12,774 > 0xc000000000295cb4 <+4>: addi r2,r2,16464 > 0xc000000000295cb8 <+8>: mflr r0 > 0xc000000000295cbc <+12>: bl 0xc00000000008bb4c <mcount> > 0xc000000000295cc0 <+16>: mflr r0 > 0xc000000000295cc4 <+20>: std r31,-8(r1) > 0xc000000000295cc8 <+24>: addi r3,r13,2354 > 0xc000000000295ccc <+28>: mr r31,r13 > 0xc000000000295cd0 <+32>: std r0,16(r1) > 0xc000000000295cd4 <+36>: stdu r1,-48(r1) > 0xc000000000295cd8 <+40>: bl 0xc000000000609b98 <__asan_store1+8> > 0xc000000000295cdc <+44>: nop > 0xc000000000295ce0 <+48>: li r9,1 > 0xc000000000295ce4 <+52>: stb r9,2354(r31) > 0xc000000000295ce8 <+56>: addi r1,r1,48 > 0xc000000000295cec <+60>: ld r0,16(r1) > 0xc000000000295cf0 <+64>: ld r31,-8(r1) > 0xc000000000295cf4 <+68>: mtlr r0 > > 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.
Thanks, Nick
| |