From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-09-01 16:55:12
If a system call is made with a transaction active, the kernel
immediately aborts it and returns. scv system calls disable irqs even
earlier in their interrupt handler, and tabort_syscall does not fix this
up.
This can result in irq soft-mask state being messed up on the next
kernel entry, and crashing at BUG_ON(arch_irq_disabled_regs(regs)) in
the kernel exit handlers, or possibly worse.
Fix this by having tabort_syscall setting irq soft-mask back to enabled
(which requires MSR[EE] be disabled first).
Reported-by: Eirik Fuller <redacted>
Fixes: 7fa95f9adaee7 ("powerpc/64s: system call support for scv/rfscv instructions")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Tested the wrong kernel before sending v1 and missed a bug, sorry.
arch/powerpc/kernel/interrupt_64.S | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-09-01 16:55:50
The basic TM vs syscall test code hard codes an sc instruction for the
system call, which fails to cover scv even when the userspace libc has
support for it.
Duplicate the tests with hard coded scv variants so both are tested
when possible.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
.../selftests/powerpc/tm/tm-syscall-asm.S | 46 +++++++++++++++++++
.../testing/selftests/powerpc/tm/tm-syscall.c | 36 ++++++++++++---
2 files changed, 75 insertions(+), 7 deletions(-)
@@ -82,15 +95,24 @@ int tm_syscall(void)*Testasyscallwithinasuspendedtransactionandverify*thatitsucceeds.*/-FAIL_IF(getppid_tm(true)==-1);/* Should succeed. */+FAIL_IF(getppid_tm(false,true)==-1);/* Should succeed. *//**Testasyscallwithinanactivetransactionandverifythat*itfailswiththecorrectfailurecode.*/-FAIL_IF(getppid_tm(false)!=-1);/* Should fail... */+FAIL_IF(getppid_tm(false,false)!=-1);/* Should fail... */FAIL_IF(!failure_is_persistent());/* ...persistently... */FAIL_IF(!failure_is_syscall());/* ...with code syscall. */++/* Now do it all again with scv if it is available. */+if(have_hwcap2(PPC_FEATURE2_SCV)){+FAIL_IF(getppid_tm(true,true)==-1);/* Should succeed. */+FAIL_IF(getppid_tm(true,false)!=-1);/* Should fail... */+FAIL_IF(!failure_is_persistent());/* ...persistently... */+FAIL_IF(!failure_is_syscall());/* ...with code syscall. */+}+gettimeofday(&now,0);}
The basic TM vs syscall test code hard codes an sc instruction for the
system call, which fails to cover scv even when the userspace libc has
support for it.
Duplicate the tests with hard coded scv variants so both are tested
when possible.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
.../selftests/powerpc/tm/tm-syscall-asm.S | 46 +++++++++++++++++++
.../testing/selftests/powerpc/tm/tm-syscall.c | 36 ++++++++++++---
2 files changed, 75 insertions(+), 7 deletions(-)
@@ -82,15 +95,24 @@ int tm_syscall(void)*Testasyscallwithinasuspendedtransactionandverify*thatitsucceeds.*/-FAIL_IF(getppid_tm(true)==-1);/* Should succeed. */+FAIL_IF(getppid_tm(false,true)==-1);/* Should succeed. *//**Testasyscallwithinanactivetransactionandverifythat*itfailswiththecorrectfailurecode.*/-FAIL_IF(getppid_tm(false)!=-1);/* Should fail... */+FAIL_IF(getppid_tm(false,false)!=-1);/* Should fail... */FAIL_IF(!failure_is_persistent());/* ...persistently... */FAIL_IF(!failure_is_syscall());/* ...with code syscall. */++/* Now do it all again with scv if it is available. */+if(have_hwcap2(PPC_FEATURE2_SCV)){+FAIL_IF(getppid_tm(true,true)==-1);/* Should succeed. */+FAIL_IF(getppid_tm(true,false)!=-1);/* Should fail... */+FAIL_IF(!failure_is_persistent());/* ...persistently... */+FAIL_IF(!failure_is_syscall());/* ...with code syscall. */+}+gettimeofday(&now,0);}
If a system call is made with a transaction active, the kernel
immediately aborts it and returns. scv system calls disable irqs even
earlier in their interrupt handler, and tabort_syscall does not fix this
up.
This can result in irq soft-mask state being messed up on the next
kernel entry, and crashing at BUG_ON(arch_irq_disabled_regs(regs)) in
the kernel exit handlers, or possibly worse.
Fix this by having tabort_syscall setting irq soft-mask back to enabled
(which requires MSR[EE] be disabled first).
Reported-by: Eirik Fuller <redacted>
Fixes: 7fa95f9adaee7 ("powerpc/64s: system call support for scv/rfscv instructions")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Tested the wrong kernel before sending v1 and missed a bug, sorry.
arch/powerpc/kernel/interrupt_64.S | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
Why not use 'rlwinm' to mask out MSR_EE ?
Something like
rlwinm r10, r10, 0, ~MSR_EE
mtmsrd r10, 0
/* tabort, this dooms the transaction, nothing else */
li r9, (TM_CAUSE_SYSCALL|TM_CAUSE_PERSISTENT)
TABORT(R9)
+ /* scv has disabled irqs so must re-enable. sc just remains enabled */
+ li r9,IRQS_ENABLED
+ stb r9,PACAIRQSOFTMASK(r13)
+
/*
* Return directly to userspace. We have corrupted user register state,
* but userspace will never see that register state. Execution will
From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-09-02 03:28:04
Excerpts from Christophe Leroy's message of September 2, 2021 3:15 am:
Le 01/09/2021 à 18:54, Nicholas Piggin a écrit :
quoted
The basic TM vs syscall test code hard codes an sc instruction for the
system call, which fails to cover scv even when the userspace libc has
support for it.
Duplicate the tests with hard coded scv variants so both are tested
when possible.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
.../selftests/powerpc/tm/tm-syscall-asm.S | 46 +++++++++++++++++++
.../testing/selftests/powerpc/tm/tm-syscall.c | 36 ++++++++++++---
2 files changed, 75 insertions(+), 7 deletions(-)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-09-02 03:34:00
Excerpts from Christophe Leroy's message of September 2, 2021 3:21 am:
Le 01/09/2021 à 18:54, Nicholas Piggin a écrit :
quoted
If a system call is made with a transaction active, the kernel
immediately aborts it and returns. scv system calls disable irqs even
earlier in their interrupt handler, and tabort_syscall does not fix this
up.
This can result in irq soft-mask state being messed up on the next
kernel entry, and crashing at BUG_ON(arch_irq_disabled_regs(regs)) in
the kernel exit handlers, or possibly worse.
Fix this by having tabort_syscall setting irq soft-mask back to enabled
(which requires MSR[EE] be disabled first).
Reported-by: Eirik Fuller <redacted>
Fixes: 7fa95f9adaee7 ("powerpc/64s: system call support for scv/rfscv instructions")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Tested the wrong kernel before sending v1 and missed a bug, sorry.
arch/powerpc/kernel/interrupt_64.S | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
Why not use 'rlwinm' to mask out MSR_EE ?
Something like
rlwinm r10, r10, 0, ~MSR_EE
Mainly because I'm bad at powerpc assembly. Why do you think I'm trying
to change as much as possible to C?
Actually there should really be no need for mfmsr either, I wanted to
rewrite the thing entirely as
ld r10,PACAKMSR(r13)
LOAD_REG_IMMEDIATE(r9, MSR_TM)
or r10,r10,r9
mtmsrd r10
But I thought that's not a minimal bug fix.
Thanks,
Nick
quoted
mtmsrd r10, 0
/* tabort, this dooms the transaction, nothing else */
li r9, (TM_CAUSE_SYSCALL|TM_CAUSE_PERSISTENT)
TABORT(R9)
+ /* scv has disabled irqs so must re-enable. sc just remains enabled */
+ li r9,IRQS_ENABLED
+ stb r9,PACAIRQSOFTMASK(r13)
+
/*
* Return directly to userspace. We have corrupted user register state,
* but userspace will never see that register state. Execution will
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-09-02 03:45:21
Nicholas Piggin [off-list ref] writes:
Excerpts from Christophe Leroy's message of September 2, 2021 3:15 am:
quoted
Le 01/09/2021 à 18:54, Nicholas Piggin a écrit :
quoted
The basic TM vs syscall test code hard codes an sc instruction for the
system call, which fails to cover scv even when the userspace libc has
support for it.
Duplicate the tests with hard coded scv variants so both are tested
when possible.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
.../selftests/powerpc/tm/tm-syscall-asm.S | 46 +++++++++++++++++++
.../testing/selftests/powerpc/tm/tm-syscall.c | 36 ++++++++++++---
2 files changed, 75 insertions(+), 7 deletions(-)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-09-02 05:36:54
Nicholas Piggin [off-list ref] writes:
quoted hunk
The basic TM vs syscall test code hard codes an sc instruction for the
system call, which fails to cover scv even when the userspace libc has
support for it.
Duplicate the tests with hard coded scv variants so both are tested
when possible.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
.../selftests/powerpc/tm/tm-syscall-asm.S | 46 +++++++++++++++++++
.../testing/selftests/powerpc/tm/tm-syscall.c | 36 ++++++++++++---
2 files changed, 75 insertions(+), 7 deletions(-)
There's some macros in tools/testing/selftests/powerpc/include/basic_asm.h
that can do some of this boiler plate stack setup/teardown.
Incremental diff below to use them, only build tested.
cheers
On Thu, Sep 02, 2021 at 01:33:10PM +1000, Nicholas Piggin wrote:
Excerpts from Christophe Leroy's message of September 2, 2021 3:21 am:
quoted
quoted
- /* Firstly we need to enable TM in the kernel */
+ /* We need to enable TM in the kernel, and disable EE (for scv) */
mfmsr r10
li r9, 1
rldimi r10, r9, MSR_TM_LG, 63-MSR_TM_LG
+ LOAD_REG_IMMEDIATE(r9, MSR_EE)
+ andc r10, r10, r9
Why not use 'rlwinm' to mask out MSR_EE ?
Something like
rlwinm r10, r10, 0, ~MSR_EE
Mainly because I'm bad at powerpc assembly. Why do you think I'm trying
to change as much as possible to C?
The actual bit (bit 31, i.e. with value 1UL << 32) cannot be cleared
with rlwinm (only the low 32 bits can). There are many ways to do it
using two insns of course.
Actually there should really be no need for mfmsr either, I wanted to
rewrite the thing entirely as
ld r10,PACAKMSR(r13)
LOAD_REG_IMMEDIATE(r9, MSR_TM)
or r10,r10,r9
mtmsrd r10
But I thought that's not a minimal bug fix.
That (LOAD_REG_IMMEDIATE+or) can be done with just two insns, not three
as written:
li r0,1
rldimi r10,r0,32,31
(you can write that last insns as
rldimi r10,r0,MSR_TM_LG,MSR_TM_LG-1
if you insist :-) )
It isn't like a few integer computational insns will kill you here of
course, there are much more important cycles to shave off :-)
Segher
On Thu, Sep 02, 2021 at 04:52:03PM -0500, Segher Boessenkool wrote:
On Thu, Sep 02, 2021 at 01:33:10PM +1000, Nicholas Piggin wrote:
quoted
Excerpts from Christophe Leroy's message of September 2, 2021 3:21 am:
quoted
quoted
- /* Firstly we need to enable TM in the kernel */
+ /* We need to enable TM in the kernel, and disable EE (for scv) */
mfmsr r10
li r9, 1
rldimi r10, r9, MSR_TM_LG, 63-MSR_TM_LG
+ LOAD_REG_IMMEDIATE(r9, MSR_EE)
+ andc r10, r10, r9
Why not use 'rlwinm' to mask out MSR_EE ?
Something like
rlwinm r10, r10, 0, ~MSR_EE
Mainly because I'm bad at powerpc assembly. Why do you think I'm trying
to change as much as possible to C?
The actual bit (bit 31, i.e. with value 1UL << 32) cannot be cleared
with rlwinm (only the low 32 bits can). There are many ways to do it
using two insns of course.
Wow I misread that, you want to clear MSR[EE] really, not MSR[TM].
You cannot use rlwinm and keep the high 32 bits of the target register
intact. You either clear all to 0 or set them to a copy of the rotated
value in the low 32 bits.
Segher
Le 03/09/2021 à 00:20, Segher Boessenkool a écrit :
On Thu, Sep 02, 2021 at 04:52:03PM -0500, Segher Boessenkool wrote:
quoted
On Thu, Sep 02, 2021 at 01:33:10PM +1000, Nicholas Piggin wrote:
quoted
Excerpts from Christophe Leroy's message of September 2, 2021 3:21 am:
quoted
quoted
- /* Firstly we need to enable TM in the kernel */
+ /* We need to enable TM in the kernel, and disable EE (for scv) */
mfmsr r10
li r9, 1
rldimi r10, r9, MSR_TM_LG, 63-MSR_TM_LG
+ LOAD_REG_IMMEDIATE(r9, MSR_EE)
+ andc r10, r10, r9
Why not use 'rlwinm' to mask out MSR_EE ?
Something like
rlwinm r10, r10, 0, ~MSR_EE
Mainly because I'm bad at powerpc assembly. Why do you think I'm trying
to change as much as possible to C?
The actual bit (bit 31, i.e. with value 1UL << 32) cannot be cleared
with rlwinm (only the low 32 bits can). There are many ways to do it
using two insns of course.
Wow I misread that, you want to clear MSR[EE] really, not MSR[TM].
You cannot use rlwinm and keep the high 32 bits of the target register
intact. You either clear all to 0 or set them to a copy of the rotated
value in the low 32 bits.
Oops, my mistake. When I tested it in C to see what was generated by GCC I forgot the ~ so I got
rlwinm r3,r3,0,16,16 and didn't realise it was different from rlwinm r3,r3,0,~(1<<15)
By the way it would be more explicit if objdump could display the mask instead of the mask
boundaries. Is there a way to do that ?
Christophe