Re: [PATCH v3 09/23] xsplice: Add support for bug frames. (v4)

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/02/16 18:05, Konrad Rzeszutek Wilk wrote:
> 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);
> +}

This would be better as a static inline in a header file, to avoid a
call into a separate translation unit.

> +
>  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 b854c0a..7f71ac6 100644
> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -42,7 +42,10 @@ 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. */
> -
> +    size_t core_size;                    /* Everything else - .data,.rodata, etc. */
> +    size_t core_text_size;               /* Only .text size. */

These two lines should be reversed, so the comments make sense.

> +    struct bug_frame *start_bug_frames[BUGFRAME_NR]; /* .bug.frame patching. */
> +    struct bug_frame *stop_bug_frames[BUGFRAME_NR];
>      char name[XEN_XSPLICE_NAME_SIZE + 1];/* Name of it. */
>  };
>  
> @@ -561,6 +564,7 @@ static int move_payload(struct payload *payload, struct xsplice_elf *elf)
>               (SHF_ALLOC|SHF_EXECINSTR) )
>              calc_section(&elf->sec[i], &size);
>      }
> +    payload->core_text_size = size;
>  
>      /* Compute rw data */
>      for ( i = 0; i < elf->hdr->e_shnum; i++ )
> @@ -579,6 +583,7 @@ static int move_payload(struct payload *payload, struct xsplice_elf *elf)
>               !(elf->sec[i].sec->sh_flags & SHF_WRITE) )
>              calc_section(&elf->sec[i], &size);
>      }
> +    payload->core_size = size;
>  
>      buf = alloc_payload(size);
>      if ( !buf ) {
> @@ -663,6 +668,24 @@ static int find_special_sections(struct payload *payload,
>              if ( f->pad[j] )
>                  return -EINVAL;
>      }
> +
> +    /* Optional sections. */
> +    for ( i = 0; i < BUGFRAME_NR; i++ )
> +    {
> +        char str[14];
> +
> +        snprintf(str, sizeof str, ".bug_frames.%d", 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) ) )
> +            return -EINVAL;

Too many spaces.  (not a common style nit!)

> +
> +        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);
> +    }
>      return 0;
>  }
>  
> @@ -961,6 +984,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;
> +
> +    /* No locking since this list is only ever changed during apply or revert
> +     * context. */
> +    list_for_each_entry ( data, &applied_list, applied_list )
> +    {
> +        for (i = 0; i < BUGFRAME_NR; i++) {

braces on new lines.

> +            if (!data->start_bug_frames[i])
> +                continue;

Newline, and can you borrow some spaces from above.

> +            if ( !((void *)eip >= data->payload_address &&
> +                   (void *)eip < (data->payload_address + data->core_text_size)))
> +                continue;
> +
> +            for ( bug = data->start_bug_frames[i]; bug != data->stop_bug_frames[i]; ++bug ) {
> +                if ( bug_loc(bug) == eip )
> +                {
> +                    *id = i;
> +                    return bug;
> +                }
> +            }
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +bool_t is_module(const void *ptr)

I would recommend naming this "is_patch", to avoid the suggestion that
Xen supports modules.

> +{
> +    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->payload_address &&
> +             ptr < (data->payload_address + data->core_size))
> +            return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +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->payload_address &&
> +             (void *)addr < (data->payload_address + data->core_text_size))
> +            return 1;
> +    }
> +
> +    return 0;
> +}
> +
>  static int __init xsplice_init(void)
>  {
>      register_keyhandler('x', xsplice_printall, "print xsplicing info", 1);
> diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
> index d6db1c2..c257b3a 100644
> --- a/xen/include/xen/xsplice.h
> +++ b/xen/include/xen/xsplice.h
> @@ -26,6 +26,9 @@ struct xsplice_patch_func {
>  #ifdef CONFIG_XSPLICE
>  int xsplice_control(struct xen_sysctl_xsplice_op *);
>  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(struct xsplice_elf *elf, uint8_t *data);
> @@ -44,5 +47,17 @@ static inline int xsplice_control(struct xen_sysctl_xsplice_op *op)
>      return -ENOSYS;
>  }
>  static inline void do_xsplice(void) { };
> +static inline struct bug_frame *xsplice_find_bug(const char *eip, int *id)
> +{
> +	return NULL;
> +}
> +static inline bool_t is_module(const void *addr)
> +{
> +	return 0;
> +}
> +static inline bool_t is_active_module_text(unsigned long addr)
> +{
> +	return 0;
> +}

There is a neater way of doing this, which doesn't involve having "if (
regular ) else if ( xsplice )" logic chains through the code.

Given a

struct virtual_region
{
    struct list_head list;
    unsigned long start, size;

    struct bug_frame *foo;
    struct exception_table_entry *bar;
};

The init code can construct one for the base hypervisor, and xsplice can
add or remove entries from the list.  Then, the trap routines search the
virtual region list for [start, size) and follow the appropriate pointers.

~Andrew