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

[Michael Neuling]

On Tue, 2016-05-03 at 08:32 +0200, Jiri Slaby wrote:
> 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.

Greg, surely your scripts could check for that?

> It is harmless though, it seems?

Yes, that should be harmless, other than a small performance penalty.

Mikey