[RFC PATCH 13/14] powerpc/tm: Do not restore TM without SPRs

[Michael Neuling]

On Tue, 2018-11-06 at 10:40 -0200, Breno Leitao wrote:
> Currently the signal context restore code enables the bit on the MSR
> register without restoring the TM SPRs, which can cause undesired side
> effects.
> 
> This is not correct because if TM is enabled in MSR, it means the TM SPR
> registers are valid and updated, which is not correct here. In fact, the
> live registers may contain previous' thread SPRs.
> 
> Functions check if the register values are valid or not through looking
> if the facility is enabled at MSR, as MSR[TM] set means that the TM SPRs
> are hot.
> 
> When just enabling MSR[TM] without updating the live SPRs, this can cause a
> crash, since current TM SPR from previous thread will be saved on the
> current thread, and might not have TEXASR[FS] set, for example.
> 
> Signed-off-by: Breno Leitao <leitao at debian.org>
> ---
>  arch/powerpc/kernel/signal_64.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 487c3b6aa2e3..156b90e8ee78 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -478,8 +478,18 @@ static long restore_tm_sigcontexts(struct task_struct
> *tsk,
>  	 * happened whilst in the signal handler and load_tm overflowed,
>  	 * disabling the TM bit. In either case we can end up with an illegal
>  	 * TM state leading to a TM Bad Thing when we return to userspace.
> +	 *
> +	 * Every time MSR_TM is enabled, mainly for the b) case, the TM SPRs
> +	 * must be restored in the live registers, since the live registers
> +	 * could contain garbage and later we want to read from live, since
> +	 * MSR_TM is enabled, and MSR[TM] is what is used to check if the
> +	 * TM SPRs live registers are valid or not.
>  	 */
> -	regs->msr |= MSR_TM;
> +	if ((regs->msr & MSR_TM) == 0) {
> +		regs->msr |= MSR_TM;
> +		tm_enable();
> +		tm_restore_sprs(&tsk->thread);
> +	}

I'm wondering if we should put the save/restore TM registers in the early
entry/exit code too. We'd need to add the check on msr[tm]/load_tm.

Distributing the SPR save/restore throughout the kernel is just going to lead us
to similar problems that we are having now with reclaim/recheckpoint.

Mikey


>  
>  	/* pull in MSR LE from user context */
>  	regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);