[Patch v4 0/4] Xen stack trace printing improvements

[Patch v4 0/4] Xen stack trace printing improvements

[Andrew Cooper]

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.

v4 contains some fixes to the wild function pointer call case, including a
test case.  There was a missed indirection in the test, and when printing the
return address.

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.

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>

[Patch v4 1/4] x86/stack: Refactor show_trace()

[Andrew Cooper]

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>
Acked-by: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

---
Changes in v4:
 * Single underscore on _show_trace()
 * Undo accidental removal of deference in the wild function pointer test
 * Correct the printing of the return address in the wild pointer case
---
 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..2906641 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 = ESP_BEFORE_EXCEPTION(regs);
+
+    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(*sp) )
+        printk("   [<%p>] %pS\n", _p(regs->rip), _p(regs->rip));
+    /*
+     * 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++;
+    }
+
+    _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

[Patch v4 2/4] x86/stack: Adjust boundary conditions for printed stacks.

[Andrew Cooper]

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>
Acked-by: 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 2906641..231d0fa 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

[Patch v4 3/4] x86/stack: Change show_stack_overflow() to use frame pointers if available

[Andrew Cooper]

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>
Acked-by: Keir Fraser <keir@xen.org>
Reviewed-by: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

---
Changes in v4:
 * knockon effects from changes in patch 1
---
 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 231d0fa..f2be1c0 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

[Patch v4 4/4] DO NOT APPLY: Test code for interesting stack overflows

[Andrew Cooper]

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        |  110 +++++++++++++++++++++++++++++++++++++++++++
 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, 147 insertions(+)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index f2be1c0..2a7fb55 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,115 @@ 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);
+}
+
+typedef void (*fn_ptr)(void);
+static void stacktest4(unsigned char key, struct cpu_user_regs * regs)
+{
+    volatile fn_ptr wild_function = NULL;
+    printk("In %s()\n", __func__);
+    wild_function();
+}
+
+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."
+    },
+
+    {
+        .irq_callback = 1,
+        .u.irq_fn = stacktest4,
+        .desc = "Xen stack #4 - Wild function pointer call."
+    },
+ };
+
+static int __init initialize_crashtable(void)
+{
+    register_keyhandler('1', &stacktest_handler[0]);
+    register_keyhandler('2', &stacktest_handler[1]);
+    register_keyhandler('3', &stacktest_handler[2]);
+    register_keyhandler('4', &stacktest_handler[3]);
+    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

Re: [Patch v4 1/4] x86/stack: Refactor show_trace()

[Jan Beulich]

>>> On 20.11.13 at 14:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> +static void show_trace(const struct cpu_user_regs *regs)
> +{
> +    unsigned long *sp = ESP_BEFORE_EXCEPTION(regs);

So you correctly made it a pointer here...

> +
> +    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(*sp) )

... and de-reference it here ...

> +        printk("   [<%p>] %pS\n", _p(regs->rip), _p(regs->rip));
> +    /*
> +     * 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++;
> +    }
> +
> +    _show_trace(*sp, regs->rbp);

... but then you now also de-reference it here? How did that end
up producing sane stack dumps?

Since one of the two _show_trace() variants wants a pointer here
anyway, you would probably best switch its first argument and use
the inverse casting in the other variant.

Jan

Re: [Patch v4 1/4] x86/stack: Refactor show_trace()

[Andrew Cooper]

On 20/11/13 13:40, Jan Beulich wrote:
>>>> On 20.11.13 at 14:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> +static void show_trace(const struct cpu_user_regs *regs)
>> +{
>> +    unsigned long *sp = ESP_BEFORE_EXCEPTION(regs);
> So you correctly made it a pointer here...
>
>> +
>> +    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(*sp) )
> ... and de-reference it here ...
>
>> +        printk("   [<%p>] %pS\n", _p(regs->rip), _p(regs->rip));
>> +    /*
>> +     * 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++;
>> +    }
>> +
>> +    _show_trace(*sp, regs->rbp);
> ... but then you now also de-reference it here? How did that end
> up producing sane stack dumps?
>
> Since one of the two _show_trace() variants wants a pointer here
> anyway, you would probably best switch its first argument and use
> the inverse casting in the other variant.
>
> Jan
>

Huh - I appear to have tested the debug build twice rather than 1 debug
and 1 non-debug build.  I clearly need to be rather more careful!

I am not so certain about changing the _show_trace() prototype.  The
naive algorithm wants everything as pointers, while the frame-pointer
algorithm wants everything as integers.  Switching to passing by pointer
would require an equal amount of ugly casting back to an integer for
get_printable_stack_bottom() etc.

Personally I would prefer to keep as integers to be in line with the
registers;  I think it looks more natural to take sp as a resister value
and cast it to a pointer to look at the stack, than passing around a
pointer and casting it to an integer for bounds.

~Andrew

Re: [Patch v4 1/4] x86/stack: Refactor show_trace()

[Jan Beulich]

>>> On 20.11.13 at 15:11, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> I am not so certain about changing the _show_trace() prototype.  The
> naive algorithm wants everything as pointers, while the frame-pointer
> algorithm wants everything as integers.  Switching to passing by pointer
> would require an equal amount of ugly casting back to an integer for
> get_printable_stack_bottom() etc.
> 
> Personally I would prefer to keep as integers to be in line with the
> registers;  I think it looks more natural to take sp as a resister value
> and cast it to a pointer to look at the stack, than passing around a
> pointer and casting it to an integer for bounds.

Fine with me, this was just a suggestion.

Jan

[Patch v5 1/4] x86/stack: Refactor show_trace()

[Andrew Cooper]

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>
Acked-by: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

---
Changes in v5:
 * Fix inappropriate deference of stack pointer.  This is now correctly tested
   in both a debug and non-debug build of Xen.

Changes in v4:
 * Single underscore on _show_trace()
 * Undo accidental removal of deference in the wild function pointer test
 * Correct the printing of the return address in the wild pointer case
---
 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..8a01065 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 = ESP_BEFORE_EXCEPTION(regs);
+
+    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(*sp) )
+        printk("   [<%p>] %pS\n", _p(regs->rip), _p(regs->rip));
+    /*
+     * 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++;
+    }
+
+    _show_trace((unsigned long)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