Le 23/08/2022 à 03:43, Zhouyi Zhou a écrit :
> 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:
...
> 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
