[PATCH 1/5] powerpc: Print MSR TM bits in oops message

[PATCH 1/5] powerpc: Print MSR TM bits in oops message

[Michael Neuling]

Print the MSR TM bits in oops messages.  This appends them to the end
like this:
 MSR: 8000000502823031 <SF,VEC,VSX,FP,ME,IR,DR,LE,TM[TE]>

You get the TM[] only if at least one TM MSR bit is set.  Inside the
TM[], E means Enabled (bit 32), S means Suspended (bit 33), and T
means Transactional (bit 34)

If no bits are set, you get no TM[] output.

Include rework of printbits() to handle this case.

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 arch/powerpc/kernel/process.c | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 75b6676..5fbe5d8 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -934,10 +934,12 @@ static void show_instructions(struct pt_regs *regs)
 	printk("\n");
 }
 
-static struct regbit {
+struct regbit {
 	unsigned long bit;
 	const char *name;
-} msr_bits[] = {
+};
+
+static struct regbit msr_bits[] = {
 #if defined(CONFIG_PPC64) && !defined(CONFIG_BOOKE)
 	{MSR_SF,	"SF"},
 	{MSR_HV,	"HV"},
@@ -967,16 +969,41 @@ static struct regbit {
 	{0,		NULL}
 };
 
-static void printbits(unsigned long val, struct regbit *bits)
+static void printbits(unsigned long val, struct regbit *bits, const char *sep)
 {
-	const char *sep = "";
+	const char *s = "";
 
-	printk("<");
 	for (; bits->bit; ++bits)
 		if (val & bits->bit) {
-			printk("%s%s", sep, bits->name);
-			sep = ",";
+			printk("%s%s", s, bits->name);
+			s = sep;
 		}
+}
+
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+static struct regbit msr_tm_bits[] = {
+	{MSR_TS_T,	"T"},
+	{MSR_TS_S,	"S"},
+	{MSR_TM,	"E"},
+	{0,		NULL}
+};
+static void printtmbits(unsigned long val)
+{
+	if (val & (MSR_TM | MSR_TS_S | MSR_TS_T)) {
+		printk(",TM[");
+		printbits(val, msr_tm_bits, "");
+		printk("]");
+	}
+}
+#else
+static void printtmbits(unsigned long val) {}
+#endif
+
+static void printmsrbits(unsigned long val)
+{
+	printk("<");
+	printbits(val, msr_bits, ",");
+	printtmbits(val);
 	printk(">");
 }
 
@@ -1001,7 +1028,7 @@ void show_regs(struct pt_regs * regs)
 	printk("REGS: %p TRAP: %04lx   %s  (%s)\n",
 	       regs, regs->trap, print_tainted(), init_utsname()->release);
 	printk("MSR: "REG" ", regs->msr);
-	printbits(regs->msr, msr_bits);
+	printmsrbits(regs->msr);
 	printk("  CR: %08lx  XER: %08lx\n", regs->ccr, regs->xer);
 	trap = TRAP(regs);
 	if ((regs->trap != 0xc00) && cpu_has_feature(CPU_FTR_CFAR))
-- 
2.5.0

[PATCH 2/5] selftests/powerpc: Add TM signal return selftest

[Michael Neuling]

Test the kernel's signal return code to ensure that it doesn't crash
when both the transactional and suspend MSR bits are set in the signal
context.

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 tools/testing/selftests/powerpc/tm/Makefile        |  2 +-
 .../selftests/powerpc/tm/tm-signal-msr-resv.c      | 64 ++++++++++++++++++++++
 2 files changed, 65 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c

diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile
index 4bea62a..0c28db7 100644
--- a/tools/testing/selftests/powerpc/tm/Makefile
+++ b/tools/testing/selftests/powerpc/tm/Makefile
@@ -1,4 +1,4 @@
-TEST_PROGS := tm-resched-dscr tm-syscall
+TEST_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv
 
 all: $(TEST_PROGS)
 
diff --git a/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c b/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c
new file mode 100644
index 0000000..14705ae
--- /dev/null
+++ b/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c
@@ -0,0 +1,64 @@
+/*
+ * Copyright 2015, Michael Neuling, IBM Corp.
+ * Licensed under GPLv2.
+ *
+ * Test the kernel's signal return code to ensure that it doesn't
+ * crash when both the transactional and suspend MSR bits are set in
+ * the signal context.
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <signal.h>
+
+#include "utils.h"
+
+struct sigaction act;
+
+void signal_segv(int signum, siginfo_t *info, void *uc)
+{
+	printf("PASSED\n");
+	exit(0);
+}
+
+void signal_usr1(int signum, siginfo_t *info, void *uc)
+{
+	ucontext_t *ucp = uc;
+
+	/* link tm checkpointed context to normal context */
+	ucp->uc_link = ucp;
+	/* set all TM bits */
+#ifdef __powerpc64__
+	ucp->uc_mcontext.gp_regs[PT_MSR] |= (7ULL << 32);
+#else
+	ucp->uc_mcontext.regs->gpr[PT_MSR] |= (7ULL);
+#endif
+	/* Should segv on return becuase of invalid context */
+	act.sa_sigaction = signal_segv;
+	if (sigaction(SIGSEGV, &act, NULL) < 0) {
+		perror("sigaction sigsegv");
+		exit(1);
+	}
+}
+
+int tm_signal_msr_resv()
+{
+
+	act.sa_sigaction = signal_usr1;
+	sigemptyset(&act.sa_mask);
+	act.sa_flags = SA_SIGINFO;
+	if (sigaction(SIGUSR1, &act, NULL) < 0) {
+		perror("sigaction sigusr1");
+		exit(1);
+	}
+
+	raise(SIGUSR1);
+
+	printf("FAILED\n");
+	return 1;
+}
+
+int main(void)
+{
+	return test_harness(tm_signal_msr_resv, "tm_signal_msr_resv");
+}
-- 
2.5.0

[PATCH 3/5] powerpc/tm: Block signal return setting invalid MSR state

[Michael Neuling]

Currently we allow both the MSR T and S bits to be set by userspace on
a signal return.  Unfortunately this is a reserved configuration and
will cause a TM Bad Thing exception if attempted (via rfid).

This patch checks for this case in both the 32 and 64bit signals code.
If these are both set, we clear them and trigger the bad stack frame
handling.

Found using a syscall fuzzer.

Signed-off-by: Michael Neuling <mikey@neuling.org>
Cc: stable@vger.kernel.org
---
 arch/powerpc/include/asm/reg.h  |  1 +
 arch/powerpc/kernel/signal_32.c | 16 ++++++++++------
 arch/powerpc/kernel/signal_64.c |  6 ++++++
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index a908ada..2220f7a 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -108,6 +108,7 @@
 #define MSR_TS_T	__MASK(MSR_TS_T_LG)	/*  Transaction Transactional */
 #define MSR_TS_MASK	(MSR_TS_T | MSR_TS_S)   /* Transaction State bits */
 #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */
+#define MSR_TM_RESV(x) (((x) & MSR_TS_MASK) == MSR_TS_MASK) /* Reserved */
 #define MSR_TM_TRANSACTIONAL(x)	(((x) & MSR_TS_MASK) == MSR_TS_T)
 #define MSR_TM_SUSPENDED(x)	(((x) & MSR_TS_MASK) == MSR_TS_S)
 
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 0dbee46..5b519c7 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -874,7 +874,16 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 		       + ELF_NEVRREG))
 		return 1;
 #endif /* CONFIG_SPE */
-
+	/* Get the top half of the MSR */
+	if (__get_user(msr_hi, &tm_sr->mc_gregs[PT_MSR]))
+		return 1;
+	/* Pull in MSR TM from user context */
+	regs->msr = (regs->msr & ~MSR_TS_MASK) | ((msr_hi<<32) & MSR_TS_MASK);
+	/* Don't let the user set both TM bits */
+	if (MSR_TM_RESV(regs->msr)) {
+		regs->msr &= ~MSR_TS_MASK;
+		return 1;
+	}
 	/* Now, recheckpoint.  This loads up all of the checkpointed (older)
 	 * registers, including FP and V[S]Rs.  After recheckpointing, the
 	 * transactional versions should be loaded.
@@ -884,11 +893,6 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 	current->thread.tm_texasr |= TEXASR_FS;
 	/* This loads the checkpointed FP/VEC state, if used */
 	tm_recheckpoint(&current->thread, msr);
-	/* Get the top half of the MSR */
-	if (__get_user(msr_hi, &tm_sr->mc_gregs[PT_MSR]))
-		return 1;
-	/* Pull in MSR TM from user context */
-	regs->msr = (regs->msr & ~MSR_TS_MASK) | ((msr_hi<<32) & MSR_TS_MASK);
 
 	/* This loads the speculative FP/VEC state, if used */
 	if (msr & MSR_FP) {
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 20756df..b320689 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -438,6 +438,12 @@ static long restore_tm_sigcontexts(struct pt_regs *regs,
 
 	/* get MSR separately, transfer the LE bit if doing signal return */
 	err |= __get_user(msr, &sc->gp_regs[PT_MSR]);
+	/* Don't allow reserved mode. */
+	if (MSR_TM_RESV(msr)) {
+		msr &= ~MSR_TS_MASK;
+		return -EINVAL;
+	}
+
 	/* pull in MSR TM from user context */
 	regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr & MSR_TS_MASK);
 
-- 
2.5.0

[PATCH 4/5] powerpc/tm: Check for already reclaimed tasks

[Michael Neuling]

Currently we can hit a scenario where we'll tm_reclaim() twice.  This
results in a TM bad thing exception because the second reclaim occurs
when not in suspend mode.

The scenario in which this can happen is the following.  We attempt to
deliver a signal to userspace.  To do this we need obtain the stack
pointer to write the signal context.  To get this stack pointer we
must tm_reclaim() in case we need to use the checkpointed stack
pointer (see get_tm_stackpointer()).  Normally we'd then return
directly to userspace to deliver the signal without going through
__switch_to().

Unfortunatley, if at this point we get an error (such as a bad
userspace stack pointer), we need to exit the process.  The exit will
result in a __switch_to().  __switch_to() will attempt to save the
process state which results in another tm_reclaim().  This
tm_reclaim() now causes a TM Bad Thing exception as this state has
already been saved and the processor is no longer in TM suspend mode.
Whee!

This patch checks the state of the MSR to ensure we are TM suspended
before we attempt the tm_reclaim().  If we've already saved the state
away, we should no longer be in TM suspend mode.  This has the
additional advantage of checking for a potential TM Bad Thing
exception.

Found using syscall fuzzer.

Signed-off-by: Michael Neuling <mikey@neuling.org>
Cc: stable@vger.kernel.org
---
 arch/powerpc/kernel/process.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 5fbe5d8..a1b41d1 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -551,6 +551,25 @@ static void tm_reclaim_thread(struct thread_struct *thr,
 		msr_diff &= MSR_FP | MSR_VEC | MSR_VSX | MSR_FE0 | MSR_FE1;
 	}
 
+	/*
+	 * Use the current MSR TM suspended bit to track if we have
+	 * checkpointed state outstanding.
+	 * On signal delivery, we'd normally reclaim the checkpointed
+	 * state to obtain stack pointer (see:get_tm_stackpointer()).
+	 * This will then directly return to userspace without going
+	 * through __switch_to(). However, if the stack frame is bad,
+	 * we need to exit this thread which calls __switch_to() which
+	 * will again attempt to reclaim the already saved tm state.
+	 * Hence we need to check that we've not already reclaimed
+	 * this state.
+	 * We do this using the current MSR, rather tracking it in
+	 * some specific bit thread_struct bit, as it has the
+	 * additional benifit of checking for a potential TM bad thing
+	 * exception.
+	 */
+	if (!MSR_TM_SUSPENDED(mfmsr()))
+		return;
+
 	tm_reclaim(thr, thr->regs->msr, cause);
 
 	/* Having done the reclaim, we now have the checkpointed
-- 
2.5.0

[PATCH 5/5] powerpc/tm: Clarify get_tm_stackpointer() by renaming it

[Michael Neuling]

get_tm_stackpointer() gets the userspace stack pointer which the
signal frame will be written from pt_regs.  To do this it performs a
tm_reclaim to access the checkpointed stack pointer.

Doing a tm_reclaim is an important operation which changes the machine
state.  Doing this in the wrong state or an attempt to do it twice it can
result in a TM Bad Thing exception.  Hence we should make it clearer
that this function is making this change.

This patch renames this function to make it clearer what's happening
when calling this function.  No functional change.

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 arch/powerpc/include/asm/signal.h | 2 +-
 arch/powerpc/kernel/process.c     | 2 +-
 arch/powerpc/kernel/signal.c      | 2 +-
 arch/powerpc/kernel/signal_32.c   | 6 ++++--
 arch/powerpc/kernel/signal_64.c   | 3 ++-
 5 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/signal.h b/arch/powerpc/include/asm/signal.h
index 9322c28..0139dc2 100644
--- a/arch/powerpc/include/asm/signal.h
+++ b/arch/powerpc/include/asm/signal.h
@@ -5,6 +5,6 @@
 #include <uapi/asm/signal.h>
 #include <uapi/asm/ptrace.h>
 
-extern unsigned long get_tm_stackpointer(struct pt_regs *regs);
+extern unsigned long get_tm_stackpointer_and_reclaim(struct pt_regs *regs);
 
 #endif /* _ASM_POWERPC_SIGNAL_H */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index a1b41d1..264a39c 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -555,7 +555,7 @@ static void tm_reclaim_thread(struct thread_struct *thr,
 	 * Use the current MSR TM suspended bit to track if we have
 	 * checkpointed state outstanding.
 	 * On signal delivery, we'd normally reclaim the checkpointed
-	 * state to obtain stack pointer (see:get_tm_stackpointer()).
+	 * state to obtain stack pointer (see:get_tm_stackpointer_and_reclaim()).
 	 * This will then directly return to userspace without going
 	 * through __switch_to(). However, if the stack frame is bad,
 	 * we need to exit this thread which calls __switch_to() which
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index cf8c7e4..b3de865 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -162,7 +162,7 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
 	user_enter();
 }
 
-unsigned long get_tm_stackpointer(struct pt_regs *regs)
+unsigned long get_tm_stackpointer_and_reclaim(struct pt_regs *regs)
 {
 	/* When in an active transaction that takes a signal, we need to be
 	 * careful with the stack.  It's possible that the stack has moved back
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 5b519c7..b2c3f8c 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -1001,7 +1001,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 
 	/* Set up Signal Frame */
 	/* Put a Real Time Context onto stack */
-	rt_sf = get_sigframe(ksig, get_tm_stackpointer(regs), sizeof(*rt_sf), 1);
+	rt_sf = get_sigframe(ksig, get_tm_stackpointer_and_reclaim(regs),
+			     sizeof(*rt_sf), 1);
 	addr = rt_sf;
 	if (unlikely(rt_sf == NULL))
 		goto badframe;
@@ -1424,7 +1425,8 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset, struct pt_regs *regs
 	unsigned long tramp;
 
 	/* Set up Signal Frame */
-	frame = get_sigframe(ksig, get_tm_stackpointer(regs), sizeof(*frame), 1);
+	frame = get_sigframe(ksig, get_tm_stackpointer_and_reclaim(regs),
+			     sizeof(*frame), 1);
 	if (unlikely(frame == NULL))
 		goto badframe;
 	sc = (struct sigcontext __user *) &frame->sctx;
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index b320689..96264be 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -731,7 +731,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, struct pt_regs *regs
 	unsigned long newsp = 0;
 	long err = 0;
 
-	frame = get_sigframe(ksig, get_tm_stackpointer(regs), sizeof(*frame), 0);
+	frame = get_sigframe(ksig, get_tm_stackpointer_and_reclaim(regs),
+			     sizeof(*frame), 0);
 	if (unlikely(frame == NULL))
 		goto badframe;
 
-- 
2.5.0

Re: [PATCH 4/5] powerpc/tm: Check for already reclaimed tasks

[Anshuman Khandual]

On 11/13/2015 10:27 AM, Michael Neuling wrote:
> Currently we can hit a scenario where we'll tm_reclaim() twice.  This
> results in a TM bad thing exception because the second reclaim occurs
> when not in suspend mode.
> 
> The scenario in which this can happen is the following.  We attempt to
> deliver a signal to userspace.  To do this we need obtain the stack
> pointer to write the signal context.  To get this stack pointer we
> must tm_reclaim() in case we need to use the checkpointed stack
> pointer (see get_tm_stackpointer()).  Normally we'd then return
> directly to userspace to deliver the signal without going through
> __switch_to().
> 
> Unfortunatley, if at this point we get an error (such as a bad
> userspace stack pointer), we need to exit the process.  The exit will
> result in a __switch_to().  __switch_to() will attempt to save the
> process state which results in another tm_reclaim().  This
> tm_reclaim() now causes a TM Bad Thing exception as this state has
> already been saved and the processor is no longer in TM suspend mode.
> Whee!
> 
> This patch checks the state of the MSR to ensure we are TM suspended
> before we attempt the tm_reclaim().  If we've already saved the state
> away, we should no longer be in TM suspend mode.  This has the
> additional advantage of checking for a potential TM Bad Thing
> exception.

Can this situation be created using a test and verified that with
this new change, the kernel can handle it successfully. I guess
the self test in the series does not cover this scenario.

> 
> Found using syscall fuzzer.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> Cc: stable@vger.kernel.org
> ---
>  arch/powerpc/kernel/process.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 5fbe5d8..a1b41d1 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -551,6 +551,25 @@ static void tm_reclaim_thread(struct thread_struct *thr,
>  		msr_diff &= MSR_FP | MSR_VEC | MSR_VSX | MSR_FE0 | MSR_FE1;
>  	}
>  
> +	/*
> +	 * Use the current MSR TM suspended bit to track if we have
> +	 * checkpointed state outstanding.
> +	 * On signal delivery, we'd normally reclaim the checkpointed
> +	 * state to obtain stack pointer (see:get_tm_stackpointer()).
> +	 * This will then directly return to userspace without going
> +	 * through __switch_to(). However, if the stack frame is bad,
> +	 * we need to exit this thread which calls __switch_to() which
> +	 * will again attempt to reclaim the already saved tm state.
> +	 * Hence we need to check that we've not already reclaimed
> +	 * this state.
> +	 * We do this using the current MSR, rather tracking it in
> +	 * some specific bit thread_struct bit, as it has the

There is one extra "bit" here ^^^^^.

Re: [PATCH 1/5] powerpc: Print MSR TM bits in oops message

[Anshuman Khandual]

On 11/13/2015 10:27 AM, Michael Neuling wrote:
> Print the MSR TM bits in oops messages.  This appends them to the end
> like this:
>  MSR: 8000000502823031 <SF,VEC,VSX,FP,ME,IR,DR,LE,TM[TE]>
> 
> You get the TM[] only if at least one TM MSR bit is set.  Inside the
> TM[], E means Enabled (bit 32), S means Suspended (bit 33), and T
> means Transactional (bit 34)
> 
> If no bits are set, you get no TM[] output.
> 
> Include rework of printbits() to handle this case.
> 

Just a small nit, the above commit message can be formatted better.

Re: [PATCH 4/5] powerpc/tm: Check for already reclaimed tasks

[Michael Neuling]

On Mon, 2015-11-16 at 12:51 +0530, Anshuman Khandual wrote:
> On 11/13/2015 10:27 AM, Michael Neuling wrote:
> > Currently we can hit a scenario where we'll tm_reclaim() twice.=20
> >  This
> > results in a TM bad thing exception because the second reclaim
> > occurs
> > when not in suspend mode.
> >=20
> > The scenario in which this can happen is the following.  We attempt
> > to
> > deliver a signal to userspace.  To do this we need obtain the stack
> > pointer to write the signal context.  To get this stack pointer we
> > must tm_reclaim() in case we need to use the checkpointed stack
> > pointer (see get_tm_stackpointer()).  Normally we'd then return
> > directly to userspace to deliver the signal without going through
> > __switch_to().
> >=20
> > Unfortunatley, if at this point we get an error (such as a bad
> > userspace stack pointer), we need to exit the process.  The exit
> > will
> > result in a __switch_to().  __switch_to() will attempt to save the
> > process state which results in another tm_reclaim().  This
> > tm_reclaim() now causes a TM Bad Thing exception as this state has
> > already been saved and the processor is no longer in TM suspend
> > mode.
> > Whee!
> >=20
> > This patch checks the state of the MSR to ensure we are TM
> > suspended
> > before we attempt the tm_reclaim().  If we've already saved the
> > state
> > away, we should no longer be in TM suspend mode.  This has the
> > additional advantage of checking for a potential TM Bad Thing
> > exception.
>=20
> Can this situation be created using a test and verified that with
> this new change, the kernel can handle it successfully. I guess
> the self test in the series does not cover this scenario.

No it doesn't.  The syscall fuzzer I have does hit it but I don't have
permission to post that.

> >=20
> > Found using syscall fuzzer.
> >=20
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  arch/powerpc/kernel/process.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >=20
> > diff --git a/arch/powerpc/kernel/process.c
> > b/arch/powerpc/kernel/process.c
> > index 5fbe5d8..a1b41d1 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -551,6 +551,25 @@ static void tm_reclaim_thread(struct
> > thread_struct *thr,
> >  		msr_diff &=3D MSR_FP | MSR_VEC | MSR_VSX | MSR_FE0 |
> > MSR_FE1;
> >  	}
> > =20
> > +	/*
> > +	 * Use the current MSR TM suspended bit to track if we
> > have
> > +	 * checkpointed state outstanding.
> > +	 * On signal delivery, we'd normally reclaim the
> > checkpointed
> > +	 * state to obtain stack pointer
> > (see:get_tm_stackpointer()).
> > +	 * This will then directly return to userspace without
> > going
> > +	 * through __switch_to(). However, if the stack frame is
> > bad,
> > +	 * we need to exit this thread which calls __switch_to()
> > which
> > +	 * will again attempt to reclaim the already saved tm
> > state.
> > +	 * Hence we need to check that we've not already reclaimed
> > +	 * this state.
> > +	 * We do this using the current MSR, rather tracking it in
> > +	 * some specific bit thread_struct bit, as it has the
>=20
> There is one extra "bit" here ^^^^^.

Thanks!

Mikey
>=20

Re: [PATCH 1/5] powerpc: Print MSR TM bits in oops message

[Michael Ellerman]

On Fri, 2015-11-13 at 15:57 +1100, Michael Neuling wrote:

> Print the MSR TM bits in oops messages.  This appends them to the end
> like this:
>  MSR: 8000000502823031 <SF,VEC,VSX,FP,ME,IR,DR,LE,TM[TE]>
> 
> You get the TM[] only if at least one TM MSR bit is set.  Inside the
> TM[], E means Enabled (bit 32), S means Suspended (bit 33), and T
> means Transactional (bit 34)

Can you duplicate this into a comment in printtmbits() or on the bit
definitions, so that I don't have to look up the commit to find the
explanation.

> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +static struct regbit msr_tm_bits[] = {
> +	{MSR_TS_T,	"T"},
> +	{MSR_TS_S,	"S"},
> +	{MSR_TM,	"E"},
> +	{0,		NULL}
> +};
> +static void printtmbits(unsigned long val)

I realise you followed the lead here with the naming, but can you call it
print_tm_bits() please. MY EYES!

> +{
> +	if (val & (MSR_TM | MSR_TS_S | MSR_TS_T)) {
> +		printk(",TM[");
> +		printbits(val, msr_tm_bits, "");
> +		printk("]");

I suspect all these individual printks are going to behave badly if we have
multiple cpus crashing simultaneously. But I won't make you fix that here. We
should look at it sometime though.

cheers

Re: [PATCH 4/5] powerpc/tm: Check for already reclaimed tasks

[Michael Ellerman]

On Mon, 2015-11-16 at 20:23 +1100, Michael Neuling wrote:
> On Mon, 2015-11-16 at 12:51 +0530, Anshuman Khandual wrote:
> > On 11/13/2015 10:27 AM, Michael Neuling wrote:
> > > Currently we can hit a scenario where we'll tm_reclaim() twice. 
> > >  This
> > > results in a TM bad thing exception because the second reclaim
> > > occurs
> > > when not in suspend mode.
> > > 
> > > The scenario in which this can happen is the following.  We attempt
> > > to
> > > deliver a signal to userspace.  To do this we need obtain the stack
> > > pointer to write the signal context.  To get this stack pointer we
> > > must tm_reclaim() in case we need to use the checkpointed stack
> > > pointer (see get_tm_stackpointer()).  Normally we'd then return
> > > directly to userspace to deliver the signal without going through
> > > __switch_to().
> > > 
> > > Unfortunatley, if at this point we get an error (such as a bad
> > > userspace stack pointer), we need to exit the process.  The exit
> > > will
> > > result in a __switch_to().  __switch_to() will attempt to save the
> > > process state which results in another tm_reclaim().  This
> > > tm_reclaim() now causes a TM Bad Thing exception as this state has
> > > already been saved and the processor is no longer in TM suspend
> > > mode.
> > > Whee!
> > > 
> > > This patch checks the state of the MSR to ensure we are TM
> > > suspended
> > > before we attempt the tm_reclaim().  If we've already saved the
> > > state
> > > away, we should no longer be in TM suspend mode.  This has the
> > > additional advantage of checking for a potential TM Bad Thing
> > > exception.
> > 
> > Can this situation be created using a test and verified that with
> > this new change, the kernel can handle it successfully. I guess
> > the self test in the series does not cover this scenario.
> 
> No it doesn't.  The syscall fuzzer I have does hit it but I don't have
> permission to post that.

And we don't really want a fuzzer as a selftest, because it might call unlink
or something else bad.

But having found the bug with the fuzzer, can't you write a test that triggers
the bad case?

>From your description it sounds like if you had a child spinning with a bad r1,
and then a parent sent it a signal that would trip it?

cheers

Re: [PATCH 5/5] powerpc/tm: Clarify get_tm_stackpointer() by renaming it

[Michael Ellerman]

On Fri, 2015-11-13 at 15:57 +1100, Michael Neuling wrote:
> get_tm_stackpointer() gets the userspace stack pointer which the
> signal frame will be written from pt_regs.  To do this it performs a
> tm_reclaim to access the checkpointed stack pointer.
> 
> Doing a tm_reclaim is an important operation which changes the machine
> state.  Doing this in the wrong state or an attempt to do it twice it can
> result in a TM Bad Thing exception.  Hence we should make it clearer
> that this function is making this change.
> 
> This patch renames this function to make it clearer what's happening
> when calling this function.  No functional change.

Urgh. It's really not very pretty.

> @@ -1001,7 +1001,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>  
>  	/* Set up Signal Frame */
>  	/* Put a Real Time Context onto stack */
> -	rt_sf = get_sigframe(ksig, get_tm_stackpointer(regs), sizeof(*rt_sf), 1);
> +	rt_sf = get_sigframe(ksig, get_tm_stackpointer_and_reclaim(regs),
> +			     sizeof(*rt_sf), 1);
>  	addr = rt_sf;
>  	if (unlikely(rt_sf == NULL))
>  		goto badframe;

I think partly we just have the wrong abstraction here. We try to hide TM
inside get_tm_stackpointer(), but then we confused ourselves by hiding the
reclaim in there. And all three call sites then *also* have an #ifdef
CONFIG_PPC_TRANSACTIONAL_MEM block in the function, so we failed at hiding TM
anyway.


I wonder if we'd be better off relying more on the compiler to elide code
behind an always false boolean, something like:

int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, struct pt_regs *regs)
{
	struct rt_sigframe __user *frame;
	unsigned long sp, newsp = 0;
	bool tm_active;
	long err = 0;

	tm_active = msr_tm_active(regs->msr);
	if (tm_active)
		sp = tm_reclaim_sp(regs);
	else
		sp = regs->gpr[1];

	frame = get_sigframe(ksig, sp, sizeof(*frame), 0);
	if (unlikely(frame == NULL))
		goto badframe;

	...

-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-	if (MSR_TM_ACTIVE(regs->msr)) {
+	if (tm_active) {
		/* The ucontext_t passed to userland points to the second
		 * ucontext_t (for transactional state) with its uc_link ptr.
		 */
		err |= __put_user(&frame->uc_transact, &frame->uc.uc_link);
		err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext,


We'd need to move more of the __put_user() logic into a helper, because
uc_transact doesn't exist when TM is disabled.

cheers

Re: [PATCH 3/5] powerpc/tm: Block signal return setting invalid MSR state

[Michael Ellerman]

On Fri, 2015-11-13 at 15:57 +1100, Michael Neuling wrote:

> Currently we allow both the MSR T and S bits to be set by userspace on
> a signal return.  Unfortunately this is a reserved configuration and
> will cause a TM Bad Thing exception if attempted (via rfid).
> 
> This patch checks for this case in both the 32 and 64bit signals code.
> If these are both set, we clear them and trigger the bad stack frame
> handling.
> 
> Found using a syscall fuzzer.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>

Is this fixes line right?

Fixes: 2b0a576d15e0 ("powerpc: Add new transactional memory state to the signal context")

Which went into v3.9.

In which case this:

> Cc: stable@vger.kernel.org

Should be:

Cc: stable@vger.kernel.org # v3.9+

> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index a908ada..2220f7a 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -108,6 +108,7 @@
>  #define MSR_TS_T	__MASK(MSR_TS_T_LG)	/*  Transaction Transactional */
>  #define MSR_TS_MASK	(MSR_TS_T | MSR_TS_S)   /* Transaction State bits */
>  #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */
> +#define MSR_TM_RESV(x) (((x) & MSR_TS_MASK) == MSR_TS_MASK) /* Reserved */

Is reserved a good name? I think illegal/bad/wrong would be more helpful,
though I haven't checked the arch to see if it uses "reserved".

>  #define MSR_TM_TRANSACTIONAL(x)	(((x) & MSR_TS_MASK) == MSR_TS_T)
>  #define MSR_TM_SUSPENDED(x)	(((x) & MSR_TS_MASK) == MSR_TS_S)
>  
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 0dbee46..5b519c7 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -874,7 +874,16 @@ static long restore_tm_user_regs(struct pt_regs *regs,
>  		       + ELF_NEVRREG))
>  		return 1;
>  #endif /* CONFIG_SPE */
> -

I know you didn't start the rot here, but can you please use your newline key a
bit more :D

> +	/* Get the top half of the MSR */

"top" scares me now that we have both kinds of endian, but you're just moving
that code anyway, and it is sane because it's a 32-bit value that we explicitly
put the top half of the 64-bit MSR in.

> +	if (__get_user(msr_hi, &tm_sr->mc_gregs[PT_MSR]))
> +		return 1;

> +	/* Pull in MSR TM from user context */
> +	regs->msr = (regs->msr & ~MSR_TS_MASK) | ((msr_hi<<32) & MSR_TS_MASK);

Using a tmp variable would be safer.

> +	/* Don't let the user set both TM bits */
> +	if (MSR_TM_RESV(regs->msr)) {
> +		regs->msr &= ~MSR_TS_MASK;

And avoid the clear here.

> +		return 1;
> +	}

At the (minor) expense of an assignment here to regs->msr.


> @@ -884,11 +893,6 @@ static long restore_tm_user_regs(struct pt_regs *regs,
>  	current->thread.tm_texasr |= TEXASR_FS;
>  	/* This loads the checkpointed FP/VEC state, if used */
>  	tm_recheckpoint(&current->thread, msr);
> -	/* Get the top half of the MSR */
> -	if (__get_user(msr_hi, &tm_sr->mc_gregs[PT_MSR]))
> -		return 1;
> -	/* Pull in MSR TM from user context */
> -	regs->msr = (regs->msr & ~MSR_TS_MASK) | ((msr_hi<<32) & MSR_TS_MASK);
>  
>  	/* This loads the speculative FP/VEC state, if used */
>  	if (msr & MSR_FP) {
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 20756df..b320689 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -438,6 +438,12 @@ static long restore_tm_sigcontexts(struct pt_regs *regs,
>  
>  	/* get MSR separately, transfer the LE bit if doing signal return */
>  	err |= __get_user(msr, &sc->gp_regs[PT_MSR]);
> +	/* Don't allow reserved mode. */
> +	if (MSR_TM_RESV(msr)) {
> +		msr &= ~MSR_TS_MASK;

Why are you clearing the bits before returning, it's a local isn't it?

> +		return -EINVAL;
> +	}
> +
>  	/* pull in MSR TM from user context */
>  	regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr & MSR_TS_MASK);
>  

cheers

Re: [PATCH 4/5] powerpc/tm: Check for already reclaimed tasks

[Michael Neuling]

On Mon, 2015-11-16 at 20:33 +1100, Michael Ellerman wrote:
> On Mon, 2015-11-16 at 20:23 +1100, Michael Neuling wrote:
> > On Mon, 2015-11-16 at 12:51 +0530, Anshuman Khandual wrote:
> > > On 11/13/2015 10:27 AM, Michael Neuling wrote:
> > > > Currently we can hit a scenario where we'll tm_reclaim() twice.
> > > >  This
> > > > results in a TM bad thing exception because the second reclaim
> > > > occurs
> > > > when not in suspend mode.
> > > >=20
> > > > The scenario in which this can happen is the following.  We
> > > > attempt
> > > > to
> > > > deliver a signal to userspace.  To do this we need obtain the
> > > > stack
> > > > pointer to write the signal context.  To get this stack pointer
> > > > we
> > > > must tm_reclaim() in case we need to use the checkpointed stack
> > > > pointer (see get_tm_stackpointer()).  Normally we'd then return
> > > > directly to userspace to deliver the signal without going
> > > > through
> > > > __switch_to().
> > > >=20
> > > > Unfortunatley, if at this point we get an error (such as a bad
> > > > userspace stack pointer), we need to exit the process.  The
> > > > exit
> > > > will
> > > > result in a __switch_to().  __switch_to() will attempt to save
> > > > the
> > > > process state which results in another tm_reclaim().  This
> > > > tm_reclaim() now causes a TM Bad Thing exception as this state
> > > > has
> > > > already been saved and the processor is no longer in TM suspend
> > > > mode.
> > > > Whee!
> > > >=20
> > > > This patch checks the state of the MSR to ensure we are TM
> > > > suspended
> > > > before we attempt the tm_reclaim().  If we've already saved the
> > > > state
> > > > away, we should no longer be in TM suspend mode.  This has the
> > > > additional advantage of checking for a potential TM Bad Thing
> > > > exception.
> > >=20
> > > Can this situation be created using a test and verified that with
> > > this new change, the kernel can handle it successfully. I guess
> > > the self test in the series does not cover this scenario.
> >=20
> > No it doesn't.  The syscall fuzzer I have does hit it but I don't
> > have
> > permission to post that.
>=20
> And we don't really want a fuzzer as a selftest, because it might
> call unlink
> or something else bad.
>=20
> But having found the bug with the fuzzer, can't you write a test that
> triggers
> the bad case?
>=20
> > From your description it sounds like if you had a child spinning
> > with a bad r1,
> and then a parent sent it a signal that would trip it?

You'd need to turn on TM too, but yeah... I have something like this
working which I'll cleanup and post as a self test:

#include <unistd.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <stdlib.h>
#include <stdio.h>
#include <signal.h>

void signal_segv(int signum)
{
	/* This should never actually run since stack is foobar */
	exit(1);
}

int main()
{
	int pid;

	pid =3D fork();
	if (pid < 0)
		exit(1);

	if (pid) {
		// Parent
		wait(NULL);
		printf("PASSED\n");
		return 0;
	}

	if (signal(SIGSEGV, signal_segv) =3D=3D SIG_ERR)
		exit(1);

	asm volatile("li 1, 0 ;"
		     "1:"
		     ".long 0x7C00051D ;" // tbegin
		     "beq 1b ;" // retry for ever
		     ".long 0x7C0005DD ; ;" // tsuspend
		     "ld 2, 0(1) ;" // trigger segv"
		     : : : "memory");

	return 1;
}

Re: [PATCH 2/5] selftests/powerpc: Add TM signal return selftest

[Michael Ellerman]

On Fri, 2015-11-13 at 15:57 +1100, Michael Neuling wrote:

> Test the kernel's signal return code to ensure that it doesn't crash
> when both the transactional and suspend MSR bits are set in the signal
> context.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> ---
>  tools/testing/selftests/powerpc/tm/Makefile        |  2 +-
>  .../selftests/powerpc/tm/tm-signal-msr-resv.c      | 64 ++++++++++++++++++++++
>  2 files changed, 65 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c
> 
> diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile
> index 4bea62a..0c28db7 100644
> --- a/tools/testing/selftests/powerpc/tm/Makefile
> +++ b/tools/testing/selftests/powerpc/tm/Makefile
> @@ -1,4 +1,4 @@
> -TEST_PROGS := tm-resched-dscr tm-syscall
> +TEST_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv
>  
>  all: $(TEST_PROGS)
>  
> diff --git a/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c b/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c
> new file mode 100644
> index 0000000..14705ae
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c
> @@ -0,0 +1,64 @@
> +/*
> + * Copyright 2015, Michael Neuling, IBM Corp.
> + * Licensed under GPLv2.
> + *
> + * Test the kernel's signal return code to ensure that it doesn't
> + * crash when both the transactional and suspend MSR bits are set in
> + * the signal context.

That's a good start, a little blurb on how we do the test is nice too. For our
future selves.

eg. We send ourselves a SIGUSR1, and in the handler we write the illegal MSR
value, then when we return from the signal the kernel should detect that and
kill us with a SIGSEGV.

> + */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <signal.h>
> +
> +#include "utils.h"
> +
> +struct sigaction act;
> +
> +void signal_segv(int signum, siginfo_t *info, void *uc)
> +{
> +	printf("PASSED\n");
> +	exit(0);

You're not allowed to call exit() from a signal handler :D

You can call _exit(), I guess you can't just set a flag and return like you'd
normally do in a handler?

> +}
> +
> +void signal_usr1(int signum, siginfo_t *info, void *uc)
> +{
> +	ucontext_t *ucp = uc;
> +
> +	/* link tm checkpointed context to normal context */
> +	ucp->uc_link = ucp;
> +	/* set all TM bits */
> +#ifdef __powerpc64__
> +	ucp->uc_mcontext.gp_regs[PT_MSR] |= (7ULL << 32);
> +#else
> +	ucp->uc_mcontext.regs->gpr[PT_MSR] |= (7ULL);
> +#endif
> +	/* Should segv on return becuase of invalid context */

On return of what? This handler or sigaction() below ?

> +	act.sa_sigaction = signal_segv;
> +	if (sigaction(SIGSEGV, &act, NULL) < 0) {
> +		perror("sigaction sigsegv");
> +		exit(1);
> +	}

I'm not clear why we're setting up the handler for SEGV in the handler for
USR1. Is that required to trip the bug? (not that I understood).

> +}
> +
> +int tm_signal_msr_resv()
> +{
> +
> +	act.sa_sigaction = signal_usr1;
> +	sigemptyset(&act.sa_mask);
> +	act.sa_flags = SA_SIGINFO;
> +	if (sigaction(SIGUSR1, &act, NULL) < 0) {
> +		perror("sigaction sigusr1");
> +		exit(1);
> +	}
> +
> +	raise(SIGUSR1);
> +
> +	printf("FAILED\n");

The harness will print a failure message for you, so this is not really
helpful. What is helpful is a message that says *why* it failed. eg: "Expected
to take SEGV but didn't?" or something like that.

> +	return 1;
> +}
> +
> +int main(void)
> +{
> +	return test_harness(tm_signal_msr_resv, "tm_signal_msr_resv");
> +}

cheers

Re: [PATCH 1/5] powerpc: Print MSR TM bits in oops message

[Anton Blanchard]

Hi,

> > +{
> > +	if (val & (MSR_TM | MSR_TS_S | MSR_TS_T)) {
> > +		printk(",TM[");
> > +		printbits(val, msr_tm_bits, "");
> > +		printk("]");
> 
> I suspect all these individual printks are going to behave badly if
> we have multiple cpus crashing simultaneously. But I won't make you
> fix that here. We should look at it sometime though.

We really need to serialise the entire oops. I had a go at fixing this
but ran out of steam:

https://lkml.org/lkml/2015/2/23/735

Anton

Re: [PATCH 1/5] powerpc: Print MSR TM bits in oops message

[Michael Neuling]

On Mon, 2015-11-16 at 20:27 +1100, Michael Ellerman wrote:
> On Fri, 2015-11-13 at 15:57 +1100, Michael Neuling wrote:
>=20
> > Print the MSR TM bits in oops messages.  This appends them to the
> > end
> > like this:
> >  MSR: 8000000502823031 <SF,VEC,VSX,FP,ME,IR,DR,LE,TM[TE]>
> >=20
> > You get the TM[] only if at least one TM MSR bit is set.  Inside
> > the
> > TM[], E means Enabled (bit 32), S means Suspended (bit 33), and T
> > means Transactional (bit 34)
>=20
> Can you duplicate this into a comment in printtmbits() or on the bit
> definitions, so that I don't have to look up the commit to find the
> explanation.

Ok.

> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > +static struct regbit msr_tm_bits[] =3D {
> > +	{MSR_TS_T,	"T"},
> > +	{MSR_TS_S,	"S"},
> > +	{MSR_TM,	"E"},
> > +	{0,		NULL}
> > +};
> > +static void printtmbits(unsigned long val)
>=20
> I realise you followed the lead here with the naming, but can you
> call it
> print_tm_bits() please. MY EYES!

Ok, I've change the rest too -> print_msr_bits(), print_tm_bits(),
print_bits()

>=20
> > +{
> > +	if (val & (MSR_TM | MSR_TS_S | MSR_TS_T)) {
> > +		printk(",TM[");
> > +		printbits(val, msr_tm_bits, "");
> > +		printk("]");
>=20
> I suspect all these individual printks are going to behave badly if
> we have
> multiple cpus crashing simultaneously. But I won't make you fix that
> here. We
> should look at it sometime though.

Seems anton failed at this one a while back, and since I'm mortal I
might skip this one :-)

Mikey

Re: [PATCH 2/5] selftests/powerpc: Add TM signal return selftest

[Michael Neuling]

hOn Mon, 2015-11-16 at 21:24 +1100, Michael Ellerman wrote:
> On Fri, 2015-11-13 at 15:57 +1100, Michael Neuling wrote:
>=20
> > Test the kernel's signal return code to ensure that it doesn't
> > crash
> > when both the transactional and suspend MSR bits are set in the
> > signal
> > context.
> >=20
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > ---
> >  tools/testing/selftests/powerpc/tm/Makefile        |  2 +-
> >  .../selftests/powerpc/tm/tm-signal-msr-resv.c      | 64
> > ++++++++++++++++++++++
> >  2 files changed, 65 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/powerpc/tm/tm-signal
> > -msr-resv.c
> >=20
> > diff --git a/tools/testing/selftests/powerpc/tm/Makefile
> > b/tools/testing/selftests/powerpc/tm/Makefile
> > index 4bea62a..0c28db7 100644
> > --- a/tools/testing/selftests/powerpc/tm/Makefile
> > +++ b/tools/testing/selftests/powerpc/tm/Makefile
> > @@ -1,4 +1,4 @@
> > -TEST_PROGS :=3D tm-resched-dscr tm-syscall
> > +TEST_PROGS :=3D tm-resched-dscr tm-syscall tm-signal-msr-resv
> > =20
> >  all: $(TEST_PROGS)
> > =20
> > diff --git a/tools/testing/selftests/powerpc/tm/tm-signal-msr
> > -resv.c b/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c
> > new file mode 100644
> > index 0000000..14705ae
> > --- /dev/null
> > +++ b/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c
> > @@ -0,0 +1,64 @@
> > +/*
> > + * Copyright 2015, Michael Neuling, IBM Corp.
> > + * Licensed under GPLv2.
> > + *
> > + * Test the kernel's signal return code to ensure that it doesn't
> > + * crash when both the transactional and suspend MSR bits are set
> > in
> > + * the signal context.
>=20
> That's a good start, a little blurb on how we do the test is nice
> too. For our
> future selves.
>=20
> eg. We send ourselves a SIGUSR1, and in the handler we write the
> illegal MSR
> value, then when we return from the signal the kernel should detect
> that and
> kill us with a SIGSEGV.

Ok, done.

>=20
> > + */
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <signal.h>
> > +
> > +#include "utils.h"
> > +
> > +struct sigaction act;
> > +
> > +void signal_segv(int signum, siginfo_t *info, void *uc)
> > +{
> > +	printf("PASSED\n");
> > +	exit(0);
>=20
> You're not allowed to call exit() from a signal handler :D

It worked :-D

I'll move to _exit().

> You can call _exit(), I guess you can't just set a flag and return
> like you'd
> normally do in a handler?

No I can't as we keep segving when we return.

>=20
> > +}
> > +
> > +void signal_usr1(int signum, siginfo_t *info, void *uc)
> > +{
> > +	ucontext_t *ucp =3D uc;
> > +
> > +	/* link tm checkpointed context to normal context */
> > +	ucp->uc_link =3D ucp;
> > +	/* set all TM bits */
> > +#ifdef __powerpc64__
> > +	ucp->uc_mcontext.gp_regs[PT_MSR] |=3D (7ULL << 32);
> > +#else
> > +	ucp->uc_mcontext.regs->gpr[PT_MSR] |=3D (7ULL);
> > +#endif
> > +	/* Should segv on return becuase of invalid context */
>=20
> On return of what? This handler or sigaction() below ?

Because the kernel detects the illegal MSR setting above.

>=20
> > +	act.sa_sigaction =3D signal_segv;
> > +	if (sigaction(SIGSEGV, &act, NULL) < 0) {
> > +		perror("sigaction sigsegv");
> > +		exit(1);
> > +	}
>=20
> I'm not clear why we're setting up the handler for SEGV in the
> handler for
> USR1. Is that required to trip the bug? (not that I understood).

I did it here to minimise the chance of taking the segv earlier and
falsely passing the test.

I've changed this to a flag now so I can register it earlier.

> > +}
> > +
> > +int tm_signal_msr_resv()
> > +{
> > +
> > +	act.sa_sigaction =3D signal_usr1;
> > +	sigemptyset(&act.sa_mask);
> > +	act.sa_flags =3D SA_SIGINFO;
> > +	if (sigaction(SIGUSR1, &act, NULL) < 0) {
> > +		perror("sigaction sigusr1");
> > +		exit(1);
> > +	}
> > +
> > +	raise(SIGUSR1);
> > +
> > +	printf("FAILED\n");
>=20
> The harness will print a failure message for you, so this is not
> really
> helpful. What is helpful is a message that says *why* it failed. eg:
> "Expected
> to take SEGV but didn't?" or something like that.

I've removed it for now.

I've also added the compiled binary to .gitignore.

Mikey

>=20
> > +	return 1;
> > +}
> > +
> > +int main(void)
> > +{
> > +	return test_harness(tm_signal_msr_resv,
> > "tm_signal_msr_resv");
> > +}
>=20
> cheers
>=20

Re: [PATCH 3/5] powerpc/tm: Block signal return setting invalid MSR state

[Michael Neuling]

On Mon, 2015-11-16 at 21:05 +1100, Michael Ellerman wrote:
> On Fri, 2015-11-13 at 15:57 +1100, Michael Neuling wrote:
>=20
> > Currently we allow both the MSR T and S bits to be set by userspace
> > on
> > a signal return.  Unfortunately this is a reserved configuration
> > and
> > will cause a TM Bad Thing exception if attempted (via rfid).
> >=20
> > This patch checks for this case in both the 32 and 64bit signals
> > code.
> > If these are both set, we clear them and trigger the bad stack
> > frame
> > handling.
> >=20
> > Found using a syscall fuzzer.
> >=20
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
>=20
> Is this fixes line right?
>=20
> Fixes: 2b0a576d15e0 ("powerpc: Add new transactional memory state to
> the signal context")
>=20
> Which went into v3.9.
>=20
> In which case this:
>=20
> > Cc: stable@vger.kernel.org
>=20
> Should be:
>=20
> Cc: stable@vger.kernel.org # v3.9+

Ok.



>=20
> > diff --git a/arch/powerpc/include/asm/reg.h
> > b/arch/powerpc/include/asm/reg.h
> > index a908ada..2220f7a 100644
> > --- a/arch/powerpc/include/asm/reg.h
> > +++ b/arch/powerpc/include/asm/reg.h
> > @@ -108,6 +108,7 @@
> >  #define MSR_TS_T	__MASK(MSR_TS_T_LG)	/*  Transaction
> > Transactional */
> >  #define MSR_TS_MASK	(MSR_TS_T | MSR_TS_S)   /* Transaction
> > State bits */
> >  #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) !=3D 0) /* Transaction
> > active? */
> > +#define MSR_TM_RESV(x) (((x) & MSR_TS_MASK) =3D=3D MSR_TS_MASK) /*
> > Reserved */
>=20
> Is reserved a good name? I think illegal/bad/wrong would be more
> helpful,
> though I haven't checked the arch to see if it uses "reserved".

It does actually call it "reserved".  In the description of the Machine
State Register, it has a little table and calls 0b11 reserved.


>=20
> >  #define MSR_TM_TRANSACTIONAL(x)	(((x) & MSR_TS_MASK) =3D=3D
> > MSR_TS_T)
> >  #define MSR_TM_SUSPENDED(x)	(((x) & MSR_TS_MASK) =3D=3D
> > MSR_TS_S)
> > =20
> > diff --git a/arch/powerpc/kernel/signal_32.c
> > b/arch/powerpc/kernel/signal_32.c
> > index 0dbee46..5b519c7 100644
> > --- a/arch/powerpc/kernel/signal_32.c
> > +++ b/arch/powerpc/kernel/signal_32.c
> > @@ -874,7 +874,16 @@ static long restore_tm_user_regs(struct
> > pt_regs *regs,
> >  		       + ELF_NEVRREG))
> >  		return 1;
> >  #endif /* CONFIG_SPE */
> > -
>=20
> I know you didn't start the rot here, but can you please use your
> newline key a
> bit more :D

ok, newline boy!

> > +	/* Get the top half of the MSR */
>=20
> "top" scares me now that we have both kinds of endian, but you're
> just moving
> that code anyway, and it is sane because it's a 32-bit value that we
> explicitly
> put the top half of the 64-bit MSR in.

Yeah, this code is just moved from below.  It needs to be run earlier
for this new test.

... and yes, this whole signals code (inside and out TM) needs a good sprin=
g clean.

> > +	if (__get_user(msr_hi, &tm_sr->mc_gregs[PT_MSR]))
> > +		return 1;
>=20
> > +	/* Pull in MSR TM from user context */
> > +	regs->msr =3D (regs->msr & ~MSR_TS_MASK) | ((msr_hi<<32) &
> > MSR_TS_MASK);
>=20
> Using a tmp variable would be safer.


> > +	/* Don't let the user set both TM bits */
> > +	if (MSR_TM_RESV(regs->msr)) {
> > +		regs->msr &=3D ~MSR_TS_MASK;
>=20
> And avoid the clear here.
>=20
> > +		return 1;
> > +	}
>=20
> At the (minor) expense of an assignment here to regs->msr.

Ok, I'll make that tmp variable cleanup

> > @@ -884,11 +893,6 @@ static long restore_tm_user_regs(struct
> > pt_regs *regs,
> >  	current->thread.tm_texasr |=3D TEXASR_FS;
> >  	/* This loads the checkpointed FP/VEC state, if used */
> >  	tm_recheckpoint(&current->thread, msr);
> > -	/* Get the top half of the MSR */
> > -	if (__get_user(msr_hi, &tm_sr->mc_gregs[PT_MSR]))
> > -		return 1;
> > -	/* Pull in MSR TM from user context */
> > -	regs->msr =3D (regs->msr & ~MSR_TS_MASK) | ((msr_hi<<32) &
> > MSR_TS_MASK);
> > =20
> >  	/* This loads the speculative FP/VEC state, if used */
> >  	if (msr & MSR_FP) {
> > diff --git a/arch/powerpc/kernel/signal_64.c
> > b/arch/powerpc/kernel/signal_64.c
> > index 20756df..b320689 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -438,6 +438,12 @@ static long restore_tm_sigcontexts(struct
> > pt_regs *regs,
> > =20
> >  	/* get MSR separately, transfer the LE bit if doing signal
> > return */
> >  	err |=3D __get_user(msr, &sc->gp_regs[PT_MSR]);
> > +	/* Don't allow reserved mode. */
> > +	if (MSR_TM_RESV(msr)) {
> > +		msr &=3D ~MSR_TS_MASK;
>=20
> Why are you clearing the bits before returning, it's a local isn't
> it?

You're right, my bad.

Mikey

>=20
> > +		return -EINVAL;
> > +	}
> > +
> >  	/* pull in MSR TM from user context */
> >  	regs->msr =3D (regs->msr & ~MSR_TS_MASK) | (msr &
> > MSR_TS_MASK);
> > =20
>=20
> cheers
>=20