[Xen-devel] [PATCH v3 0/2] x86/traps: improve show_trace()'s top-of-stack handling

Jan Beulich posted 2 patches 4 years, 8 months ago
Only 0 patches received!
[Xen-devel] [PATCH v3 0/2] x86/traps: improve show_trace()'s top-of-stack handling
Posted by Jan Beulich 4 years, 8 months ago
1: guard top-of-stack reads
2: widen condition for logging top-of-stack

The issue patch 2 fixes (a curious lack of an intermediate call stack
entry) was observed in practice; patch 1 is a result of me just looking
at the code.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3 1/2] x86/traps: guard top-of-stack reads
Posted by Jan Beulich 4 years, 8 months ago
Nothing guarantees that the original frame's stack pointer points at
readable memory. Avoid a (likely nested) crash by attaching exception
recovery to the read (making it a single read at the same time). Don't
even invoke _show_trace() in case of a non-readable top slot.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: Name asm() arguments. Use explicit "fault" variable.

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -486,17 +486,31 @@ static void _show_trace(unsigned long sp
  
  static void show_trace(const struct cpu_user_regs *regs)
  {
-    unsigned long *sp = ESP_BEFORE_EXCEPTION(regs);
+    unsigned long *sp = ESP_BEFORE_EXCEPTION(regs), tos = 0;
+    bool fault = false;
  
      printk("Xen call trace:\n");
  
+    /* Guarded read of the stack top. */
+    asm ( "1: mov %[data], %[tos]; 2:\n"
+          ".pushsection .fixup,\"ax\"\n"
+          "3: movb $1, %[fault]; jmp 2b\n"
+          ".popsection\n"
+          _ASM_EXTABLE(1b, 3b)
+          : [tos] "+r" (tos), [fault] "+qm" (fault) : [data] "m" (*sp) );
+
      /*
       * 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) )
+         !is_active_kernel_text(tos) )
          printk("   [<%p>] %pS\n", _p(regs->rip), _p(regs->rip));
+    else if ( fault )
+    {
+        printk("   [Fault on access]\n");
+        return;
+    }
      /*
       * 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
@@ -505,7 +519,7 @@ static void show_trace(const struct cpu_
       */
      else
      {
-        printk("   [<%p>] %pS\n", _p(*sp), _p(*sp));
+        printk("   [<%p>] %pS\n", _p(tos), _p(tos));
          sp++;
      }
  

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/2] x86/traps: guard top-of-stack reads
Posted by Andrew Cooper 4 years, 6 months ago
On 15/07/2019 16:00, Jan Beulich wrote:
> Nothing guarantees that the original frame's stack pointer points at
> readable memory. Avoid a (likely nested) crash by attaching exception
> recovery to the read (making it a single read at the same time). Don't
> even invoke _show_trace() in case of a non-readable top slot.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/2] x86/traps: guard top-of-stack reads
Posted by Jan Beulich 4 years, 6 months ago
On 23.09.2019 16:20, Andrew Cooper wrote:
> On 15/07/2019 16:00, Jan Beulich wrote:
>> Nothing guarantees that the original frame's stack pointer points at
>> readable memory. Avoid a (likely nested) crash by attaching exception
>> recovery to the read (making it a single read at the same time). Don't
>> even invoke _show_trace() in case of a non-readable top slot.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

With this, ...

> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

... was this perhaps meant for patch 2 of this short series?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/2] x86/traps: guard top-of-stack reads
Posted by Andrew Cooper 4 years, 6 months ago
On 23/09/2019 16:12, Jan Beulich wrote:
> On 23.09.2019 16:20, Andrew Cooper wrote:
>> On 15/07/2019 16:00, Jan Beulich wrote:
>>> Nothing guarantees that the original frame's stack pointer points at
>>> readable memory. Avoid a (likely nested) crash by attaching exception
>>> recovery to the read (making it a single read at the same time). Don't
>>> even invoke _show_trace() in case of a non-readable top slot.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> With this, ...
>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ... was this perhaps meant for patch 2 of this short series?

No.  I didn't spot my R-b tag and only had enough free time to read the
first email.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3 2/2] x86/traps: widen condition for logging top-of-stack
Posted by Jan Beulich 4 years, 8 months ago
Despite -fno-omit-frame-pointer the compiler may omit the frame pointer,
often for relatively simple leaf functions. (To give a specific example,
the case I've run into this with is _pci_hide_device() and gcc 8.
Interestingly the even more simple neighboring iommu_has_feature() does
get a frame pointer set up, around just a single instruction. But this
may be a result of the size-of-asm() effects discussed elsewhere.)

Log the top-of-stack value if it looks valid _or_ if RIP looks invalid.

Also annotate all stack trace entries with a marker, to indicate their
origin:
R: register state
F: frame pointer based
S: raw stack contents

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Tag stack entries consistently, but differently than in v2.
v2: Re-base over changes to earlier patch.

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -433,7 +433,7 @@ static void _show_trace(unsigned long sp
      {
          addr = *stack++;
          if ( is_active_kernel_text(addr) )
-            printk("   [<%p>] %pS\n", _p(addr), _p(addr));
+            printk("   [<%p>] S %pS\n", _p(addr), _p(addr));
      }
  }
  
@@ -476,7 +476,7 @@ static void _show_trace(unsigned long sp
              addr  = frame[1];
          }
  
-        printk("   [<%p>] %pS\n", _p(addr), _p(addr));
+        printk("   [<%p>] F %pS\n", _p(addr), _p(addr));
  
          low = (unsigned long)&frame[2];
      }
@@ -505,21 +505,26 @@ static void show_trace(const struct cpu_
       */
      if ( is_active_kernel_text(regs->rip) ||
           !is_active_kernel_text(tos) )
-        printk("   [<%p>] %pS\n", _p(regs->rip), _p(regs->rip));
-    else if ( fault )
+        printk("   [<%p>] R %pS\n", _p(regs->rip), _p(regs->rip));
+
+    if ( fault )
      {
          printk("   [Fault on access]\n");
          return;
      }
+
      /*
-     * 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
+     * If RIP looks bad or the top of the stack looks good, log the top of
+     * stack as well.  Perhaps we followed a wild function pointer, or we're
+     * in a function without frame pointer, or in a function prologue before
+     * the frame pointer gets set up?  Let's assume the top of the stack is a
       * return address; print it and skip past so _show_trace() doesn't print
       * it again.
       */
-    else
+    if ( !is_active_kernel_text(regs->rip) ||
+         is_active_kernel_text(tos) )
      {
-        printk("   [<%p>] %pS\n", _p(tos), _p(tos));
+        printk("   [<%p>] S %pS\n", _p(tos), _p(tos));
          sp++;
      }
  

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 2/2] x86/traps: widen condition for logging top-of-stack
Posted by Andrew Cooper 4 years, 6 months ago
On 15/07/2019 16:01, Jan Beulich wrote:
> Despite -fno-omit-frame-pointer the compiler may omit the frame pointer,
> often for relatively simple leaf functions. (To give a specific example,
> the case I've run into this with is _pci_hide_device() and gcc 8.
> Interestingly the even more simple neighboring iommu_has_feature() does
> get a frame pointer set up, around just a single instruction. But this
> may be a result of the size-of-asm() effects discussed elsewhere.)
>
> Log the top-of-stack value if it looks valid _or_ if RIP looks invalid.
>
> Also annotate all stack trace entries with a marker, to indicate their
> origin:
> R: register state
> F: frame pointer based
> S: raw stack contents
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] Ping: [PATCH v3 0/2] x86/traps: improve show_trace()'s top-of-stack handling
Posted by Jan Beulich 4 years, 6 months ago
Andrew,

On 15.07.2019 16:58, Jan Beulich wrote:
> 1: guard top-of-stack reads
> 2: widen condition for logging top-of-stack
> 
> The issue patch 2 fixes (a curious lack of an intermediate call stack
> entry) was observed in practice; patch 1 is a result of me just looking
> at the code.

I realize you're not in the office for the next several days (and
in particular not until code freeze), but I think this is another
item which would be nice if we could have in 4.13, and it's been
pending for quite some time. Of course, if we can't reach agreement
on how this should work, then so be it (and I'll carry the patches
locally for the foreseeable future) ...

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel