powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state
Nicholas Piggin
reviewerPATCH v3 1/2 powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state
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.
This can't easily be fixed in asm because at this point an async irq may have hit, which is soft-masked and marked pending. The pending interrupt has to be replayed before returning to userspace. The fix is to move the tabort_syscall code to C in the main syscall handler, and just skip the system call but otherwise return as usual, which will take care of the pending irqs. This also does a bunch of other things including possible signal delivery to the process, but the doomed transaction should still be aborted when it is eventually returned to.
The sc system call path is changed to use the new C function as well to reduce code and path differences. This slows down how quickly system calls are aborted when called while a transaction is active, which could potentially impact TM performance. But making any system call is already bad for performance, and TM is on the way out, so go with simpler over faster.
---
v2 of this fix had a bug where an irq could be soft masked and pending before we hard disable interrupts in tabort_syscall for the case of scv (because it enters the kernel with EE enabled). So this actually requires a pretty large change to fix because we can't replay interrupts just from this early asm context.
Thanks,
Nick
arch/powerpc/kernel/interrupt.c | 29 +++++++++++++++++++++ arch/powerpc/kernel/interrupt_64.S | 41 ------------------------------ 2 files changed, 29 insertions(+), 41 deletions(-)
arch/powerpc/kernel/interrupt.c +29 -0
@@ -19,6 +19,7 @@#include <asm/switch_to.h> #include <asm/syscall.h> #include <asm/time.h> +#include <asm/tm.h> #include <asm/unistd.h> #if defined(CONFIG_PPC_ADV_DEBUG_REGS ) && defined(CONFIG_PPC32 )
@@ -138,6 +139,34 @@ notrace long system_call_exception(long r3, long r4, long r5,*/ irq_soft_mask_regs_set_state (regs,IRQS_ENABLED ); +/* +* If the system call was made with a transaction active, doom it and +* return without performing the system call. Unless it was an +* unsupported scv vector, in which case it's treated like an illegal +* instruction. +*/ +if (IS_ENABLED (CONFIG_PPC_TRANSACTIONAL_MEM ) && +unlikely (MSR_TM_TRANSACTIONAL (regs->msr)) && + !trap_is_unsupported_scv (regs)) { +/* Enable TM in the kernel, and disable EE (for scv) */ +hard_irq_disable (); +mtmsr (mfmsr () |MSR_TM ); + +/* tabort, this dooms the transaction, nothing else */ + asmvolatile (".long 0x7c00071d | ((%0) << 16)" + ::"r" (TM_CAUSE_SYSCALL |TM_CAUSE_PERSISTENT )); + +/* +* Userspace will never see the return value. Execution will +* resume after the tbegin. of the aborted transaction with the +* checkpointed register state. A context switch could occur +* or signal delivered to the process before resuming the +* doomed transaction context, but that should all be handled +* as expected. +*/ +return -ENOSYS ; + } +local_irq_enable ();if (unlikely (current_thread_info ()->flags & _TIF_SYSCALL_DOTRACE)) {
arch/powerpc/kernel/interrupt_64.S +0 -41
65 lines not on this page · raw message
Signature · 1 line
2.23.0
Nicholas Piggin
reviewerPATCH v3 2/2 selftests/powerpc: Add scv versions of the basic TM syscall tests
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.
--- .../selftests/powerpc/tm/tm-syscall-asm.S | 32 ++++++++++++++++- .../testing/selftests/powerpc/tm/tm-syscall.c | 36 +++++++++++++++---- 2 files changed, 60 insertions(+), 8 deletions(-)
tools/testing/selftests/powerpc/tm/tm-syscall-asm.S +31 -1
@@ -1,5 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0 */ -#include <ppc-asm.h> +#include <basic_asm.h> #include <asm/unistd.h> .text
@@ -26,3 +26,33 @@ FUNC_START(getppid_tm_suspended) 1: li r3, -1 blr + +FUNC_START(getppid_scv_tm_active) + PUSH_BASIC_STACK(0) + tbegin. + beq 1f + li r0, __NR_getppid + scv 0 + tend. + POP_BASIC_STACK(0) + blr +1: + li r3, -1 + POP_BASIC_STACK(0) + blr + +FUNC_START(getppid_scv_tm_suspended) + PUSH_BASIC_STACK(0) + tbegin. + beq 1f + li r0, __NR_getppid + tsuspend. + scv 0 + tresume. + tend. + POP_BASIC_STACK(0) + blr +1: + li r3, -1 + POP_BASIC_STACK(0) + blr
tools/testing/selftests/powerpc/tm/tm-syscall.c +29 -7
@@ -19,24 +19,37 @@#include "utils.h" #include "tm.h" +#ifndef PPC_FEATURE2_SCV +#define PPC_FEATURE2_SCV 0x00100000/* scv syscall */ +#endif +extern int getppid_tm_active (void );extern int getppid_tm_suspended (void ); +extern int getppid_scv_tm_active (void ); +extern int getppid_scv_tm_suspended (void );unsigned retries =0 ;#define TEST_DURATION 10/* seconds */ #define TM_RETRIES 100 -pid_t getppid_tm (bool suspend) +pid_t getppid_tm (bool scv,bool suspend) {int i;pid_t pid;for (i =0 ; i <TM_RETRIES ; i++) { -if (suspend) - pid =getppid_tm_suspended (); -else - pid =getppid_tm_active (); +if (suspend) { +if (scv) + pid =getppid_scv_tm_suspended (); +else + pid =getppid_tm_suspended (); + }else { +if (scv) + pid =getppid_scv_tm_active (); +else + pid =getppid_tm_active (); + }if (pid >=0 )return pid;
@@ -82,15 +95,24 @@ int tm_syscall(void)* Test a syscall within a suspended transaction and verify * that it succeeds. */ -FAIL_IF (getppid_tm (true) ==-1 );/* Should succeed. */ +FAIL_IF (getppid_tm (false, true) ==-1 );/* Should succeed. */ /* * Test a syscall within an active transaction and verify that * it fails with the correct failure code. */ -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 ); }
Signature · 1 line
2.23.0
Michael Ellerman
AppliedQuoted 10 lines · Nicholas Piggin
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_syscalldoes 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.
[...]
Applied to powerpc/fixes.
[1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state
https://git.kernel.org/powerpc/c/b871895b148256f1721bc565d803860242755a0b [2/2] selftests/powerpc: Add scv versions of the basic TM syscall tests https://git.kernel.org/powerpc/c/5379ef2a60431232b9bb01c6d3580b875123d723
cheers