[PATCH 0/3] x86/debug: Fixes

[PATCH 0/3] x86/debug: Fixes

[Peter Zijlstra]

Hi,

Triggered by the x86/entry rework, the resulting #DB cleanup (obviously :/)
broke something. Kyle reported that ptrace_get_debugreg(6) no longer contained
DR_STEP after PTRACE_SINGLESTEP which broke RR.

While looking at this, I realized that a kernel #DB should not consume a
userspace BTF, and equally a kernel #DB should not clobber the (userspace)
ptrace DR6 state. Both these have been busted since forever afaict.

I've added a few Wine folks to Cc, with the hope that they can test Wine
on 5.10-rc and make sure it all still works as expected. There have been
significant changes. Although hopefully it all works again after these
patches.

[PATCH 1/3] x86/debug: Fix BTF handling

[Peter Zijlstra]

The SDM states that #DB clears DEBUGCTLMSR_BTF, this means that when
we set that bit for userspace (TIF_BLOCKSTEP) and get a kernel #DB
first, we'll loose the BTF bit meant for userspace execution.

Have the kernel #DB handler restore the BTF bit when it was requested
for userspace.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/traps.c |   28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -799,13 +799,6 @@ static __always_inline unsigned long deb
 	 */
 	current->thread.virtual_dr6 = 0;
 
-	/*
-	 * 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);
-
 	return dr6;
 }
 
@@ -873,6 +866,20 @@ static __always_inline void exc_debug_ke
 	 */
 	WARN_ON_ONCE(user_mode(regs));
 
+	if (test_thread_flag(TIF_BLOCKSTEP)) {
+		/*
+		 * The SDM says "The processor clears the BTF flag when it
+		 * generates a debug exception." but PTRACE_BLOCKSTEP requested
+		 * it for userspace, but we just took a kernel #DB, so re-set
+		 * BTF.
+		 */
+		unsigned long debugctl;
+
+		rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
+		debugctl |= DEBUGCTLMSR_BTF;
+		wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
+	}
+
 	/*
 	 * Catch SYSENTER with TF set and clear DR_STEP. If this hit a
 	 * watchpoint at the same time then that will still be handled.
@@ -936,6 +943,13 @@ static __always_inline void exc_debug_us
 	instrumentation_begin();
 
 	/*
+	 * 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.
 	 * User wants a sigtrap for that.

[PATCH 2/3] x86/debug: Only clear/set ->virtual_dr6 for userspace #DB

[Peter Zijlstra]

The ->virtual_dr6 is the value used by ptrace_{get,set}_debugreg(6). A
kernel #DB clearing it could mean spurious malfunction of ptrace()
expectations.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/traps.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -793,12 +793,6 @@ static __always_inline unsigned long deb
 	set_debugreg(DR6_RESERVED, 6);
 	dr6 ^= DR6_RESERVED; /* Flip to positive polarity */
 
-	/*
-	 * Clear the virtual DR6 value, ptrace routines will set bits here for
-	 * things we want signals for.
-	 */
-	current->thread.virtual_dr6 = 0;
-
 	return dr6;
 }
 
@@ -943,6 +937,12 @@ static __always_inline void exc_debug_us
 	instrumentation_begin();
 
 	/*
+	 * Clear the virtual DR6 value, ptrace() routines will set bits here
+	 * for things it wants signals for.
+	 */
+	current->thread.virtual_dr6 = 0;
+
+	/*
 	 * 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.

[PATCH 3/3] x86/debug: Fix PTRACE_{BLOCK,SINGLE}STEP vs ptrace_get_debugreg(6)

[Peter Zijlstra]

Commit d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to
thread.virtual_dr6") changed the semantics of the variable from random
collection of bits, to exactly only those bits that ptrace() needs.

Unfortunately we lost DR_STEP for PTRACE_{BLOCK,SINGLE}STEP.

Fixes: d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6")
Reported-by: Kyle Huey <me@kylehuey.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/ptrace.h |    2 ++
 arch/x86/kernel/step.c        |    9 +++++++++
 arch/x86/kernel/traps.c       |    2 +-
 3 files changed, 12 insertions(+), 1 deletion(-)

--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -355,6 +355,8 @@ static inline unsigned long regs_get_ker
 #define arch_has_block_step()	(boot_cpu_data.x86 >= 6)
 #endif
 
+extern unsigned long user_dr_step(unsigned long dr6);
+
 #define ARCH_HAS_USER_SINGLE_STEP_REPORT
 
 struct user_desc;
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -235,3 +235,12 @@ void user_disable_single_step(struct tas
 	if (test_and_clear_tsk_thread_flag(child, TIF_FORCED_TF))
 		task_pt_regs(child)->flags &= ~X86_EFLAGS_TF;
 }
+
+unsigned long user_dr_step(unsigned long dr6)
+{
+	if (test_thread_flag(TIF_BLOCKSTEP) ||
+	    test_thread_flag(TIF_SINGLESTEP))
+		return dr6 & DR_STEP;
+
+	return 0;
+}
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -940,7 +940,7 @@ static __always_inline void exc_debug_us
 	 * Clear the virtual DR6 value, ptrace() routines will set bits here
 	 * for things it wants signals for.
 	 */
-	current->thread.virtual_dr6 = 0;
+	current->thread.virtual_dr6 = user_dr_step(dr6);
 
 	/*
 	 * The SDM says "The processor clears the BTF flag when it

Re: [PATCH 3/3] x86/debug: Fix PTRACE_{BLOCK,SINGLE}STEP vs ptrace_get_debugreg(6)

[Kyle Huey]

On Tue, Oct 27, 2020 at 2:44 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Commit d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to
> thread.virtual_dr6") changed the semantics of the variable from random
> collection of bits, to exactly only those bits that ptrace() needs.
>
> Unfortunately we lost DR_STEP for PTRACE_{BLOCK,SINGLE}STEP.
>
> Fixes: d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6")
> Reported-by: Kyle Huey <me@kylehuey.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/ptrace.h |    2 ++
>  arch/x86/kernel/step.c        |    9 +++++++++
>  arch/x86/kernel/traps.c       |    2 +-
>  3 files changed, 12 insertions(+), 1 deletion(-)
>
> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -355,6 +355,8 @@ static inline unsigned long regs_get_ker
>  #define arch_has_block_step()  (boot_cpu_data.x86 >= 6)
>  #endif
>
> +extern unsigned long user_dr_step(unsigned long dr6);
> +
>  #define ARCH_HAS_USER_SINGLE_STEP_REPORT
>
>  struct user_desc;
> --- a/arch/x86/kernel/step.c
> +++ b/arch/x86/kernel/step.c
> @@ -235,3 +235,12 @@ void user_disable_single_step(struct tas
>         if (test_and_clear_tsk_thread_flag(child, TIF_FORCED_TF))
>                 task_pt_regs(child)->flags &= ~X86_EFLAGS_TF;
>  }
> +
> +unsigned long user_dr_step(unsigned long dr6)
> +{
> +       if (test_thread_flag(TIF_BLOCKSTEP) ||
> +           test_thread_flag(TIF_SINGLESTEP))
> +               return dr6 & DR_STEP;
> +
> +       return 0;
> +}
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -940,7 +940,7 @@ static __always_inline void exc_debug_us
>          * Clear the virtual DR6 value, ptrace() routines will set bits here
>          * for things it wants signals for.
>          */
> -       current->thread.virtual_dr6 = 0;
> +       current->thread.virtual_dr6 = user_dr_step(dr6);
>
>         /*
>          * The SDM says "The processor clears the BTF flag when it
>
>

Tested-by: Kyle Huey <me@kylehuey.com>

Confirmed that this patch series fixes rr when applied on top of the
v5.10-rc1 tag.

- Kyle

[PATCH v2 3/3] Fix DR_STEP vs ptrace_get_debugreg(6)

[Peter Zijlstra]

Commit d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to
thread.virtual_dr6") changed the semantics of the variable from random
collection of bits, to exactly only those bits that ptrace() needs.

Unfortunately we lost DR_STEP for PTRACE_{BLOCK,SINGLE}STEP.

Furthermore, it turns out that userspace expects DR_STEP to be
unconditionally available, even for manual TF usage outside of
PTRACE_{BLOCK,SINGLE}_STEP.

Fixes: d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6")
Reported-by: Kyle Huey <me@kylehuey.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
v2: uncondtionally provide DR_STEP, at the very least WINE relies on this

 arch/x86/kernel/traps.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -937,10 +937,13 @@ static __always_inline void exc_debug_us
 	instrumentation_begin();
 
 	/*
-	 * Clear the virtual DR6 value, ptrace() routines will set bits here
-	 * for things it wants signals for.
+	 * Start the virtual/ptrace DR6 value with just the DR_STEP mask
+	 * of the real DR6. ptrace_triggered() will set the DR_TRAPn bits.
+	 *
+	 * Userspace expects DR_STEP to be visible in ptrace_get_debugreg(6)
+	 * even if it is not the result of PTRACE_SINGLESTEP.
 	 */
-	current->thread.virtual_dr6 = 0;
+	current->thread.virtual_dr6 = (dr6 & DR_STEP);
 
 	/*
 	 * The SDM says "The processor clears the BTF flag when it

Re: [PATCH 1/3] x86/debug: Fix BTF handling

[Peter Zijlstra]

On Tue, Oct 27, 2020 at 10:15:05AM +0100, Peter Zijlstra wrote:

> @@ -873,6 +866,20 @@ static __always_inline void exc_debug_ke
>  	 */
>  	WARN_ON_ONCE(user_mode(regs));
>  
> +	if (test_thread_flag(TIF_BLOCKSTEP)) {
> +		/*
> +		 * The SDM says "The processor clears the BTF flag when it
> +		 * generates a debug exception." but PTRACE_BLOCKSTEP requested
> +		 * it for userspace, but we just took a kernel #DB, so re-set
> +		 * BTF.
> +		 */
> +		unsigned long debugctl;
> +
> +		rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
> +		debugctl |= DEBUGCTLMSR_BTF;
> +		wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
> +	}
> +
>  	/*
>  	 * Catch SYSENTER with TF set and clear DR_STEP. If this hit a
>  	 * watchpoint at the same time then that will still be handled.

Masami, how does BTF interact with !optimized kprobes that single-step?

The best answer I can come up with is 'poorly' :/

[tip: x86/urgent] x86/debug: Fix DR_STEP vs ptrace_get_debugreg(6)

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     cb05143bdf428f280a5d519c82abf196d7871c11
Gitweb:        https://git.kernel.org/tip/cb05143bdf428f280a5d519c82abf196d7871c11
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Tue, 27 Oct 2020 19:33:30 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 27 Oct 2020 23:15:24 +01:00

x86/debug: Fix DR_STEP vs ptrace_get_debugreg(6)

Commit d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to
thread.virtual_dr6") changed the semantics of the variable from random
collection of bits, to exactly only those bits that ptrace() needs.

Unfortunately this lost DR_STEP for PTRACE_{BLOCK,SINGLE}STEP.

Furthermore, it turns out that userspace expects DR_STEP to be
unconditionally available, even for manual TF usage outside of
PTRACE_{BLOCK,SINGLE}_STEP.

Fixes: d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6")
Reported-by: Kyle Huey <me@kylehuey.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Kyle Huey <me@kylehuey.com> 
Link: https://lore.kernel.org/r/20201027183330.GM2628@hirez.programming.kicks-ass.net

---
 arch/x86/kernel/traps.c |  9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 32b2d39..e19df6c 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -937,10 +937,13 @@ static __always_inline void exc_debug_user(struct pt_regs *regs,
 	instrumentation_begin();
 
 	/*
-	 * Clear the virtual DR6 value, ptrace() routines will set bits here
-	 * for things it wants signals for.
+	 * Start the virtual/ptrace DR6 value with just the DR_STEP mask
+	 * of the real DR6. ptrace_triggered() will set the DR_TRAPn bits.
+	 *
+	 * Userspace expects DR_STEP to be visible in ptrace_get_debugreg(6)
+	 * even if it is not the result of PTRACE_SINGLESTEP.
 	 */
-	current->thread.virtual_dr6 = 0;
+	current->thread.virtual_dr6 = (dr6 & DR_STEP);
 
 	/*
 	 * The SDM says "The processor clears the BTF flag when it

[tip: x86/urgent] x86/debug: Only clear/set ->virtual_dr6 for userspace #DB

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     a195f3d4528a2f88d6f986f6b1101775ad4891cf
Gitweb:        https://git.kernel.org/tip/a195f3d4528a2f88d6f986f6b1101775ad4891cf
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Tue, 27 Oct 2020 10:15:06 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 27 Oct 2020 23:15:23 +01:00

x86/debug: Only clear/set ->virtual_dr6 for userspace #DB

The ->virtual_dr6 is the value used by ptrace_{get,set}_debugreg(6). A
kernel #DB clearing it could mean spurious malfunction of ptrace()
expectations.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Kyle Huey <me@kylehuey.com> 
Link: https://lore.kernel.org/r/20201027093608.028952500@infradead.org

---
 arch/x86/kernel/traps.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index b5208aa..32b2d39 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -793,12 +793,6 @@ static __always_inline unsigned long debug_read_clear_dr6(void)
 	set_debugreg(DR6_RESERVED, 6);
 	dr6 ^= DR6_RESERVED; /* Flip to positive polarity */
 
-	/*
-	 * Clear the virtual DR6 value, ptrace routines will set bits here for
-	 * things we want signals for.
-	 */
-	current->thread.virtual_dr6 = 0;
-
 	return dr6;
 }
 
@@ -943,6 +937,12 @@ static __always_inline void exc_debug_user(struct pt_regs *regs,
 	instrumentation_begin();
 
 	/*
+	 * Clear the virtual DR6 value, ptrace() routines will set bits here
+	 * for things it wants signals for.
+	 */
+	current->thread.virtual_dr6 = 0;
+
+	/*
 	 * 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.

[tip: x86/urgent] x86/debug: Fix BTF handling

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     2a9baf5ad4884108b3c6d56a50e8105ccf8a4ee7
Gitweb:        https://git.kernel.org/tip/2a9baf5ad4884108b3c6d56a50e8105ccf8a4ee7
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Tue, 27 Oct 2020 10:15:05 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 27 Oct 2020 23:15:23 +01:00

x86/debug: Fix BTF handling

The SDM states that #DB clears DEBUGCTLMSR_BTF, this means that when the
bit is set for userspace (TIF_BLOCKSTEP) and a kernel #DB happens first,
the BTF bit meant for userspace execution is lost.

Have the kernel #DB handler restore the BTF bit when it was requested
for userspace.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Kyle Huey <me@kylehuey.com> 
Link: https://lore.kernel.org/r/20201027093607.956147736@infradead.org

---
 arch/x86/kernel/traps.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 3c70fb3..b5208aa 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -799,13 +799,6 @@ static __always_inline unsigned long debug_read_clear_dr6(void)
 	 */
 	current->thread.virtual_dr6 = 0;
 
-	/*
-	 * 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);
-
 	return dr6;
 }
 
@@ -873,6 +866,20 @@ static __always_inline void exc_debug_kernel(struct pt_regs *regs,
 	 */
 	WARN_ON_ONCE(user_mode(regs));
 
+	if (test_thread_flag(TIF_BLOCKSTEP)) {
+		/*
+		 * The SDM says "The processor clears the BTF flag when it
+		 * generates a debug exception." but PTRACE_BLOCKSTEP requested
+		 * it for userspace, but we just took a kernel #DB, so re-set
+		 * BTF.
+		 */
+		unsigned long debugctl;
+
+		rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
+		debugctl |= DEBUGCTLMSR_BTF;
+		wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
+	}
+
 	/*
 	 * Catch SYSENTER with TF set and clear DR_STEP. If this hit a
 	 * watchpoint at the same time then that will still be handled.
@@ -936,6 +943,13 @@ static __always_inline void exc_debug_user(struct pt_regs *regs,
 	instrumentation_begin();
 
 	/*
+	 * 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.
 	 * User wants a sigtrap for that.

Re: [PATCH 1/3] x86/debug: Fix BTF handling

[Masami Hiramatsu]

On Tue, 27 Oct 2020 20:41:26 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Oct 27, 2020 at 10:15:05AM +0100, Peter Zijlstra wrote:
> 
> > @@ -873,6 +866,20 @@ static __always_inline void exc_debug_ke
> >  	 */
> >  	WARN_ON_ONCE(user_mode(regs));
> >  
> > +	if (test_thread_flag(TIF_BLOCKSTEP)) {
> > +		/*
> > +		 * The SDM says "The processor clears the BTF flag when it
> > +		 * generates a debug exception." but PTRACE_BLOCKSTEP requested
> > +		 * it for userspace, but we just took a kernel #DB, so re-set
> > +		 * BTF.
> > +		 */
> > +		unsigned long debugctl;
> > +
> > +		rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
> > +		debugctl |= DEBUGCTLMSR_BTF;
> > +		wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
> > +	}
> > +
> >  	/*
> >  	 * Catch SYSENTER with TF set and clear DR_STEP. If this hit a
> >  	 * watchpoint at the same time then that will still be handled.
> 
> Masami, how does BTF interact with !optimized kprobes that single-step?

Good question, BTF is cleared right before single-stepping and restored
after single-stepping. It will be done accoding to TIF_BLOCKSTEP bit as below.

(in arch/x86/kernel/kprobes/core.c)

static nokprobe_inline void clear_btf(void)
{
        if (test_thread_flag(TIF_BLOCKSTEP)) {
                unsigned long debugctl = get_debugctlmsr();

                debugctl &= ~DEBUGCTLMSR_BTF;
                update_debugctlmsr(debugctl);
        }
}

static nokprobe_inline void restore_btf(void)
{
        if (test_thread_flag(TIF_BLOCKSTEP)) {
                unsigned long debugctl = get_debugctlmsr();

                debugctl |= DEBUGCTLMSR_BTF;
                update_debugctlmsr(debugctl);
        }
}

Hrm, so it seems that we do same ... maybe we don't need clear_btf() too?

> 
> The best answer I can come up with is 'poorly' :/

Is this what you expected? :)

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 1/3] x86/debug: Fix BTF handling

[Peter Zijlstra]

On Wed, Oct 28, 2020 at 06:20:25PM +0900, Masami Hiramatsu wrote:
> On Tue, 27 Oct 2020 20:41:26 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Tue, Oct 27, 2020 at 10:15:05AM +0100, Peter Zijlstra wrote:
> > 
> > > @@ -873,6 +866,20 @@ static __always_inline void exc_debug_ke
> > >  	 */
> > >  	WARN_ON_ONCE(user_mode(regs));
> > >  
> > > +	if (test_thread_flag(TIF_BLOCKSTEP)) {
> > > +		/*
> > > +		 * The SDM says "The processor clears the BTF flag when it
> > > +		 * generates a debug exception." but PTRACE_BLOCKSTEP requested
> > > +		 * it for userspace, but we just took a kernel #DB, so re-set
> > > +		 * BTF.
> > > +		 */
> > > +		unsigned long debugctl;
> > > +
> > > +		rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
> > > +		debugctl |= DEBUGCTLMSR_BTF;
> > > +		wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
> > > +	}
> > > +
> > >  	/*
> > >  	 * Catch SYSENTER with TF set and clear DR_STEP. If this hit a
> > >  	 * watchpoint at the same time then that will still be handled.
> > 
> > Masami, how does BTF interact with !optimized kprobes that single-step?
> 
> Good question, BTF is cleared right before single-stepping and restored
> after single-stepping. It will be done accoding to TIF_BLOCKSTEP bit as below.
> 
> (in arch/x86/kernel/kprobes/core.c)
> 
> static nokprobe_inline void clear_btf(void)
> {
>         if (test_thread_flag(TIF_BLOCKSTEP)) {
>                 unsigned long debugctl = get_debugctlmsr();
> 
>                 debugctl &= ~DEBUGCTLMSR_BTF;
>                 update_debugctlmsr(debugctl);
>         }
> }
> 
> static nokprobe_inline void restore_btf(void)
> {
>         if (test_thread_flag(TIF_BLOCKSTEP)) {
>                 unsigned long debugctl = get_debugctlmsr();
> 
>                 debugctl |= DEBUGCTLMSR_BTF;
>                 update_debugctlmsr(debugctl);
>         }
> }
> 
> Hrm, so it seems that we do same ... maybe we don't need clear_btf() too?

No, I think you do very much need clear_btf(). But with my patch perhaps
restore_btf() is no longer needed. Is there only a single single-step
between setup_singlestep() and resume_execution() ? (I think so).

Also, I note that we should employ get_debugctlmsr() more consistently.

> > The best answer I can come up with is 'poorly' :/
> 
> Is this what you expected? :)

Nah, I missed the above, you seems to do the right thing.

Re: [PATCH 1/3] x86/debug: Fix BTF handling

[Masami Hiramatsu]

On Wed, 28 Oct 2020 10:59:19 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Oct 28, 2020 at 06:20:25PM +0900, Masami Hiramatsu wrote:
> > On Tue, 27 Oct 2020 20:41:26 +0100
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Tue, Oct 27, 2020 at 10:15:05AM +0100, Peter Zijlstra wrote:
> > > 
> > > > @@ -873,6 +866,20 @@ static __always_inline void exc_debug_ke
> > > >  	 */
> > > >  	WARN_ON_ONCE(user_mode(regs));
> > > >  
> > > > +	if (test_thread_flag(TIF_BLOCKSTEP)) {
> > > > +		/*
> > > > +		 * The SDM says "The processor clears the BTF flag when it
> > > > +		 * generates a debug exception." but PTRACE_BLOCKSTEP requested
> > > > +		 * it for userspace, but we just took a kernel #DB, so re-set
> > > > +		 * BTF.
> > > > +		 */
> > > > +		unsigned long debugctl;
> > > > +
> > > > +		rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
> > > > +		debugctl |= DEBUGCTLMSR_BTF;
> > > > +		wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
> > > > +	}
> > > > +
> > > >  	/*
> > > >  	 * Catch SYSENTER with TF set and clear DR_STEP. If this hit a
> > > >  	 * watchpoint at the same time then that will still be handled.
> > > 
> > > Masami, how does BTF interact with !optimized kprobes that single-step?
> > 
> > Good question, BTF is cleared right before single-stepping and restored
> > after single-stepping. It will be done accoding to TIF_BLOCKSTEP bit as below.
> > 
> > (in arch/x86/kernel/kprobes/core.c)
> > 
> > static nokprobe_inline void clear_btf(void)
> > {
> >         if (test_thread_flag(TIF_BLOCKSTEP)) {
> >                 unsigned long debugctl = get_debugctlmsr();
> > 
> >                 debugctl &= ~DEBUGCTLMSR_BTF;
> >                 update_debugctlmsr(debugctl);
> >         }
> > }
> > 
> > static nokprobe_inline void restore_btf(void)
> > {
> >         if (test_thread_flag(TIF_BLOCKSTEP)) {
> >                 unsigned long debugctl = get_debugctlmsr();
> > 
> >                 debugctl |= DEBUGCTLMSR_BTF;
> >                 update_debugctlmsr(debugctl);
> >         }
> > }
> > 
> > Hrm, so it seems that we do same ... maybe we don't need clear_btf() too?
> 
> No, I think you do very much need clear_btf(). But with my patch perhaps
> restore_btf() is no longer needed. Is there only a single single-step
> between setup_singlestep() and resume_execution() ? (I think so).

It depends on what the single step instruction does, if it access to the non-present
memory (like user-memory) it kicks the fault handler instead of debug handler.
(e.g. putting a kprobe on the fixup source address in copy_from_user() )
Hmm, on this path, it seems not calling restore_btf()...

Thanks,

> Also, I note that we should employ get_debugctlmsr() more consistently.
> 
> > > The best answer I can come up with is 'poorly' :/
> > 
> > Is this what you expected? :)
> 
> Nah, I missed the above, you seems to do the right thing.


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH] x86/kprobes: Restore BTF if the single-stepping is cancelled

[Masami Hiramatsu]

Fix to restore BTF if single-stepping causes a page fault and
it is cancelled.

Usually the BTF flag was restored when the single stepping is done
(in resume_execution()). However, if a page fault happens on the
single stepping instruction, the fault handler is invoked and
the single stepping is cancelled. Thus, the BTF flag is not
restored.

Fixes: 1ecc798c6764 ("x86: debugctlmsr kprobes")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 547c7abb39f5..39f7d8c3c064 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -937,6 +937,11 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 		 * So clear it by resetting the current kprobe:
 		 */
 		regs->flags &= ~X86_EFLAGS_TF;
+		/*
+		 * Since the single step (trap) has been cancelled,
+		 * we need to restore BTF here.
+		 */
+		restore_btf();
 
 		/*
 		 * If the TF flag was set before the kprobe hit,

Re: [PATCH] x86/kprobes: Restore BTF if the single-stepping is cancelled

[Steven Rostedt]

Did this patch fall through the cracks?

-- Steve


On Wed, 28 Oct 2020 23:31:10 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Fix to restore BTF if single-stepping causes a page fault and
> it is cancelled.
> 
> Usually the BTF flag was restored when the single stepping is done
> (in resume_execution()). However, if a page fault happens on the
> single stepping instruction, the fault handler is invoked and
> the single stepping is cancelled. Thus, the BTF flag is not
> restored.
> 
> Fixes: 1ecc798c6764 ("x86: debugctlmsr kprobes")
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/x86/kernel/kprobes/core.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 547c7abb39f5..39f7d8c3c064 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -937,6 +937,11 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
>  		 * So clear it by resetting the current kprobe:
>  		 */
>  		regs->flags &= ~X86_EFLAGS_TF;
> +		/*
> +		 * Since the single step (trap) has been cancelled,
> +		 * we need to restore BTF here.
> +		 */
> +		restore_btf();
>  
>  		/*
>  		 * If the TF flag was set before the kprobe hit,

[tip: perf/core] x86/kprobes: Restore BTF if the single-stepping is cancelled

[tip-bot2 for Masami Hiramatsu]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     78ff2733ff352175eb7f4418a34654346e1b6cd2
Gitweb:        https://git.kernel.org/tip/78ff2733ff352175eb7f4418a34654346e1b6cd2
Author:        Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate:    Wed, 28 Oct 2020 23:31:10 +09:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 09 Dec 2020 17:08:57 +01:00

x86/kprobes: Restore BTF if the single-stepping is cancelled

Fix to restore BTF if single-stepping causes a page fault and
it is cancelled.

Usually the BTF flag was restored when the single stepping is done
(in resume_execution()). However, if a page fault happens on the
single stepping instruction, the fault handler is invoked and
the single stepping is cancelled. Thus, the BTF flag is not
restored.

Fixes: 1ecc798c6764 ("x86: debugctlmsr kprobes")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/160389546985.106936.12727996109376240993.stgit@devnote2
---
 arch/x86/kernel/kprobes/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 547c7ab..39f7d8c 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -937,6 +937,11 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 		 * So clear it by resetting the current kprobe:
 		 */
 		regs->flags &= ~X86_EFLAGS_TF;
+		/*
+		 * Since the single step (trap) has been cancelled,
+		 * we need to restore BTF here.
+		 */
+		restore_btf();
 
 		/*
 		 * If the TF flag was set before the kprobe hit,