Re: [Xen-devel] [PATCH v9 16/27] x86, xsplice: Print payload's symbol name and payload name in backtraces

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

> >> When text_addr is void *, how is this calculation wrong then?
> > 
> > I'm sorry, ignore that. I temporarily forgot how void* arithmetic is 
> > defined for GCC.
> > 
> > The other two points are still valid and may result in incorrect 
> > backtraces with > 1 payload loaded.
> 
> Of course.

So it would look like this then:

From affca85da4d57c466cc3a603afa4d57fea7ed092 Mon Sep 17 00:00:00 2001
From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
Date: Fri, 22 Apr 2016 11:16:36 -0400
Subject: [PATCH] x86, xsplice: Print payload's symbol name and payload name in
 backtraces

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 <ross.lagerwall@xxxxxxxxxx>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

---
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Keir Fraser <keir@xxxxxxx>
Cc: Tim Deegan <tim@xxxxxxx>

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
v6: Add comment explaining the purpose behind the strcmp.
    Redid per Jan's review.
v7: Add Andrew's Review-by
    Drop the strcmp and just do pointer checks.
v9: Do pointer comparison on vsprintf by itself, no need for intermediate
    payload bool_t
    Add const in xsplice_symbols_lookup
    Make 'best' in xsplice_symbols_lookup be unsigned int.
    Use an RCU list for iterating the applied_list. Define the RCU lock.
v10:
    In xsplice_symbols_lookup use || instead of && when skipping.
    Also in xsplice_symbols_lookup use ->text_size instead of ->pages.
---
---
 xen/common/vsprintf.c     | 12 +++++++++
 xen/common/xsplice.c      | 69 ++++++++++++++++++++++++++++++++++++++++++++---
 xen/include/xen/xsplice.h |  1 +
 3 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index 18d2634..70e1edf 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>
 
@@ -354,6 +355,17 @@ static char *pointer(char *str, char *end, const char 
**fmt_ptr,
             str = number(str, end, sym_size, 16, -1, -1, SPECIAL);
         }
 
+        /*
+         * namebuf contents and s for core hypervisor are same but for xSplice
+         * payloads they differ (namebuf contains the name of the payload).
+         */
+        if ( namebuf != s )
+        {
+            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 85af98a..188b850 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>
@@ -31,10 +33,9 @@ static LIST_HEAD(payload_list);
 
 /*
  * Patches which have been applied. Need RCU in case we crash (and then
- * traps code would iterate via applied_list) when adding entries on the list.
- *
- * Note: There are no 'rcu_applied_lock' as we don't iterate yet the list.
+ * traps code would iterate via applied_list) when adding entries onthe list.
  */
+static DEFINE_RCU_READ_LOCK(rcu_applied_lock);
 static LIST_HEAD(applied_list);
 
 static unsigned int payload_cnt;
@@ -56,6 +57,8 @@ struct payload {
     unsigned int nfuncs;                 /* Nr of functions to patch. */
     const struct xsplice_symbol *symtab; /* All symbols. */
     const char *strtab;                  /* Pointer to .strtab. */
+    struct virtual_region region;        /* symbol, bug.frame patching and
+                                            exception table (x86). */
     unsigned int nsyms;                  /* Nr of entries in .strtab and 
symbols. */
     char name[XEN_XSPLICE_NAME_SIZE];    /* Name of it. */
 };
@@ -139,6 +142,55 @@ void *xsplice_symbols_lookup_by_name(const char *symname)
     return 0;
 }
 
+static const char *xsplice_symbols_lookup(unsigned long addr,
+                                          unsigned long *symbolsize,
+                                          unsigned long *offset,
+                                          char *namebuf)
+{
+    const struct payload *data;
+    unsigned int i, best;
+    const void *va = (const void *)addr;
+    const char *n = NULL;
+
+    /*
+     * Only RCU locking since this list is only ever changed during apply
+     * or revert context. And in case it dies there we need an safe list.
+     */
+    rcu_read_lock(&rcu_applied_lock);
+    list_for_each_entry_rcu ( data, &applied_list, applied_list )
+    {
+        if ( va < data->text_addr ||
+             va >= (data->text_addr + data->text_size) )
+            continue;
+
+        best = UINT_MAX;
+
+        for ( i = 0; i < data->nsyms; i++ )
+        {
+            if ( data->symtab[i].value <= va &&
+                 (best == UINT_MAX ||
+                  data->symtab[best].value < data->symtab[i].value) )
+                best = i;
+        }
+
+        if ( best == UINT_MAX )
+            break;
+
+        if ( symbolsize )
+            *symbolsize = data->symtab[best].size;
+        if ( offset )
+            *offset = va - data->symtab[best].value;
+        if ( namebuf )
+            strlcpy(namebuf, data->name, KSYM_NAME_LEN);
+
+        n = data->symtab[best].name;
+        break;
+    }
+    rcu_read_unlock(&rcu_applied_lock);
+
+    return n;
+}
+
 static struct payload *find_payload(const char *name)
 {
     struct payload *data, *found = NULL;
@@ -375,6 +427,7 @@ static int prepare_payload(struct payload *payload,
     const struct xsplice_elf_sec *sec;
     unsigned int i;
     struct xsplice_patch_func *f;
+    struct virtual_region *region;
 
     sec = xsplice_elf_sec_by_name(elf, ELF_XSPLICE_FUNC);
     ASSERT(sec);
@@ -431,6 +484,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 = payload->text_addr;
+    region->end = payload->text_addr + payload->text_size;
+
     return 0;
 }
 
@@ -507,6 +567,7 @@ static int build_symbol_table(struct payload *payload,
         if ( is_payload_symbol(elf, elf->sym + i) )
         {
             symtab[nsyms].name = strtab + strtab_len;
+            symtab[nsyms].size = elf->sym[i].sym->st_size;
             symtab[nsyms].value = (void *)elf->sym[i].sym->st_value;
             symtab[nsyms].new_symbol = 0; /* May be overwritten below. */
             strtab_len += strlcpy(strtab + strtab_len, elf->sym[i].name,
@@ -797,6 +858,7 @@ static int apply_payload(struct payload *data)
      * The applied_list is iterated by the trap code.
      */
     list_add_tail_rcu(&data->applied_list, &applied_list);
+    register_virtual_region(&data->region);
 
     return 0;
 }
@@ -819,6 +881,7 @@ static int revert_payload(struct payload *data)
      * The applied_list is iterated by the trap code.
      */
     list_del_rcu(&data->applied_list);
+    unregister_virtual_region(&data->region);
 
     return 0;
 }
diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
index 1526752..bb8baee 100644
--- a/xen/include/xen/xsplice.h
+++ b/xen/include/xen/xsplice.h
@@ -22,6 +22,7 @@ struct xen_sysctl_xsplice_op;
 struct xsplice_symbol {
     const char *name;
     void *value;
+    unsigned int size;
     bool_t new_symbol;
 };
 
-- 
2.4.3


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel