Re: [PATCH] powerpc/signals: Mark VSX not saved with small contexts

[Michael Neuling <mikey@neuling.org>]

Carlos O'Donell <carlos@redhat.com> wrote:

> On 11/21/2013 06:33 AM, Michael Ellerman wrote:
> > On Wed, Nov 20, 2013 at 04:18:54PM +1100, Michael Neuling wrote:
> >> The VSX MSR bit in the user context indicates if the context contains VSX
> >> state.  Currently we set this when the process has touched VSX at any stage.
> >>
> >> Unfortunately, if the user has not provided enough space to save the VSX state,
> >> we can't save it but we currently still set the MSR VSX bit.
> >>
> >> This patch changes this to clear the MSR VSX bit when the user doesn't provide
> >> enough space.  This indicates that there is no valid VSX state in the user
> >> context.
> >>
> >> This is needed to support get/set/make/swapcontext for applications that use
> >> VSX but only provide a small context.  For example, getcontext in glibc
> >> provides a smaller context since the VSX registers don't need to be saved over
> >> the glibc function call.  But since the program calling getcontext may have
> >> used VSX, the kernel currently says the VSX state is valid when it's not.  If
> >> the returned context is then used in setcontext (ie. a small context without
> >> VSX but with MSR VSX set), the kernel will refuse the context.  This situation
> >> has been reported by the glibc community.
> > 
> > Broken since forever?
> 
> Yes, broken since forever. At least it was known in glibc 2.18 that this was
> broken, but without an active distribution using it the defect wasn't analyzed.
> 
> >> Tested-by: Haren Myneni <haren@linux.vnet.ibm.com>
> >> Signed-off-by: Michael Neuling <mikey@neuling.org>
> >> Cc: stable@vger.kernel.org
> >> ---
> >>  arch/powerpc/kernel/signal_32.c | 10 +++++++++-
> > 
> > What about the 64-bit code? I don't know the code but it appears at a glance to
> > have the same bug.
>  
> It doesn't happen with 64-bit code because there the context contains
> a sigcontext which on ppc64 has vmx_reserve to store the entire VMX
> state. Therefore 64-bit ppc always has space to store the VMX registers
> in a userspace context switch. It is only the 32-bit ppc ABI that lacks
> the space.

VMX?  I don't understand this at all.  We extended the ucontext to
handle the extra VSX state, so older code may still be using the small
ucontext and we already have a bunch of checks in the 64bit case for
this.

I agree with Michael, we should add this to the 64 bit case.  If we
can't put VSX state in, then clear MSR VSX.

>  
> >> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> >> index 749778e..1844298 100644
> >> --- a/arch/powerpc/kernel/signal_32.c
> >> +++ b/arch/powerpc/kernel/signal_32.c
> >> @@ -457,7 +457,15 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
> >>  		if (copy_vsx_to_user(&frame->mc_vsregs, current))
> >>  			return 1;
> >>  		msr |= MSR_VSX;
> >> -	}
> >> +	} else if (!ctx_has_vsx_region)
> >> +		/*
> >> +		 * With a small context structure we can't hold the VSX
> >> +		 * registers, hence clear the MSR value to indicate the state
> >> +		 * was not saved.
> >> +		 */
> >> +		msr &= ~MSR_VSX;
> > 
> > I think it'd be clearer if this was just the else case. The full context is:
> > 
> >     if (current->thread.used_vsr && ctx_has_vsx_region) {
> >             __giveup_vsx(current);
> >             if (copy_vsx_to_user(&frame->mc_vsregs, current))
> >                     return 1;
> >             msr |= MSR_VSX;
> > +   } else if (!ctx_has_vsx_region)
> > +           /*
> > +            * With a small context structure we can't hold the VSX
> > +            * registers, hence clear the MSR value to indicate the state
> > +            * was not saved.
> > +            */
> > +           msr &= ~MSR_VSX;
> > 
> > Which means if current->thread.user_vsr and ctx_has_vsx_region are both false
> > we potentially leave MSR_VSX set in msr. I think it should be the case that
> > MSR_VSX is only ever set if current->thread.used_vsr is true, so it should be
> > OK in pratice, but it seems unnecessarily fragile.
> 
> If current->thread.user_vsr and ctx_has_vsx_region are both false then
> !ctx_has_vsx_region is true and we clear MSR_VSX.
> 
> Perhaps you mean if current->thread.user_vsr is false, but ctx_has_vsx_region
> is true? 
> 
> Previously the else clause reset MSR_VSX if:
> 1. current->thread.used_vsr == 0 && ctx_has_vsx_region == 0
> 2. current->thread.used_vsr == 1 && ctx_has_vsx_region == 0,
> 
> Now it resets MSR_VSX additionally for:
> 3. current->thread.used_vsr == 0 && ctx_has_vsx_region == 1,
> 
> 3. is a valid state. The task has not touched the VSX state and the context
> is large enough to be saved into. This may be a future state for ppc32 if 
> we adjust the ABI to have a large enough context buffer. However at present 
> it's not a plausible state. It's also a "don't care" state since there is 
> nothing saved in the context, and if nothing was saved in the context then
> MSR_VSX is not set.

This makes my head hurt.

MSR VSX needs to be cleared always if there is no VSX region.  It's
independant of used_vsr, so let's make that clear in the code.

>  
> > The logic should be "if we write VSX we set MSR_VSX, else we clear MSR_VSX", ie:
> > 
> >     if (current->thread.used_vsr && ctx_has_vsx_region) {
> >             __giveup_vsx(current);
> >             if (copy_vsx_to_user(&frame->mc_vsregs, current))
> >                     return 1;
> >             msr |= MSR_VSX;
> >     } else
> >             msr &= ~MSR_VSX;
> 
> If anything I dislike this because it might mask a bug in earlier code that
> might erroneously set MSR_VSX even though current->thread.user_vsr is not
> true. If anything the extra state 3. covered here is a buggy state.
> 
> I agree that your suggestion is more robust though since the definition of
> robustness is to operate despite failures.

The basic idea of the patch is that if the user hasn't passed a VSX
region, then we clear MSR VSX to indicate there is no VSX data.
It's independant of used_vsr.

So how about we make it that simple and put it independent of the other
if statement?

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 749778e..f4a7fd4 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -458,6 +458,14 @@ static int save_user_regs(struct pt_regs *regs, struct mcon
                        return 1;
                msr |= MSR_VSX;
        }
+       /*
+        * With a small context structure we can't hold the VSX registers,
+        * hence clear the MSR value to indicate the state was not saved.
+        */
+       if (!ctx_has_vsx_region)
+               msr &= ~MSR_VSX;
+
+
 #endif /* CONFIG_VSX */
 #ifdef CONFIG_SPE
        /* save spe registers */