[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.5] x86/stack: Avoid peeking into unmapped guard pages when dumping Xens stack
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |