Re: [PATCH v2 5/8] x86/debug: Simplify #DB signal code
On Mon, Aug 24, 2020 at 4:05 AM wrote: > > On Sun, Aug 23, 2020 at 04:09:42PM -0700, Andy Lutomirski wrote: > > On Fri, Aug 21, 2020 at 3:21 AM Peter Zijlstra wrote: > > > > > > Get rid of the two variables, avoid computing si_code when not needed > > > and be consistent about which dr6 value is used. > > > > > > > > - if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS) || > > > user_icebp) > > > - send_sigtrap(regs, 0, si_code); > > > + /* > > > +* 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. > > > +* User wants a sigtrap for that. > > > +*/ > > > + if (dr6 & (DR_STEP | DR_TRAP_BITS) || !dr6) > > > + send_sigtrap(regs, 0, get_si_code(dr6)); > > > > The old condition was ... || (actual DR6 value) and the new condition > > was ... || (stupid notifier-modified DR6 value). I think the old code > > was more correct. > > Hurmph.. /me goes re-read the SDM. > > INT1 is a trap, > instruction breakpoint is a fault > > So if you have: > > INT1 > 1: some-instr > > and set an X breakpoint on 1, we'll loose the INT1, right? > > And because INT1 is a trap, we can't easily decode the instruction > stream either :-( > > Now, OTOH, I suppose you're right in that looking at DR6 early (before > we let people muck about with it) is more reliable for detecting INT1. > Esp since the hw_breakpoint crud can consume all bits. > > So I'll go fix that. What a giant pain in the ass all this is. > > > The right fix is to get rid of the notifier garbage. Want to pick up > > these two changes into your series: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry=b695a5adfdd661a10eead1eebd4002d08400e7ef > > https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry=40ca016606bd39a465feaf5802a8dc3e937aaa06 > > > > And then there is no code left that modifies ->debugreg6 out from under you. > > I'm not convinced. Even with those patches applied we have to use > debugreg6, and that code very much relies on clearing DR_TRAP_BITS > early, and then having ptrace_triggered() re-set bits in it. > > This is so that hw_breakpoint handlers can use breakpoints in userspace > without causing send_sigtrap() on them. Ick. Maybe we can rename this to thread->virtual_dr6. So the traps.c machinery would process dr6 (the actual value from hardware) and generate virtual_dr6 to report to userspace. And no one gets confused about which is which, and no one ever consumes bits from the virtual one.
Re: [PATCH v2 5/8] x86/debug: Simplify #DB signal code
On Mon, Aug 24, 2020 at 4:26 AM Andrew Cooper wrote: > > On 24/08/2020 12:05, pet...@infradead.org wrote: > > On Sun, Aug 23, 2020 at 04:09:42PM -0700, Andy Lutomirski wrote: > >> On Fri, Aug 21, 2020 at 3:21 AM Peter Zijlstra > >> wrote: > >>> Get rid of the two variables, avoid computing si_code when not needed > >>> and be consistent about which dr6 value is used. > >>> > >>> - if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS) || > >>> user_icebp) > >>> - send_sigtrap(regs, 0, si_code); > >>> + /* > >>> +* 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. > >>> +* User wants a sigtrap for that. > >>> +*/ > >>> + if (dr6 & (DR_STEP | DR_TRAP_BITS) || !dr6) > >>> + send_sigtrap(regs, 0, get_si_code(dr6)); > >> The old condition was ... || (actual DR6 value) and the new condition > >> was ... || (stupid notifier-modified DR6 value). I think the old code > >> was more correct. > > Hurmph.. /me goes re-read the SDM. > > > > INT1 is a trap, > > instruction breakpoint is a fault > > > > So if you have: > > > > INT1 > > 1:some-instr > > > > and set an X breakpoint on 1, we'll loose the INT1, right? > > You should get two. First with a dr6 of 0 (ICEBP, RIP pointing at 1:), > and a second with dr6 indicating the X breakpoint (again, RIP pointing > at 1:). > > SDM Vol3 6.9 PRIORITY AMONG SIMULTANEOUS EXCEPTIONS AND INTERRUPTS > > Traps on previous instructions are at priority 4, because they still > "part" of the previous instruction. X breakpoints are priority 7. > > The two #DB's shouldn't merge because nothing inhibits delivery[1] of > the trap at priority 4, and on return from the handler, RF isn't set so > the instruction breakpoint will trigger. > I think that the whole "priority among simultaneous exceptions and interrupts" section is largely BS. An instruction that traps will deliver the exception when it finishes executing unless some other event prevents the instruction from retiring or unless some other trap preempts it. This will happen before the following instruction is decoded, let alone before any possible faults from its execution will occur. Correct me if I'm wrong, but I don't think I am.
Re: [PATCH v2 5/8] x86/debug: Simplify #DB signal code
On Mon, Aug 24, 2020 at 12:26:01PM +0100, Andrew Cooper wrote: > > INT1 is a trap, > > instruction breakpoint is a fault > > > > So if you have: > > > > INT1 > > 1: some-instr > > > > and set an X breakpoint on 1, we'll loose the INT1, right? > > You should get two. First with a dr6 of 0 (ICEBP, RIP pointing at 1:), > and a second with dr6 indicating the X breakpoint (again, RIP pointing > at 1:). > > SDM Vol3 6.9 PRIORITY AMONG SIMULTANEOUS EXCEPTIONS AND INTERRUPTS Ooh, shiny. Clearly I didn't read enough SDM this morning.
Re: [PATCH v2 5/8] x86/debug: Simplify #DB signal code
On 24/08/2020 12:05, pet...@infradead.org wrote: > On Sun, Aug 23, 2020 at 04:09:42PM -0700, Andy Lutomirski wrote: >> On Fri, Aug 21, 2020 at 3:21 AM Peter Zijlstra wrote: >>> Get rid of the two variables, avoid computing si_code when not needed >>> and be consistent about which dr6 value is used. >>> >>> - if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS) || user_icebp) >>> - send_sigtrap(regs, 0, si_code); >>> + /* >>> +* 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. >>> +* User wants a sigtrap for that. >>> +*/ >>> + if (dr6 & (DR_STEP | DR_TRAP_BITS) || !dr6) >>> + send_sigtrap(regs, 0, get_si_code(dr6)); >> The old condition was ... || (actual DR6 value) and the new condition >> was ... || (stupid notifier-modified DR6 value). I think the old code >> was more correct. > Hurmph.. /me goes re-read the SDM. > > INT1 is a trap, > instruction breakpoint is a fault > > So if you have: > > INT1 > 1:some-instr > > and set an X breakpoint on 1, we'll loose the INT1, right? You should get two. First with a dr6 of 0 (ICEBP, RIP pointing at 1:), and a second with dr6 indicating the X breakpoint (again, RIP pointing at 1:). SDM Vol3 6.9 PRIORITY AMONG SIMULTANEOUS EXCEPTIONS AND INTERRUPTS Traps on previous instructions are at priority 4, because they still "part" of the previous instruction. X breakpoints are priority 7. The two #DB's shouldn't merge because nothing inhibits delivery[1] of the trap at priority 4, and on return from the handler, RF isn't set so the instruction breakpoint will trigger. ~Andrew [1] Anyone playing with MovSS gets to keep all resulting pieces.
Re: [PATCH v2 5/8] x86/debug: Simplify #DB signal code
On Sun, Aug 23, 2020 at 04:09:42PM -0700, Andy Lutomirski wrote: > On Fri, Aug 21, 2020 at 3:21 AM Peter Zijlstra wrote: > > > > Get rid of the two variables, avoid computing si_code when not needed > > and be consistent about which dr6 value is used. > > > > > - if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS) || user_icebp) > > - send_sigtrap(regs, 0, si_code); > > + /* > > +* 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. > > +* User wants a sigtrap for that. > > +*/ > > + if (dr6 & (DR_STEP | DR_TRAP_BITS) || !dr6) > > + send_sigtrap(regs, 0, get_si_code(dr6)); > > The old condition was ... || (actual DR6 value) and the new condition > was ... || (stupid notifier-modified DR6 value). I think the old code > was more correct. Hurmph.. /me goes re-read the SDM. INT1 is a trap, instruction breakpoint is a fault So if you have: INT1 1: some-instr and set an X breakpoint on 1, we'll loose the INT1, right? And because INT1 is a trap, we can't easily decode the instruction stream either :-( Now, OTOH, I suppose you're right in that looking at DR6 early (before we let people muck about with it) is more reliable for detecting INT1. Esp since the hw_breakpoint crud can consume all bits. So I'll go fix that. What a giant pain in the ass all this is. > The right fix is to get rid of the notifier garbage. Want to pick up > these two changes into your series: > > https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry=b695a5adfdd661a10eead1eebd4002d08400e7ef > https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry=40ca016606bd39a465feaf5802a8dc3e937aaa06 > > And then there is no code left that modifies ->debugreg6 out from under you. I'm not convinced. Even with those patches applied we have to use debugreg6, and that code very much relies on clearing DR_TRAP_BITS early, and then having ptrace_triggered() re-set bits in it. This is so that hw_breakpoint handlers can use breakpoints in userspace without causing send_sigtrap() on them. So while I hate notifiers as much as the next person, I'm not sure those patches actually help anything at all in this particular case. We can't actually remove the notifier, we can't remove debugreg6 and debugreg6 will still get modified.
Re: [PATCH v2 5/8] x86/debug: Simplify #DB signal code
On Fri, Aug 21, 2020 at 3:21 AM Peter Zijlstra wrote: > > Get rid of the two variables, avoid computing si_code when not needed > and be consistent about which dr6 value is used. > > - if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS) || user_icebp) > - send_sigtrap(regs, 0, si_code); > + /* > +* 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. > +* User wants a sigtrap for that. > +*/ > + if (dr6 & (DR_STEP | DR_TRAP_BITS) || !dr6) > + send_sigtrap(regs, 0, get_si_code(dr6)); The old condition was ... || (actual DR6 value) and the new condition was ... || (stupid notifier-modified DR6 value). I think the old code was more correct. The right fix is to get rid of the notifier garbage. Want to pick up these two changes into your series: https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry=b695a5adfdd661a10eead1eebd4002d08400e7ef https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry=40ca016606bd39a465feaf5802a8dc3e937aaa06 And then there is no code left that modifies ->debugreg6 out from under you. --Andy
[PATCH v2 5/8] x86/debug: Simplify #DB signal code
Get rid of the two variables, avoid computing si_code when not needed
and be consistent about which dr6 value is used.
This makes it easier to shuffle code around later.
Signed-off-by: Peter Zijlstra (Intel)
---
arch/x86/kernel/traps.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -786,15 +786,6 @@ static __always_inline unsigned long deb
static void handle_debug(struct pt_regs *regs, unsigned long dr6)
{
struct task_struct *tsk = current;
- bool user_icebp;
- int si_code;
-
- /*
-* 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.
-* User wants a sigtrap for that.
-*/
- user_icebp = !dr6;
/* Store the virtualized DR6 value */
tsk->thread.debugreg6 = dr6;
@@ -813,6 +804,11 @@ static void handle_debug(struct pt_regs
goto out;
}
+ /*
+* Reload dr6, the notifier might have changed it.
+*/
+ dr6 = tsk->thread.debugreg6;
+
if (WARN_ON_ONCE((dr6 & DR_STEP) && !user_mode(regs))) {
/*
* Historical junk that used to handle SYSENTER single-stepping.
@@ -825,9 +821,13 @@ static void handle_debug(struct pt_regs
regs->flags &= ~X86_EFLAGS_TF;
}
- si_code = get_si_code(tsk->thread.debugreg6);
- if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS) || user_icebp)
- send_sigtrap(regs, 0, si_code);
+ /*
+* 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.
+* User wants a sigtrap for that.
+*/
+ if (dr6 & (DR_STEP | DR_TRAP_BITS) || !dr6)
+ send_sigtrap(regs, 0, get_si_code(dr6));
out:
cond_local_irq_disable(regs);
