| Message ID | 1455300361-13092-10-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/02/16 18:05, Konrad Rzeszutek Wilk wrote: > diff --git a/xen/common/symbols.c b/xen/common/symbols.c > index a59c59d..bf5623f 100644 > --- a/xen/common/symbols.c > +++ b/xen/common/symbols.c > @@ -17,6 +17,7 @@ > #include <xen/lib.h> > #include <xen/string.h> > #include <xen/spinlock.h> > +#include <xen/xsplice.h> > #include <public/platform.h> > #include <xen/guest_access.h> > > @@ -101,6 +102,12 @@ bool_t is_active_kernel_text(unsigned long addr) > (system_state < SYS_STATE_active && is_kernel_inittext(addr))); > } > > +bool_t is_active_text(unsigned long addr) > +{ > + return is_active_kernel_text(addr) || > + is_active_module_text(addr); > +} This would be better as a static inline in a header file, to avoid a call into a separate translation unit. > + > const char *symbols_lookup(unsigned long addr, > unsigned long *symbolsize, > unsigned long *offset, > diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c > index b854c0a..7f71ac6 100644 > --- a/xen/common/xsplice.c > +++ b/xen/common/xsplice.c > @@ -42,7 +42,10 @@ 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. */ > - > + size_t core_size; /* Everything else - .data,.rodata, etc. */ > + size_t core_text_size; /* Only .text size. */ These two lines should be reversed, so the comments make sense. > + struct bug_frame *start_bug_frames[BUGFRAME_NR]; /* .bug.frame patching. */ > + struct bug_frame *stop_bug_frames[BUGFRAME_NR]; > char name[XEN_XSPLICE_NAME_SIZE + 1];/* Name of it. */ > }; > > @@ -561,6 +564,7 @@ static int move_payload(struct payload *payload, struct xsplice_elf *elf) > (SHF_ALLOC|SHF_EXECINSTR) ) > calc_section(&elf->sec[i], &size); > } > + payload->core_text_size = size; > > /* Compute rw data */ > for ( i = 0; i < elf->hdr->e_shnum; i++ ) > @@ -579,6 +583,7 @@ static int move_payload(struct payload *payload, struct xsplice_elf *elf) > !(elf->sec[i].sec->sh_flags & SHF_WRITE) ) > calc_section(&elf->sec[i], &size); > } > + payload->core_size = size; > > buf = alloc_payload(size); > if ( !buf ) { > @@ -663,6 +668,24 @@ static int find_special_sections(struct payload *payload, > if ( f->pad[j] ) > return -EINVAL; > } > + > + /* Optional sections. */ > + for ( i = 0; i < BUGFRAME_NR; i++ ) > + { > + char str[14]; > + > + snprintf(str, sizeof str, ".bug_frames.%d", i); > + sec = xsplice_elf_sec_by_name(elf, str); > + if ( !sec ) > + continue; > + > + if ( ( !sec->sec->sh_size ) || > + ( sec->sec->sh_size % sizeof (struct bug_frame) ) ) > + return -EINVAL; Too many spaces. (not a common style nit!) > + > + payload->start_bug_frames[i] = (struct bug_frame *)sec->load_addr; > + payload->stop_bug_frames[i] = (struct bug_frame *)(sec->load_addr + sec->sec->sh_size); > + } > return 0; > } > > @@ -961,6 +984,72 @@ void do_xsplice(void) > } > } > > + > +/* > + * Functions for handling special sections. > + */ > +struct bug_frame *xsplice_find_bug(const char *eip, int *id) > +{ > + struct payload *data; > + struct bug_frame *bug; > + int i; > + > + /* No locking since this list is only ever changed during apply or revert > + * context. */ > + list_for_each_entry ( data, &applied_list, applied_list ) > + { > + for (i = 0; i < BUGFRAME_NR; i++) { braces on new lines. > + if (!data->start_bug_frames[i]) > + continue; Newline, and can you borrow some spaces from above. > + if ( !((void *)eip >= data->payload_address && > + (void *)eip < (data->payload_address + data->core_text_size))) > + continue; > + > + for ( bug = data->start_bug_frames[i]; bug != data->stop_bug_frames[i]; ++bug ) { > + if ( bug_loc(bug) == eip ) > + { > + *id = i; > + return bug; > + } > + } > + } > + } > + > + return NULL; > +} > + > +bool_t is_module(const void *ptr) I would recommend naming this "is_patch", to avoid the suggestion that Xen supports modules. > +{ > + struct payload *data; > + > + /* No locking since this list is only ever changed during apply or revert > + * context. */ > + list_for_each_entry ( data, &applied_list, applied_list ) > + { > + if ( ptr >= data->payload_address && > + ptr < (data->payload_address + data->core_size)) > + return 1; > + } > + > + return 0; > +} > + > +bool_t is_active_module_text(unsigned long addr) > +{ > + struct payload *data; > + > + /* 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->payload_address && > + (void *)addr < (data->payload_address + data->core_text_size)) > + return 1; > + } > + > + return 0; > +} > + > static int __init xsplice_init(void) > { > register_keyhandler('x', xsplice_printall, "print xsplicing info", 1); > diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h > index d6db1c2..c257b3a 100644 > --- a/xen/include/xen/xsplice.h > +++ b/xen/include/xen/xsplice.h > @@ -26,6 +26,9 @@ struct xsplice_patch_func { > #ifdef CONFIG_XSPLICE > int xsplice_control(struct xen_sysctl_xsplice_op *); > void do_xsplice(void); > +struct bug_frame *xsplice_find_bug(const char *eip, int *id); > +bool_t is_module(const void *addr); > +bool_t is_active_module_text(unsigned long addr); > > /* Arch hooks */ > int xsplice_verify_elf(struct xsplice_elf *elf, uint8_t *data); > @@ -44,5 +47,17 @@ static inline int xsplice_control(struct xen_sysctl_xsplice_op *op) > return -ENOSYS; > } > static inline void do_xsplice(void) { }; > +static inline struct bug_frame *xsplice_find_bug(const char *eip, int *id) > +{ > + return NULL; > +} > +static inline bool_t is_module(const void *addr) > +{ > + return 0; > +} > +static inline bool_t is_active_module_text(unsigned long addr) > +{ > + return 0; > +} There is a neater way of doing this, which doesn't involve having "if ( regular ) else if ( xsplice )" logic chains through the code. Given a struct virtual_region { struct list_head list; unsigned long start, size; struct bug_frame *foo; struct exception_table_entry *bar; }; The init code can construct one for the base hypervisor, and xsplice can add or remove entries from the list. Then, the trap routines search the virtual region list for [start, size) and follow the appropriate pointers. ~Andrew
. snip.. > There is a neater way of doing this, which doesn't involve having "if ( > regular ) else if ( xsplice )" logic chains through the code. s/chains/chain/ There is only one that uses the 'xsplice' name in it:-) The other two are wrapped with the 'is_patch'. > > Given a > > struct virtual_region > { > struct list_head list; > unsigned long start, size; > > struct bug_frame *foo; > struct exception_table_entry *bar; > }; > > The init code can construct one for the base hypervisor, and xsplice can > add or remove entries from the list. Then, the trap routines search the > virtual region list for [start, size) and follow the appropriate pointers. You are suggesting that on bootup we parse the the __stop_bug_frames_[0-3] (different on ARM), and create an linked list to contain those. Then xSplice can call in this API to add their own - and on unload it can unlink them and free them. If m understanding is correct - while it is certainly much nicer, it has drawbacks: - Increases the code to now handle the linked list and all the code around it (And correspondingly we may have now some extra bugs to track). - Bigger memory consumption - we now to have to consume memory for this list - even for the built-in ones. - More code to do for v1 of this patchset. Can we perhaps we can make this a lesser priority and keep it the existing if ( .. ) else if (xsplice_find_bug()..) code construct for Xen 4.7? Thanks.
On 24/02/16 16:22, Konrad Rzeszutek Wilk wrote: > . snip.. >> There is a neater way of doing this, which doesn't involve having "if ( >> regular ) else if ( xsplice )" logic chains through the code. > s/chains/chain/ > > There is only one that uses the 'xsplice' name in it:-) > > The other two are wrapped with the 'is_patch'. >> Given a >> >> struct virtual_region >> { >> struct list_head list; >> unsigned long start, size; >> >> struct bug_frame *foo; >> struct exception_table_entry *bar; >> }; >> >> The init code can construct one for the base hypervisor, and xsplice can >> add or remove entries from the list. Then, the trap routines search the >> virtual region list for [start, size) and follow the appropriate pointers. > You are suggesting that on bootup we parse the the __stop_bug_frames_[0-3] > (different on ARM), and create an linked list to contain those. Can probably manage this at compile time for the builtin ones. > > Then xSplice can call in this API to add their own - and on unload it can > unlink them and free them. > > If m understanding is correct - while it is certainly much nicer, it has drawbacks: > - Increases the code to now handle the linked list and all the code around it > (And correspondingly we may have now some extra bugs to track). > - Bigger memory consumption - we now to have to consume memory for this list - even > for the built-in ones. I mean having one struct virtual_region for Xen itself (perhaps two; one for .text and one for .init which gets removed later), and one extra for each xpatch. The extra memory consumption is a 6 pointers, traded against far cleaner logic for bugfames and extable redirections. ~Andrew
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 26a5026..f3adefa 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -48,6 +48,7 @@ #include <xen/kexec.h> #include <xen/trace.h> #include <xen/paging.h> +#include <xen/xsplice.h> #include <xen/watchdog.h> #include <asm/system.h> #include <asm/io.h> @@ -1161,20 +1162,29 @@ void do_invalid_op(struct cpu_user_regs *regs) return; } - if ( !is_active_kernel_text(regs->eip) || + if ( !is_active_text(regs->eip) || __copy_from_user(bug_insn, eip, sizeof(bug_insn)) || memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) ) goto die; - for ( bug = __start_bug_frames, id = 0; stop_frames[id]; ++bug ) + if ( likely(is_active_kernel_text(regs->eip)) ) { - while ( unlikely(bug == stop_frames[id]) ) - ++id; - if ( bug_loc(bug) == eip ) - break; + for ( bug = __start_bug_frames, id = 0; stop_frames[id]; ++bug ) + { + while ( unlikely(bug == stop_frames[id]) ) + ++id; + if ( bug_loc(bug) == eip ) + break; + } + if ( !stop_frames[id] ) + goto die; + } + else + { + bug = xsplice_find_bug(eip, &id); + if ( !bug ) + goto die; } - if ( !stop_frames[id] ) - goto die; eip += sizeof(bug_insn); if ( id == BUGFRAME_run_fn ) @@ -1188,7 +1198,7 @@ void do_invalid_op(struct cpu_user_regs *regs) /* WARN, BUG or ASSERT: decode the filename pointer and line number. */ filename = bug_ptr(bug); - if ( !is_kernel(filename) ) + if ( !is_kernel(filename) && !is_module(filename) ) goto die; fixup = strlen(filename); if ( fixup > 50 ) @@ -1215,7 +1225,7 @@ void do_invalid_op(struct cpu_user_regs *regs) case BUGFRAME_assert: /* ASSERT: decode the predicate string pointer. */ predicate = bug_msg(bug); - if ( !is_kernel(predicate) ) + if ( !is_kernel(predicate) && !is_module(predicate) ) predicate = "<unknown>"; printk("Assertion '%s' failed at %s%s:%d\n", diff --git a/xen/common/symbols.c b/xen/common/symbols.c index a59c59d..bf5623f 100644 --- a/xen/common/symbols.c +++ b/xen/common/symbols.c @@ -17,6 +17,7 @@ #include <xen/lib.h> #include <xen/string.h> #include <xen/spinlock.h> +#include <xen/xsplice.h> #include <public/platform.h> #include <xen/guest_access.h> @@ -101,6 +102,12 @@ bool_t is_active_kernel_text(unsigned long addr) (system_state < SYS_STATE_active && is_kernel_inittext(addr))); } +bool_t is_active_text(unsigned long addr) +{ + return is_active_kernel_text(addr) || + is_active_module_text(addr); +} + const char *symbols_lookup(unsigned long addr, unsigned long *symbolsize, unsigned long *offset, diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c index b854c0a..7f71ac6 100644 --- a/xen/common/xsplice.c +++ b/xen/common/xsplice.c @@ -42,7 +42,10 @@ 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. */ - + size_t core_size; /* Everything else - .data,.rodata, etc. */ + size_t core_text_size; /* Only .text size. */ + struct bug_frame *start_bug_frames[BUGFRAME_NR]; /* .bug.frame patching. */ + struct bug_frame *stop_bug_frames[BUGFRAME_NR]; char name[XEN_XSPLICE_NAME_SIZE + 1];/* Name of it. */ }; @@ -561,6 +564,7 @@ static int move_payload(struct payload *payload, struct xsplice_elf *elf) (SHF_ALLOC|SHF_EXECINSTR) ) calc_section(&elf->sec[i], &size); } + payload->core_text_size = size; /* Compute rw data */ for ( i = 0; i < elf->hdr->e_shnum; i++ ) @@ -579,6 +583,7 @@ static int move_payload(struct payload *payload, struct xsplice_elf *elf) !(elf->sec[i].sec->sh_flags & SHF_WRITE) ) calc_section(&elf->sec[i], &size); } + payload->core_size = size; buf = alloc_payload(size); if ( !buf ) { @@ -663,6 +668,24 @@ static int find_special_sections(struct payload *payload, if ( f->pad[j] ) return -EINVAL; } + + /* Optional sections. */ + for ( i = 0; i < BUGFRAME_NR; i++ ) + { + char str[14]; + + snprintf(str, sizeof str, ".bug_frames.%d", i); + sec = xsplice_elf_sec_by_name(elf, str); + if ( !sec ) + continue; + + if ( ( !sec->sec->sh_size ) || + ( sec->sec->sh_size % sizeof (struct bug_frame) ) ) + return -EINVAL; + + payload->start_bug_frames[i] = (struct bug_frame *)sec->load_addr; + payload->stop_bug_frames[i] = (struct bug_frame *)(sec->load_addr + sec->sec->sh_size); + } return 0; } @@ -961,6 +984,72 @@ void do_xsplice(void) } } + +/* + * Functions for handling special sections. + */ +struct bug_frame *xsplice_find_bug(const char *eip, int *id) +{ + struct payload *data; + struct bug_frame *bug; + int i; + + /* No locking since this list is only ever changed during apply or revert + * context. */ + list_for_each_entry ( data, &applied_list, applied_list ) + { + for (i = 0; i < BUGFRAME_NR; i++) { + if (!data->start_bug_frames[i]) + continue; + if ( !((void *)eip >= data->payload_address && + (void *)eip < (data->payload_address + data->core_text_size))) + continue; + + for ( bug = data->start_bug_frames[i]; bug != data->stop_bug_frames[i]; ++bug ) { + if ( bug_loc(bug) == eip ) + { + *id = i; + return bug; + } + } + } + } + + return NULL; +} + +bool_t is_module(const void *ptr) +{ + struct payload *data; + + /* No locking since this list is only ever changed during apply or revert + * context. */ + list_for_each_entry ( data, &applied_list, applied_list ) + { + if ( ptr >= data->payload_address && + ptr < (data->payload_address + data->core_size)) + return 1; + } + + return 0; +} + +bool_t is_active_module_text(unsigned long addr) +{ + struct payload *data; + + /* 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->payload_address && + (void *)addr < (data->payload_address + data->core_text_size)) + return 1; + } + + return 0; +} + static int __init xsplice_init(void) { register_keyhandler('x', xsplice_printall, "print xsplicing info", 1); diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h index ab9e811..4df6b2a 100644 --- a/xen/include/asm-arm/bug.h +++ b/xen/include/asm-arm/bug.h @@ -31,6 +31,7 @@ struct bug_frame { #define BUGFRAME_warn 0 #define BUGFRAME_bug 1 #define BUGFRAME_assert 2 +#define BUGFRAME_NR 3 /* Many versions of GCC doesn't support the asm %c parameter which would * be preferable to this unpleasantness. We use mergeable string @@ -39,6 +40,7 @@ struct bug_frame { */ #define BUG_FRAME(type, line, file, has_msg, msg) do { \ BUILD_BUG_ON((line) >> 16); \ + BUILD_BUG_ON(type >= BUGFRAME_NR); \ asm ("1:"BUG_INSTR"\n" \ ".pushsection .rodata.str, \"aMS\", %progbits, 1\n" \ "2:\t.asciz " __stringify(file) "\n" \ diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h index e868e85..5443191 100644 --- a/xen/include/asm-x86/bug.h +++ b/xen/include/asm-x86/bug.h @@ -9,6 +9,7 @@ #define BUGFRAME_warn 1 #define BUGFRAME_bug 2 #define BUGFRAME_assert 3 +#define BUGFRAME_NR 4 #ifndef __ASSEMBLY__ @@ -51,6 +52,7 @@ struct bug_frame { #define BUG_FRAME(type, line, ptr, second_frame, msg) do { \ BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)); \ + BUILD_BUG_ON((type) >= (BUGFRAME_NR)); \ asm volatile ( _ASM_BUGFRAME_TEXT(second_frame) \ :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \ } while (0) diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h index 548b64d..df57754 100644 --- a/xen/include/xen/kernel.h +++ b/xen/include/xen/kernel.h @@ -99,6 +99,7 @@ extern enum system_state { } system_state; bool_t is_active_kernel_text(unsigned long addr); +bool_t is_active_text(unsigned long addr); #endif /* _LINUX_KERNEL_H */ diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h index d6db1c2..c257b3a 100644 --- a/xen/include/xen/xsplice.h +++ b/xen/include/xen/xsplice.h @@ -26,6 +26,9 @@ struct xsplice_patch_func { #ifdef CONFIG_XSPLICE int xsplice_control(struct xen_sysctl_xsplice_op *); void do_xsplice(void); +struct bug_frame *xsplice_find_bug(const char *eip, int *id); +bool_t is_module(const void *addr); +bool_t is_active_module_text(unsigned long addr); /* Arch hooks */ int xsplice_verify_elf(struct xsplice_elf *elf, uint8_t *data); @@ -44,5 +47,17 @@ static inline int xsplice_control(struct xen_sysctl_xsplice_op *op) return -ENOSYS; } static inline void do_xsplice(void) { }; +static inline struct bug_frame *xsplice_find_bug(const char *eip, int *id) +{ + return NULL; +} +static inline bool_t is_module(const void *addr) +{ + return 0; +} +static inline bool_t is_active_module_text(unsigned long addr) +{ + return 0; +} #endif #endif /* __XEN_XSPLICE_H__ */