* r...@redhat.com <r...@redhat.com> wrote:
> From: Rik van Riel <r...@redhat.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 <r...@redhat.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
