thr3ads.net - Xen devel - [Patch v4 0/4] Xen stack trace printing improvements [Nov 2013]

If this information is useful, please help other people find it:
Share via:

Andrew Cooper

2013-Nov-20 13:09 UTC

[Patch v4 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.

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>

Andrew Cooper

2013-Nov-20 13:09 UTC

head link

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

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

Andrew Cooper

2013-Nov-20 13:09 UTC

head link

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

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

Andrew Cooper

2013-Nov-20 13:09 UTC

head link

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

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

Andrew Cooper

2013-Nov-20 13:09 UTC

head link

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

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

Jan Beulich

2013-Nov-20 13:40 UTC

head link

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

>>> 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

Andrew Cooper

2013-Nov-20 14:11 UTC

head link

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

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

Jan Beulich

2013-Nov-20 14:22 UTC

head link

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

>>> 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

Andrew Cooper

2013-Nov-20 14:30 UTC

head link

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

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

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

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

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

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

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

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

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

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

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

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