[RFC Patch 0/2] Improvements to stack traces

[RFC Patch 0/2] Improvements to stack traces

[Andrew Cooper]

This series is RFC for two reasons; firstly because I have not dev-tested it
yet, but mainly because of a specific question.

In the algorithm using frame pointers, the lower bound is adjusted by two
words from the provided stack pointer.

This appears to be the behaiour right from its introduction in:

    commit aa24d38a469b59abf1b95b732b6ea9ed86e511cf
    Author: kaf24@firebug.cl.cam.ac.uk <kaf24@firebug.cl.cam.ac.uk>
    Date:   Thu Sep 1 15:31:12 2005 +0000

What is the reason for the adjustment?  Tim and I couldn't think of a case
where a valid frame pointer could be outside the stack. Any well formed use of
frame pointers should require the callee to push the old frame pointer at
entry, and pop it on right before exit.

Am I missing something obvious?

The potential problem comes in the stack overflow case, where rsp points to
the boundary of the primary stack, and rbp points just below it, at which
point the bounday condition will pass but referencing rbp will cause a triple
fault.

This can be detected and worked around, but if the adjustment is erronious
then by far the easiest solution is to just discard the adjustment.

~Andrew

CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

--
1.7.10.4

[RFC Patch 1/2] x86/traps: 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>

---

The new wild pointer logic is rather larger than its pre-refactor version, but
is rather more legible.  I cant think of a way to compact it without making it
substantially less legible.
---
 xen/arch/x86/traps.c |   66 +++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 27 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 57dbd0c..324b0f1 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -192,14 +192,13 @@ 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 rbp parameter.
+ */
+static void __show_trace(unsigned long sp, unsigned long bp)
 {
-    unsigned long *stack = ESP_BEFORE_EXCEPTION(regs), addr;
-
-    printk("Xen call trace:\n   ");
-
-    printk("[<%p>]", _p(regs->eip));
-    print_symbol(" %s\n   ", regs->eip);
+    unsigned long *stack = (unsigned long *)sp, addr;
 
     while ( ((long)stack & (STACK_SIZE-BYTES_PER_LONG)) != 0 )
     {
@@ -210,36 +209,22 @@ static void show_trace(struct cpu_user_regs *regs)
             print_symbol(" %s\n   ", 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;
 
-    printk("Xen call trace:\n   ");
-
-    /*
-     * 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("[<%p>]", _p(addr));
-    print_symbol(" %s\n   ", 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 ( ; ; )
     {
@@ -272,12 +257,39 @@ 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 look sensible,
+     * print RIP at the top of the stack trace. */
+    if ( is_active_kernel_text(regs->rip) ||
+         !is_active_kernel_text(regs->rsp) )
+    {
+        printk("[<%p>]", _p(regs->rip));
+        print_symbol(" %s\n   ", regs->rip);
+    }
+    /* else RIP looks bad but the top of the stack looks ok.  Perhaps we
+     * followed a wild function pointer, so lets assume the top of the stack is
+     * a return address.  Skip past it so__show_trace() doesn't print it
+     * again. */
+    else
+    {
+        printk("[<%p>]", _p(sp));
+        print_symbol(" %s\n   ", 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

[RFC Patch 2/2] x86/traps: 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.

One issue in the case with frame pointer; subtracting two words from the stack
pointer and using it as a boundary condition might now result in a triple
fault.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/traps.c            |   19 +++++--------------
 xen/arch/x86/x86_64/traps.c     |    2 +-
 xen/include/asm-x86/processor.h |    2 +-
 3 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 324b0f1..05e5b74 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -219,8 +219,8 @@ static void __show_trace(unsigned long sp, unsigned long bp)
     unsigned long *frame, next, addr, low, high;
 
     /* Bounds for range of valid frame pointer. */
-    low  = sp - 2*sizeof(unsigned long);
-    high = (low & ~(STACK_SIZE - 1)) + 
+    low = sp;
+    high = (low & ~(STACK_SIZE - 1)) +
         (STACK_SIZE - sizeof(struct cpu_info) - 2*sizeof(unsigned long));
 
     /* The initial frame pointer. */
@@ -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;
@@ -343,16 +343,7 @@ void show_stack_overflow(unsigned int cpu, unsigned long esp)
     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>]", stack, _p(addr));
-            print_symbol(" %s\n   ", 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 bcd7609..385b366 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -247,7 +247,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 5cdacc7..d050bac 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -507,7 +507,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

Re: [RFC Patch 0/2] Improvements to stack traces

[Keir Fraser]

On 08/08/2013 17:19, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

> This series is RFC for two reasons; firstly because I have not dev-tested it
> yet, but mainly because of a specific question.
> 
> In the algorithm using frame pointers, the lower bound is adjusted by two
> words from the provided stack pointer.
> 
> This appears to be the behaiour right from its introduction in:
> 
>     commit aa24d38a469b59abf1b95b732b6ea9ed86e511cf
>     Author: kaf24@firebug.cl.cam.ac.uk <kaf24@firebug.cl.cam.ac.uk>
>     Date:   Thu Sep 1 15:31:12 2005 +0000
> 
> What is the reason for the adjustment?  Tim and I couldn't think of a case
> where a valid frame pointer could be outside the stack. Any well formed use of
> frame pointers should require the callee to push the old frame pointer at
> entry, and pop it on right before exit.
> 
> Am I missing something obvious?
> 
> The potential problem comes in the stack overflow case, where rsp points to
> the boundary of the primary stack, and rbp points just below it, at which
> point the bounday condition will pass but referencing rbp will cause a triple
> fault.
> 
> This can be detected and worked around, but if the adjustment is erronious
> then by far the easiest solution is to just discard the adjustment.

I think it was just an attempt at paranoia when I implemented this. I'm
happy for it to be ripped out.

 -- Keir

> ~Andrew
> 
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> 
> --
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel