Skip to contentSkip to conversation

uprobes: handle_swbp() fixes

PATCH In review Oleg Nesterov 3 participants 20 messages Sep 14, 2012 – Sep 29, 2012

Subsystems (3)
x86 perf events uprobes
lore
Share & tools
lore

Oleg Nesterov

author maintainer
#0 lore raw

cover uprobes: handle_swbp() fixes

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(-)
author maintainer
#1 lore raw

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

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 <[email protected]>
---
 kernel/events/uprobes.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
kernel/events/uprobes.c +4 -1
index 26f06a6..760acc1 100644
@@ -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. */
Signature · 1 line
1.5.5.1
author maintainer
#2 lore raw

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

(additional note to simplify the review)

Quoted 3 lines · Oleg Nesterov

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.

Acked-by on v1 1/5
#3 lore raw

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

In reply to Oleg Nesterov
Quoted 27 lines

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 <[email protected]>
---
 kernel/events/uprobes.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
kernel/events/uprobes.c +4 -0
index 26f06a6..760acc1 100644
@@ -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. */

kernel/events/uprobes.c static void handle_swbp(struct pt_regs *regs) · line 1495

Quoted 7 lines
kernel/events/uprobes.c +0 -1
@@ -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 <[email protected]>
author maintainer
#4 lore raw

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

In reply to 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 <[email protected]>
---
 kernel/events/uprobes.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)
kernel/events/uprobes.c +3 -6
index 760acc1..9893cba 100644
@@ -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))
 
 		/*
Signature · 1 line
1.5.5.1
Acked-by on v1 2/5
#5 lore raw

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

Quoted 33 lines · 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 <[email protected]>
---
 kernel/events/uprobes.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)
kernel/events/uprobes.c +3 -2
index 760acc1..9893cba 100644
@@ -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;

kernel/events/uprobes.c static void handle_swbp(struct pt_regs *regs) · line 1514

Quoted 10 lines
kernel/events/uprobes.c +0 -4
 	}
 
 cleanup_ret:
-	if (utask) {
-		utask->active_uprobe = NULL;
-		utask->state = UTASK_RUNNING;
-	}
 	if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
 
 		/*
Acked-by Srikar Dronamraju <[email protected]>
author maintainer
#6 lore raw

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

In reply to 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 <[email protected]>
---
 kernel/events/uprobes.c |   31 +++++++++++++++----------------
 1 files changed, 15 insertions(+), 16 deletions(-)
kernel/events/uprobes.c +15 -16
index 9893cba..403d2e0 100644
@@ -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);
 }
Signature · 1 line
1.5.5.1
#7 lore raw

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

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

Quoted 8 lines

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

author maintainer
#8 lore raw

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

Quoted 8 lines · Ananth N Mavinakayanahalli
Quoted 3 lines · Oleg Nesterov

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.

#9 lore raw

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

Quoted 4 lines · Oleg Nesterov
Quoted 3 lines · Ananth N Mavinakayanahalli
Quoted 1 line · Oleg Nesterov
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 9 lines

I compiled this program

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

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.

Quoted 1 line
	  40047f:       c0 c3 90                rol    $0x90,%bl

we dont skip this too.

Signature · 2 lines
Thanks and Regards
Srikar
author maintainer
#10 lore raw

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

Quoted 15 lines · Srikar Dronamraju
Quoted 4 lines · Oleg Nesterov

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.

Quoted 4 lines

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 ?

Quoted 7 lines

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".

Quoted 2 lines

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.

Quoted 8 lines
Quoted 8 lines

I compiled this program

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

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.

#11 lore raw

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

Quoted 16 lines · Oleg Nesterov
Quoted 6 lines
Quoted 5 lines

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 ?

Quoted 7 lines

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

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

Quoted 10 lines
	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.

Quoted 19 lines
Quoted 2 lines

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.

Quoted 13 lines
Quoted 12 lines

I compiled this program

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

and objdump reports:

	000000000040047c <main>:
	  40047c:       0f 1f 31                nopl   (%rcx)
Quoted 7 lines
Quoted 2 lines

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

author maintainer
#12 lore raw

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

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

And I am still confused...

Quoted 19 lines · Srikar Dronamraju
Quoted 16 lines · Oleg Nesterov
Quoted 15 lines
Quoted 8 lines

I compiled this program

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

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.

Quoted 6 lines
Quoted 4 lines

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,

Quoted 2 lines

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.

Quoted 1 line

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.

author maintainer
#13 lore raw

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

Quoted 18 lines · Oleg Nesterov

Anyway,

Quoted 2 lines

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.

Quoted 1 line

0f 1f 44 00 00

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

Quoted 7 lines

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.

Acked-by on v1 3/5
#14 lore raw

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

In reply to Oleg Nesterov
Quoted 5 lines · 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.

Quoted 7 lines

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

Quoted 2 lines

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.

Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Srikar Dronamraju <[email protected]>
Quoted 66 lines
---
 kernel/events/uprobes.c |   31 +++++++++++++++----------------
 1 files changed, 15 insertions(+), 16 deletions(-)
kernel/events/uprobes.c +15 -16
index 9893cba..403d2e0 100644
@@ -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);
 }
Signature · 1 line
1.5.5.1
author maintainer
#15 lore raw

PATCH 4/5 uprobes: Kill UTASK_BP_HIT state

In reply to 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 <[email protected]>
---
 include/linux/uprobes.h |    1 -
 kernel/events/uprobes.c |   29 +++++++++--------------------
 2 files changed, 9 insertions(+), 21 deletions(-)
include/linux/uprobes.h +0 -1
index e6f0331..18d839d 100644
@@ -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,
kernel/events/uprobes.c +9 -20
index 403d2e0..4ea0f0b 100644
@@ -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;
 }
Signature · 1 line
1.5.5.1
author maintainer
#16 lore raw

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

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

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

Quoted 1 line

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.

Acked-by on v1 4/5
#17 lore raw

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

In reply to Oleg Nesterov
Quoted 11 lines · 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 <[email protected]>
Acked-by Srikar Dronamraju <[email protected]>
Quoted 94 lines
---
 include/linux/uprobes.h |    1 -
 kernel/events/uprobes.c |   29 +++++++++--------------------
 2 files changed, 9 insertions(+), 21 deletions(-)
include/linux/uprobes.h +0 -1
index e6f0331..18d839d 100644
@@ -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,
kernel/events/uprobes.c +9 -20
index 403d2e0..4ea0f0b 100644
@@ -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;
 }
Signature · 1 line
1.5.5.1
author maintainer
#18 lore raw

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

In reply to 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 <[email protected]>
---
 arch/x86/kernel/signal.c |    4 +---
 kernel/events/uprobes.c  |    2 ++
 2 files changed, 3 insertions(+), 3 deletions(-)
arch/x86/kernel/signal.c +1 -3
index b280908..0041e5a 100644
@@ -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)
kernel/events/uprobes.c +2 -0
index 4ea0f0b..14c2e99 100644
@@ -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);
Signature · 1 line
1.5.5.1
Acked-by on v1 5/5
#19 lore raw

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

Quoted 4 lines · 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 <[email protected]>
Acked-by Srikar Dronamraju <[email protected]>
Quoted 37 lines
---
 arch/x86/kernel/signal.c |    4 +---
 kernel/events/uprobes.c  |    2 ++
 2 files changed, 3 insertions(+), 3 deletions(-)
arch/x86/kernel/signal.c +1 -3
index b280908..0041e5a 100644
@@ -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)
kernel/events/uprobes.c +2 -0
index 4ea0f0b..14c2e99 100644
@@ -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);
Signature · 1 line
1.5.5.1