Re: [PATCH 3/3] powerpc/tm: P9 disable transactionally suspended sigcontexts

[Benjamin Herrenschmidt <benh@au1.ibm.com>]

On Fri, 2017-10-06 at 18:46 +1100, Cyril Bur wrote:
> From: Michael Neuling <mikey@neuling.org>
> 
> Unfortunately userspace can construct a sigcontext which enables
> suspend. Thus userspace can force Linux into a path where trechkpt is
> executed.
> 
> This patch blocks this from happening on POWER9 but sanity checking
> sigcontexts passed in.
> 
> ptrace doesn't have this problem as only MSR SE and BE can be changed
> via ptrace.
> 
> This patch also adds a number of WARN_ON() in case we every enter
> suspend when we shouldn't. This should catch systems that don't have
> the firmware change and are running TM.
> 
> A future firmware change will allow suspend mode on POWER9 but that is
> going to require additional Linux changes to support. In the interim,
> this allows TM to continue to (partially) work while stopping
> userspace from crashing Linux.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  arch/powerpc/kernel/process.c   | 2 ++
>  arch/powerpc/kernel/signal_32.c | 4 ++++
>  arch/powerpc/kernel/signal_64.c | 5 +++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index a0c74bbf3454..5b81673c5026 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -903,6 +903,8 @@ static inline void tm_reclaim_task(struct task_struct *tsk)
>  	if (!MSR_TM_ACTIVE(thr->regs->msr))
>  		goto out_and_saveregs;
>  
> +	WARN_ON(!tm_suspend_supported());
> +

What does this function really says ? That TM is supported or that TM
supports suspend ? Because the implementation in the previous patch
seems to indicate that what it actually indicates is that TM is
supported, period.

>  	TM_DEBUG("--- tm_reclaim on pid %d (NIP=%lx, "
>  		 "ccr=%lx, msr=%lx, trap=%lx)\n",
>  		 tsk->pid, thr->regs->nip,
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 92fb1c8dbbd8..9eac0131c080 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -519,6 +519,8 @@ static int save_tm_user_regs(struct pt_regs *regs,
>  {
>  	unsigned long msr = regs->msr;
>  
> +	WARN_ON(!tm_suspend_supported());
> +
>  	/* 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
> @@ -769,6 +771,8 @@ static long restore_tm_user_regs(struct pt_regs *regs,
>  	int i;
>  #endif
>  
> +	if (!tm_suspend_supported())
> +		return 1;
>  	/*
>  	 * restore general registers but not including MSR or SOFTE. Also
>  	 * take care of keeping r2 (TLS) intact if not a signal.
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index c83c115858c1..6d28caf8496f 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -214,6 +214,8 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>  
>  	BUG_ON(!MSR_TM_ACTIVE(regs->msr));
>  
> +	WARN_ON(!tm_suspend_supported());
> +
>  	/* 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
> @@ -430,6 +432,9 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
>  
>  	BUG_ON(tsk != current);
>  
> +	if (!tm_suspend_supported())
> +		return -EINVAL;
> +
>  	/* copy the GPRs */
>  	err |= __copy_from_user(regs->gpr, tm_sc->gp_regs, sizeof(regs->gpr));
>  	err |= __copy_from_user(&tsk->thread.ckpt_regs, sc->gp_regs,