Re: [Xen-devel] [PATCH v5 13/28] xsplice, symbols: Implement symbol name resolution on address.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.03.16 at 21:00, <konrad.wilk@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -113,12 +113,14 @@ $(TARGET)-syms: prelink.o xen.lds 
> $(BASEDIR)/common/symbols-dummy.o
>       $(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
>           $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
>       $(NM) -pa --format=sysv $(@D)/.$(@F).0 \
> -             | $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).0.S
> +             | $(BASEDIR)/tools/symbols --all-symbols --sysv --sort \
> +             >$(@D)/.$(@F).0.S
>       $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0.o
>       $(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
>           $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1
>       $(NM) -pa --format=sysv $(@D)/.$(@F).1 \
> -             | $(BASEDIR)/tools/symbols --sysv --sort --warn-dup 
> >$(@D)/.$(@F).1.S
> +             | $(BASEDIR)/tools/symbols --all-symbols --sysv --sort 
> --warn-dup \
> +             >$(@D)/.$(@F).1.S

This addition should be dependent on CONFIG_XSPLICE, not the
least because I expect it to bloat the symbol table quite a bit. And
then - how come this is needed here, but not in the xen.efi rule?

> +uint64_t symbols_lookup_by_name(const char *symname)

I don't think such a function can reasonably return other than void *
or unsigned long.

> +{
> +    uint32_t symnum = 0;
> +    uint64_t addr = 0, outaddr = 0;
> +    int rc;
> +    char type;
> +    char name[KSYM_NAME_LEN + 1] = {0};

This and likely addr's initializer seem pointless. (And it's questionable
whether you really need both addr and outaddr.)

> +    do {
> +        rc = xensyms_read(&symnum, &type, &addr, name);
> +        if ( rc )
> +            break;
> +
> +        if ( !strcmp(name, symname) )
> +        {
> +            outaddr = addr;
> +            break;
> +        }
> +    } while ( name[0] != '\0' );
> +
> +    return outaddr;
> +}

Huh - a brute force linear lookup. We've got some 7,000 symbols
right now, and I think we can't expect that to go down.

> +uint64_t xsplice_symbols_lookup_by_name(const char *symname)
> +{
> +    struct payload *data;
> +    unsigned int i;
> +    uint64_t value = 0;
> +
> +    spin_lock_recursive(&payload_lock);
> +
> +    list_for_each_entry ( data, &payload_list, list )
> +    {
> +        for ( i = 0; i < data->nsyms; i++ )
> +        {
> +            if ( !data->symtab[i].new_symbol )
> +                continue;
> +
> +            if ( !strcmp(data->symtab[i].name, symname) )
> +            {
> +                value = data->symtab[i].value;
> +                goto out;
> +            }
> +        }
> +    }
> +
> +out:

Label indentation.

> @@ -397,8 +429,126 @@ static int prepare_payload(struct payload *payload,
>          for ( j = 0; j < 24; j++ )
>              if ( f->pad[j] )
>                  return -EINVAL;
> +
> +        /* Lookup function's old address if not already resolved. */
> +        if ( !f->old_addr )
> +        {
> +            f->old_addr = symbols_lookup_by_name(f->name);
> +            if ( !f->old_addr )
> +            {
> +                f->old_addr = xsplice_symbols_lookup_by_name(f->name);

This returns with the lock not held - can you really rely on the symbol
staying around?

> +                if ( !f->old_addr )
> +                {
> +                    printk(XENLOG_ERR "%s%s: Could not resolve old address 
> of %s\n",
> +                           XSPLICE, elf->name, f->name);

Any log messages not rate limited by default need careful
consideration. I don't think this is one which needs to be
XENLOG_ERR, even if the condition is an error one.

> +static bool_t is_core_symbol(const struct xsplice_elf *elf,
> +                             const struct xsplice_elf_sym *sym)

What does "core" here mean?

> +{
> +    if ( sym->sym->st_shndx == SHN_UNDEF ||
> +         sym->sym->st_shndx >= elf->hdr->e_shnum )
> +        return 0;
> +
> +    return !!( (elf->sec[sym->sym->st_shndx].sec->sh_flags & SHF_ALLOC) &&
> +               (ELF64_ST_TYPE(sym->sym->st_info) == STT_OBJECT ||
> +                ELF64_ST_TYPE(sym->sym->st_info) == STT_FUNC) );
> +}

How does the symbol type matter? Don't you rather mean to
ignore static symbols? Or maybe even just section ones?

Also - stray blanks and !!.

> +static int build_symbol_table(struct payload *payload,
> +                              const struct xsplice_elf *elf)
> +{
> +    unsigned int i, j, nsyms = 0;
> +    size_t strtab_len = 0;
> +    struct xsplice_symbol *symtab;
> +    char *strtab;
> +
> +    ASSERT(payload->nfuncs);
> +
> +    /* Recall that section @0 is always NULL. */
> +    for ( i = 1; i < elf->nsym; i++ )
> +    {
> +        if ( is_core_symbol(elf, elf->sym + i) )
> +        {
> +            nsyms++;
> +            strtab_len += strlen(elf->sym[i].name) + 1;
> +        }
> +    }
> +
> +    symtab = xmalloc_array(struct xsplice_symbol, nsyms);
> +    if ( !symtab )
> +        return -ENOMEM;
> +
> +    strtab = xmalloc_bytes(strtab_len);

xmalloc_array(char, strtab_len);

> +    if ( !strtab )
> +    {
> +        xfree(symtab);
> +        return -ENOMEM;
> +    }

To limit the number of return paths, could I talk you into doing both
allocations and only then checking both return values?

> +    nsyms = 0;
> +    strtab_len = 0;
> +    for ( i = 1; i < elf->nsym; i++ )
> +    {
> +        if ( is_core_symbol(elf, elf->sym + i) )
> +        {
> +            symtab[nsyms].name = strtab + strtab_len;
> +            symtab[nsyms].size = elf->sym[i].sym->st_size;
> +            symtab[nsyms].value = elf->sym[i].sym->st_value;
> +            symtab[nsyms].new_symbol = 0; /* To be checked below. */
> +            strtab_len += strlcpy(strtab + strtab_len, elf->sym[i].name,
> +                                  KSYM_NAME_LEN) + 1;
> +            nsyms++;
> +        }
> +    }
> +
> +    for ( i = 0; i < nsyms; i++ )
> +    {
> +        bool_t found = 0;
> +
> +        for ( j = 0; j < payload->nfuncs; j++ )
> +        {
> +            if ( symtab[i].value == payload->funcs[j].new_addr )
> +            {
> +                found = 1;
> +                break;
> +            }
> +        }
> +
> +        if ( !found )
> +        {
> +            if ( xsplice_symbols_lookup_by_name(symtab[i].name) )
> +            {
> +                printk(XENLOG_ERR "%s%s: duplicate new symbol: %s\n",
> +                       XSPLICE, elf->name, symtab[i].name);
> +                xfree(symtab);
> +                xfree(strtab);
> +                return -EEXIST;
> +            }
> +            symtab[i].new_symbol = 1;
> +            dprintk(XENLOG_DEBUG, "%s%s: new symbol %s\n",
> +                    XSPLICE, elf->name, symtab[i].name);
> +        }
> +        else
> +        {
> +            dprintk(XENLOG_DEBUG, "%s%s: overriding symbol %s\n",
> +                    XSPLICE, elf->name, symtab[i].name);

Since you don't do anything here - how is this an override of some
sort?

> @@ -235,15 +236,27 @@ int xsplice_elf_resolve_symbols(struct xsplice_elf  
> *elf)
>                  break;
>  
>              case SHN_UNDEF:
> -                printk(XENLOG_ERR "%s%s: Unknown symbol: %s\n",
> -                       XSPLICE, elf->name, elf->sym[i].name);
> -                return -ENOENT;
> +                elf->sym[i].sym->st_value = 
> symbols_lookup_by_name(elf->sym[i].name);
> +                if ( !elf->sym[i].sym->st_value )
> +                {
> +                    elf->sym[i].sym->st_value =
> +                        xsplice_symbols_lookup_by_name(elf->sym[i].name);
> +                    if ( !elf->sym[i].sym->st_value )
> +                    {
> +                        printk(XENLOG_ERR "%s%s: Unknown symbol: %s\n",
> +                               XSPLICE, elf->name, elf->sym[i].name);
> +                        return -ENOENT;
> +                    }
> +                }
> +                dprintk(XENLOG_DEBUG, "%s%s: Undefined symbol resolved: %s 
> => 0x%"PRIx64"\n",
> +                       XSPLICE, elf->name, elf->sym[i].name,
> +                       (uint64_t)elf->sym[i].sym->st_value);
>                  break;
>  
>              case SHN_ABS:
>                  dprintk(XENLOG_DEBUG, "%s%s: Absolute symbol: %s => 
> 0x%"PRIx64"\n",
>                        XSPLICE, elf->name, elf->sym[i].name,
> -                      elf->sym[i].sym->st_value);
> +                      (uint64_t)elf->sym[i].sym->st_value);

This change does not seem to belong here.

> --- a/xen/include/xen/xsplice.h
> +++ b/xen/include/xen/xsplice.h
> @@ -33,8 +33,16 @@ struct xsplice_patch_func {
>  /* Convenience define for printk. */
>  #define XSPLICE "xsplice: "
>  
> +struct xsplice_symbol {
> +    const char *name;
> +    uint64_t value;
> +    ssize_t size;

While mentioned in the revision log - why signed?

Jan

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