[PATCH 0/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks

[PATCH 0/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks

[Oleg Nesterov]

Hello.

arch_uprobe_enable/disable_step() were only needed to not break
the pending powerpc port. They buy nothing and they are simply
wrong. Uprobes should not use ptrace helpers for the stepping.

Now that powepc port was merged we should kill them asap, before
arm port.

1/4 is minor/offtopic cleanup. powerpc changes were not tested
at all and thus need the review from Ananth.

Oleg.

[PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()

[Oleg Nesterov]

Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.

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

diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index a2dc757..3b99711 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
 
 void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
 {
-	if (thread_info_flags & _TIF_UPROBE) {
-		clear_thread_flag(TIF_UPROBE);
+	if (thread_info_flags & _TIF_UPROBE)
 		uprobe_notify_resume(regs);
-	}
 
 	if (thread_info_flags & _TIF_SIGPENDING)
 		do_signal(regs);
-- 
1.5.5.1

[PATCH 2/4] uprobes/powerpc: Do not use arch_uprobe_*_step() helpers

[Oleg Nesterov]

No functional changes.

powerpc is the only user of arch_uprobe_enable/disable_step() helpers,
but they should die. They can not be used correctly, every arch needs
its own implementation (like x86 does). And they do not really help
even as initial-and-almost-working code, arch_uprobe_*_xol() hooks can
easily use user_enable/disable_single_step() directly.

Change arch_uprobe_*_step() to do nothing, and convert powerpc to use
ptrace helpers. This is equally wrong, powerpc needs the arch-specific
fixes.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/powerpc/kernel/uprobes.c |    6 ++++++
 kernel/events/uprobes.c       |    2 --
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index d2d46d1..bc77834 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -64,6 +64,8 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	autask->saved_trap_nr = current->thread.trap_nr;
 	current->thread.trap_nr = UPROBE_TRAP_NR;
 	regs->nip = current->utask->xol_vaddr;
+
+	user_enable_single_step(current);
 	return 0;
 }
 
@@ -119,6 +121,8 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	 * to be executed.
 	 */
 	regs->nip = utask->vaddr + MAX_UINSN_BYTES;
+
+	user_disable_single_step(current);
 	return 0;
 }
 
@@ -162,6 +166,8 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 
 	current->thread.trap_nr = utask->autask.saved_trap_nr;
 	instruction_pointer_set(regs, utask->vaddr);
+
+	user_disable_single_step(current);
 }
 
 /*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 672227a..916391e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1432,12 +1432,10 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 
 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);
 }
 
 /*
-- 
1.5.5.1

[PATCH 3/4] uprobes/x86: Cleanup the single-stepping code

[Oleg Nesterov]

No functional changes.

Now that default arch_uprobe_enable/disable_step() helpers do nothing,
x86 has no reason to reimplement them. Change arch_uprobe_*_xol() hooks
to do the necessary work and remove the x86-specific hooks.

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

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index aafa555..c71025b 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -478,6 +478,11 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	regs->ip = current->utask->xol_vaddr;
 	pre_xol_rip_insn(auprobe, regs, autask);
 
+	autask->saved_tf = !!(regs->flags & X86_EFLAGS_TF);
+	regs->flags |= X86_EFLAGS_TF;
+	if (test_tsk_thread_flag(current, TIF_BLOCKSTEP))
+		set_task_blockstep(current, false);
+
 	return 0;
 }
 
@@ -603,6 +608,16 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	if (auprobe->fixups & UPROBE_FIX_CALL)
 		result = adjust_ret_addr(regs->sp, correction);
 
+	/*
+	 * arch_uprobe_pre_xol() doesn't save the state of TIF_BLOCKSTEP
+	 * so we can get an extra SIGTRAP if we do not clear TF. We need
+	 * to examine the opcode to make it right.
+	 */
+	if (utask->autask.saved_tf)
+		send_sig(SIGTRAP, current, 0);
+	else if (!(auprobe->fixups & UPROBE_FIX_SETF))
+		regs->flags &= ~X86_EFLAGS_TF;
+
 	return result;
 }
 
@@ -647,6 +662,10 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	current->thread.trap_nr = utask->autask.saved_trap_nr;
 	handle_riprel_post_xol(auprobe, regs, NULL);
 	instruction_pointer_set(regs, utask->vaddr);
+
+	/* clear TF if it was set by us in arch_uprobe_pre_xol() */
+	if (!utask->autask.saved_tf)
+		regs->flags &= ~X86_EFLAGS_TF;
 }
 
 /*
@@ -676,38 +695,3 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
 		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->saved_tf = !!(regs->flags & X86_EFLAGS_TF);
-
-	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 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 (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

[PATCH 4/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks

[Oleg Nesterov]

Kill arch_uprobe_enable/disable_step() hooks, they do nothing and
nobody needs them.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/uprobes.h |    2 --
 kernel/events/uprobes.c |   10 ----------
 2 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 2459457..2615c4d 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -101,8 +101,6 @@ 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 916391e..02d7c5f 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1430,14 +1430,6 @@ 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)
-{
-}
-
-void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)
-{
-}
-
 /*
  * Run handler and ask thread to singlestep.
  * Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
@@ -1491,7 +1483,6 @@ static void handle_swbp(struct pt_regs *regs)
 		goto out;
 
 	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
-		arch_uprobe_enable_step(&uprobe->arch);
 		utask->active_uprobe = uprobe;
 		utask->state = UTASK_SSTEP;
 		return;
@@ -1523,7 +1514,6 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
 	else
 		WARN_ON_ONCE(1);
 
-	arch_uprobe_disable_step(&uprobe->arch);
 	put_uprobe(uprobe);
 	utask->active_uprobe = NULL;
 	utask->state = UTASK_RUNNING;
-- 
1.5.5.1

Re: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()

[Ananth N Mavinakayanahalli]

On Sun, Oct 28, 2012 at 06:39:25PM +0100, Oleg Nesterov wrote:

Hi Oleg,

> Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/powerpc/kernel/signal.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> index a2dc757..3b99711 100644
> --- a/arch/powerpc/kernel/signal.c
> +++ b/arch/powerpc/kernel/signal.c
> @@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
> 
>  void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)

But this _is_ do_notify_resume()... I don't see this flag cleared
anywhere else.

Did you have something else in mind?

Ananth

>  {
> -	if (thread_info_flags & _TIF_UPROBE) {
> -		clear_thread_flag(TIF_UPROBE);
> +	if (thread_info_flags & _TIF_UPROBE)
>  		uprobe_notify_resume(regs);
> -	}
> 
>  	if (thread_info_flags & _TIF_SIGPENDING)
>  		do_signal(regs);
> -- 
> 1.5.5.1

Re: [PATCH 2/4] uprobes/powerpc: Do not use arch_uprobe_*_step() helpers

[Ananth N Mavinakayanahalli]

On Sun, Oct 28, 2012 at 06:39:28PM +0100, Oleg Nesterov wrote:
> No functional changes.
> 
> powerpc is the only user of arch_uprobe_enable/disable_step() helpers,
> but they should die. They can not be used correctly, every arch needs
> its own implementation (like x86 does). And they do not really help
> even as initial-and-almost-working code, arch_uprobe_*_xol() hooks can
> easily use user_enable/disable_single_step() directly.
> 
> Change arch_uprobe_*_step() to do nothing, and convert powerpc to use
> ptrace helpers. This is equally wrong, powerpc needs the arch-specific
> fixes.

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

I will send a powerpc patch to directly use the MSR bits for stepping.

Ananth

Re: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()

[Srikar Dronamraju]

* Ananth N Mavinakayanahalli <ananth@in.ibm.com> [2012-10-29 10:57:07]:

> On Sun, Oct 28, 2012 at 06:39:25PM +0100, Oleg Nesterov wrote:
> 
> Hi Oleg,
> 
> > Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
> > 
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > ---
> >  arch/powerpc/kernel/signal.c |    4 +---
> >  1 files changed, 1 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> > index a2dc757..3b99711 100644
> > --- a/arch/powerpc/kernel/signal.c
> > +++ b/arch/powerpc/kernel/signal.c
> > @@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
> > 
> >  void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
> 
> But this _is_ do_notify_resume()... I don't see this flag cleared
> anywhere else.
> 
> Did you have something else in mind?

uprobe_notify_resume() itself clears TIF_UPROBES.

This change is already part of -tip but not mainline.
Hence you might have missed it.

> >  {
> > -	if (thread_info_flags & _TIF_UPROBE) {
> > -		clear_thread_flag(TIF_UPROBE);
> > +	if (thread_info_flags & _TIF_UPROBE)
> >  		uprobe_notify_resume(regs);
> > -	}
> > 
> >  	if (thread_info_flags & _TIF_SIGPENDING)
> >  		do_signal(regs);

Re: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()

[Oleg Nesterov]

On 10/29, Ananth N Mavinakayanahalli wrote:
>
> On Sun, Oct 28, 2012 at 06:39:25PM +0100, Oleg Nesterov wrote:
>
> Hi Oleg,
>
> > Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > ---
> >  arch/powerpc/kernel/signal.c |    4 +---
> >  1 files changed, 1 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> > index a2dc757..3b99711 100644
> > --- a/arch/powerpc/kernel/signal.c
> > +++ b/arch/powerpc/kernel/signal.c
> > @@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
> >
> >  void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
>
> But this _is_ do_notify_resume()...

Argh. I'll fix the changelog, see v2 below.

> I don't see this flag cleared
> anywhere else.
>
> Did you have something else in mind?

Sorry for confusion.

As Srikar explained, it is cleared by uprobe_notify_resume(). See db023ea5
"uprobes: Move clear_thread_flag(TIF_UPROBE) to uprobe_notify_resume()", it
is already in Linus's tree.

Oleg.

------------------------------------------------------------------------------
Subject: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()

Cleanup. No need to clear TIF_UPROBE, uprobe_notify_resume() does this.

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

diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index a2dc757..3b99711 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
 
 void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
 {
-	if (thread_info_flags & _TIF_UPROBE) {
-		clear_thread_flag(TIF_UPROBE);
+	if (thread_info_flags & _TIF_UPROBE)
 		uprobe_notify_resume(regs);
-	}
 
 	if (thread_info_flags & _TIF_SIGPENDING)
 		do_signal(regs);
-- 
1.5.5.1

Re: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()

[Ananth N Mavinakayanahalli]

On Mon, Oct 29, 2012 at 01:43:25PM +0100, Oleg Nesterov wrote:
> On 10/29, Ananth N Mavinakayanahalli wrote:
> >
> > On Sun, Oct 28, 2012 at 06:39:25PM +0100, Oleg Nesterov wrote:
> >
> > Hi Oleg,
> >
> > > Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
> > >
> > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > > ---
> > >  arch/powerpc/kernel/signal.c |    4 +---
> > >  1 files changed, 1 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> > > index a2dc757..3b99711 100644
> > > --- a/arch/powerpc/kernel/signal.c
> > > +++ b/arch/powerpc/kernel/signal.c
> > > @@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
> > >
> > >  void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
> >
> > But this _is_ do_notify_resume()...
> 
> Argh. I'll fix the changelog, see v2 below.
> 
> > I don't see this flag cleared
> > anywhere else.
> >
> > Did you have something else in mind?
> 
> Sorry for confusion.
> 
> As Srikar explained, it is cleared by uprobe_notify_resume(). See db023ea5
> "uprobes: Move clear_thread_flag(TIF_UPROBE) to uprobe_notify_resume()", it
> is already in Linus's tree.

I was looking at rc2.. that explains it.

> Oleg.
> 
> ------------------------------------------------------------------------------
> Subject: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()
> 
> Cleanup. No need to clear TIF_UPROBE, uprobe_notify_resume() does this.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Thanks Oleg!

Ananth

Re: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()

[Srikar Dronamraju]

> Subject: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()
> 
> Cleanup. No need to clear TIF_UPROBE, uprobe_notify_resume() does this.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>


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

> ---
>  arch/powerpc/kernel/signal.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> index a2dc757..3b99711 100644
> --- a/arch/powerpc/kernel/signal.c
> +++ b/arch/powerpc/kernel/signal.c
> @@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
> 
>  void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
>  {
> -	if (thread_info_flags & _TIF_UPROBE) {
> -		clear_thread_flag(TIF_UPROBE);
> +	if (thread_info_flags & _TIF_UPROBE)
>  		uprobe_notify_resume(regs);
> -	}
> 
>  	if (thread_info_flags & _TIF_SIGPENDING)
>  		do_signal(regs);
> -- 
> 1.5.5.1
> 
> 

Re: [PATCH 2/4] uprobes/powerpc: Do not use arch_uprobe_*_step() helpers

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-10-28 18:39:28]:

> No functional changes.
> 
> powerpc is the only user of arch_uprobe_enable/disable_step() helpers,
> but they should die. They can not be used correctly, every arch needs
> its own implementation (like x86 does). And they do not really help
> even as initial-and-almost-working code, arch_uprobe_*_xol() hooks can
> easily use user_enable/disable_single_step() directly.
> 
> Change arch_uprobe_*_step() to do nothing, and convert powerpc to use
> ptrace helpers. This is equally wrong, powerpc needs the arch-specific
> fixes.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>


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

> ---
>  arch/powerpc/kernel/uprobes.c |    6 ++++++
>  kernel/events/uprobes.c       |    2 --
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
> index d2d46d1..bc77834 100644
> --- a/arch/powerpc/kernel/uprobes.c
> +++ b/arch/powerpc/kernel/uprobes.c
> @@ -64,6 +64,8 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  	autask->saved_trap_nr = current->thread.trap_nr;
>  	current->thread.trap_nr = UPROBE_TRAP_NR;
>  	regs->nip = current->utask->xol_vaddr;
> +
> +	user_enable_single_step(current);
>  	return 0;
>  }
> 
> @@ -119,6 +121,8 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  	 * to be executed.
>  	 */
>  	regs->nip = utask->vaddr + MAX_UINSN_BYTES;
> +
> +	user_disable_single_step(current);
>  	return 0;
>  }
> 
> @@ -162,6 +166,8 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> 
>  	current->thread.trap_nr = utask->autask.saved_trap_nr;
>  	instruction_pointer_set(regs, utask->vaddr);
> +
> +	user_disable_single_step(current);
>  }
> 
>  /*
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 672227a..916391e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1432,12 +1432,10 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> 
>  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);
>  }
> 
>  /*
> -- 
> 1.5.5.1
> 

Re: [PATCH 4/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-10-28 18:39:36]:

> Kill arch_uprobe_enable/disable_step() hooks, they do nothing and
> nobody needs them.
> 
> 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 |   10 ----------
>  2 files changed, 0 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 2459457..2615c4d 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -101,8 +101,6 @@ 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 916391e..02d7c5f 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1430,14 +1430,6 @@ 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)
> -{
> -}
> -
> -void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)
> -{
> -}
> -
>  /*
>   * Run handler and ask thread to singlestep.
>   * Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
> @@ -1491,7 +1483,6 @@ static void handle_swbp(struct pt_regs *regs)
>  		goto out;
> 
>  	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
> -		arch_uprobe_enable_step(&uprobe->arch);
>  		utask->active_uprobe = uprobe;
>  		utask->state = UTASK_SSTEP;
>  		return;
> @@ -1523,7 +1514,6 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
>  	else
>  		WARN_ON_ONCE(1);
> 
> -	arch_uprobe_disable_step(&uprobe->arch);
>  	put_uprobe(uprobe);
>  	utask->active_uprobe = NULL;
>  	utask->state = UTASK_RUNNING;
> -- 
> 1.5.5.1
> 

Re: [PATCH 3/4] uprobes/x86: Cleanup the single-stepping code

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-10-28 18:39:31]:

> No functional changes.
> 
> Now that default arch_uprobe_enable/disable_step() helpers do nothing,
> x86 has no reason to reimplement them. Change arch_uprobe_*_xol() hooks
> to do the necessary work and remove the x86-specific hooks.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>


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

> ---
>  arch/x86/kernel/uprobes.c |   54 +++++++++++++++-----------------------------
>  1 files changed, 19 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index aafa555..c71025b 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -478,6 +478,11 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  	regs->ip = current->utask->xol_vaddr;
>  	pre_xol_rip_insn(auprobe, regs, autask);
> 
> +	autask->saved_tf = !!(regs->flags & X86_EFLAGS_TF);
> +	regs->flags |= X86_EFLAGS_TF;
> +	if (test_tsk_thread_flag(current, TIF_BLOCKSTEP))
> +		set_task_blockstep(current, false);
> +
>  	return 0;
>  }
> 
> @@ -603,6 +608,16 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  	if (auprobe->fixups & UPROBE_FIX_CALL)
>  		result = adjust_ret_addr(regs->sp, correction);
> 
> +	/*
> +	 * arch_uprobe_pre_xol() doesn't save the state of TIF_BLOCKSTEP
> +	 * so we can get an extra SIGTRAP if we do not clear TF. We need
> +	 * to examine the opcode to make it right.
> +	 */
> +	if (utask->autask.saved_tf)
> +		send_sig(SIGTRAP, current, 0);
> +	else if (!(auprobe->fixups & UPROBE_FIX_SETF))
> +		regs->flags &= ~X86_EFLAGS_TF;
> +
>  	return result;
>  }
> 
> @@ -647,6 +662,10 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  	current->thread.trap_nr = utask->autask.saved_trap_nr;
>  	handle_riprel_post_xol(auprobe, regs, NULL);
>  	instruction_pointer_set(regs, utask->vaddr);
> +
> +	/* clear TF if it was set by us in arch_uprobe_pre_xol() */
> +	if (!utask->autask.saved_tf)
> +		regs->flags &= ~X86_EFLAGS_TF;
>  }
> 
>  /*
> @@ -676,38 +695,3 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  		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->saved_tf = !!(regs->flags & X86_EFLAGS_TF);
> -
> -	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 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 (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
>