stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim
@ 2020-01-16 22:05 Gustavo Luiz Duarte
  2020-01-17 20:57 ` Gustavo Romero
  2020-01-21 14:30 ` kbuild test robot
  0 siblings, 2 replies; 3+ messages in thread
From: Gustavo Luiz Duarte @ 2020-01-16 22:05 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: mikey, gromero, Gustavo Luiz Duarte, stable

After a treclaim, we expect to be in non-transactional state. If we don't
immediately clear the current thread's MSR[TS] and we get preempted, then
tm_recheckpoint_new_task() will recheckpoint and we get rescheduled in
suspended transaction state.

When handling a signal caught in transactional state, handle_rt_signal64()
calls get_tm_stackpointer() that treclaims the transaction using
tm_reclaim_current() but without clearing the thread's MSR[TS]. This can cause
the TM Bad Thing exception below if later we pagefault and get preempted trying
to access the user's sigframe, using __put_user(). Afterwards, when we are
rescheduled back into do_page_fault() (but now in suspended state since the
thread's MSR[TS] was not cleared), upon executing 'rfid' after completion of
the page fault handling, the exception is raised because a transition from
suspended to non-transactional state is invalid.

	Unexpected TM Bad Thing exception at c00000000000de44 (msr 0x8000000302a03031) tm_scratch=800000010280b033
	Oops: Unrecoverable exception, sig: 6 [#1]
	LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
	Modules linked in: nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6_tables ip_tables nft_compat ip_set nf_tables nfnetlink xts vmx_crypto sg virtio_balloon
	r_mod cdrom virtio_net net_failover virtio_blk virtio_scsi failover dm_mirror dm_region_hash dm_log dm_mod
	CPU: 25 PID: 15547 Comm: a.out Not tainted 5.4.0-rc2 #32
	NIP:  c00000000000de44 LR: c000000000034728 CTR: 0000000000000000
	REGS: c00000003fe7bd70 TRAP: 0700   Not tainted  (5.4.0-rc2)
	MSR:  8000000302a03031 <SF,VEC,VSX,FP,ME,IR,DR,LE,TM[SE]>  CR: 44000884  XER: 00000000
	CFAR: c00000000000dda4 IRQMASK: 0
	PACATMSCRATCH: 800000010280b033
	GPR00: c000000000034728 c000000f65a17c80 c000000001662800 00007fffacf3fd78
	GPR04: 0000000000001000 0000000000001000 0000000000000000 c000000f611f8af0
	GPR08: 0000000000000000 0000000078006001 0000000000000000 000c000000000000
	GPR12: c000000f611f84b0 c00000003ffcb200 0000000000000000 0000000000000000
	GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
	GPR20: 0000000000000000 0000000000000000 0000000000000000 c000000f611f8140
	GPR24: 0000000000000000 00007fffacf3fd68 c000000f65a17d90 c000000f611f7800
	GPR28: c000000f65a17e90 c000000f65a17e90 c000000001685e18 00007fffacf3f000
	NIP [c00000000000de44] fast_exception_return+0xf4/0x1b0
	LR [c000000000034728] handle_rt_signal64+0x78/0xc50
	Call Trace:
	[c000000f65a17c80] [c000000000034710] handle_rt_signal64+0x60/0xc50 (unreliable)
	[c000000f65a17d30] [c000000000023640] do_notify_resume+0x330/0x460
	[c000000f65a17e20] [c00000000000dcc4] ret_from_except_lite+0x70/0x74
	Instruction dump:
	7c4ff120 e8410170 7c5a03a6 38400000 f8410060 e8010070 e8410080 e8610088
	60000000 60000000 e8810090 e8210078 <4c000024> 48000000 e8610178 88ed0989
	---[ end trace 93094aa44b442f87 ]---

The simplified sequence of events that triggers the above exception is:

  ...				# userspace in NON-TRANSACTIONAL state
  tbegin			# userspace in TRANSACTIONAL state
  signal delivery		# kernelspace in SUSPENDED state
  handle_rt_signal64()
    get_tm_stackpointer()
      treclaim			# kernelspace in NON-TRANSACTIONAL state
    __put_user()
      page fault happens. We will never get back here because of the TM Bad Thing exception.

  page fault handling kicks in and we voluntarily preempt ourselves
  do_page_fault()
    __schedule()
      __switch_to(other_task)

  our task is rescheduled and we recheckpoint because the thread's MSR[TS] was not cleared
  __switch_to(our_task)
    switch_to_tm()
      tm_recheckpoint_new_task()
        trechkpt			# kernelspace in SUSPENDED state

  The page fault handling resumes, but now we are in suspended transaction state
  do_page_fault()    completes
  rfid     <----- trying to get back where the page fault happened (we were non-transactional back then)
  TM Bad Thing			# illegal transition from suspended to non-transactional

This patch fixes that issue by clearing the current thread's MSR[TS] just after
treclaim in get_tm_stackpointer() so that we stay in non-transactional state in
case we are preempted. In order to make treclaim and clearing the thread's
MSR[TS] atomic from a preemption perspective when CONFIG_PREEMPT is set,
preempt_disable/enable() is used. It's also necessary to save the previous
value of the thread's MSR before get_tm_stackpointer() is called so that it can
be exposed to the signal handler later in setup_tm_sigcontexts() to inform the
userspace MSR at the moment of the signal delivery.

Found with tm-signal-context-force-tm kernel selftest on P8 KVM.

Fixes: 2b0a576d15e0 ("powerpc: Add new transactional memory state to the signal context")
Cc: stable@vger.kernel.org # v3.9
Signed-off-by: Gustavo Luiz Duarte <gustavold@linux.ibm.com>
---
 arch/powerpc/kernel/signal.c    | 17 +++++++++++++++--
 arch/powerpc/kernel/signal_32.c | 24 ++++++++++--------------
 arch/powerpc/kernel/signal_64.c | 20 ++++++++------------
 3 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index e6c30cee6abf..1660be1061ac 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -200,14 +200,27 @@ unsigned long get_tm_stackpointer(struct task_struct *tsk)
 	 * normal/non-checkpointed stack pointer.
 	 */
 
+	unsigned long ret = tsk->thread.regs->gpr[1];
+
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	BUG_ON(tsk != current);
 
 	if (MSR_TM_ACTIVE(tsk->thread.regs->msr)) {
+		preempt_disable();
 		tm_reclaim_current(TM_CAUSE_SIGNAL);
 		if (MSR_TM_TRANSACTIONAL(tsk->thread.regs->msr))
-			return tsk->thread.ckpt_regs.gpr[1];
+			ret = tsk->thread.ckpt_regs.gpr[1];
+
+		/* If we treclaim, we must immediately clear the current
+		 * thread's TM bits. Otherwise we might be preempted and have
+		 * the live MSR[TS] changed behind our back
+		 * (tm_recheckpoint_new_task() would recheckpoint).
+		 * Besides, we enter the signal handler in non-transactional
+		 * state.
+		 */
+		tsk->thread.regs->msr &= ~MSR_TS_MASK;
+		preempt_enable();
 	}
 #endif
-	return tsk->thread.regs->gpr[1];
+	return ret;
 }
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 98600b276f76..132a092cd170 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -489,19 +489,11 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
  */
 static int save_tm_user_regs(struct pt_regs *regs,
 			     struct mcontext __user *frame,
-			     struct mcontext __user *tm_frame, int sigret)
+			     struct mcontext __user *tm_frame, int sigret,
+			     unsigned long msr)
 {
-	unsigned long msr = regs->msr;
-
 	WARN_ON(tm_suspend_disabled);
 
-	/* Remove TM bits from thread's MSR.  The MSR in the sigcontext
-	 * just indicates to userland that we were doing a transaction, but we
-	 * don't want to return in transactional state.  This also ensures
-	 * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
-	 */
-	regs->msr &= ~MSR_TS_MASK;
-
 	/* Save both sets of general registers */
 	if (save_general_regs(&current->thread.ckpt_regs, frame)
 	    || save_general_regs(regs, tm_frame))
@@ -912,6 +904,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	int sigret;
 	unsigned long tramp;
 	struct pt_regs *regs = tsk->thread.regs;
+	/* Save the thread's msr before get_tm_stackpointer() changes it */
+	unsigned long msr = regs->msr;
 
 	BUG_ON(tsk != current);
 
@@ -944,13 +938,13 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	tm_frame = &rt_sf->uc_transact.uc_mcontext;
-	if (MSR_TM_ACTIVE(regs->msr)) {
+	if (MSR_TM_ACTIVE(msr)) {
 		if (__put_user((unsigned long)&rt_sf->uc_transact,
 			       &rt_sf->uc.uc_link) ||
 		    __put_user((unsigned long)tm_frame,
 			       &rt_sf->uc_transact.uc_regs))
 			goto badframe;
-		if (save_tm_user_regs(regs, frame, tm_frame, sigret))
+		if (save_tm_user_regs(regs, frame, tm_frame, sigret, msr))
 			goto badframe;
 	}
 	else
@@ -1369,6 +1363,8 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 	int sigret;
 	unsigned long tramp;
 	struct pt_regs *regs = tsk->thread.regs;
+	/* Save the thread's msr before get_tm_stackpointer() changes it */
+	unsigned long msr = regs->msr;
 
 	BUG_ON(tsk != current);
 
@@ -1402,9 +1398,9 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	tm_mctx = &frame->mctx_transact;
-	if (MSR_TM_ACTIVE(regs->msr)) {
+	if (MSR_TM_ACTIVE(msr)) {
 		if (save_tm_user_regs(regs, &frame->mctx, &frame->mctx_transact,
-				      sigret))
+				      sigret, msr))
 			goto badframe;
 	}
 	else
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 117515564ec7..e5b5f9738056 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -192,7 +192,8 @@ static long setup_sigcontext(struct sigcontext __user *sc,
 static long setup_tm_sigcontexts(struct sigcontext __user *sc,
 				 struct sigcontext __user *tm_sc,
 				 struct task_struct *tsk,
-				 int signr, sigset_t *set, unsigned long handler)
+				 int signr, sigset_t *set, unsigned long handler,
+				 unsigned long msr)
 {
 	/* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
 	 * process never used altivec yet (MSR_VEC is zero in pt_regs of
@@ -207,12 +208,11 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
 	elf_vrreg_t __user *tm_v_regs = sigcontext_vmx_regs(tm_sc);
 #endif
 	struct pt_regs *regs = tsk->thread.regs;
-	unsigned long msr = tsk->thread.regs->msr;
 	long err = 0;
 
 	BUG_ON(tsk != current);
 
-	BUG_ON(!MSR_TM_ACTIVE(regs->msr));
+	BUG_ON(!MSR_TM_ACTIVE(msr));
 
 	WARN_ON(tm_suspend_disabled);
 
@@ -222,13 +222,6 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
 	 */
 	msr |= tsk->thread.ckpt_regs.msr & (MSR_FP | MSR_VEC | MSR_VSX);
 
-	/* Remove TM bits from thread's MSR.  The MSR in the sigcontext
-	 * just indicates to userland that we were doing a transaction, but we
-	 * don't want to return in transactional state.  This also ensures
-	 * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
-	 */
-	regs->msr &= ~MSR_TS_MASK;
-
 #ifdef CONFIG_ALTIVEC
 	err |= __put_user(v_regs, &sc->v_regs);
 	err |= __put_user(tm_v_regs, &tm_sc->v_regs);
@@ -824,6 +817,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	unsigned long newsp = 0;
 	long err = 0;
 	struct pt_regs *regs = tsk->thread.regs;
+	/* Save the thread's msr before get_tm_stackpointer() changes it */
+	unsigned long msr = regs->msr;
 
 	BUG_ON(tsk != current);
 
@@ -841,7 +836,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	err |= __put_user(0, &frame->uc.uc_flags);
 	err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-	if (MSR_TM_ACTIVE(regs->msr)) {
+	if (MSR_TM_ACTIVE(msr)) {
 		/* The ucontext_t passed to userland points to the second
 		 * ucontext_t (for transactional state) with its uc_link ptr.
 		 */
@@ -849,7 +844,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 		err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext,
 					    &frame->uc_transact.uc_mcontext,
 					    tsk, ksig->sig, NULL,
-					    (unsigned long)ksig->ka.sa.sa_handler);
+					    (unsigned long)ksig->ka.sa.sa_handler,
+					    msr);
 	} else
 #endif
 	{
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim
  2020-01-16 22:05 [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim Gustavo Luiz Duarte
@ 2020-01-17 20:57 ` Gustavo Romero
  2020-01-21 14:30 ` kbuild test robot
  1 sibling, 0 replies; 3+ messages in thread
From: Gustavo Romero @ 2020-01-17 20:57 UTC (permalink / raw)
  To: Gustavo Luiz Duarte, linuxppc-dev; +Cc: mikey, gromero, stable

Hi Gustavo,

Thanks for fixing that TM issue.

On 01/16/2020 07:05 PM, Gustavo Luiz Duarte wrote:
> Fixes: 2b0a576d15e0 ("powerpc: Add new transactional memory state to the signal context")
> Cc: stable@vger.kernel.org # v3.9
> Signed-off-by: Gustavo Luiz Duarte <gustavold@linux.ibm.com>
> ---
>   arch/powerpc/kernel/signal.c    | 17 +++++++++++++++--
>   arch/powerpc/kernel/signal_32.c | 24 ++++++++++--------------
>   arch/powerpc/kernel/signal_64.c | 20 ++++++++------------
>   3 files changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> index e6c30cee6abf..1660be1061ac 100644
> --- a/arch/powerpc/kernel/signal.c
> +++ b/arch/powerpc/kernel/signal.c
> @@ -200,14 +200,27 @@ unsigned long get_tm_stackpointer(struct task_struct *tsk)
>   	 * normal/non-checkpointed stack pointer.
>   	 */
>   
> +	unsigned long ret = tsk->thread.regs->gpr[1];
> +
>   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>   	BUG_ON(tsk != current);
>   
>   	if (MSR_TM_ACTIVE(tsk->thread.regs->msr)) {
> +		preempt_disable();
>   		tm_reclaim_current(TM_CAUSE_SIGNAL);
>   		if (MSR_TM_TRANSACTIONAL(tsk->thread.regs->msr))
> -			return tsk->thread.ckpt_regs.gpr[1];
> +			ret = tsk->thread.ckpt_regs.gpr[1];
> +
> +		/* If we treclaim, we must immediately clear the current
> +		 * thread's TM bits. Otherwise we might be preempted and have
> +		 * the live MSR[TS] changed behind our back
> +		 * (tm_recheckpoint_new_task() would recheckpoint).
> +		 * Besides, we enter the signal handler in non-transactional
> +		 * state.
> +		 */
> +		tsk->thread.regs->msr &= ~MSR_TS_MASK;
> +		preempt_enable();
>   	}
>   #endif
> -	return tsk->thread.regs->gpr[1];
> +	return ret;
>   }
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 98600b276f76..132a092cd170 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -489,19 +489,11 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
>    */
>   static int save_tm_user_regs(struct pt_regs *regs,
>   			     struct mcontext __user *frame,
> -			     struct mcontext __user *tm_frame, int sigret)
> +			     struct mcontext __user *tm_frame, int sigret,
> +			     unsigned long msr)
>   {
> -	unsigned long msr = regs->msr;
> -
>   	WARN_ON(tm_suspend_disabled);
>   
> -	/* Remove TM bits from thread's MSR.  The MSR in the sigcontext
> -	 * just indicates to userland that we were doing a transaction, but we
> -	 * don't want to return in transactional state.  This also ensures
> -	 * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
> -	 */
> -	regs->msr &= ~MSR_TS_MASK;
> -
>   	/* Save both sets of general registers */
>   	if (save_general_regs(&current->thread.ckpt_regs, frame)
>   	    || save_general_regs(regs, tm_frame))
> @@ -912,6 +904,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>   	int sigret;
>   	unsigned long tramp;
>   	struct pt_regs *regs = tsk->thread.regs;
> +	/* Save the thread's msr before get_tm_stackpointer() changes it */
> +	unsigned long msr = regs->msr;
>   
>   	BUG_ON(tsk != current);
>   
> @@ -944,13 +938,13 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>   
>   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>   	tm_frame = &rt_sf->uc_transact.uc_mcontext;
> -	if (MSR_TM_ACTIVE(regs->msr)) {
> +	if (MSR_TM_ACTIVE(msr)) {
>   		if (__put_user((unsigned long)&rt_sf->uc_transact,
>   			       &rt_sf->uc.uc_link) ||
>   		    __put_user((unsigned long)tm_frame,
>   			       &rt_sf->uc_transact.uc_regs))
>   			goto badframe;
> -		if (save_tm_user_regs(regs, frame, tm_frame, sigret))
> +		if (save_tm_user_regs(regs, frame, tm_frame, sigret, msr))
>   			goto badframe;
>   	}
>   	else
> @@ -1369,6 +1363,8 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
>   	int sigret;
>   	unsigned long tramp;
>   	struct pt_regs *regs = tsk->thread.regs;
> +	/* Save the thread's msr before get_tm_stackpointer() changes it */
> +	unsigned long msr = regs->msr;
>   
>   	BUG_ON(tsk != current);
>   
> @@ -1402,9 +1398,9 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
>   
>   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>   	tm_mctx = &frame->mctx_transact;
> -	if (MSR_TM_ACTIVE(regs->msr)) {
> +	if (MSR_TM_ACTIVE(msr)) {
>   		if (save_tm_user_regs(regs, &frame->mctx, &frame->mctx_transact,
> -				      sigret))
> +				      sigret, msr))
>   			goto badframe;
>   	}
>   	else
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 117515564ec7..e5b5f9738056 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -192,7 +192,8 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>   static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>   				 struct sigcontext __user *tm_sc,
>   				 struct task_struct *tsk,
> -				 int signr, sigset_t *set, unsigned long handler)
> +				 int signr, sigset_t *set, unsigned long handler,
> +				 unsigned long msr)
>   {
>   	/* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
>   	 * process never used altivec yet (MSR_VEC is zero in pt_regs of
> @@ -207,12 +208,11 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>   	elf_vrreg_t __user *tm_v_regs = sigcontext_vmx_regs(tm_sc);
>   #endif
>   	struct pt_regs *regs = tsk->thread.regs;
> -	unsigned long msr = tsk->thread.regs->msr;
>   	long err = 0;
>   
>   	BUG_ON(tsk != current);
>   
> -	BUG_ON(!MSR_TM_ACTIVE(regs->msr));
> +	BUG_ON(!MSR_TM_ACTIVE(msr));
>   
>   	WARN_ON(tm_suspend_disabled);
>   
> @@ -222,13 +222,6 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>   	 */
>   	msr |= tsk->thread.ckpt_regs.msr & (MSR_FP | MSR_VEC | MSR_VSX);
>   
> -	/* Remove TM bits from thread's MSR.  The MSR in the sigcontext
> -	 * just indicates to userland that we were doing a transaction, but we
> -	 * don't want to return in transactional state.  This also ensures
> -	 * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
> -	 */
> -	regs->msr &= ~MSR_TS_MASK;
> -
>   #ifdef CONFIG_ALTIVEC
>   	err |= __put_user(v_regs, &sc->v_regs);
>   	err |= __put_user(tm_v_regs, &tm_sc->v_regs);
> @@ -824,6 +817,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>   	unsigned long newsp = 0;
>   	long err = 0;
>   	struct pt_regs *regs = tsk->thread.regs;
> +	/* Save the thread's msr before get_tm_stackpointer() changes it */
> +	unsigned long msr = regs->msr;
>   
>   	BUG_ON(tsk != current);
>   
> @@ -841,7 +836,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>   	err |= __put_user(0, &frame->uc.uc_flags);
>   	err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
>   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> -	if (MSR_TM_ACTIVE(regs->msr)) {
> +	if (MSR_TM_ACTIVE(msr)) {
>   		/* The ucontext_t passed to userland points to the second
>   		 * ucontext_t (for transactional state) with its uc_link ptr.
>   		 */
> @@ -849,7 +844,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>   		err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext,
>   					    &frame->uc_transact.uc_mcontext,
>   					    tsk, ksig->sig, NULL,
> -					    (unsigned long)ksig->ka.sa.sa_handler);
> +					    (unsigned long)ksig->ka.sa.sa_handler,
> +					    msr);
>   	} else
>   #endif
>   	{
> 

The change needs to be improved in case CONFIG_PPC_TRANSACTIONAL_MEM=n, like:

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 132a092cd170..1b090a76b444 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -904,8 +904,10 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
         int sigret;
         unsigned long tramp;
         struct pt_regs *regs = tsk->thread.regs;
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
         /* Save the thread's msr before get_tm_stackpointer() changes it */
         unsigned long msr = regs->msr;
+#endif
  
         BUG_ON(tsk != current);
  
@@ -1363,8 +1365,10 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
         int sigret;
         unsigned long tramp;
         struct pt_regs *regs = tsk->thread.regs;
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
         /* Save the thread's msr before get_tm_stackpointer() changes it */
         unsigned long msr = regs->msr;
+#endif
  
         BUG_ON(tsk != current);
  
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index e5b5f9738056..84ed2e77ef9c 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -817,8 +817,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
         unsigned long newsp = 0;
         long err = 0;
         struct pt_regs *regs = tsk->thread.regs;
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
         /* Save the thread's msr before get_tm_stackpointer() changes it */
         unsigned long msr = regs->msr;
+#endif
  
         BUG_ON(tsk != current);

or -Werror=unused-variable will catch it like:

/linux/arch/powerpc/kernel/signal_32.c: In function 'handle_rt_signal32':
/linux/arch/powerpc/kernel/signal_32.c:908:16: error: unused variable 'msr' [-Werror=unused-variable]
   908 |  unsigned long msr = regs->msr;
       |                ^~~
/linux/arch/powerpc/kernel/signal_32.c: In function 'handle_signal32':
/linux/arch/powerpc/kernel/signal_32.c:1367:16: error: unused variable 'msr' [-Werror=unused-variable]
  1367 |  unsigned long msr = regs->msr;
       |


Feel free to send a v2 only after Mikey's review.

Otherwise, LGTM.

Reviewed-by: Gustavo Romero <gromero@linux.ibm.com>

Best regards,
Gustavo

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim
  2020-01-16 22:05 [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim Gustavo Luiz Duarte
  2020-01-17 20:57 ` Gustavo Romero
@ 2020-01-21 14:30 ` kbuild test robot
  1 sibling, 0 replies; 3+ messages in thread
From: kbuild test robot @ 2020-01-21 14:30 UTC (permalink / raw)
  To: Gustavo Luiz Duarte
  Cc: kbuild-all, linuxppc-dev, mikey, Gustavo Luiz Duarte, stable, gromero

[-- Attachment #1: Type: text/plain, Size: 3877 bytes --]

Hi Gustavo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on scottwood/next v5.5-rc7 next-20200120]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Gustavo-Luiz-Duarte/powerpc-tm-Clear-the-current-thread-s-MSR-TS-after-treclaim/20200118-034925
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-a001-20200121 (attached as .config)
compiler: powerpc64le-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/powerpc/kernel/signal_32.c: In function 'handle_rt_signal32':
>> arch/powerpc/kernel/signal_32.c:908:16: error: unused variable 'msr' [-Werror=unused-variable]
     unsigned long msr = regs->msr;
                   ^~~
   arch/powerpc/kernel/signal_32.c: In function 'handle_signal32':
   arch/powerpc/kernel/signal_32.c:1367:16: error: unused variable 'msr' [-Werror=unused-variable]
     unsigned long msr = regs->msr;
                   ^~~
   cc1: all warnings being treated as errors
--
   arch/powerpc/kernel/signal_64.c: In function 'handle_rt_signal64':
>> arch/powerpc/kernel/signal_64.c:821:16: error: unused variable 'msr' [-Werror=unused-variable]
     unsigned long msr = regs->msr;
                   ^~~
   cc1: all warnings being treated as errors

vim +/msr +908 arch/powerpc/kernel/signal_32.c

   891	
   892	/*
   893	 * Set up a signal frame for a "real-time" signal handler
   894	 * (one which gets siginfo).
   895	 */
   896	int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
   897			       struct task_struct *tsk)
   898	{
   899		struct rt_sigframe __user *rt_sf;
   900		struct mcontext __user *frame;
   901		struct mcontext __user *tm_frame = NULL;
   902		void __user *addr;
   903		unsigned long newsp = 0;
   904		int sigret;
   905		unsigned long tramp;
   906		struct pt_regs *regs = tsk->thread.regs;
   907		/* Save the thread's msr before get_tm_stackpointer() changes it */
 > 908		unsigned long msr = regs->msr;
   909	
   910		BUG_ON(tsk != current);
   911	
   912		/* Set up Signal Frame */
   913		/* Put a Real Time Context onto stack */
   914		rt_sf = get_sigframe(ksig, get_tm_stackpointer(tsk), sizeof(*rt_sf), 1);
   915		addr = rt_sf;
   916		if (unlikely(rt_sf == NULL))
   917			goto badframe;
   918	
   919		/* Put the siginfo & fill in most of the ucontext */
   920		if (copy_siginfo_to_user(&rt_sf->info, &ksig->info)
   921		    || __put_user(0, &rt_sf->uc.uc_flags)
   922		    || __save_altstack(&rt_sf->uc.uc_stack, regs->gpr[1])
   923		    || __put_user(to_user_ptr(&rt_sf->uc.uc_mcontext),
   924			    &rt_sf->uc.uc_regs)
   925		    || put_sigset_t(&rt_sf->uc.uc_sigmask, oldset))
   926			goto badframe;
   927	
   928		/* Save user registers on the stack */
   929		frame = &rt_sf->uc.uc_mcontext;
   930		addr = frame;
   931		if (vdso32_rt_sigtramp && tsk->mm->context.vdso_base) {
   932			sigret = 0;
   933			tramp = tsk->mm->context.vdso_base + vdso32_rt_sigtramp;
   934		} else {
   935			sigret = __NR_rt_sigreturn;
   936			tramp = (unsigned long) frame->tramp;
   937		}
   938	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29883 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-01-21 14:30 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-16 22:05 [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim Gustavo Luiz Duarte
2020-01-17 20:57 ` Gustavo Romero
2020-01-21 14:30 ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).