| Message ID | 1458849640-22588-14-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> 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
On Fri, Apr 01, 2016 at 09:11:40AM -0600, Jan Beulich wrote: > >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> 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? I added it to xen.efi rule and got: home/konrad/xen/xen/.xen.efi.0s.S: Assembler messages: /home/konrad/xen/xen/.xen.efi.0s.S:21: Warning: value 0x7d2f80000543 truncated to 0x80000543 /home/konrad/xen/xen/.xen.efi.0s.S:22: Warning: value 0x7d2f800008b2 truncated to 0x800008b2 /home/konrad/xen/xen/.xen.efi.0s.S:23: Warning: value 0x7d2f800008b4 truncated to 0x800008b4 /home/konrad/xen/xen/.xen.efi.0s.S:24: Warning: value 0x7d2f800008b9 truncated to 0x800008b9 /home/konrad/xen/xen/.xen.efi.0s.S:25: Warning: value 0x7d2f8000103f truncated to 0x8000103f /home/konrad/xen/xen/.xen.efi.0s.S:26: Warning: value 0x7d2f80001043 truncated to 0x80001043 /home/konrad/xen/xen/.xen.efi.0s.S:27: Warning: value 0x7d2f80001047 truncated to 0x80001047 /home/konrad/xen/xen/.xen.efi.0s.S:6746: Warning: value 0x100650000 truncated to 0x650000 and so on.. Not sure why. The xen.efi file boots thought? > > + 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. I left it as such. I will need to redo this as the xensyms_read is not the best for this and this will need to be redone to be much faster.
>>> On 07.04.16 at 05:14, <konrad.wilk@oracle.com> wrote: > On Fri, Apr 01, 2016 at 09:11:40AM -0600, Jan Beulich wrote: >> >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> 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? > > I added it to xen.efi rule and got: > > home/konrad/xen/xen/.xen.efi.0s.S: Assembler messages: > /home/konrad/xen/xen/.xen.efi.0s.S:21: Warning: value 0x7d2f80000543 > truncated to 0x80000543 > /home/konrad/xen/xen/.xen.efi.0s.S:22: Warning: value 0x7d2f800008b2 > truncated to 0x800008b2 > /home/konrad/xen/xen/.xen.efi.0s.S:23: Warning: value 0x7d2f800008b4 > truncated to 0x800008b4 > /home/konrad/xen/xen/.xen.efi.0s.S:24: Warning: value 0x7d2f800008b9 > truncated to 0x800008b9 > /home/konrad/xen/xen/.xen.efi.0s.S:25: Warning: value 0x7d2f8000103f > truncated to 0x8000103f > /home/konrad/xen/xen/.xen.efi.0s.S:26: Warning: value 0x7d2f80001043 > truncated to 0x80001043 > /home/konrad/xen/xen/.xen.efi.0s.S:27: Warning: value 0x7d2f80001047 > truncated to 0x80001047 > /home/konrad/xen/xen/.xen.efi.0s.S:6746: Warning: value 0x100650000 > truncated to 0x650000 > > and so on.. Not sure why. The xen.efi file boots thought? It's the kallsyms symbol table that suffers, so the image booting fine is not really surprising. But we'd need to understand what specific data objects these warnings originate from - perhaps linker generated symbols not sitting inside sections? Jan
On Thu, Apr 07, 2016 at 09:46:49AM -0600, Jan Beulich wrote: > >>> On 07.04.16 at 05:14, <konrad.wilk@oracle.com> wrote: > > On Fri, Apr 01, 2016 at 09:11:40AM -0600, Jan Beulich wrote: > >> >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> 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? > > > > I added it to xen.efi rule and got: > > > > home/konrad/xen/xen/.xen.efi.0s.S: Assembler messages: > > /home/konrad/xen/xen/.xen.efi.0s.S:21: Warning: value 0x7d2f80000543 > > truncated to 0x80000543 > > /home/konrad/xen/xen/.xen.efi.0s.S:22: Warning: value 0x7d2f800008b2 > > truncated to 0x800008b2 > > /home/konrad/xen/xen/.xen.efi.0s.S:23: Warning: value 0x7d2f800008b4 > > truncated to 0x800008b4 > > /home/konrad/xen/xen/.xen.efi.0s.S:24: Warning: value 0x7d2f800008b9 > > truncated to 0x800008b9 > > /home/konrad/xen/xen/.xen.efi.0s.S:25: Warning: value 0x7d2f8000103f > > truncated to 0x8000103f > > /home/konrad/xen/xen/.xen.efi.0s.S:26: Warning: value 0x7d2f80001043 > > truncated to 0x80001043 > > /home/konrad/xen/xen/.xen.efi.0s.S:27: Warning: value 0x7d2f80001047 > > truncated to 0x80001047 > > /home/konrad/xen/xen/.xen.efi.0s.S:6746: Warning: value 0x100650000 > > truncated to 0x650000 > > > > and so on.. Not sure why. The xen.efi file boots thought? > > It's the kallsyms symbol table that suffers, so the image booting > fine is not really surprising. But we'd need to understand what > specific data objects these warnings originate from - perhaps > linker generated symbols not sitting inside sections? From the .xen.efi.0s.S: .globl symbols_offsets ALGN symbols_offsets: #endif PTR 0x544 - SYMBOLS_ORIGIN PTR 0xa01 - SYMBOLS_ORIGIN PTR 0xa03 - SYMBOLS_ORIGIN PTR 0xa08 - SYMBOLS_ORIGIN PTR 0x118e - SYMBOLS_ORIGIN PTR 0x1192 - SYMBOLS_ORIGIN PTR 0x1196 - SYMBOLS_ORIGIN PTR 0xffff82d080100000 - SYMBOLS_ORIGIN PTR 0xffff82d080100000 - SYMBOLS_ORIGIN which corresponds to: multiboot1_header_start|0000000000000544| ? | | | | multiboot1_header_start|0000000000000a01| ? | | | | multiboot1_header_start|0000000000000a03| ? | | | | multiboot1_header_start|0000000000000a08| ? | | | | multiboot1_header_start|000000000000118e| ? | | | | multiboot1_header_start|0000000000001192| ? | | | | multiboot1_header_start|0000000000001196| ? | | | | which is also found: multiboot1_header_start|ffff82d080100008| t | | | | > > Jan >
>>> On 08.04.16 at 03:32, <konrad@kernel.org> wrote: > On Thu, Apr 07, 2016 at 09:46:49AM -0600, Jan Beulich wrote: >> >>> On 07.04.16 at 05:14, <konrad.wilk@oracle.com> wrote: >> > On Fri, Apr 01, 2016 at 09:11:40AM -0600, Jan Beulich wrote: >> >> >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> 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? >> > >> > I added it to xen.efi rule and got: >> > >> > home/konrad/xen/xen/.xen.efi.0s.S: Assembler messages: >> > /home/konrad/xen/xen/.xen.efi.0s.S:21: Warning: value 0x7d2f80000543 >> > truncated to 0x80000543 >> > /home/konrad/xen/xen/.xen.efi.0s.S:22: Warning: value 0x7d2f800008b2 >> > truncated to 0x800008b2 >> > /home/konrad/xen/xen/.xen.efi.0s.S:23: Warning: value 0x7d2f800008b4 >> > truncated to 0x800008b4 >> > /home/konrad/xen/xen/.xen.efi.0s.S:24: Warning: value 0x7d2f800008b9 >> > truncated to 0x800008b9 >> > /home/konrad/xen/xen/.xen.efi.0s.S:25: Warning: value 0x7d2f8000103f >> > truncated to 0x8000103f >> > /home/konrad/xen/xen/.xen.efi.0s.S:26: Warning: value 0x7d2f80001043 >> > truncated to 0x80001043 >> > /home/konrad/xen/xen/.xen.efi.0s.S:27: Warning: value 0x7d2f80001047 >> > truncated to 0x80001047 >> > /home/konrad/xen/xen/.xen.efi.0s.S:6746: Warning: value 0x100650000 >> > truncated to 0x650000 >> > >> > and so on.. Not sure why. The xen.efi file boots thought? >> >> It's the kallsyms symbol table that suffers, so the image booting >> fine is not really surprising. But we'd need to understand what >> specific data objects these warnings originate from - perhaps >> linker generated symbols not sitting inside sections? > > From the .xen.efi.0s.S: > .globl symbols_offsets > ALGN > symbols_offsets: > #endif > PTR 0x544 - SYMBOLS_ORIGIN > PTR 0xa01 - SYMBOLS_ORIGIN > PTR 0xa03 - SYMBOLS_ORIGIN > PTR 0xa08 - SYMBOLS_ORIGIN > PTR 0x118e - SYMBOLS_ORIGIN > PTR 0x1192 - SYMBOLS_ORIGIN > PTR 0x1196 - SYMBOLS_ORIGIN > PTR 0xffff82d080100000 - SYMBOLS_ORIGIN > PTR 0xffff82d080100000 - SYMBOLS_ORIGIN > > which corresponds to: > multiboot1_header_start|0000000000000544| ? | | | | > multiboot1_header_start|0000000000000a01| ? | | | | > multiboot1_header_start|0000000000000a03| ? | | | | > multiboot1_header_start|0000000000000a08| ? | | | | > multiboot1_header_start|000000000000118e| ? | | | | > multiboot1_header_start|0000000000001192| ? | | | | > multiboot1_header_start|0000000000001196| ? | | | | > > which is also found: > multiboot1_header_start|ffff82d080100008| t | | | | That looks like a binutils issue, associating the wrong name with certain symbols. Depending on what binutils version you have in use, this may be well be one of those I've fixed already. Jan
On Fri, Apr 08, 2016 at 09:21:37AM -0600, Jan Beulich wrote: > >>> On 08.04.16 at 03:32, <konrad@kernel.org> wrote: > > On Thu, Apr 07, 2016 at 09:46:49AM -0600, Jan Beulich wrote: > >> >>> On 07.04.16 at 05:14, <konrad.wilk@oracle.com> wrote: > >> > On Fri, Apr 01, 2016 at 09:11:40AM -0600, Jan Beulich wrote: > >> >> >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> 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? > >> > > >> > I added it to xen.efi rule and got: > >> > > >> > home/konrad/xen/xen/.xen.efi.0s.S: Assembler messages: > >> > /home/konrad/xen/xen/.xen.efi.0s.S:21: Warning: value 0x7d2f80000543 > >> > truncated to 0x80000543 > >> > /home/konrad/xen/xen/.xen.efi.0s.S:22: Warning: value 0x7d2f800008b2 > >> > truncated to 0x800008b2 > >> > /home/konrad/xen/xen/.xen.efi.0s.S:23: Warning: value 0x7d2f800008b4 > >> > truncated to 0x800008b4 > >> > /home/konrad/xen/xen/.xen.efi.0s.S:24: Warning: value 0x7d2f800008b9 > >> > truncated to 0x800008b9 > >> > /home/konrad/xen/xen/.xen.efi.0s.S:25: Warning: value 0x7d2f8000103f > >> > truncated to 0x8000103f > >> > /home/konrad/xen/xen/.xen.efi.0s.S:26: Warning: value 0x7d2f80001043 > >> > truncated to 0x80001043 > >> > /home/konrad/xen/xen/.xen.efi.0s.S:27: Warning: value 0x7d2f80001047 > >> > truncated to 0x80001047 > >> > /home/konrad/xen/xen/.xen.efi.0s.S:6746: Warning: value 0x100650000 > >> > truncated to 0x650000 > >> > > >> > and so on.. Not sure why. The xen.efi file boots thought? > >> > >> It's the kallsyms symbol table that suffers, so the image booting > >> fine is not really surprising. But we'd need to understand what > >> specific data objects these warnings originate from - perhaps > >> linker generated symbols not sitting inside sections? > > > > From the .xen.efi.0s.S: > > .globl symbols_offsets > > ALGN > > symbols_offsets: > > #endif > > PTR 0x544 - SYMBOLS_ORIGIN > > PTR 0xa01 - SYMBOLS_ORIGIN > > PTR 0xa03 - SYMBOLS_ORIGIN > > PTR 0xa08 - SYMBOLS_ORIGIN > > PTR 0x118e - SYMBOLS_ORIGIN > > PTR 0x1192 - SYMBOLS_ORIGIN > > PTR 0x1196 - SYMBOLS_ORIGIN > > PTR 0xffff82d080100000 - SYMBOLS_ORIGIN > > PTR 0xffff82d080100000 - SYMBOLS_ORIGIN > > > > which corresponds to: > > multiboot1_header_start|0000000000000544| ? | | | | > > multiboot1_header_start|0000000000000a01| ? | | | | > > multiboot1_header_start|0000000000000a03| ? | | | | > > multiboot1_header_start|0000000000000a08| ? | | | | > > multiboot1_header_start|000000000000118e| ? | | | | > > multiboot1_header_start|0000000000001192| ? | | | | > > multiboot1_header_start|0000000000001196| ? | | | | > > > > which is also found: > > multiboot1_header_start|ffff82d080100008| t | | | | > > That looks like a binutils issue, associating the wrong name with > certain symbols. Depending on what binutils version you have in > use, this may be well be one of those I've fixed already. [konrad@x230 ~]$ rpm -qa | grep binutils binutils-2.25-9.fc22.x86_64 mingw64-binutils-2.25-1.fc22.x86_64 binutils-devel-2.25-9.fc22.x86_64 mingw-binutils-generic-2.25-1.fc22.x86_64 > > Jan >
>>> On 08.04.16 at 17:27, <konrad.wilk@oracle.com> wrote: > On Fri, Apr 08, 2016 at 09:21:37AM -0600, Jan Beulich wrote: >> >>> On 08.04.16 at 03:32, <konrad@kernel.org> wrote: >> > On Thu, Apr 07, 2016 at 09:46:49AM -0600, Jan Beulich wrote: >> >> >>> On 07.04.16 at 05:14, <konrad.wilk@oracle.com> wrote: >> >> > On Fri, Apr 01, 2016 at 09:11:40AM -0600, Jan Beulich wrote: >> >> >> >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> 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? >> >> > >> >> > I added it to xen.efi rule and got: >> >> > >> >> > home/konrad/xen/xen/.xen.efi.0s.S: Assembler messages: >> >> > /home/konrad/xen/xen/.xen.efi.0s.S:21: Warning: value 0x7d2f80000543 >> >> > truncated to 0x80000543 >> >> > /home/konrad/xen/xen/.xen.efi.0s.S:22: Warning: value 0x7d2f800008b2 >> >> > truncated to 0x800008b2 >> >> > /home/konrad/xen/xen/.xen.efi.0s.S:23: Warning: value 0x7d2f800008b4 >> >> > truncated to 0x800008b4 >> >> > /home/konrad/xen/xen/.xen.efi.0s.S:24: Warning: value 0x7d2f800008b9 >> >> > truncated to 0x800008b9 >> >> > /home/konrad/xen/xen/.xen.efi.0s.S:25: Warning: value 0x7d2f8000103f >> >> > truncated to 0x8000103f >> >> > /home/konrad/xen/xen/.xen.efi.0s.S:26: Warning: value 0x7d2f80001043 >> >> > truncated to 0x80001043 >> >> > /home/konrad/xen/xen/.xen.efi.0s.S:27: Warning: value 0x7d2f80001047 >> >> > truncated to 0x80001047 >> >> > /home/konrad/xen/xen/.xen.efi.0s.S:6746: Warning: value 0x100650000 >> >> > truncated to 0x650000 >> >> > >> >> > and so on.. Not sure why. The xen.efi file boots thought? >> >> >> >> It's the kallsyms symbol table that suffers, so the image booting >> >> fine is not really surprising. But we'd need to understand what >> >> specific data objects these warnings originate from - perhaps >> >> linker generated symbols not sitting inside sections? >> > >> > From the .xen.efi.0s.S: >> > .globl symbols_offsets >> > ALGN >> > symbols_offsets: >> > #endif >> > PTR 0x544 - SYMBOLS_ORIGIN >> > PTR 0xa01 - SYMBOLS_ORIGIN >> > PTR 0xa03 - SYMBOLS_ORIGIN >> > PTR 0xa08 - SYMBOLS_ORIGIN >> > PTR 0x118e - SYMBOLS_ORIGIN >> > PTR 0x1192 - SYMBOLS_ORIGIN >> > PTR 0x1196 - SYMBOLS_ORIGIN >> > PTR 0xffff82d080100000 - SYMBOLS_ORIGIN >> > PTR 0xffff82d080100000 - SYMBOLS_ORIGIN >> > >> > which corresponds to: >> > multiboot1_header_start|0000000000000544| ? | | | | >> > multiboot1_header_start|0000000000000a01| ? | | | | >> > multiboot1_header_start|0000000000000a03| ? | | | | >> > multiboot1_header_start|0000000000000a08| ? | | | | >> > multiboot1_header_start|000000000000118e| ? | | | | >> > multiboot1_header_start|0000000000001192| ? | | | | >> > multiboot1_header_start|0000000000001196| ? | | | | >> > >> > which is also found: >> > multiboot1_header_start|ffff82d080100008| t | | | | > >> >> That looks like a binutils issue, associating the wrong name with >> certain symbols. Depending on what binutils version you have in >> use, this may be well be one of those I've fixed already. > > [konrad@x230 ~]$ rpm -qa | grep binutils > binutils-2.25-9.fc22.x86_64 > mingw64-binutils-2.25-1.fc22.x86_64 > binutils-devel-2.25-9.fc22.x86_64 > mingw-binutils-generic-2.25-1.fc22.x86_64 Yeah, that's surely too old (assuming it also doesn't have those fixes backported) - iirc said changes didn't even make it into 2.26. Jan
On 04/08/2016 06:38 PM, Jan Beulich wrote: > (Did you drop all Cc-s for a reason?) No. Re-adding CCs. > >>>> On 08.04.16 at 18:04, <ross.lagerwall@citrix.com> wrote: >> On 04/01/2016 04:11 PM, Jan Beulich wrote: >>>> + 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? >> >> The binary patch that is being loaded is overriding a hypervisor symbol >> or a symbol introduced in a previous patch. i.e. you're replacing the >> old function with a different one. Binary patches can also introduce >> completely new functions (just above). > > So you mean to say that in symbol lookup, patches take priority > over the core binary? Once we get rid of linear ordering of > patches, how would this yield deterministic results? > Yes. If a patch replaces some functions in the hypervisor, then when performing a symbol lookup you'd want to get the address of the function currently in use, which is the one from the patch. If the linear ordering restriction were removed, then the symbol lookup would simply need to be updated so that it still gets the address of the function currently in use (however that is determined). There's also a different type of symbol lookup in the xsplice code: looking up the address of the symbol to be replaced. In this case, it is the original symbol thatr needs to be returned. This prevents having a chain of jumps if a function is patched multiple times.
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index d9416d1..a1ef24b 100644 --- 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 $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o $(LD) $(LDFLAGS) -T xen.lds -N prelink.o \ $(@D)/.$(@F).1.o -o $@ diff --git a/xen/arch/x86/test/Makefile b/xen/arch/x86/test/Makefile index 7a364c1..22fcb14 100644 --- a/xen/arch/x86/test/Makefile +++ b/xen/arch/x86/test/Makefile @@ -35,14 +35,12 @@ clean:: # the last entry in the build target. # .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)") > $@ .PHONY: xsplice xsplice: config.h diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c index f6ac098..243eb3f 100644 --- a/xen/arch/x86/test/xen_hello_world.c +++ b/xen/arch/x86/test/xen_hello_world.c @@ -11,10 +11,13 @@ static char xen_hello_world_name[] = "xen_hello_world"; extern const char *xen_hello_world(void); +/* External symbol. */ +extern const char *xen_extra_version(void); + struct xsplice_patch_func __section(".xsplice.funcs") xsplice_xen_hello_world = { .name = xen_hello_world_name, .new_addr = (unsigned long)(xen_hello_world), - .old_addr = OLD_CODE, + .old_addr = (unsigned long)(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 92fb0ee..329eba2 100644 --- a/xen/common/symbols.c +++ b/xen/common/symbols.c @@ -207,3 +207,36 @@ int xensyms_read(uint32_t *symnum, char *type, return 0; } + +uint64_t symbols_lookup_by_name(const char *symname) +{ + uint32_t symnum = 0; + uint64_t addr = 0, outaddr = 0; + int rc; + char type; + char name[KSYM_NAME_LEN + 1] = {0}; + + do { + rc = xensyms_read(&symnum, &type, &addr, name); + if ( rc ) + break; + + if ( !strcmp(name, symname) ) + { + outaddr = addr; + break; + } + } while ( name[0] != '\0' ); + + return outaddr; +} + +/* + * 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 a0d7fe0..24100e7 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> @@ -54,6 +55,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. */ + struct xsplice_symbol *symtab; /* All symbols. */ + char *strtab; /* Pointer to .strtab. */ + unsigned int nsyms; /* Nr of entries in .strtab and symbols. */ char name[XEN_XSPLICE_NAME_SIZE]; /* Name of it. */ }; @@ -107,6 +111,34 @@ static int verify_payload(const xen_sysctl_xsplice_upload_t *upload) return 0; } +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: + spin_unlock_recursive(&payload_lock); + return value; +} + /* * We may be holding the payload_lock or not. Hence we need * the recursive spinlock. Or we can judiciously use an @@ -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); + if ( !f->old_addr ) + { + printk(XENLOG_ERR "%s%s: Could not resolve old address of %s\n", + XSPLICE, elf->name, f->name); + return -ENOENT; + } + } + dprintk(XENLOG_DEBUG, "%s%s: Resolved old address %s => 0x%"PRIx64"\n", + XSPLICE, elf->name, f->name, f->old_addr); + } + } + + return 0; +} + +static bool_t is_core_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) ); +} + +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); + if ( !strtab ) + { + xfree(symtab); + return -ENOMEM; + } + + 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); + } } + payload->symtab = symtab; + payload->strtab = strtab; + payload->nsyms = nsyms; + return 0; } @@ -410,6 +560,8 @@ static void free_payload(struct payload *data) payload_cnt--; payload_version++; free_payload_data(data); + xfree(data->symtab); + xfree(data->strtab); xfree(data); } @@ -450,6 +602,10 @@ static int load_payload_data(struct payload *payload, void *raw, ssize_t len) if ( rc ) goto out; + rc = build_symbol_table(payload, &elf); + if ( rc ) + goto out; + rc = secure_payload(payload, &elf); out: @@ -517,7 +673,11 @@ static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload) out: vfree(raw_data); if ( rc ) + { + xfree(data->symtab); + xfree(data->strtab); xfree(data); + } return rc; } diff --git a/xen/common/xsplice_elf.c b/xen/common/xsplice_elf.c index e5c0b12..248e3f3 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> @@ -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); break; default: diff --git a/xen/include/xen/symbols.h b/xen/include/xen/symbols.h index f58e611..d679aa9 100644 --- a/xen/include/xen/symbols.h +++ b/xen/include/xen/symbols.h @@ -23,4 +23,6 @@ const char *symbols_lookup(unsigned long addr, int xensyms_read(uint32_t *symnum, char *type, uint64_t *address, char *name); +uint64_t 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 00b3cb2..8bc55f3 100644 --- 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; + bool_t new_symbol; +}; + int xsplice_op(struct xen_sysctl_xsplice_op *); void check_for_xsplice_work(void); +uint64_t xsplice_symbols_lookup_by_name(const char *symname); /* Arch hooks. */ int arch_xsplice_verify_elf(const struct xsplice_elf *elf, void *data);