Re: [REGRESSION] x86/debug: After PTRACE_SINGLESTEP DR_STEP is no longer reported in dr6

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Oct 26, 2020 at 10:12:30AM -0700, Kyle Huey wrote:
> On Mon, Oct 26, 2020 at 9:55 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > @@ -935,6 +936,26 @@ static __always_inline void exc_debug_user(struct pt_regs *regs,
> >         irqentry_enter_from_user_mode(regs);
> >         instrumentation_begin();
> >
> > +       /*
> > +        * Clear the virtual DR6 value, ptrace routines will set bits here for
> > +        * things we want signals for.
> > +        */
> > +       current->thread.virtual_dr6 = 0;
> > +
> > +       /*
> > +        * If PTRACE requested SINGLE(BLOCK)STEP, make sure to reflect that in
> > +        * the ptrace visible DR6 copy.
> > +        */
> > +       if (test_thread_flag(TIF_BLOCKSTEP) || test_thread_flag(TIF_SINGLESTEP))
> > +               current->thread.virtual_dr6 |= (dr6 & DR_STEP);
> > +
> > +       /*
> > +        * The SDM says "The processor clears the BTF flag when it
> > +        * generates a debug exception."  Clear TIF_BLOCKSTEP to keep
> > +        * TIF_BLOCKSTEP in sync with the hardware BTF flag.
> > +        */
> > +       clear_thread_flag(TIF_BLOCKSTEP);
> > +
> >         /*
> >          * If dr6 has no reason to give us about the origin of this trap,
> >          * then it's very likely the result of an icebp/int01 trap.
> 
> This looks good to me (at least the non BTF parts), and I'll test it
> shortly, but especially now that clearing virtual_dr6 is moved to
> exc_debug_user I still don't see why it's not ok to copy the entire
> dr6 value into virtual_dr6 unconditionally.  Any extraneous dr6 state
> from an in-kernel #DB would have been picked up and cleared already
> when we entered exc_debug_kernel.

There is !ptrace user breakpoints as well. Why should we want potential
random bits in dr6 ?

Suppose perf and ptrace set a user breakpoint on the exact same
instruction. The #DB fires and has two DR_TRAP# bits set. perf consumes
one and ptrace consumes one.

Only the ptrace one should be visible to ptrace, the perf one doesn't
affect the userspace execution at all and shouldn't be visible.