[PATCH 0/5] uprobes: handle_swbp() fixes

[PATCH 0/5] uprobes: handle_swbp() fixes

[Oleg Nesterov]

Hello.

Mostly fixes the error hanling in handle_swbp().

Srikar, please review.

Oleg.

 arch/x86/kernel/signal.c |    4 +--
 include/linux/uprobes.h  |    1 -
 kernel/events/uprobes.c  |   66 +++++++++++++++++++--------------------------
 3 files changed, 29 insertions(+), 42 deletions(-)

[PATCH 1/5] uprobes: Do not leak UTASK_BP_HIT if find_active_uprobe() fails

[Oleg Nesterov]

If handle_swbp()->find_active_uprobe() fails we return with
utask->state = UTASK_BP_HIT.

Change handle_swbp() to reset utask->state at the start. Note
that we do this unconditionally, see the next patch(es).

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 26f06a6..760acc1 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1468,6 +1468,10 @@ static void handle_swbp(struct pt_regs *regs)
 	bp_vaddr = uprobe_get_swbp_addr(regs);
 	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
 
+	utask = current->utask;
+	if (utask)
+		utask->state = UTASK_RUNNING;
+
 	if (!uprobe) {
 		if (is_swbp > 0) {
 			/* No matching uprobe; signal SIGTRAP. */
@@ -1486,7 +1490,6 @@ static void handle_swbp(struct pt_regs *regs)
 		return;
 	}
 
-	utask = current->utask;
 	if (!utask) {
 		utask = add_utask();
 		/* Cannot allocate; re-execute the instruction. */
-- 
1.5.5.1

[PATCH 2/5] uprobes: Do not setup ->active_uprobe/state prematurely

[Oleg Nesterov]

handle_swbp() sets utask->active_uprobe before handler_chain(),
and UTASK_SSTEP before pre_ssout(). This complicates the code
for no reason,  arch_ hooks or consumer->handler() should not
(and can't) use this info.

Change handle_swbp() to initialize them after pre_ssout(), and
remove the no longer needed cleanup-utask code.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 760acc1..9893cba 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1496,22 +1496,19 @@ static void handle_swbp(struct pt_regs *regs)
 		if (!utask)
 			goto cleanup_ret;
 	}
-	utask->active_uprobe = uprobe;
+
 	handler_chain(uprobe, regs);
 	if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs))
 		goto cleanup_ret;
 
-	utask->state = UTASK_SSTEP;
 	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
 		arch_uprobe_enable_step(&uprobe->arch);
+		utask->active_uprobe = uprobe;
+		utask->state = UTASK_SSTEP;
 		return;
 	}
 
 cleanup_ret:
-	if (utask) {
-		utask->active_uprobe = NULL;
-		utask->state = UTASK_RUNNING;
-	}
 	if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
 
 		/*
-- 
1.5.5.1

[PATCH 3/5] uprobes: Fix UPROBE_SKIP_SSTEP checks in handle_swbp()

[Oleg Nesterov]

If handle_swbp()->add_utask() fails but UPROBE_SKIP_SSTEP is set,
cleanup_ret: path do not restart the insn, this is wrong. Remove
this check and add the additional label for can_skip_sstep() = T
case.

Note also that UPROBE_SKIP_SSTEP can be false positive, we simply
can not trust it unless arch_uprobe_skip_sstep() was already called.

Also, move another UPROBE_SKIP_SSTEP check before can_skip_sstep()
into this helper, this looks more clean and understandable.

Note: probably we should rename "skip" to "emulate" and I think
that "clear UPROBE_SKIP_SSTEP" should be moved to arch_can_skip.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |   31 +++++++++++++++----------------
 1 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 9893cba..403d2e0 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1389,10 +1389,11 @@ bool uprobe_deny_signal(void)
  */
 static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)
 {
-	if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
-		return true;
-
-	uprobe->flags &= ~UPROBE_SKIP_SSTEP;
+	if (uprobe->flags & UPROBE_SKIP_SSTEP) {
+		if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
+			return true;
+		uprobe->flags &= ~UPROBE_SKIP_SSTEP;
+	}
 	return false;
 }
 
@@ -1494,12 +1495,12 @@ static void handle_swbp(struct pt_regs *regs)
 		utask = add_utask();
 		/* Cannot allocate; re-execute the instruction. */
 		if (!utask)
-			goto cleanup_ret;
+			goto restart;
 	}
 
 	handler_chain(uprobe, regs);
-	if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs))
-		goto cleanup_ret;
+	if (can_skip_sstep(uprobe, regs))
+		goto out;
 
 	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
 		arch_uprobe_enable_step(&uprobe->arch);
@@ -1508,15 +1509,13 @@ static void handle_swbp(struct pt_regs *regs)
 		return;
 	}
 
-cleanup_ret:
-	if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
-
-		/*
-		 * cannot singlestep; cannot skip instruction;
-		 * re-execute the instruction.
-		 */
-		instruction_pointer_set(regs, bp_vaddr);
-
+restart:
+	/*
+	 * cannot singlestep; cannot skip instruction;
+	 * re-execute the instruction.
+	 */
+	instruction_pointer_set(regs, bp_vaddr);
+out:
 	put_uprobe(uprobe);
 }
 
-- 
1.5.5.1

[PATCH 4/5] uprobes: Kill UTASK_BP_HIT state

[Oleg Nesterov]

Kill UTASK_BP_HIT state, it buys nothing but complicates the code.
It is only used in uprobe_notify_resume() to decide who should be
called, we can check utask->active_uprobe != NULL instead. And this
allows us to simplify handle_swbp(), no need to clear utask->state.

Likewise we could kill UTASK_SSTEP, but UTASK_BP_HIT is worse and
imho should die. The problem is, it creates the special case when
task->utask is NULL, we can't distinguish RUNNING and BP_HIT. With
his patch utask == NULL always means RUNNING.

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

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index e6f0331..18d839d 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -59,7 +59,6 @@ struct uprobe_consumer {
 #ifdef CONFIG_UPROBES
 enum uprobe_task_state {
 	UTASK_RUNNING,
-	UTASK_BP_HIT,
 	UTASK_SSTEP,
 	UTASK_SSTEP_ACK,
 	UTASK_SSTEP_TRAPPED,
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 403d2e0..4ea0f0b 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1469,10 +1469,6 @@ static void handle_swbp(struct pt_regs *regs)
 	bp_vaddr = uprobe_get_swbp_addr(regs);
 	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
 
-	utask = current->utask;
-	if (utask)
-		utask->state = UTASK_RUNNING;
-
 	if (!uprobe) {
 		if (is_swbp > 0) {
 			/* No matching uprobe; signal SIGTRAP. */
@@ -1491,6 +1487,7 @@ static void handle_swbp(struct pt_regs *regs)
 		return;
 	}
 
+	utask = current->utask;
 	if (!utask) {
 		utask = add_utask();
 		/* Cannot allocate; re-execute the instruction. */
@@ -1547,13 +1544,12 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
 }
 
 /*
- * On breakpoint hit, breakpoint notifier sets the TIF_UPROBE flag.  (and on
- * subsequent probe hits on the thread sets the state to UTASK_BP_HIT) and
- * allows the thread to return from interrupt.
+ * On breakpoint hit, breakpoint notifier sets the TIF_UPROBE flag and
+ * allows the thread to return from interrupt. After that handle_swbp()
+ * sets utask->active_uprobe.
  *
- * On singlestep exception, singlestep notifier sets the TIF_UPROBE flag and
- * also sets the state to UTASK_SSTEP_ACK and allows the thread to return from
- * interrupt.
+ * On singlestep exception, singlestep notifier sets the TIF_UPROBE flag
+ * and allows the thread to return from interrupt.
  *
  * While returning to userspace, thread notices the TIF_UPROBE flag and calls
  * uprobe_notify_resume().
@@ -1563,10 +1559,10 @@ void uprobe_notify_resume(struct pt_regs *regs)
 	struct uprobe_task *utask;
 
 	utask = current->utask;
-	if (!utask || utask->state == UTASK_BP_HIT)
-		handle_swbp(regs);
-	else
+	if (utask && utask->active_uprobe)
 		handle_singlestep(utask, regs);
+	else
+		handle_swbp(regs);
 }
 
 /*
@@ -1575,17 +1571,10 @@ void uprobe_notify_resume(struct pt_regs *regs)
  */
 int uprobe_pre_sstep_notifier(struct pt_regs *regs)
 {
-	struct uprobe_task *utask;
-
 	if (!current->mm || !test_bit(MMF_HAS_UPROBES, &current->mm->flags))
 		return 0;
 
-	utask = current->utask;
-	if (utask)
-		utask->state = UTASK_BP_HIT;
-
 	set_thread_flag(TIF_UPROBE);
-
 	return 1;
 }
 
-- 
1.5.5.1

[PATCH 5/5] uprobes: Move clear_thread_flag(TIF_UPROBE) to uprobe_notify_resume()

[Oleg Nesterov]

Move clear_thread_flag(TIF_UPROBE) from do_notify_resume() to
uprobe_notify_resume() for !CONFIG_UPROBES case.

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

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index b280908..0041e5a 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -785,10 +785,8 @@ do_notify_resume(struct pt_regs *regs, void *unused, __u32 thread_info_flags)
 		mce_notify_process();
 #endif /* CONFIG_X86_64 && CONFIG_X86_MCE */
 
-	if (thread_info_flags & _TIF_UPROBE) {
-		clear_thread_flag(TIF_UPROBE);
+	if (thread_info_flags & _TIF_UPROBE)
 		uprobe_notify_resume(regs);
-	}
 
 	/* deal with pending signal delivery */
 	if (thread_info_flags & _TIF_SIGPENDING)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 4ea0f0b..14c2e99 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1558,6 +1558,8 @@ void uprobe_notify_resume(struct pt_regs *regs)
 {
 	struct uprobe_task *utask;
 
+	clear_thread_flag(TIF_UPROBE);
+
 	utask = current->utask;
 	if (utask && utask->active_uprobe)
 		handle_singlestep(utask, regs);
-- 
1.5.5.1

Re: [PATCH 1/5] uprobes: Do not leak UTASK_BP_HIT if find_active_uprobe() fails

[Oleg Nesterov]

(additional note to simplify the review)

On 09/14, Oleg Nesterov wrote:
>
> Note
> that we do this unconditionally, see the next patch(es).

IOW, we could move it under "if (!uprobe)" branch, but we do not.

This is mostly needed for 2/5, which in turn is needed for 3/5.
If can_skip_sstep() succeeds we need to reset utask->state too.

ALternatively we could start with 4/5, but this is the "separate"
change (and it is not bug-fix) which needs your review/ack.

Oleg.

Re: [PATCH 3/5] uprobes: Fix UPROBE_SKIP_SSTEP checks in handle_swbp()

[Ananth N Mavinakayanahalli]

On Fri, Sep 14, 2012 at 07:15:57PM +0200, Oleg Nesterov wrote:
> If handle_swbp()->add_utask() fails but UPROBE_SKIP_SSTEP is set,
> cleanup_ret: path do not restart the insn, this is wrong. Remove
> this check and add the additional label for can_skip_sstep() = T
> case.
> 
> Note also that UPROBE_SKIP_SSTEP can be false positive, we simply
> can not trust it unless arch_uprobe_skip_sstep() was already called.
> 
> Also, move another UPROBE_SKIP_SSTEP check before can_skip_sstep()
> into this helper, this looks more clean and understandable.
> 
> Note: probably we should rename "skip" to "emulate" and I think
> that "clear UPROBE_SKIP_SSTEP" should be moved to arch_can_skip.

Agree. emulate is more accurate in this situation since, especially on
powerpc, we do emulate most instructions.

Ananth

Re: [PATCH 3/5] uprobes: Fix UPROBE_SKIP_SSTEP checks in handle_swbp()

[Oleg Nesterov]

On 09/15, Ananth N Mavinakayanahalli wrote:
>
> On Fri, Sep 14, 2012 at 07:15:57PM +0200, Oleg Nesterov wrote:
> >
> > Note: probably we should rename "skip" to "emulate" and I think
> > that "clear UPROBE_SKIP_SSTEP" should be moved to arch_can_skip.
>
> Agree. emulate is more accurate in this situation since, especially on
> powerpc, we do emulate most instructions.

Yes. And even on x86, perhaps we should emulate at least pushf to
not expose TF set by uprobes.

Off-topic question... I am trying to understand if arch_uprobe_skip_sstep()
is correct on x86.

It doesn't update regs->ip. Probably this is fine, at least this is
fine if it finds "nop" eventually. But I can't undestand what
"0x66* { 0x90 | 0x0f 0x1f | 0x0f 0x19 | 0x87 0xc0 }" means.
OK, 0x66 and 0x90 are clear. But, say, 0x0f 0x1f ?

I compiled this program

	int main(void)
	{
		asm volatile (".word 0x1f0f");
		return 0;
	}

and objdump reports:

	000000000040047c <main>:
	  40047c:       0f 1f 31                nopl   (%rcx)
	  40047f:       c0 c3 90                rol    $0x90,%bl

Could you explain?

Oleg.

Re: [PATCH 4/5] uprobes: Kill UTASK_BP_HIT state

[Oleg Nesterov]

On 09/14, Oleg Nesterov wrote:
>
> Kill UTASK_BP_HIT state, it buys nothing but complicates the code.
> It is only used in uprobe_notify_resume() to decide who should be
> called, we can check utask->active_uprobe != NULL instead. And this
> allows us to simplify handle_swbp(), no need to clear utask->state.

I am starting to think this patch makes even more sense than I thought.

> Likewise we could kill UTASK_SSTEP,

In fact we can kill utask->state. But I just realized we can also kill
uprobe_deny_signal() and simplify arch_uprobe_xol_was_trapped() logic.

But this needs some (simple) changes in arch/ code, so we need to wait
until powerpc is merged.

Oleg.

Re: [PATCH 3/5] uprobes: Fix UPROBE_SKIP_SSTEP checks in handle_swbp()

[Srikar Dronamraju]

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

> On 09/15, Ananth N Mavinakayanahalli wrote:
> >
> > On Fri, Sep 14, 2012 at 07:15:57PM +0200, Oleg Nesterov wrote:
> > >
> > > Note: probably we should rename "skip" to "emulate" and I think
> > > that "clear UPROBE_SKIP_SSTEP" should be moved to arch_can_skip.
> >
> > Agree. emulate is more accurate in this situation since, especially on
> > powerpc, we do emulate most instructions.
> 
> Yes. And even on x86, perhaps we should emulate at least pushf to
> not expose TF set by uprobes.
> 

Good idea. 

> Off-topic question... I am trying to understand if arch_uprobe_skip_sstep()
> is correct on x86.
> 
> It doesn't update regs->ip. 

Right. we need to adjust for the size of the instruction.

> Probably this is fine, at least this is
> fine if it finds "nop" eventually. But I can't undestand what
> "0x66* { 0x90 | 0x0f 0x1f | 0x0f 0x19 | 0x87 0xc0 }" means.
> OK, 0x66 and 0x90 are clear. But, say, 0x0f 0x1f ?

we skip is 0x66 ..0x66 0x0f 0x1f

So we have a check
if (i == (MAX_UINSN_BYTES - 1)) 

so this ensures that we are consider 0x0f 0x1f as nop if and only if
they are at the end and preceeded by 0x66. This is not an exhaustive
list of nops.

So are you suggesting extending the list of nops or is it that we are
considering non nop instructions as nops?

Extending the list, we certainly should not just for nops.

> 
> I compiled this program
> 
> 	int main(void)
> 	{
> 		asm volatile (".word 0x1f0f");
> 		return 0;
> 	}
> 
> and objdump reports:
> 
> 	000000000040047c <main>:
> 	  40047c:       0f 1f 31                nopl   (%rcx)

Current uprobes code wouldnt skip the above insn because it has 31
following it.

> 	  40047f:       c0 c3 90                rol    $0x90,%bl

we dont skip this too.

-- 
Thanks and Regards
Srikar

Re: [PATCH 3/5] uprobes: Fix UPROBE_SKIP_SSTEP checks in handle_swbp()

[Oleg Nesterov]

On 09/17, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-09-15 17:01:20]:
>
> > Off-topic question... I am trying to understand if arch_uprobe_skip_sstep()
> > is correct on x86.
> >
> > It doesn't update regs->ip.
>
> Right. we need to adjust for the size of the instruction.
>
> > Probably this is fine, at least this is
> > fine if it finds "nop" eventually. But I can't undestand what
> > "0x66* { 0x90 | 0x0f 0x1f | 0x0f 0x19 | 0x87 0xc0 }" means.
> > OK, 0x66 and 0x90 are clear. But, say, 0x0f 0x1f ?
>
> we skip is 0x66 ..0x66 0x0f 0x1f
>
> So we have a check
> if (i == (MAX_UINSN_BYTES - 1))
>
> so this ensures that we are consider 0x0f 0x1f as nop if and only if
> they are at the end and preceeded by 0x66.

Hmm. How so? The code does

	if (i == (MAX_UINSN_BYTES - 1))
		break;

	if ((auprobe->insn[i] == 0x0f) && (auprobe->insn[i+1] == 0x1f))
		return true;


So, afaics, if the intent was to skip 1f0f at the end only, it should do

	if (i == (MAX_UINSN_BYTES - 1)) {
		if ((auprobe->insn[i] == 0x0f) && (auprobe->insn[i+1] == 0x1f))
			return true;
		...
	}

"and preceeded by 0x66" above doesn't look true too, perhaps you
meant "may be preceeded by 0x66".

> So are you suggesting extending the list of nops or is it that we are
> considering non nop instructions as nops?

No, I am trying to understand which insns arch_skip tries to skip.
In particular, what "0x0f 0x1f" means.

> > I compiled this program
> >
> > 	int main(void)
> > 	{
> > 		asm volatile (".word 0x1f0f");
> > 		return 0;
> > 	}
> >
> > and objdump reports:
> >
> > 	000000000040047c <main>:
> > 	  40047c:       0f 1f 31                nopl   (%rcx)
>
> Current uprobes code wouldnt skip the above insn because it has 31
> following it.

See above.

And again, could you explain which insn has 1f0f (at the end or not) ?
IOW, what we are trying to skip?

Oleg.

Re: [PATCH 1/5] uprobes: Do not leak UTASK_BP_HIT if find_active_uprobe() fails

[Srikar Dronamraju]

> If handle_swbp()->find_active_uprobe() fails we return with
> utask->state = UTASK_BP_HIT.
> 
> Change handle_swbp() to reset utask->state at the start. Note
> that we do this unconditionally, see the next patch(es).
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 26f06a6..760acc1 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1468,6 +1468,10 @@ static void handle_swbp(struct pt_regs *regs)
>  	bp_vaddr = uprobe_get_swbp_addr(regs);
>  	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> 
> +	utask = current->utask;
> +	if (utask)
> +		utask->state = UTASK_RUNNING;
> +
>  	if (!uprobe) {
>  		if (is_swbp > 0) {
>  			/* No matching uprobe; signal SIGTRAP. */
> @@ -1486,7 +1490,6 @@ static void handle_swbp(struct pt_regs *regs)
>  		return;
>  	}
> 
> -	utask = current->utask;
>  	if (!utask) {
>  		utask = add_utask();
>  		/* Cannot allocate; re-execute the instruction. */


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

Re: [PATCH 2/5] uprobes: Do not setup ->active_uprobe/state prematurely

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-14 19:15:54]:

> handle_swbp() sets utask->active_uprobe before handler_chain(),
> and UTASK_SSTEP before pre_ssout(). This complicates the code
> for no reason,  arch_ hooks or consumer->handler() should not
> (and can't) use this info.
> 
> Change handle_swbp() to initialize them after pre_ssout(), and
> remove the no longer needed cleanup-utask code.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 760acc1..9893cba 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1496,22 +1496,19 @@ static void handle_swbp(struct pt_regs *regs)
>  		if (!utask)
>  			goto cleanup_ret;
>  	}
> -	utask->active_uprobe = uprobe;
> +
>  	handler_chain(uprobe, regs);
>  	if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs))
>  		goto cleanup_ret;
> 
> -	utask->state = UTASK_SSTEP;
>  	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
>  		arch_uprobe_enable_step(&uprobe->arch);
> +		utask->active_uprobe = uprobe;
> +		utask->state = UTASK_SSTEP;
>  		return;
>  	}
> 
>  cleanup_ret:
> -	if (utask) {
> -		utask->active_uprobe = NULL;
> -		utask->state = UTASK_RUNNING;
> -	}
>  	if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
> 
>  		/*

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

Re: [PATCH 3/5] uprobes: Fix UPROBE_SKIP_SSTEP checks in handle_swbp()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-14 19:15:57]:

> If handle_swbp()->add_utask() fails but UPROBE_SKIP_SSTEP is set,
> cleanup_ret: path do not restart the insn, this is wrong. Remove
> this check and add the additional label for can_skip_sstep() = T
> case.
> 
> Note also that UPROBE_SKIP_SSTEP can be false positive, we simply
> can not trust it unless arch_uprobe_skip_sstep() was already called.
> 
> Also, move another UPROBE_SKIP_SSTEP check before can_skip_sstep()
> into this helper, this looks more clean and understandable.
> 
> Note: probably we should rename "skip" to "emulate" and I think

yes we can rename can_skip_step to can_emulate_insn and
arch_uprobe_skip_step() to arch_uprobe_emulate_insn

Similarly UPROBE_SKIP_SSTEP can be renamed as UPROBE_EMULATE_INSN

> that "clear UPROBE_SKIP_SSTEP" should be moved to arch_can_skip.
> 

Currently struct uprobe is not exposed to arch specific code as
suggested by Ingo. Adding a flag in arch_uprobe just for this and
expecting all archs to define one is probably an overhead.
Hence I am not sure moving the clear flag to arch is a good idea.

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

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

> ---
>  kernel/events/uprobes.c |   31 +++++++++++++++----------------
>  1 files changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 9893cba..403d2e0 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1389,10 +1389,11 @@ bool uprobe_deny_signal(void)
>   */
>  static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)
>  {
> -	if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
> -		return true;
> -
> -	uprobe->flags &= ~UPROBE_SKIP_SSTEP;
> +	if (uprobe->flags & UPROBE_SKIP_SSTEP) {
> +		if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
> +			return true;
> +		uprobe->flags &= ~UPROBE_SKIP_SSTEP;
> +	}
>  	return false;
>  }
> 
> @@ -1494,12 +1495,12 @@ static void handle_swbp(struct pt_regs *regs)
>  		utask = add_utask();
>  		/* Cannot allocate; re-execute the instruction. */
>  		if (!utask)
> -			goto cleanup_ret;
> +			goto restart;
>  	}
> 
>  	handler_chain(uprobe, regs);
> -	if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs))
> -		goto cleanup_ret;
> +	if (can_skip_sstep(uprobe, regs))
> +		goto out;
> 
>  	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
>  		arch_uprobe_enable_step(&uprobe->arch);
> @@ -1508,15 +1509,13 @@ static void handle_swbp(struct pt_regs *regs)
>  		return;
>  	}
> 
> -cleanup_ret:
> -	if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
> -
> -		/*
> -		 * cannot singlestep; cannot skip instruction;
> -		 * re-execute the instruction.
> -		 */
> -		instruction_pointer_set(regs, bp_vaddr);
> -
> +restart:
> +	/*
> +	 * cannot singlestep; cannot skip instruction;
> +	 * re-execute the instruction.
> +	 */
> +	instruction_pointer_set(regs, bp_vaddr);
> +out:
>  	put_uprobe(uprobe);
>  }
> 
> -- 
> 1.5.5.1
> 

Re: [PATCH 4/5] uprobes: Kill UTASK_BP_HIT state

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-14 19:16:00]:

> Kill UTASK_BP_HIT state, it buys nothing but complicates the code.
> It is only used in uprobe_notify_resume() to decide who should be
> called, we can check utask->active_uprobe != NULL instead. And this
> allows us to simplify handle_swbp(), no need to clear utask->state.
> 
> Likewise we could kill UTASK_SSTEP, but UTASK_BP_HIT is worse and
> imho should die. The problem is, it creates the special case when
> task->utask is NULL, we can't distinguish RUNNING and BP_HIT. With
> his patch utask == NULL always means RUNNING.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>


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

> ---
>  include/linux/uprobes.h |    1 -
>  kernel/events/uprobes.c |   29 +++++++++--------------------
>  2 files changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index e6f0331..18d839d 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -59,7 +59,6 @@ struct uprobe_consumer {
>  #ifdef CONFIG_UPROBES
>  enum uprobe_task_state {
>  	UTASK_RUNNING,
> -	UTASK_BP_HIT,
>  	UTASK_SSTEP,
>  	UTASK_SSTEP_ACK,
>  	UTASK_SSTEP_TRAPPED,
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 403d2e0..4ea0f0b 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1469,10 +1469,6 @@ static void handle_swbp(struct pt_regs *regs)
>  	bp_vaddr = uprobe_get_swbp_addr(regs);
>  	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> 
> -	utask = current->utask;
> -	if (utask)
> -		utask->state = UTASK_RUNNING;
> -
>  	if (!uprobe) {
>  		if (is_swbp > 0) {
>  			/* No matching uprobe; signal SIGTRAP. */
> @@ -1491,6 +1487,7 @@ static void handle_swbp(struct pt_regs *regs)
>  		return;
>  	}
> 
> +	utask = current->utask;
>  	if (!utask) {
>  		utask = add_utask();
>  		/* Cannot allocate; re-execute the instruction. */
> @@ -1547,13 +1544,12 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
>  }
> 
>  /*
> - * On breakpoint hit, breakpoint notifier sets the TIF_UPROBE flag.  (and on
> - * subsequent probe hits on the thread sets the state to UTASK_BP_HIT) and
> - * allows the thread to return from interrupt.
> + * On breakpoint hit, breakpoint notifier sets the TIF_UPROBE flag and
> + * allows the thread to return from interrupt. After that handle_swbp()
> + * sets utask->active_uprobe.
>   *
> - * On singlestep exception, singlestep notifier sets the TIF_UPROBE flag and
> - * also sets the state to UTASK_SSTEP_ACK and allows the thread to return from
> - * interrupt.
> + * On singlestep exception, singlestep notifier sets the TIF_UPROBE flag
> + * and allows the thread to return from interrupt.
>   *
>   * While returning to userspace, thread notices the TIF_UPROBE flag and calls
>   * uprobe_notify_resume().
> @@ -1563,10 +1559,10 @@ void uprobe_notify_resume(struct pt_regs *regs)
>  	struct uprobe_task *utask;
> 
>  	utask = current->utask;
> -	if (!utask || utask->state == UTASK_BP_HIT)
> -		handle_swbp(regs);
> -	else
> +	if (utask && utask->active_uprobe)
>  		handle_singlestep(utask, regs);
> +	else
> +		handle_swbp(regs);
>  }
> 
>  /*
> @@ -1575,17 +1571,10 @@ void uprobe_notify_resume(struct pt_regs *regs)
>   */
>  int uprobe_pre_sstep_notifier(struct pt_regs *regs)
>  {
> -	struct uprobe_task *utask;
> -
>  	if (!current->mm || !test_bit(MMF_HAS_UPROBES, &current->mm->flags))
>  		return 0;
> 
> -	utask = current->utask;
> -	if (utask)
> -		utask->state = UTASK_BP_HIT;
> -
>  	set_thread_flag(TIF_UPROBE);
> -
>  	return 1;
>  }
> 
> -- 
> 1.5.5.1
> 

Re: [PATCH 5/5] uprobes: Move clear_thread_flag(TIF_UPROBE) to uprobe_notify_resume()

[Srikar Dronamraju]

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

> Move clear_thread_flag(TIF_UPROBE) from do_notify_resume() to
> uprobe_notify_resume() for !CONFIG_UPROBES case.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  arch/x86/kernel/signal.c |    4 +---
>  kernel/events/uprobes.c  |    2 ++
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index b280908..0041e5a 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -785,10 +785,8 @@ do_notify_resume(struct pt_regs *regs, void *unused, __u32 thread_info_flags)
>  		mce_notify_process();
>  #endif /* CONFIG_X86_64 && CONFIG_X86_MCE */
> 
> -	if (thread_info_flags & _TIF_UPROBE) {
> -		clear_thread_flag(TIF_UPROBE);
> +	if (thread_info_flags & _TIF_UPROBE)
>  		uprobe_notify_resume(regs);
> -	}
> 
>  	/* deal with pending signal delivery */
>  	if (thread_info_flags & _TIF_SIGPENDING)
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 4ea0f0b..14c2e99 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1558,6 +1558,8 @@ void uprobe_notify_resume(struct pt_regs *regs)
>  {
>  	struct uprobe_task *utask;
> 
> +	clear_thread_flag(TIF_UPROBE);
> +
>  	utask = current->utask;
>  	if (utask && utask->active_uprobe)
>  		handle_singlestep(utask, regs);
> -- 
> 1.5.5.1
> 

Re: [PATCH 3/5] uprobes: Fix UPROBE_SKIP_SSTEP checks in handle_swbp()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-18 18:07:38]:

> >
> > > Probably this is fine, at least this is
> > > fine if it finds "nop" eventually. But I can't undestand what
> > > "0x66* { 0x90 | 0x0f 0x1f | 0x0f 0x19 | 0x87 0xc0 }" means.
> > > OK, 0x66 and 0x90 are clear. But, say, 0x0f 0x1f ?
> >
> > we skip is 0x66 ..0x66 0x0f 0x1f
> >
> > So we have a check
> > if (i == (MAX_UINSN_BYTES - 1))
> >
> > so this ensures that we are consider 0x0f 0x1f as nop if and only if
> > they are at the end and preceeded by 0x66.
> 
> Hmm. How so? The code does
> 
> 	if (i == (MAX_UINSN_BYTES - 1))
> 		break;
> 
> 	if ((auprobe->insn[i] == 0x0f) && (auprobe->insn[i+1] == 0x1f))
> 		return true;
> 
> 
> So, afaics, if the intent was to skip 1f0f at the end only, it should do

Its 0f1f and not 1f0f

> 
> 	if (i == (MAX_UINSN_BYTES - 1)) {
> 		if ((auprobe->insn[i] == 0x0f) && (auprobe->insn[i+1] == 0x1f))
> 			return true;
> 		...
> 	}
> 
> "and preceeded by 0x66" above doesn't look true too, perhaps you
> meant "may be preceeded by 0x66".
> 

Yup, as always you are right. We expect 0x0f 0x1f preceeded by 0x66 to
be nop instructions.

> > So are you suggesting extending the list of nops or is it that we are
> > considering non nop instructions as nops?
> 
> No, I am trying to understand which insns arch_skip tries to skip.
> In particular, what "0x0f 0x1f" means.
> 
> > > I compiled this program
> > >
> > > 	int main(void)
> > > 	{
> > > 		asm volatile (".word 0x1f0f");
> > > 		return 0;
> > > 	}
> > >
> > > and objdump reports:
> > >
> > > 	000000000040047c <main>:
> > > 	  40047c:       0f 1f 31                nopl   (%rcx)
> >
> > Current uprobes code wouldnt skip the above insn because it has 31
> > following it.
> 
> See above.
> 
> And again, could you explain which insn has 1f0f (at the end or not) ?
> IOW, what we are trying to skip?

Again its 0f1f and not 1f0f

for example 
0f 1f 40 00
0f 1f 44 00 00
66 0f 1f 44 00 00

I referred arch/x86/include/asm/nops.h, arch/x86/lib/x86-opcode-map.txt
and disassembly of libc.  And ofcourse Jim Keniston helped me in most of
the x86 stuff.

--
thanks and regards
Srikar

Re: [PATCH 3/5] uprobes: Fix UPROBE_SKIP_SSTEP checks in handle_swbp()

[Oleg Nesterov]

Srikar, sorry for delay, somehow I missed this email.

And I am still confused...

On 09/20, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-09-18 18:07:38]:
>
> > > > I compiled this program
> > > >
> > > > 	int main(void)
> > > > 	{
> > > > 		asm volatile (".word 0x1f0f");
> > > > 		return 0;
> > > > 	}
> > > >
> > > > and objdump reports:
> > > >
> > > > 	000000000040047c <main>:
> > > > 	  40047c:       0f 1f 31                nopl   (%rcx)
> > >
> > > Current uprobes code wouldnt skip the above insn because it has 31
> > > following it.
> >
> > See above.
> >
> > And again, could you explain which insn has 1f0f (at the end or not) ?
> > IOW, what we are trying to skip?
>
> Again its 0f1f and not 1f0f

The first byte is 0x0f, the next is 0x1f, so 0x1f0f looks correct,
but this doesn't matter.

Anyway,

> for example
> 0f 1f 40 00

OK, thanks, objdump reports "nopl 0x0(%rax)", looks fine.

But. I do not see how __skip_sstep() can handle this case correctly.
Not only it should update regs->ip afaics, it should also account 2
extra bytes _after_ 0f 1f.

> 0f 1f 44 00 00

OK, nopl   0x0(%rax,%rax,1), but in this case we need to skip 3
extra bytes.


I am starting to think this code is broken and we should simply remove
all checks except 0x66 and 0x90. In this case we do not even need to
update regs->ip.

Otherwise this code needs to know the insn's length.

Right?

Oleg.

Re: [PATCH 3/5] uprobes: Fix UPROBE_SKIP_SSTEP checks in handle_swbp()

[Oleg Nesterov]

On 09/24, Oleg Nesterov wrote:
>
> Anyway,
>
> > for example
> > 0f 1f 40 00
>
> OK, thanks, objdump reports "nopl 0x0(%rax)", looks fine.
>
> But. I do not see how __skip_sstep() can handle this case correctly.
> Not only it should update regs->ip afaics, it should also account 2
> extra bytes _after_ 0f 1f.
>
> > 0f 1f 44 00 00
>
> OK, nopl   0x0(%rax,%rax,1), but in this case we need to skip 3
> extra bytes.
>
>
> I am starting to think this code is broken and we should simply remove
> all checks except 0x66 and 0x90. In this case we do not even need to
> update regs->ip.
>
> Otherwise this code needs to know the insn's length.
>
> Right?

OK, I verified that the application is really killed by SIGILL if
you probe such an instruction. I'll send the fix which simply removes
this code. I do not know how to figure out the actual insn length.

Oleg.