* [PATCH] Revert "powerpc/irq: Don't open code irq_soft_mask helpers"
@ 2022-08-31 13:10 Michael Ellerman
2022-08-31 13:15 ` Christophe Leroy
2022-09-02 11:30 ` Michael Ellerman
0 siblings, 2 replies; 4+ messages in thread
From: Michael Ellerman @ 2022-08-31 13:10 UTC (permalink / raw)
To: linuxppc-dev; +Cc: zhouzhouyi, npiggin
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 <zhouzhouyi@gmail.com>
[mpe: Construct change log based on Zhouyi's original report]
Signed-off-by: Michael Ellerman <mpe@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");
}
static inline notrace unsigned long irq_soft_mask_set_return(unsigned long mask)
{
- unsigned long flags = irq_soft_mask_return();
+ unsigned long flags;
- irq_soft_mask_set(mask);
+#ifdef CONFIG_PPC_IRQ_SOFT_MASK_DEBUG
+ WARN_ON(mask && !(mask & IRQS_DISABLED));
+#endif
+
+ asm volatile(
+ "lbz %0,%1(13); stb %2,%1(13)"
+ : "=&r" (flags)
+ : "i" (offsetof(struct paca_struct, irq_soft_mask)),
+ "r" (mask)
+ : "memory");
return flags;
}
static inline notrace unsigned long irq_soft_mask_or_return(unsigned long mask)
{
- unsigned long flags = irq_soft_mask_return();
+ unsigned long flags, tmp;
+
+ asm volatile(
+ "lbz %0,%2(13); or %1,%0,%3; stb %1,%2(13)"
+ : "=&r" (flags), "=r" (tmp)
+ : "i" (offsetof(struct paca_struct, irq_soft_mask)),
+ "r" (mask)
+ : "memory");
- irq_soft_mask_set(flags | mask);
+#ifdef CONFIG_PPC_IRQ_SOFT_MASK_DEBUG
+ WARN_ON((mask | flags) && !((mask | flags) & IRQS_DISABLED));
+#endif
return flags;
}
--
2.37.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "powerpc/irq: Don't open code irq_soft_mask helpers"
2022-08-31 13:10 [PATCH] Revert "powerpc/irq: Don't open code irq_soft_mask helpers" Michael Ellerman
@ 2022-08-31 13:15 ` Christophe Leroy
2022-09-01 3:52 ` Michael Ellerman
2022-09-02 11:30 ` Michael Ellerman
1 sibling, 1 reply; 4+ messages in thread
From: Christophe Leroy @ 2022-08-31 13:15 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: zhouzhouyi, npiggin
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 <zhouzhouyi@gmail.com>
> [mpe: Construct change log based on Zhouyi's original report]
> Signed-off-by: Michael Ellerman <mpe@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.
>
> static inline notrace unsigned long irq_soft_mask_set_return(unsigned long mask)
> {
> - unsigned long flags = irq_soft_mask_return();
> + unsigned long flags;
>
> - irq_soft_mask_set(mask);
> +#ifdef CONFIG_PPC_IRQ_SOFT_MASK_DEBUG
> + WARN_ON(mask && !(mask & IRQS_DISABLED));
> +#endif
> +
> + asm volatile(
> + "lbz %0,%1(13); stb %2,%1(13)"
> + : "=&r" (flags)
> + : "i" (offsetof(struct paca_struct, irq_soft_mask)),
> + "r" (mask)
> + : "memory");
>
> return flags;
> }
>
> static inline notrace unsigned long irq_soft_mask_or_return(unsigned long mask)
> {
> - unsigned long flags = irq_soft_mask_return();
> + unsigned long flags, tmp;
> +
> + asm volatile(
> + "lbz %0,%2(13); or %1,%0,%3; stb %1,%2(13)"
> + : "=&r" (flags), "=r" (tmp)
> + : "i" (offsetof(struct paca_struct, irq_soft_mask)),
> + "r" (mask)
> + : "memory");
>
> - irq_soft_mask_set(flags | mask);
> +#ifdef CONFIG_PPC_IRQ_SOFT_MASK_DEBUG
> + WARN_ON((mask | flags) && !((mask | flags) & IRQS_DISABLED));
> +#endif
>
> return flags;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "powerpc/irq: Don't open code irq_soft_mask helpers"
2022-08-31 13:15 ` Christophe Leroy
@ 2022-09-01 3:52 ` Michael Ellerman
0 siblings, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2022-09-01 3:52 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: zhouzhouyi, npiggin
Christophe Leroy <christophe.leroy@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 <zhouzhouyi@gmail.com>
>> [mpe: Construct change log based on Zhouyi's original report]
>> Signed-off-by: Michael Ellerman <mpe@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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "powerpc/irq: Don't open code irq_soft_mask helpers"
2022-08-31 13:10 [PATCH] Revert "powerpc/irq: Don't open code irq_soft_mask helpers" Michael Ellerman
2022-08-31 13:15 ` Christophe Leroy
@ 2022-09-02 11:30 ` Michael Ellerman
1 sibling, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2022-09-02 11:30 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman; +Cc: zhouzhouyi, npiggin
On Wed, 31 Aug 2022 23:10:52 +1000, Michael Ellerman wrote:
> 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
>
> [...]
Applied to powerpc/fixes.
[1/1] Revert "powerpc/irq: Don't open code irq_soft_mask helpers"
https://git.kernel.org/powerpc/c/684c68d92e2e1b97fa2f31c35c1b0f7671a8618a
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-09-02 11:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-31 13:10 [PATCH] Revert "powerpc/irq: Don't open code irq_soft_mask helpers" Michael Ellerman
2022-08-31 13:15 ` Christophe Leroy
2022-09-01 3:52 ` Michael Ellerman
2022-09-02 11:30 ` Michael Ellerman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.