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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

~Andrew

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