[v5,14/28] x86, xsplice: Print payload's symbol name and payload name in backtraces

Commit Message

Konrad Rzeszutek Wilk March 24, 2016, 8 p.m. UTC

Naturally the backtrace is presented when an instruction
hits an bug_frame or %p is used.

The payloads do not support bug_frames yet - however the functions
the payloads call could hit an BUG() or WARN().

The traps.c has logic to scan for it this - and eventually it will
find the correct bug_frame and the walk the stack using %p to print
the backtrace. For %p and symbols to print a string -  the
'is_active_kernel_text' is consulted which uses an 'struct virtual_region'.

Therefore we register our start->end addresses so that
'is_active_kernel_text' will include our payload address.

We also register our symbol lookup table function so that it can
scan the list of payloads and retrieve the correct name.

Lastly we change vsprintf to take into account s and namebuf.
For core code they are the same, but for payloads they are different.
This gets us:

Xen call trace:
   [<ffff82d080a00041>] revert_hook+0x31/0x35 [xen_hello_world]
   [<ffff82d0801431bd>] xsplice.c#revert_payload+0x86/0xc6
   [<ffff82d080143502>] check_for_xsplice_work+0x233/0x3cd
   [<ffff82d08017a0b2>] domain.c#continue_idle_domain+0x9/0x1f

Which is great if payloads have similar or same symbol names.

Signed-off-by: Ross Lagerwall <[e-mail]>
Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>

---
Cc: Ian Jackson <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Keir Fraser <[e-mail]>
Cc: Tim Deegan <[e-mail]>

v2: Add missing full stop.
v3: s/module/payload/
v4: Expand comment and include registration of 'virtual_region'
    Redo the vsprintf handling of payload name.
    Drop the ->skip function
---
 xen/common/vsprintf.c | 15 ++++++++++---
 xen/common/xsplice.c  | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 3 deletions(-)

Comments

Jan Beulich April 1, 2016, 3:23 p.m. UTC | #1

>>> On 24.03.16 at 21:00, <[e-mail]> wrote:
> @@ -331,16 +332,17 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>      {
>          unsigned long sym_size, sym_offset;
>          char namebuf[KSYM_NAME_LEN+1];
> +        bool_t payload = 0;
>  
>          /* Advance parents fmt string, as we have consumed 's' or 'S' */
>          ++*fmt_ptr;
>  
>          s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf);
> -
> -        /* If the symbol is not found, fall back to printing the address */
> +        /* If the symbol is not found, fall back to printing the address. */
>          if ( !s )
>              break;
> -

Please don't drop blank lines like this.

> +        if ( strncmp(namebuf, s, KSYM_NAME_LEN) )
> +            payload = 1;

What is this about? A comment is absolutely needed here, the
more that without context "payload" is also an unclear term.
And then - would simply comparing the two pointers suffice?

> +static const char *xsplice_symbols_lookup(unsigned long addr,
> +                                          unsigned long *symbolsize,
> +                                          unsigned long *offset,
> +                                          char *namebuf)
> +{
> +    struct payload *data;
> +    unsigned int i;
> +    int best;
> +
> +    /*
> +     * No locking since this list is only ever changed during apply or revert
> +     * context.
> +     */
> +    list_for_each_entry ( data, &applied_list, applied_list )
> +    {
> +        if ( !((void *)addr >= data->text_addr &&
> +               (void *)addr < (data->text_addr + data->text_size)) )
> +            continue;

It may be just me, but I find such !() constructs harder to understand
than the equivalent without that negation.

> +        best = -1;
> +
> +        for ( i = 0; i < data->nsyms; i++ )
> +        {
> +            if ( data->symtab[i].value <= addr &&
> +                 ( best == -1 ||

Stray blank.

Jan

Konrad Rzeszutek Wilk April 6, 2016, 2:39 a.m. UTC | #2

On Fri, Apr 01, 2016 at 09:23:15AM -0600, Jan Beulich wrote:
> >>> On 24.03.16 at 21:00, <[e-mail]> wrote:
> > @@ -331,16 +332,17 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
> >      {
> >          unsigned long sym_size, sym_offset;
> >          char namebuf[KSYM_NAME_LEN+1];
> > +        bool_t payload = 0;
> >  
> >          /* Advance parents fmt string, as we have consumed 's' or 'S' */
> >          ++*fmt_ptr;
> >  
> >          s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf);
> > -
> > -        /* If the symbol is not found, fall back to printing the address */
> > +        /* If the symbol is not found, fall back to printing the address. */
> >          if ( !s )
> >              break;
> > -
> 
> Please don't drop blank lines like this.
> 
> > +        if ( strncmp(namebuf, s, KSYM_NAME_LEN) )
> > +            payload = 1;
> 
> What is this about? A comment is absolutely needed here, the
> more that without context "payload" is also an unclear term.
> And then - would simply comparing the two pointers suffice?

Sadly no. The namebuf is defined on the stack (and the value copied in)
while the 's' is a pointer to the char* in the symbols.

For all Xen hypervisor built-in symbols the 'namebuf' contents and 's'
are exactly the same. The only difference is when xSplice comes in
and the 'namebuf' has the name of the xSplice payload.

I will put a comment here.

Jan Beulich April 7, 2016, 1:07 a.m. UTC | #3

>>> Konrad Rzeszutek Wilk <[e-mail]> 04/06/16 4:39 AM >>>
>On Fri, Apr 01, 2016 at 09:23:15AM -0600, Jan Beulich wrote:
>> >>> On 24.03.16 at 21:00, <[e-mail]> wrote:
>> > @@ -331,16 +332,17 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>> >      {
>> >          unsigned long sym_size, sym_offset;
>> >          char namebuf[KSYM_NAME_LEN+1];
>> > +        bool_t payload = 0;
>> >  
>> >          /* Advance parents fmt string, as we have consumed 's' or 'S' */
>> >          ++*fmt_ptr;
>> >  
>> >          s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf);
>> > -
>> > -        /* If the symbol is not found, fall back to printing the address */
>> > +        /* If the symbol is not found, fall back to printing the address. */
>> >          if ( !s )
>> >              break;
>> > -
>> > +        if ( strncmp(namebuf, s, KSYM_NAME_LEN) )
>> > +            payload = 1;
>> 
>> What is this about? A comment is absolutely needed here, the
>> more that without context "payload" is also an unclear term.
>> And then - would simply comparing the two pointers suffice?
>
>Sadly no. The namebuf is defined on the stack (and the value copied in)
>while the 's' is a pointer to the char* in the symbols.

How does one being on the stack and the other in (I assume) a string table
prevent their pointers to be compared? Iirc for the in-hypervisor symbols
symbols_lookup() simply returns the passed in namebuf, and hence
s == namebuf above. Or I must be missing something...

Jan

Patch

diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index 18d2634..f0b743f 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -20,6 +20,7 @@ 
 #include <xen/symbols.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
+#include <xen/xsplice.h>
 #include <asm/div64.h>
 #include <asm/page.h>
 
@@ -331,16 +332,17 @@  static char *pointer(char *str, char *end, const char **fmt_ptr,
     {
         unsigned long sym_size, sym_offset;
         char namebuf[KSYM_NAME_LEN+1];
+        bool_t payload = 0;
 
         /* Advance parents fmt string, as we have consumed 's' or 'S' */
         ++*fmt_ptr;
 
         s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf);
-
-        /* If the symbol is not found, fall back to printing the address */
+        /* If the symbol is not found, fall back to printing the address. */
         if ( !s )
             break;
-
+        if ( strncmp(namebuf, s, KSYM_NAME_LEN) )
+            payload = 1;
         /* Print symbol name */
         str = string(str, end, s, -1, -1, 0);
 
@@ -354,6 +356,13 @@  static char *pointer(char *str, char *end, const char **fmt_ptr,
             str = number(str, end, sym_size, 16, -1, -1, SPECIAL);
         }
 
+        if ( payload )
+        {
+            str = string(str, end, " [", -1, -1, 0);
+            str = string(str, end, namebuf, -1, -1, 0);
+            str = string(str, end, "]", -1, -1, 0);
+        }
+
         return str;
     }
 
diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index 24100e7..5e77360 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -14,7 +14,9 @@ 
 #include <xen/smp.h>
 #include <xen/softirq.h>
 #include <xen/spinlock.h>
+#include <xen/string.h>
 #include <xen/symbols.h>
+#include <xen/virtual_region.h>
 #include <xen/vmap.h>
 #include <xen/wait.h>
 #include <xen/xsplice_elf.h>
@@ -55,6 +57,8 @@  struct payload {
     struct list_head applied_list;       /* Linked to 'applied_list'. */
     struct xsplice_patch_func *funcs;    /* The array of functions to patch. */
     unsigned int nfuncs;                 /* Nr of functions to patch. */
+    struct virtual_region region;        /* symbol, bug.frame patching and
+                                            exception table (x86). */
     struct xsplice_symbol *symtab;       /* All symbols. */
     char *strtab;                        /* Pointer to .strtab. */
     unsigned int nsyms;                  /* Nr of entries in .strtab and symbols. */
@@ -139,6 +143,51 @@  out:
     return value;
 }
 
+static const char *xsplice_symbols_lookup(unsigned long addr,
+                                          unsigned long *symbolsize,
+                                          unsigned long *offset,
+                                          char *namebuf)
+{
+    struct payload *data;
+    unsigned int i;
+    int best;
+
+    /*
+     * No locking since this list is only ever changed during apply or revert
+     * context.
+     */
+    list_for_each_entry ( data, &applied_list, applied_list )
+    {
+        if ( !((void *)addr >= data->text_addr &&
+               (void *)addr < (data->text_addr + data->text_size)) )
+            continue;
+
+        best = -1;
+
+        for ( i = 0; i < data->nsyms; i++ )
+        {
+            if ( data->symtab[i].value <= addr &&
+                 ( best == -1 ||
+                   data->symtab[best].value < data->symtab[i].value) )
+                best = i;
+        }
+
+        if ( best == -1 )
+            return NULL;
+
+        if ( symbolsize )
+            *symbolsize = data->symtab[best].size;
+        if ( offset )
+            *offset = addr - data->symtab[best].value;
+        if ( namebuf )
+            strlcpy(namebuf, data->name, KSYM_NAME_LEN);
+
+        return data->symtab[best].name;
+    }
+
+    return NULL;
+}
+
 /*
  * We may be holding the payload_lock or not. Hence we need
  * the recursive spinlock. Or we can judiciously use an
@@ -394,6 +443,7 @@  static int prepare_payload(struct payload *payload,
     struct xsplice_elf_sec *sec;
     unsigned int i;
     struct xsplice_patch_func *f;
+    struct virtual_region *region;
 
     sec = xsplice_elf_sec_by_name(elf, ".xsplice.funcs");
     if ( sec )
@@ -449,6 +499,13 @@  static int prepare_payload(struct payload *payload,
         }
     }
 
+    /* Setup the virtual region with proper data. */
+    region = &payload->region;
+
+    region->symbols_lookup = xsplice_symbols_lookup;
+    region->start = (unsigned long)payload->text_addr;
+    region->end = (unsigned long)(payload->text_addr + payload->text_size);
+
     return 0;
 }
 
@@ -795,6 +852,7 @@  static int apply_payload(struct payload *data)
     arch_xsplice_patching_leave();
 
     list_add_tail(&data->applied_list, &applied_list);
+    register_virtual_region(&data->region);
 
     return 0;
 }
@@ -817,6 +875,7 @@  static int revert_payload(struct payload *data)
     arch_xsplice_patching_leave();
 
     list_del_init(&data->applied_list);
+    unregister_virtual_region(&data->region);
 
     return 0;
 }