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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 23/08/2022 à 03:43, Zhouyi Zhou a écrit :
> On Mon, Aug 22, 2022 at 2:04 PM Christophe Leroy
> <christophe.leroy@csgroup.eu> 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:
...
> Could we rewrite this macro into a static inline function as
> irq_soft_mask_set does, and disable sanitizer for it?

Yes we could try to do that, hoping it will not bring too much troubles 
with circular header inclusion.

Another possible approach could be to create a WRITE_ONCE_NOCHECK() more 
or less similar to existing READ_ONCE_NOCHECK().

We could also change READ_ONCE_NOCHECK() to accept bytes size then use 
it for irq_soft_mask_return() .

Christophe