Re: [Xen-devel] [PATCH v8.1 14/27] xsplice, symbols: Implement symbol name resolution on address.

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/14/16 12:02 AM >>>
>--- a/xen/arch/x86/test/xen_hello_world.c
>+++ b/xen/arch/x86/test/xen_hello_world.c
>@@ -10,11 +10,14 @@
 >static char hello_world_patch_this_fnc[] = "xen_extra_version";
 >extern const char *xen_hello_world(void);
 >
>+/* External symbol. */
>+extern const char *xen_extra_version(void);

To give a good example, I think this would better include the respective
header.

>struct xsplice_patch_func __section(".xsplice.funcs") xsplice_xen_hello_world 
>= {
     >.version = XSPLICE_PAYLOAD_VERSION,
     >.name = hello_world_patch_this_fnc,
     >.new_addr = (unsigned long)(xen_hello_world),
>-    .old_addr = OLD_CODE,
>+    .old_addr = (unsigned long)(xen_extra_version),
     
Pointless parentheses (also btw on the new_addr initializer, which would be
nice to get fixed in the earlier patch).

>+unsigned long symbols_lookup_by_name(const char *symname)
>+{
>+    char name[KSYM_NAME_LEN + 1] = { 0 };

I continue to think that this initializer is pointless. And if this or a 
variant filling
just the first byte was needed, then please be consistent with the rest of the
function and either use '\0' here or plain 0 in the other two relevant places.

>+    uint32_t symnum = 0;
>+    char type;
>+    uint64_t addr = 0;

Same here.

>+    int rc;
>+
>+    if ( symname == '\0' )

DYM *symname?

>@@ -51,6 +52,9 @@ struct payload {
     >struct list_head applied_list;       /* Linked to 'applied_list'. */
     >struct xsplice_patch_func_internal *funcs;    /* The array of functions 
to patch. */
     >unsigned int nfuncs;                 /* Nr of functions to patch. */
>+    struct xsplice_symbol *symtab;       /* All symbols. */
>+    char *strtab;                        /* Pointer to .strtab. */

At least the latter of the two I'm convinced can be const.

>+unsigned long xsplice_symbols_lookup_by_name(const char *symname)
>+{
>+    struct payload *data;

const

>+    ASSERT(spin_is_locked(&payload_lock));
>+    list_for_each_entry ( data, &payload_list, list )
>+    {
>+        unsigned int i;
>+
>+        for ( i = 0; i < data->nsyms; i++ )
>+        {
>+            if ( !data->symtab[i].new_symbol )
>+                continue;

Do you need symbols other than those marked "new_symbol" past the loading
of the module? If not, wouldn't it be worthwhile to shrink the symbol table to 
just
those, likely speeding up the lookup?

>@@ -379,11 +405,129 @@ static int prepare_payload(struct payload *payload,
         >for ( j = 0; j < ARRAY_SIZE(f->u.pad); j++ )
             >if ( f->u.pad[j] )
                 >return -EINVAL;
>+
>+        /* Lookup function's old address if not already resolved. */
>+        if ( !f->old_addr )
>+        {
>+            f->old_addr = (void *)symbols_lookup_by_name(f->name);
>+            if ( !f->old_addr )
>+            {
>+                f->old_addr = (void *)xsplice_symbols_lookup_by_name(f->name);

The two casts make me wonder whether the two functions shouldn't return
void *, and then whether struct xsplice_symbol's value field shouldn't then
perhaps be void * too.

>+                if ( !f->old_addr )
>+                {
>+                    dprintk(XENLOG_ERR, XSPLICE "%s: Could not resolve old 
>address of %s\n",
>+                            elf->name, f->name);
>+                    return -ENOENT;
>+                }
>+            }
>+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => 
>%p\n",
>+                    elf->name, f->name, f->old_addr);
>+        }
     >}
 >
     >return 0;
 >}
 
So one thing I'm realizing only now: Is there no support for using 
<symbol>+<offset>
to fill ->old_addr?

>+static bool_t is_payload_symbol(const struct xsplice_elf *elf,
>+                                const struct xsplice_elf_sym *sym)
>+{
>+    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);

I don't recall having seen a reply to the question on not allowing STT_NOTYPE 
here.

>+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_payload_symbol(elf, elf->sym + i) )
>+        {
>+            nsyms++;
>+            strtab_len += strlen(elf->sym[i].name) + 1;
>+        }
>+    }
>+
>+    symtab = xmalloc_array(struct xsplice_symbol, nsyms);
>+    strtab = xmalloc_array(char, strtab_len);
>+
>+    if ( !strtab || !symtab )
>+    {
>+        xfree(strtab);
>+        xfree(symtab);
>+        return -ENOMEM;
>+    }
>+
>+    nsyms = 0;
>+    strtab_len = 0;
>+    for ( i = 1; i < elf->nsym; i++ )
>+    {
>+        if ( is_payload_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. */

Why "checked"? The only thing happening further down is this possibly
getting overwritten with 1.

>@@ -274,9 +275,21 @@ int xsplice_elf_resolve_symbols(struct xsplice_elf *elf)
             >break;
 >
         >case SHN_UNDEF:
>-            dprintk(XENLOG_ERR, XSPLICE "%s: Unknown symbol: %s\n",
>-                    elf->name, elf->sym[i].name);
>-            rc = -ENOENT;
>+            elf->sym[i].sym->st_value = (unsigned 
>long)symbols_lookup_by_name(elf->sym[i].name);

With its current return value, the cast is pointless. But this makes clear that 
either
here or above a cast is going to be needed. So perhaps just keep it as is, 
except
for this unnecessary cast.

>+            if ( !elf->sym[i].sym->st_value )
>+            {
>+                elf->sym[i].sym->st_value = (unsigned 
>long)xsplice_symbols_lookup_by_name(elf->sym[i].name);

And this one then, obviously.

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

Either void * (see above) or unsigned long. No point in being 64-bit for e.g. 
ARM32.

>+    size_t size;

I don't see this field being used for anything, it seems to only ever get 
written to.

Jan


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