Re: [PATCH linux-next] powerpc: disable sanitizer in irq_soft_mask_set

[Zhouyi Zhou]

On Mon, Aug 22, 2022 at 2:04 PM Christophe Leroy
<christophe.leroy@xxxxxxxxxx> wrote:
>
>
>
> Le 21/08/2022 à 03:00, Zhouyi Zhou a écrit :
> > 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.
> >
> > This patch disable sanitizer in irq_soft_mask_set.
>
> Well spotted, thanks.
Thank Christophe for reviewing my patch and your guidance!
>
> You should add:
>
> Fixes: ef5b570d3700 ("powerpc/irq: Don't open code irq_soft_mask helpers")
OK, I will do it!
>
> By the way, I think the READ_ONCE() likely has the same issue so you
> should fix irq_soft_mask_return() at the same time.
Yes, after disassembling irq_soft_mask_return, she has the same issue
as irq_soft_mask_set.

In addition, I read hw_irq.h by naked eye to search for removed inline
assembly one by one,
I found another place that we could possible enhance (Thank Paul E.
McKenny for teaching me use git blame ;-)):
077fc62b2b66a("powerpc/irq: remove inline assembly in hard_irq_disable macro")
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -282,9 +282,7 @@ static inline bool pmi_irq_pending(void)
        flags = irq_soft_mask_set_return(IRQS_ALL_DISABLED);            \
        local_paca->irq_happened |= PACA_IRQ_HARD_DIS;                  \
        if (!arch_irqs_disabled_flags(flags)) {                         \
-               asm ("stdx %%r1, 0, %1 ;"                               \
-                    : "=m" (local_paca->saved_r1)                      \
-                    : "b" (&local_paca->saved_r1));                    \
+               WRITE_ONCE(local_paca->saved_r1, current_stack_pointer);\
                trace_hardirqs_off();                                   \
        }                                                               \
 } while(0)
I wrap the macro hard_irq_disable into a test function and disassemble
it, she has the above issue too:
(gdb) disassemble test002
Dump of assembler code for function test002:
   0xc000000000295db0 <+0>:    addis   r2,r12,774
   0xc000000000295db4 <+4>:    addi    r2,r2,16464
   0xc000000000295db8 <+8>:    mflr    r0
   0xc000000000295dbc <+12>:    bl      0xc00000000008bacc <mcount>
   0xc000000000295dc0 <+16>:    mflr    r0
   0xc000000000295dc4 <+20>:    std     r30,-16(r1)
   0xc000000000295dc8 <+24>:    std     r31,-8(r1)
   0xc000000000295dcc <+28>:    li      r9,2
   0xc000000000295dd0 <+32>:    std     r0,16(r1)
   0xc000000000295dd4 <+36>:    stdu    r1,-48(r1)
   0xc000000000295dd8 <+40>:    mtmsrd  r9,1
   0xc000000000295ddc <+44>:    addi    r3,r13,2354
   0xc000000000295de0 <+48>:    mr      r31,r13
   0xc000000000295de4 <+52>:    bl      0xc000000000609838 <__asan_load1+8>
   0xc000000000295de8 <+56>:    nop
   0xc000000000295dec <+60>:    li      r3,3
   0xc000000000295df0 <+64>:    lbz     r30,2354(r31)
   0xc000000000295df4 <+68>:    bl      0xc00000000028de90 <irq_soft_mask_set>
   0xc000000000295df8 <+72>:    mr      r31,r13
   0xc000000000295dfc <+76>:    addi    r3,r13,2355
   0xc000000000295e00 <+80>:    bl      0xc000000000609838 <__asan_load1+8>
   0xc000000000295e04 <+84>:    nop
   0xc000000000295e08 <+88>:    lbz     r9,2355(r31)
   0xc000000000295e0c <+92>:    andi.   r10,r30,1
   0xc000000000295e10 <+96>:    ori     r9,r9,1
   0xc000000000295e14 <+100>:    stb     r9,2355(r31)
   0xc000000000295e18 <+104>:    bne     0xc000000000295e30 <test002+128>
   0xc000000000295e1c <+108>:    mr      r30,r1
   0xc000000000295e20 <+112>:    addi    r3,r31,2328
   0xc000000000295e24 <+116>:    bl      0xc000000000609dd8 <__asan_store8+8>
   0xc000000000295e28 <+120>:    nop
   0xc000000000295e2c <+124>:    std     r30,2328(r31)
   0xc000000000295e30 <+128>:    addi    r1,r1,48
   0xc000000000295e34 <+132>:    ld      r0,16(r1)
   0xc000000000295e38 <+136>:    ld      r30,-16(r1)
   0xc000000000295e3c <+140>:    ld      r31,-8(r1)
   0xc000000000295e40 <+144>:    mtlr    r0
   0xc000000000295e44 <+148>:    blr
Could we rewrite this macro into a static inline function as
irq_soft_mask_set does, and disable sanitizer for it?

Thanks again
Cheers
Zhouyi
>
> >
> > Signed-off-by: Zhouyi Zhou <zhouzhouyi@xxxxxxxxx>
> > ---
> > Dear PPC developers
> >
> > I found this bug when trying to do rcutorture tests in ppc VM of
> > Open Source Lab of Oregon State University following Paul E. McKenny's guidance.
> >
> > console.log report following bug:
> >
> > [  346.527467][  T100] BUG: using smp_processor_id() in preemptible [00000000] code: rcu_torture_rea/100^M
> > [  346.529416][  T100] caller is rcu_preempt_deferred_qs_irqrestore+0x74/0xed0^M
> > [  346.531157][  T100] CPU: 4 PID: 100 Comm: rcu_torture_rea Tainted: G        W          5.19.0-rc5-next-20220708-dirty #253^M
> > [  346.533620][  T100] Call Trace:^M
> > [  346.534449][  T100] [c0000000094876c0] [c000000000ce2b68] dump_stack_lvl+0xbc/0x108 (unreliable)^M
> > [  346.536632][  T100] [c000000009487710] [c000000001712954] check_preemption_disabled+0x154/0x160^M
> > [  346.538665][  T100] [c0000000094877a0] [c0000000002ce2d4] rcu_preempt_deferred_qs_irqrestore+0x74/0xed0^M
> > [  346.540830][  T100] [c0000000094878b0] [c0000000002cf3c0] __rcu_read_unlock+0x290/0x3b0^M
> > [  346.542746][  T100] [c000000009487910] [c0000000002bb330] rcu_torture_read_unlock+0x30/0xb0^M
> > [  346.544779][  T100] [c000000009487930] [c0000000002b7ff8] rcutorture_one_extend+0x198/0x810^M
> > [  346.546851][  T100] [c000000009487a10] [c0000000002b8bfc] rcu_torture_one_read+0x58c/0xc90^M
> > [  346.548844][  T100] [c000000009487ca0] [c0000000002b942c] rcu_torture_reader+0x12c/0x360^M
> > [  346.550784][  T100] [c000000009487db0] [c0000000001de978] kthread+0x1e8/0x220^M
> > [  346.552555][  T100] [c000000009487e10] [c00000000000cd54] ret_from_kernel_thread+0x5c/0x64^M
> >
> > After 12 days debugging, I finally narrow the problem to irq_soft_mask_set.
> >
> > I am a beginner, hope I can be of some beneficial to the community ;-)
> >
> > Thanks
> > Zhouyi
> > --
> >   arch/powerpc/include/asm/hw_irq.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> > index 26ede09c521d..a5ae8d82cc9d 100644
> > --- a/arch/powerpc/include/asm/hw_irq.h
> > +++ b/arch/powerpc/include/asm/hw_irq.h
> > @@ -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 inline notrace __no_kcsan __no_sanitize_address void irq_soft_mask_set(unsigned long mask)
> >   {
> >       /*
> >        * The irq mask must always include the STD bit if any are set.