Carlos O'Donell <car...@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 <ha...@linux.vnet.ibm.com>
> >> Signed-off-by: Michael Neuling <mi...@neuling.org>
> >> Cc: sta...@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 */
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
