Messages in this thread Patch in this message | | | Subject | Re: [PATCH] x86/debug: 'Fix' ptrace dr6 output | From | Tom de Vries <> | Date | Thu, 28 Jan 2021 21:21:04 +0100 |
| |
On 1/28/21 7:20 PM, Peter Zijlstra wrote: > > Tom reported that one of the GDB test-cases failed, and Boris bisected > it to commit: > > d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6") > > The debugging session led us to commit: > > 6c0aca288e72 ("x86: Ignore trap bits on single step exceptions") > > Which describes a nice Wine case in it's bugzilla. Interaction between > TF and DR7 is such that TF has the highest priority, but if both are > triggered by the same instruction and are both trap, DR6 might contain > both BS and B# flags. > > Commit 6c0aca288e72 made sure to not process the B# flags when the BS > flag is set, which is correct since the hardware will generate another > exception. > > The result of commit d53d9bc0cf78 was that in this case the DR6 > register presented to ptrace() would no longer reflect the B# flags > when BS was set. > > Furthermore, previously it would pass down the DR6 bits as observed by > the hardware, which doesn't need to match the order that ptrace() > thinks (although typically there is a 1:1 mapping). > > So explicitly copy DR6 B# bits when BS is set, but make sure to only > copy those bits ptrace() knows about and indexed consistent with what > ptrace expects. > > This restores function to the GDB testcase and retains functionality > of the Wine test-case (which Thomas replicated in a small C program). > > Noteworthy is that Wine explicitly ignores B# bits when BS is set, > because Windows isn't consistent about this either. Why GDB cares > about them is a bit of a mystery, but so be it. >
Looking in gdb.repo/gdb/nat/x86-dregs.c: ... /* Did the watchpoint whose address is in the I'th register break? */ #define X86_DR_WATCH_HIT(dr6, i) ((dr6) & (1 << (i))) ... the point of B# bits seems to be to test which watchpoint hit.
My guess about why gdb wants to know about both BS and B# at the same time: because either of them causes gdb to stop and present a user prompt, and you want to report both events at one user stop, instead of presenting two separate stops and user prompts at the same address.
Thanks, - Tom
> Fixes: d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6") > Reported-by: Tom de Vries <tdevries@suse.de> > Bisected-by: Borislav Petkov <bp@alien8.de> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > Tested-by: Borislav Petkov <bp@alien8.de> > --- > arch/x86/include/asm/hw_breakpoint.h | 2 + > arch/x86/kernel/hw_breakpoint.c | 39 ++++++++++++++++++++++++----------- > arch/x86/kernel/ptrace.c | 26 ++++++++++++++++------- > arch/x86/kernel/traps.c | 5 ++++ > 4 files changed, 52 insertions(+), 20 deletions(-) > > --- a/arch/x86/include/asm/hw_breakpoint.h > +++ b/arch/x86/include/asm/hw_breakpoint.h > @@ -75,6 +75,8 @@ int decode_dr7(unsigned long dr7, int bp > extern int arch_bp_generic_fields(int x86_len, int x86_type, > int *gen_len, int *gen_type); > > +extern int ptrace_bp_idx(struct perf_event *bp); > + > extern struct pmu perf_ops_bp; > > #endif /* _I386_HW_BREAKPOINT_H */ > --- a/arch/x86/kernel/hw_breakpoint.c > +++ b/arch/x86/kernel/hw_breakpoint.c > @@ -496,9 +496,32 @@ static int hw_breakpoint_handler(struct > 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) > + /* > + * If DR_STEP is set, TRAP bits might also be set, but we must not > + * process them since another exception (without DR_STEP) will follow. > + */ > + if (dr6 & DR_STEP) { > + /* > + * Still, userspace wants to see them, so copy those that are > + * due to ptrace() out into their right index. > + */ > + if (user_mode(args->regs)) { > + int idx; > + > + for (i = 0; i < HBP_NUM; i++) { > + if (likely(!(dr6 & (DR_TRAP0 << i)))) > + continue; > + > + bp = this_cpu_read(bp_per_reg[i]); > + idx = ptrace_bp_idx(bp); > + if (idx < 0) > + continue; > + > + current->thread.virtual_dr6 |= (DR_TRAP0 << idx); > + } > + } > return NOTIFY_DONE; > + } > > /* Do an early return if no trap bits are set in DR6 */ > if ((dr6 & DR_TRAP_BITS) == 0) > @@ -509,14 +534,6 @@ static int hw_breakpoint_handler(struct > if (likely(!(dr6 & (DR_TRAP0 << i)))) > continue; > > - /* > - * The counter may be concurrently released but that can only > - * occur from a call_rcu() path. We can then safely fetch > - * the breakpoint, use its callback, touch its counter > - * while we are in an rcu_read_lock() path. > - */ > - rcu_read_lock(); > - > bp = this_cpu_read(bp_per_reg[i]); > /* > * Reset the 'i'th TRAP bit in dr6 to denote completion of > @@ -540,8 +557,6 @@ static int hw_breakpoint_handler(struct > */ > if (bp->hw.info.type == X86_BREAKPOINT_EXECUTE) > args->regs->flags |= X86_EFLAGS_RF; > - > - rcu_read_unlock(); > } > /* > * Further processing in do_debug() is needed for a) user-space > --- a/arch/x86/kernel/ptrace.c > +++ b/arch/x86/kernel/ptrace.c > @@ -449,22 +449,33 @@ static int genregs_set(struct task_struc > return ret; > } > > +int ptrace_bp_idx(struct perf_event *bp) > +{ > + struct thread_struct *thread = ¤t->thread; > + int i; > + > + for (i = 0; i < HBP_NUM; i++) { > + if (thread->ptrace_bps[i] == bp) > + return i; > + } > + > + return -1; > +} > + > static void ptrace_triggered(struct perf_event *bp, > struct perf_sample_data *data, > struct pt_regs *regs) > { > - int i; > struct thread_struct *thread = &(current->thread); > + int i = ptrace_bp_idx(bp); > + > + if (WARN_ON_ONCE(i < 0)) > + return; > > /* > * Store in the virtual DR6 register the fact that the breakpoint > * was hit so the thread's debugger will see it. > */ > - for (i = 0; i < HBP_NUM; i++) { > - if (thread->ptrace_bps[i] == bp) > - break; > - } > - > thread->virtual_dr6 |= (DR_TRAP0 << i); > } > > @@ -518,8 +529,7 @@ ptrace_register_breakpoint(struct task_s > if (err) > return ERR_PTR(err); > > - return register_user_hw_breakpoint(&attr, ptrace_triggered, > - NULL, tsk); > + return register_user_hw_breakpoint(&attr, ptrace_triggered, NULL, tsk); > } > > static int ptrace_modify_breakpoint(struct perf_event *bp, int len, int type, > --- a/arch/x86/kernel/traps.c > +++ b/arch/x86/kernel/traps.c > @@ -838,6 +840,9 @@ static bool notify_debug(struct pt_regs > * > * Notifiers will set bits in @virtual_dr6 to indicate the desire > * for signals - ptrace_triggered(), kgdb_hw_overflow_handler(). > + * > + * @dr6 is in hardware order > + * @virtual_dr6 is in ptrace order > */ > if (notify_die(DIE_DEBUG, "debug", regs, (long)dr6, 0, SIGTRAP) == NOTIFY_STOP) > return true; >
| |