debugger_trap_entry() is reachable during early boot where its unconditional use of current is unsafe. Add a warning to the function to this effect.
Perform the vector check first, as this allows the compiler to elide the other content from most of its callsites. Check guest_mode(regs) before using current, which makes the path safe on early boot. While editing this area, drop DEBUGGER_trap_{entry,fatal}, as hiding a return statement in a function-like macro is very antisocial programming; show the real control flow at each of the callsites. Finally, switch debugger_trap_{entry,fatal} to having boolean return types, to match their semantics. No behavioural change. Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> --- CC: Jan Beulich <jbeul...@suse.com> --- xen/arch/x86/traps.c | 46 ++++++++++++++++++++++++++++++------------ xen/include/asm-x86/debugger.h | 30 ++++++++++++++------------- 2 files changed, 49 insertions(+), 27 deletions(-) diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 767d0b0..b042976 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -736,7 +736,9 @@ void do_reserved_trap(struct cpu_user_regs *regs) { unsigned int trapnr = regs->entry_vector; - DEBUGGER_trap_fatal(trapnr, regs); + if ( debugger_trap_fatal(trapnr, regs) ) + return; + show_execution_state(regs); panic("FATAL RESERVED TRAP %#x: %s", trapnr, trapstr(trapnr)); } @@ -750,7 +752,8 @@ static void do_trap(struct cpu_user_regs *regs, int use_error_code) if ( regs->error_code & X86_XEC_EXT ) goto hardware_trap; - DEBUGGER_trap_entry(trapnr, regs); + if ( debugger_trap_entry(trapnr, regs) ) + return; if ( guest_mode(regs) ) { @@ -777,7 +780,8 @@ static void do_trap(struct cpu_user_regs *regs, int use_error_code) } hardware_trap: - DEBUGGER_trap_fatal(trapnr, regs); + if ( debugger_trap_fatal(trapnr, regs) ) + return; show_execution_state(regs); panic("FATAL TRAP: vector = %d (%s)\n" @@ -1307,7 +1311,8 @@ void do_invalid_op(struct cpu_user_regs *regs) int id = -1, lineno; const struct virtual_region *region; - DEBUGGER_trap_entry(TRAP_invalid_op, regs); + if ( debugger_trap_entry(TRAP_invalid_op, regs) ) + return; if ( likely(guest_mode(regs)) ) { @@ -1377,7 +1382,10 @@ void do_invalid_op(struct cpu_user_regs *regs) case BUGFRAME_bug: printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); - DEBUGGER_trap_fatal(TRAP_invalid_op, regs); + + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) + return; + show_execution_state(regs); panic("Xen BUG at %s%s:%d", prefix, filename, lineno); @@ -1389,7 +1397,10 @@ void do_invalid_op(struct cpu_user_regs *regs) printk("Assertion '%s' failed at %s%s:%d\n", predicate, prefix, filename, lineno); - DEBUGGER_trap_fatal(TRAP_invalid_op, regs); + + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) + return; + show_execution_state(regs); panic("Assertion '%s' failed at %s%s:%d", predicate, prefix, filename, lineno); @@ -1402,14 +1413,18 @@ void do_invalid_op(struct cpu_user_regs *regs) regs->eip = fixup; return; } - DEBUGGER_trap_fatal(TRAP_invalid_op, regs); + + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) + return; + show_execution_state(regs); panic("FATAL TRAP: vector = %d (invalid opcode)", TRAP_invalid_op); } void do_int3(struct cpu_user_regs *regs) { - DEBUGGER_trap_entry(TRAP_int3, regs); + if ( debugger_trap_entry(TRAP_int3, regs) ) + return; if ( !guest_mode(regs) ) { @@ -1744,7 +1759,8 @@ void do_page_fault(struct cpu_user_regs *regs) /* fixup_page_fault() might change regs->error_code, so cache it here. */ error_code = regs->error_code; - DEBUGGER_trap_entry(TRAP_page_fault, regs); + if ( debugger_trap_entry(TRAP_page_fault, regs) ) + return; perfc_incr(page_faults); @@ -1774,7 +1790,8 @@ void do_page_fault(struct cpu_user_regs *regs) return; } - DEBUGGER_trap_fatal(TRAP_page_fault, regs); + if ( debugger_trap_fatal(TRAP_page_fault, regs) ) + return; show_execution_state(regs); show_page_walk(addr); @@ -3471,7 +3488,8 @@ void do_general_protection(struct cpu_user_regs *regs) struct vcpu *v = current; unsigned long fixup; - DEBUGGER_trap_entry(TRAP_gp_fault, regs); + if ( debugger_trap_entry(TRAP_gp_fault, regs) ) + return; if ( regs->error_code & X86_XEC_EXT ) goto hardware_gp; @@ -3543,7 +3561,8 @@ void do_general_protection(struct cpu_user_regs *regs) } hardware_gp: - DEBUGGER_trap_fatal(TRAP_gp_fault, regs); + if ( debugger_trap_fatal(TRAP_gp_fault, regs) ) + return; show_execution_state(regs); panic("GENERAL PROTECTION FAULT\n[error_code=%04x]", regs->error_code); @@ -3778,7 +3797,8 @@ void do_debug(struct cpu_user_regs *regs) { struct vcpu *v = current; - DEBUGGER_trap_entry(TRAP_debug, regs); + if ( debugger_trap_entry(TRAP_debug, regs) ) + return; if ( !guest_mode(regs) ) { diff --git a/xen/include/asm-x86/debugger.h b/xen/include/asm-x86/debugger.h index 271cbaa..82e3dc9 100644 --- a/xen/include/asm-x86/debugger.h +++ b/xen/include/asm-x86/debugger.h @@ -33,17 +33,11 @@ #include <asm/regs.h> #include <asm/processor.h> -/* The main trap handlers use these helper macros which include early bail. */ -#define DEBUGGER_trap_entry(_v, _r) \ - if ( debugger_trap_entry(_v, _r) ) return; -#define DEBUGGER_trap_fatal(_v, _r) \ - if ( debugger_trap_fatal(_v, _r) ) return; - #ifdef CONFIG_CRASH_DEBUG #include <xen/gdbstub.h> -static inline int debugger_trap_fatal( +static inline bool debugger_trap_fatal( unsigned int vector, struct cpu_user_regs *regs) { int rc = __trap_to_gdb(regs, vector); @@ -55,31 +49,39 @@ static inline int debugger_trap_fatal( #else -static inline int debugger_trap_fatal( +static inline bool debugger_trap_fatal( unsigned int vector, struct cpu_user_regs *regs) { - return 0; + return false; } #define debugger_trap_immediate() ((void)0) #endif -static inline int debugger_trap_entry( +static inline bool debugger_trap_entry( unsigned int vector, struct cpu_user_regs *regs) { + /* + * This function is called before any checks are made. Amongst other + * things, be aware that during early boot, current is not a safe pointer + * to follow. + */ struct vcpu *v = current; - if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached && - ((vector == TRAP_int3) || (vector == TRAP_debug)) ) + if ( vector != TRAP_int3 && vector != TRAP_debug ) + return false; + + if ( guest_mode(regs) && guest_kernel_mode(v, regs) && + v->domain->debugger_attached ) { if ( vector != TRAP_debug ) /* domain pause is good enough */ current->arch.gdbsx_vcpu_event = vector; domain_pause_for_debugger(); - return 1; + return true; } - return 0; + return false; } unsigned int dbg_rw_mem(void * __user addr, void * __user buf, -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel