Message ID | 1458849640-22588-15-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: > @@ -331,16 +332,17 @@ static char *pointer(char *str, char *end, const char **fmt_ptr, > { > unsigned long sym_size, sym_offset; > char namebuf[KSYM_NAME_LEN+1]; > + bool_t payload = 0; > > /* Advance parents fmt string, as we have consumed 's' or 'S' */ > ++*fmt_ptr; > > s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf); > - > - /* If the symbol is not found, fall back to printing the address */ > + /* If the symbol is not found, fall back to printing the address. */ > if ( !s ) > break; > - Please don't drop blank lines like this. > + if ( strncmp(namebuf, s, KSYM_NAME_LEN) ) > + payload = 1; What is this about? A comment is absolutely needed here, the more that without context "payload" is also an unclear term. And then - would simply comparing the two pointers suffice? > +static const char *xsplice_symbols_lookup(unsigned long addr, > + unsigned long *symbolsize, > + unsigned long *offset, > + char *namebuf) > +{ > + struct payload *data; > + unsigned int i; > + int best; > + > + /* > + * No locking since this list is only ever changed during apply or revert > + * context. > + */ > + list_for_each_entry ( data, &applied_list, applied_list ) > + { > + if ( !((void *)addr >= data->text_addr && > + (void *)addr < (data->text_addr + data->text_size)) ) > + continue; It may be just me, but I find such !() constructs harder to understand than the equivalent without that negation. > + best = -1; > + > + for ( i = 0; i < data->nsyms; i++ ) > + { > + if ( data->symtab[i].value <= addr && > + ( best == -1 || Stray blank. Jan
On Fri, Apr 01, 2016 at 09:23:15AM -0600, Jan Beulich wrote: > >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: > > @@ -331,16 +332,17 @@ static char *pointer(char *str, char *end, const char **fmt_ptr, > > { > > unsigned long sym_size, sym_offset; > > char namebuf[KSYM_NAME_LEN+1]; > > + bool_t payload = 0; > > > > /* Advance parents fmt string, as we have consumed 's' or 'S' */ > > ++*fmt_ptr; > > > > s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf); > > - > > - /* If the symbol is not found, fall back to printing the address */ > > + /* If the symbol is not found, fall back to printing the address. */ > > if ( !s ) > > break; > > - > > Please don't drop blank lines like this. > > > + if ( strncmp(namebuf, s, KSYM_NAME_LEN) ) > > + payload = 1; > > What is this about? A comment is absolutely needed here, the > more that without context "payload" is also an unclear term. > And then - would simply comparing the two pointers suffice? Sadly no. The namebuf is defined on the stack (and the value copied in) while the 's' is a pointer to the char* in the symbols. For all Xen hypervisor built-in symbols the 'namebuf' contents and 's' are exactly the same. The only difference is when xSplice comes in and the 'namebuf' has the name of the xSplice payload. I will put a comment here.
>>> Konrad Rzeszutek Wilk <konrad@kernel.org> 04/06/16 4:39 AM >>> >On Fri, Apr 01, 2016 at 09:23:15AM -0600, Jan Beulich wrote: >> >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: >> > @@ -331,16 +332,17 @@ static char *pointer(char *str, char *end, const char **fmt_ptr, >> > { >> > unsigned long sym_size, sym_offset; >> > char namebuf[KSYM_NAME_LEN+1]; >> > + bool_t payload = 0; >> > >> > /* Advance parents fmt string, as we have consumed 's' or 'S' */ >> > ++*fmt_ptr; >> > >> > s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf); >> > - >> > - /* If the symbol is not found, fall back to printing the address */ >> > + /* If the symbol is not found, fall back to printing the address. */ >> > if ( !s ) >> > break; >> > - >> > + if ( strncmp(namebuf, s, KSYM_NAME_LEN) ) >> > + payload = 1; >> >> What is this about? A comment is absolutely needed here, the >> more that without context "payload" is also an unclear term. >> And then - would simply comparing the two pointers suffice? > >Sadly no. The namebuf is defined on the stack (and the value copied in) >while the 's' is a pointer to the char* in the symbols. How does one being on the stack and the other in (I assume) a string table prevent their pointers to be compared? Iirc for the in-hypervisor symbols symbols_lookup() simply returns the passed in namebuf, and hence s == namebuf above. Or I must be missing something... Jan
diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c index 18d2634..f0b743f 100644 --- a/xen/common/vsprintf.c +++ b/xen/common/vsprintf.c @@ -20,6 +20,7 @@ #include <xen/symbols.h> #include <xen/lib.h> #include <xen/sched.h> +#include <xen/xsplice.h> #include <asm/div64.h> #include <asm/page.h> @@ -331,16 +332,17 @@ static char *pointer(char *str, char *end, const char **fmt_ptr, { unsigned long sym_size, sym_offset; char namebuf[KSYM_NAME_LEN+1]; + bool_t payload = 0; /* Advance parents fmt string, as we have consumed 's' or 'S' */ ++*fmt_ptr; s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf); - - /* If the symbol is not found, fall back to printing the address */ + /* If the symbol is not found, fall back to printing the address. */ if ( !s ) break; - + if ( strncmp(namebuf, s, KSYM_NAME_LEN) ) + payload = 1; /* Print symbol name */ str = string(str, end, s, -1, -1, 0); @@ -354,6 +356,13 @@ static char *pointer(char *str, char *end, const char **fmt_ptr, str = number(str, end, sym_size, 16, -1, -1, SPECIAL); } + if ( payload ) + { + str = string(str, end, " [", -1, -1, 0); + str = string(str, end, namebuf, -1, -1, 0); + str = string(str, end, "]", -1, -1, 0); + } + return str; } diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c index 24100e7..5e77360 100644 --- a/xen/common/xsplice.c +++ b/xen/common/xsplice.c @@ -14,7 +14,9 @@ #include <xen/smp.h> #include <xen/softirq.h> #include <xen/spinlock.h> +#include <xen/string.h> #include <xen/symbols.h> +#include <xen/virtual_region.h> #include <xen/vmap.h> #include <xen/wait.h> #include <xen/xsplice_elf.h> @@ -55,6 +57,8 @@ 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 virtual_region region; /* symbol, bug.frame patching and + exception table (x86). */ struct xsplice_symbol *symtab; /* All symbols. */ char *strtab; /* Pointer to .strtab. */ unsigned int nsyms; /* Nr of entries in .strtab and symbols. */ @@ -139,6 +143,51 @@ out: return value; } +static const char *xsplice_symbols_lookup(unsigned long addr, + unsigned long *symbolsize, + unsigned long *offset, + char *namebuf) +{ + struct payload *data; + unsigned int i; + int best; + + /* + * No locking since this list is only ever changed during apply or revert + * context. + */ + list_for_each_entry ( data, &applied_list, applied_list ) + { + if ( !((void *)addr >= data->text_addr && + (void *)addr < (data->text_addr + data->text_size)) ) + continue; + + best = -1; + + for ( i = 0; i < data->nsyms; i++ ) + { + if ( data->symtab[i].value <= addr && + ( best == -1 || + data->symtab[best].value < data->symtab[i].value) ) + best = i; + } + + if ( best == -1 ) + return NULL; + + if ( symbolsize ) + *symbolsize = data->symtab[best].size; + if ( offset ) + *offset = addr - data->symtab[best].value; + if ( namebuf ) + strlcpy(namebuf, data->name, KSYM_NAME_LEN); + + return data->symtab[best].name; + } + + return NULL; +} + /* * We may be holding the payload_lock or not. Hence we need * the recursive spinlock. Or we can judiciously use an @@ -394,6 +443,7 @@ static int prepare_payload(struct payload *payload, struct xsplice_elf_sec *sec; unsigned int i; struct xsplice_patch_func *f; + struct virtual_region *region; sec = xsplice_elf_sec_by_name(elf, ".xsplice.funcs"); if ( sec ) @@ -449,6 +499,13 @@ static int prepare_payload(struct payload *payload, } } + /* Setup the virtual region with proper data. */ + region = &payload->region; + + region->symbols_lookup = xsplice_symbols_lookup; + region->start = (unsigned long)payload->text_addr; + region->end = (unsigned long)(payload->text_addr + payload->text_size); + return 0; } @@ -795,6 +852,7 @@ static int apply_payload(struct payload *data) arch_xsplice_patching_leave(); list_add_tail(&data->applied_list, &applied_list); + register_virtual_region(&data->region); return 0; } @@ -817,6 +875,7 @@ static int revert_payload(struct payload *data) arch_xsplice_patching_leave(); list_del_init(&data->applied_list); + unregister_virtual_region(&data->region); return 0; }