[v5,17/28] xsplice: Add support for exception tables.

Commit Message

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

From: Ross Lagerwall <[e-mail]>

Add support for exception tables contained within xSplice payloads. If an
exception occurs search either the main exception table or a particular
active payload's exception table depending on the instruction pointer.

Also we add an test-case to make sure we have an exception that
is handled.

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]>

v3:
 - s/module/payload/
 - sanity checks.
 - Move code around.
 - s/module/payload/
v4: Use 'struct virtual_region'
v5:
  - Expand test-case.
---
 xen/arch/x86/extable.c              | 30 +++++++++++++++++-------------
 xen/arch/x86/test/xen_hello_world.c | 11 +++++++++++
 xen/common/xsplice.c                | 19 +++++++++++++++++++
 xen/include/asm-x86/uaccess.h       |  5 +++++
 4 files changed, 52 insertions(+), 13 deletions(-)

Comments

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

>>> On 24.03.16 at 21:00, <[e-mail]> wrote:
> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -573,6 +573,25 @@ static int prepare_payload(struct payload *payload,
>          region->frame[i].n_bugs = sec->sec->sh_size / sizeof(struct bug_frame);
>      }
>  
> +#ifdef CONFIG_X86
> +    sec = xsplice_elf_sec_by_name(elf, ".ex_table");
> +    if ( sec )
> +    {
> +        if ( !sec->sec->sh_size ||
> +             (sec->sec->sh_size % sizeof (struct exception_table_entry)) )
> +        {
> +            dprintk(XENLOG_DEBUG, "%s%s: Wrong size of .ex_table (exp:%lu vs %lu)!\n",
> +                    XSPLICE, elf->name, sizeof (struct exception_table_entry),
> +                    sec->sec->sh_size);
> +            return -EINVAL;
> +        }
> +
> +        region->ex = (struct exception_table_entry *)sec->load_addr;
> +        region->ex_end = (struct exception_table_entry *)(sec->load_addr + sec->sec->sh_size);
> +
> +        sort_exception_table(region->ex, region->ex_end);
> +    }
> +#endif

Nothing here is really x86-specific, so the earlier comment on the
conditionals better going away applies here too.

> --- a/xen/include/asm-x86/uaccess.h
> +++ b/xen/include/asm-x86/uaccess.h
> @@ -276,6 +276,11 @@ extern struct exception_table_entry 
> __start___pre_ex_table[];
>  extern struct exception_table_entry __stop___pre_ex_table[];
>  
>  extern unsigned long search_exception_table(unsigned long);
> +extern unsigned long search_one_extable(const struct exception_table_entry *first,
> +                                        const struct exception_table_entry *last,
> +                                        unsigned long value);

I can't seem to find a use of the outside its defining file. Why is
this being made global?

Jan

Konrad Rzeszutek Wilk April 6, 2016, 2:41 p.m. UTC | #2

On Fri, Apr 01, 2016 at 10:06:54AM -0600, Jan Beulich wrote:
> >>> On 24.03.16 at 21:00, <[e-mail]> wrote:
> > --- a/xen/common/xsplice.c
> > +++ b/xen/common/xsplice.c
> > @@ -573,6 +573,25 @@ static int prepare_payload(struct payload *payload,
> >          region->frame[i].n_bugs = sec->sec->sh_size / sizeof(struct bug_frame);
> >      }
> >  
> > +#ifdef CONFIG_X86
> > +    sec = xsplice_elf_sec_by_name(elf, ".ex_table");
> > +    if ( sec )
> > +    {
> > +        if ( !sec->sec->sh_size ||
> > +             (sec->sec->sh_size % sizeof (struct exception_table_entry)) )
> > +        {
> > +            dprintk(XENLOG_DEBUG, "%s%s: Wrong size of .ex_table (exp:%lu vs %lu)!\n",
> > +                    XSPLICE, elf->name, sizeof (struct exception_table_entry),
> > +                    sec->sec->sh_size);
> > +            return -EINVAL;
> > +        }
> > +
> > +        region->ex = (struct exception_table_entry *)sec->load_addr;
> > +        region->ex_end = (struct exception_table_entry *)(sec->load_addr + sec->sec->sh_size);
> > +
> > +        sort_exception_table(region->ex, region->ex_end);
> > +    }
> > +#endif
> 
> Nothing here is really x86-specific, so the earlier comment on the
> conditionals better going away applies here too.

But there is no sort_exception_table on ARM, nor would the sizeof work
on ARM? Or are you saying I should add an .. empty function and
structure for this?

> 
> > --- a/xen/include/asm-x86/uaccess.h
> > +++ b/xen/include/asm-x86/uaccess.h
> > @@ -276,6 +276,11 @@ extern struct exception_table_entry 
> > __start___pre_ex_table[];
> >  extern struct exception_table_entry __stop___pre_ex_table[];
> >  
> >  extern unsigned long search_exception_table(unsigned long);
> > +extern unsigned long search_one_extable(const struct exception_table_entry *first,
> > +                                        const struct exception_table_entry *last,
> > +                                        unsigned long value);
> 
> I can't seem to find a use of the outside its defining file. Why is
> this being made global?
> 
> Jan
>

Andrew Cooper April 6, 2016, 3:32 p.m. UTC | #3

On 06/04/16 15:41, Konrad Rzeszutek Wilk wrote:
> On Fri, Apr 01, 2016 at 10:06:54AM -0600, Jan Beulich wrote:
>>>>> On 24.03.16 at 21:00, <[e-mail]> wrote:
>>> --- a/xen/common/xsplice.c
>>> +++ b/xen/common/xsplice.c
>>> @@ -573,6 +573,25 @@ static int prepare_payload(struct payload *payload,
>>>          region->frame[i].n_bugs = sec->sec->sh_size / sizeof(struct bug_frame);
>>>      }
>>>  
>>> +#ifdef CONFIG_X86
>>> +    sec = xsplice_elf_sec_by_name(elf, ".ex_table");
>>> +    if ( sec )
>>> +    {
>>> +        if ( !sec->sec->sh_size ||
>>> +             (sec->sec->sh_size % sizeof (struct exception_table_entry)) )
>>> +        {
>>> +            dprintk(XENLOG_DEBUG, "%s%s: Wrong size of .ex_table (exp:%lu vs %lu)!\n",
>>> +                    XSPLICE, elf->name, sizeof (struct exception_table_entry),
>>> +                    sec->sec->sh_size);
>>> +            return -EINVAL;
>>> +        }
>>> +
>>> +        region->ex = (struct exception_table_entry *)sec->load_addr;
>>> +        region->ex_end = (struct exception_table_entry *)(sec->load_addr + sec->sec->sh_size);
>>> +
>>> +        sort_exception_table(region->ex, region->ex_end);
>>> +    }
>>> +#endif
>> Nothing here is really x86-specific, so the earlier comment on the
>> conditionals better going away applies here too.
> But there is no sort_exception_table on ARM, nor would the sizeof work
> on ARM? Or are you saying I should add an .. empty function and
> structure for this?

Given the lack of "struct exception_table_entry" entirely on ARM, would
recommend keeping the #ifdefs, as being the far cleaner option.

Longterm, ARM should gain exception table handing.

~Andrew

Jan Beulich April 7, 2016, 1:21 a.m. UTC | #4

>>> Konrad Rzeszutek Wilk <[e-mail]> 04/06/16 4:42 PM >>>
>On Fri, Apr 01, 2016 at 10:06:54AM -0600, Jan Beulich wrote:
>> >>> On 24.03.16 at 21:00, <[e-mail]> wrote:
>> > --- a/xen/common/xsplice.c
>> > +++ b/xen/common/xsplice.c
>> > @@ -573,6 +573,25 @@ static int prepare_payload(struct payload *payload,
>> >          region->frame[i].n_bugs = sec->sec->sh_size / sizeof(struct bug_frame);
>> >      }
>> >  
>> > +#ifdef CONFIG_X86
>> > +    sec = xsplice_elf_sec_by_name(elf, ".ex_table");
>> > +    if ( sec )
>> > +    {
>> > +        if ( !sec->sec->sh_size ||
>> > +             (sec->sec->sh_size % sizeof (struct exception_table_entry)) )
>> > +        {
>> > +            dprintk(XENLOG_DEBUG, "%s%s: Wrong size of .ex_table (exp:%lu vs %lu)!\n",
>> > +                    XSPLICE, elf->name, sizeof (struct exception_table_entry),
>> > +                    sec->sec->sh_size);
>> > +            return -EINVAL;
>> > +        }
>> > +
>> > +        region->ex = (struct exception_table_entry *)sec->load_addr;
>> > +        region->ex_end = (struct exception_table_entry *)(sec->load_addr + sec->sec->sh_size);
>> > +
>> > +        sort_exception_table(region->ex, region->ex_end);
>> > +    }
>> > +#endif
>> 
>> Nothing here is really x86-specific, so the earlier comment on the
>> conditionals better going away applies here too.
>
>But there is no sort_exception_table on ARM, nor would the sizeof work
>on ARM? Or are you saying I should add an .. empty function and
>structure for this?

The lack of sort_exception_table() would be easy to deal with, but I indeed
overlooked the sizeof(). So I agree with you and Andrew that the conditionals
should stay, but I'd like them to be inverted (i.e. test for !ARM).

Jan

Patch

diff --git a/xen/arch/x86/extable.c b/xen/arch/x86/extable.c
index c6c367a..a7f0144 100644
--- a/xen/arch/x86/extable.c
+++ b/xen/arch/x86/extable.c
@@ -20,7 +20,7 @@  static inline unsigned long ex_cont(const struct exception_table_entry *x)
 	return EX_FIELD(x, cont);
 }
 
-static int __init cmp_ex(const void *a, const void *b)
+static int cmp_ex(const void *a, const void *b)
 {
 	const struct exception_table_entry *l = a, *r = b;
 	unsigned long lip = ex_addr(l);
@@ -35,7 +35,7 @@  static int __init cmp_ex(const void *a, const void *b)
 }
 
 #ifndef swap_ex
-static void __init swap_ex(void *a, void *b, int size)
+static void swap_ex(void *a, void *b, int size)
 {
 	struct exception_table_entry *l = a, *r = b, tmp;
 	long delta = b - a;
@@ -48,19 +48,23 @@  static void __init swap_ex(void *a, void *b, int size)
 }
 #endif
 
-void __init sort_exception_tables(void)
+void sort_exception_table(struct exception_table_entry *start,
+                          struct exception_table_entry *stop)
 {
-    sort(__start___ex_table, __stop___ex_table - __start___ex_table,
-         sizeof(struct exception_table_entry), cmp_ex, swap_ex);
-    sort(__start___pre_ex_table,
-         __stop___pre_ex_table - __start___pre_ex_table,
+    sort(start, stop - start,
          sizeof(struct exception_table_entry), cmp_ex, swap_ex);
 }
 
-static inline unsigned long
-search_one_table(const struct exception_table_entry *first,
-                 const struct exception_table_entry *last,
-                 unsigned long value)
+void __init sort_exception_tables(void)
+{
+    sort_exception_table(__start___ex_table, __stop___ex_table);
+    sort_exception_table(__start___pre_ex_table, __stop___pre_ex_table);
+}
+
+unsigned long
+search_one_extable(const struct exception_table_entry *first,
+                   const struct exception_table_entry *last,
+                   unsigned long value)
 {
     const struct exception_table_entry *mid;
     long diff;
@@ -85,7 +89,7 @@  search_exception_table(unsigned long addr)
     struct virtual_region *region = search_for_text(addr);
 
     if ( region && region->ex )
-        return search_one_table(region->ex, region->ex_end-1, addr);
+        return search_one_extable(region->ex, region->ex_end-1, addr);
 
     return 0;
 }
@@ -94,7 +98,7 @@  unsigned long
 search_pre_exception_table(struct cpu_user_regs *regs)
 {
     unsigned long addr = (unsigned long)regs->eip;
-    unsigned long fixup = search_one_table(
+    unsigned long fixup = search_one_extable(
         __start___pre_ex_table, __stop___pre_ex_table-1, addr);
     if ( fixup )
     {
diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c
index 5364114..0f26b06 100644
--- a/xen/arch/x86/test/xen_hello_world.c
+++ b/xen/arch/x86/test/xen_hello_world.c
@@ -12,6 +12,7 @@ 
 
 static char xen_hello_world_name[] = "xen_hello_world";
 extern const char *xen_hello_world(void);
+static unsigned long *non_canonical_addr = (unsigned long *)(1UL<<48);
 
 /* External symbol. */
 extern const char *xen_extra_version(void);
@@ -26,10 +27,20 @@  void apply_hook(void)
 
 void revert_hook(void)
 {
+    unsigned long tmp = 0xdeadbeef;
+    int rc;
+
     printk(KERN_DEBUG "Hook unloaded.\n");
     /* The hook is called  _after_ the unpatching. */
     if ( !strcmp(xen_extra_version(), "Hello World") )
         BUG();
+    /*
+     * But before unregistering the virtual region. Which means any
+     * BUG, or WARN_ON will contain symbol name. And also exceptions
+     * will be caught and processed properly.
+     */
+    rc = __get_user(tmp, non_canonical_addr);
+    BUG_ON(rc != -EFAULT);
 }
 
 XSPLICE_LOAD_HOOK(apply_hook);
diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index 7b92602..4548b8b 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -573,6 +573,25 @@  static int prepare_payload(struct payload *payload,
         region->frame[i].n_bugs = sec->sec->sh_size / sizeof(struct bug_frame);
     }
 
+#ifdef CONFIG_X86
+    sec = xsplice_elf_sec_by_name(elf, ".ex_table");
+    if ( sec )
+    {
+        if ( !sec->sec->sh_size ||
+             (sec->sec->sh_size % sizeof (struct exception_table_entry)) )
+        {
+            dprintk(XENLOG_DEBUG, "%s%s: Wrong size of .ex_table (exp:%lu vs %lu)!\n",
+                    XSPLICE, elf->name, sizeof (struct exception_table_entry),
+                    sec->sec->sh_size);
+            return -EINVAL;
+        }
+
+        region->ex = (struct exception_table_entry *)sec->load_addr;
+        region->ex_end = (struct exception_table_entry *)(sec->load_addr + sec->sec->sh_size);
+
+        sort_exception_table(region->ex, region->ex_end);
+    }
+#endif
     return 0;
 }
 
diff --git a/xen/include/asm-x86/uaccess.h b/xen/include/asm-x86/uaccess.h
index 947470d..9e67bf0 100644
--- a/xen/include/asm-x86/uaccess.h
+++ b/xen/include/asm-x86/uaccess.h
@@ -276,6 +276,11 @@  extern struct exception_table_entry __start___pre_ex_table[];
 extern struct exception_table_entry __stop___pre_ex_table[];
 
 extern unsigned long search_exception_table(unsigned long);
+extern unsigned long search_one_extable(const struct exception_table_entry *first,
+                                        const struct exception_table_entry *last,
+                                        unsigned long value);
 extern void sort_exception_tables(void);
+extern void sort_exception_table(struct exception_table_entry *start,
+                                 struct exception_table_entry *stop);
 
 #endif /* __X86_UACCESS_H__ */