[PATCH v10 11/24] xsplice, symbols: Implement symbol name resolution on address.

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

From: Ross Lagerwall <ross.lagerwall@citrix.com>

If in the payload we do not have the old_addr we can resolve
the virtual address based on the UNDEFined symbols.

We also use an boolean flag: new_symbol to track symbols. The usual
case this is used is by:

* A payload may introduce a new symbol
* A payload may override an existing symbol (introduced in Xen or another
  payload)
* Overriding symbols must exist in the symtab for backtraces.
* A payload must always link against the object which defines the new symbol.

Considering that payloads may be loaded in any order it would be incorrect to
link against a payload which simply overrides a symbol because you could end
up with a chain of jumps which is inefficient and may result in the expected
function not being executed.

Since the payload we get is an relocatable image (partial linked ELF file)
we have to match up the symbols. We follow the ELF visibility rules for that
and for local symbols do what bintutils ld does.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v1: Ross original version.
v2: Include test-case and document update.
v2: s/size_t/ssize_t/
    Include core_text_size, core_text calculation
v4: Cast on dprintk to uint64_t to make ELF 32bit build.
v6: Rebase where the spinlock is no more recursive. Drop the spinlock
    usage in xsplice_symbols_lookup_by_name
v7: Add Andrew's Reviewed-by
    Initialize addr and symname to zero as xensyms_read uses it.
v8: Change one XENLOG_DEBUG to XENLOG_ERR.
    Change printk to dprintk on symbols and one error case.
v9: 'new_addr' is now void, so change it to from unsigned long
    to void *.
    Include <xen/version.h> header in test case.
    Drop initialized for name in symbols_lookup_by_name.
    Make ->symtab and ->strtab of struct payload const. As such
    cast void* when freeing it.`
    Drop 'size' from xsplice_symbol struct.
    Make return value be void*
    Make 'is_core_symbol' code have the same behavior as what binutils linker
    has.
v10:
   In XENPF_get_symbol use unsigned long instead of uint64_t
   Add Acked-by from Jan.
   Make the return value from symbols_lookup_by_name and xsplice_symbols_lookup_by_name
   be unsigned long to match st_value.
   st_value is now Elf_Addr so use PRI.. to print it.

1
---
 xen/arch/x86/Makefile               |  16 +++-
 xen/arch/x86/platform_hypercall.c   |   5 +-
 xen/arch/x86/test/Makefile          |   4 +-
 xen/arch/x86/test/xen_hello_world.c |   3 +-
 xen/common/symbols.c                |  36 +++++++-
 xen/common/xsplice.c                | 179 +++++++++++++++++++++++++++++++++++-
 xen/common/xsplice_elf.c            |  18 +++-
 xen/include/xen/elfstructs.h        |   1 +
 xen/include/xen/symbols.h           |   4 +-
 xen/include/xen/xsplice.h           |   7 ++
 10 files changed, 257 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index f8f6eeb..fdf4202 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -72,6 +72,12 @@ efi-y := $(shell if [ ! -r $(BASEDIR)/include/xen/compile.h -o \
                       -O $(BASEDIR)/include/xen/compile.h ]; then \
                          echo '$(TARGET).efi'; fi)
 
+ifdef CONFIG_XSPLICE
+all_symbols = --all-symbols
+else
+all_symbols =
+endif
+
 $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
 	./boot/mkelf32 $(TARGET)-syms $(TARGET) 0x100000 \
 	`$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'`
@@ -111,12 +117,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
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
 	    $(@D)/.$(@F).1.o -o $@
@@ -140,14 +148,14 @@ $(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbol
 	                $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).$(base).0 &&) :
 	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S
 	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \
-		| $(guard) $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).0s.S
+		| $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0s.S
 	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
 	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< \
 	                $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o -o $(@D)/.$(@F).$(base).1 &&) :
 	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
 	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \
-		| $(guard) $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).1s.S
+		| $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1s.S
 	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o
 	$(guard) $(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T efi.lds -N $< \
 	                $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o -o $@
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 39fa808..780f22d 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -798,12 +798,13 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
         static char name[KSYM_NAME_LEN + 1]; /* protected by xenpf_lock */
         XEN_GUEST_HANDLE(char) nameh;
         uint32_t namelen, copylen;
+        unsigned long addr;
 
         guest_from_compat_handle(nameh, op->u.symdata.name);
 
         ret = xensyms_read(&op->u.symdata.symnum, &op->u.symdata.type,
-                           &op->u.symdata.address, name);
-
+                           &addr, name);
+        op->u.symdata.address = addr;
         namelen = strlen(name) + 1;
 
         if ( namelen > op->u.symdata.namelen )
diff --git a/xen/arch/x86/test/Makefile b/xen/arch/x86/test/Makefile
index bd81fcd..364408d 100644
--- a/xen/arch/x86/test/Makefile
+++ b/xen/arch/x86/test/Makefile
@@ -23,14 +23,12 @@ clean::
 # and xen_hello_world_func.o to be already compiled.
 #
 .PHONY: config.h
-config.h: OLD_CODE=$(call CODE_ADDR,$(BASEDIR)/xen-syms,xen_extra_version)
 config.h: OLD_CODE_SZ=$(call CODE_SZ,$(BASEDIR)/xen-syms,xen_extra_version)
 config.h: NEW_CODE_SZ=$(call CODE_SZ,$<,xen_hello_world)
 config.h: xen_hello_world_func.o
 	(set -e; \
 	 echo "#define NEW_CODE_SZ $(NEW_CODE_SZ)"; \
-	 echo "#define OLD_CODE_SZ $(OLD_CODE_SZ)"; \
-	 echo "#define OLD_CODE $(OLD_CODE)") > $@
+	 echo "#define OLD_CODE_SZ $(OLD_CODE_SZ)") > $@
 
 xen_hello_world.o: config.h
 
diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c
index f42b25c..8afd66e 100644
--- a/xen/arch/x86/test/xen_hello_world.c
+++ b/xen/arch/x86/test/xen_hello_world.c
@@ -5,6 +5,7 @@
 
 #include "config.h"
 #include <xen/types.h>
+#include <xen/version.h>
 #include <xen/xsplice.h>
 
 #include <public/sysctl.h>
@@ -16,7 +17,7 @@ struct xsplice_patch_func __section(".xsplice.funcs") xsplice_xen_hello_world =
     .version = XSPLICE_PAYLOAD_VERSION,
     .name = hello_world_patch_this_fnc,
     .new_addr = xen_hello_world,
-    .old_addr = (void *)OLD_CODE,
+    .old_addr = xen_extra_version,
     .new_size = NEW_CODE_SZ,
     .old_size = OLD_CODE_SZ,
 };
diff --git a/xen/common/symbols.c b/xen/common/symbols.c
index b18ddcd1..e02a2af 100644
--- a/xen/common/symbols.c
+++ b/xen/common/symbols.c
@@ -170,7 +170,7 @@ static char symbols_get_symbol_type(unsigned int off)
 }
 
 int xensyms_read(uint32_t *symnum, char *type,
-                 uint64_t *address, char *name)
+                 unsigned long *address, char *name)
 {
     /*
      * Symbols are most likely accessed sequentially so we remember position
@@ -207,3 +207,37 @@ int xensyms_read(uint32_t *symnum, char *type,
 
     return 0;
 }
+
+unsigned long symbols_lookup_by_name(const char *symname)
+{
+    char name[KSYM_NAME_LEN + 1];
+    uint32_t symnum = 0;
+    char type;
+    unsigned long addr;
+    int rc;
+
+    if ( *symname == '\0' )
+        return 0;
+
+    do {
+        rc = xensyms_read(&symnum, &type, &addr, name);
+        if ( rc )
+           break;
+
+        if ( !strcmp(name, symname) )
+            return addr;
+
+    } while ( name[0] != '\0' );
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index fcec564..eaea8a4 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -14,6 +14,7 @@
 #include <xen/smp.h>
 #include <xen/softirq.h>
 #include <xen/spinlock.h>
+#include <xen/symbols.h>
 #include <xen/vmap.h>
 #include <xen/wait.h>
 #include <xen/xsplice_elf.h>
@@ -53,6 +54,9 @@ 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. */
+    const struct xsplice_symbol *symtab; /* All symbols. */
+    const char *strtab;                  /* Pointer to .strtab. */
+    unsigned int nsyms;                  /* Nr of entries in .strtab and symbols. */
     char name[XEN_XSPLICE_NAME_SIZE];    /* Name of it. */
 };
 
@@ -113,6 +117,28 @@ static int verify_payload(const xen_sysctl_xsplice_upload_t *upload, char *n)
     return 0;
 }
 
+unsigned long xsplice_symbols_lookup_by_name(const char *symname)
+{
+    const struct payload *data;
+
+    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;
+
+            if ( !strcmp(data->symtab[i].name, symname) )
+                return data->symtab[i].value;
+        }
+    }
+
+    return 0;
+}
+
 static struct payload *find_payload(const char *name)
 {
     struct payload *data, *found = NULL;
@@ -377,8 +403,149 @@ static int prepare_payload(struct payload *payload,
         rc = arch_xsplice_verify_func(f);
         if ( rc )
             return rc;
+
+        /* 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);
+                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;
+}
+
+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;
+
+    /*
+     * The payload is not a final image as we dynmically link against it.
+     * As such the linker has left symbols we don't care about and which
+     * binutils would have removed had it be a final image. Hence we:
+     * - For SHF_ALLOC - ignore symbols referring to sections that are not
+     *   loaded.
+     */
+    if ( !(elf->sec[sym->sym->st_shndx].sec->sh_flags & SHF_ALLOC) )
+        return 0;
+
+    /* - And ignore empty symbols (\0). */
+    if ( *sym->name == '\0' )
+        return 0;
+
+    /*
+     * - For SHF_MERGE - ignore local symbols referring to mergeable sections.
+     *    (ld squashes them all in one section and discards the symbols) when
+     *    those symbols start with '.L' (like .LCx). Those are intermediate
+     *    artifacts of assembly.
+     *
+     * See elf_link_input_bfd and _bfd_elf_is_local_label_name in binutils.
+     */
+    if ( (elf->sec[sym->sym->st_shndx].sec->sh_flags & SHF_MERGE) &&
+         !strncmp(sym->name, ".L", 2) )
+        return 0;
+
+    return 1;
+}
+
+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].value = elf->sym[i].sym->st_value;
+            symtab[nsyms].new_symbol = 0; /* May be overwritten 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 == (unsigned long)payload->funcs[j].new_addr )
+            {
+                found = 1;
+                break;
+            }
+        }
+
+        if ( !found )
+        {
+            if ( xsplice_symbols_lookup_by_name(symtab[i].name) )
+            {
+                dprintk(XENLOG_ERR, XSPLICE "%s: duplicate new symbol: %s\n",
+                        elf->name, symtab[i].name);
+                xfree(symtab);
+                xfree(strtab);
+                return -EEXIST;
+            }
+            symtab[i].new_symbol = 1;
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n",
+                     elf->name, symtab[i].name);
+        }
+        else
+        {
+            /* new_symbol is not set. */
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n",
+                    elf->name, symtab[i].name);
+        }
+    }
+
+    payload->symtab = symtab;
+    payload->strtab = strtab;
+    payload->nsyms = nsyms;
+
     return 0;
 }
 
@@ -389,6 +556,8 @@ static void free_payload(struct payload *data)
     payload_cnt--;
     payload_version++;
     free_payload_data(data);
+    xfree((void *)data->symtab);
+    xfree((void *)data->strtab);
     xfree(data);
 }
 
@@ -421,6 +590,10 @@ static int load_payload_data(struct payload *payload, void *raw, size_t len)
     if ( rc )
         goto out;
 
+    rc = build_symbol_table(payload, &elf);
+    if ( rc )
+        goto out;
+
     rc = secure_payload(payload, &elf);
 
  out:
@@ -480,8 +653,12 @@ static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)
 
     vfree(raw_data);
 
-    if ( rc )
+    if ( rc && data )
+    {
+        xfree((void *)data->symtab);
+        xfree((void *)data->strtab);
         xfree(data);
+    }
 
     return rc;
 }
diff --git a/xen/common/xsplice_elf.c b/xen/common/xsplice_elf.c
index f51a161..e403a0e 100644
--- a/xen/common/xsplice_elf.c
+++ b/xen/common/xsplice_elf.c
@@ -4,6 +4,7 @@
 
 #include <xen/errno.h>
 #include <xen/lib.h>
+#include <xen/symbols.h>
 #include <xen/xsplice_elf.h>
 #include <xen/xsplice.h>
 
@@ -273,9 +274,20 @@ 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;
+            st_value = symbols_lookup_by_name(elf->sym[i].name);
+            if ( !st_value )
+            {
+                st_value = xsplice_symbols_lookup_by_name(elf->sym[i].name);
+                if ( !st_value )
+                {
+                    dprintk(XENLOG_ERR, XSPLICE "%s: Unknown symbol: %s\n",
+                            elf->name, elf->sym[i].name);
+                    rc = -ENOENT;
+                    break;
+                }
+            }
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Undefined symbol resolved: %s => %#"PRIxElfAddr"\n",
+                    elf->name, elf->sym[i].name, st_value);
             break;
 
         case SHN_ABS:
diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h
index 2b9bd3f..ab9b1ea 100644
--- a/xen/include/xen/elfstructs.h
+++ b/xen/include/xen/elfstructs.h
@@ -263,6 +263,7 @@ typedef struct {
 #define SHF_WRITE	0x1		/* Writable */
 #define SHF_ALLOC	0x2		/* occupies memory */
 #define SHF_EXECINSTR	0x4		/* executable */
+#define SHF_MERGE	0x10            /* mergeable */
 #define SHF_MASKPROC	0xf0000000	/* reserved bits for processor */
 					/*  specific section attributes */
 
diff --git a/xen/include/xen/symbols.h b/xen/include/xen/symbols.h
index f58e611..015fcb4 100644
--- a/xen/include/xen/symbols.h
+++ b/xen/include/xen/symbols.h
@@ -21,6 +21,8 @@ const char *symbols_lookup(unsigned long addr,
                            char *namebuf);
 
 int xensyms_read(uint32_t *symnum, char *type,
-                 uint64_t *address, char *name);
+                 unsigned long *address, char *name);
+
+unsigned long symbols_lookup_by_name(const char *symname);
 
 #endif /*_XEN_SYMBOLS_H*/
diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
index c9723e4..0a6020d 100644
--- a/xen/include/xen/xsplice.h
+++ b/xen/include/xen/xsplice.h
@@ -19,8 +19,15 @@ struct xen_sysctl_xsplice_op;
 /* ELF payload special section names. */
 #define ELF_XSPLICE_FUNC    ".xsplice.funcs"
 
+struct xsplice_symbol {
+    const char *name;
+    unsigned long value;
+    bool_t new_symbol;
+};
+
 int xsplice_op(struct xen_sysctl_xsplice_op *);
 void check_for_xsplice_work(void);
+unsigned long xsplice_symbols_lookup_by_name(const char *symname);
 
 /* Arch hooks. */
 int arch_xsplice_verify_elf(const struct xsplice_elf *elf);
-- 
2.5.0


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