Re: [PATCH v1 09/11] xsplice: Add support for bug frames

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Nov 03, 2015 at 06:16:06PM +0000, Ross Lagerwall wrote:
> Add support for handling bug frames contained with xsplice modules. If a
> trap occurs search either the kernel bug table or an applied module's

payload.
> bug table depending on the instruction pointer.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  xen/arch/x86/traps.c      |  30 ++++++++-----
>  xen/common/symbols.c      |   7 +++
>  xen/common/xsplice.c      | 107 +++++++++++++++++++++++++++++++++++++++++-----
>  xen/include/xen/kernel.h  |   1 +
>  xen/include/xen/xsplice.h |   4 ++
>  5 files changed, 129 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index b32f696..cd51cfd 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -48,6 +48,7 @@
>  #include <xen/kexec.h>
>  #include <xen/trace.h>
>  #include <xen/paging.h>
> +#include <xen/xsplice.h>
>  #include <xen/watchdog.h>
>  #include <asm/system.h>
>  #include <asm/io.h>
> @@ -1076,20 +1077,29 @@ void do_invalid_op(struct cpu_user_regs *regs)
>          return;
>      }
>  
> -    if ( !is_active_kernel_text(regs->eip) ||
> +    if ( !is_active_text(regs->eip) ||
>           __copy_from_user(bug_insn, eip, sizeof(bug_insn)) ||
>           memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) )
>          goto die;
>  
> -    for ( bug = __start_bug_frames, id = 0; stop_frames[id]; ++bug )
> +    if ( likely(is_active_kernel_text(regs->eip)) )
>      {
> -        while ( unlikely(bug == stop_frames[id]) )
> -            ++id;
> -        if ( bug_loc(bug) == eip )
> -            break;
> +        for ( bug = __start_bug_frames, id = 0; stop_frames[id]; ++bug )
> +        {
> +            while ( unlikely(bug == stop_frames[id]) )
> +                ++id;
> +            if ( bug_loc(bug) == eip )
> +                break;
> +        }
> +        if ( !stop_frames[id] )
> +            goto die;
> +    }
> +    else
> +    {
> +        bug = xsplice_find_bug(eip, &id);
> +        if ( !bug )
> +            goto die;
>      }
> -    if ( !stop_frames[id] )
> -        goto die;
>  
>      eip += sizeof(bug_insn);
>      if ( id == BUGFRAME_run_fn )
> @@ -1103,7 +1113,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_module(filename) )
>          goto die;
>      fixup = strlen(filename);
>      if ( fixup > 50 )
> @@ -1130,7 +1140,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_module(predicate) )

is_payload ?
>              predicate = "<unknown>";
>  
>          printk("Assertion '%s' failed at %s%s:%d\n",
> diff --git a/xen/common/symbols.c b/xen/common/symbols.c
> index a59c59d..bf5623f 100644
> --- a/xen/common/symbols.c
> +++ b/xen/common/symbols.c
> @@ -17,6 +17,7 @@
>  #include <xen/lib.h>
>  #include <xen/string.h>
>  #include <xen/spinlock.h>
> +#include <xen/xsplice.h>
>  #include <public/platform.h>
>  #include <xen/guest_access.h>
>  
> @@ -101,6 +102,12 @@ bool_t is_active_kernel_text(unsigned long addr)
>              (system_state < SYS_STATE_active && is_kernel_inittext(addr)));
>  }
>  
> +bool_t is_active_text(unsigned long addr)
> +{
> +    return is_active_kernel_text(addr) ||
> +           is_active_module_text(addr);

Ditto?
> +}
> +
>  const char *symbols_lookup(unsigned long addr,
>                             unsigned long *symbolsize,
>                             unsigned long *offset,
> diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
> index 4476be5..982954b 100644
> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -40,6 +40,11 @@ struct payload {
>      int nfuncs;
>      void *module_address;
>      size_t module_pages;
> +    size_t core_size;
> +    size_t core_text_size;
> +
> +    struct bug_frame *start_bug_frames[4];
> +    struct bug_frame *stop_bug_frames[4];

You need a #define for the 4 value.
>  
>      char  id[XEN_XSPLICE_NAME_SIZE + 1];          /* Name of it. */
>  };
> @@ -525,26 +530,27 @@ static void free_module(struct payload *payload)
>      payload->module_pages = 0;
>  }
>  
> -static void alloc_section(struct xsplice_elf_sec *sec, size_t *core_size)
> +static void alloc_section(struct xsplice_elf_sec *sec, size_t *size)
>  {
> -    size_t align_size = ROUNDUP(*core_size, sec->sec->sh_addralign);
> +    size_t align_size = ROUNDUP(*size, sec->sec->sh_addralign);
>      sec->sec->sh_entsize = align_size;
> -    *core_size = sec->sec->sh_size + align_size;
> +    *size = sec->sec->sh_size + align_size;
>  }

That looks to be unrelated to this patch. Perhaps squash it in earlier?

>  
>  static int move_module(struct payload *payload, struct xsplice_elf *elf)
>  {
>      uint8_t *buf;
>      int i;
> -    size_t core_size = 0;
> +    size_t size = 0;
>  
>      /* Allocate text regions */
>      for ( i = 0; i < elf->hdr->e_shnum; i++ )
>      {
>          if ( (elf->sec[i].sec->sh_flags & (SHF_ALLOC|SHF_EXECINSTR)) ==
>               (SHF_ALLOC|SHF_EXECINSTR) )
> -            alloc_section(&elf->sec[i], &core_size);
> +            alloc_section(&elf->sec[i], &size);
>      }
> +    payload->core_text_size = size;
>  
>      /* Allocate rw data */
>      for ( i = 0; i < elf->hdr->e_shnum; i++ )
> @@ -552,7 +558,7 @@ static int move_module(struct payload *payload, struct xsplice_elf *elf)
>          if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
>               !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
>               (elf->sec[i].sec->sh_flags & SHF_WRITE) )
> -            alloc_section(&elf->sec[i], &core_size);
> +            alloc_section(&elf->sec[i], &size);
>      }
>  
>      /* Allocate ro data */
> @@ -561,15 +567,16 @@ static int move_module(struct payload *payload, struct xsplice_elf *elf)
>          if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
>               !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
>               !(elf->sec[i].sec->sh_flags & SHF_WRITE) )
> -            alloc_section(&elf->sec[i], &core_size);
> +            alloc_section(&elf->sec[i], &size);
>      }
> +    payload->core_size = size;
>  
> -    buf = alloc_module(core_size);
> +    buf = alloc_module(size);
>      if ( !buf ) {
>          printk(XENLOG_ERR "Could not allocate memory for module\n");
>          return -ENOMEM;
>      }
> -    memset(buf, 0, core_size);
> +    memset(buf, 0, size);
>  
>      for ( i = 0; i < elf->hdr->e_shnum; i++ )
>      {
> @@ -584,7 +591,7 @@ static int move_module(struct payload *payload, struct xsplice_elf *elf)
>      }
>  
>      payload->module_address = buf;
> -    payload->module_pages = PFN_UP(core_size);
> +    payload->module_pages = PFN_UP(size);
>  
>      return 0;
>  }
> @@ -659,6 +666,7 @@ static int find_special_sections(struct payload *payload,
>                                   struct xsplice_elf *elf)
>  {
>      struct xsplice_elf_sec *sec;
> +    int i;

unsigned int 
>  
>      sec = xsplice_elf_sec_by_name(elf, ".xsplice.funcs");
>      if ( !sec )
> @@ -670,6 +678,19 @@ static int find_special_sections(struct payload *payload,
>      payload->funcs = (struct xsplice_patch_func *)sec->load_addr;
>      payload->nfuncs = sec->sec->sh_size / (sizeof *payload->funcs);
>  
> +    for ( i = 0; i < 4; i++ )

use the #define.
> +    {
> +        char str[14];

And this needs a #define as well.
> +
> +        snprintf(str, sizeof str, ".bug_frames.%d", i);
> +        sec = xsplice_elf_sec_by_name(elf, str);
> +        if ( !sec )
> +            continue;
> +
> +        payload->start_bug_frames[i] = (struct bug_frame *)sec->load_addr;
> +        payload->stop_bug_frames[i] = (struct bug_frame *)(sec->load_addr + sec->sec->sh_size);

You probably should double check that the 'sh_size' is not greater than
4. 
> +    }
> +
>      return 0;
>  }
>  
> @@ -912,6 +933,72 @@ void do_xsplice(void)
>      }
>  }
>  
> +
> +/*
> + * Functions for handling special sections.
> + */
> +struct bug_frame *xsplice_find_bug(const char *eip, int *id)
> +{
> +    struct payload *data;
> +    struct bug_frame *bug;
> +    int i;

unsigned int;
> +
> +    /* No locking since this list is only ever changed during apply or revert
> +     * context. */

Please use hte different style of comments:

/*
 * blah blah blah
 */

> +    list_for_each_entry ( data, &applied_list, applied_list )
> +    {
> +        for (i = 0; i < 4; i++) {
> +            if (!data->start_bug_frames[i])
> +                continue;
> +            if ( !((void *)eip >= data->module_address &&
> +                   (void *)eip < (data->module_address + data->core_text_size)))
> +                continue;
> +
> +            for ( bug = data->start_bug_frames[i]; bug != data->stop_bug_frames[i]; ++bug ) {

Could we ever have the situation of where there is a NULL structure
within start_bug_frames and stop_bug_frames?

[Say the file is corrupted]
Perhaps we should double-check that in find_special_sections?
> +                if ( bug_loc(bug) == eip )
> +                {
> +                    *id = i;
> +                    return bug;
> +                }
> +            }
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +bool_t is_module(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->module_address &&
> +             ptr < (data->module_address + data->core_size))
> +            return true;
> +    }
> +
> +    return false;
> +}
> +
> +bool_t is_active_module_text(unsigned long addr)
> +{
> +    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 ( (void *)addr >= data->module_address &&
> +             (void *)addr < (data->module_address + data->core_text_size))
> +            return true;
> +    }
> +
> +    return false;
> +}
> +

Those two look very very much the same. Could one of them call the
other?

Actually why not just have one? And do some casting?


>  static int __init xsplice_init(void)
>  {
>      register_keyhandler('x', xsplice_printall, "print xsplicing info", 1);
> diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
> index 548b64d..df57754 100644
> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -99,6 +99,7 @@ extern enum system_state {
>  } system_state;
>  
>  bool_t is_active_kernel_text(unsigned long addr);
> +bool_t is_active_text(unsigned long addr);
>  
>  #endif /* _LINUX_KERNEL_H */
>  
> diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
> index 507829c..772fa3a 100644
> --- a/xen/include/xen/xsplice.h
> +++ b/xen/include/xen/xsplice.h
> @@ -22,6 +22,10 @@ extern void xsplice_printall(unsigned char key);
>  
>  void do_xsplice(void);
>  
> +struct bug_frame * xsplice_find_bug(const char *eip, int *id);
> +bool_t is_module(const void *addr);
> +bool_t is_active_module_text(unsigned long addr);
> +
>  /* Arch hooks */
>  int xsplice_verify_elf(uint8_t *data, ssize_t len);
>  int xsplice_perform_rel(struct xsplice_elf *elf,
> -- 
> 2.4.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel