Xen devel - Nov 2013 - [Patch v3 0/4] Xen stack trace printing improvements

This series consists of improvements to Xen''s ability to print traces
of its
own stack, and specifically for the stack overflow case to be able to use
frame pointers in a debug build.

I have dev tested the series in debug and non-debug cases, with and without
memory guards, and I believe that all the stack traces look correct (given the
available information Xen has), and that the boundaries are now correct. This
series has had a substantial rebase on top of the %pS series.

George: Regarding the 4.4 code, I would like to argue this as a bugfix rather
than feature, therefore being exempt from the freeze at the moment.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>

Before, show_trace() had two implementations depending on
CONFIG_FRAME_POINTER.  Some parts were common, while the loops to wander up
the stack were different.

The version aided by frame pointers had a special case for function calls on
wild function pointers, but this doesn''t need to be a special case.

After the refactoring, there are now two implementations of __show_trace()
which differ depending on CONFIG_FRAME_POINTER, and a single show_trace()
with the common bits, including the logic for wild function pointers.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/traps.c |   65 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e5b3585..5917291 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -195,12 +195,14 @@ static void show_guest_stack(struct vcpu *v, struct
cpu_user_regs *regs)
 
 #if !defined(CONFIG_FRAME_POINTER)
 
-static void show_trace(struct cpu_user_regs *regs)
+/*
+ * Stack trace from pointers found in stack, unaided by frame pointers.  For
+ * caller convenience, this has the same prototype as its alternative, and
+ * simply ignores the base pointer parameter.
+ */
+static void __show_trace(unsigned long sp, unsigned long __maybe_unused bp)
 {
-    unsigned long *stack = ESP_BEFORE_EXCEPTION(regs), addr;
-
-    printk("Xen call trace:\n"
-           "   [<%p>] %pS\n", _p(regs->eip),
_p(regs->eip));
+    unsigned long *stack = (unsigned long *)sp, addr;
 
     while ( ((long)stack & (STACK_SIZE-BYTES_PER_LONG)) != 0 )
     {
@@ -208,35 +210,22 @@ static void show_trace(struct cpu_user_regs *regs)
         if ( is_active_kernel_text(addr) )
             printk("   [<%p>] %pS\n", _p(addr), _p(addr));
     }
-
-    printk("\n");
 }
 
 #else
 
-static void show_trace(struct cpu_user_regs *regs)
+/* Stack trace from frames in the stack, using frame pointers */
+static void __show_trace(unsigned long sp, unsigned long bp)
 {
     unsigned long *frame, next, addr, low, high;
 
-    /*
-     * If RIP is not pointing into hypervisor code then someone may have
-     * called into oblivion. Peek to see if they left a return address at
-     * top of stack.
-     */
-    addr = is_active_kernel_text(regs->eip) ||
-           !is_active_kernel_text(*ESP_BEFORE_EXCEPTION(regs)) ?
-           regs->eip : *ESP_BEFORE_EXCEPTION(regs);
-
-    printk("Xen call trace:\n"
-           "   [<%p>] %pS\n", _p(addr), _p(addr));
-
     /* Bounds for range of valid frame pointer. */
-    low  = (unsigned long)(ESP_BEFORE_EXCEPTION(regs) - 2);
+    low  = sp - 2*sizeof(unsigned long);
     high = (low & ~(STACK_SIZE - 1)) + 
         (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long));
 
     /* The initial frame pointer. */
-    next = regs->ebp;
+    next = bp;
 
     for ( ; ; )
     {
@@ -268,12 +257,40 @@ static void show_trace(struct cpu_user_regs *regs)
 
         low = (unsigned long)&frame[2];
     }
-
-    printk("\n");
 }
 
 #endif
 
+static void show_trace(const struct cpu_user_regs *regs)
+{
+    unsigned long sp = regs->rsp;
+
+    printk("Xen call trace:\n");
+
+    /*
+     * If RIP looks sensible, or the top of the stack doesn''t, print
RIP at
+     * the top of the stack trace.
+     */
+    if ( is_active_kernel_text(regs->rip) ||
+         !is_active_kernel_text(regs->rsp) )
+        printk("   [<%p>] %pS\n", _p(regs->eip),
_p(regs->eip));
+    /*
+     * else RIP looks bad but the top of the stack looks good.  Perhaps we
+     * followed a wild function pointer? Lets assume the top of the stack is a
+     * return address; print it and skip past so __show_trace()
doesn''t print
+     * it again.
+     */
+    else
+    {
+        printk("   [<%p>] %pS\n", _p(sp), _p(sp));
+        sp += sizeof (unsigned long);
+    }
+
+    __show_trace(sp, regs->rbp);
+
+    printk("\n");
+}
+
 void show_stack(struct cpu_user_regs *regs)
 {
     unsigned long *stack = ESP_BEFORE_EXCEPTION(regs), addr;
-- 
1.7.10.4

Move the boundary into current.h along with the other stack manipulation code.
The boundary is now the word adjacent to a struct cpu_info on the stack.

This also fixes the somewhat spurious bounds for the case with frame pointers.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

---

Changes since v1:
 * Change printable bottom depending on frame pointers
---
 xen/arch/x86/traps.c          |    9 ++++-----
 xen/include/asm-x86/current.h |    9 +++++++++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 5917291..d68f93c 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -203,8 +203,9 @@ static void show_guest_stack(struct vcpu *v, struct
cpu_user_regs *regs)
 static void __show_trace(unsigned long sp, unsigned long __maybe_unused bp)
 {
     unsigned long *stack = (unsigned long *)sp, addr;
+    unsigned long *bottom = (unsigned long *)get_printable_stack_bottom(sp);
 
-    while ( ((long)stack & (STACK_SIZE-BYTES_PER_LONG)) != 0 )
+    while ( stack <= bottom )
     {
         addr = *stack++;
         if ( is_active_kernel_text(addr) )
@@ -217,12 +218,10 @@ static void __show_trace(unsigned long sp, unsigned long
__maybe_unused bp)
 /* Stack trace from frames in the stack, using frame pointers */
 static void __show_trace(unsigned long sp, unsigned long bp)
 {
-    unsigned long *frame, next, addr, low, high;
+    unsigned long *frame, next, addr;
 
     /* Bounds for range of valid frame pointer. */
-    low  = sp - 2*sizeof(unsigned long);
-    high = (low & ~(STACK_SIZE - 1)) + 
-        (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long));
+    unsigned long low = sp, high = get_printable_stack_bottom(sp);
 
     /* The initial frame pointer. */
     next = bp;
diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
index bec4dbe..c2792ce 100644
--- a/xen/include/asm-x86/current.h
+++ b/xen/include/asm-x86/current.h
@@ -50,6 +50,15 @@ static inline struct cpu_info *get_cpu_info(void)
 #define get_stack_bottom()                      \
     ((unsigned long)&get_cpu_info()->guest_cpu_user_regs.es)
 
+/*
+ * Get the bottom-of-stack, as useful for printing stack traces.  This is the
+ * highest word on the stack which might be part of a stack trace, and is the
+ * adjacent word to a struct cpu_info on the stack.
+ */
+#define get_printable_stack_bottom(sp)          \
+    ((sp & (~(STACK_SIZE-1))) +                 \
+     (STACK_SIZE - sizeof(struct cpu_info) - sizeof(unsigned long)))
+
 #define reset_stack_and_jump(__fn)              \
     __asm__ __volatile__ (                      \
         "mov %0,%%"__OP"sp; jmp %c1"            \
-- 
1.7.10.4

Pass a full set of cpu_user_regs, and defer the hand-coded stack printing to
__show_trace(), which will correctly use frame pointers if available.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/traps.c            |   14 ++++----------
 xen/arch/x86/x86_64/traps.c     |    2 +-
 xen/include/asm-x86/processor.h |    2 +-
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index d68f93c..67be3c4 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -316,11 +316,11 @@ void show_stack(struct cpu_user_regs *regs)
     show_trace(regs);
 }
 
-void show_stack_overflow(unsigned int cpu, unsigned long esp)
+void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs)
 {
 #ifdef MEMORY_GUARD
+    unsigned long esp = regs->rsp;
     unsigned long esp_top, esp_bottom;
-    unsigned long *stack, addr;
 
     esp_bottom = (esp | (STACK_SIZE - 1)) + 1;
     esp_top    = esp_bottom - PRIMARY_STACK_SIZE;
@@ -340,16 +340,10 @@ void show_stack_overflow(unsigned int cpu, unsigned long
esp)
     if ( esp < esp_top )
         esp = esp_top;
 
-    printk("Xen stack overflow (dumping trace %p-%p):\n   ",
+    printk("Xen stack overflow (dumping trace %p-%p):\n",
            (void *)esp, (void *)esp_bottom);
 
-    stack = (unsigned long *)esp;
-    while ( ((long)stack & (STACK_SIZE-BYTES_PER_LONG)) != 0 )
-    {
-        addr = *stack++;
-        if ( is_active_kernel_text(addr) )
-            printk("%p: [<%p>] %pS\n", stack, _p(addr),
_p(addr));
-    }
+    __show_trace(esp, regs->rbp);
 
     printk("\n");
 #endif
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 8e6a7c1..bcf72b6 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -248,7 +248,7 @@ void do_double_fault(struct cpu_user_regs *regs)
 
     printk("CPU:    %d\n", cpu);
     _show_registers(regs, crs, CTXT_hypervisor, NULL);
-    show_stack_overflow(cpu, regs->rsp);
+    show_stack_overflow(cpu, regs);
 
     panic("DOUBLE FAULT -- system shutdown\n");
 }
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 73a3202..c120460 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -509,7 +509,7 @@ extern always_inline void prefetchw(const void *x)
 #endif
 
 void show_stack(struct cpu_user_regs *regs);
-void show_stack_overflow(unsigned int cpu, unsigned long esp);
+void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs);
 void show_registers(struct cpu_user_regs *regs);
 void show_execution_state(struct cpu_user_regs *regs);
 #define dump_execution_state() run_in_exception_handler(show_execution_state)
-- 
1.7.10.4

I do not recommend trying stacktest3 without stack guards - it interacts very
badly between trashing the NMI stack and the NMI watchdog.
---
 xen/arch/x86/traps.c        |   95 +++++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/x86_64/traps.c |   15 +++++++
 xen/drivers/char/console.c  |    8 ++++
 xen/drivers/char/serial.c   |   12 ++++++
 xen/include/xen/console.h   |    1 +
 xen/include/xen/serial.h    |    1 +
 6 files changed, 132 insertions(+)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 67be3c4..88ae3d4 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -34,6 +34,7 @@
 #include <xen/console.h>
 #include <xen/shutdown.h>
 #include <xen/guest_access.h>
+#include <xen/keyhandler.h>
 #include <asm/regs.h>
 #include <xen/delay.h>
 #include <xen/event.h>
@@ -3762,6 +3763,100 @@ void asm_domain_crash_synchronous(unsigned long addr)
     __domain_crash_synchronous();
 }
 
+static noinline int recursion(int depth)
+{
+    /* junk on stack to fool naive algorithm */
+    volatile unsigned long addr1 = (unsigned long)&do_get_debugreg;
+    volatile unsigned long tsc;
+    volatile unsigned long addr2 = (unsigned long)&do_set_debugreg;
+
+    if ( depth == 0 )
+    {
+        printk("Did you mean recursion()?\n");
+        run_in_exception_handler(show_stack);
+        return 0;
+    }
+    else
+    {
+        int r;
+        rdtscll(tsc);
+        printk("depth %d, addr1 0x%016"PRIx64", addr2
0x%016"PRIx64", tsc %"PRIu64"\n",
+               depth, addr1, addr2, tsc);
+        r = recursion(depth-1);
+        if ( r )
+            printk("done\n"); /* So GCC cant perform tailcall
optimisation */
+        return r;
+    }
+}
+
+static void stacktest1(unsigned char key)
+{
+    printk("In %s()\n", __func__);
+    recursion(5);
+}
+
+static void stacktest2(unsigned char key, struct cpu_user_regs * regs)
+{
+    printk("In %s()\n", __func__);
+    recursion(5);
+}
+
+volatile int in_stacktest3 = 0;
+unsigned long saved_sp, saved_ip, saved_flags = 0;
+static void stacktest3(unsigned char key)
+{
+    printk("In %s()\n", __func__);
+
+    local_irq_save(saved_flags);
+    in_stacktest3 = 1;
+
+    /* Hack up an ability to longjump() out of the #DF handler. */
+recover:
+    asm volatile ("mov %%rsp, %0;\n\t"
+                  : "=m"(saved_sp) );
+    saved_ip = (unsigned long)&&recover;
+
+    if ( in_stacktest3 )
+    {
+        printk("Recovery info: sp %p, ip %p\n",
+               _p(saved_sp), _p(saved_ip));
+        recursion(500);
+    }
+    else
+        printk("Recovered from #DF\n");
+
+    local_irq_restore(saved_flags);
+}
+
+static struct keyhandler stacktest_handler[] = {
+    {
+        .irq_callback = 0,
+        .u.fn = stacktest1,
+        .desc = "Xen stack #1 - Recurse a little and see trace."
+    },
+
+    {
+        .irq_callback = 1,
+        .u.irq_fn = stacktest2,
+        .desc = "Xen stack #2 - Recurse a little and see trace in
irq."
+    },
+
+    {
+        .irq_callback = 0,
+        .u.fn = stacktest3,
+        .desc = "Xen stack #3 - Cause stack overflow."
+    },
+ };
+
+static int __init initialize_crashtable(void)
+{
+    register_keyhandler(''1'', &stacktest_handler[0]);
+    register_keyhandler(''2'', &stacktest_handler[1]);
+    register_keyhandler(''3'', &stacktest_handler[2]);
+    return 1;
+}
+__initcall(initialize_crashtable);
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index bcf72b6..a687954 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -223,6 +223,9 @@ void show_page_walk(unsigned long addr)
            l1_table_offset(addr), l1e_get_intpte(l1e), pfn);
 }
 
+extern volatile int in_stacktest3;
+extern unsigned long saved_sp, saved_ip;
+
 void double_fault(void);
 void do_double_fault(struct cpu_user_regs *regs)
 {
@@ -250,6 +253,18 @@ void do_double_fault(struct cpu_user_regs *regs)
     _show_registers(regs, crs, CTXT_hypervisor, NULL);
     show_stack_overflow(cpu, regs);
 
+    if ( in_stacktest3 )
+    {
+        in_stacktest3 = 0;
+
+        undo_console_force_unlock();
+        preempt_count() = 0;
+        enable_nmis();
+
+        asm volatile ("mov %0, %%rsp; jmp *%1":
+                      : "r" (saved_sp), "r" (saved_ip) :
"memory" );
+    }
+
     panic("DOUBLE FAULT -- system shutdown\n");
 }
 
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 508f845..967fd53 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -803,6 +803,14 @@ void console_force_unlock(void)
     console_start_sync();
 }
 
+void undo_console_force_unlock(void)
+{
+    watchdog_enable();
+    undo_serial_force_unlock(sercon_handle);
+    console_locks_busted = 0;
+    console_end_sync();
+}
+
 void console_start_sync(void)
 {
     atomic_inc(&print_everything);
diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
index 9b006f2..79874f1 100644
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -380,6 +380,18 @@ void serial_force_unlock(int handle)
     serial_start_sync(handle);
 }
 
+void undo_serial_force_unlock(int handle)
+{
+    struct serial_port *port;
+
+    if ( handle == -1 )
+        return;
+
+    port = &com[handle & SERHND_IDX];
+
+    serial_end_sync(handle);
+}
+
 void serial_start_sync(int handle)
 {
     struct serial_port *port;
diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
index cfb07a2..41c96f8 100644
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -21,6 +21,7 @@ int console_has(const char *device);
 int fill_console_start_info(struct dom0_vga_console_info *);
 
 void console_force_unlock(void);
+void undo_console_force_unlock(void);
 
 void console_start_sync(void);
 void console_end_sync(void);
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index f38c9b7..fbdcad4 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -123,6 +123,7 @@ char serial_getc(int handle);
 /* Forcibly prevent serial lockup when the system is in a bad way. */
 /* (NB. This also forces an implicit serial_start_sync()). */
 void serial_force_unlock(int handle);
+void undo_serial_force_unlock(int handle);
 
 /* Start/end a synchronous region (temporarily disable interrupt-driven tx). */
 void serial_start_sync(int handle);
-- 
1.7.10.4

On Mon, Nov 18, 2013 at 7:34 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> This series consists of improvements to Xen''s ability to print
traces of its
> own stack, and specifically for the stack overflow case to be able to use
> frame pointers in a debug build.
>
> I have dev tested the series in debug and non-debug cases, with and without
> memory guards, and I believe that all the stack traces look correct (given
the
> available information Xen has), and that the boundaries are now correct.
This
> series has had a substantial rebase on top of the %pS series.
>
> George: Regarding the 4.4 code, I would like to argue this as a bugfix
rather
> than feature, therefore being exempt from the freeze at the moment.
Well that argument is BS.  It''s not a bug fix; it''s clearly
exactly
what the series summary describes it as -- an improvement.

The questions you need to answer are:
* What are the benefits to 4.4 of accepting this patch?
* What are the risks in accepting this patch if it turned out to be
not quite correct?

Re the benefits, I''m guessing the main one is to be able to use frame
pointers in an extra case, making the stack more readable on a crash.

The risk, it seems to me, would be if there were other crashes that
might have garbled stacks that are no longer useful; or, more
importantly, that if someone was using a debug key to print out the
hypervisor stack, that, it might cause the whole host to crash.

I''m kind of on the fence on this one -- Jan / Keir, any thoughts?

 -George

On 19/11/2013 10:10, George Dunlap wrote:
> On Mon, Nov 18, 2013 at 7:34 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> This series consists of improvements to Xen''s ability to print
traces of its
>> own stack, and specifically for the stack overflow case to be able to
use
>> frame pointers in a debug build.
>>
>> I have dev tested the series in debug and non-debug cases, with and
without
>> memory guards, and I believe that all the stack traces look correct
(given the
>> available information Xen has), and that the boundaries are now
correct. This
>> series has had a substantial rebase on top of the %pS series.
>>
>> George: Regarding the 4.4 code, I would like to argue this as a bugfix
rather
>> than feature, therefore being exempt from the freeze at the moment.
> Well that argument is BS.  It''s not a bug fix; it''s
clearly exactly
> what the series summary describes it as -- an improvement.
>
> The questions you need to answer are:
> * What are the benefits to 4.4 of accepting this patch?
> * What are the risks in accepting this patch if it turned out to be
> not quite correct?
>
> Re the benefits, I''m guessing the main one is to be able to use
frame
> pointers in an extra case, making the stack more readable on a crash.
>
> The risk, it seems to me, would be if there were other crashes that
> might have garbled stacks that are no longer useful; or, more
> importantly, that if someone was using a debug key to print out the
> hypervisor stack, that, it might cause the whole host to crash.
>
> I''m kind of on the fence on this one -- Jan / Keir, any thoughts?
>
>  -George
Benefits:
* Use frame pointers in the stack overflow case
* Correct boundaries for frame pointer traces
* Wild function pointer semantics in the common case now

Risks:
* Issues with printing stack traces (although if you notice, I haven''t
actually changed either of the printing algorithms)

This series seemed accepted-in-principle at v1 ages ago, pending me
confirming the boundaries. (which have admittedly be tweaked in this
latest series).

~Andrew

On 11/19/2013 10:50 AM, Andrew Cooper wrote:
> On 19/11/2013 10:10, George Dunlap wrote:
>> On Mon, Nov 18, 2013 at 7:34 PM, Andrew Cooper
>> <andrew.cooper3@citrix.com> wrote:
>>> This series consists of improvements to Xen''s ability to
print traces of its
>>> own stack, and specifically for the stack overflow case to be able
to use
>>> frame pointers in a debug build.
>>>
>>> I have dev tested the series in debug and non-debug cases, with and
without
>>> memory guards, and I believe that all the stack traces look correct
(given the
>>> available information Xen has), and that the boundaries are now
correct. This
>>> series has had a substantial rebase on top of the %pS series.
>>>
>>> George: Regarding the 4.4 code, I would like to argue this as a
bugfix rather
>>> than feature, therefore being exempt from the freeze at the moment.
>> Well that argument is BS.  It''s not a bug fix; it''s
clearly exactly
>> what the series summary describes it as -- an improvement.
>>
>> The questions you need to answer are:
>> * What are the benefits to 4.4 of accepting this patch?
>> * What are the risks in accepting this patch if it turned out to be
>> not quite correct?
>>
>> Re the benefits, I''m guessing the main one is to be able to
use frame
>> pointers in an extra case, making the stack more readable on a crash.
>>
>> The risk, it seems to me, would be if there were other crashes that
>> might have garbled stacks that are no longer useful; or, more
>> importantly, that if someone was using a debug key to print out the
>> hypervisor stack, that, it might cause the whole host to crash.
>>
>> I''m kind of on the fence on this one -- Jan / Keir, any
thoughts?
>>
>>   -George
>
> Benefits:
> * Use frame pointers in the stack overflow case
> * Correct boundaries for frame pointer traces
> * Wild function pointer semantics in the common case now
>
> Risks:
> * Issues with printing stack traces (although if you notice, I
haven''t
> actually changed either of the printing algorithms)
>
> This series seemed accepted-in-principle at v1 ages ago, pending me
> confirming the boundaries. (which have admittedly be tweaked in this
> latest series).
That makes more sense, thanks.  If Jan and/or Keir see it that way, then 
the series is fine with me f/ a release perspective.

  -George

On 19/11/2013 03:01, "George Dunlap"
<george.dunlap@eu.citrix.com> wrote:
> On 11/19/2013 10:50 AM, Andrew Cooper wrote:
>> On 19/11/2013 10:10, George Dunlap wrote:
>> Benefits:
>> * Use frame pointers in the stack overflow case
>> * Correct boundaries for frame pointer traces
>> * Wild function pointer semantics in the common case now
>> 
>> Risks:
>> * Issues with printing stack traces (although if you notice, I
haven''t
>> actually changed either of the printing algorithms)
>> 
>> This series seemed accepted-in-principle at v1 ages ago, pending me
>> confirming the boundaries. (which have admittedly be tweaked in this
>> latest series).
> 
> That makes more sense, thanks.  If Jan and/or Keir see it that way, then
> the series is fine with me f/ a release perspective.
Yes, this series should go in.

 -- Keir
>   -George

>>> On 19.11.13 at 17:07, Keir Fraser <keir.xen@gmail.com> wrote:
> On 19/11/2013 03:01, "George Dunlap"
<george.dunlap@eu.citrix.com> wrote:
> 
>> On 11/19/2013 10:50 AM, Andrew Cooper wrote:
>>> On 19/11/2013 10:10, George Dunlap wrote:
> 
>>> Benefits:
>>> * Use frame pointers in the stack overflow case
>>> * Correct boundaries for frame pointer traces
>>> * Wild function pointer semantics in the common case now
>>> 
>>> Risks:
>>> * Issues with printing stack traces (although if you notice, I
haven''t
>>> actually changed either of the printing algorithms)
>>> 
>>> This series seemed accepted-in-principle at v1 ages ago, pending me
>>> confirming the boundaries. (which have admittedly be tweaked in
this
>>> latest series).
>> 
>> That makes more sense, thanks.  If Jan and/or Keir see it that way,
then
>> the series is fine with me f/ a release perspective.
> 
> Yes, this series should go in.
Which I''ll then read as an ack (but I still want to go through the
patches before committing them).

Jan

> +++ b/xen/arch/x86/traps.c
> @@ -195,12 +195,14 @@ static void show_guest_stack(struct vcpu *v, struct
cpu_user_regs *regs)
>  
>  #if !defined(CONFIG_FRAME_POINTER)
>  
> -static void show_trace(struct cpu_user_regs *regs)
> +/*
> + * Stack trace from pointers found in stack, unaided by frame pointers. 
For
> + * caller convenience, this has the same prototype as its alternative, and
> + * simply ignores the base pointer parameter.
> + */
> +static void __show_trace(unsigned long sp, unsigned long __maybe_unused
bp)
A single leading underscore only please.
> -    /*
> -     * If RIP is not pointing into hypervisor code then someone may have
> -     * called into oblivion. Peek to see if they left a return address at
> -     * top of stack.
> -     */
> -    addr = is_active_kernel_text(regs->eip) ||
> -           !is_active_kernel_text(*ESP_BEFORE_EXCEPTION(regs)) ?
Please note the pointer deref here.
> +static void show_trace(const struct cpu_user_regs *regs)
> +{
> +    unsigned long sp = regs->rsp;
> +
> +    printk("Xen call trace:\n");
> +
> +    /*
> +     * If RIP looks sensible, or the top of the stack doesn''t,
print RIP at
> +     * the top of the stack trace.
> +     */
> +    if ( is_active_kernel_text(regs->rip) ||
> +         !is_active_kernel_text(regs->rsp) )
Which is gone here. In this shape, I don''t see what the purpose of
the check is.
> +        printk("   [<%p>] %pS\n", _p(regs->eip),
_p(regs->eip));
> +    /*
> +     * else RIP looks bad but the top of the stack looks good.  Perhaps we
"Else"

Jan

>>> On 18.11.13 at 20:34, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> Move the boundary into current.h along with the other stack manipulation 
> code.
> The boundary is now the word adjacent to a struct cpu_info on the stack.
> 
> This also fixes the somewhat spurious bounds for the case with frame 
> pointers.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> 
> ---
> 
> Changes since v1:
>  * Change printable bottom depending on frame pointers
> ---
>  xen/arch/x86/traps.c          |    9 ++++-----
>  xen/include/asm-x86/current.h |    9 +++++++++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 5917291..d68f93c 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -203,8 +203,9 @@ static void show_guest_stack(struct vcpu *v, struct 
> cpu_user_regs *regs)
>  static void __show_trace(unsigned long sp, unsigned long __maybe_unused
bp)
>  {
>      unsigned long *stack = (unsigned long *)sp, addr;
> +    unsigned long *bottom = (unsigned long 
> *)get_printable_stack_bottom(sp);
>  
> -    while ( ((long)stack & (STACK_SIZE-BYTES_PER_LONG)) != 0 )
> +    while ( stack <= bottom )
>      {
>          addr = *stack++;
>          if ( is_active_kernel_text(addr) )
> @@ -217,12 +218,10 @@ static void __show_trace(unsigned long sp, unsigned 
> long __maybe_unused bp)
>  /* Stack trace from frames in the stack, using frame pointers */
>  static void __show_trace(unsigned long sp, unsigned long bp)
>  {
> -    unsigned long *frame, next, addr, low, high;
> +    unsigned long *frame, next, addr;
>  
>      /* Bounds for range of valid frame pointer. */
> -    low  = sp - 2*sizeof(unsigned long);
> -    high = (low & ~(STACK_SIZE - 1)) + 
> -        (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long));
> +    unsigned long low = sp, high = get_printable_stack_bottom(sp);
>  
>      /* The initial frame pointer. */
>      next = bp;
> diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
> index bec4dbe..c2792ce 100644
> --- a/xen/include/asm-x86/current.h
> +++ b/xen/include/asm-x86/current.h
> @@ -50,6 +50,15 @@ static inline struct cpu_info *get_cpu_info(void)
>  #define get_stack_bottom()                      \
>      ((unsigned long)&get_cpu_info()->guest_cpu_user_regs.es)
>  
> +/*
> + * Get the bottom-of-stack, as useful for printing stack traces.  This is
the
> + * highest word on the stack which might be part of a stack trace, and is 
> the
> + * adjacent word to a struct cpu_info on the stack.
> + */
> +#define get_printable_stack_bottom(sp)          \
> +    ((sp & (~(STACK_SIZE-1))) +                 \
> +     (STACK_SIZE - sizeof(struct cpu_info) - sizeof(unsigned long)))
> +
>  #define reset_stack_and_jump(__fn)              \
>      __asm__ __volatile__ (                      \
>          "mov %0,%%"__OP"sp; jmp %c1"            \
> -- 
> 1.7.10.4

>>> On 18.11.13 at 20:34, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> Pass a full set of cpu_user_regs, and defer the hand-coded stack printing
to
> __show_trace(), which will correctly use frame pointers if available.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> ---
>  xen/arch/x86/traps.c            |   14 ++++----------
>  xen/arch/x86/x86_64/traps.c     |    2 +-
>  xen/include/asm-x86/processor.h |    2 +-
>  3 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index d68f93c..67be3c4 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -316,11 +316,11 @@ void show_stack(struct cpu_user_regs *regs)
>      show_trace(regs);
>  }
>  
> -void show_stack_overflow(unsigned int cpu, unsigned long esp)
> +void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs 
> *regs)
>  {
>  #ifdef MEMORY_GUARD
> +    unsigned long esp = regs->rsp;
>      unsigned long esp_top, esp_bottom;
> -    unsigned long *stack, addr;
>  
>      esp_bottom = (esp | (STACK_SIZE - 1)) + 1;
>      esp_top    = esp_bottom - PRIMARY_STACK_SIZE;
> @@ -340,16 +340,10 @@ void show_stack_overflow(unsigned int cpu, unsigned 
> long esp)
>      if ( esp < esp_top )
>          esp = esp_top;
>  
> -    printk("Xen stack overflow (dumping trace %p-%p):\n   ",
> +    printk("Xen stack overflow (dumping trace %p-%p):\n",
>             (void *)esp, (void *)esp_bottom);
>  
> -    stack = (unsigned long *)esp;
> -    while ( ((long)stack & (STACK_SIZE-BYTES_PER_LONG)) != 0 )
> -    {
> -        addr = *stack++;
> -        if ( is_active_kernel_text(addr) )
> -            printk("%p: [<%p>] %pS\n", stack, _p(addr),
_p(addr));
> -    }
> +    __show_trace(esp, regs->rbp);
>  
>      printk("\n");
>  #endif
> diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
> index 8e6a7c1..bcf72b6 100644
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -248,7 +248,7 @@ void do_double_fault(struct cpu_user_regs *regs)
>  
>      printk("CPU:    %d\n", cpu);
>      _show_registers(regs, crs, CTXT_hypervisor, NULL);
> -    show_stack_overflow(cpu, regs->rsp);
> +    show_stack_overflow(cpu, regs);
>  
>      panic("DOUBLE FAULT -- system shutdown\n");
>  }
> diff --git a/xen/include/asm-x86/processor.h
b/xen/include/asm-x86/processor.h
> index 73a3202..c120460 100644
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -509,7 +509,7 @@ extern always_inline void prefetchw(const void *x)
>  #endif
>  
>  void show_stack(struct cpu_user_regs *regs);
> -void show_stack_overflow(unsigned int cpu, unsigned long esp);
> +void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs 
> *regs);
>  void show_registers(struct cpu_user_regs *regs);
>  void show_execution_state(struct cpu_user_regs *regs);
>  #define dump_execution_state() 
> run_in_exception_handler(show_execution_state)
> -- 
> 1.7.10.4