From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ross Lagerwall Subject: Re: [PATCH v1 07/11] xsplice: Implement payload loading Date: Thu, 5 Nov 2015 11:15:34 +0000 Message-ID: <563B3A56.8020706@citrix.com> References: <1446574568-9644-1-git-send-email-ross.lagerwall@citrix.com> <1446574568-9644-7-git-send-email-ross.lagerwall@citrix.com> <20151104222118.GA25657@char.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20151104222118.GA25657@char.us.oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: Andrew Cooper , Stefano Stabellini , Ian Campbell , Jan Beulich , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 11/04/2015 10:21 PM, Konrad Rzeszutek Wilk wrote: snip >> >> + >> +/* >> + * The following functions prepare an xSplice module to be executed by >> + * allocating space, loading the allocated sections, resolving symbols, >> + * performing relocations, etc. >> + */ >> +#ifdef CONFIG_X86 >> +static void *alloc_module(size_t size) > > s/module/payload/ My intention was that all the code which implements the "module loader" functionality (and is sort of independent from xSplice) uses the term "module" whereas the payload implies the loaded module + the other xSplice-specific bits. Your thoughts? >> +{ >> + mfn_t *mfn, *mfn_ptr; >> + size_t pages, i; >> + struct page_info *pg; >> + unsigned long hole_start, hole_end, cur; >> + struct payload *data, *data2; >> + >> + ASSERT(size); >> + >> + pages = PFN_UP(size); >> + mfn = xmalloc_array(mfn_t, pages); >> + if ( mfn == NULL ) >> + return NULL; >> + >> + for ( i = 0; i < pages; i++ ) >> + { >> + pg = alloc_domheap_page(NULL, 0); >> + if ( pg == NULL ) >> + goto error; >> + mfn[i] = _mfn(page_to_mfn(pg)); >> + } > > This looks like 'vmalloc'. Why not use that? > (That explanation should be part of the commit description probably) vmalloc allocates pages and then maps them to an arbitrary virtual address with PAGE_HYPERVISOR. I needed to use a specific virtual address with PAGE_HYPERVISOR_RWX. > >> + >> + hole_start = (unsigned long)module_virt_start; >> + hole_end = hole_start + pages * PAGE_SIZE; >> + spin_lock(&payload_list_lock); >> + list_for_each_entry ( data, &payload_list, list ) >> + { >> + list_for_each_entry ( data2, &payload_list, list ) >> + { >> + unsigned long start, end; >> + >> + start = (unsigned long)data2->module_address; >> + end = start + data2->module_pages * PAGE_SIZE; >> + if ( hole_end > start && hole_start < end ) >> + { >> + hole_start = end; >> + hole_end = hole_start + pages * PAGE_SIZE; >> + break; >> + } >> + } >> + if ( &data2->list == &payload_list ) >> + break; >> + } >> + spin_unlock(&payload_list_lock); > > This could be made in a nice function. 'find_hole' perhaps? > >> + >> + if ( hole_end >= module_virt_end ) >> + goto error; >> + >> + for ( cur = hole_start, mfn_ptr = mfn; pages--; ++mfn_ptr, cur += PAGE_SIZE ) >> + { >> + if ( map_pages_to_xen(cur, mfn_x(*mfn_ptr), 1, PAGE_HYPERVISOR_RWX) ) >> + { >> + if ( cur != hole_start ) >> + destroy_xen_mappings(hole_start, cur); > > I think 'destroy_xen_mappings' is OK handling hole_start == cur. > >> + goto error; >> + } >> + } >> + xfree(mfn); >> + return (void *)hole_start; >> + >> + error: >> + while ( i-- ) >> + free_domheap_page(mfn_to_page(mfn_x(mfn[i]))); >> + xfree(mfn); >> + return NULL; >> +} >> +#else >> +static void *alloc_module(size_t size) > > s/module/payload/ >> +{ >> + return NULL; >> +} >> +#endif >> + >> +static void free_module(struct payload *payload) >> +{ >> + int i; > > unsigned int; > >> + struct page_info *pg; >> + PAGE_LIST_HEAD(pg_list); >> + void *va = payload->module_address; >> + unsigned long addr = (unsigned long)va; >> + >> + if ( !payload->module_address ) >> + return; > > How about 'if ( !addr ) > return; > ? > >> + >> + payload->module_address = NULL; >> + >> + for ( i = 0; i < payload->module_pages; i++ ) >> + page_list_add(vmap_to_page(va + i * PAGE_SIZE), &pg_list); >> + >> + destroy_xen_mappings(addr, addr + payload->module_pages * PAGE_SIZE); >> + >> + while ( (pg = page_list_remove_head(&pg_list)) != NULL ) >> + free_domheap_page(pg); >> + >> + payload->module_pages = 0; >> +} >> + >> +static void alloc_section(struct xsplice_elf_sec *sec, size_t *core_size) > > s/alloc/compute/? > >> +{ >> + size_t align_size = ROUNDUP(*core_size, sec->sec->sh_addralign); >> + sec->sec->sh_entsize = align_size; >> + *core_size = sec->sec->sh_size + align_size; >> +} >> + >> +static int move_module(struct payload *payload, struct xsplice_elf *elf) >> +{ >> + uint8_t *buf; >> + int i; > > unsigned int i; > >> + size_t core_size = 0; >> + >> + /* Allocate text regions */ > > s/Allocate/Compute/ > >> + for ( i = 0; i < elf->hdr->e_shnum; i++ ) >> + { >> + if ( (elf->sec[i].sec->sh_flags & (SHF_ALLOC|SHF_EXECINSTR)) == >> + (SHF_ALLOC|SHF_EXECINSTR) ) >> + alloc_section(&elf->sec[i], &core_size); >> + } >> + >> + /* Allocate rw data */ >> + for ( i = 0; i < elf->hdr->e_shnum; i++ ) >> + { >> + if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) && >> + !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) && >> + (elf->sec[i].sec->sh_flags & SHF_WRITE) ) >> + alloc_section(&elf->sec[i], &core_size); >> + } >> + >> + /* Allocate ro data */ >> + for ( i = 0; i < elf->hdr->e_shnum; i++ ) >> + { >> + if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) && >> + !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) && >> + !(elf->sec[i].sec->sh_flags & SHF_WRITE) ) >> + alloc_section(&elf->sec[i], &core_size); >> + } >> + >> + buf = alloc_module(core_size); >> + if ( !buf ) { >> + printk(XENLOG_ERR "Could not allocate memory for module\n"); > > (%s: Could not allocate %u memory for payload!\n", elf->name, core_size); > >> + return -ENOMEM; >> + } >> + memset(buf, 0, core_size); > > Perhaps for fun it ought to be 'ud2' ? There's no point. It either gets memcpy'd over or needs to be set to zero for BSS. > >> + >> + for ( i = 0; i < elf->hdr->e_shnum; i++ ) >> + { >> + if ( elf->sec[i].sec->sh_flags & SHF_ALLOC ) >> + { >> + elf->sec[i].load_addr = buf + elf->sec[i].sec->sh_entsize; >> + memcpy(elf->sec[i].load_addr, elf->sec[i].data, >> + elf->sec[i].sec->sh_size); >> + printk(XENLOG_DEBUG "Loaded %s at 0x%p\n", > > Add %s: at the start .. >> + elf->sec[i].name, elf->sec[i].load_addr); > > which would be elf->name. > >> + } >> + } >> + >> + payload->module_address = buf; >> + payload->module_pages = PFN_UP(core_size); > > Instead of module could we name it payload? See comment above. > >> + >> + return 0; >> +} >> + >> +static int resolve_symbols(struct xsplice_elf *elf) > > s/resolve/check/ No, this is resolving section symbols. > >> +{ >> + int i; > > unsigned int; > >> + >> + for ( i = 1; i < elf->nsym; i++ ) > > Why 1? Please explain as comment. The first entry of an ELF symbol table is the "undefined symbol index". This code is expected to be read alongside the ELF specification :-) > > >> + { >> + switch ( elf->sym[i].sym->st_shndx ) >> + { >> + case SHN_COMMON: >> + printk(XENLOG_ERR "Unexpected common symbol: %s\n", >> + elf->sym[i].name); > > Please also include elf->name in the error. > >> + return -EINVAL; >> + break; >> + case SHN_UNDEF: >> + printk(XENLOG_ERR "Unknown symbol: %s\n", elf->sym[i].name); > > Ditto. >> + return -ENOENT; >> + break; >> + case SHN_ABS: >> + printk(XENLOG_DEBUG "Absolute symbol: %s => 0x%p\n", >> + elf->sym[i].name, (void *)elf->sym[i].sym->st_value); >> + break; >> + default: >> + if ( elf->sec[elf->sym[i].sym->st_shndx].sec->sh_flags & SHF_ALLOC ) >> + { >> + elf->sym[i].sym->st_value += >> + (unsigned long)elf->sec[elf->sym[i].sym->st_shndx].load_addr; >> + printk(XENLOG_DEBUG "Symbol resolved: %s => 0x%p\n", > > Ditto; >> + elf->sym[i].name, (void *)elf->sym[i].sym->st_value); >> + } >> + } >> + } >> + >> + return 0; >> +} >> + >> +static int perform_relocs(struct xsplice_elf *elf) >> +{ >> + struct xsplice_elf_sec *rela, *base; >> + int i, rc; >> + > > unsigned int i; > >> + for ( i = 0; i < elf->hdr->e_shnum; i++ ) >> + { >> + rela = &elf->sec[i]; >> + >> + /* Is it a valid relocation section? */ >> + if ( rela->sec->sh_info >= elf->hdr->e_shnum ) >> + continue; > > Um, don't we want to mark it as invalid or such? > Or overwrite it so we won't use it? The code doesn't use it. I don't understand the concern. > >> + >> + base = &elf->sec[rela->sec->sh_info]; >> + >> + /* Don't relocate non-allocated sections */ >> + if ( !(base->sec->sh_flags & SHF_ALLOC) ) >> + continue; > >> + >> + if ( elf->sec[i].sec->sh_type == SHT_RELA ) >> + rc = xsplice_perform_rela(elf, base, rela); >> + else if ( elf->sec[i].sec->sh_type == SHT_REL ) >> + rc = xsplice_perform_rel(elf, base, rela); >> + >> + if ( rc ) >> + return rc; >> + } >> + >> + return 0; >> +} >> + >> +static int load_module(struct payload *payload, uint8_t *raw, ssize_t len) >> +{ >> + struct xsplice_elf elf; > > Wait a minute? We ditch it after this? > >> + int rc = 0; >> + >> + rc = xsplice_verify_elf(raw, len); >> + if ( rc ) >> + return rc; >> + >> + rc = xsplice_elf_load(&elf, raw, len); >> + if ( rc ) >> + return rc; >> + >> + rc = move_module(payload, &elf); >> + if ( rc ) >> + goto err_elf; >> + >> + rc = resolve_symbols(&elf); >> + if ( rc ) >> + goto err_module; >> + >> + rc = perform_relocs(&elf); >> + if ( rc ) >> + goto err_module; >> + > > Shouldn't you call xsplice_elf_free(&elf) here? Or > hook up the elf to the 'struct payload'? > > > If not, who is going to clean up elf->sec and elf->sym when the > payload is unloaded? Yes, I forgot to free it here. >> + return 0; >> + >> + err_module: >> + free_module(payload); >> + err_elf: >> + xsplice_elf_free(&elf); >> + >> + return rc; >> +} >> + >> static int __init xsplice_init(void) >> { >> register_keyhandler('x', xsplice_printall, "print xsplicing info", 1); >> diff --git a/xen/include/asm-x86/x86_64/page.h b/xen/include/asm-x86/x86_64/page.h >> index 19ab4d0..e6f08e9 100644 >> --- a/xen/include/asm-x86/x86_64/page.h >> +++ b/xen/include/asm-x86/x86_64/page.h >> @@ -38,6 +38,8 @@ >> #include >> >> extern unsigned long xen_virt_end; >> +extern unsigned long module_virt_start; >> +extern unsigned long module_virt_end; >> >> #define spage_to_pdx(spg) (((spg) - spage_table)<<(SUPERPAGE_SHIFT-PAGE_SHIFT)) >> #define pdx_to_spage(pdx) (spage_table + ((pdx)>>(SUPERPAGE_SHIFT-PAGE_SHIFT))) >> diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h >> index 41e28da..a3946a3 100644 >> --- a/xen/include/xen/xsplice.h >> +++ b/xen/include/xen/xsplice.h >> @@ -1,9 +1,31 @@ >> #ifndef __XEN_XSPLICE_H__ >> #define __XEN_XSPLICE_H__ >> >> +struct xsplice_elf; >> +struct xsplice_elf_sec; >> +struct xsplice_elf_sym; >> + >> +struct xsplice_patch_func { >> + unsigned long new_addr; >> + unsigned long new_size; >> + unsigned long old_addr; >> + unsigned long old_size; >> + char *name; >> + unsigned char undo[8]; >> +}; > > We don't use them in this patch. They could be moved to another patch. OK. -- Ross Lagerwall