Re: [Xen-devel] [PATCH for-4.5] x86/stack: Avoid peeking into unmapped guard pages when dumping Xens stack

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/12/14 13:32, Jan Beulich wrote:
>>>> On 04.12.14 at 13:58, <andrew.cooper3@xxxxxxxxxx> wrote:
>> Konrad: I am requesting a release ack for this change.  It aids the clarity 
>> of
>> certain crash information, and prevents cascade pagefaults in certain
>> circumstances, which would prevent execution of the crash kernel or a system
>> reboot.
> With
>
> #ifndef NDEBUG
> #define MEMORY_GUARD
> #endif
>
> I don't think this qualifies as a necessary bug fix at this point of 4.5.

Without frame pointers, you get a call trace of almost complete junk,
spanning 6 pages.  This is admittedly less bad, but still a bug.

>
>> +unsigned long get_stack_trace_bottom(unsigned long sp)
>> +{
>> +    switch ( get_stack_page(sp) )
>> +    {
>> +    case 0 ... 2:
>> +        return ROUNDUP(sp, PAGE_SIZE) -
>> +            offsetof(struct cpu_user_regs, es) - sizeof(unsigned long);
> The only concern I have here is that this may wrongly truncate a
> dump/trace when one of the IST stacks overflowed. But I think we
> can live with that - an overflow of the first one would already have
> similar behavior today.

I was already planning to do some improvements for 4.6, based on work
developed for the xen-crashdump-analyser which has proved very useful
when investigating XenServer crashes.  This includes the ability to spot
valid exception frames, and follow them back to the primary stack
through a set of nested IST faults.

Along the bugfix line, there are some other issues which would be nice
to fix.  Putting a guard pages in stack pages 0 and 7 to completely
prevent a stack overflow on cpu0 from sliding into Xens BSS.  This would
leave 3 unmapped guard pages, 2 pages of primary stack and 3 IST
stacks.  The syscall trampoline can safely live at the bottom of the #DF
stack.

For AMD hardware, the lack of TR switch on vmexit causes any interaction
with MEMORY_GUARD to be a triple fault as we currently must disable IST
for security reasons.  I am hoping that the overhead of switching the
task register will prove to be negligible, and that we can run without
playing any IST games in the slightest.  If not, then the switch can
reasonably be gated on MEMORY_GUARD, in which case we still
conditionally keep the IST games, but don't take the TR switch overhead
in non-debug builds.

>
>> +
>> +#ifndef MEMORY_GUARD
>> +    case 3 ... 5:
>> +#endif
>> +    case 6 ... 7:
>> +        return ROUNDUP(sp, STACK_SIZE) -
>> +            sizeof(struct cpu_info) - sizeof(unsigned long);
>> +
>> +#ifdef MEMORY_GUARD
>> +    case 3 ... 5:
>> +#endif
>> +    default:
> What is the #ifdef good for when this is "default:" anyway? With
> this dropped (also from the other function)
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Ah yes - a side effect of the way the patch was developed.  They can be
dropped.

~Andrew


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