Christophe Leroy <christophe.le...@csgroup.eu> writes:
> Le 31/08/2022 à 15:10, Michael Ellerman a écrit :
>> This reverts commit ef5b570d3700fbb8628a58da0487486ceeb713cd.
>>
>> Zhouyi reported that commit is causing crashes when running rcutorture
>> with KASAN enabled:
>>
>> BUG: using smp_processor_id() in preemptible [00000000] code:
>> rcu_torture_rea/100
>> caller is rcu_preempt_deferred_qs_irqrestore+0x74/0xed0
>> CPU: 4 PID: 100 Comm: rcu_torture_rea Tainted: G W
>> 5.19.0-rc5-next-20220708-dirty #253
>> Call Trace:
>> dump_stack_lvl+0xbc/0x108 (unreliable)
>> check_preemption_disabled+0x154/0x160
>> rcu_preempt_deferred_qs_irqrestore+0x74/0xed0
>> __rcu_read_unlock+0x290/0x3b0
>> rcu_torture_read_unlock+0x30/0xb0
>> rcutorture_one_extend+0x198/0x810
>> rcu_torture_one_read+0x58c/0xc90
>> rcu_torture_reader+0x12c/0x360
>> kthread+0x1e8/0x220
>> ret_from_kernel_thread+0x5c/0x64
>>
>> KASAN will generate instrumentation instructions around the
>> 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 usual solution of marking the code ineligible for instrumentation
>> forces the code out-of-line, which we would prefer to avoid. Christophe
>> proposed a partial revert, but Nick raised some concerns with that. So
>> for now do a full revert.
>>
>> Reported-by: Zhouyi Zhou <zhouzho...@gmail.com>
>> [mpe: Construct change log based on Zhouyi's original report]
>> Signed-off-by: Michael Ellerman <m...@ellerman.id.au>
>> ---
>> arch/powerpc/include/asm/hw_irq.h | 43 ++++++++++++++++++++++++++-----
>> 1 file changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/hw_irq.h
>> b/arch/powerpc/include/asm/hw_irq.h
>> index 3c8cb48f88ae..983551859891 100644
>> --- a/arch/powerpc/include/asm/hw_irq.h
>> +++ b/arch/powerpc/include/asm/hw_irq.h
>> @@ -113,7 +113,14 @@ 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 %0,%1(13)"
>> + : "=r" (flags)
>> + : "i" (offsetof(struct paca_struct, irq_soft_mask)));
>> +
>> + return flags;
>> }
>>
>> /*
>> @@ -140,24 +147,46 @@ 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 %0,%1(13)"
>> + :
>> + : "r" (mask),
>> + "i" (offsetof(struct paca_struct, irq_soft_mask))
>> + : "memory");
>> }
>
> Only the above changes need to be reverted, below ones should remain as
> they are.
I agree, except that it's nearly rc4.
So I want to go back to code that's been well tested, and we can do any
further changes in next for v6.1.
cheers
