On a signal handler return, the user could set a context with MSR[TS] bits
set, and these bits would be copied to task regs->msr.
At restore_tm_sigcontexts(), after current task regs->msr[TS] bits are set,
several __get_user() are called and then a recheckpoint is executed.
This is a problem since a page fault (in kernel space) could happen when
calling __get_user(). If it happens, the process MSR[TS] bits were
already set, but recheckpoint was not executed, and SPRs are still invalid.
The page fault can cause the current process to be de-scheduled, with
MSR[TS] active and without tm_recheckpoint() being called. More
importantly, without TEXASR[FS] bit set also.
Since TEXASR might not have the FS bit set, and when the process is
scheduled back, it will try to reclaim, which will be aborted because of
the CPU is not in the suspended state, and, then, recheckpoint. This
recheckpoint will restore thread->texasr into TEXASR SPR, which might be
zero, hitting a BUG_ON().
kernel BUG at /build/linux-sf3Co9/linux-4.9.30/arch/powerpc/kernel/tm.S:434!
cpu 0xb: Vector: 700 (Program Check) at [c00000041f1576d0]
pc: c000000000054550: restore_gprs+0xb0/0x180
lr: 0000000000000000
sp: c00000041f157950
msr: 8000000100021033
current = 0xc00000041f143000
paca = 0xc00000000fb86300 softe: 0 irq_happened: 0x01
pid = 1021, comm = kworker/11:1
kernel BUG at /build/linux-sf3Co9/linux-4.9.30/arch/powerpc/kernel/tm.S:434!
Linux version 4.9.0-3-powerpc64le (debian-kernel@lists.debian.org) (gcc version 6.3.0 20170516 (Debian 6.3.0-18) ) #1 SMP Debian 4.9.30-2+deb9u2 (2017-06-26)
enter ? for help
[c00000041f157b30] c00000000001bc3c tm_recheckpoint.part.11+0x6c/0xa0
[c00000041f157b70] c00000000001d184 __switch_to+0x1e4/0x4c0
[c00000041f157bd0] c00000000082eeb8 __schedule+0x2f8/0x990
[c00000041f157cb0] c00000000082f598 schedule+0x48/0xc0
[c00000041f157ce0] c0000000000f0d28 worker_thread+0x148/0x610
[c00000041f157d80] c0000000000f96b0 kthread+0x120/0x140
[c00000041f157e30] c00000000000c0e0 ret_from_kernel_thread+0x5c/0x7c
This patch simply delays the MSR[TS] set, so, if there is any page fault in
the __get_user() section, it does not have regs->msr[TS] set, since the TM
structures are still invalid, thus avoiding doing TM operations for
in-kernel exceptions and possible process reschedule.
With this patch, the MSR[TS] will only be set just before recheckpointing
and setting TEXASR[FS] = 1, thus avoiding an interrupt with TM registers in
invalid state.
Other than that, if CONFIG_PREEMPT is set, there might be a preemption just
after setting MSR[TS] and before tm_recheckpoint(), thus, this block must
be atomic from a preemption perspective, thus, calling
preempt_disable/enable() on this code.
It is not possible to move tm_recheckpoint to happen earlier, because it is
required to get the checkpointed registers from userspace, with
__get_user(), thus, the only way to avoid this undesired behavior is
delaying the MSR[TS] set.
The 32-bits signal handler seems to be safe this current issue, but, it
might be exposed to the preemption issue, thus, disabling preemption in
this chunk of code.
Changes from v2:
* Run the critical section with preempt_disable.
Fixes: 87b4e5393af7 ("powerpc/tm: Fix return of active 64bit signals")
Cc: stable@vger.kernel.org (v3.9+)
Signed-off-by: Breno Leitao <leitao@debian.org>
@@ -848,7 +848,23 @@ static long restore_tm_user_regs(struct pt_regs *regs,/* If TM bits are set to the reserved value, it's an invalid context */if(MSR_TM_RESV(msr_hi))return1;-/* Pull in the MSR TM bits from the user context */++/*+*Disablingpreemption,sinceitisunsafetobepreempted+*withMSR[TS]setwithoutrecheckpointing.+*/+preempt_disable();++/*+*CAUTION:+*Afterregs->MSR[TS]beingupdated,makesurethatget_user(),+*put_user()orsimilarfunctionsare*not*called.These+*functionscangeneratepagefaultswhichwillcausetheprocess+*tobede-scheduledwithMSR[TS]setbutwithoutcalling+*tm_recheckpoint().Thiscancauseabug.+*+*PullintheMSRTMbitsfromtheusercontext+*/regs->msr=(regs->msr&~MSR_TS_MASK)|(msr_hi&MSR_TS_MASK);/* Now, recheckpoint. This loads up all of the checkpointed (older)*registers,includingFPandV[S]Rs.Afterrecheckpointing,the
@@ -873,6 +889,8 @@ static long restore_tm_user_regs(struct pt_regs *regs,}#endif+preempt_enable();+return0;}#endif
@@ -467,20 +467,6 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,if(MSR_TM_RESV(msr))return-EINVAL;-/* pull in MSR TS bits from user context */-regs->msr=(regs->msr&~MSR_TS_MASK)|(msr&MSR_TS_MASK);--/*-*EnsurethatTMisenabledinregs->msrbeforeweleavethesignal-*handler.Itcouldbethecasethat(a)userdisabledtheTMbit-*throughthemanipulationoftheMSRbitsinuc_mcontextor(b)the-*TMbitwasdisabledbecauseasufficientnumberofcontextswitches-*happenedwhilstinthesignalhandlerandload_tmoverflowed,-*disablingtheTMbit.Ineithercasewecanendupwithanillegal-*TMstateleadingtoaTMBadThingwhenwereturntouserspace.-*/-regs->msr|=MSR_TM;-/* pull in MSR LE from user context */regs->msr=(regs->msr&~MSR_LE)|(msr&MSR_LE);
@@ -572,6 +558,34 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,tm_enable();/* Make sure the transaction is marked as failed */tsk->thread.tm_texasr|=TEXASR_FS;++/*+*Disablingpreemption,sinceitisunsafetobepreempted+*withMSR[TS]setwithoutrecheckpointing.+*/+preempt_disable();++/* pull in MSR TS bits from user context */+regs->msr=(regs->msr&~MSR_TS_MASK)|(msr&MSR_TS_MASK);++/*+*EnsurethatTMisenabledinregs->msrbeforeweleavethesignal+*handler.Itcouldbethecasethat(a)userdisabledtheTMbit+*throughthemanipulationoftheMSRbitsinuc_mcontextor(b)the+*TMbitwasdisabledbecauseasufficientnumberofcontextswitches+*happenedwhilstinthesignalhandlerandload_tmoverflowed,+*disablingtheTMbit.Ineithercasewecanendupwithanillegal+*TMstateleadingtoaTMBadThingwhenwereturntouserspace.+*+*CAUTION:+*Afterregs->MSR[TS]beingupdated,makesurethatget_user(),+*put_user()orsimilarfunctionsare*not*called.These+*functionscangeneratepagefaultswhichwillcausetheprocess+*tobede-scheduledwithMSR[TS]setbutwithoutcalling+*tm_recheckpoint().Thiscancauseabug.+*/+regs->msr|=MSR_TM;+/* This loads the checkpointed FP/VEC state, if used */tm_recheckpoint(&tsk->thread);
@@ -585,6 +599,8 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,regs->msr|=MSR_VEC;}+preempt_enable();+returnerr;}#endif
A new self test that forces MSR[TS] to be set without calling any TM
instruction. This test also tries to cause a page fault at a signal
handler, exactly between MSR[TS] set and tm_recheckpoint(), forcing
thread->texasr to be rewritten with TEXASR[FS] = 0, which will cause a BUG
when tm_recheckpoint() is called.
This test is not deterministic since it is hard to guarantee that the page
access will cause a page fault. Tests have shown that the bug could be
exposed with few interactions in a buggy kernel. This test is configured to
loop 5000x, having a good chance to hit the kernel issue in just one run.
This self test takes less than two seconds to run.
This test uses set/getcontext because the kernel will recheckpoint
zeroed structures, causing the test to segfault, which is undesired because
the test needs to rerun, so, there is a signal handler for SIGSEGV which
will restart the test.
Signed-off-by: Breno Leitao <leitao@debian.org>
---
tools/testing/selftests/powerpc/tm/.gitignore | 1 +
tools/testing/selftests/powerpc/tm/Makefile | 3 +-
.../powerpc/tm/tm-signal-force-msr.c | 115 ++++++++++++++++++
3 files changed, 118 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/powerpc/tm/tm-signal-force-msr.c
@@ -0,0 +1,115 @@+// SPDX-License-Identifier: GPL-2.0+/*+*Copyright2018,BrenoLeitao,GustavoRomero,IBMCorp.+*/++#define _GNU_SOURCE+#include<stdio.h>+#include<stdlib.h>+#include<signal.h>+#include<string.h>+#include<ucontext.h>+#include<unistd.h>++#include"tm.h"+#include"utils.h"++#define __MASK(X) (1UL<<(X))+#define MSR_TS_S_LG 33 /* Trans Mem state: Suspended */+#define MSR_TM __MASK(MSR_TM_LG) /* Transactional Mem Available */+#define MSR_TS_S __MASK(MSR_TS_S_LG) /* Transaction Suspended */++#define COUNT_MAX 5000 /* Number of interactions */++/* Setting contexts because the test will crash and we want to recover */+ucontext_tinit_context,main_context;++staticintcount,first_time;++voidtrap_signal_handler(intsigno,siginfo_t*si,void*uc)+{+ucontext_t*ucp=uc;++/*+*Allocatingmemoryinasignalhandler,andneverfreeingiton+*purpose,forcingtheheapincrease,so,thememoryleakiswhat+*wewanthere.+*/+ucp->uc_link=malloc(sizeof(ucontext_t));+memcpy(&ucp->uc_link,&ucp->uc_mcontext,sizeof(ucp->uc_mcontext));++/* Forcing to enable MSR[TM] */+ucp->uc_mcontext.gp_regs[PT_MSR]|=MSR_TS_S;++/*+*Aforkinsideasignalhandlerseemstobemoreefficientthana+*fork()priortothesignalbeingraised.+*/+if(fork()==0){+/*+*Bothchildandparentwillreturn,but,childreturns+*withcountsetsoitwillexitinthenextsegfault.+*Parentwillcontinuetoloop.+*/+count=COUNT_MAX;+}++/*+*Ifthechangeabovedoesnothitthebug,itwillcausea+*segmentationfault,sincetheckstructuresareNULL.+*/+}++voidseg_signal_handler(intsigno,siginfo_t*si,void*uc)+{+if(count==COUNT_MAX){+/* Return to tm_signal_force_msr() and exit */+setcontext(&main_context);+}++count++;+/* Reexecute the test */+setcontext(&init_context);+}++voidtm_trap_test(void)+{+structsigactiontrap_sa,seg_sa;++trap_sa.sa_flags=SA_SIGINFO;+trap_sa.sa_sigaction=trap_signal_handler;++seg_sa.sa_flags=SA_SIGINFO;+seg_sa.sa_sigaction=seg_signal_handler;++/*+*Setinitialcontext.Willgetbackherefrom+*seg_signal_handler()+*/+getcontext(&init_context);++/* The signal handler will enable MSR_TS */+sigaction(SIGUSR1,&trap_sa,NULL);+/* If it does not crash, it will segfault, avoid it to retest */+sigaction(SIGSEGV,&seg_sa,NULL);++raise(SIGUSR1);+}++inttm_signal_force_msr(void)+{+SKIP_IF(!have_htm());++/* Will get back here after COUNT_MAX interactions */+getcontext(&main_context);++if(!first_time++)+tm_trap_test();++returnEXIT_SUCCESS;+}++intmain(intargc,char**argv)+{+test_harness(tm_signal_force_msr,"tm_signal_force_msr");+}
From: Michael Neuling <hidden> Date: 2018-11-29 02:16:42
On Wed, 2018-11-28 at 11:23 -0200, Breno Leitao wrote:
A new self test that forces MSR[TS] to be set without calling any TM
instruction. This test also tries to cause a page fault at a signal
handler, exactly between MSR[TS] set and tm_recheckpoint(), forcing
thread->texasr to be rewritten with TEXASR[FS] = 0, which will cause a BUG
when tm_recheckpoint() is called.
This test is not deterministic since it is hard to guarantee that the page
access will cause a page fault. Tests have shown that the bug could be
exposed with few interactions in a buggy kernel. This test is configured to
loop 5000x, having a good chance to hit the kernel issue in just one run.
This self test takes less than two seconds to run.
You could try using sigaltstack() to put the ucontext somewhere else. Then you
could play tricks with that memory to try to force a fault.
madvise()+MADV_DONTNEED or fadvise()+POSIX_FADV_DONTNEED might do the trick.
This is more extra credit to make it more reliable. Not a requirement.
This test uses set/getcontext because the kernel will recheckpoint
zeroed structures, causing the test to segfault, which is undesired because
the test needs to rerun, so, there is a signal handler for SIGSEGV which
will restart the test.
Please put this description at the top of the test also.
Other than that, it looks good.
Mikey
@@ -0,0 +1,115 @@+// SPDX-License-Identifier: GPL-2.0+/*+*Copyright2018,BrenoLeitao,GustavoRomero,IBMCorp.+*/++#define _GNU_SOURCE+#include<stdio.h>+#include<stdlib.h>+#include<signal.h>+#include<string.h>+#include<ucontext.h>+#include<unistd.h>++#include"tm.h"+#include"utils.h"++#define __MASK(X) (1UL<<(X))+#define MSR_TS_S_LG 33 /* Trans Mem state: Suspended */+#define MSR_TM __MASK(MSR_TM_LG) /* Transactional Mem Available */+#define MSR_TS_S __MASK(MSR_TS_S_LG) /* Transaction Suspended */
Surely we have these defined somewhere else in selftests?
+
+#define COUNT_MAX 5000 /* Number of interactions */
+
+/* Setting contexts because the test will crash and we want to recover */
+ucontext_t init_context, main_context;
+
+static int count, first_time;
+
+void trap_signal_handler(int signo, siginfo_t *si, void *uc)
+{
+ ucontext_t *ucp = uc;
+
+ /*
+ * Allocating memory in a signal handler, and never freeing it on
+ * purpose, forcing the heap increase, so, the memory leak is what
+ * we want here.
+ */
+ ucp->uc_link = malloc(sizeof(ucontext_t));
+ memcpy(&ucp->uc_link, &ucp->uc_mcontext, sizeof(ucp->uc_mcontext));
+
+ /* Forcing to enable MSR[TM] */
+ ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
+
+ /*
+ * A fork inside a signal handler seems to be more efficient than a
+ * fork() prior to the signal being raised.
+ */
+ if (fork() == 0) {
+ /*
+ * Both child and parent will return, but, child returns
+ * with count set so it will exit in the next segfault.
+ * Parent will continue to loop.
+ */
+ count = COUNT_MAX;
+ }
+
+ /*
+ * If the change above does not hit the bug, it will cause a
+ * segmentation fault, since the ck structures are NULL.
+ */
+}
+
+void seg_signal_handler(int signo, siginfo_t *si, void *uc)
+{
+ if (count == COUNT_MAX) {
+ /* Return to tm_signal_force_msr() and exit */
+ setcontext(&main_context);
+ }
+
+ count++;
+ /* Reexecute the test */
+ setcontext(&init_context);
+}
+
+void tm_trap_test(void)
+{
+ struct sigaction trap_sa, seg_sa;
+
+ trap_sa.sa_flags = SA_SIGINFO;
+ trap_sa.sa_sigaction = trap_signal_handler;
+
+ seg_sa.sa_flags = SA_SIGINFO;
+ seg_sa.sa_sigaction = seg_signal_handler;
+
+ /*
+ * Set initial context. Will get back here from
+ * seg_signal_handler()
+ */
+ getcontext(&init_context);
+
+ /* The signal handler will enable MSR_TS */
+ sigaction(SIGUSR1, &trap_sa, NULL);
+ /* If it does not crash, it will segfault, avoid it to retest */
+ sigaction(SIGSEGV, &seg_sa, NULL);
+
+ raise(SIGUSR1);
+}
+
+int tm_signal_force_msr(void)
+{
+ SKIP_IF(!have_htm());
+
+ /* Will get back here after COUNT_MAX interactions */
+ getcontext(&main_context);
+
+ if (!first_time++)
+ tm_trap_test();
+
+ return EXIT_SUCCESS;
+}
+
+int main(int argc, char **argv)
+{
+ test_harness(tm_signal_force_msr, "tm_signal_force_msr");
+}
Hi Mikey,
On 11/29/18 12:11 AM, Michael Neuling wrote:
On Wed, 2018-11-28 at 11:23 -0200, Breno Leitao wrote:
quoted
A new self test that forces MSR[TS] to be set without calling any TM
instruction. This test also tries to cause a page fault at a signal
handler, exactly between MSR[TS] set and tm_recheckpoint(), forcing
thread->texasr to be rewritten with TEXASR[FS] = 0, which will cause a BUG
when tm_recheckpoint() is called.
This test is not deterministic since it is hard to guarantee that the page
access will cause a page fault. Tests have shown that the bug could be
exposed with few interactions in a buggy kernel. This test is configured to
loop 5000x, having a good chance to hit the kernel issue in just one run.
This self test takes less than two seconds to run.
You could try using sigaltstack() to put the ucontext somewhere else. Then you
could play tricks with that memory to try to force a fault.
madvise()+MADV_DONTNEED or fadvise()+POSIX_FADV_DONTNEED might do the trick.
Yes, it sounded interesting and I implemented the test using madvice(). Thanks
for the suggestion!
The current approach didn't seem to improve the amount of page faults at it
seems that MADV_DONTNEED makes no difference when using a Lazy page loading.
This is the test I did, where 'original' is my current patch and 'madvice` is
the patch below:
Performance counter stats for './original':
0 major-faults
125,100 minor-faults
2.575479619 seconds time elapsed
Performance counter stats for './madvice':
0 major-faults
125,099 minor-faults
Other than that, I didn't see any improvements in the reproduction rate also, although
it is a bit challenging to measure, since it crashes the machine and I can't run a
full statistical model.
This is the current patch I compared to the original one
---
commit 082a9fe29412943adfa2d6a363f44bac8e81d0ce
Author: Breno Leitao [off-list ref]
Date: Tue Nov 13 18:02:57 2018 -0500
selftests/powerpc: New TM signal self test
A new self test that forces MSR[TS] to be set without calling any TM
instruction. This test also tries to cause a page fault at a signal
handler, exactly between MSR[TS] set and tm_recheckpoint(), forcing
thread->texasr to be rewritten with TEXASR[FS] = 0, which will cause a BUG
when tm_recheckpoint() is called.
This test is not deterministic, since it is hard to guarantee that the page
access will cause a page fault. In order to force more page faults at
signal context, the signal handler and the ucontext are being mapped into a
MADV_DONTNEED memory chunks.
Tests have shown that the bug could be exposed with few interactions in a
buggy kernel. This test is configured to loop 5000x, having a good chance
to hit the kernel issue in just one run. This self test takes less than
two seconds to run.
This test uses set/getcontext because the kernel will recheckpoint
zeroed structures, causing the test to segfault, which is undesired because
the test needs to rerun, so, there is a signal handler for SIGSEGV which
will restart the test.
Signed-off-by: Breno Leitao [off-list ref]
@@ -0,0 +1,164 @@+// SPDX-License-Identifier: GPL-2.0+/*+*Copyright2018,BrenoLeitao,GustavoRomero,IBMCorp.+*+*ThistestraisesaSIGUSR1signal,andtoggletheMSR[TS]+*fieldsatthesignalhandler.WithMSR[TS]beingset,thekernelwill+*forcearecheckpoint,whichmaycauseasegfaultwhenreturningto+*userspace.Sincethekernelneedstore-run,thesegfaultneedstobe+*caughtandhandled.+*+*Inordertocontinuethetestevenafterasegfault,thecontextis+*savedpriortothesignalbeingraised,anditisrestoredwhenthereis+*asegmentationfault.ThishappensforCOUNT_MAXtimes.+*/++#define _GNU_SOURCE+#include<stdio.h>+#include<stdlib.h>+#include<signal.h>+#include<string.h>+#include<ucontext.h>+#include<unistd.h>+#include<sys/mman.h>++#include"tm.h"+#include"utils.h"+#include"reg.h"++#define COUNT_MAX 5000 /* Number of interactions */++/* Setting contexts because the test will crash and we want to recover */+ucontext_tinit_context,main_context;++staticintcount,first_time;++voidusr_signal_handler(intsigno,siginfo_t*si,void*uc)+{+ucontext_t*ucp=uc;+intret;++/*+*Allocatingmemoryinasignalhandler,andneverfreeingiton+*purpose,forcingtheheapincrease,so,thememoryleakiswhat+*wewanthere.+*/+ucp->uc_link=mmap(NULL,sizeof(ucontext_t),+PROT_READ|PROT_WRITE,+MAP_PRIVATE|MAP_ANONYMOUS,0,0);+if(ucp->uc_link==(void*)-1){+perror("Mmap failed");+exit(-1);+}++/* Forcing the page to be allocated in a page fault */+ret=madvise(ucp->uc_link,sizeof(ucontext_t),MADV_DONTNEED);+if(ret){+perror("madvise failed");+exit(-1);+}++memcpy(&ucp->uc_link,&ucp->uc_mcontext,sizeof(ucp->uc_mcontext));++/* Forcing to enable MSR[TM] */+ucp->uc_mcontext.gp_regs[PT_MSR]|=MSR_TS_S;++/*+*Aforkinsideasignalhandlerseemstobemoreefficientthana+*fork()priortothesignalbeingraised.+*/+if(fork()==0){+/*+*Bothchildandparentwillreturn,but,childreturns+*withcountsetsoitwillexitinthenextsegfault.+*Parentwillcontinuetoloop.+*/+count=COUNT_MAX;+}++/*+*Ifthechangeabovedoesnothitthebug,itwillcausea+*segmentationfault,sincetheckstructuresareNULL.+*/+}++voidseg_signal_handler(intsigno,siginfo_t*si,void*uc)+{+if(count==COUNT_MAX){+/* Return to tm_signal_force_msr() and exit */+setcontext(&main_context);+}++count++;++/* Reexecute the test */+setcontext(&init_context);+}++voidtm_trap_test(void)+{+structsigactionusr_sa,seg_sa;+stack_tss;++usr_sa.sa_flags=SA_SIGINFO|SA_ONSTACK;+usr_sa.sa_sigaction=usr_signal_handler;++seg_sa.sa_flags=SA_SIGINFO;+seg_sa.sa_sigaction=seg_signal_handler;++/*+*Setinitialcontext.Willgetbackherefrom+*seg_signal_handler()+*/+getcontext(&init_context);++/* Allocated am alternative signal stack area */+ss.ss_sp=mmap(NULL,SIGSTKSZ,PROT_READ|PROT_WRITE,+MAP_PRIVATE|MAP_ANONYMOUS,0,0);+ss.ss_size=SIGSTKSZ;+ss.ss_flags=0;++if(ss.ss_sp==(void*)-1){+perror("mmap error\n");+exit(-1);+}++/* Force the allocation through a page fault */+if(madvise(ss.ss_sp,SIGSTKSZ,MADV_DONTNEED)){+perror("madvise\n");+exit(-1);+}++/* Setting a alternative stack to generate a page fault when+*thesignalisraised.+*/+if(sigaltstack(&ss,NULL)){+perror("sigaltstack\n");+exit(-1);+}++/* The signal handler will enable MSR_TS */+sigaction(SIGUSR1,&usr_sa,NULL);+/* If it does not crash, it will segfault, avoid it to retest */+sigaction(SIGSEGV,&seg_sa,NULL);++raise(SIGUSR1);+}++inttm_signal_force_msr(void)+{+SKIP_IF(!have_htm());++/* Will get back here after COUNT_MAX interactions */+getcontext(&main_context);++if(!first_time++)+tm_trap_test();++returnEXIT_SUCCESS;+}++intmain(intargc,char**argv)+{+test_harness(tm_signal_force_msr,"tm_signal_force_msr");+}
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2018-12-20 12:54:21
Breno Leitao [off-list ref] writes:
A new self test that forces MSR[TS] to be set without calling any TM
instruction. This test also tries to cause a page fault at a signal
handler, exactly between MSR[TS] set and tm_recheckpoint(), forcing
thread->texasr to be rewritten with TEXASR[FS] = 0, which will cause a BUG
when tm_recheckpoint() is called.
This test is not deterministic since it is hard to guarantee that the page
access will cause a page fault. Tests have shown that the bug could be
exposed with few interactions in a buggy kernel. This test is configured to
loop 5000x, having a good chance to hit the kernel issue in just one run.
This self test takes less than two seconds to run.
This test uses set/getcontext because the kernel will recheckpoint
zeroed structures, causing the test to segfault, which is undesired because
the test needs to rerun, so, there is a signal handler for SIGSEGV which
will restart the test.
Hi Breno,
Thanks for the test, some of these TM tests are getting pretty advanced! :)
Unfortunately it doesn't build in a few configurations.
On Ubuntu 18.10 built with powerpc-linux-gnu-gcc I get:
tm-signal-force-msr.c: In function 'trap_signal_handler':
tm-signal-force-msr.c:42:19: error: 'union uc_regs_ptr' has no member named 'gp_regs'; did you mean 'uc_regs'?
ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
^~~~~~~
uc_regs
tm-signal-force-msr.c:17:29: error: left shift count >= width of type [-Werror=shift-count-overflow]
#define __MASK(X) (1UL<<(X))
^~
tm-signal-force-msr.c:20:25: note: in expansion of macro '__MASK'
#define MSR_TS_S __MASK(MSR_TS_S_LG) /* Transaction Suspended */
^~~~~~
tm-signal-force-msr.c:42:38: note: in expansion of macro 'MSR_TS_S'
ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
^~~~~~~~
And using powerpc64le-linux-gnu-gcc I get:
In file included from /usr/powerpc64le-linux-gnu/include/string.h:494,
from tm-signal-force-msr.c:10:
In function 'memcpy',
inlined from 'trap_signal_handler' at tm-signal-force-msr.c:39:2:
/usr/powerpc64le-linux-gnu/include/bits/string_fortified.h:34:10: error: '__builtin_memcpy' accessing 1272 bytes at offsets 8 and 168 overlaps 1112 bytes at offset 168 [-Werror=restrict]
return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cheers
From: Michael Ellerman <hidden> Date: 2018-12-23 13:34:57
On Wed, 2018-11-21 at 19:21:09 UTC, Breno Leitao wrote:
On a signal handler return, the user could set a context with MSR[TS] bits
set, and these bits would be copied to task regs->msr.
At restore_tm_sigcontexts(), after current task regs->msr[TS] bits are set,
several __get_user() are called and then a recheckpoint is executed.
This is a problem since a page fault (in kernel space) could happen when
calling __get_user(). If it happens, the process MSR[TS] bits were
already set, but recheckpoint was not executed, and SPRs are still invalid.
The page fault can cause the current process to be de-scheduled, with
MSR[TS] active and without tm_recheckpoint() being called. More
importantly, without TEXASR[FS] bit set also.
Since TEXASR might not have the FS bit set, and when the process is
scheduled back, it will try to reclaim, which will be aborted because of
the CPU is not in the suspended state, and, then, recheckpoint. This
recheckpoint will restore thread->texasr into TEXASR SPR, which might be
zero, hitting a BUG_ON().
kernel BUG at /build/linux-sf3Co9/linux-4.9.30/arch/powerpc/kernel/tm.S:434!
cpu 0xb: Vector: 700 (Program Check) at [c00000041f1576d0]
pc: c000000000054550: restore_gprs+0xb0/0x180
lr: 0000000000000000
sp: c00000041f157950
msr: 8000000100021033
current = 0xc00000041f143000
paca = 0xc00000000fb86300 softe: 0 irq_happened: 0x01
pid = 1021, comm = kworker/11:1
kernel BUG at /build/linux-sf3Co9/linux-4.9.30/arch/powerpc/kernel/tm.S:434!
Linux version 4.9.0-3-powerpc64le (debian-kernel@lists.debian.org) (gcc version 6.3.0 20170516 (Debian 6.3.0-18) ) #1 SMP Debian 4.9.30-2+deb9u2 (2017-06-26)
enter ? for help
[c00000041f157b30] c00000000001bc3c tm_recheckpoint.part.11+0x6c/0xa0
[c00000041f157b70] c00000000001d184 __switch_to+0x1e4/0x4c0
[c00000041f157bd0] c00000000082eeb8 __schedule+0x2f8/0x990
[c00000041f157cb0] c00000000082f598 schedule+0x48/0xc0
[c00000041f157ce0] c0000000000f0d28 worker_thread+0x148/0x610
[c00000041f157d80] c0000000000f96b0 kthread+0x120/0x140
[c00000041f157e30] c00000000000c0e0 ret_from_kernel_thread+0x5c/0x7c
This patch simply delays the MSR[TS] set, so, if there is any page fault in
the __get_user() section, it does not have regs->msr[TS] set, since the TM
structures are still invalid, thus avoiding doing TM operations for
in-kernel exceptions and possible process reschedule.
With this patch, the MSR[TS] will only be set just before recheckpointing
and setting TEXASR[FS] = 1, thus avoiding an interrupt with TM registers in
invalid state.
Other than that, if CONFIG_PREEMPT is set, there might be a preemption just
after setting MSR[TS] and before tm_recheckpoint(), thus, this block must
be atomic from a preemption perspective, thus, calling
preempt_disable/enable() on this code.
It is not possible to move tm_recheckpoint to happen earlier, because it is
required to get the checkpointed registers from userspace, with
__get_user(), thus, the only way to avoid this undesired behavior is
delaying the MSR[TS] set.
The 32-bits signal handler seems to be safe this current issue, but, it
might be exposed to the preemption issue, thus, disabling preemption in
this chunk of code.
Changes from v2:
* Run the critical section with preempt_disable.
Fixes: 87b4e5393af7 ("powerpc/tm: Fix return of active 64bit signals")
Cc: stable@vger.kernel.org (v3.9+)
Signed-off-by: Breno Leitao <leitao@debian.org>
Hi Michael,
On 12/20/18 10:51 AM, Michael Ellerman wrote:
Breno Leitao [off-list ref] writes:
quoted
A new self test that forces MSR[TS] to be set without calling any TM
instruction. This test also tries to cause a page fault at a signal
handler, exactly between MSR[TS] set and tm_recheckpoint(), forcing
thread->texasr to be rewritten with TEXASR[FS] = 0, which will cause a BUG
when tm_recheckpoint() is called.
This test is not deterministic since it is hard to guarantee that the page
access will cause a page fault. Tests have shown that the bug could be
exposed with few interactions in a buggy kernel. This test is configured to
loop 5000x, having a good chance to hit the kernel issue in just one run.
This self test takes less than two seconds to run.
This test uses set/getcontext because the kernel will recheckpoint
zeroed structures, causing the test to segfault, which is undesired because
the test needs to rerun, so, there is a signal handler for SIGSEGV which
will restart the test.
And reference the ucontext->mcontext MSR using UCONTEXT_MSR() macro.
Hi Breno,
Thanks for the test, some of these TM tests are getting pretty advanced! :)
Unfortunately it doesn't build in a few configurations.
On Ubuntu 18.10 built with powerpc-linux-gnu-gcc I get:
tm-signal-force-msr.c: In function 'trap_signal_handler':
tm-signal-force-msr.c:42:19: error: 'union uc_regs_ptr' has no member named 'gp_regs'; did you mean 'uc_regs'?
ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
^~~~~~~
uc_regs
tm-signal-force-msr.c:17:29: error: left shift count >= width of type [-Werror=shift-count-overflow]
#define __MASK(X) (1UL<<(X))
^~
tm-signal-force-msr.c:20:25: note: in expansion of macro '__MASK'
#define MSR_TS_S __MASK(MSR_TS_S_LG) /* Transaction Suspended */
^~~~~~
tm-signal-force-msr.c:42:38: note: in expansion of macro 'MSR_TS_S'
ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
^~~~~~~~
That is because I missed the -m64 compilation flag on Makefile. I understand
that this test only make sense when compiled in 64 bits. Do you agree?
I might also add a macro to address ucontext->mcontext MSR. This will avoid
problems like that in the future.
index ae43a614835d..7636bf45d5d5 100644
--- a/tools/testing/selftests/powerpc/include/utils.h
+++ b/tools/testing/selftests/powerpc/include/utils.h
@@ -102,8 +102,10 @@ do {
#if defined(__powerpc64__)
#define UCONTEXT_NIA(UC) (UC)->uc_mcontext.gp_regs[PT_NIP]
+#define UCONTEXT_MSR(UC) (UC)->uc_mcontext.gp_regs[PT_MSR]
#elif defined(__powerpc__)
#define UCONTEXT_NIA(UC) (UC)->uc_mcontext.uc_regs->gregs[PT_NIP]
+#define UCONTEXT_MSR(UC) (UC)->uc_mcontext.uc_regs->gregs[PT_MSR]
#else
#error implement UCONTEXT_NIA
#endif
And using powerpc64le-linux-gnu-gcc I get:
In file included from /usr/powerpc64le-linux-gnu/include/string.h:494,
from tm-signal-force-msr.c:10:
In function 'memcpy',
inlined from 'trap_signal_handler' at tm-signal-force-msr.c:39:2:
/usr/powerpc64le-linux-gnu/include/bits/string_fortified.h:34:10: error: '__builtin_memcpy' accessing 1272 bytes at offsets 8 and 168 overlaps 1112 bytes at offset 168 [-Werror=restrict]
return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Damn, that is because I do not know how to use C pointers. Fixing it on v3 also.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2019-01-08 10:17:56
Breno Leitao [off-list ref] writes:
On 12/20/18 10:51 AM, Michael Ellerman wrote:
quoted
Breno Leitao [off-list ref] writes:
quoted
A new self test that forces MSR[TS] to be set without calling any TM
instruction. This test also tries to cause a page fault at a signal
handler, exactly between MSR[TS] set and tm_recheckpoint(), forcing
thread->texasr to be rewritten with TEXASR[FS] = 0, which will cause a BUG
when tm_recheckpoint() is called.
This test is not deterministic since it is hard to guarantee that the page
access will cause a page fault. Tests have shown that the bug could be
exposed with few interactions in a buggy kernel. This test is configured to
loop 5000x, having a good chance to hit the kernel issue in just one run.
This self test takes less than two seconds to run.
This test uses set/getcontext because the kernel will recheckpoint
zeroed structures, causing the test to segfault, which is undesired because
the test needs to rerun, so, there is a signal handler for SIGSEGV which
will restart the test.
And reference the ucontext->mcontext MSR using UCONTEXT_MSR() macro.
Hi Breno,
Thanks for the test, some of these TM tests are getting pretty advanced! :)
Unfortunately it doesn't build in a few configurations.
On Ubuntu 18.10 built with powerpc-linux-gnu-gcc I get:
tm-signal-force-msr.c: In function 'trap_signal_handler':
tm-signal-force-msr.c:42:19: error: 'union uc_regs_ptr' has no member named 'gp_regs'; did you mean 'uc_regs'?
ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
^~~~~~~
uc_regs
tm-signal-force-msr.c:17:29: error: left shift count >= width of type [-Werror=shift-count-overflow]
#define __MASK(X) (1UL<<(X))
^~
tm-signal-force-msr.c:20:25: note: in expansion of macro '__MASK'
#define MSR_TS_S __MASK(MSR_TS_S_LG) /* Transaction Suspended */
^~~~~~
tm-signal-force-msr.c:42:38: note: in expansion of macro 'MSR_TS_S'
ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
^~~~~~~~
That is because I missed the -m64 compilation flag on Makefile. I understand
that this test only make sense when compiled in 64 bits. Do you agree?
I think the test could work as a 32-bit binary on a 64-bit kernel, but I
don't mind if you force it to build 64-bit.
cheers