On Mon, 2018-11-19 at 10:44 -0200, 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.
Do we have the same problem on the entry side? There are a bunch of
copy_from_user() before we do a tm_reclaim() (ie when still transactional). Is
there some chance of the same problem there?
> 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 TEXAR[FS] bit set also.
This patch is great and should go in ASAP
> 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().
>
> [ 2181.457997] kernel BUG at arch/powerpc/kernel/tm.S:446!
Can you put more of the backtrace here? Can be useful for people searching for
a similar problem.
> 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.
Can you put a comment in the code what says after the MSR[TS] setting, there can
be no get/put_user before the recheckpoint?
Also, it looks like the 32bit signals case is safe, but please add a comment in
there too.
> 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.
>
> 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, as done in this patch.
>
> Fixes: 87b4e5393af7 ("powerpc/tm: Fix return of active 64bit signals")
> Cc: sta...@vger.kernel.org (v3.9+)
> Signed-off-by: Breno Leitao <lei...@debian.org>
> ---
> arch/powerpc/kernel/signal_64.c | 29 +++++++++++++++--------------
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 83d51bf586c7..15b153bdd826 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -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);
> -
> - /*
> - * Ensure that TM is enabled in regs->msr before we leave the signal
> - * handler. It could be the case that (a) user disabled the TM bit
> - * through the manipulation of the MSR bits in uc_mcontext or (b) the
> - * TM bit was disabled because a sufficient number of context switches
> - * 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.
> - */
> - regs->msr |= MSR_TM;
> -
> /* pull in MSR LE from user context */
> regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
>
> @@ -572,6 +558,21 @@ 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;
> +
> + /* pull in MSR TS bits from user context */
> + regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr & MSR_TS_MASK);
> +
> + /*
> + * Ensure that TM is enabled in regs->msr before we leave the signal
> + * handler. It could be the case that (a) user disabled the TM bit
> + * through the manipulation of ararararthe MSR bits in uc_mcontext or
> (b) the
> + * TM bit was disabled because a sufficient number of context switches
> + * 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.
> + */
> + regs->msr |= MSR_TM;
> +
> /* This loads the checkpointed FP/VEC state, if used */
> tm_recheckpoint(&tsk->thread);
>
