uprobes: Kill arch_uprobe_enable/disable_step() hooks

PATCH In review Oleg Nesterov 3 participants 14 messages Oct 28, 2012 – Nov 2, 2012

Subsystems (4)

x86 PowerPC perf events uprobes

lore

Share & tools

lore

Oleg Nesterov

author maintainer

2012-10-28 17:38 UTC #0 lore raw

cover uprobes: Kill arch_uprobe_enable/disable_step() hooks

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.

Oleg Nesterov

author maintainer

2012-10-28 17:38 UTC #1 lore raw

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

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

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/powerpc/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

arch/powerpc/kernel/signal.c +1 -3

index a2dc757..3b99711 100644
@@ -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);

Signature · 1 line

1.5.5.1

Ananth N Mavinakayanahalli

2012-10-29 05:27 UTC #2 lore raw

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

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

Hi Oleg,

Quoted 12 lines

Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
Signed-off-by Oleg Nesterov <[email protected]>
---
 arch/powerpc/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
arch/powerpc/kernel/signal.c +0 -0
index a2dc757..3b99711 100644

arch/powerpc/kernel/signal.c static int do_signal(struct pt_regs *regs) · line 159

Quoted 2 lines

arch/powerpc/kernel/signal.c +0 -0

@@ -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

Quoted 11 lines

arch/powerpc/kernel/signal.c +1 -3

 {
-	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);

Signature · 1 line

1.5.5.1

Srikar Dronamraju

2012-10-29 06:59 UTC #3 lore raw

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

Quoted 17 lines · Ananth N Mavinakayanahalli

On Sun, Oct 28, 2012 at 06:39:25PM +0100, Oleg Nesterov wrote:
Hi Oleg,
Quoted 13 lines
Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
Signed-off-by Oleg Nesterov <[email protected]>
---
 arch/powerpc/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
arch/powerpc/kernel/signal.c +0 -0
index a2dc757..3b99711 100644
@@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
 

Quoted 6 lines

arch/powerpc/kernel/signal.c static int do_signal(struct pt_regs *regs) · line 159
Quoted 1 line
arch/powerpc/kernel/signal.c +0 -0
 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.

Quoted 9 lines

arch/powerpc/kernel/signal.c +1 -3

 {
-	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);

Oleg Nesterov

author maintainer

2012-10-29 12:43 UTC #4 lore raw

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

In reply to Ananth N Mavinakayanahalli

Quoted 18 lines · Ananth N Mavinakayanahalli

On Sun, Oct 28, 2012 at 06:39:25PM +0100, Oleg Nesterov wrote:
Hi Oleg,
Quoted 13 lines
Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
Signed-off-by Oleg Nesterov <[email protected]>
---
 arch/powerpc/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
arch/powerpc/kernel/signal.c +0 -0
index a2dc757..3b99711 100644
@@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
 

Quoted 3 lines

arch/powerpc/kernel/signal.c static int do_signal(struct pt_regs *regs) · line 159
Quoted 1 line
arch/powerpc/kernel/signal.c +0 -0
 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.

Quoted 4 lines

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 <[email protected]>

---
 arch/powerpc/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

arch/powerpc/kernel/signal.c +1 -3

index a2dc757..3b99711 100644
@@ -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);

Signature · 1 line

1.5.5.1

Ananth N Mavinakayanahalli

Acked-by

2012-10-29 13:31 UTC #5 lore raw

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

Quoted 28 lines · Oleg Nesterov

Quoted 18 lines · Ananth N Mavinakayanahalli
On Sun, Oct 28, 2012 at 06:39:25PM +0100, Oleg Nesterov wrote:
Hi Oleg,
Quoted 13 lines
Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
Signed-off-by Oleg Nesterov <[email protected]>
---
 arch/powerpc/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
arch/powerpc/kernel/signal.c +0 -0
index a2dc757..3b99711 100644
@@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
 
Quoted 3 lines
arch/powerpc/kernel/signal.c static int do_signal(struct pt_regs *regs) · line 159
Quoted 1 line
arch/powerpc/kernel/signal.c +0 -0
 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.
Quoted 3 lines
I don't see this flag cleared
anywhere else.

Quoted 7 lines

Quoted 1 line
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.

Quoted 8 lines

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 <[email protected]>

Acked-by Ananth N Mavinakayanahalli <[email protected]>

Thanks Oleg!

Ananth

Srikar Dronamraju

Acked-by

2012-11-02 05:09 UTC #6 lore raw

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

In reply to Oleg Nesterov

Quoted 5 lines

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 <[email protected]>

Acked-by Srikar Dronamraju <[email protected]>

Quoted 24 lines

---
 arch/powerpc/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

arch/powerpc/kernel/signal.c +1 -3

index a2dc757..3b99711 100644
@@ -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);

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2012-10-28 17:38 UTC #7 lore raw

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

In reply to 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 <[email protected]>

---
 arch/powerpc/kernel/uprobes.c |    6 ++++++
 kernel/events/uprobes.c       |    2 --
 2 files changed, 6 insertions(+), 2 deletions(-)

arch/powerpc/kernel/uprobes.c +6 -0

index d2d46d1..bc77834 100644
@@ -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);
 }
 
 /*

kernel/events/uprobes.c +0 -2

index 672227a..916391e 100644
@@ -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);
 }
 
 /*

Signature · 1 line

1.5.5.1

Ananth N Mavinakayanahalli

Acked-by on v1 2/4

2012-10-29 05:33 UTC #8 lore raw

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

Quoted 11 lines · 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.

Acked-by Ananth N Mavinakayanahalli <[email protected]>

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

Ananth

Srikar Dronamraju

Acked-by on v1 2/4

2012-11-02 05:30 UTC #9 lore raw

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

In reply to Oleg Nesterov

Quoted 8 lines · 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.

Quoted 5 lines

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 <[email protected]>

Acked-by Srikar Dronamraju <[email protected]>

Quoted 56 lines

---
 arch/powerpc/kernel/uprobes.c |    6 ++++++
 kernel/events/uprobes.c       |    2 --
 2 files changed, 6 insertions(+), 2 deletions(-)

arch/powerpc/kernel/uprobes.c +6 -0

index d2d46d1..bc77834 100644
@@ -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);
 }
 
 /*

kernel/events/uprobes.c +0 -2

index 672227a..916391e 100644
@@ -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);
 }
 
 /*

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2012-10-28 17:39 UTC #10 lore raw

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

In reply to 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 <[email protected]>

---
 arch/x86/kernel/uprobes.c |   54 +++++++++++++++-----------------------------
 1 files changed, 19 insertions(+), 35 deletions(-)

arch/x86/kernel/uprobes.c +19 ~~-35~~

index aafa555..c71025b 100644
@@ -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;
-	}
-}

Signature · 1 line

1.5.5.1

Srikar Dronamraju

Acked-by on v1 3/4

2012-11-02 12:47 UTC #11 lore raw

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

Quoted 7 lines · 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 <[email protected]>

Acked-by Srikar Dronamraju <[email protected]>

Quoted 90 lines

---
 arch/x86/kernel/uprobes.c |   54 +++++++++++++++-----------------------------
 1 files changed, 19 insertions(+), 35 deletions(-)

arch/x86/kernel/uprobes.c +19 ~~-35~~

index aafa555..c71025b 100644
@@ -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;
-	}
-}

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2012-10-28 17:38 UTC #12 lore raw

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

In reply to Oleg Nesterov

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

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/linux/uprobes.h |    2 --
 kernel/events/uprobes.c |   10 ----------
 2 files changed, 0 insertions(+), 12 deletions(-)

include/linux/uprobes.h +0 -2

index 2459457..2615c4d 100644
@@ -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);

kernel/events/uprobes.c +0 ~~-10~~

index 916391e..02d7c5f 100644
@@ -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;

Signature · 1 line

1.5.5.1

Srikar Dronamraju

Acked-by on v1 4/4

2012-11-02 12:47 UTC #13 lore raw

Quoted 4 lines · Oleg Nesterov

Kill arch_uprobe_enable/disable_step() hooks, they do nothing and nobody needs them.
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Srikar Dronamraju <[email protected]>

Quoted 56 lines

---
 include/linux/uprobes.h |    2 --
 kernel/events/uprobes.c |   10 ----------
 2 files changed, 0 insertions(+), 12 deletions(-)

include/linux/uprobes.h +0 -2

index 2459457..2615c4d 100644
@@ -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);

kernel/events/uprobes.c +0 ~~-10~~

index 916391e..02d7c5f 100644
@@ -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;

Signature · 1 line

1.5.5.1