Messages in this thread | | | Date | Wed, 28 Oct 2015 14:55:09 -0600 | Subject | hw_breakpoint.c -- a bad idea | From | Jeffrey Merkey <> |
| |
Allowing other subsystems to clobber the DR7 register is a bad idea when debuggers are loaded. I realize that this piece of code was written for perf monitors, and poor KGDB and KDB have to use this interface to set and clear hardware breakpoints. Problem with this architecture is that is you are using complex chaining of conditional breakpoints in a debugger monitoring cross processor events, this code has no way of knowing the state of subsystems using the breakpoints while it blindly clears the DR7 register during a breakpoint exception. It needs to be either disabled or the ability of a subsystem debugger to flag it as "owned" for a specific period of time.
I realize that the way its coded is designed to share the DR registers, but if a debugger is active, this code is completely disabled anyway, so having it is pointless except for the perf functions when no debugger is active. I am debating adapting MDB to use this interface rather than manage breakpoint registers itself, but I don't see the point in dancing around this code and trying to stand on my head and implement something that removes features and capability from the debugger. MDB supports complex SMP conditional breakpoints, unlike KDB or KGDB.
This interface should be restructured to allow it to be completely disabled if necessary by the debugger or owned and a snipet of code in an event handler should not be blindly clobbering DR7 without knowing the state of debuggers above it. I can move the functionality into this section but is kind of defeats the purpose of having a self contained debugger. A debugger should have minimal dependencies on OS code and be as much a self contained bubble as possible.
At any rate I disable it whem MDB is loaded. I will investigate a batter way to do this to allow perf code to run as well.
Jeff static int hw_breakpoint_handler(struct die_args *args) { int i, cpu, rc = NOTIFY_STOP; struct perf_event *bp; unsigned long dr7, dr6; unsigned long *dr6_p;
/* The DR6 value is pointed by args->err */ dr6_p = (unsigned long *)ERR_PTR(args->err); dr6 = *dr6_p;
/* If it's a single step, TRAP bits are random */ if (dr6 & DR_STEP) return NOTIFY_DONE;
/* Do an early return if no trap bits are set in DR6 */ if ((dr6 & DR_TRAP_BITS) == 0) return NOTIFY_DONE;
// if MDB is loaded turn off #if defined(CONFIG_MDB) || defined(CONFIG_MDB_MODULE) if (disable_hw_bp_interface) return NOTIFY_DONE; #endif
get_debugreg(dr7, 7); /* Disable breakpoints during exception handling */
set_debugreg(0UL, 7); // <--- fucking broken, breaks proceed, conditional, and temp breakpoints in MDB
/* * Assert that local interrupts are disabled * Reset the DRn bits in the virtualized register value. * The ptrace trigger routine will add in whatever is needed. */
| |