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

[Michael Ellerman <mpe@ellerman.id.au>]

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?

> 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.


> 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.

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;

cheers