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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.11.16 at 17:38, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 14/11/16 15:02, Jan Beulich wrote:
>>>>> On 14.11.16 at 15:38, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 14/11/16 14:24, Jan Beulich wrote:
>>>>>>> On 14.11.16 at 14:45, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel