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

[Ross Lagerwall <ross.lagerwall@citrix.com>]

On 04/25/2016 04:35 PM, Konrad Rzeszutek Wilk wrote:
snip
> +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. */
>   };
> @@ -142,6 +145,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->pages * PAGE_SIZE) )

This calculation is wrong due to the use of void * and results in 
incorrect backtrace results.

You also need to have || rather than &&.

Additionally, I think it should use data->text_size rather than 
data->pages * PAGE_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;
> @@ -366,6 +418,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);
> @@ -422,6 +475,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;

This calculation is wrong due to the use of void *.

-- 
Ross Lagerwall

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