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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.11.16 at 11:26, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 14/11/16 16:54, Jan Beulich wrote:
>>>>> 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).
> 
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel