[v5,16/28] xsplice: Add support for bug frames.

Commit Message

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

From: Ross Lagerwall <[e-mail]>

Add support for handling bug frames contained with xsplice modules. If a
trap occurs search either the kernel bug table or an applied payload's
bug table depending on the instruction pointer.

We also include a test-case - which will test the function that was
patched to make sure it has the right value. And will only be triggered
if something has gone horribly wrong.

P.S.
If one really wants to test, insert an WARN_ON(1) at the end of
the revert_hook.

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

---
Cc: Keir Fraser <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Andrew Cooper <[e-mail]>

v2:- s/module/payload/
   - add build time check in case amount of bug frames expands.
   - add define for the number of bug-frames.
v3:
  - add missing BUGFRAME_NR, squash s/core_size/core/ in earlier patch.
  - Moved code around.
  - Changed per Andrew's recommendation.
  - Fixed style changes.
  - Made it compile under ARM (PRIu32,PRIu64)
v4: Use 'struct virtual_region'
  - Rip more of the is_active_text code.
  - Use one function for the ->skip
  - Include test-case
v5: Rip out the ->skip function.
---
 xen/arch/x86/test/xen_hello_world.c |  6 ++++++
 xen/arch/x86/traps.c                |  5 +++--
 xen/common/xsplice.c                | 40 +++++++++++++++++++++++++++++++++++++
 xen/include/xen/xsplice.h           |  5 +++++
 4 files changed, 54 insertions(+), 2 deletions(-)

Comments

Jan Beulich April 1, 2016, 4 p.m. UTC | #1

>>> On 24.03.16 at 21:00, <[e-mail]> wrote:
> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -120,6 +120,24 @@ static int verify_payload(const xen_sysctl_xsplice_upload_t *upload)
>      return 0;
>  }
>  
> +bool_t is_patch(const void *ptr)
> +{
> +    struct payload *data;
> +
> +    /*
> +     * No locking since this list is only ever changed during apply or revert
> +     * context.
> +     */
> +    list_for_each_entry ( data, &applied_list, applied_list )
> +    {
> +        if ( ptr >= data->ro_addr &&
> +             ptr < (data->ro_addr + data->ro_size) )
> +            return 1;
> +    }
> +
> +    return 0;
> +}

A function with this name can't look at just .rodata. Nor may you
assume in the first place that the file names you look for are
guaranteed to be in .rodata.

> @@ -533,6 +551,28 @@ static int prepare_payload(struct payload *payload,
>      region->start = (unsigned long)payload->text_addr;
>      region->end = (unsigned long)(payload->text_addr + payload->text_size);
>  
> +    /* Optional sections. */
> +    for ( i = 0; i < BUGFRAME_NR; i++ )
> +    {
> +        char str[14];
> +
> +        snprintf(str, sizeof str, ".bug_frames.%u", i);
> +        sec = xsplice_elf_sec_by_name(elf, str);
> +        if ( !sec )
> +            continue;
> +
> +        if ( !sec->sec->sh_size ||

I don't think there's anything wrong with this section being empty.

> +             (sec->sec->sh_size % sizeof (struct bug_frame)) )

Please use sizeof(*region->frame->bugs) here to avoid a possible
disconnect between the expression here and the actual type
needed.

> +        {
> +            dprintk(XENLOG_DEBUG, "%s%s: Wrong size of .bug_frames.%u!\n",
> +                    XSPLICE, elf->name, i);
> +            return -EINVAL;
> +        }
> +
> +        region->frame[i].bugs = (struct bug_frame *)sec->load_addr;

The more of these casts I see the more convinced I am that - if
they're needed - the type of load_addr was badly chosen.

> +        region->frame[i].n_bugs = sec->sec->sh_size / sizeof(struct bug_frame);

Same remark for the sizeof() as above.

Jan

Patch

diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c
index d2b3cc2..5364114 100644
--- a/xen/arch/x86/test/xen_hello_world.c
+++ b/xen/arch/x86/test/xen_hello_world.c
@@ -19,11 +19,17 @@  extern const char *xen_extra_version(void);
 void apply_hook(void)
 {
     printk(KERN_DEBUG "Hook executing.\n");
+    /* The hook is called  _after_ the patching. */
+    if ( strcmp(xen_extra_version(), "Hello World") )
+        BUG();
 }
 
 void revert_hook(void)
 {
     printk(KERN_DEBUG "Hook unloaded.\n");
+    /* The hook is called  _after_ the unpatching. */
+    if ( !strcmp(xen_extra_version(), "Hello World") )
+        BUG();
 }
 
 XSPLICE_LOAD_HOOK(apply_hook);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6c73198..e05656a 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -50,6 +50,7 @@ 
 #include <xen/paging.h>
 #include <xen/virtual_region.h>
 #include <xen/watchdog.h>
+#include <xen/xsplice.h>
 #include <asm/system.h>
 #include <asm/io.h>
 #include <asm/atomic.h>
@@ -1190,7 +1191,7 @@  void do_invalid_op(struct cpu_user_regs *regs)
 
     /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
     filename = bug_ptr(bug);
-    if ( !is_kernel(filename) )
+    if ( !is_kernel(filename) && !is_patch(filename) )
         goto die;
     fixup = strlen(filename);
     if ( fixup > 50 )
@@ -1217,7 +1218,7 @@  void do_invalid_op(struct cpu_user_regs *regs)
     case BUGFRAME_assert:
         /* ASSERT: decode the predicate string pointer. */
         predicate = bug_msg(bug);
-        if ( !is_kernel(predicate) )
+        if ( !is_kernel(predicate) && !is_patch(predicate) )
             predicate = "<unknown>";
 
         printk("Assertion '%s' failed at %s%s:%d\n",
diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index 03b32e4..7b92602 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -120,6 +120,24 @@  static int verify_payload(const xen_sysctl_xsplice_upload_t *upload)
     return 0;
 }
 
+bool_t is_patch(const void *ptr)
+{
+    struct payload *data;
+
+    /*
+     * No locking since this list is only ever changed during apply or revert
+     * context.
+     */
+    list_for_each_entry ( data, &applied_list, applied_list )
+    {
+        if ( ptr >= data->ro_addr &&
+             ptr < (data->ro_addr + data->ro_size) )
+            return 1;
+    }
+
+    return 0;
+}
+
 uint64_t xsplice_symbols_lookup_by_name(const char *symname)
 {
     struct payload *data;
@@ -533,6 +551,28 @@  static int prepare_payload(struct payload *payload,
     region->start = (unsigned long)payload->text_addr;
     region->end = (unsigned long)(payload->text_addr + payload->text_size);
 
+    /* Optional sections. */
+    for ( i = 0; i < BUGFRAME_NR; i++ )
+    {
+        char str[14];
+
+        snprintf(str, sizeof str, ".bug_frames.%u", i);
+        sec = xsplice_elf_sec_by_name(elf, str);
+        if ( !sec )
+            continue;
+
+        if ( !sec->sec->sh_size ||
+             (sec->sec->sh_size % sizeof (struct bug_frame)) )
+        {
+            dprintk(XENLOG_DEBUG, "%s%s: Wrong size of .bug_frames.%u!\n",
+                    XSPLICE, elf->name, i);
+            return -EINVAL;
+        }
+
+        region->frame[i].bugs = (struct bug_frame *)sec->load_addr;
+        region->frame[i].n_bugs = sec->sec->sh_size / sizeof(struct bug_frame);
+    }
+
     return 0;
 }
 
diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
index 8bc55f3..47ef1c2 100644
--- a/xen/include/xen/xsplice.h
+++ b/xen/include/xen/xsplice.h
@@ -42,6 +42,7 @@  struct xsplice_symbol {
 
 int xsplice_op(struct xen_sysctl_xsplice_op *);
 void check_for_xsplice_work(void);
+bool_t is_patch(const void *addr);
 uint64_t xsplice_symbols_lookup_by_name(const char *symname);
 
 /* Arch hooks. */
@@ -106,6 +107,10 @@  static inline int xsplice_op(struct xen_sysctl_xsplice_op *op)
     return -EOPNOTSUPP;
 }
 static inline void check_for_xsplice_work(void) { };
+static inline bool_t is_patch(const void *addr)
+{
+    return 0;
+}
 #endif /* CONFIG_XSPLICE */
 
 #endif /* __XEN_XSPLICE_H__ */