Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1183939 > unrolled thread

4.2-rc2: early boot memory corruption from FPU rework

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  4.2-rc2: early boot memory corruption from FPU rework Dave Hansen <dave.hansen@linux.intel.com> - 2015-07-14 21:50 +0200
    Re: 4.2-rc2: early boot memory corruption from FPU rework "H. Peter Anvin" <hpa@zytor.com> - 2015-07-15 03:30 +0200
    Re: 4.2-rc2: early boot memory corruption from FPU rework Ingo Molnar <mingo@kernel.org> - 2015-07-15 13:10 +0200
      [REGRESSION] 4.2-rc2: early boot memory corruption from FPU rework Dave Hansen <dave.hansen@linux.intel.com> - 2015-07-16 02:40 +0200
        Re: [REGRESSION] 4.2-rc2: early boot memory corruption from FPU rework Linus Torvalds <torvalds@linux-foundation.org> - 2015-07-16 04:40 +0200
        Re: [REGRESSION] 4.2-rc2: early boot memory corruption from FPU rework Linus Torvalds <torvalds@linux-foundation.org> - 2015-07-16 05:00 +0200
        Re: [REGRESSION] 4.2-rc2: early boot memory corruption from FPU  rework Ingo Molnar <mingo@kernel.org> - 2015-07-17 09:50 +0200
          Re: [REGRESSION] 4.2-rc2: early boot memory corruption from FPU  rework Ingo Molnar <mingo@kernel.org> - 2015-07-17 11:00 +0200

#1183939 — 4.2-rc2: early boot memory corruption from FPU rework

On 05/05/2015 10:49 AM, Ingo Molnar wrote:
> @@ -574,12 +573,10 @@ static void setup_init_fpu_buf(void)
>  	on_boot_cpu = 0;
>  
>  	/*
> -	 * Setup init_xstate_buf to represent the init state of
> +	 * Setup init_xstate_ctx to represent the init state of
>  	 * all the features managed by the xsave
>  	 */
> -	init_xstate_buf = alloc_bootmem_align(xstate_size,
> -					      __alignof__(struct xsave_struct));
> -	fx_finit(&init_xstate_buf->i387);
> +	fx_finit(&init_xstate_ctx.i387);

This is causing memory corruption in 4.2-rc2.

We do not know the size of the 'init_xstate_buf' before we boot.  It's
completely enumerated in CPUID leaves but it is not static by any means.
 This commit when applied (3e5e126774) tries to replace the dynamic
allocation with a static one.  When we do the first 'xrstor' (in
copy_xregs_to_kernel_booting()) it overruns init_fpstate and corrupts
the next chunk of memory (which is xfeatures_mask in my case).

I'm seeing this on a system with states not represented in
XSTATE_RESERVE (XSTATE_ZMM_Hi256 / XSTATE_OPMASK / XSTATE_Hi16_ZMM).
The systems affected are not widely available, but this is something
that we absolutely do not want to see regress.

This bug could also occur if a future CPU decided to change the amount
of storage allocated for a given xstate feature (which would be
architecturally OK).

According to the commit:

>     This removes the last bootmem allocation from the FPU init path, allowing
>     it to be called earlier in the boot sequence.

so we can't easily just revert this, although I'm not 100% that this is
before bootmem is availalble.

This patch works around the problem, btw:

	https://www.sr71.net/~dave/intel/bloat-xsave-gunk-2.patch

One curiosity here is that the bisect for this actually turned up the
patch that disables 'XSAVES' support.  When we used 'XSAVES' and the
"compacted" format, we managed to fit in to the buffer and things worked
(accidentally).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1184176

Actually we could statically associate a biggerbuffer based on the XCR0 features we support.  That would preclude dynamic enabling and really just adds complexity for no good reason.

On July 14, 2015 12:46:17 PM PDT, Dave Hansen <dave.hansen@linux.intel.com> wrote:
>On 05/05/2015 10:49 AM, Ingo Molnar wrote:
>> @@ -574,12 +573,10 @@ static void setup_init_fpu_buf(void)
>>  	on_boot_cpu = 0;
>>  
>>  	/*
>> -	 * Setup init_xstate_buf to represent the init state of
>> +	 * Setup init_xstate_ctx to represent the init state of
>>  	 * all the features managed by the xsave
>>  	 */
>> -	init_xstate_buf = alloc_bootmem_align(xstate_size,
>> -					      __alignof__(struct xsave_struct));
>> -	fx_finit(&init_xstate_buf->i387);
>> +	fx_finit(&init_xstate_ctx.i387);
>
>This is causing memory corruption in 4.2-rc2.
>
>We do not know the size of the 'init_xstate_buf' before we boot.  It's
>completely enumerated in CPUID leaves but it is not static by any
>means.
> This commit when applied (3e5e126774) tries to replace the dynamic
>allocation with a static one.  When we do the first 'xrstor' (in
>copy_xregs_to_kernel_booting()) it overruns init_fpstate and corrupts
>the next chunk of memory (which is xfeatures_mask in my case).
>
>I'm seeing this on a system with states not represented in
>XSTATE_RESERVE (XSTATE_ZMM_Hi256 / XSTATE_OPMASK / XSTATE_Hi16_ZMM).
>The systems affected are not widely available, but this is something
>that we absolutely do not want to see regress.
>
>This bug could also occur if a future CPU decided to change the amount
>of storage allocated for a given xstate feature (which would be
>architecturally OK).
>
>According to the commit:
>
>>     This removes the last bootmem allocation from the FPU init path,
>allowing
>>     it to be called earlier in the boot sequence.
>
>so we can't easily just revert this, although I'm not 100% that this is
>before bootmem is availalble.
>
>This patch works around the problem, btw:
>
>	https://www.sr71.net/~dave/intel/bloat-xsave-gunk-2.patch
>
>One curiosity here is that the bisect for this actually turned up the
>patch that disables 'XSAVES' support.  When we used 'XSAVES' and the
>"compacted" format, we managed to fit in to the buffer and things
>worked
>(accidentally).

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1184688

* Dave Hansen <dave.hansen@linux.intel.com> wrote:

> On 05/05/2015 10:49 AM, Ingo Molnar wrote:
> > @@ -574,12 +573,10 @@ static void setup_init_fpu_buf(void)
> >  	on_boot_cpu = 0;
> >  
> >  	/*
> > -	 * Setup init_xstate_buf to represent the init state of
> > +	 * Setup init_xstate_ctx to represent the init state of
> >  	 * all the features managed by the xsave
> >  	 */
> > -	init_xstate_buf = alloc_bootmem_align(xstate_size,
> > -					      __alignof__(struct xsave_struct));
> > -	fx_finit(&init_xstate_buf->i387);
> > +	fx_finit(&init_xstate_ctx.i387);
> 
> This is causing memory corruption in 4.2-rc2.
> 
> We do not know the size of the 'init_xstate_buf' before we boot.  It's
> completely enumerated in CPUID leaves but it is not static by any means.
>  This commit when applied (3e5e126774) tries to replace the dynamic
> allocation with a static one.  When we do the first 'xrstor' (in
> copy_xregs_to_kernel_booting()) it overruns init_fpstate and corrupts
> the next chunk of memory (which is xfeatures_mask in my case).
> 
> I'm seeing this on a system with states not represented in
> XSTATE_RESERVE (XSTATE_ZMM_Hi256 / XSTATE_OPMASK / XSTATE_Hi16_ZMM).
> The systems affected are not widely available, but this is something
> that we absolutely do not want to see regress.
> 
> This bug could also occur if a future CPU decided to change the amount
> of storage allocated for a given xstate feature (which would be
> architecturally OK).
> 
> According to the commit:
> 
> >     This removes the last bootmem allocation from the FPU init path, allowing
> >     it to be called earlier in the boot sequence.
> 
> so we can't easily just revert this, although I'm not 100% that this is
> before bootmem is availalble.
> 
> This patch works around the problem, btw:
> 
> 	https://www.sr71.net/~dave/intel/bloat-xsave-gunk-2.patch

Yeah, so I got this prototype hardware boot crash reported in private mail and 
decoded it and after some debugging I suggested the +PAGE_SIZE hack - possibly you 
got that hack from the same person?

My suggestion was to solve this properly: if we list xstate features as supported 
then we should size their max size correctly. The AVX bits are currently not 
properly enumerated and sized - and I refuse to add feature support to the kernel 
where per task CPU state fields that the kernel saves/restores are opaque...

So please add proper AVX512 support structures to fpu/types.h and size 
XSTATE_RESERVE correctly - or alternatively we can remove the current incomplete 
AVX512 bits.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1185151 — [REGRESSION] 4.2-rc2: early boot memory corruption from FPU rework

[Multipart message — attachments visible in raw view] — view raw

On 07/15/2015 04:07 AM, Ingo Molnar wrote:
> * Dave Hansen <dave.hansen@linux.intel.com> wrote:
>>>  	/*
>>> -	 * Setup init_xstate_buf to represent the init state of
>>> +	 * Setup init_xstate_ctx to represent the init state of
>>>  	 * all the features managed by the xsave
>>>  	 */
>>> -	init_xstate_buf = alloc_bootmem_align(xstate_size,
>>> -					      __alignof__(struct xsave_struct));
>>> -	fx_finit(&init_xstate_buf->i387);
>>> +	fx_finit(&init_xstate_ctx.i387);
>>
>> This is causing memory corruption in 4.2-rc2.
...
>> This patch works around the problem, btw:
>>
>> 	https://www.sr71.net/~dave/intel/bloat-xsave-gunk-2.patch
> 
> Yeah, so I got this prototype hardware boot crash reported in private mail and 
> decoded it and after some debugging I suggested the +PAGE_SIZE hack - possibly you 
> got that hack from the same person?

Nope, I came up with that gem of a patch all on my own.

I also wouldn't characterize this as prototype hardware.  There are
obviously plenty of folks depending on mainline to boot and function on
hardware that has AVX-512 support.  That's why two different Intel folks
came to you independently.

> My suggestion was to solve this properly: if we list xstate features as supported 
> then we should size their max size correctly. The AVX bits are currently not 
> properly enumerated and sized - and I refuse to add feature support to the kernel 
> where per task CPU state fields that the kernel saves/restores are opaque...

We might know the size and composition of the individual components, but
we do not know the size of the buffer.  Different implementations of a
given feature are quite free to have different data stored in the
buffer, or even to rearrange or pad it.  That's why the sizes are not
explicitly called out by the architecture and why we enumerated them
before your patch that caused this regression.

The component itself may not be opaque, but the size of the *buffer* is
not a simple sum of the component sizes.  Here's a real-world example:

[    0.000000] x86/fpu: xstate_offset[2]: 0240, xstate_sizes[2]: 0100
[    0.000000] x86/fpu: xstate_offset[3]: 03c0, xstate_sizes[3]: 0040

Notice that component 3 is not at 0x240+0x100.  This means our existing
init_xstate_size(), and why any attempt to staticlly-size the buffer is
broken.

I understand why you were misled by it, but the old "xsave_hdr_struct"
was wrong.  Fenghua even posted patches to remove it before the FPU
rework (you were cc'd):

	https://lkml.org/lkml/2015/4/18/164

> So please add proper AVX512 support structures to fpu/types.h and size 
> XSTATE_RESERVE correctly - or alternatively we can remove the current incomplete 
> AVX512 bits.

The old code sized the buffer in a fully architectural way and it
worked.  The CPU *tells* you how much memory the 'xsave' instruction is
going to scribble on.  The new code just merrily calls it and let it
scribble away.  This is as clear-cut a regression as I've ever seen.

The least we can do is detect that the kernel undersized the buffer and
disable support for the features that do not fit.  A very lightly tested
patch to do that is attached.  I'm not super eager to put that in to an
-rc2 kernel though.

[toc] | [prev] | [next] | [standalone]

#1185448 — Re: [REGRESSION] 4.2-rc2: early boot memory corruption from FPU rework

On Wed, Jul 15, 2015 at 5:34 PM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
>
> The old code sized the buffer in a fully architectural way and it
> worked.  The CPU *tells* you how much memory the 'xsave' instruction is
> going to scribble on.  The new code just merrily calls it and let it
> scribble away.  This is as clear-cut a regression as I've ever seen.

Yes, I think we'll need to revert it, or do something else drastic
like make that initial fp state allocation *much* bigger and then have
a "disable xsaves if if it's still not big enough".

setup_xstate_features() should be able to easily just say "this was
the maximum offset+size we saw", and we can take that to either do a
proper allocation, or verify that the static allocation is indeed big
enough.

Apparently a straight revert doesn't work, if only because things in
that area have been renamed very aggressively (both files and
functions and variables). Ingo?

            Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1185455 — Re: [REGRESSION] 4.2-rc2: early boot memory corruption from FPU rework

On Wed, Jul 15, 2015 at 5:34 PM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
>
> I understand why you were misled by it, but the old "xsave_hdr_struct"
> was wrong.  Fenghua even posted patches to remove it before the FPU
> rework (you were cc'd):
>
>         https://lkml.org/lkml/2015/4/18/164

Oh, and that patch looks like a good idea.

I wish there was some way to make sure sizeof() fail on it so that
we'd enforce that nobody allocates that thing as-is. I had this dim
memory that an unsized array at the end would do that, but I was
clearly wrong. It's just the array itself you can't do sizeof on, not
the structure that contains it. Is there some magic trick that I'm
forgetting?

                      Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186535 — Re: [REGRESSION] 4.2-rc2: early boot memory corruption from FPU rework

* Dave Hansen <dave.hansen@linux.intel.com> wrote:

> On 07/15/2015 04:07 AM, Ingo Molnar wrote:
> > * Dave Hansen <dave.hansen@linux.intel.com> wrote:
> >>>  	/*
> >>> -	 * Setup init_xstate_buf to represent the init state of
> >>> +	 * Setup init_xstate_ctx to represent the init state of
> >>>  	 * all the features managed by the xsave
> >>>  	 */
> >>> -	init_xstate_buf = alloc_bootmem_align(xstate_size,
> >>> -					      __alignof__(struct xsave_struct));
> >>> -	fx_finit(&init_xstate_buf->i387);
> >>> +	fx_finit(&init_xstate_ctx.i387);
> >>
> >> This is causing memory corruption in 4.2-rc2.
> ...
> >> This patch works around the problem, btw:
> >>
> >> 	https://www.sr71.net/~dave/intel/bloat-xsave-gunk-2.patch
> > 
> > Yeah, so I got this prototype hardware boot crash reported in private mail and 
> > decoded it and after some debugging I suggested the +PAGE_SIZE hack - possibly you 
> > got that hack from the same person?
> 
> Nope, I came up with that gem of a patch all on my own.

:)

> I also wouldn't characterize this as prototype hardware.  There are obviously 
> plenty of folks depending on mainline to boot and function on hardware that has 
> AVX-512 support.  That's why two different Intel folks came to you 
> independently.

Yeah, so I treat it as a regression even if it's unreleased hw, what matters to 
regressions is number of people affected, plus that the kernel should work for a 
reasonable set of future hardware as well, without much trouble.

Just curious: does any released hardware have AVX-512? I went by Wikipedia, which 
seems to list pre-release hw:

  https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512

    Intel
        Xeon Phi Knights Landing: AVX-512 F, CDI, PFI and ERI[1] in 2015[6]
        Xeon Skylake: AVX-512 F, CDI, VL, BW, and DQ[7] in 2015[8]
        Cannonlake (speculation)

> > My suggestion was to solve this properly: if we list xstate features as 
> > supported then we should size their max size correctly. The AVX bits are 
> > currently not properly enumerated and sized - and I refuse to add feature 
> > support to the kernel where per task CPU state fields that the kernel 
> > saves/restores are opaque...
> 
> We might know the size and composition of the individual components, but we do 
> not know the size of the buffer.  Different implementations of a given feature 
> are quite free to have different data stored in the buffer, or even to rearrange 
> or pad it.  That's why the sizes are not explicitly called out by the 
> architecture and why we enumerated them before your patch that caused this 
> regression.

But we _have_ to know their structure and layout of the XSAVE context for any 
reasonable ptrace and signal frame support. Can you set/get AVX-512 registers via 
ptrace? MPX state?

That's one of the reasons why I absolutely hate how this 'opaque per task CPU 
context blob' concept snuck into the x86 code via the XSAVE patches without proper 
enumeration of the data structures, sorry...

It makes it way too easy to 'support' CPU features without actually doing a good 
job of it - and in fact it makes certain reasonable things impossible or very, 
very hard, which makes me nervous.

But we'll fix the boot regression, no argument about that!

> The component itself may not be opaque, but the size of the *buffer* is not a 
> simple sum of the component sizes.  Here's a real-world example:
> 
> [    0.000000] x86/fpu: xstate_offset[2]: 0240, xstate_sizes[2]: 0100
> [    0.000000] x86/fpu: xstate_offset[3]: 03c0, xstate_sizes[3]: 0040
> 
> Notice that component 3 is not at 0x240+0x100.  This means our existing 
> init_xstate_size(), and why any attempt to staticlly-size the buffer is broken.
> 
> I understand why you were misled by it, but the old "xsave_hdr_struct" was 
> wrong.  Fenghua even posted patches to remove it before the FPU rework (you were 
> cc'd):
> 
> 	https://lkml.org/lkml/2015/4/18/164

Yeah, so I thought the worst bugs were fixed and that these would re-emerge on top 
of the new code.

Whether we have a static limit or not is orthogonal to the issue of sizing it 
properly - and the plan was to have a dynamic context area in any case.

> > So please add proper AVX512 support structures to fpu/types.h and size 
> > XSTATE_RESERVE correctly - or alternatively we can remove the current 
> > incomplete AVX512 bits.
> 
> The old code sized the buffer in a fully architectural way and it worked.  The 
> CPU *tells* you how much memory the 'xsave' instruction is going to scribble on.  
> The new code just merrily calls it and let it scribble away.  This is as 
> clear-cut a regression as I've ever seen.

This is a regression which we'll fix, but the 'old' dynamic code clearly did not 
work for a long time, I'm sure you still remember my attempt at addressing the 
worst fallout in:

  e88221c50cad ("x86/fpu: Disable XSAVES* support for now")

Those kinds of totally non-working aspects were what made me nervous about the 
opaque data structure aspect.

Because we can have dynamic sizing of the context area and non-opaque data 
structures.

> The least we can do is detect that the kernel undersized the buffer and disable 
> support for the features that do not fit.  A very lightly tested patch to do 
> that is attached.  I'm not super eager to put that in to an -rc2 kernel though.

Ok, this approach looks good to me as an interim fix. I'll give it a whirl on 
older hardware. I agree with you that it needs to be sized dynamically.

> This came out a lot more complicated than I would have liked.
> 
> Instead of simply enabling all of the XSAVE features that we both know about and 
> the CPU supports, we have to be careful to not overflow our buffer in 
> 'init_fpstate.xsave'.

Yeah, and this can be fixed separately and on top of your fix: my plan during the 
FPU rework was to move the context area to the end of task_struct and size it 
dynamically.

This needs some (very minor) changes to kernel/fork.c to allow an architecture to 
determine the full task_struct size dynamically - but looks very doable and clean. 
Wanna try this, or should I?

> To do this, we enable each XSAVE feature and then ask the CPU how large that 
> makes the buffer.  If we would overflow the buffer we allocated, we turn off the 
> feature.
> 
> This means that no matter what the CPU does, we will not corrupt random memory 
> like we do before this patch.  It also means that we can fall back in a way 
> which cripples the system the least.

Yes, agreed.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186586 — Re: [REGRESSION] 4.2-rc2: early boot memory corruption from FPU rework

* Ingo Molnar <mingo@kernel.org> wrote:

> > The least we can do is detect that the kernel undersized the buffer and 
> > disable support for the features that do not fit.  A very lightly tested patch 
> > to do that is attached.  I'm not super eager to put that in to an -rc2 kernel 
> > though.
> 
> Ok, this approach looks good to me as an interim fix. I'll give it a whirl on 
> older hardware. I agree with you that it needs to be sized dynamically.

Hm, so this patch crashed the boot of 2 out of 3 systems that I tried :-/

But it does not really matter, as I think the dynamic allocation is the right fix 
in any case (your last patch), so this patch should be moot.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web