[PATCH] x86/fpu: set the xcomp_bv when we fake up a XSAVES area

[PATCH] x86/fpu: set the xcomp_bv when we fake up a XSAVES area

[Kevin Hao]

I got the following calltrace on a Apollo Lake SoC with 32bit kernel.
  WARNING: CPU: 2 PID: 261 at arch/x86/include/asm/fpu/internal.h:363 fpu__restore+0x1f5/0x260
  Modules linked in:
  CPU: 2 PID: 261 Comm: check_hostname. Not tainted 4.10.0-rc4-next-20170120 #90
  Hardware name: Intel Corp. Broxton P/NOTEBOOK, BIOS APLIRVPA.X64.0138.B35.1608091058 08/09/2016
  Call Trace:
   dump_stack+0x47/0x5f
   __warn+0xea/0x110
   ? fpu__restore+0x1f5/0x260
   warn_slowpath_null+0x2a/0x30
   fpu__restore+0x1f5/0x260
   __fpu__restore_sig+0x165/0x6b0
   fpu__restore_sig+0x2f/0x50
   restore_sigcontext.isra.9+0xe0/0xf0
   sys_sigreturn+0xaa/0xf0
   do_int80_syscall_32+0x59/0xb0
   entry_INT80_32+0x2a/0x2a
  EIP: 0xb77acc61
  EFLAGS: 00000246 CPU: 2
  EAX: 00000000 EBX: 00000003 ECX: 08151d38 EDX: 00000000
  ESI: bfa9ce20 EDI: 08151d38 EBP: 0000000c ESP: bfa9cdbc
   DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b

The reason is that a #GP occurs when executing XRSTORS. The root cause
is that we forget to set the xcomp_bv when we fake up the XSAVES area
in function copyin_to_xsaves().

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 arch/x86/kernel/fpu/xstate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 35f7024aace5..2c0df2681481 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1071,6 +1071,8 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
 	 * Add back in the features that came in from userspace:
 	 */
 	xsave->header.xfeatures |= xfeatures;
+	xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
+				 xsave->header.xfeatures;
 
 	return 0;
 }
-- 
2.9.3

[tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[tip-bot for Kevin Hao]

Commit-ID:  5fa356458b5c918bdf8307b070a3d74bc015d910
Gitweb:     http://git.kernel.org/tip/5fa356458b5c918bdf8307b070a3d74bc015d910
Author:     Kevin Hao <haokexin@gmail.com>
AuthorDate: Sun, 22 Jan 2017 16:50:23 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 23 Jan 2017 09:03:03 +0100

x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

I got the following calltrace on a Apollo Lake SoC with 32-bit kernel:

  WARNING: CPU: 2 PID: 261 at arch/x86/include/asm/fpu/internal.h:363 fpu__restore+0x1f5/0x260
  [...]
  Hardware name: Intel Corp. Broxton P/NOTEBOOK, BIOS APLIRVPA.X64.0138.B35.1608091058 08/09/2016
  Call Trace:
   dump_stack()
   __warn()
   ? fpu__restore()
   warn_slowpath_null()
   fpu__restore()
   __fpu__restore_sig()
   fpu__restore_sig()
   restore_sigcontext.isra.9()
   sys_sigreturn()
   do_int80_syscall_32()
   entry_INT80_32()

The reason is that a #GP occurs when executing XRSTORS. The root cause
is that we forget to set the xcomp_bv when we fake up the XSAVES area
in the copyin_to_xsaves() function.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Link: http://lkml.kernel.org/r/1485075023-30161-1-git-send-email-haokexin@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/xstate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 1d77704..e287b90 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1070,6 +1070,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
 	 * Add back in the features that came in from userspace:
 	 */
 	xsave->header.xfeatures |= xfeatures;
+	xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | xsave->header.xfeatures;
 
 	return 0;
 }

[tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[tip-bot for Kevin Hao]

Commit-ID:  4c833368f0bf748d4147bf301b1f95bc8eccb3c0
Gitweb:     http://git.kernel.org/tip/4c833368f0bf748d4147bf301b1f95bc8eccb3c0
Author:     Kevin Hao <haokexin@gmail.com>
AuthorDate: Sun, 22 Jan 2017 16:50:23 +0800
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 23 Jan 2017 10:40:18 +0100

x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

I got the following calltrace on a Apollo Lake SoC with 32-bit kernel:

  WARNING: CPU: 2 PID: 261 at arch/x86/include/asm/fpu/internal.h:363 fpu__restore+0x1f5/0x260
  [...]
  Hardware name: Intel Corp. Broxton P/NOTEBOOK, BIOS APLIRVPA.X64.0138.B35.1608091058 08/09/2016
  Call Trace:
   dump_stack()
   __warn()
   ? fpu__restore()
   warn_slowpath_null()
   fpu__restore()
   __fpu__restore_sig()
   fpu__restore_sig()
   restore_sigcontext.isra.9()
   sys_sigreturn()
   do_int80_syscall_32()
   entry_INT80_32()

The reason is that a #GP occurs when executing XRSTORS. The root cause
is that we forget to set the xcomp_bv when we fake up the XSAVES area
in the copyin_to_xsaves() function.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Link: http://lkml.kernel.org/r/1485075023-30161-1-git-send-email-haokexin@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/fpu/xstate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 1d77704..e287b90 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1070,6 +1070,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
 	 * Add back in the features that came in from userspace:
 	 */
 	xsave->header.xfeatures |= xfeatures;
+	xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | xsave->header.xfeatures;
 
 	return 0;
 }

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Dave Hansen]

On 01/23/2017 12:28 AM, tip-bot for Kevin Hao wrote:
> x86/fpu: Set the xcomp_bv when we fake up a XSAVES area
> 
> I got the following calltrace on a Apollo Lake SoC with 32-bit kernel:

...
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1070,6 +1070,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
>  	 * Add back in the features that came in from userspace:
>  	 */
>  	xsave->header.xfeatures |= xfeatures;
> +	xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | xsave->header.xfeatures;
>  
>  	return 0;
>  }

First of all, thanks for finding this!  As you found, the 32-bit XSAVES
code is a bit light on testing.

I don't doubt that we have a code path that was screwed up like this,
but I don't think this is the right fix.

The kernel xsave buffer should *ALWAYS* have the
XCOMP_BV_COMPACTED_FORMAT bit set.  It should have been set before the
copyin and it should be set when it's finished.

The best fix here would be not to paper over the issue in the copy
function but find where it got clobbered, or where some initialization
code failed to set it.

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Yu-cheng Yu]

On Mon, Jan 23, 2017 at 07:36:20AM -0800, Dave Hansen wrote:
> The kernel xsave buffer should *ALWAYS* have the
> XCOMP_BV_COMPACTED_FORMAT bit set.  It should have been set before the
> copyin and it should be set when it's finished.
> 
> The best fix here would be not to paper over the issue in the copy
> function but find where it got clobbered, or where some initialization
> code failed to set it.

Someone else reported different issues from the same bug and a different
patch was just tested OK this morning.  I think that adding xfeatures bits
to xcomp_bv should have been done in fpstate_init().

Also, in copy_init_fpstate_to_fpregs(), we do:

	copy_kernel_to_xregs(&init_fpstate.xsave, -1).

That (-1) could mean (0) because the parameters are declared as:

	copy_kernel_to_xregs(struct xregs_state *, u64)

Yu-cheng

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Dave Hansen]

On 01/23/2017 08:55 AM, Yu-cheng Yu wrote:
> On Mon, Jan 23, 2017 at 07:36:20AM -0800, Dave Hansen wrote:
>> The kernel xsave buffer should *ALWAYS* have the
>> XCOMP_BV_COMPACTED_FORMAT bit set.  It should have been set before the
>> copyin and it should be set when it's finished.
>>
>> The best fix here would be not to paper over the issue in the copy
>> function but find where it got clobbered, or where some initialization
>> code failed to set it.
> 
> Someone else reported different issues from the same bug and a different
> patch was just tested OK this morning.  I think that adding xfeatures bits
> to xcomp_bv should have been done in fpstate_init().

Right.  So where did it get cleared out?

> Also, in copy_init_fpstate_to_fpregs(), we do:
> 
> 	copy_kernel_to_xregs(&init_fpstate.xsave, -1).
> 
> That (-1) could mean (0) because the parameters are declared as:
> 
> 	copy_kernel_to_xregs(struct xregs_state *, u64)

I'm not sure what you're saying.  -1 just means "all 1's" when cast to
an unsigned type.  This shouldn't case any problems.

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Yu-cheng Yu]

On Mon, Jan 23, 2017 at 09:23:06AM -0800, Dave Hansen wrote:
> On 01/23/2017 08:55 AM, Yu-cheng Yu wrote:
> >> The best fix here would be not to paper over the issue in the copy
> >> function but find where it got clobbered, or where some initialization
> >> code failed to set it.
> > 
> > Someone else reported different issues from the same bug and a different
> > patch was just tested OK this morning.  I think that adding xfeatures bits
> > to xcomp_bv should have been done in fpstate_init().
> 
> Right.  So where did it get cleared out?

It is not set until a task triggers XSAVES.  We did not set it in fpstate_init()
because there is no valid data at the time.  The problem happens when Linux 
copies data to the XSAVES area, like we see here; the kernel is not expected
to change XSAVES format (xcomp_bv) but xcomp_bv is still blank (except bit 63). 

Because XSAVES format is not changed after boot time and xcomp_bv does not affect
INIT optimization, why don't we fix the problem in fpstate_init()?  

Yu-cheng
 

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Dave Hansen]

On 01/23/2017 12:57 PM, Yu-cheng Yu wrote:
> On Mon, Jan 23, 2017 at 09:23:06AM -0800, Dave Hansen wrote:
>> On 01/23/2017 08:55 AM, Yu-cheng Yu wrote:
>>>> The best fix here would be not to paper over the issue in the copy
>>>> function but find where it got clobbered, or where some initialization
>>>> code failed to set it.
>>>
>>> Someone else reported different issues from the same bug and a different
>>> patch was just tested OK this morning.  I think that adding xfeatures bits
>>> to xcomp_bv should have been done in fpstate_init().
>>
>> Right.  So where did it get cleared out?
> 
> It is not set until a task triggers XSAVES.  We did not set it in fpstate_init()
> because there is no valid data at the time.

The code is:

> void fpstate_init(union fpregs_state *state)
> {
>         if (!static_cpu_has(X86_FEATURE_FPU)) {
>                 fpstate_init_soft(&state->soft);
>                 return;
>         }
> 
>         memset(state, 0, fpu_kernel_xstate_size);
> 
>         /*
>          * XRSTORS requires that this bit is set in xcomp_bv, or
>          * it will #GP. Make sure it is replaced after the memset().
>          */
>         if (static_cpu_has(X86_FEATURE_XSAVES))
>                 state->xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT;

That seems to set it unconditionally.  What am I missing?

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Yu-cheng Yu]

On Mon, Jan 23, 2017 at 01:10:20PM -0800, Dave Hansen wrote:
> The code is:
> 
> > void fpstate_init(union fpregs_state *state)
> > {
> >         if (!static_cpu_has(X86_FEATURE_FPU)) {
> >                 fpstate_init_soft(&state->soft);
> >                 return;
> >         }
> > 
> >         memset(state, 0, fpu_kernel_xstate_size);
> > 
> >         /*
> >          * XRSTORS requires that this bit is set in xcomp_bv, or
> >          * it will #GP. Make sure it is replaced after the memset().
> >          */
> >         if (static_cpu_has(X86_FEATURE_XSAVES))
> >                 state->xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT;
> 
> That seems to set it unconditionally.  What am I missing?

The fix I am proposing is...

	state->xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
				       xfeatures_mask;

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Dave Hansen]

On 01/23/2017 01:16 PM, Yu-cheng Yu wrote:
> On Mon, Jan 23, 2017 at 01:10:20PM -0800, Dave Hansen wrote:
>> The code is:
>>
>>> void fpstate_init(union fpregs_state *state)
>>> {
>>>         if (!static_cpu_has(X86_FEATURE_FPU)) {
>>>                 fpstate_init_soft(&state->soft);
>>>                 return;
>>>         }
>>>
>>>         memset(state, 0, fpu_kernel_xstate_size);
>>>
>>>         /*
>>>          * XRSTORS requires that this bit is set in xcomp_bv, or
>>>          * it will #GP. Make sure it is replaced after the memset().
>>>          */
>>>         if (static_cpu_has(X86_FEATURE_XSAVES))
>>>                 state->xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT;
>>
>> That seems to set it unconditionally.  What am I missing?
> 
> The fix I am proposing is...
> 
> 	state->xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
> 				       xfeatures_mask;

Ahh, that makes sense.  That does indeed look like a bug in fpstate_init().

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On Mon, Jan 23, 2017 at 01:16:40PM -0800, Yu-cheng Yu wrote:
> On Mon, Jan 23, 2017 at 01:10:20PM -0800, Dave Hansen wrote:
> > The code is:
> > 
> > > void fpstate_init(union fpregs_state *state)
> > > {
> > >         if (!static_cpu_has(X86_FEATURE_FPU)) {
> > >                 fpstate_init_soft(&state->soft);
> > >                 return;
> > >         }
> > > 
> > >         memset(state, 0, fpu_kernel_xstate_size);
> > > 
> > >         /*
> > >          * XRSTORS requires that this bit is set in xcomp_bv, or
> > >          * it will #GP. Make sure it is replaced after the memset().
> > >          */
> > >         if (static_cpu_has(X86_FEATURE_XSAVES))
> > >                 state->xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT;
> > 
> > That seems to set it unconditionally.  What am I missing?
> 
> The fix I am proposing is...
> 
> 	state->xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
> 				       xfeatures_mask;

Actually I thought about this change before I made this patch, but I don't this
is the right fix. It is always error prone to init the xcomp_bv to all the
supported feature. In case like copyin_to_xsaves(), it is possible that the
features which should be set in xcomp_bv do not equal to all the supported
features. Please see the following codes in copyin_to_xsaves():
	/*
	 * The state that came in from userspace was user-state only.
	 * Mask all the user states out of 'xfeatures':
	 */
	xsave->header.xfeatures &= XFEATURE_MASK_SUPERVISOR;

	/*
	 * Add back in the features that came in from userspace:
	 */
	xsave->header.xfeatures |= xfeatures;

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Dave Hansen]

>> The fix I am proposing is...
>>
>> 	state->xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
>> 				       xfeatures_mask;
> 
> Actually I thought about this change before I made this patch, but I don't this
> is the right fix. It is always error prone to init the xcomp_bv to all the
> supported feature. In case like copyin_to_xsaves(), it is possible that the
> features which should be set in xcomp_bv do not equal to all the supported
> features. Please see the following codes in copyin_to_xsaves():
> 	/*
> 	 * The state that came in from userspace was user-state only.
> 	 * Mask all the user states out of 'xfeatures':
> 	 */
> 	xsave->header.xfeatures &= XFEATURE_MASK_SUPERVISOR;
> 
> 	/*
> 	 * Add back in the features that came in from userspace:
> 	 */
> 	xsave->header.xfeatures |= xfeatures;

Hi Kevin,

I think you may be confusing 'xfeatures' with 'xcomp_bv'.  xfeatures
tells you what features are present in the buffer while xcomp_bv tells
you what *format* the buffer is in.

Userspace never dictates the *format* of the kernel buffer, only the
contents of each state.  So, it makes sense that the copyin code would
not (and should not) modify xcomp_bv.

We ensure that xcomp_bv has all supported states set all the time, or
we're *supposed* to.

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]

On Mon, Jan 23, 2017 at 04:53:25PM -0800, Dave Hansen wrote:
> >> The fix I am proposing is...
> >>
> >> 	state->xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
> >> 				       xfeatures_mask;
> > 
> > Actually I thought about this change before I made this patch, but I don't this
> > is the right fix. It is always error prone to init the xcomp_bv to all the
> > supported feature. In case like copyin_to_xsaves(), it is possible that the
> > features which should be set in xcomp_bv do not equal to all the supported
> > features. Please see the following codes in copyin_to_xsaves():
> > 	/*
> > 	 * The state that came in from userspace was user-state only.
> > 	 * Mask all the user states out of 'xfeatures':
> > 	 */
> > 	xsave->header.xfeatures &= XFEATURE_MASK_SUPERVISOR;
> > 
> > 	/*
> > 	 * Add back in the features that came in from userspace:
> > 	 */
> > 	xsave->header.xfeatures |= xfeatures;
> 
> Hi Kevin,
> 
> I think you may be confusing 'xfeatures' with 'xcomp_bv'.  xfeatures
> tells you what features are present in the buffer while xcomp_bv tells
> you what *format* the buffer is in.

According to the ISA manual, XSAVES also set the XCOMP_BV[62:0]. My code only
try to be compatible with what the cpu does when excuting XSAVES. The following
is quoted from 325462-sdm-vol-1-2abcd-3abcd.pdf.
  The XSAVES instructions sets bit 63 of the XCOMP_BV field of the XSAVE header while writing RFBM[62:0] to
  XCOMP_BV[62:0]. The XSAVES instruction does not write any part of the XSAVE header other than the XSTATE_BV
  and XCOMP_BV fields.

Thanks,
Kevin

> 
> Userspace never dictates the *format* of the kernel buffer, only the
> contents of each state.  So, it makes sense that the copyin code would
> not (and should not) modify xcomp_bv.
> 
> We ensure that xcomp_bv has all supported states set all the time, or
> we're *supposed* to.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Dave Hansen]

On 01/23/2017 05:50 PM, Kevin Hao wrote:
> According to the ISA manual, XSAVES also set the XCOMP_BV[62:0]. My code only
> try to be compatible with what the cpu does when excuting XSAVES. The following
> is quoted from 325462-sdm-vol-1-2abcd-3abcd.pdf.
>   The XSAVES instructions sets bit 63 of the XCOMP_BV field of the XSAVE header while writing RFBM[62:0] to
>   XCOMP_BV[62:0]. The XSAVES instruction does not write any part of the XSAVE header other than the XSTATE_BV
>   and XCOMP_BV fields.

What purpose does it serve to make copyin_to_xsaves() set that bit,
other than helping to hide bugs?

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

On Mon, Jan 23, 2017 at 06:01:10PM -0800, Dave Hansen wrote:
> On 01/23/2017 05:50 PM, Kevin Hao wrote:
> > According to the ISA manual, XSAVES also set the XCOMP_BV[62:0]. My code only
> > try to be compatible with what the cpu does when excuting XSAVES. The following
> > is quoted from 325462-sdm-vol-1-2abcd-3abcd.pdf.
> >   The XSAVES instructions sets bit 63 of the XCOMP_BV field of the XSAVE header while writing RFBM[62:0] to
> >   XCOMP_BV[62:0]. The XSAVES instruction does not write any part of the XSAVE header other than the XSTATE_BV
> >   and XCOMP_BV fields.
> 
> What purpose does it serve to make copyin_to_xsaves() set that bit,

We try to fake up a memory area which is supposed to be composed by XSAVES
instruction. My code is just trying to do what the XSAVES do.

> other than helping to hide bugs?

Why do you think it hide the bug? In contrast, I think my patch fixes what the
bug really is. The memory area we fake up is bug, we should fix it there.

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Dave Hansen]

On 01/23/2017 06:09 PM, Kevin Hao wrote:
> On Mon, Jan 23, 2017 at 06:01:10PM -0800, Dave Hansen wrote:
>> On 01/23/2017 05:50 PM, Kevin Hao wrote:
>>> According to the ISA manual, XSAVES also set the XCOMP_BV[62:0]. My code only
>>> try to be compatible with what the cpu does when excuting XSAVES. The following
>>> is quoted from 325462-sdm-vol-1-2abcd-3abcd.pdf.
>>>   The XSAVES instructions sets bit 63 of the XCOMP_BV field of the XSAVE header while writing RFBM[62:0] to
>>>   XCOMP_BV[62:0]. The XSAVES instruction does not write any part of the XSAVE header other than the XSTATE_BV
>>>   and XCOMP_BV fields.
>> What purpose does it serve to make copyin_to_xsaves() set that bit,
> We try to fake up a memory area which is supposed to be composed by XSAVES
> instruction. My code is just trying to do what the XSAVES do.

No. copyin_to_xsaves() copies data into an *existing* XSAVES-formatted
buffer.  If you want to change what it does, fine, but that's not what
it does or tries to do today.

>> other than helping to hide bugs?
> Why do you think it hide the bug? In contrast, I think my patch fixes what the
> bug really is. The memory area we fake up is bug, we should fix it there.

Yu-cheng found the bug.  That bug will probably manifest in other code
paths than copyin_to_xsaves().  If we did your patch, it would hide the
bug in those other code paths.

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 1961 bytes --]

On Mon, Jan 23, 2017 at 06:38:42PM -0800, Dave Hansen wrote:
> On 01/23/2017 06:09 PM, Kevin Hao wrote:
> > On Mon, Jan 23, 2017 at 06:01:10PM -0800, Dave Hansen wrote:
> >> On 01/23/2017 05:50 PM, Kevin Hao wrote:
> >>> According to the ISA manual, XSAVES also set the XCOMP_BV[62:0]. My code only
> >>> try to be compatible with what the cpu does when excuting XSAVES. The following
> >>> is quoted from 325462-sdm-vol-1-2abcd-3abcd.pdf.
> >>>   The XSAVES instructions sets bit 63 of the XCOMP_BV field of the XSAVE header while writing RFBM[62:0] to
> >>>   XCOMP_BV[62:0]. The XSAVES instruction does not write any part of the XSAVE header other than the XSTATE_BV
> >>>   and XCOMP_BV fields.
> >> What purpose does it serve to make copyin_to_xsaves() set that bit,
> > We try to fake up a memory area which is supposed to be composed by XSAVES
> > instruction. My code is just trying to do what the XSAVES do.
> 
> No. copyin_to_xsaves() copies data into an *existing* XSAVES-formatted
> buffer.  If you want to change what it does, fine, but that's not what
> it does or tries to do today.

No, I didn't change what the function copyin_to_xsaves() does. I just tried to
fix the bug in it.

> 
> >> other than helping to hide bugs?
> > Why do you think it hide the bug? In contrast, I think my patch fixes what the
> > bug really is. The memory area we fake up is bug, we should fix it there.
> 
> Yu-cheng found the bug.  That bug will probably manifest in other code
> paths than copyin_to_xsaves().  If we did your patch, it would hide the
> bug in those other code paths.

The XCOMP_BV[62:0] is supposed to be updated by XSAVEC/XSAVES instructions.
It should not be touched by the software in theory except some special cases
like what we do in copyin_to_xsaves(). Trying to init the XCOMP_BV[62:0] to some
assuming values in fpstate_init() should be error prone and just paper over the
real bug.

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Ingo Molnar]

* Kevin Hao <haokexin@gmail.com> wrote:

> > other than helping to hide bugs?
> 
> Why do you think it hide the bug? In contrast, I think my patch fixes what the 
> bug really is. The memory area we fake up is bug, we should fix it there.

The intention is to have a single FPU format set at bootup and xcomp_bv is 
essentially an invariant (constant) inherited by all tasks from early boot. In 
that sense setting xsave.header.xcomp_bv in copyin_to_xsaves() is misplaced.

So I combined the two commits: I kept your original fix but applied Yu-cheng Yu's 
patch that moves the initialization from copyin_to_xsaves() to fpstate_init().

Thanks,

	Ingo

Re: [tip:x86/urgent] x86/fpu: Set the xcomp_bv when we fake up a XSAVES area

[Dave Hansen]

On 01/23/2017 01:43 AM, tip-bot for Kevin Hao wrote:
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 1d77704..e287b90 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1070,6 +1070,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
>  	 * Add back in the features that came in from userspace:
>  	 */
>  	xsave->header.xfeatures |= xfeatures;
> +	xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | xsave->header.xfeatures;

FYI, this commit bit me today.  If userspace happens to have bits clear
in the 'xfeatures' field, this will *CLEAR* bits in xcomp_bv, changing
the format of the XSAVE buffer, and breaking anything that looks at the
buffer that doesn't use the instructions.

Yu-cheng's dffba9a31c commit removed this line and fixed it up, but this
might bite someone who is bisecting.