uprobes/x86: introduce uprobe_xol_ops and arch_uprobe->ops

UPROBE_COPY_INSN, UPROBE_SKIP_SSTEP, and uprobe->flags must die. This patch kills UPROBE_SKIP_SSTEP. I never understood why it was added; not only it doesn't help, it harms.

It can only help to avoid arch_uprobe_skip_sstep() if it was already called before and failed. But this is ugly, if we want to know whether we can emulate this instruction or not we should do this analysis in arch_uprobe_analyze_insn(), not when we hit this probe for the first time.

And in fact this logic is simply wrong. arch_uprobe_skip_sstep() can fail or not depending on the task/register state, if this insn can be emulated but, say, put_user() fails we need to xol it this time, but this doesn't mean we shouldn't try to emulate it when this or another thread hist this bp next time.

And this is the actual reason for this change. We need to emulate the "call" insn, but push(return-address) can obviously fail.

Per-arch notes:

x86: __skip_sstep() can only emulate "rep;nop". With this
change it will be called every time and most probably for no reason.

This will be fixed by the next changes. We need to change this suboptimal code anyway.

arm: Should not be affected. It has its own "bool simulate"
flag checked in arch_uprobe_skip_sstep().

ppc: Looks like, it can emulate almost everything. Does it
actually needs to record the fact that emulate_step() failed? Hopefully not. But if yes, it can add the ppc- specific flag into arch_uprobe.

TODO: rename arch_uprobe_skip_sstep() to arch_uprobe_emulate_insn(),

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

---
 kernel/events/uprobes.c |   23 ++---------------------
 1 files changed, 2 insertions(+), 21 deletions(-)

kernel/events/uprobes.c +2 -21

index 307d87c..7a3e14e 100644
@@ -60,8 +60,6 @@ static struct percpu_rw_semaphore dup_mmap_sem;
 
 /* Have a copy of original instruction */
 #define UPROBE_COPY_INSN	0
-/* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP	1
 
 struct uprobe {
 	struct rb_node		rb_node;	/* node in the rb tree */

@@ -491,12 +489,9 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
 	uprobe->offset = offset;
 	init_rwsem(&uprobe->register_rwsem);
 	init_rwsem(&uprobe->consumer_rwsem);
-	/* For now assume that the instruction need not be single-stepped */
-	__set_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
 
 	/* add to uprobes_tree, sorted on inode:offset */
 	cur_uprobe = insert_uprobe(uprobe);
-
 	/* a uprobe exists for this inode:offset combination */
 	if (cur_uprobe) {
 		kfree(uprobe);

@@ -1628,20 +1623,6 @@ bool uprobe_deny_signal(void)
 	return true;
 }
 
-/*
- * Avoid singlestepping the original instruction if the original instruction
- * is a NOP or can be emulated.
- */
-static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)
-{
-	if (test_bit(UPROBE_SKIP_SSTEP, &uprobe->flags)) {
-		if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
-			return true;
-		clear_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
-	}
-	return false;
-}
-
 static void mmf_recalc_uprobes(struct mm_struct *mm)
 {
 	struct vm_area_struct *vma;

@@ -1859,13 +1840,13 @@ static void handle_swbp(struct pt_regs *regs)
 		goto out;
 
 	handler_chain(uprobe, regs);
-	if (can_skip_sstep(uprobe, regs))
+	if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
 		goto out;
 
 	if (!pre_ssout(uprobe, regs, bp_vaddr))
 		return;
 
-	/* can_skip_sstep() succeeded, or restart if can't singlestep */
+	/* arch_uprobe_skip_sstep() succeeded, or restart if can't singlestep */
 out:
 	put_uprobe(uprobe);
 }

UPROBE_COPY_INSN, UPROBE_SKIP_SSTEP, and uprobe->flags must die. This patch kills UPROBE_SKIP_SSTEP. I never understood why it was added; not only it doesn't help, it harms.

It can only help to avoid arch_uprobe_skip_sstep() if it was already called before and failed. But this is ugly, if we want to know whether we can emulate this instruction or not we should do this analysis in arch_uprobe_analyze_insn(), not when we hit this probe for the first time.

And in fact this logic is simply wrong. arch_uprobe_skip_sstep() can fail or not depending on the task/register state, if this insn can be emulated but, say, put_user() fails we need to xol it this time, but this doesn't mean we shouldn't try to emulate it when this or another thread hist this bp next time.

And this is the actual reason for this change. We need to emulate the "call" insn, but push(return-address) can obviously fail.

Per-arch notes:

x86: __skip_sstep() can only emulate "rep;nop". With this
change it will be called every time and most probably for no reason.

This will be fixed by the next changes. We need to change this suboptimal code anyway.

arm: Should not be affected. It has its own "bool simulate"
flag checked in arch_uprobe_skip_sstep().

ppc: Looks like, it can emulate almost everything. Does it
actually needs to record the fact that emulate_step() failed? Hopefully not. But if yes, it can add the ppc- specific flag into arch_uprobe.

TODO: rename arch_uprobe_skip_sstep() to arch_uprobe_emulate_insn(),

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

---
  kernel/events/uprobes.c |   23 ++---------------------
  1 files changed, 2 insertions(+), 21 deletions(-)

kernel/events/uprobes.c +1 -20

index 307d87c..7a3e14e 100644
@@ -60,8 +60,6 @@ static struct percpu_rw_semaphore dup_mmap_sem;
 
  /* Have a copy of original instruction */
  #define UPROBE_COPY_INSN	0
-/* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP	1
 
  struct uprobe {
  	struct rb_node		rb_node;	/* node in the rb tree */

@@ -491,12 +489,9 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
  	uprobe->offset = offset;
  	init_rwsem(&uprobe->register_rwsem);
  	init_rwsem(&uprobe->consumer_rwsem);
-	/* For now assume that the instruction need not be single-stepped */
-	__set_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
 
  	/* add to uprobes_tree, sorted on inode:offset */
  	cur_uprobe = insert_uprobe(uprobe);
-
  	/* a uprobe exists for this inode:offset combination */
  	if (cur_uprobe) {
  		kfree(uprobe);

@@ -1628,20 +1623,6 @@ bool uprobe_deny_signal(void)
  	return true;
  }
 
-/*
- * Avoid singlestepping the original instruction if the original instruction
- * is a NOP or can be emulated.
- */
-static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)
-{
-	if (test_bit(UPROBE_SKIP_SSTEP, &uprobe->flags)) {
-		if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
-			return true;
-		clear_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
-	}
-	return false;
-}
-
  static void mmf_recalc_uprobes(struct mm_struct *mm)
  {
  	struct vm_area_struct *vma;

@@ -1859,13 +1840,13 @@ static void handle_swbp(struct pt_regs *regs)
  		goto out;
 
  	handler_chain(uprobe, regs);
-	if (can_skip_sstep(uprobe, regs))
+	if (arch_uprobe_skip_sstep(&uprobe->arch, regs))

UPROBE_COPY_INSN, UPROBE_SKIP_SSTEP, and uprobe->flags must die. This patch kills UPROBE_SKIP_SSTEP. I never understood why it was added; not only it doesn't help, it harms.

It can only help to avoid arch_uprobe_skip_sstep() if it was already called before and failed. But this is ugly, if we want to know whether we can emulate this instruction or not we should do this analysis in arch_uprobe_analyze_insn(), not when we hit this probe for the first time.

And in fact this logic is simply wrong. arch_uprobe_skip_sstep() can fail or not depending on the task/register state, if this insn can be emulated but, say, put_user() fails we need to xol it this time, but this doesn't mean we shouldn't try to emulate it when this or another thread hist this bp next time.

And this is the actual reason for this change. We need to emulate the "call" insn, but push(return-address) can obviously fail.

Per-arch notes:

x86: __skip_sstep() can only emulate "rep;nop". With this
change it will be called every time and most probably for no reason.

This will be fixed by the next changes. We need to change this suboptimal code anyway.

arm: Should not be affected. It has its own "bool simulate"
flag checked in arch_uprobe_skip_sstep().

ppc: Looks like, it can emulate almost everything. Does it
actually needs to record the fact that emulate_step() failed? Hopefully not. But if yes, it can add the ppc- specific flag into arch_uprobe.

TODO: rename arch_uprobe_skip_sstep() to arch_uprobe_emulate_insn(),

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

No functional changes, preparation.

Shift the code from prepare_fixups() to arch_uprobe_analyze_insn() with the following modifications:

- Do not call insn_get_opcode() again, it was already called by validate_insn_bits().

- Move "case 0xea" up. This way "case 0xff" can fall through to default case.

- change "case 0xff" to use the nested "switch (MODRM_REG)", this way the code looks a bit simpler.

- Make the comments look consistent.

While at it, kill the initialization of rip_rela_target_address and ->fixups, we can rely on kzalloc(). We will add the new members into arch_uprobe, it would be better to assume that everything is zero by default.

TODO: cleanup/fix the mess in validate_insn_bits() paths:

- validate_insn_64bits() and validate_insn_32bits() should be unified.

- "ifdef" is not used consistently; if good_insns_64 depends on CONFIG_X86_64, then probably good_insns_32 should depend on CONFIG_X86_32/EMULATION

- the usage of mm->context.ia32_compat looks wrong if the task is TIF_X32.

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

---
 arch/x86/kernel/uprobes.c |  110 +++++++++++++++++++--------------------------
 1 files changed, 47 insertions(+), 63 deletions(-)

arch/x86/kernel/uprobes.c +47 -63

index 2ed8459..098e56e 100644
@@ -53,7 +53,7 @@
 #define OPCODE1(insn)		((insn)->opcode.bytes[0])
 #define OPCODE2(insn)		((insn)->opcode.bytes[1])
 #define OPCODE3(insn)		((insn)->opcode.bytes[2])
-#define MODRM_REG(insn)		X86_MODRM_REG(insn->modrm.value)
+#define MODRM_REG(insn)		X86_MODRM_REG((insn)->modrm.value)
 
 #define W(row, b0, b1, b2, b3, b4, b5, b6, b7, b8, b9, ba, bb, bc, bd, be, bf)\
 	(((b0##UL << 0x0)|(b1##UL << 0x1)|(b2##UL << 0x2)|(b3##UL << 0x3) |   \

@@ -229,63 +229,6 @@ static int validate_insn_32bits(struct arch_uprobe *auprobe, struct insn *insn)
 	return -ENOTSUPP;
 }
 
-/*
- * Figure out which fixups arch_uprobe_post_xol() will need to perform, and
- * annotate arch_uprobe->fixups accordingly.  To start with,
- * arch_uprobe->fixups is either zero or it reflects rip-related fixups.
- */
-static void prepare_fixups(struct arch_uprobe *auprobe, struct insn *insn)
-{
-	bool fix_ip = true, fix_call = false;	/* defaults */
-	int reg;
-
-	insn_get_opcode(insn);	/* should be a nop */
-
-	switch (OPCODE1(insn)) {
-	case 0x9d:
-		/* popf */
-		auprobe->fixups |= UPROBE_FIX_SETF;
-		break;
-	case 0xc3:		/* ret/lret */
-	case 0xcb:
-	case 0xc2:
-	case 0xca:
-		/* ip is correct */
-		fix_ip = false;
-		break;
-	case 0xe8:		/* call relative - Fix return addr */
-		fix_call = true;
-		break;
-	case 0x9a:		/* call absolute - Fix return addr, not ip */
-		fix_call = true;
-		fix_ip = false;
-		break;
-	case 0xff:
-		insn_get_modrm(insn);
-		reg = MODRM_REG(insn);
-		if (reg == 2 || reg == 3) {
-			/* call or lcall, indirect */
-			/* Fix return addr; ip is correct. */
-			fix_call = true;
-			fix_ip = false;
-		} else if (reg == 4 || reg == 5) {
-			/* jmp or ljmp, indirect */
-			/* ip is correct. */
-			fix_ip = false;
-		}
-		break;
-	case 0xea:		/* jmp absolute -- ip is correct */
-		fix_ip = false;
-		break;
-	default:
-		break;
-	}
-	if (fix_ip)
-		auprobe->fixups |= UPROBE_FIX_IP;
-	if (fix_call)
-		auprobe->fixups |= UPROBE_FIX_CALL;
-}
-
 #ifdef CONFIG_X86_64
 /*
  * If arch_uprobe->insn doesn't use rip-relative addressing, return

@@ -318,7 +261,6 @@ handle_riprel_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, struct ins
 	if (mm->context.ia32_compat)
 		return;
 
-	auprobe->rip_rela_target_address = 0x0;
 	if (!insn_rip_relative(insn))
 		return;
 

@@ -421,16 +363,58 @@ static int validate_insn_bits(struct arch_uprobe *auprobe, struct mm_struct *mm,
  */
 int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long addr)
 {
-	int ret;
 	struct insn insn;
+	bool fix_ip = true, fix_call = false;
+	int ret;
 
-	auprobe->fixups = 0;
 	ret = validate_insn_bits(auprobe, mm, &insn);
-	if (ret != 0)
+	if (ret)
 		return ret;
 
+	/*
+	 * Figure out which fixups arch_uprobe_post_xol() will need to perform,
+	 * and annotate arch_uprobe->fixups accordingly. To start with, ->fixups
+	 * is either zero or it reflects rip-related fixups.
+	 */
 	handle_riprel_insn(auprobe, mm, &insn);
-	prepare_fixups(auprobe, &insn);
+
+	switch (OPCODE1(&insn)) {
+	case 0x9d:		/* popf */
+		auprobe->fixups |= UPROBE_FIX_SETF;
+		break;
+	case 0xc3:		/* ret or lret -- ip is correct */
+	case 0xcb:
+	case 0xc2:
+	case 0xca:
+		fix_ip = false;
+		break;
+	case 0xe8:		/* call relative - Fix return addr */
+		fix_call = true;
+		break;
+	case 0x9a:		/* call absolute - Fix return addr, not ip */
+		fix_call = true;
+		fix_ip = false;
+		break;
+	case 0xea:		/* jmp absolute -- ip is correct */
+		fix_ip = false;
+		break;
+	case 0xff:
+		insn_get_modrm(&insn);
+		switch (MODRM_REG(&insn)) {
+		case 2: case 3:			/* call or lcall, indirect */
+			fix_call = true;
+		case 4: case 5:			/* jmp or ljmp, indirect */
+			fix_ip = false;
+		}
+		break;
+	default:
+		break;
+	}
+
+	if (fix_ip)
+		auprobe->fixups |= UPROBE_FIX_IP;
+	if (fix_call)
+		auprobe->fixups |= UPROBE_FIX_CALL;
 
 	return 0;
 }

No functional changes, preparation.

Shift the code from prepare_fixups() to arch_uprobe_analyze_insn() with the following modifications:

- Do not call insn_get_opcode() again, it was already called by validate_insn_bits().

- Move "case 0xea" up. This way "case 0xff" can fall through to default case.

- change "case 0xff" to use the nested "switch (MODRM_REG)", this way the code looks a bit simpler.

- Make the comments look consistent.

While at it, kill the initialization of rip_rela_target_address and ->fixups, we can rely on kzalloc(). We will add the new members into arch_uprobe, it would be better to assume that everything is zero by default.

TODO: cleanup/fix the mess in validate_insn_bits() paths:

- validate_insn_64bits() and validate_insn_32bits() should be unified.

- "ifdef" is not used consistently; if good_insns_64 depends on CONFIG_X86_64, then probably good_insns_32 should depend on CONFIG_X86_32/EMULATION

- the usage of mm->context.ia32_compat looks wrong if the task is TIF_X32.

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

arch_uprobe_analyze_insn() calls handle_riprel_insn() at the start, but only "0xff" and "default" cases need the UPROBE_FIX_RIP_ logic. Move the callsite into "default" case and change the "0xff" case to fall-through.

We are going to add the various hooks to handle the rip-relative jmp/call instructions (and more), we need this change to enforce the fact that the new code can't conflict with is_riprel_insn() code.

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

---
 arch/x86/kernel/uprobes.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

arch/x86/kernel/uprobes.c +2 -4

index 098e56e..d72dfbf 100644
@@ -376,8 +376,6 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	 * and annotate arch_uprobe->fixups accordingly. To start with, ->fixups
 	 * is either zero or it reflects rip-related fixups.
 	 */
-	handle_riprel_insn(auprobe, mm, &insn);
-
 	switch (OPCODE1(&insn)) {
 	case 0x9d:		/* popf */
 		auprobe->fixups |= UPROBE_FIX_SETF;

@@ -406,9 +404,9 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 		case 4: case 5:			/* jmp or ljmp, indirect */
 			fix_ip = false;
 		}
-		break;
+		/* fall through */
 	default:
-		break;
+		handle_riprel_insn(auprobe, mm, &insn);
 	}
 
 	if (fix_ip)

Quoted 9 lines · Oleg Nesterov

arch_uprobe_analyze_insn() calls handle_riprel_insn() at the start, but only "0xff" and "default" cases need the UPROBE_FIX_RIP_ logic. Move the callsite into "default" case and change the "0xff" case to fall-through.

We are going to add the various hooks to handle the rip-relative jmp/call instructions (and more), we need this change to enforce the fact that the new code can't conflict with is_riprel_insn() code.

Quoted 1 line

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

Hmm, this seems not obviously reasonable at this point. However, the code itself is not wrong. Could you merge this change to that new hooks?

Good point, I'll send v2.

Quoted 2 lines · Oleg Nesterov

Good point, I'll send v2.

Masami, et al, I was going to send v2 plus the next short RFC series which fixes the problem today, but it turns out that I have no time. Will try to do this tomorrow.

So let me explain the problem, and how (I think) it should be solved. Unfortunately, I do not even know the terminology, so firstly I have to explain you the things I recently learned when I investigated the bug report ;)

Once again, if this can work we need more changes to handle jmp's/etc. But lets discuss this later. I am thinking in horror about conditional jmp ;) In fact this should be simple, just I do not know (yet) to parse such an insn, and I simply do not know if lib/insn.c can help me to figure out which flag in regs->flags ->emulate() should check.

So this all looks a bit complicated, but I do not see a simpler solution. Well, we could avoid ->emulate() altogether, we could just mangle the problematic instructions and complicate arch_uprobe_post_xol(), but I do not think this would be better. And if nothing else, ->emulate is a) simple and b) should likely succeed and make the probing faster.

But perhaps I missed something?

Oleg.

Quoted 5 lines · Oleg Nesterov

So let me explain the problem, and how (I think) it should be solved. Unfortunately, I do not even know the terminology, so firstly I have to explain you the things I recently learned when I investigated the bug report ;)

[problem description and proposed solution snipped]

Thanks for your work on this. I think your analysis is correct.

As you
say, emulating calls is tricky because of the possibility that the call will incur a page fault when it grows the stack. Your best solution might be to emulate jumps,

but rewrite call instructions using a scratch register, similar to how we handle rip-relative instructions.

Quoted 5 lines

Once again, if this can work we need more changes to handle jmp's/etc. But lets discuss this later. I am thinking in horror about conditional jmp ;) In fact this should be simple, just I do not know (yet) to parse such an insn, and I simply do not know if lib/insn.c can help me to figure out which flag in regs->flags ->emulate() should check.

Emulating jumps (including conditional jumps) shouldn't be all that much code. In case you haven't already found it, the "AMD64 Architecture Programmer's Manual, Volume 3" provides the sort of info you need.

One thing about emulating jumps is that if the task has block stepping enabled, then a trap is expected on every successful branch.

Kill the "mm->context.ia32_compat" check in handle_riprel_insn(), if it is true insn_rip_relative() must return false. validate_insn_bits() passed "ia32_compat" as !x86_64 to insn_init(), and insn_rip_relative() checks insn->x86_64.

Also, remove the no longer needed "struct mm_struct *mm" argument and the unnecessary "return" at the end.

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

---
 arch/x86/kernel/uprobes.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

arch/x86/kernel/uprobes.c +3 -7

index d72dfbf..382a69d 100644
@@ -253,14 +253,11 @@ static int validate_insn_32bits(struct arch_uprobe *auprobe, struct insn *insn)
  *  - The displacement is always 4 bytes.
  */
 static void
-handle_riprel_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, struct insn *insn)
+handle_riprel_insn(struct arch_uprobe *auprobe, struct insn *insn)
 {
 	u8 *cursor;
 	u8 reg;
 
-	if (mm->context.ia32_compat)
-		return;
-
 	if (!insn_rip_relative(insn))
 		return;
 

@@ -314,7 +311,6 @@ handle_riprel_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, struct ins
 		cursor++;
 		memmove(cursor, cursor + insn->displacement.nbytes, insn->immediate.nbytes);
 	}
-	return;
 }
 
 static int validate_insn_64bits(struct arch_uprobe *auprobe, struct insn *insn)

@@ -343,7 +339,7 @@ static int validate_insn_bits(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	return validate_insn_64bits(auprobe, insn);
 }
 #else /* 32-bit: */
-static void handle_riprel_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, struct insn *insn)
+static void handle_riprel_insn(struct arch_uprobe *auprobe, struct insn *insn)
 {
 	/* No RIP-relative addressing on 32-bit */
 }

@@ -406,7 +402,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 		}
 		/* fall through */
 	default:
-		handle_riprel_insn(auprobe, mm, &insn);
+		handle_riprel_insn(auprobe, &insn);
 	}
 
 	if (fix_ip)

Kill the "mm->context.ia32_compat" check in handle_riprel_insn(), if it is true insn_rip_relative() must return false. validate_insn_bits() passed "ia32_compat" as !x86_64 to insn_init(), and insn_rip_relative() checks insn->x86_64.

Also, remove the no longer needed "struct mm_struct *mm" argument and the unnecessary "return" at the end.

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

Cosmetic. Move pre_xol_rip_insn() and handle_riprel_post_xol() up to the closely related handle_riprel_insn().

This way it is simpler to read and understand this code, and this lessens the number of ifdef's.

TODO: rename them somehow to make the naming consistent.

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

---
 arch/x86/kernel/uprobes.c |  118 ++++++++++++++++++++-------------------------
 1 files changed, 53 insertions(+), 65 deletions(-)

arch/x86/kernel/uprobes.c +53 -65

index 382a69d..e8d7b38 100644
@@ -313,6 +313,48 @@ handle_riprel_insn(struct arch_uprobe *auprobe, struct insn *insn)
 	}
 }
 
+/*
+ * If we're emulating a rip-relative instruction, save the contents
+ * of the scratch register and store the target address in that register.
+ */
+static void
+pre_xol_rip_insn(struct arch_uprobe *auprobe, struct pt_regs *regs,
+				struct arch_uprobe_task *autask)
+{
+	if (auprobe->fixups & UPROBE_FIX_RIP_AX) {
+		autask->saved_scratch_register = regs->ax;
+		regs->ax = current->utask->vaddr;
+		regs->ax += auprobe->rip_rela_target_address;
+	} else if (auprobe->fixups & UPROBE_FIX_RIP_CX) {
+		autask->saved_scratch_register = regs->cx;
+		regs->cx = current->utask->vaddr;
+		regs->cx += auprobe->rip_rela_target_address;
+	}
+}
+
+static void
+handle_riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs, long *correction)
+{
+	if (auprobe->fixups & (UPROBE_FIX_RIP_AX | UPROBE_FIX_RIP_CX)) {
+		struct arch_uprobe_task *autask;
+
+		autask = &current->utask->autask;
+		if (auprobe->fixups & UPROBE_FIX_RIP_AX)
+			regs->ax = autask->saved_scratch_register;
+		else
+			regs->cx = autask->saved_scratch_register;
+
+		/*
+		 * The original instruction includes a displacement, and so
+		 * is 4 bytes longer than what we've just single-stepped.
+		 * Fall through to handle stuff like "jmpq *...(%rip)" and
+		 * "callq *...(%rip)".
+		 */
+		if (correction)
+			*correction += 4;
+	}
+}
+
 static int validate_insn_64bits(struct arch_uprobe *auprobe, struct insn *insn)
 {
 	insn_init(insn, auprobe->insn, true);

@@ -339,9 +381,19 @@ static int validate_insn_bits(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	return validate_insn_64bits(auprobe, insn);
 }
 #else /* 32-bit: */
+/*
+ * No RIP-relative addressing on 32-bit
+ */
 static void handle_riprel_insn(struct arch_uprobe *auprobe, struct insn *insn)
 {
-	/* No RIP-relative addressing on 32-bit */
+}
+static void pre_xol_rip_insn(struct arch_uprobe *auprobe, struct pt_regs *regs,
+				struct arch_uprobe_task *autask)
+{
+}
+static void handle_riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs,
+					long *correction)
+{
 }
 
 static int validate_insn_bits(struct arch_uprobe *auprobe, struct mm_struct *mm,  struct insn *insn)

@@ -413,34 +465,6 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	return 0;
 }
 
-#ifdef CONFIG_X86_64
-/*
- * If we're emulating a rip-relative instruction, save the contents
- * of the scratch register and store the target address in that register.
- */
-static void
-pre_xol_rip_insn(struct arch_uprobe *auprobe, struct pt_regs *regs,
-				struct arch_uprobe_task *autask)
-{
-	if (auprobe->fixups & UPROBE_FIX_RIP_AX) {
-		autask->saved_scratch_register = regs->ax;
-		regs->ax = current->utask->vaddr;
-		regs->ax += auprobe->rip_rela_target_address;
-	} else if (auprobe->fixups & UPROBE_FIX_RIP_CX) {
-		autask->saved_scratch_register = regs->cx;
-		regs->cx = current->utask->vaddr;
-		regs->cx += auprobe->rip_rela_target_address;
-	}
-}
-#else
-static void
-pre_xol_rip_insn(struct arch_uprobe *auprobe, struct pt_regs *regs,
-				struct arch_uprobe_task *autask)
-{
-	/* No RIP-relative addressing on 32-bit */
-}
-#endif
-
 /*
  * arch_uprobe_pre_xol - prepare to execute out of line.
  * @auprobe: the probepoint information.

@@ -490,42 +514,6 @@ static int adjust_ret_addr(unsigned long sp, long correction)
 	return 0;
 }
 
-#ifdef CONFIG_X86_64
-static bool is_riprel_insn(struct arch_uprobe *auprobe)
-{
-	return ((auprobe->fixups & (UPROBE_FIX_RIP_AX | UPROBE_FIX_RIP_CX)) != 0);
-}
-
-static void
-handle_riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs, long *correction)
-{
-	if (is_riprel_insn(auprobe)) {
-		struct arch_uprobe_task *autask;
-
-		autask = &current->utask->autask;
-		if (auprobe->fixups & UPROBE_FIX_RIP_AX)
-			regs->ax = autask->saved_scratch_register;
-		else
-			regs->cx = autask->saved_scratch_register;
-
-		/*
-		 * The original instruction includes a displacement, and so
-		 * is 4 bytes longer than what we've just single-stepped.
-		 * Fall through to handle stuff like "jmpq *...(%rip)" and
-		 * "callq *...(%rip)".
-		 */
-		if (correction)
-			*correction += 4;
-	}
-}
-#else
-static void
-handle_riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs, long *correction)
-{
-	/* No RIP-relative addressing on 32-bit */
-}
-#endif
-
 /*
  * If xol insn itself traps and generates a signal(Say,
  * SIGILL/SIGSEGV/etc), then detect the case where a singlestepped

arch/x86/kernel/uprobes.c +12 -0

+static void
+handle_riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs, long *correction)
+{
+	if (auprobe->fixups & (UPROBE_FIX_RIP_AX | UPROBE_FIX_RIP_CX)) {
+		struct arch_uprobe_task *autask;
+
+		autask = &current->utask->autask;
+		if (auprobe->fixups & UPROBE_FIX_RIP_AX)
+			regs->ax = autask->saved_scratch_register;
+		else
+			regs->cx = autask->saved_scratch_register;
+

On Mon, 2014-03-31 at 21:44 +0200, Oleg Nesterov wrote:
...

Quoted 20 lines

arch/x86/kernel/uprobes.c +20 -0

+static void
+handle_riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs, long *correction)
+{
+	if (auprobe->fixups & (UPROBE_FIX_RIP_AX | UPROBE_FIX_RIP_CX)) {
+		struct arch_uprobe_task *autask;
+
+		autask = &current->utask->autask;
+		if (auprobe->fixups & UPROBE_FIX_RIP_AX)
+			regs->ax = autask->saved_scratch_register;
+		else
+			regs->cx = autask->saved_scratch_register;
+
+		/*
+		 * The original instruction includes a displacement, and so
+		 * is 4 bytes longer than what we've just single-stepped.
+		 * Fall through to handle stuff like "jmpq *...(%rip)" and
+		 * "callq *...(%rip)".
+		 */
+		if (correction)
+			*correction += 4;

Review

Quoted 2 lines

(quoted diff) +2 -0

+	}
+}

This is another case of a glitch in the code being moved. Since this code was moved to its own function, the "Fall through" comment makes no sense. Maybe
* Caller may need to apply other fixups to handle stuff
* like "jmpq *...(%rip)" and "callq *...(%rip)".

No functional changes. Preparation to simplify the review of the next change. Just reorder the code in arch_uprobe_pre/post_xol() functions so that UPROBE_FIX_{RIP_*,IP,CALL} logic goes to the end.

Also change arch_uprobe_pre_xol() to use utask instead of autask, to make the code more symmetrical with arch_uprobe_post_xol().

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

---
 arch/x86/kernel/uprobes.c |   30 ++++++++++++++----------------
 1 files changed, 14 insertions(+), 16 deletions(-)

arch/x86/kernel/uprobes.c +14 -16

index e8d7b38..7dd65dc 100644
@@ -472,19 +472,18 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
  */
 int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
-	struct arch_uprobe_task *autask;
+	struct uprobe_task *utask = current->utask;
 
-	autask = &current->utask->autask;
-	autask->saved_trap_nr = current->thread.trap_nr;
+	regs->ip = utask->xol_vaddr;
+	utask->autask.saved_trap_nr = current->thread.trap_nr;
 	current->thread.trap_nr = UPROBE_TRAP_NR;
-	regs->ip = current->utask->xol_vaddr;
-	pre_xol_rip_insn(auprobe, regs, autask);
 
-	autask->saved_tf = !!(regs->flags & X86_EFLAGS_TF);
+	utask->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);
 
+	pre_xol_rip_insn(auprobe, regs, &utask->autask);
 	return 0;
 }
 

@@ -558,22 +557,13 @@ bool arch_uprobe_xol_was_trapped(struct task_struct *t)
  */
 int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
-	struct uprobe_task *utask;
+	struct uprobe_task *utask = current->utask;
 	long correction;
 	int result = 0;
 
 	WARN_ON_ONCE(current->thread.trap_nr != UPROBE_TRAP_NR);
 
-	utask = current->utask;
 	current->thread.trap_nr = utask->autask.saved_trap_nr;
-	correction = (long)(utask->vaddr - utask->xol_vaddr);
-	handle_riprel_post_xol(auprobe, regs, &correction);
-	if (auprobe->fixups & UPROBE_FIX_IP)
-		regs->ip += correction;
-
-	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

@@ -584,6 +574,14 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	else if (!(auprobe->fixups & UPROBE_FIX_SETF))
 		regs->flags &= ~X86_EFLAGS_TF;
 
+	correction = (long)(utask->vaddr - utask->xol_vaddr);
+	handle_riprel_post_xol(auprobe, regs, &correction);
+	if (auprobe->fixups & UPROBE_FIX_IP)
+		regs->ip += correction;
+
+	if (auprobe->fixups & UPROBE_FIX_CALL)
+		result = adjust_ret_addr(regs->sp, correction);
+
 	return result;
 }
 

No functional changes. Preparation to simplify the review of the next change. Just reorder the code in arch_uprobe_pre/post_xol() functions so that UPROBE_FIX_{RIP_*,IP,CALL} logic goes to the end.

Also change arch_uprobe_pre_xol() to use utask instead of autask, to make the code more symmetrical with arch_uprobe_post_xol().

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

Introduce arch_uprobe->ops pointing to the "struct uprobe_xol_ops", move the current UPROBE_FIX_{RIP*,IP,CALL} code into the default set of methods and change arch_uprobe_pre/post_xol() accordingly.

This way we can add the new uprobe_xol_ops's to handle the insns which need the special processing (rip-relative jmp/call at least).

TODO: An error from adjust_ret_addr() shouldn't be silently ignored, we should teach arch_uprobe_post_xol() or handle_singlestep() paths to restart the probed insn in this case. And probably "adjust" can be simplified and turned into set_ret_addr(). It seems that we do not really need copy_from_user(), we can always calculate the value we need to write into *regs->sp.

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

---
 arch/x86/include/asm/uprobes.h |    7 ++-
 arch/x86/kernel/uprobes.c      |  107 +++++++++++++++++++++++++--------------
 2 files changed, 74 insertions(+), 40 deletions(-)

arch/x86/include/asm/uprobes.h +6 -1

index 3087ea9..9f8210b 100644
@@ -33,12 +33,17 @@ typedef u8 uprobe_opcode_t;
 #define UPROBE_SWBP_INSN		0xcc
 #define UPROBE_SWBP_INSN_SIZE		   1
 
+struct uprobe_xol_ops;
+
 struct arch_uprobe {
-	u16				fixups;
 	union {
 		u8			insn[MAX_UINSN_BYTES];
 		u8			ixol[MAX_UINSN_BYTES];
 	};
+
+	u16				fixups;
+	const struct uprobe_xol_ops	*ops;
+
 #ifdef CONFIG_X86_64
 	unsigned long			rip_rela_target_address;
 #endif

arch/x86/kernel/uprobes.c +46 -0

index 7dd65dc..a822de5 100644
@@ -402,6 +402,64 @@ static int validate_insn_bits(struct arch_uprobe *auprobe, struct mm_struct *mm,
 }
 #endif /* CONFIG_X86_64 */
 
+struct uprobe_xol_ops {
+	bool	(*emulate)(struct arch_uprobe *, struct pt_regs *);
+	int	(*pre_xol)(struct arch_uprobe *, struct pt_regs *);
+	int	(*post_xol)(struct arch_uprobe *, struct pt_regs *);
+};
+
+static int default_pre_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	pre_xol_rip_insn(auprobe, regs, &current->utask->autask);
+	return 0;
+}
+
+/*
+ * Adjust the return address pushed by a call insn executed out of line.
+ */
+static int adjust_ret_addr(unsigned long sp, long correction)
+{
+	int rasize, ncopied;
+	long ra = 0;
+
+	if (is_ia32_task())
+		rasize = 4;
+	else
+		rasize = 8;
+
+	ncopied = copy_from_user(&ra, (void __user *)sp, rasize);
+	if (unlikely(ncopied))
+		return -EFAULT;
+
+	ra += correction;
+	ncopied = copy_to_user((void __user *)sp, &ra, rasize);
+	if (unlikely(ncopied))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;
+	long correction = (long)(utask->vaddr - utask->xol_vaddr);
+	int ret = 0;
+
+	handle_riprel_post_xol(auprobe, regs, &correction);
+	if (auprobe->fixups & UPROBE_FIX_IP)
+		regs->ip += correction;

arch/x86/kernel/uprobes.c static int validate_insn_bits(struct arch_uprobe *auprobe, struct mm_struct *mm, · line 460

Quoted 4 lines · Oleg Nesterov

arch/x86/kernel/uprobes.c +4 -0

+
+static struct uprobe_xol_ops defaule_xop_ops = {
+	.pre_xol  = default_pre_xol_op,
+	.post_xol = default_post_xol_op,

If there is no ops->emulate, I think it should not be defined now. You can add it when you really need it. :)

arch/x86/kernel/uprobes.c +4 -0

+static struct uprobe_xol_ops defaule_xop_ops = {
+	.pre_xol  = default_pre_xol_op,
+	.post_xol = default_post_xol_op,
+};

arch/x86/kernel/uprobes.c +14 -0

+/*
+ * Adjust the return address pushed by a call insn executed out of line.
+ */
+static int adjust_ret_addr(unsigned long sp, long correction)
+{
+	int rasize, ncopied;
+	long ra = 0;
+
+	if (is_ia32_task())
+		rasize = 4;
+	else
+		rasize = 8;
+
+	ncopied = copy_from_user(&ra, (void __user *)sp, rasize);

On Mon, 2014-03-31 at 21:44 +0200, Oleg Nesterov wrote:
...

Quoted 14 lines

arch/x86/kernel/uprobes.c +14 -0

+/*
+ * Adjust the return address pushed by a call insn executed out of line.
+ */
+static int adjust_ret_addr(unsigned long sp, long correction)
+{
+	int rasize, ncopied;
+	long ra = 0;
+
+	if (is_ia32_task())
+		rasize = 4;
+	else
+		rasize = 8;
+
+	ncopied = copy_from_user(&ra, (void __user *)sp, rasize);

arch/x86/kernel/uprobes.c static int validate_insn_bits(struct arch_uprobe *auprobe, struct mm_struct *mm, · line 440

Quoted 10 lines

arch/x86/kernel/uprobes.c +10 -0

+	if (unlikely(ncopied))
+		return -EFAULT;
+
+	ra += correction;
+	ncopied = copy_to_user((void __user *)sp, &ra, rasize);
+	if (unlikely(ncopied))
+		return -EFAULT;
+
+	return 0;
+}

This isn't your bug, Oleg -- you're just moving code -- but consider taking this opportunity to fix it...

"ncopied" is a misnomer here. copy_from_user() and copy_to_user() return the number of bytes that could NOT be copied.

Hello.

x86 can not handle the rip-relative jmp/call instrsuctions, the probed task can be killed by general protection fault. I'll describe this in more details when I send the fixes. Now I am sending the preparations which (I hope) make sense anyway, please review.

My main concern is 3/7. I know absolutely nothing about instruction decoding, so I can only guess what, for example, OPCODE1() == 0xff or MODRM_REG() == 2 actually means. Please review.

Ananth, David, please ack/nack the first change, it affects powerpc/arm.

Oleg.

 arch/x86/include/asm/uprobes.h |    7 +-
 arch/x86/kernel/uprobes.c      |  343 ++++++++++++++++++++--------------------
 kernel/events/uprobes.c        |   23 +---
 3 files changed, 176 insertions(+), 197 deletions(-)

when I send the fixes.