[PATCH 0/7] uprobes: single-step fixes

[PATCH 0/7] uprobes: single-step fixes

[Oleg Nesterov]

Hello.

In short: uprobes should not use user_enable/disable_single_step().
This is unneeded and wrong.

Sebastian, I changed your patches a bit:

	1/7:

		- Change the subject and update the changelog. In particular,
		  s/utrace/uprobes/. I am wondering where this typo came from ;)

	2/7:

		- Rename UPROBE_TF_CHANGES to UPROBE_FIX_SETF to match other
		  *_FIX_* defines.

		- Update the changelog.

		- !!!REMOVE send_sig(SIGTRAP) from arch_uprobe_disable_step!!!
		  Didn't I ask you to make a separate patch for this change? ;)

		  This "else send_sig(SIGTRAP)" is very wrong. Just suppose an
		  application does asm ("pushf; popf") and the 2nd insn is probed.
		  And otoh this is not enough.

		  See 6/7.

please let me know if you disagree.

Oleg.

[PATCH 1/7] uprobes: Introduce arch_uprobe_enable/disable_step()

[Oleg Nesterov]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

As Oleg pointed out in [0] uprobe should not use the ptrace interface
for enabling/disabling single stepping.

[0] http://lkml.kernel.org/20120730141638.GA5306@redhat.com

Add the new "__weak arch" helpers which simply call user_*_single_step()
as a preparation. This is only needed to not break the powerpc port, we
will fold this logic into arch_uprobe_pre/post_xol() hooks later.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/uprobes.h |    2 ++
 kernel/events/uprobes.c |   14 ++++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 6d4fe79..e6f0331 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -112,6 +112,8 @@ extern void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm);
 extern void uprobe_free_utask(struct task_struct *t);
 extern void uprobe_copy_process(struct task_struct *t);
 extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
+extern void __weak arch_uprobe_enable_step(struct arch_uprobe *arch);
+extern void __weak arch_uprobe_disable_step(struct arch_uprobe *arch);
 extern int uprobe_post_sstep_notifier(struct pt_regs *regs);
 extern int uprobe_pre_sstep_notifier(struct pt_regs *regs);
 extern void uprobe_notify_resume(struct pt_regs *regs);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 82a1043..125d1b7 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1444,6 +1444,16 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 	return uprobe;
 }
 
+void __weak arch_uprobe_enable_step(struct arch_uprobe *arch)
+{
+	user_enable_single_step(current);
+}
+
+void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)
+{
+	user_disable_single_step(current);
+}
+
 /*
  * Run handler and ask thread to singlestep.
  * Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
@@ -1490,7 +1500,7 @@ static void handle_swbp(struct pt_regs *regs)
 
 	utask->state = UTASK_SSTEP;
 	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
-		user_enable_single_step(current);
+		arch_uprobe_enable_step(&uprobe->arch);
 		return;
 	}
 
@@ -1529,7 +1539,7 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
 	put_uprobe(uprobe);
 	utask->active_uprobe = NULL;
 	utask->state = UTASK_RUNNING;
-	user_disable_single_step(current);
+	arch_uprobe_disable_step(&uprobe->arch);
 	xol_free_insn_slot(current);
 
 	spin_lock_irq(&current->sighand->siglock);
-- 
1.5.5.1

Re: [PATCH 1/7] uprobes: Introduce arch_uprobe_enable/disable_step()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-03 17:25:59]:

> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> As Oleg pointed out in [0] uprobe should not use the ptrace interface
> for enabling/disabling single stepping.
> 
> [0] http://lkml.kernel.org/20120730141638.GA5306@redhat.com

I think the link should be 
http://lkml.kernel.org/r/20120730141638.GA5306@redhat.com


> 
> Add the new "__weak arch" helpers which simply call user_*_single_step()
> as a preparation. This is only needed to not break the powerpc port, we
> will fold this logic into arch_uprobe_pre/post_xol() hooks later.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  include/linux/uprobes.h |    2 ++
>  kernel/events/uprobes.c |   14 ++++++++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 6d4fe79..e6f0331 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -112,6 +112,8 @@ extern void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm);
>  extern void uprobe_free_utask(struct task_struct *t);
>  extern void uprobe_copy_process(struct task_struct *t);
>  extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
> +extern void __weak arch_uprobe_enable_step(struct arch_uprobe *arch);
> +extern void __weak arch_uprobe_disable_step(struct arch_uprobe *arch);
>  extern int uprobe_post_sstep_notifier(struct pt_regs *regs);
>  extern int uprobe_pre_sstep_notifier(struct pt_regs *regs);
>  extern void uprobe_notify_resume(struct pt_regs *regs);
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 82a1043..125d1b7 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1444,6 +1444,16 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
>  	return uprobe;
>  }
> 
> +void __weak arch_uprobe_enable_step(struct arch_uprobe *arch)
> +{
> +	user_enable_single_step(current);
> +}
> +
> +void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)
> +{
> +	user_disable_single_step(current);
> +}
> +
>  /*
>   * Run handler and ask thread to singlestep.
>   * Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
> @@ -1490,7 +1500,7 @@ static void handle_swbp(struct pt_regs *regs)
> 
>  	utask->state = UTASK_SSTEP;
>  	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
> -		user_enable_single_step(current);
> +		arch_uprobe_enable_step(&uprobe->arch);
>  		return;
>  	}
> 
> @@ -1529,7 +1539,7 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
>  	put_uprobe(uprobe);
>  	utask->active_uprobe = NULL;
>  	utask->state = UTASK_RUNNING;
> -	user_disable_single_step(current);
> +	arch_uprobe_disable_step(&uprobe->arch);
>  	xol_free_insn_slot(current);
> 
>  	spin_lock_irq(&current->sighand->siglock);
> -- 
> 1.5.5.1
> 

[PATCH 2/7] uprobes: x86: Implement x86 specific arch_uprobe_*_step

[Oleg Nesterov]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

The arch specific implementation behaves like user_enable_single_step()
except that it does not disable single stepping if it was already
enabled by ptrace. This allows the debugger to single step over an
uprobe. The state of block stepping is not restored. It makes only sense
together with TF and if that was enabled then the debugger is notified.

Note: this is still not correct. For example, TIF_SINGLESTEP check
is not right, the application itsel can set X86_EFLAGS_TF. And otoh
we leak TIF_SINGLESTEP (set by enable) if the probed insn is "popf".
See the next patches, we need the changes in arch/x86/kernel/step.c
first.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/include/asm/uprobes.h |    2 ++
 arch/x86/kernel/uprobes.c      |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index f3971bb..cee5862 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -46,6 +46,8 @@ struct arch_uprobe_task {
 #ifdef CONFIG_X86_64
 	unsigned long			saved_scratch_register;
 #endif
+#define UPROBE_CLEAR_TF			(1 << 0)
+	unsigned int			restore_flags;
 };
 
 extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 36fd420..309a0e0 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -41,6 +41,9 @@
 /* Adjust the return address of a call insn */
 #define UPROBE_FIX_CALL	0x2
 
+/* Instruction will modify TF, don't change it */
+#define UPROBE_FIX_SETF	0x4
+
 #define UPROBE_FIX_RIP_AX	0x8000
 #define UPROBE_FIX_RIP_CX	0x4000
 
@@ -239,6 +242,10 @@ static void prepare_fixups(struct arch_uprobe *auprobe, struct insn *insn)
 	insn_get_opcode(insn);	/* should be a nop */
 
 	switch (OPCODE1(insn)) {
+	case 0x9d:
+		/* popf */
+		auprobe->fixups |= UPROBE_FIX_SETF;
+		break;
 	case 0xc3:		/* ret/lret */
 	case 0xcb:
 	case 0xc2:
@@ -673,3 +680,29 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	}
 	return false;
 }
+
+void arch_uprobe_enable_step(struct arch_uprobe *auprobe)
+{
+	struct uprobe_task	*utask		= current->utask;
+	struct arch_uprobe_task	*autask		= &utask->autask;
+
+	autask->restore_flags = 0;
+	if (!test_tsk_thread_flag(current, TIF_SINGLESTEP) &&
+			!(auprobe->fixups & UPROBE_FIX_SETF))
+		autask->restore_flags |= UPROBE_CLEAR_TF;
+	/*
+	 * The state of TIF_BLOCKSTEP is not saved. With the TF flag set we
+	 * would to examine the opcode and the flags to make it right. Without
+	 * TF block stepping makes no sense.
+	 */
+	user_enable_single_step(current);
+}
+
+void arch_uprobe_disable_step(struct arch_uprobe *auprobe)
+{
+	struct uprobe_task *utask		= current->utask;
+	struct arch_uprobe_task	*autask		= &utask->autask;
+
+	if (autask->restore_flags & UPROBE_CLEAR_TF)
+		user_disable_single_step(current);
+}
-- 
1.5.5.1

Re: [PATCH 2/7] uprobes: x86: Implement x86 specific arch_uprobe_*_step

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-03 17:26:02]:

> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> The arch specific implementation behaves like user_enable_single_step()
> except that it does not disable single stepping if it was already
> enabled by ptrace. This allows the debugger to single step over an
> uprobe. The state of block stepping is not restored. It makes only sense
> together with TF and if that was enabled then the debugger is notified.
> 
> Note: this is still not correct. For example, TIF_SINGLESTEP check
> is not right, the application itsel can set X86_EFLAGS_TF. And otoh

nit: 
s/itsel/itself

> we leak TIF_SINGLESTEP (set by enable) if the probed insn is "popf".
> See the next patches, we need the changes in arch/x86/kernel/step.c
> first.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>


Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  arch/x86/include/asm/uprobes.h |    2 ++
>  arch/x86/kernel/uprobes.c      |   33 +++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index f3971bb..cee5862 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -46,6 +46,8 @@ struct arch_uprobe_task {
>  #ifdef CONFIG_X86_64
>  	unsigned long			saved_scratch_register;
>  #endif
> +#define UPROBE_CLEAR_TF			(1 << 0)
> +	unsigned int			restore_flags;
>  };
> 
>  extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 36fd420..309a0e0 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -41,6 +41,9 @@
>  /* Adjust the return address of a call insn */
>  #define UPROBE_FIX_CALL	0x2
> 
> +/* Instruction will modify TF, don't change it */
> +#define UPROBE_FIX_SETF	0x4
> +
>  #define UPROBE_FIX_RIP_AX	0x8000
>  #define UPROBE_FIX_RIP_CX	0x4000
> 
> @@ -239,6 +242,10 @@ static void prepare_fixups(struct arch_uprobe *auprobe, struct insn *insn)
>  	insn_get_opcode(insn);	/* should be a nop */
> 
>  	switch (OPCODE1(insn)) {
> +	case 0x9d:
> +		/* popf */
> +		auprobe->fixups |= UPROBE_FIX_SETF;
> +		break;
>  	case 0xc3:		/* ret/lret */
>  	case 0xcb:
>  	case 0xc2:
> @@ -673,3 +680,29 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  	}
>  	return false;
>  }
> +
> +void arch_uprobe_enable_step(struct arch_uprobe *auprobe)
> +{
> +	struct uprobe_task	*utask		= current->utask;
> +	struct arch_uprobe_task	*autask		= &utask->autask;
> +
> +	autask->restore_flags = 0;
> +	if (!test_tsk_thread_flag(current, TIF_SINGLESTEP) &&
> +			!(auprobe->fixups & UPROBE_FIX_SETF))
> +		autask->restore_flags |= UPROBE_CLEAR_TF;
> +	/*
> +	 * The state of TIF_BLOCKSTEP is not saved. With the TF flag set we
> +	 * would to examine the opcode and the flags to make it right. Without
> +	 * TF block stepping makes no sense.
> +	 */
> +	user_enable_single_step(current);
> +}
> +
> +void arch_uprobe_disable_step(struct arch_uprobe *auprobe)
> +{
> +	struct uprobe_task *utask		= current->utask;
> +	struct arch_uprobe_task	*autask		= &utask->autask;
> +
> +	if (autask->restore_flags & UPROBE_CLEAR_TF)
> +		user_disable_single_step(current);
> +}
> -- 
> 1.5.5.1
> 

[PATCH 3/7] ptrace: Introduce set_task_blockstep() helper

[Oleg Nesterov]

No functional changes, preparation for the next fix and for uprobes
single-step fixes.

Move the code playing with TIF_BLOCKSTEP/DEBUGCTLMSR_BTF into the
new helper, set_task_blockstep().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/step.c |   41 +++++++++++++++++++++--------------------
 1 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
index c346d11..7a51498 100644
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -157,6 +157,21 @@ static int enable_single_step(struct task_struct *child)
 	return 1;
 }
 
+static void set_task_blockstep(struct task_struct *task, bool on)
+{
+	unsigned long debugctl;
+
+	debugctl = get_debugctlmsr();
+	if (on) {
+		debugctl |= DEBUGCTLMSR_BTF;
+		set_tsk_thread_flag(task, TIF_BLOCKSTEP);
+	} else {
+		debugctl &= ~DEBUGCTLMSR_BTF;
+		clear_tsk_thread_flag(task, TIF_BLOCKSTEP);
+	}
+	update_debugctlmsr(debugctl);
+}
+
 /*
  * Enable single or block step.
  */
@@ -169,19 +184,10 @@ static void enable_step(struct task_struct *child, bool block)
 	 * So no one should try to use debugger block stepping in a program
 	 * that uses user-mode single stepping itself.
 	 */
-	if (enable_single_step(child) && block) {
-		unsigned long debugctl = get_debugctlmsr();
-
-		debugctl |= DEBUGCTLMSR_BTF;
-		update_debugctlmsr(debugctl);
-		set_tsk_thread_flag(child, TIF_BLOCKSTEP);
-	} else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
-		unsigned long debugctl = get_debugctlmsr();
-
-		debugctl &= ~DEBUGCTLMSR_BTF;
-		update_debugctlmsr(debugctl);
-		clear_tsk_thread_flag(child, TIF_BLOCKSTEP);
-	}
+	if (enable_single_step(child) && block)
+		set_task_blockstep(child, true);
+	else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP))
+		set_task_blockstep(child, false);
 }
 
 void user_enable_single_step(struct task_struct *child)
@@ -199,13 +205,8 @@ void user_disable_single_step(struct task_struct *child)
 	/*
 	 * Make sure block stepping (BTF) is disabled.
 	 */
-	if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
-		unsigned long debugctl = get_debugctlmsr();
-
-		debugctl &= ~DEBUGCTLMSR_BTF;
-		update_debugctlmsr(debugctl);
-		clear_tsk_thread_flag(child, TIF_BLOCKSTEP);
-	}
+	if (test_tsk_thread_flag(child, TIF_BLOCKSTEP))
+		set_task_blockstep(child, false);
 
 	/* Always clear TIF_SINGLESTEP... */
 	clear_tsk_thread_flag(child, TIF_SINGLESTEP);
-- 
1.5.5.1

Re: [PATCH 3/7] ptrace: Introduce set_task_blockstep() helper

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-03 17:26:06]:

> No functional changes, preparation for the next fix and for uprobes
> single-step fixes.
> 
> Move the code playing with TIF_BLOCKSTEP/DEBUGCTLMSR_BTF into the
> new helper, set_task_blockstep().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  arch/x86/kernel/step.c |   41 +++++++++++++++++++++--------------------
>  1 files changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
> index c346d11..7a51498 100644
> --- a/arch/x86/kernel/step.c
> +++ b/arch/x86/kernel/step.c
> @@ -157,6 +157,21 @@ static int enable_single_step(struct task_struct *child)
>  	return 1;
>  }
> 
> +static void set_task_blockstep(struct task_struct *task, bool on)
> +{
> +	unsigned long debugctl;
> +
> +	debugctl = get_debugctlmsr();
> +	if (on) {
> +		debugctl |= DEBUGCTLMSR_BTF;
> +		set_tsk_thread_flag(task, TIF_BLOCKSTEP);
> +	} else {
> +		debugctl &= ~DEBUGCTLMSR_BTF;
> +		clear_tsk_thread_flag(task, TIF_BLOCKSTEP);
> +	}
> +	update_debugctlmsr(debugctl);
> +}
> +
>  /*
>   * Enable single or block step.
>   */
> @@ -169,19 +184,10 @@ static void enable_step(struct task_struct *child, bool block)
>  	 * So no one should try to use debugger block stepping in a program
>  	 * that uses user-mode single stepping itself.
>  	 */
> -	if (enable_single_step(child) && block) {
> -		unsigned long debugctl = get_debugctlmsr();
> -
> -		debugctl |= DEBUGCTLMSR_BTF;
> -		update_debugctlmsr(debugctl);
> -		set_tsk_thread_flag(child, TIF_BLOCKSTEP);
> -	} else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
> -		unsigned long debugctl = get_debugctlmsr();
> -
> -		debugctl &= ~DEBUGCTLMSR_BTF;
> -		update_debugctlmsr(debugctl);
> -		clear_tsk_thread_flag(child, TIF_BLOCKSTEP);
> -	}
> +	if (enable_single_step(child) && block)
> +		set_task_blockstep(child, true);
> +	else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP))
> +		set_task_blockstep(child, false);
>  }
> 
>  void user_enable_single_step(struct task_struct *child)
> @@ -199,13 +205,8 @@ void user_disable_single_step(struct task_struct *child)
>  	/*
>  	 * Make sure block stepping (BTF) is disabled.
>  	 */
> -	if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
> -		unsigned long debugctl = get_debugctlmsr();
> -
> -		debugctl &= ~DEBUGCTLMSR_BTF;
> -		update_debugctlmsr(debugctl);
> -		clear_tsk_thread_flag(child, TIF_BLOCKSTEP);
> -	}
> +	if (test_tsk_thread_flag(child, TIF_BLOCKSTEP))
> +		set_task_blockstep(child, false);
> 
>  	/* Always clear TIF_SINGLESTEP... */
>  	clear_tsk_thread_flag(child, TIF_SINGLESTEP);
> -- 
> 1.5.5.1
> 

[PATCH 4/7] ptrace: Partly fix set_task_blockstep()->update_debugctlmsr() logic

[Oleg Nesterov]

Afaics the usage of update_debugctlmsr() and TIF_BLOCKSTEP in
step.c was always very wrong.

1. update_debugctlmsr() was simply unneeded. The child sleeps
   TASK_TRACED, __switch_to_xtra(next_p => child) should notice
   TIF_BLOCKSTEP and set/clear DEBUGCTLMSR_BTF after resume if
   needed.

2. It is wrong. The state of DEBUGCTLMSR_BTF bit in CPU register
   should always match the state of current's TIF_BLOCKSTEP bit.

3. Even get_debugctlmsr() + update_debugctlmsr() itself does not
   look right. Irq can change other bits in MSR_IA32_DEBUGCTLMSR
   register or the caller can be preempted in between.

4. It is not safe to play with TIF_BLOCKSTEP if task != current.
   DEBUGCTLMSR_BTF and TIF_BLOCKSTEP should always match each
   other if the task is running. The tracee is stopped but it
   can be SIGKILL'ed right before set/clear_tsk_thread_flag().

However, now that uprobes uses user_enable_single_step(current)
we can't simply remove update_debugctlmsr(). So this patch adds
the additional "task == current" check and disables irqs to avoid
the race with interrupts/preemption.

Unfortunately this patch doesn't solve the last problem, we need
another fix. Probably we should teach ptrace_stop() to set/clear
single/block stepping after resume.

And afaics there is yet another problem: perf can play with
MSR_IA32_DEBUGCTLMSR from nmi, this obviously means that even
__switch_to_xtra() has problems.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/step.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
index 7a51498..f89cdc6 100644
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -161,6 +161,16 @@ static void set_task_blockstep(struct task_struct *task, bool on)
 {
 	unsigned long debugctl;
 
+	/*
+	 * Ensure irq/preemption can't change debugctl in between.
+	 * Note also that both TIF_BLOCKSTEP and debugctl should
+	 * be changed atomically wrt preemption.
+	 * FIXME: this means that set/clear TIF_BLOCKSTEP is simply
+	 * wrong if task != current, SIGKILL can wakeup the stopped
+	 * tracee and set/clear can play with the running task, this
+	 * can confuse the next __switch_to_xtra().
+	 */
+	local_irq_disable();
 	debugctl = get_debugctlmsr();
 	if (on) {
 		debugctl |= DEBUGCTLMSR_BTF;
@@ -169,7 +179,9 @@ static void set_task_blockstep(struct task_struct *task, bool on)
 		debugctl &= ~DEBUGCTLMSR_BTF;
 		clear_tsk_thread_flag(task, TIF_BLOCKSTEP);
 	}
-	update_debugctlmsr(debugctl);
+	if (task == current)
+		update_debugctlmsr(debugctl);
+	local_irq_enable();
 }
 
 /*
-- 
1.5.5.1

Re: [PATCH 4/7] ptrace: Partly fix set_task_blockstep()->update_debugctlmsr() logic

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-03 17:26:09]:

> Afaics the usage of update_debugctlmsr() and TIF_BLOCKSTEP in
> step.c was always very wrong.
> 
> 1. update_debugctlmsr() was simply unneeded. The child sleeps
>    TASK_TRACED, __switch_to_xtra(next_p => child) should notice
>    TIF_BLOCKSTEP and set/clear DEBUGCTLMSR_BTF after resume if
>    needed.
> 
> 2. It is wrong. The state of DEBUGCTLMSR_BTF bit in CPU register
>    should always match the state of current's TIF_BLOCKSTEP bit.
> 
> 3. Even get_debugctlmsr() + update_debugctlmsr() itself does not
>    look right. Irq can change other bits in MSR_IA32_DEBUGCTLMSR
>    register or the caller can be preempted in between.
> 
> 4. It is not safe to play with TIF_BLOCKSTEP if task != current.
>    DEBUGCTLMSR_BTF and TIF_BLOCKSTEP should always match each
>    other if the task is running. The tracee is stopped but it
>    can be SIGKILL'ed right before set/clear_tsk_thread_flag().
> 
> However, now that uprobes uses user_enable_single_step(current)
> we can't simply remove update_debugctlmsr(). So this patch adds
> the additional "task == current" check and disables irqs to avoid
> the race with interrupts/preemption.
> 
> Unfortunately this patch doesn't solve the last problem, we need
> another fix. Probably we should teach ptrace_stop() to set/clear
> single/block stepping after resume.
> 
> And afaics there is yet another problem: perf can play with
> MSR_IA32_DEBUGCTLMSR from nmi, this obviously means that even
> __switch_to_xtra() has problems.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/x86/kernel/step.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
> index 7a51498..f89cdc6 100644
> --- a/arch/x86/kernel/step.c
> +++ b/arch/x86/kernel/step.c
> @@ -161,6 +161,16 @@ static void set_task_blockstep(struct task_struct *task, bool on)
>  {
>  	unsigned long debugctl;
> 
> +	/*
> +	 * Ensure irq/preemption can't change debugctl in between.
> +	 * Note also that both TIF_BLOCKSTEP and debugctl should
> +	 * be changed atomically wrt preemption.
> +	 * FIXME: this means that set/clear TIF_BLOCKSTEP is simply
> +	 * wrong if task != current, SIGKILL can wakeup the stopped
> +	 * tracee and set/clear can play with the running task, this
> +	 * can confuse the next __switch_to_xtra().
> +	 */
> +	local_irq_disable();
>  	debugctl = get_debugctlmsr();
>  	if (on) {
>  		debugctl |= DEBUGCTLMSR_BTF;
> @@ -169,7 +179,9 @@ static void set_task_blockstep(struct task_struct *task, bool on)
>  		debugctl &= ~DEBUGCTLMSR_BTF;
>  		clear_tsk_thread_flag(task, TIF_BLOCKSTEP);
>  	}
> -	update_debugctlmsr(debugctl);
> +	if (task == current)
> +		update_debugctlmsr(debugctl);
> +	local_irq_enable();
>  }
> 
>  /*
> 

The changes look simple and neat. But I would prefer somebody with
better x86 knowledgde comment on this.

-- 
Thanks and Regards
Srikar

Re: [PATCH 4/7] ptrace: Partly fix set_task_blockstep()->update_debugctlmsr() logic

[Sebastian Andrzej Siewior]

* Oleg Nesterov | 2012-09-03 17:26:09 [+0200]:

>Afaics the usage of update_debugctlmsr() and TIF_BLOCKSTEP in
>step.c was always very wrong.
>
>1. update_debugctlmsr() was simply unneeded. The child sleeps
>   TASK_TRACED, __switch_to_xtra(next_p => child) should notice
>   TIF_BLOCKSTEP and set/clear DEBUGCTLMSR_BTF after resume if
>   needed.
>
>2. It is wrong. The state of DEBUGCTLMSR_BTF bit in CPU register
>   should always match the state of current's TIF_BLOCKSTEP bit.
>
>3. Even get_debugctlmsr() + update_debugctlmsr() itself does not
>   look right. Irq can change other bits in MSR_IA32_DEBUGCTLMSR
>   register or the caller can be preempted in between.

ptrace and uprobe are calling this function from process context. As
long as you have here get_cpu() instead of local_irq_disable() you should
be safe here.
The only user that is touching this bits in irq context is perf. perf
uses raw_local_irqsave() (raw_* most likely due to -RT). I have no idea
what you can against NMI unless not touching the register in NMI
context.

Sebastian

Re: [PATCH 4/7] ptrace: Partly fix set_task_blockstep()->update_debugctlmsr() logic

[Peter Zijlstra]

On Mon, 2012-09-10 at 18:57 +0200, Sebastian Andrzej Siewior wrote:
> The only user that is touching this bits in irq context is perf. perf
> uses raw_local_irqsave() (raw_* most likely due to -RT).

# git grep raw_local_irq arch/x86/kernel/cpu/perf_* kernel/events/ | wc -l
0

I think you're confusing raw_spin_lock_irq{,save}() with
raw_local_irq{,save}(), those two are not the same.

raw_spin_lock* is a lock that is always a spinlock -- unlike spin_lock,
which can be a PI-mutex (PREEMPT_RT=y).

raw_local_irq* is like arch_spin_lock* and avoids things like tracing
and lockdep and should not be used unless you really know wth you're
doing, and then still not ;-)

>  I have no idea
> what you can against NMI unless not touching the register in NMI
> context. 

I think perf will actually touch this from NMI context under 'rare'
conditions, in particular:

  x86_pmu_handle_irq() (NMI handler)
    x86_pmu_stop()
      x86_pmu.disable -> intel_pmu_disable_event()
        intel_pmu_lbr_disable()
          __intel_pmu_lbr_disable()
            wrmsrl(MSR_IA32_DEBUGCTLMSR,... );

I suppose I should look into it not doing that...

Re: [PATCH 4/7] ptrace: Partly fix set_task_blockstep()->update_debugctlmsr() logic

[Oleg Nesterov]

Sebastian, I am replying to my message because I removed your email
by mistake. Fortunately I can see it on marc.info...

> ptrace and uprobe are calling this function from process context. As
> long as you have here get_cpu() instead of local_irq_disable() you should
> be safe here.

local_irq_disable() looks more safe. We can have new users playing
with MSR_IA32_DEBUGCTLMSR from irq.

> perf
> uses raw_local_irqsave() (raw_* most likely due to -RT).

This is completely irrelevant, we alrady discussed this.

> I have no idea
> what you can against NMI unless not touching the register in NMI
> context.

Neither me, and this is documented in the changelog:

	And afaics there is yet another problem: perf can play with
	MSR_IA32_DEBUGCTLMSR from nmi, this obviously means that even
	__switch_to_xtra() has problems.

and please note __switch_to_xtra() above, it has the same problem
by the same reason.

Oleg.

[PATCH 5/7] uprobes: Do not (ab)use TIF_SINGLESTEP/user_*_single_step() for single-stepping

[Oleg Nesterov]

user_enable/disable_single_step() was designed for ptrace, it assumes
a single user and does unnecessary and wrong things for uprobes. For
example:

	- arch_uprobe_enable_step() can't trust TIF_SINGLESTEP, an
	  application itself can set X86_EFLAGS_TF which must be
	  preserved after arch_uprobe_disable_step().

	- we do not want to set TIF_SINGLESTEP/TIF_FORCED_TF in
	  arch_uprobe_enable_step(), this only makes sense for ptrace.

	- otoh we leak TIF_SINGLESTEP if arch_uprobe_disable_step()
	  doesn't do user_disable_single_step(), the application will
	  be killed after the next syscall.

	- arch_uprobe_enable_step() does access_process_vm() we do
	  not need/want.

Change arch_uprobe_enable/disable_step() to set/clear X86_EFLAGS_TF
directly, this is much simpler and more correct. However, we need to
clear TIF_BLOCKSTEP/DEBUGCTLMSR_BTF before executing the probed insn,
add set_task_blockstep(false).

Note: with or without this patch, there is another (hopefully minor)
problem. A probed "pushf" insn can see the wrong X86_EFLAGS_TF set by
uprobes. Perhaps we should change _disable to update the stack, or
teach arch_uprobe_skip_sstep() to emulate this insn.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/include/asm/processor.h |    2 ++
 arch/x86/kernel/step.c           |    2 +-
 arch/x86/kernel/uprobes.c        |   32 ++++++++++++++++++--------------
 3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index d048cad..433d2e5 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -759,6 +759,8 @@ static inline void update_debugctlmsr(unsigned long debugctlmsr)
 	wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctlmsr);
 }
 
+extern void set_task_blockstep(struct task_struct *task, bool on);
+
 /*
  * from system description table in BIOS. Mostly for MCA use, but
  * others may find it useful:
diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
index f89cdc6..cd3b243 100644
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -157,7 +157,7 @@ static int enable_single_step(struct task_struct *child)
 	return 1;
 }
 
-static void set_task_blockstep(struct task_struct *task, bool on)
+void set_task_blockstep(struct task_struct *task, bool on)
 {
 	unsigned long debugctl;
 
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 309a0e0..3b4aae6 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -683,26 +683,30 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
 
 void arch_uprobe_enable_step(struct arch_uprobe *auprobe)
 {
-	struct uprobe_task	*utask		= current->utask;
-	struct arch_uprobe_task	*autask		= &utask->autask;
+	struct task_struct *task = current;
+	struct arch_uprobe_task	*autask	= &task->utask->autask;
+	struct pt_regs *regs = task_pt_regs(task);
 
 	autask->restore_flags = 0;
-	if (!test_tsk_thread_flag(current, TIF_SINGLESTEP) &&
-			!(auprobe->fixups & UPROBE_FIX_SETF))
+	if (!(regs->flags & X86_EFLAGS_TF) &&
+	    !(auprobe->fixups & UPROBE_FIX_SETF))
 		autask->restore_flags |= UPROBE_CLEAR_TF;
-	/*
-	 * The state of TIF_BLOCKSTEP is not saved. With the TF flag set we
-	 * would to examine the opcode and the flags to make it right. Without
-	 * TF block stepping makes no sense.
-	 */
-	user_enable_single_step(current);
+
+	regs->flags |= X86_EFLAGS_TF;
+	if (test_tsk_thread_flag(task, TIF_BLOCKSTEP))
+		set_task_blockstep(task, false);
 }
 
 void arch_uprobe_disable_step(struct arch_uprobe *auprobe)
 {
-	struct uprobe_task *utask		= current->utask;
-	struct arch_uprobe_task	*autask		= &utask->autask;
-
+	struct task_struct *task = current;
+	struct arch_uprobe_task	*autask	= &task->utask->autask;
+	struct pt_regs *regs = task_pt_regs(task);
+	/*
+	 * The state of TIF_BLOCKSTEP was not saved so we can get an extra
+	 * SIGTRAP if we do not clear TF. We need to examine the opcode to
+	 * make it right.
+	 */
 	if (autask->restore_flags & UPROBE_CLEAR_TF)
-		user_disable_single_step(current);
+		regs->flags &= ~X86_EFLAGS_TF;
 }
-- 
1.5.5.1

Re: [PATCH 5/7] uprobes: Do not (ab)use TIF_SINGLESTEP/user_*_single_step() for single-stepping

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-03 17:26:13]:

> user_enable/disable_single_step() was designed for ptrace, it assumes
> a single user and does unnecessary and wrong things for uprobes. For
> example:
> 
> 	- arch_uprobe_enable_step() can't trust TIF_SINGLESTEP, an
> 	  application itself can set X86_EFLAGS_TF which must be
> 	  preserved after arch_uprobe_disable_step().
> 
> 	- we do not want to set TIF_SINGLESTEP/TIF_FORCED_TF in
> 	  arch_uprobe_enable_step(), this only makes sense for ptrace.
> 
> 	- otoh we leak TIF_SINGLESTEP if arch_uprobe_disable_step()
> 	  doesn't do user_disable_single_step(), the application will
> 	  be killed after the next syscall.
> 
> 	- arch_uprobe_enable_step() does access_process_vm() we do
> 	  not need/want.
> 
> Change arch_uprobe_enable/disable_step() to set/clear X86_EFLAGS_TF
> directly, this is much simpler and more correct. However, we need to
> clear TIF_BLOCKSTEP/DEBUGCTLMSR_BTF before executing the probed insn,
> add set_task_blockstep(false).
> 
> Note: with or without this patch, there is another (hopefully minor)
> problem. A probed "pushf" insn can see the wrong X86_EFLAGS_TF set by
> uprobes. Perhaps we should change _disable to update the stack, or
> teach arch_uprobe_skip_sstep() to emulate this insn.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  arch/x86/include/asm/processor.h |    2 ++
>  arch/x86/kernel/step.c           |    2 +-
>  arch/x86/kernel/uprobes.c        |   32 ++++++++++++++++++--------------
>  3 files changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index d048cad..433d2e5 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -759,6 +759,8 @@ static inline void update_debugctlmsr(unsigned long debugctlmsr)
>  	wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctlmsr);
>  }
> 
> +extern void set_task_blockstep(struct task_struct *task, bool on);
> +
>  /*
>   * from system description table in BIOS. Mostly for MCA use, but
>   * others may find it useful:
> diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
> index f89cdc6..cd3b243 100644
> --- a/arch/x86/kernel/step.c
> +++ b/arch/x86/kernel/step.c
> @@ -157,7 +157,7 @@ static int enable_single_step(struct task_struct *child)
>  	return 1;
>  }
> 
> -static void set_task_blockstep(struct task_struct *task, bool on)
> +void set_task_blockstep(struct task_struct *task, bool on)
>  {
>  	unsigned long debugctl;
> 
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 309a0e0..3b4aae6 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -683,26 +683,30 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
> 
>  void arch_uprobe_enable_step(struct arch_uprobe *auprobe)
>  {
> -	struct uprobe_task	*utask		= current->utask;
> -	struct arch_uprobe_task	*autask		= &utask->autask;
> +	struct task_struct *task = current;

Any particular reason to use task instead of current?

> +	struct arch_uprobe_task	*autask	= &task->utask->autask;
> +	struct pt_regs *regs = task_pt_regs(task);
> 
>  	autask->restore_flags = 0;
> -	if (!test_tsk_thread_flag(current, TIF_SINGLESTEP) &&
> -			!(auprobe->fixups & UPROBE_FIX_SETF))
> +	if (!(regs->flags & X86_EFLAGS_TF) &&
> +	    !(auprobe->fixups & UPROBE_FIX_SETF))
>  		autask->restore_flags |= UPROBE_CLEAR_TF;
> -	/*
> -	 * The state of TIF_BLOCKSTEP is not saved. With the TF flag set we
> -	 * would to examine the opcode and the flags to make it right. Without
> -	 * TF block stepping makes no sense.
> -	 */
> -	user_enable_single_step(current);
> +
> +	regs->flags |= X86_EFLAGS_TF;
> +	if (test_tsk_thread_flag(task, TIF_BLOCKSTEP))
> +		set_task_blockstep(task, false);
>  }
> 
>  void arch_uprobe_disable_step(struct arch_uprobe *auprobe)
>  {
> -	struct uprobe_task *utask		= current->utask;
> -	struct arch_uprobe_task	*autask		= &utask->autask;
> -
> +	struct task_struct *task = current;
> +	struct arch_uprobe_task	*autask	= &task->utask->autask;
> +	struct pt_regs *regs = task_pt_regs(task);
> +	/*
> +	 * The state of TIF_BLOCKSTEP was not saved so we can get an extra
> +	 * SIGTRAP if we do not clear TF. We need to examine the opcode to
> +	 * make it right.
> +	 */
>  	if (autask->restore_flags & UPROBE_CLEAR_TF)
> -		user_disable_single_step(current);
> +		regs->flags &= ~X86_EFLAGS_TF;
>  }
> -- 
> 1.5.5.1
> 

Re: [PATCH 5/7] uprobes: Do not (ab)use TIF_SINGLESTEP/user_*_single_step() for single-stepping

[Oleg Nesterov]

On 09/07, Srikar Dronamraju wrote:
>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thanks!

(and thanks, I'll fix the typo in 2/7 you pointed out)

> >  void arch_uprobe_enable_step(struct arch_uprobe *auprobe)
> >  {
> > -	struct uprobe_task	*utask		= current->utask;
> > -	struct arch_uprobe_task	*autask		= &utask->autask;
> > +	struct task_struct *task = current;
>
> Any particular reason to use task instead of current?

No particular reason, and I think in this case asm will be the same.

Please let me know if you prefer to remove this variable, I'll redo
this patch.

Oleg.

Re: [PATCH 5/7] uprobes: Do not (ab)use TIF_SINGLESTEP/user_*_single_step() for single-stepping

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-07 17:50:57]:

> On 09/07, Srikar Dronamraju wrote:
> >
> > Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> 
> Thanks!
> 
> (and thanks, I'll fix the typo in 2/7 you pointed out)
> 
> > >  void arch_uprobe_enable_step(struct arch_uprobe *auprobe)
> > >  {
> > > -	struct uprobe_task	*utask		= current->utask;
> > > -	struct arch_uprobe_task	*autask		= &utask->autask;
> > > +	struct task_struct *task = current;
> >
> > Any particular reason to use task instead of current?
> 
> No particular reason, and I think in this case asm will be the same.
> 
> Please let me know if you prefer to remove this variable, I'll redo
> this patch.

No, the patch is fine.

I have no problem with the additional variable, I was just curious if
you had any other plans that needed that a exta variable that alls.

-- 
thanks and regards
Srikar

[PATCH 6/7] uprobes: Xol should send SIGTRAP if X86_EFLAGS_TF was set

[Oleg Nesterov]

arch_uprobe_disable_step() correctly preserves X86_EFLAGS_TF and
returns to user-mode. But this means the application gets SIGTRAP
only after the next insn.

This means that UPROBE_CLEAR_TF logic is not really right. _enable
should only record the state of X86_EFLAGS_TF, and _disable should
check it separately from UPROBE_FIX_SETF.

Remove arch_uprobe_task->restore_flags, add ->saved_tf instead, and
change enable/disable accordingly.

arch_uprobe_skip_sstep() logic has the same problem, change it to
check X86_EFLAGS_TF and send SIGTRAP as well. We will cleanup this
all after we fold enable/disable_step into pre/post_hol hooks.

Note: send_sig(SIGTRAP) is not actually right, we need send_sigtrap().
But this needs more changes, handle_swbp() does the same and this is
equally wrong.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/include/asm/uprobes.h |    3 +--
 arch/x86/kernel/uprobes.c      |   19 +++++++++++++------
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index cee5862..d561ff5 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -46,8 +46,7 @@ struct arch_uprobe_task {
 #ifdef CONFIG_X86_64
 	unsigned long			saved_scratch_register;
 #endif
-#define UPROBE_CLEAR_TF			(1 << 0)
-	unsigned int			restore_flags;
+	unsigned int			saved_tf;
 };
 
 extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 3b4aae6..7e993d1 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -653,7 +653,7 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
  * Skip these instructions as per the currently known x86 ISA.
  * 0x66* { 0x90 | 0x0f 0x1f | 0x0f 0x19 | 0x87 0xc0 }
  */
-bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
+static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
 	int i;
 
@@ -681,16 +681,21 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	return false;
 }
 
+bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	bool ret = __skip_sstep(auprobe, regs);
+	if (ret && (regs->flags & X86_EFLAGS_TF))
+		send_sig(SIGTRAP, current, 0);
+	return ret;
+}
+
 void arch_uprobe_enable_step(struct arch_uprobe *auprobe)
 {
 	struct task_struct *task = current;
 	struct arch_uprobe_task	*autask	= &task->utask->autask;
 	struct pt_regs *regs = task_pt_regs(task);
 
-	autask->restore_flags = 0;
-	if (!(regs->flags & X86_EFLAGS_TF) &&
-	    !(auprobe->fixups & UPROBE_FIX_SETF))
-		autask->restore_flags |= UPROBE_CLEAR_TF;
+	autask->saved_tf = !!(regs->flags & X86_EFLAGS_TF);
 
 	regs->flags |= X86_EFLAGS_TF;
 	if (test_tsk_thread_flag(task, TIF_BLOCKSTEP))
@@ -707,6 +712,8 @@ void arch_uprobe_disable_step(struct arch_uprobe *auprobe)
 	 * SIGTRAP if we do not clear TF. We need to examine the opcode to
 	 * make it right.
 	 */
-	if (autask->restore_flags & UPROBE_CLEAR_TF)
+	if (autask->saved_tf)
+		send_sig(SIGTRAP, task, 0);
+	else if (!(auprobe->fixups & UPROBE_FIX_SETF))
 		regs->flags &= ~X86_EFLAGS_TF;
 }
-- 
1.5.5.1

Re: [PATCH 6/7] uprobes: Xol should send SIGTRAP if X86_EFLAGS_TF was set

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-03 17:26:16]:

> arch_uprobe_disable_step() correctly preserves X86_EFLAGS_TF and
> returns to user-mode. But this means the application gets SIGTRAP
> only after the next insn.
> 

Agree.

> This means that UPROBE_CLEAR_TF logic is not really right. _enable
> should only record the state of X86_EFLAGS_TF, and _disable should
> check it separately from UPROBE_FIX_SETF.
> 
> Remove arch_uprobe_task->restore_flags, add ->saved_tf instead, and
> change enable/disable accordingly.
> 
> arch_uprobe_skip_sstep() logic has the same problem, change it to
> check X86_EFLAGS_TF and send SIGTRAP as well. We will cleanup this
> all after we fold enable/disable_step into pre/post_hol hooks.
> 
> Note: send_sig(SIGTRAP) is not actually right, we need send_sigtrap().

Are you pointing to the info field not being filled? or are there other
problems?

> But this needs more changes, handle_swbp() does the same and this is
> equally wrong.

send_sigtrap() is arch specific and defined for only few archs.
we would have to force. But these are not related to the patch.

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>


Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  arch/x86/include/asm/uprobes.h |    3 +--
>  arch/x86/kernel/uprobes.c      |   19 +++++++++++++------
>  2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index cee5862..d561ff5 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -46,8 +46,7 @@ struct arch_uprobe_task {
>  #ifdef CONFIG_X86_64
>  	unsigned long			saved_scratch_register;
>  #endif
> -#define UPROBE_CLEAR_TF			(1 << 0)
> -	unsigned int			restore_flags;
> +	unsigned int			saved_tf;
>  };
> 
>  extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 3b4aae6..7e993d1 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -653,7 +653,7 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>   * Skip these instructions as per the currently known x86 ISA.
>   * 0x66* { 0x90 | 0x0f 0x1f | 0x0f 0x19 | 0x87 0xc0 }
>   */
> -bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  {
>  	int i;
> 
> @@ -681,16 +681,21 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  	return false;
>  }
> 
> +bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	bool ret = __skip_sstep(auprobe, regs);

Should we add a comment here saying ptrace or some debugger wants to
trace this instruction and we singlestepped and hence we want to inform
them now and otherwise they would only know one instruction later?

> +	if (ret && (regs->flags & X86_EFLAGS_TF))
> +		send_sig(SIGTRAP, current, 0);
> +	return ret;
> +}
> +
>  void arch_uprobe_enable_step(struct arch_uprobe *auprobe)
>  {
>  	struct task_struct *task = current;
>  	struct arch_uprobe_task	*autask	= &task->utask->autask;
>  	struct pt_regs *regs = task_pt_regs(task);
> 
> -	autask->restore_flags = 0;
> -	if (!(regs->flags & X86_EFLAGS_TF) &&
> -	    !(auprobe->fixups & UPROBE_FIX_SETF))
> -		autask->restore_flags |= UPROBE_CLEAR_TF;
> +	autask->saved_tf = !!(regs->flags & X86_EFLAGS_TF);
> 
>  	regs->flags |= X86_EFLAGS_TF;
>  	if (test_tsk_thread_flag(task, TIF_BLOCKSTEP))
> @@ -707,6 +712,8 @@ void arch_uprobe_disable_step(struct arch_uprobe *auprobe)
>  	 * SIGTRAP if we do not clear TF. We need to examine the opcode to
>  	 * make it right.
>  	 */
> -	if (autask->restore_flags & UPROBE_CLEAR_TF)
> +	if (autask->saved_tf)
> +		send_sig(SIGTRAP, task, 0);
> +	else if (!(auprobe->fixups & UPROBE_FIX_SETF))
>  		regs->flags &= ~X86_EFLAGS_TF;
>  }
> -- 
> 1.5.5.1
> 

Re: [PATCH 6/7] uprobes: Xol should send SIGTRAP if X86_EFLAGS_TF was set

[Oleg Nesterov]

On 09/12, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-09-03 17:26:16]:
>
> > Note: send_sig(SIGTRAP) is not actually right, we need send_sigtrap().
>
> Are you pointing to the info field not being filled? or are there other
> problems?

Yes.

Also, I am not sure but I feel we should move this into generic code
somehow. We will see.

> > But this needs more changes, handle_swbp() does the same and this is
> > equally wrong.
>
> send_sigtrap() is arch specific and defined for only few archs.
> we would have to force.

Yes, but we have user_single_step_siginfo(), probably this is enough.

> But these are not related to the patch.

Agreed, lets do this later.

> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thanks!

Oleg.

[PATCH 7/7] uprobes: Make arch_uprobe_task->saved_trap_nr "unsigned int"

[Oleg Nesterov]

Make arch_uprobe_task->saved_trap_nr "unsigned int" and move it down
after ->saved_scratch_register, this changes sizeof() from 24 to 16.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/include/asm/uprobes.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index d561ff5..8ff8be7 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -42,10 +42,10 @@ struct arch_uprobe {
 };
 
 struct arch_uprobe_task {
-	unsigned long			saved_trap_nr;
 #ifdef CONFIG_X86_64
 	unsigned long			saved_scratch_register;
 #endif
+	unsigned int			saved_trap_nr;
 	unsigned int			saved_tf;
 };
 
-- 
1.5.5.1

Re: [PATCH 7/7] uprobes: Make arch_uprobe_task->saved_trap_nr "unsigned int"

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-03 17:26:20]:

> Make arch_uprobe_task->saved_trap_nr "unsigned int" and move it down
> after ->saved_scratch_register, this changes sizeof() from 24 to 16.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>


Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  arch/x86/include/asm/uprobes.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index d561ff5..8ff8be7 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -42,10 +42,10 @@ struct arch_uprobe {
>  };
> 
>  struct arch_uprobe_task {
> -	unsigned long			saved_trap_nr;
>  #ifdef CONFIG_X86_64
>  	unsigned long			saved_scratch_register;
>  #endif
> +	unsigned int			saved_trap_nr;
>  	unsigned int			saved_tf;
>  };
> 
> -- 
> 1.5.5.1
> 

Re: [PATCH 0/7] uprobes: single-step fixes

[Oleg Nesterov]

On 09/03, Oleg Nesterov wrote:
>
> Sebastian, I changed your patches a bit:
>
> 	1/7:
>
> 		- Change the subject and update the changelog. In particular,
> 		  s/utrace/uprobes/. I am wondering where this typo came from ;)

Hmm. I just noticed this patch is buggy. arch_uprobe_disable_step(&uprobe->arch)
is not safe after put_uprobe().

Srikar, I fixed this in my tree with the following change,

	--- kernel/events/uprobes.c~	2012-09-02 16:52:54.000000000 +0200
	+++ kernel/events/uprobes.c	2012-09-08 18:56:44.000000000 +0200
	@@ -1536,10 +1536,10 @@ static void handle_singlestep(struct upr
		else
			WARN_ON_ONCE(1);
	 
	+	arch_uprobe_disable_step(&uprobe->arch);
		put_uprobe(uprobe);
		utask->active_uprobe = NULL;
		utask->state = UTASK_RUNNING;
	-	arch_uprobe_disable_step(&uprobe->arch);
		xol_free_insn_slot(current);
	 
		spin_lock_irq(&current->sighand->siglock);

I hope your ack is still valid.

And this also allows us to rely on utask->state in disable_step(), see
the new 8/7 I'll send in a minute. I was going to fix this later, but
I just realized that "disable if trapped" is more buggy than I thought.

Assuming that you are agree with 6 and 8. I'd prefer the new one as a
separate change, but if you prefer to join them please let me know.

Oleg.

Re: [PATCH 0/7] uprobes: single-step fixes

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-08 19:06:01]:

> On 09/03, Oleg Nesterov wrote:
> >
> > Sebastian, I changed your patches a bit:
> >
> > 	1/7:
> >
> > 		- Change the subject and update the changelog. In particular,
> > 		  s/utrace/uprobes/. I am wondering where this typo came from ;)
> 
> Hmm. I just noticed this patch is buggy. arch_uprobe_disable_step(&uprobe->arch)
> is not safe after put_uprobe().
> 
> Srikar, I fixed this in my tree with the following change,
> 
> 	--- kernel/events/uprobes.c~	2012-09-02 16:52:54.000000000 +0200
> 	+++ kernel/events/uprobes.c	2012-09-08 18:56:44.000000000 +0200
> 	@@ -1536,10 +1536,10 @@ static void handle_singlestep(struct upr
> 		else
> 			WARN_ON_ONCE(1);
> 	 
> 	+	arch_uprobe_disable_step(&uprobe->arch);
> 		put_uprobe(uprobe);
> 		utask->active_uprobe = NULL;
> 		utask->state = UTASK_RUNNING;
> 	-	arch_uprobe_disable_step(&uprobe->arch);
> 		xol_free_insn_slot(current);
> 	 
> 		spin_lock_irq(&current->sighand->siglock);
> 
> I hope your ack is still valid.
> 

Yes, Please merge this into 1/7 patch.

-- 
Thanks and Regards
Srikar

[PATCH 8/7] uprobes: Fix arch_uprobe_disable_step() && UTASK_SSTEP_TRAPPED interaction

[Oleg Nesterov]

arch_uprobe_disable_step() should also take UTASK_SSTEP_TRAPPED into
account. In this case the probed insn was not executed, we need to
clear X86_EFLAGS_TF if it was set by us and that is all.

Again, this code will look more clean when we move it into
arch_uprobe_post_xol() and arch_uprobe_abort_xol().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/uprobes.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 7e993d1..9538f00 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -706,14 +706,20 @@ void arch_uprobe_disable_step(struct arch_uprobe *auprobe)
 {
 	struct task_struct *task = current;
 	struct arch_uprobe_task	*autask	= &task->utask->autask;
+	bool trapped = (task->utask->state == UTASK_SSTEP_TRAPPED);
 	struct pt_regs *regs = task_pt_regs(task);
 	/*
 	 * The state of TIF_BLOCKSTEP was not saved so we can get an extra
 	 * SIGTRAP if we do not clear TF. We need to examine the opcode to
 	 * make it right.
 	 */
-	if (autask->saved_tf)
-		send_sig(SIGTRAP, task, 0);
-	else if (!(auprobe->fixups & UPROBE_FIX_SETF))
-		regs->flags &= ~X86_EFLAGS_TF;
+	if (unlikely(trapped)) {
+		if (!autask->saved_tf)
+			regs->flags &= ~X86_EFLAGS_TF;
+	} else {
+		if (autask->saved_tf)
+			send_sig(SIGTRAP, task, 0);
+		else if (!(auprobe->fixups & UPROBE_FIX_SETF))
+			regs->flags &= ~X86_EFLAGS_TF;
+	}
 }
-- 
1.5.5.1

Re: [PATCH 8/7] uprobes: Fix arch_uprobe_disable_step() && UTASK_SSTEP_TRAPPED interaction

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-08 19:06:23]:

> arch_uprobe_disable_step() should also take UTASK_SSTEP_TRAPPED into
> account. In this case the probed insn was not executed, we need to
> clear X86_EFLAGS_TF if it was set by us and that is all.
> 
> Again, this code will look more clean when we move it into
> arch_uprobe_post_xol() and arch_uprobe_abort_xol().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>



Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

note:
I also think this should be merged into 6/7 patch if possible

-- 
thanks and regards
Srikar

> ---
>  arch/x86/kernel/uprobes.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 7e993d1..9538f00 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -706,14 +706,20 @@ void arch_uprobe_disable_step(struct arch_uprobe *auprobe)
>  {
>  	struct task_struct *task = current;
>  	struct arch_uprobe_task	*autask	= &task->utask->autask;
> +	bool trapped = (task->utask->state == UTASK_SSTEP_TRAPPED);
>  	struct pt_regs *regs = task_pt_regs(task);
>  	/*
>  	 * The state of TIF_BLOCKSTEP was not saved so we can get an extra
>  	 * SIGTRAP if we do not clear TF. We need to examine the opcode to
>  	 * make it right.
>  	 */
> -	if (autask->saved_tf)
> -		send_sig(SIGTRAP, task, 0);
> -	else if (!(auprobe->fixups & UPROBE_FIX_SETF))
> -		regs->flags &= ~X86_EFLAGS_TF;
> +	if (unlikely(trapped)) {
> +		if (!autask->saved_tf)
> +			regs->flags &= ~X86_EFLAGS_TF;
> +	} else {
> +		if (autask->saved_tf)
> +			send_sig(SIGTRAP, task, 0);
> +		else if (!(auprobe->fixups & UPROBE_FIX_SETF))
> +			regs->flags &= ~X86_EFLAGS_TF;
> +	}
>  }
> -- 
> 1.5.5.1
> 
> 

Re: [PATCH 0/7] uprobes: single-step fixes

[Sebastian Andrzej Siewior]

On 09/03/2012 05:25 PM, Oleg Nesterov wrote:
> Hello.

Hi Oleg,

> Sebastian, I changed your patches a bit:

<snip>

> please let me know if you disagree.

Thank you very much. I'm fine with those. Just tested, looks good so
far.

>
> Oleg.
>

Sebastian