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 09:14:13AM -0700, Kyle Huey wrote:
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index 3c70fb34028b..0e7641ac19a8 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -799,6 +799,13 @@ static __always_inline unsigned long debug_read_clear_dr6(void)
> >          */
> >         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
> 
> I don't think the `& DR_STEP` should be necessary, that bit should be
> set by the hardware, I believe.

Yeah, the idea is to only 'copy' the DR_STEP bit, not any others.

> Also if a program sets TF on itself in EFLAGS and generates a trap it
> should still have DR_STEP set in the ptrace-visible dr6, which this
> won't do.

Why? The state was not requested by ptrace(). And the signal should have
it's si_code set to TRACE_TRACE.

> Is there a reason not to always copy dr6 into virtual_dr6 here,
> regardless of TIF_SINGLESTEP/etc?

Why should we expose DR6 bits that are the result of kernel internal
actions?

Suppose someone sets an in-kernel breakpoint (perf can do that for
example), the #DB happens and we write whatever into virtual_dr6.

Even if you have a userspace breakpoint not through ptrace(), then why
should we expose that in dr6 ?


In that respect, I think the current virtual_dr6 = 0 is placed wrong, it
should only be in exc_debug_user(). The only 'problem' then is that we
seem to be able to loose BTF, but perhaps that is already an extant bug.

Consider:

 - perf: setup in-kernel #DB
 - tracer: ptrace(PTRACE_SINGLEBLOCK)
 - tracee: #DB on perf breakpoint, looses BTF
 - tracee .. never triggers actual blockstep

Hmm ? Should we re-set BTF when TIF_BLOCKSTEP && !user_mode(regs) ?