Re: [PATCH 4.4 60/67] powerpc/tm: Check for already reclaimed tasks

From: Jiri Slaby
Date: Tue May 03 2016 - 02:32:43 EST


On 01/27/2016, 07:12 PM, Greg Kroah-Hartman wrote:
> 4.4-stable review patch. If anyone has any objections, please let me know.
>
> ------------------
>
> From: Michael Neuling <mikey@xxxxxxxxxxx>
>
> commit 7f821fc9c77a9b01fe7b1d6e72717b33d8d64142 upstream.
>
> 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.
>
> Fixes: fb09692e71f1 ("powerpc: Add reclaim and recheckpoint functions for context switching transactional memory processes")
> Signed-off-by: Michael Neuling <mikey@xxxxxxxxxxx>
> Signed-off-by: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>
> ---
> arch/powerpc/kernel/process.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -569,6 +569,24 @@ static void tm_reclaim_thread(struct thr
> if (!MSR_TM_SUSPENDED(mfmsr()))
> return;
>
> + /*
> + * 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 thread_struct bit, as it has the additional
> + * benifit of checking for a potential TM bad thing exception.
> + */
> + if (!MSR_TM_SUSPENDED(mfmsr()))
> + return;

This one should have not been applied to 4.4. The patch is in mainline
since 4.4-rc6. Hence the check is duplicated as can be seen above.

It is harmless though, it seems?

thanks,
--
js
suse labs