Message-ID: <20170126081417.GC3399@gmail.com>
Date:   Thu, 26 Jan 2017 09:14:17 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     riel@...hat.com
Cc:     linux-kernel@...r.kernel.org, luto@...nel.org,
        yu-cheng.yu@...el.com, dave.hansen@...ux.intel.com, bp@...e.de
Subject: Re: [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state


* riel@...hat.com <riel@...hat.com> wrote:

> From: Rik van Riel <riel@...hat.com>
> 
> On Skylake CPUs I noticed that XRSTOR is unable to deal with states
> created by copyout_from_xsaves if the xstate has only SSE/YMM state, and
> no FP state. That is, xfeatures had XFEATURE_MASK_SSE set, but not
> XFEATURE_MASK_FP.
> 
> The reason is that part of the SSE/YMM state lives in the MXCSR and
> MXCSR_FLAGS fields of the FP state.
> 
> Ensure that whenever we copy SSE or YMM state around, the MXCSR and
> MXCSR_FLAGS fields are also copied around.
> 
> Signed-off-by: Rik van Riel <riel@...hat.com>
> ---
>  arch/x86/kernel/fpu/xstate.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c1508d56ecfb..10b10917af81 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1004,6 +1004,23 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
>  	}
>  
>  	/*
> +	 * Restoring SSE/YMM state requires that MXCSR & MXCSR_MASK are saved.
> +	 * Those fields are part of the legacy FP state, and only get saved
> +	 * above if XFEATURES_MASK_FP is set.
> +	 *
> +	 * Copy out those fields if we have SSE/YMM but no FP register data.
> +	 */
> +	if ((header.xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)) &&
> +			!(header.xfeatures & XFEATURE_MASK_FP)) {
> +		size = sizeof(u64);
> +		ret = xstate_copyout(offset, size, kbuf, ubuf,
> +				     &xsave->i387.mxcsr, 0, count);

So this u64 copy copies both i387.mxcsr and i387.mxcsr_mask, which only works 
because the two fields are next to each other and there's no hole inbetween in the 
structure, right?

That fact should at minimum be commented upon.

> @@ -1053,7 +1071,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,

Also, I clearly wasn't paying enough attention when I merged the commit that 
introduced these ptrace conversion bits:

  91c3dba7dbc1 ("x86/fpu/xstate: Fix PTRACE frames for XSAVES")

1)

the 'copyin/copyout' nomenclature needlessly departs from what the modern FPU code 
uses, which is:

 copy_fpregs_to_fpstate()
 copy_fpstate_to_sigframe()
 copy_fregs_to_user()
 copy_fxregs_to_kernel()
 copy_fxregs_to_user()
 copy_kernel_to_fpregs()
 copy_kernel_to_fregs()
 copy_kernel_to_fxregs()
 copy_kernel_to_xregs()
 copy_user_to_fregs()
 copy_user_to_fxregs()
 copy_user_to_xregs()
 copy_xregs_to_kernel()
 copy_xregs_to_user()

I.e. according to this pattern, the following rename should be done:

  copyin_to_xsaves()    -> copy_user_to_xstate()
  copyout_from_xsaves() -> copy_xstate_to_user()

or, if we want to be pedantic, denote that that the user-space format is ptrace:

  copyin_to_xsaves()    -> copy_user_ptrace_to_xstate()
  copyout_from_xsaves() -> copy_xstate_to_user_ptrace()

(But I'd suggest the shorter, non-pedantic name.)

But there's other problems:

2)

The copy_user_to_xstate() parameter order departs from the regular memcpy() 
pattern we try to follow:

  int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
                     struct xregs_state *xsave);

it should be the other way around:

  int copy_user_to_xstate(struct xregs_state *xsave, const void *kbuf, const void __user *ubuf)

3)

But there's worse problems - the 'kbuf' parameter in both APIs, for example in 
copy_xstate_to_user():

        if (kbuf) {
                memcpy(&xfeatures, kbuf + offset, size);
        } else {
                if (__copy_from_user(&xfeatures, ubuf + offset, size))
                        return -EFAULT;
	}

WTF: memory copy API semantics dependent on argument presence? Whether it's truly 
a 'user' copy depends on whether 'kbuf' is NULL??

This should be split into four APIs:

	copy_xstate_to_user()
	copy_xstate_to_kernel()
	copy_user_to_xstate()
	copy_kernel_to_xstate()

This decoupling would remove the weird 'kbuf, ubuf, xstate' triple argument 
dependence and turn them into regular two-argument memcpy() variant APIs:

	copy_xstate_to_user   (ubuf, xstate)
	copy_xstate_to_kernel (kbuf, xstate)
	copy_user_to_xstate   (xstate, ubuf)
	copy_kernel_to_xstate (xstate, kbuf)

... and would restore the type cleanliness/robustness of these APIs as well.

4)

>  	/*
> +	 * SSE/YMM state depends on the MXCSR & MXCSR_MASK fields from the FP
> +	 * state. If we restored only SSE/YMM state but not FP state, copy
> +	 * those fields to ensure the SSE/YMM state restore works.
> +	 */
> +	if ((xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)) &&
> +			!(xfeatures & XFEATURE_MASK_FP)) {

So this pattern is used twice and it's quite a mouthful. How about introducing 
such a helper:

/*
 * Weird legacy quirk: indicate whether the MXCSR/MXCSR_MASK part of the FP state 
 * is used, even though the xfeatures flag lies about it being unused:
 */
static inline bool xfeatures_fp_mxcsr_used(u64 xfeatures)
{
	if (!(xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)))
		return 0;

	if (xfeatures & XFEATURE_MASK_FP)
		return 0;

	return 1;
}

?

5)

While at it I noticed this code:

                u64 mask = ((u64)1 << i);

instead of the ugly type cast, cannot that be written as:

		u64 mask = 1ULL << i;

which is shorter and cleaner?

I.e. this code needs some serious love and I'm not surprised it had bugs in it...

But hindsight is 20/20 and I merged it myself and all that, so I'm not really 
complaining - but let's not repeat the mistake, ok?

Thanks,

	Ingo

Powered by blists - more mailing lists