Re: BUG : PowerPC RCU: torture test failed with __stack_chk_fail

[Michael Ellerman]

Hi Boqun,

Thanks for debugging this ...

Boqun Feng <boqun.feng@xxxxxxxxx> writes:
> On Sat, Apr 22, 2023 at 09:28:39PM +0200, Joel Fernandes wrote:
>> On Sat, Apr 22, 2023 at 2:47 PM Zhouyi Zhou <zhouzhouyi@xxxxxxxxx> wrote:
>> >
>> > Dear PowerPC and RCU developers:
>> > During the RCU torture test on mainline (on the VM of Opensource Lab
>> > of Oregon State University), SRCU-P failed with __stack_chk_fail:
>> > [  264.381952][   T99] [c000000006c7bab0] [c0000000010c67c0]
>> > dump_stack_lvl+0x94/0xd8 (unreliable)
>> > [  264.383786][   T99] [c000000006c7bae0] [c00000000014fc94] panic+0x19c/0x468
>> > [  264.385128][   T99] [c000000006c7bb80] [c0000000010fca24]
>> > __stack_chk_fail+0x24/0x30
>> > [  264.386610][   T99] [c000000006c7bbe0] [c0000000002293b4]
>> > srcu_gp_start_if_needed+0x5c4/0x5d0
>> > [  264.388188][   T99] [c000000006c7bc70] [c00000000022f7f4]
>> > srcu_torture_call+0x34/0x50
>> > [  264.389611][   T99] [c000000006c7bc90] [c00000000022b5e8]
>> > rcu_torture_fwd_prog+0x8c8/0xa60
>> > [  264.391439][   T99] [c000000006c7be00] [c00000000018e37c] kthread+0x15c/0x170
>> > [  264.392792][   T99] [c000000006c7be50] [c00000000000df94]
>> > ret_from_kernel_thread+0x5c/0x64
>> > The kernel config file can be found in [1].
>> > And I write a bash script to accelerate the bug reproducing [2].
>> > After a week's debugging, I found the cause of the bug is because the
>> > register r10 used to judge for stack overflow is not constant between
>> > context switches.
>> > The assembly code for srcu_gp_start_if_needed is located at [3]:
>> > c000000000226eb4:   78 6b aa 7d     mr      r10,r13
>> > c000000000226eb8:   14 42 29 7d     add     r9,r9,r8
>> > c000000000226ebc:   ac 04 00 7c     hwsync
>> > c000000000226ec0:   10 00 7b 3b     addi    r27,r27,16
>> > c000000000226ec4:   14 da 29 7d     add     r9,r9,r27
>> > c000000000226ec8:   a8 48 00 7d     ldarx   r8,0,r9
>> > c000000000226ecc:   01 00 08 31     addic   r8,r8,1
>> > c000000000226ed0:   ad 49 00 7d     stdcx.  r8,0,r9
>> > c000000000226ed4:   f4 ff c2 40     bne-    c000000000226ec8
>> > <srcu_gp_start_if_needed+0x1c8>
>> > c000000000226ed8:   28 00 21 e9     ld      r9,40(r1)
>> > c000000000226edc:   78 0c 4a e9     ld      r10,3192(r10)
>> > c000000000226ee0:   79 52 29 7d     xor.    r9,r9,r10
>> > c000000000226ee4:   00 00 40 39     li      r10,0
>> > c000000000226ee8:   b8 03 82 40     bne     c0000000002272a0
>> > <srcu_gp_start_if_needed+0x5a0>
>> > by debugging, I see the r10 is assigned with r13 on c000000000226eb4,
>> > but if there is a context-switch before c000000000226edc, a false
>> > positive will be reported.
>> >
>> > [1] http://154.220.3.115/logs/0422/configformainline.txt
>> > [2] 154.220.3.115/logs/0422/whilebash.sh
>> > [3] http://154.220.3.115/logs/0422/srcu_gp_start_if_needed.txt
>> >
>> > My analysis and debugging may not be correct, but the bug is easily
>> > reproducible.
>> 
>> If this is a bug in the stack smashing protection as you seem to hint,
>> I wonder if you see the issue with a specific gcc version and is a
>> compiler-specific issue. It's hard to say, but considering this I
>
> Very likely, more asm code from Zhouyi's link:
>
> This is the __srcu_read_unlock_nmisafe(), since "hwsync" is
> smp_mb__{after,before}_atomic(), and the following code is first
> barrier then atomic, so it's the unlock.
>
> 	c000000000226eb4:	78 6b aa 7d 	mr      r10,r13
>
> ^ r13 is the pointer to percpu data on PPC64 kernel, and it's also
> the pointer to TLS data for userspace code.

I've never understood why the compiler wants to make a copy of a
register variable into another register!? >:#

> 	c000000000226eb8:	14 42 29 7d 	add     r9,r9,r8
> 	c000000000226ebc:	ac 04 00 7c 	hwsync
> 	c000000000226ec0:	10 00 7b 3b 	addi    r27,r27,16
> 	c000000000226ec4:	14 da 29 7d 	add     r9,r9,r27
> 	c000000000226ec8:	a8 48 00 7d 	ldarx   r8,0,r9
> 	c000000000226ecc:	01 00 08 31 	addic   r8,r8,1
> 	c000000000226ed0:	ad 49 00 7d 	stdcx.  r8,0,r9
> 	c000000000226ed4:	f4 ff c2 40 	bne-    c000000000226ec8 <srcu_gp_start_if_needed+0x1c8>
> 	c000000000226ed8:	28 00 21 e9 	ld      r9,40(r1)
> 	c000000000226edc:	78 0c 4a e9 	ld      r10,3192(r10)
>
> here I think that the compiler is using r10 as an alias to r13, since
> for userspace program, it's safe to assume the TLS pointer doesn't
> change. However this is not true for kernel percpu pointer.
>
> The real intention here is to compare 40(r1) vs 3192(r13) for stack
> guard checking, however since r13 is the percpu pointer in kernel, so
> the value of r13 can be changed if the thread gets scheduled to a
> different CPU after reading r13 for r10.

Yeah that's not good.

> If I'm correct, the following should be a workaround:
>
> 	diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> 	index ab4ee58af84b..f5ae3be3d04d 100644
> 	--- a/kernel/rcu/srcutree.c
> 	+++ b/kernel/rcu/srcutree.c
> 	@@ -747,6 +747,7 @@ void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>
> 		smp_mb__before_atomic(); /* C */  /* Avoid leaking the critical section. */
> 		atomic_long_inc(&sdp->srcu_unlock_count[idx]);
> 	+       asm volatile("" : : : "r13", "memory");
> 	 }
> 	 EXPORT_SYMBOL_GPL(__srcu_read_unlock_nmisafe);
>
> Zhouyi, could you give a try? Note I think the "memory" clobber here is
> unnecesarry, but I just add it in case I'm wrong.
>
> Needless to say, the correct fix is to make ppc stack protector aware of
> r13 is volatile.

I suspect the compiler developers will tell us to go jump :)

The problem of the compiler caching r13 has come up in the past, but I
only remember it being "a worry" rather than causing an actual bug.

We've had the DEBUG_PREEMPT checks in get_paca(), which have given us at
least some comfort that if the compiler is caching r13, it shouldn't be
doing it in preemptable regions.

But obviously that doesn't help at all with the stack protector check.

I don't see an easy fix.

Adding "volatile" to the definition of local_paca seems to reduce but
not elimate the caching of r13, and the GCC docs explicitly say *not* to
use volatile. It also triggers lots of warnings about volatile being
discarded.

Short term we can make stack protector depend on !PREEMPT.

Longer term possibly we can move to having current in a register like
32-bit does, and then use that as the stack protector reg.

Or something simple I haven't thought of? :)

cheers