[PATCH] x86: re-add stack alignment check

[PATCH] x86: re-add stack alignment check

[Jan Beulich]

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

Commit 279840d5ea ("x86/boot: install trap handlers much earlier on
boot"), perhaps not really intentionally, removed this check. Add it
back,
- preventing it to trigger before any output is set up,
- accompanying it with a (weaker, due to its open coding of what
  get_stack_bottom() does) build time check.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
While not strictly needed for 4.8, I thought I'd still submit it as a
possible candidate.

--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -643,6 +643,11 @@ void load_system_tables(void)
 		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
 	};
 
+	/* Bottom-of-stack must be 16-byte aligned! */
+	BUILD_BUG_ON((sizeof(struct cpu_info) -
+		      offsetof(struct cpu_info, guest_cpu_user_regs.es)) & 0xf);
+	BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));
+
 	/* Main stack for interrupts/exceptions. */
 	tss->rsp0 = stack_bottom;
 	tss->bitmap = IOBMP_INVALID_OFFSET;




[-- Attachment #2: x86-stack-bottom-check.patch --]
[-- Type: text/plain, Size: 1035 bytes --]

x86: re-add stack alignment check

Commit 279840d5ea ("x86/boot: install trap handlers much earlier on
boot"), perhaps not really intentionally, removed this check. Add it
back,
- preventing it to trigger before any output is set up,
- accompanying it with a (weaker, due to its open coding of what
  get_stack_bottom() does) build time check.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
While not strictly needed for 4.8, I thought I'd still submit it as a
possible candidate.

--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -643,6 +643,11 @@ void load_system_tables(void)
 		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
 	};
 
+	/* Bottom-of-stack must be 16-byte aligned! */
+	BUILD_BUG_ON((sizeof(struct cpu_info) -
+		      offsetof(struct cpu_info, guest_cpu_user_regs.es)) & 0xf);
+	BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));
+
 	/* Main stack for interrupts/exceptions. */
 	tss->rsp0 = stack_bottom;
 	tss->bitmap = IOBMP_INVALID_OFFSET;

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: re-add stack alignment check

[Andrew Cooper]

On 14/11/16 13:25, Jan Beulich wrote:
> Commit 279840d5ea ("x86/boot: install trap handlers much earlier on
> boot"), perhaps not really intentionally, removed this check. Add it

No - that was deliberate.  The check isn't useful (see below).

> back,
> - preventing it to trigger before any output is set up,

"preventing it from triggering ..."

> - accompanying it with a (weaker, due to its open coding of what
>   get_stack_bottom() does) build time check.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> While not strictly needed for 4.8, I thought I'd still submit it as a
> possible candidate.
>
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -643,6 +643,11 @@ void load_system_tables(void)
>  		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>  	};
>  
> +	/* Bottom-of-stack must be 16-byte aligned! */
> +	BUILD_BUG_ON((sizeof(struct cpu_info) -
> +		      offsetof(struct cpu_info, guest_cpu_user_regs.es)) & 0xf);
> +	BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));

This will still triple fault the system if it triggers on an AP. 
Exceptions aren't hooked up yet.

The reason I dropped the check was that there was no way it was going to
fail on the BSP (because of the alignment of cpu0_stack) or APs (because
of the alloc_xenheap_pages(STACK_ORDER, memflags) allocation).

The BUILD_BUG_ON() is useful to retain, but I would suggest making
get_stack_bottom() a static inline and putting the BUILD_BUG_ON() there.

~Andrew

> +
>  	/* Main stack for interrupts/exceptions. */
>  	tss->rsp0 = stack_bottom;
>  	tss->bitmap = IOBMP_INVALID_OFFSET;
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: re-add stack alignment check

[Jan Beulich]

>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> wrote:
> On 14/11/16 13:25, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -643,6 +643,11 @@ void load_system_tables(void)
>>  		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>>  	};
>>  
>> +	/* Bottom-of-stack must be 16-byte aligned! */
>> +	BUILD_BUG_ON((sizeof(struct cpu_info) -
>> +		      offsetof(struct cpu_info, guest_cpu_user_regs.es)) & 0xf);
>> +	BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));
> 
> This will still triple fault the system if it triggers on an AP. 
> Exceptions aren't hooked up yet.

Hmm, true. They could be moved to the very end of the function
though?

> The reason I dropped the check was that there was no way it was going to
> fail on the BSP (because of the alignment of cpu0_stack) or APs (because
> of the alloc_xenheap_pages(STACK_ORDER, memflags) allocation).

These being page aligned has nothing to do with the BUG_ON()
triggering. I found its dropping being questionable in the context of
the old discussion rooted at this patch of mine
https://lists.xenproject.org/archives/html/xen-devel/2010-07/msg00642.html
I'd like to particularly point you to the various dependencies which
weren't properly spelled out or enforced back then. Specifically
it (not) triggering depends on the number and type of fields in
struct cpu_info following the guest_cpu_user_regs field.

> The BUILD_BUG_ON() is useful to retain, but I would suggest making
> get_stack_bottom() a static inline and putting the BUILD_BUG_ON() there.

I would agree if we were to not add back the BUG_ON(), but as
per above I'm not convinced yet.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: re-add stack alignment check

[Jan Beulich]

>>> On 14.11.16 at 15:24, <JBeulich@suse.com> wrote:
>>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> wrote:
>> The BUILD_BUG_ON() is useful to retain, but I would suggest making
>> get_stack_bottom() a static inline and putting the BUILD_BUG_ON() there.
> 
> I would agree if we were to not add back the BUG_ON(), but as
> per above I'm not convinced yet.

Actually no - the 16-byte alignment requirement exists only here,
not for every caller of this function.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: re-add stack alignment check

[Andrew Cooper]

On 14/11/16 14:24, Jan Beulich wrote:
>>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> wrote:
>> On 14/11/16 13:25, Jan Beulich wrote:
>>> --- a/xen/arch/x86/cpu/common.c
>>> +++ b/xen/arch/x86/cpu/common.c
>>> @@ -643,6 +643,11 @@ void load_system_tables(void)
>>>  		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>>>  	};
>>>  
>>> +	/* Bottom-of-stack must be 16-byte aligned! */
>>> +	BUILD_BUG_ON((sizeof(struct cpu_info) -
>>> +		      offsetof(struct cpu_info, guest_cpu_user_regs.es)) & 0xf);
>>> +	BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));
>> This will still triple fault the system if it triggers on an AP. 
>> Exceptions aren't hooked up yet.
> Hmm, true. They could be moved to the very end of the function
> though?

That would avoid the triple fault, but doesn't make the BUG_ON() useful.

>
>> The reason I dropped the check was that there was no way it was going to
>> fail on the BSP (because of the alignment of cpu0_stack) or APs (because
>> of the alloc_xenheap_pages(STACK_ORDER, memflags) allocation).
> These being page aligned has nothing to do with the BUG_ON()
> triggering. I found its dropping being questionable in the context of
> the old discussion rooted at this patch of mine
> https://lists.xenproject.org/archives/html/xen-devel/2010-07/msg00642.html
> I'd like to particularly point you to the various dependencies which
> weren't properly spelled out or enforced back then. Specifically
> it (not) triggering depends on the number and type of fields in
> struct cpu_info following the guest_cpu_user_regs field.

get_stack_bottom() takes the stack pointer, or's it up to the 8-page
boundary, overlays a struct cpu_info, then returns the address of es.

There is no value of %rsp which will ever cause it to fail.  The only
think which will cause a failure is the layout of struct cpu_info, but
the BUILD_BUG_ON() will catch that.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: re-add stack alignment check

[Jan Beulich]

>>> On 14.11.16 at 15:38, <andrew.cooper3@citrix.com> wrote:
> On 14/11/16 14:24, Jan Beulich wrote:
>>>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> wrote:
>>> On 14/11/16 13:25, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/cpu/common.c
>>>> +++ b/xen/arch/x86/cpu/common.c
>>>> @@ -643,6 +643,11 @@ void load_system_tables(void)
>>>>  		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>>>>  	};
>>>>  
>>>> +	/* Bottom-of-stack must be 16-byte aligned! */
>>>> +	BUILD_BUG_ON((sizeof(struct cpu_info) -
>>>> +		      offsetof(struct cpu_info, guest_cpu_user_regs.es)) & 0xf);
>>>> +	BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));
>>> This will still triple fault the system if it triggers on an AP. 
>>> Exceptions aren't hooked up yet.
>> Hmm, true. They could be moved to the very end of the function
>> though?
> 
> That would avoid the triple fault, but doesn't make the BUG_ON() useful.

And that's because? (I'm sorry if I'm overlooking the obvious.)

>>> The reason I dropped the check was that there was no way it was going to
>>> fail on the BSP (because of the alignment of cpu0_stack) or APs (because
>>> of the alloc_xenheap_pages(STACK_ORDER, memflags) allocation).
>> These being page aligned has nothing to do with the BUG_ON()
>> triggering. I found its dropping being questionable in the context of
>> the old discussion rooted at this patch of mine
>> https://lists.xenproject.org/archives/html/xen-devel/2010-07/msg00642.html 
>> I'd like to particularly point you to the various dependencies which
>> weren't properly spelled out or enforced back then. Specifically
>> it (not) triggering depends on the number and type of fields in
>> struct cpu_info following the guest_cpu_user_regs field.
> 
> get_stack_bottom() takes the stack pointer, or's it up to the 8-page
> boundary, overlays a struct cpu_info, then returns the address of es.
> 
> There is no value of %rsp which will ever cause it to fail.  The only
> think which will cause a failure is the layout of struct cpu_info, but
> the BUILD_BUG_ON() will catch that.

Yes. Otherwise a BUILD_BUG_ON() wouldn't work in the first place.
But please also take into consideration my other reply: Moving the
BUILD_BUG_ON() into get_stack_bottom() is inappropriate, and
having it here without the BUG_ON() risks someone updating code
elsewhere such that the BUILD_BUG_ON() wouldn't trigger, but the
BUG_ON() would. That could be avoided only if we could make the
expression handed to BUILD_BUG_ON() use get_stack_bottom().

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: re-add stack alignment check

[Andrew Cooper]

On 14/11/16 15:02, Jan Beulich wrote:
>>>> On 14.11.16 at 15:38, <andrew.cooper3@citrix.com> wrote:
>> On 14/11/16 14:24, Jan Beulich wrote:
>>>>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> wrote:
>>>> On 14/11/16 13:25, Jan Beulich wrote:
>>>>> --- a/xen/arch/x86/cpu/common.c
>>>>> +++ b/xen/arch/x86/cpu/common.c
>>>>> @@ -643,6 +643,11 @@ void load_system_tables(void)
>>>>>  		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>>>>>  	};
>>>>>  
>>>>> +	/* Bottom-of-stack must be 16-byte aligned! */
>>>>> +	BUILD_BUG_ON((sizeof(struct cpu_info) -
>>>>> +		      offsetof(struct cpu_info, guest_cpu_user_regs.es)) & 0xf);
>>>>> +	BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));
>>>> This will still triple fault the system if it triggers on an AP. 
>>>> Exceptions aren't hooked up yet.
>>> Hmm, true. They could be moved to the very end of the function
>>> though?
>> That would avoid the triple fault, but doesn't make the BUG_ON() useful.
> And that's because? (I'm sorry if I'm overlooking the obvious.)
>
>>>> The reason I dropped the check was that there was no way it was going to
>>>> fail on the BSP (because of the alignment of cpu0_stack) or APs (because
>>>> of the alloc_xenheap_pages(STACK_ORDER, memflags) allocation).
>>> These being page aligned has nothing to do with the BUG_ON()
>>> triggering. I found its dropping being questionable in the context of
>>> the old discussion rooted at this patch of mine
>>> https://lists.xenproject.org/archives/html/xen-devel/2010-07/msg00642.html 
>>> I'd like to particularly point you to the various dependencies which
>>> weren't properly spelled out or enforced back then. Specifically
>>> it (not) triggering depends on the number and type of fields in
>>> struct cpu_info following the guest_cpu_user_regs field.
>> get_stack_bottom() takes the stack pointer, or's it up to the 8-page
>> boundary, overlays a struct cpu_info, then returns the address of es.
>>
>> There is no value of %rsp which will ever cause it to fail.  The only
>> think which will cause a failure is the layout of struct cpu_info, but
>> the BUILD_BUG_ON() will catch that.
> Yes. Otherwise a BUILD_BUG_ON() wouldn't work in the first place.
> But please also take into consideration my other reply: Moving the
> BUILD_BUG_ON() into get_stack_bottom() is inappropriate, and
> having it here without the BUG_ON() risks someone updating code
> elsewhere such that the BUILD_BUG_ON() wouldn't trigger, but the
> BUG_ON() would. That could be avoided only if we could make the
> expression handed to BUILD_BUG_ON() use get_stack_bottom().

What problems are we actually trying to detect here?

Irrespective of what actually gets written into tss.rsp0, hardware will
align the stack on entry.

Xen cares that the value written into tss.rsp0 is exactly as expected,
(i.e. some small number of words under the 8-page boundary) so that
constructs such as current and guest_cpu_user_regs() work properly.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: re-add stack alignment check

[Jan Beulich]

>>> On 14.11.16 at 17:38, <andrew.cooper3@citrix.com> wrote:
> On 14/11/16 15:02, Jan Beulich wrote:
>>>>> On 14.11.16 at 15:38, <andrew.cooper3@citrix.com> wrote:
>>> On 14/11/16 14:24, Jan Beulich wrote:
>>>>>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> wrote:
>>>>> On 14/11/16 13:25, Jan Beulich wrote:
>>>>>> --- a/xen/arch/x86/cpu/common.c
>>>>>> +++ b/xen/arch/x86/cpu/common.c
>>>>>> @@ -643,6 +643,11 @@ void load_system_tables(void)
>>>>>>  		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>>>>>>  	};
>>>>>>  
>>>>>> +	/* Bottom-of-stack must be 16-byte aligned! */
>>>>>> +	BUILD_BUG_ON((sizeof(struct cpu_info) -
>>>>>> +		      offsetof(struct cpu_info, guest_cpu_user_regs.es)) & 0xf);
>>>>>> +	BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));
>>>>> This will still triple fault the system if it triggers on an AP. 
>>>>> Exceptions aren't hooked up yet.
>>>> Hmm, true. They could be moved to the very end of the function
>>>> though?
>>> That would avoid the triple fault, but doesn't make the BUG_ON() useful.
>> And that's because? (I'm sorry if I'm overlooking the obvious.)
>>
>>>>> The reason I dropped the check was that there was no way it was going to
>>>>> fail on the BSP (because of the alignment of cpu0_stack) or APs (because
>>>>> of the alloc_xenheap_pages(STACK_ORDER, memflags) allocation).
>>>> These being page aligned has nothing to do with the BUG_ON()
>>>> triggering. I found its dropping being questionable in the context of
>>>> the old discussion rooted at this patch of mine
>>>> https://lists.xenproject.org/archives/html/xen-devel/2010-07/msg00642.html 
>>>> I'd like to particularly point you to the various dependencies which
>>>> weren't properly spelled out or enforced back then. Specifically
>>>> it (not) triggering depends on the number and type of fields in
>>>> struct cpu_info following the guest_cpu_user_regs field.
>>> get_stack_bottom() takes the stack pointer, or's it up to the 8-page
>>> boundary, overlays a struct cpu_info, then returns the address of es.
>>>
>>> There is no value of %rsp which will ever cause it to fail.  The only
>>> think which will cause a failure is the layout of struct cpu_info, but
>>> the BUILD_BUG_ON() will catch that.
>> Yes. Otherwise a BUILD_BUG_ON() wouldn't work in the first place.
>> But please also take into consideration my other reply: Moving the
>> BUILD_BUG_ON() into get_stack_bottom() is inappropriate, and
>> having it here without the BUG_ON() risks someone updating code
>> elsewhere such that the BUILD_BUG_ON() wouldn't trigger, but the
>> BUG_ON() would. That could be avoided only if we could make the
>> expression handed to BUILD_BUG_ON() use get_stack_bottom().
> 
> What problems are we actually trying to detect here?
> 
> Irrespective of what actually gets written into tss.rsp0, hardware will
> align the stack on entry.

And that's the problem: Everything will break in that case (being off
by 8 bytes). I recall from the time when I did put together the old
patch I did point you to. If you want to try out, just add a single
8-byte field to struct cpu_info and try booting the resulting
hypervisor. IOW the checks being added are to verify the comment
in the struct cpu_info declaration.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: re-add stack alignment check

[Andrew Cooper]

On 14/11/16 16:54, Jan Beulich wrote:
>>>> On 14.11.16 at 17:38, <andrew.cooper3@citrix.com> wrote:
>> On 14/11/16 15:02, Jan Beulich wrote:
>>>>>> On 14.11.16 at 15:38, <andrew.cooper3@citrix.com> wrote:
>>>> On 14/11/16 14:24, Jan Beulich wrote:
>>>>>>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 14/11/16 13:25, Jan Beulich wrote:
>>>>>>> --- a/xen/arch/x86/cpu/common.c
>>>>>>> +++ b/xen/arch/x86/cpu/common.c
>>>>>>> @@ -643,6 +643,11 @@ void load_system_tables(void)
>>>>>>>  		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>>>>>>>  	};
>>>>>>>  
>>>>>>> +	/* Bottom-of-stack must be 16-byte aligned! */
>>>>>>> +	BUILD_BUG_ON((sizeof(struct cpu_info) -
>>>>>>> +		      offsetof(struct cpu_info, guest_cpu_user_regs.es)) & 0xf);
>>>>>>> +	BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));
>>>>>> This will still triple fault the system if it triggers on an AP. 
>>>>>> Exceptions aren't hooked up yet.
>>>>> Hmm, true. They could be moved to the very end of the function
>>>>> though?
>>>> That would avoid the triple fault, but doesn't make the BUG_ON() useful.
>>> And that's because? (I'm sorry if I'm overlooking the obvious.)
>>>
>>>>>> The reason I dropped the check was that there was no way it was going to
>>>>>> fail on the BSP (because of the alignment of cpu0_stack) or APs (because
>>>>>> of the alloc_xenheap_pages(STACK_ORDER, memflags) allocation).
>>>>> These being page aligned has nothing to do with the BUG_ON()
>>>>> triggering. I found its dropping being questionable in the context of
>>>>> the old discussion rooted at this patch of mine
>>>>> https://lists.xenproject.org/archives/html/xen-devel/2010-07/msg00642.html 
>>>>> I'd like to particularly point you to the various dependencies which
>>>>> weren't properly spelled out or enforced back then. Specifically
>>>>> it (not) triggering depends on the number and type of fields in
>>>>> struct cpu_info following the guest_cpu_user_regs field.
>>>> get_stack_bottom() takes the stack pointer, or's it up to the 8-page
>>>> boundary, overlays a struct cpu_info, then returns the address of es.
>>>>
>>>> There is no value of %rsp which will ever cause it to fail.  The only
>>>> think which will cause a failure is the layout of struct cpu_info, but
>>>> the BUILD_BUG_ON() will catch that.
>>> Yes. Otherwise a BUILD_BUG_ON() wouldn't work in the first place.
>>> But please also take into consideration my other reply: Moving the
>>> BUILD_BUG_ON() into get_stack_bottom() is inappropriate, and
>>> having it here without the BUG_ON() risks someone updating code
>>> elsewhere such that the BUILD_BUG_ON() wouldn't trigger, but the
>>> BUG_ON() would. That could be avoided only if we could make the
>>> expression handed to BUILD_BUG_ON() use get_stack_bottom().
>> What problems are we actually trying to detect here?
>>
>> Irrespective of what actually gets written into tss.rsp0, hardware will
>> align the stack on entry.
> And that's the problem: Everything will break in that case (being off
> by 8 bytes).

Not quite.

Everything will indeed break if it is off by 8, but everything will also
be similarly-broken if it is off by 16 or off by -8.

Checking for 16-byte alignment doesn't catch half the broken cases.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: re-add stack alignment check

[Jan Beulich]

>>> On 15.11.16 at 11:26, <andrew.cooper3@citrix.com> wrote:
> On 14/11/16 16:54, Jan Beulich wrote:
>>>>> On 14.11.16 at 17:38, <andrew.cooper3@citrix.com> wrote:
>>> On 14/11/16 15:02, Jan Beulich wrote:
>>>>>>> On 14.11.16 at 15:38, <andrew.cooper3@citrix.com> wrote:
>>>>> On 14/11/16 14:24, Jan Beulich wrote:
>>>>>>>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> wrote:
>>>>>>> On 14/11/16 13:25, Jan Beulich wrote:
>>>>>>>> --- a/xen/arch/x86/cpu/common.c
>>>>>>>> +++ b/xen/arch/x86/cpu/common.c
>>>>>>>> @@ -643,6 +643,11 @@ void load_system_tables(void)
>>>>>>>>  		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>>>>>>>>  	};
>>>>>>>>  
>>>>>>>> +	/* Bottom-of-stack must be 16-byte aligned! */
>>>>>>>> +	BUILD_BUG_ON((sizeof(struct cpu_info) -
>>>>>>>> +		      offsetof(struct cpu_info, guest_cpu_user_regs.es)) & 0xf);
>>>>>>>> +	BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));
>>>>>>> This will still triple fault the system if it triggers on an AP. 
>>>>>>> Exceptions aren't hooked up yet.
>>>>>> Hmm, true. They could be moved to the very end of the function
>>>>>> though?
>>>>> That would avoid the triple fault, but doesn't make the BUG_ON() useful.
>>>> And that's because? (I'm sorry if I'm overlooking the obvious.)
>>>>
>>>>>>> The reason I dropped the check was that there was no way it was going to
>>>>>>> fail on the BSP (because of the alignment of cpu0_stack) or APs (because
>>>>>>> of the alloc_xenheap_pages(STACK_ORDER, memflags) allocation).
>>>>>> These being page aligned has nothing to do with the BUG_ON()
>>>>>> triggering. I found its dropping being questionable in the context of
>>>>>> the old discussion rooted at this patch of mine
>>>>>> https://lists.xenproject.org/archives/html/xen-devel/2010-07/msg00642.html 
>>>>>> I'd like to particularly point you to the various dependencies which
>>>>>> weren't properly spelled out or enforced back then. Specifically
>>>>>> it (not) triggering depends on the number and type of fields in
>>>>>> struct cpu_info following the guest_cpu_user_regs field.
>>>>> get_stack_bottom() takes the stack pointer, or's it up to the 8-page
>>>>> boundary, overlays a struct cpu_info, then returns the address of es.
>>>>>
>>>>> There is no value of %rsp which will ever cause it to fail.  The only
>>>>> think which will cause a failure is the layout of struct cpu_info, but
>>>>> the BUILD_BUG_ON() will catch that.
>>>> Yes. Otherwise a BUILD_BUG_ON() wouldn't work in the first place.
>>>> But please also take into consideration my other reply: Moving the
>>>> BUILD_BUG_ON() into get_stack_bottom() is inappropriate, and
>>>> having it here without the BUG_ON() risks someone updating code
>>>> elsewhere such that the BUILD_BUG_ON() wouldn't trigger, but the
>>>> BUG_ON() would. That could be avoided only if we could make the
>>>> expression handed to BUILD_BUG_ON() use get_stack_bottom().
>>> What problems are we actually trying to detect here?
>>>
>>> Irrespective of what actually gets written into tss.rsp0, hardware will
>>> align the stack on entry.
>> And that's the problem: Everything will break in that case (being off
>> by 8 bytes).
> 
> Not quite.
> 
> Everything will indeed break if it is off by 8, but everything will also
> be similarly-broken if it is off by 16 or off by -8.

Off by -8 will be caught as well. And there's no possible off-by-16,
as then the calculations will be right again (read: you can freely
add two longs to struct cpu_info, but you mustn't add just one).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Ping: [PATCH] x86: re-add stack alignment check

[Jan Beulich]

>>> On 15.11.16 at 11:46, <JBeulich@suse.com> wrote:
>>>> On 15.11.16 at 11:26, <andrew.cooper3@citrix.com> wrote:
>> Everything will indeed break if it is off by 8, but everything will also
>> be similarly-broken if it is off by 16 or off by -8.
> 
> Off by -8 will be caught as well. And there's no possible off-by-16,
> as then the calculations will be right again (read: you can freely
> add two longs to struct cpu_info, but you mustn't add just one).

This discussion appears to have stalled, and so far I've seen no
reason to change other than the position of the checks. I'm
hesitant to send out v2 though without having reached agreement
on the other aspects.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Ping: [PATCH] x86: re-add stack alignment check

[Andrew Cooper]

On 22/11/16 10:14, Jan Beulich wrote:
>>>> On 15.11.16 at 11:46, <JBeulich@suse.com> wrote:
>>>>> On 15.11.16 at 11:26, <andrew.cooper3@citrix.com> wrote:
>>> Everything will indeed break if it is off by 8, but everything will also
>>> be similarly-broken if it is off by 16 or off by -8.
>> Off by -8 will be caught as well. And there's no possible off-by-16,
>> as then the calculations will be right again (read: you can freely
>> add two longs to struct cpu_info, but you mustn't add just one).
> This discussion appears to have stalled, and so far I've seen no
> reason to change other than the position of the checks. I'm
> hesitant to send out v2 though without having reached agreement
> on the other aspects.

Fine - lets get this fix back in for 4.8, then argue over how it should
be improved.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Ping: [PATCH] x86: re-add stack alignment check

[Wei Liu]

On Fri, Nov 25, 2016 at 10:34:30AM +0000, Andrew Cooper wrote:
> On 22/11/16 10:14, Jan Beulich wrote:
> >>>> On 15.11.16 at 11:46, <JBeulich@suse.com> wrote:
> >>>>> On 15.11.16 at 11:26, <andrew.cooper3@citrix.com> wrote:
> >>> Everything will indeed break if it is off by 8, but everything will also
> >>> be similarly-broken if it is off by 16 or off by -8.
> >> Off by -8 will be caught as well. And there's no possible off-by-16,
> >> as then the calculations will be right again (read: you can freely
> >> add two longs to struct cpu_info, but you mustn't add just one).
> > This discussion appears to have stalled, and so far I've seen no
> > reason to change other than the position of the checks. I'm
> > hesitant to send out v2 though without having reached agreement
> > on the other aspects.
> 
> Fine - lets get this fix back in for 4.8, then argue over how it should
> be improved.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel