[PATCH 0/7] uprobes: register/unregister bugfixes

[PATCH 0/7] uprobes: register/unregister bugfixes

[Oleg Nesterov]

Hello.

Misc bugfixes, mostly register/unregister related.

Note: 4/7 and especially 6/7 are the really ugly (but hopefully
temporary) hacks. We already discussed this, we should inspect
the original insn in uprobe_register(). But this is not easy, and
we need something simple to fix (some of) the bugs right now.

Even 7/7 is ugly. Imho UPROBE_SKIP_SSTEP should die as well but
this is another story.

Oleg.

 arch/x86/kernel/uprobes.c |   16 +------
 include/linux/uprobes.h   |   10 ----
 kernel/events/uprobes.c   |  107 +++++++++++++++++++++++++++++++-------------
 3 files changed, 77 insertions(+), 56 deletions(-)

[PATCH 1/7] uprobes/x86: Only rep+nop can be emulated correctly

[Oleg Nesterov]

__skip_sstep() correctly detects the "nontrivial" nop insns,
but since it doesn't update regs->ip we can not really skip
"0x0f 0x1f | 0x0f 0x19 | 0x87 0xc0", the probed application
is killed by SIGILL'ed handle_swbp().

Remove these additional checks. If we want to implement this
correctly we need to know the full insn length to update ->ip.

rep* + nop is fine even without updating ->ip.

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

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 9538f00..aafa555 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -651,31 +651,19 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 
 /*
  * Skip these instructions as per the currently known x86 ISA.
- * 0x66* { 0x90 | 0x0f 0x1f | 0x0f 0x19 | 0x87 0xc0 }
+ * rep=0x66*; nop=0x90
  */
 static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
 	int i;
 
 	for (i = 0; i < MAX_UINSN_BYTES; i++) {
-		if ((auprobe->insn[i] == 0x66))
+		if (auprobe->insn[i] == 0x66)
 			continue;
 
 		if (auprobe->insn[i] == 0x90)
 			return true;
 
-		if (i == (MAX_UINSN_BYTES - 1))
-			break;
-
-		if ((auprobe->insn[i] == 0x0f) && (auprobe->insn[i+1] == 0x1f))
-			return true;
-
-		if ((auprobe->insn[i] == 0x0f) && (auprobe->insn[i+1] == 0x19))
-			return true;
-
-		if ((auprobe->insn[i] == 0x87) && (auprobe->insn[i+1] == 0xc0))
-			return true;
-
 		break;
 	}
 	return false;
-- 
1.5.5.1

[PATCH 2/7] uprobes: Don't return success if alloc_uprobe() fails

[Oleg Nesterov]

If alloc_uprobe() fails uprobe_register() should return ENOMEM, not 0.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a741ba7..3ec114c 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -813,7 +813,9 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
 	mutex_lock(uprobes_hash(inode));
 	uprobe = alloc_uprobe(inode, offset);
 
-	if (uprobe && !consumer_add(uprobe, uc)) {
+	if (!uprobe) {
+		ret = -ENOMEM;
+	} else if (!consumer_add(uprobe, uc)) {
 		ret = __uprobe_register(uprobe);
 		if (ret) {
 			uprobe->consumers = NULL;
-- 
1.5.5.1

[PATCH 3/7] uprobes: Do not delete uprobe if uprobe_unregister() fails

[Oleg Nesterov]

delete_uprobe() must not be called if register_for_each_vma(false)
fails to remove all breakpoints, __uprobe_unregister() is correct.
The problem is that register_for_each_vma(false) always returns 0
and thus this logic does not work.

1. Change verify_opcode() to return 0 rather than -EINVAL when
   unregister detects the !is_swbp insn, we can treat this case
   as success and currently unregister paths ignore the error
   code anyway.

2. Change remove_breakpoint() to propagate the error code from
   write_opcode().

3. Change register_for_each_vma(is_register => false) to remove
   as much breakpoints as possible but return non-zero if
   remove_breakpoint() fails at least once.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 3ec114c..6058231 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -203,7 +203,7 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t
 			return 0;
 	} else {
 		if (!is_swbp)		/* unregister: was it changed by us? */
-			return -EINVAL;
+			return 0;
 	}
 
 	return 1;
@@ -616,15 +616,15 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 	return ret;
 }
 
-static void
+static int
 remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr)
 {
 	/* can happen if uprobe_register() fails */
 	if (!test_bit(MMF_HAS_UPROBES, &mm->flags))
-		return;
+		return 0;
 
 	set_bit(MMF_RECALC_UPROBES, &mm->flags);
-	set_orig_insn(&uprobe->arch, mm, vaddr);
+	return set_orig_insn(&uprobe->arch, mm, vaddr);
 }
 
 /*
@@ -740,7 +740,7 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 		struct mm_struct *mm = info->mm;
 		struct vm_area_struct *vma;
 
-		if (err)
+		if (err && is_register)
 			goto free;
 
 		down_write(&mm->mmap_sem);
@@ -756,7 +756,7 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 		if (is_register)
 			err = install_breakpoint(uprobe, mm, vma, info->vaddr);
 		else
-			remove_breakpoint(uprobe, mm, info->vaddr);
+			err |= remove_breakpoint(uprobe, mm, info->vaddr);
 
  unlock:
 		up_write(&mm->mmap_sem);
-- 
1.5.5.1

[PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race

[Oleg Nesterov]

Strictly speaking this race was added by me in 56bb4cf6. However
I think that this bug is just another indication that we should
move copy_insn/uprobe_analyze_insn code from install_breakpoint()
to uprobe_register(), there are a lot of other reasons for that.
Until then, add a hack to close the race.

A task can hit uprobe U1, but before it calls find_uprobe() this
uprobe can be unregistered *AND* another uprobe U2 can be added to
uprobes_tree at the same inode/offset. In this case handle_swbp()
will use the not-fully-initialized U2, in particular its arch.insn
for xol.

Add the additional !UPROBE_COPY_INSN check into find_active_uprobe,
if this flag is not set we simply restart as if the new uprobe was
not inserted yet. This is not very nice, we need barriers, but we
will remove this hack when we change uprobe_register().

Note: with or without this patch install_breakpoint() can race with
itself, yet another reson to kill UPROBE_COPY_INSN altogether. And
even the usage of uprobe->flags is not safe. See the next patches.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 6058231..a81080f 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -596,6 +596,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 		BUG_ON((uprobe->offset & ~PAGE_MASK) +
 				UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
 
+		smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
 		uprobe->flags |= UPROBE_COPY_INSN;
 	}
 
@@ -1391,6 +1392,16 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 	if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
 		mmf_recalc_uprobes(mm);
 	up_read(&mm->mmap_sem);
+	/*
+	 * TODO: move copy_insn/etc into _register and remove this hack.
+	 * After we hit the bp, _unregister + _register can install the
+	 * new and not-yet-analyzed uprobe at the same address, restart.
+	 */
+	smp_rmb(); /* pairs with wmb() in install_breakpoint() */
+	if (uprobe && unlikely(!(uprobe->flags & UPROBE_COPY_INSN))) {
+		uprobe = NULL;
+		*is_swbp = 0;
+	}
 
 	return uprobe;
 }
-- 
1.5.5.1

[PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

[Oleg Nesterov]

Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
from install_breakpoint() into the new helper, uprobe_copy_insn().

And move uprobe->flags defines from uprobes.h to uprobes.c, nobody
else can use them anyway.

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

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 18d839d..2459457 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -35,16 +35,6 @@ struct inode;
 # include <asm/uprobes.h>
 #endif
 
-/* flags that denote/change uprobes behaviour */
-
-/* Have a copy of original instruction */
-#define UPROBE_COPY_INSN	0x1
-
-/* Dont run handlers when first register/ last unregister in progress*/
-#define UPROBE_RUN_HANDLER	0x2
-/* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP	0x4
-
 struct uprobe_consumer {
 	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
 	/*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a81080f..5c0c1b0 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
  */
 static atomic_t uprobe_events = ATOMIC_INIT(0);
 
+/* Have a copy of original instruction */
+#define UPROBE_COPY_INSN	0x1
+/* Dont run handlers when first register/ last unregister in progress*/
+#define UPROBE_RUN_HANDLER	0x2
+/* Can skip singlestep */
+#define UPROBE_SKIP_SSTEP	0x4
+
 struct uprobe {
 	struct rb_node		rb_node;	/* node in the rb tree */
 	atomic_t		ref;
@@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file *filp)
 	return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, uprobe->offset);
 }
 
+static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
+				struct mm_struct *mm, unsigned long vaddr)
+{
+	int ret = 0;
+
+	if (uprobe->flags & UPROBE_COPY_INSN)
+		return ret;
+
+	ret = copy_insn(uprobe, file);
+	if (ret)
+		goto out;
+
+	ret = -ENOTSUPP;
+	if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
+		goto out;
+
+	ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
+	if (ret)
+		goto out;
+
+	/* write_opcode() assumes we don't cross page boundary */
+	BUG_ON((uprobe->offset & ~PAGE_MASK) +
+			UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
+
+	smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
+	uprobe->flags |= UPROBE_COPY_INSN;
+	ret = 0;
+ out:
+	return ret;
+}
+
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 			struct vm_area_struct *vma, unsigned long vaddr)
@@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 	if (!uprobe->consumers)
 		return 0;
 
-	if (!(uprobe->flags & UPROBE_COPY_INSN)) {
-		ret = copy_insn(uprobe, vma->vm_file);
-		if (ret)
-			return ret;
-
-		if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
-			return -ENOTSUPP;
-
-		ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
-		if (ret)
-			return ret;
-
-		/* write_opcode() assumes we don't cross page boundary */
-		BUG_ON((uprobe->offset & ~PAGE_MASK) +
-				UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
-
-		smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
-		uprobe->flags |= UPROBE_COPY_INSN;
-	}
+	ret = uprobe_copy_insn(uprobe, vma->vm_file, mm, vaddr);
+	if (ret)
+		return ret;
 
 	/*
 	 * set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
-- 
1.5.5.1

[PATCH 6/7] uprobes: Fix uprobe_copy_insn() race with itself

[Oleg Nesterov]

install_breakpoint() is called under mm->mmap_sem, this protects
set_swbp() but not uprobe_copy_insn(). Two or more different tasks
can call install_breakpoint()->uprobe_copy_insn() at the same time,
this leads to numerous problems if UPROBE_COPY_INSN is not set.

Just for example, the second copy_insn() can corrupt the already
analyzed/fixuped uprobe->arch.insn and race with handle_swbp().

This patch simply adds uprobe->copy_mutex to serialize this code.
We could probably reuse ->consumer_rwsem, but this would mean that
consumer->handler() can not use mm->mmap_sem, not good.

Note: this is another temporary ugly hack until we move this logic
into uprobe_register().

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 5c0c1b0..8410388 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -89,6 +89,7 @@ struct uprobe {
 	struct rb_node		rb_node;	/* node in the rb tree */
 	atomic_t		ref;
 	struct rw_semaphore	consumer_rwsem;
+	struct mutex		copy_mutex;	/* TODO: kill me and UPROBE_COPY_INSN */
 	struct list_head	pending_list;
 	struct uprobe_consumer	*consumers;
 	struct inode		*inode;		/* Also hold a ref to inode */
@@ -444,6 +445,7 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
 	uprobe->inode = igrab(inode);
 	uprobe->offset = offset;
 	init_rwsem(&uprobe->consumer_rwsem);
+	mutex_init(&uprobe->copy_mutex);
 
 	/* add to uprobes_tree, sorted on inode:offset */
 	cur_uprobe = insert_uprobe(uprobe);
@@ -578,6 +580,10 @@ static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
 	if (uprobe->flags & UPROBE_COPY_INSN)
 		return ret;
 
+	mutex_lock(&uprobe->copy_mutex);
+	if (uprobe->flags & UPROBE_COPY_INSN)
+		goto out;
+
 	ret = copy_insn(uprobe, file);
 	if (ret)
 		goto out;
@@ -598,6 +604,8 @@ static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
 	uprobe->flags |= UPROBE_COPY_INSN;
 	ret = 0;
  out:
+	mutex_unlock(&uprobe->copy_mutex);
+
 	return ret;
 }
 
-- 
1.5.5.1

[PATCH 7/7] uprobes: Fix the racy uprobe->flags manipulation

[Oleg Nesterov]

Multiple threads can manipulate uprobe->flags, this is obviously
unsafe. For example mmap can set UPROBE_COPY_INSN while register
tries to set UPROBE_RUN_HANDLER, the latter can also race with
can_skip_sstep() which clears UPROBE_SKIP_SSTEP.

Change this code to use bitops.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 8410388..3d8c815 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -79,11 +79,11 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
 static atomic_t uprobe_events = ATOMIC_INIT(0);
 
 /* Have a copy of original instruction */
-#define UPROBE_COPY_INSN	0x1
+#define UPROBE_COPY_INSN	0
 /* Dont run handlers when first register/ last unregister in progress*/
-#define UPROBE_RUN_HANDLER	0x2
+#define UPROBE_RUN_HANDLER	1
 /* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP	0x4
+#define UPROBE_SKIP_SSTEP	2
 
 struct uprobe {
 	struct rb_node		rb_node;	/* node in the rb tree */
@@ -94,7 +94,7 @@ struct uprobe {
 	struct uprobe_consumer	*consumers;
 	struct inode		*inode;		/* Also hold a ref to inode */
 	loff_t			offset;
-	int			flags;
+	unsigned long		flags;
 	struct arch_uprobe	arch;
 };
 
@@ -423,7 +423,7 @@ static struct uprobe *insert_uprobe(struct uprobe *uprobe)
 	spin_unlock(&uprobes_treelock);
 
 	/* For now assume that the instruction need not be single-stepped */
-	uprobe->flags |= UPROBE_SKIP_SSTEP;
+	__set_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
 
 	return u;
 }
@@ -466,7 +466,7 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 {
 	struct uprobe_consumer *uc;
 
-	if (!(uprobe->flags & UPROBE_RUN_HANDLER))
+	if (!test_bit(UPROBE_RUN_HANDLER, &uprobe->flags))
 		return;
 
 	down_read(&uprobe->consumer_rwsem);
@@ -577,11 +577,11 @@ static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
 {
 	int ret = 0;
 
-	if (uprobe->flags & UPROBE_COPY_INSN)
+	if (test_bit(UPROBE_COPY_INSN, &uprobe->flags))
 		return ret;
 
 	mutex_lock(&uprobe->copy_mutex);
-	if (uprobe->flags & UPROBE_COPY_INSN)
+	if (test_bit(UPROBE_COPY_INSN, &uprobe->flags))
 		goto out;
 
 	ret = copy_insn(uprobe, file);
@@ -601,7 +601,7 @@ static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
 			UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
 
 	smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
-	uprobe->flags |= UPROBE_COPY_INSN;
+	set_bit(UPROBE_COPY_INSN, &uprobe->flags);
 	ret = 0;
  out:
 	mutex_unlock(&uprobe->copy_mutex);
@@ -852,7 +852,7 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
 			uprobe->consumers = NULL;
 			__uprobe_unregister(uprobe);
 		} else {
-			uprobe->flags |= UPROBE_RUN_HANDLER;
+			set_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
 		}
 	}
 
@@ -885,7 +885,7 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
 	if (consumer_del(uprobe, uc)) {
 		if (!uprobe->consumers) {
 			__uprobe_unregister(uprobe);
-			uprobe->flags &= ~UPROBE_RUN_HANDLER;
+			clear_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
 		}
 	}
 
@@ -1346,10 +1346,10 @@ bool uprobe_deny_signal(void)
  */
 static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)
 {
-	if (uprobe->flags & UPROBE_SKIP_SSTEP) {
+	if (test_bit(UPROBE_SKIP_SSTEP, &uprobe->flags)) {
 		if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
 			return true;
-		uprobe->flags &= ~UPROBE_SKIP_SSTEP;
+		clear_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
 	}
 	return false;
 }
@@ -1428,7 +1428,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 	 * new and not-yet-analyzed uprobe at the same address, restart.
 	 */
 	smp_rmb(); /* pairs with wmb() in install_breakpoint() */
-	if (uprobe && unlikely(!(uprobe->flags & UPROBE_COPY_INSN))) {
+	if (uprobe && unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags))) {
 		uprobe = NULL;
 		*is_swbp = 0;
 	}
-- 
1.5.5.1

Re: [PATCH 0/7] uprobes: register/unregister bugfixes

[Oleg Nesterov]

Forgot to mention,

This is on top of "[PATCH 0/3] uprobes: mprotect fixes" series,
which is still waiting for review...

On 09/30, Oleg Nesterov wrote:
>
> Hello.
>
> Misc bugfixes, mostly register/unregister related.
>
> Note: 4/7 and especially 6/7 are the really ugly (but hopefully
> temporary) hacks. We already discussed this, we should inspect
> the original insn in uprobe_register(). But this is not easy, and
> we need something simple to fix (some of) the bugs right now.
>
> Even 7/7 is ugly. Imho UPROBE_SKIP_SSTEP should die as well but
> this is another story.
>
> Oleg.
>
>  arch/x86/kernel/uprobes.c |   16 +------
>  include/linux/uprobes.h   |   10 ----
>  kernel/events/uprobes.c   |  107 +++++++++++++++++++++++++++++++-------------
>  3 files changed, 77 insertions(+), 56 deletions(-)

Re: [PATCH 0/7] uprobes: register/unregister bugfixes

[Srikar Dronamraju]

> This is on top of "[PATCH 0/3] uprobes: mprotect fixes" series,
> which is still waiting for review...
> 

Did you mean [PATCH 0/4] uprobes: remove is_swbp_at_addr() from
register/unregister?

I have already reviewed and acked "[PATCH 0/3] uprobes: mprotect fixes"
series".  

Got distracted by the register/unregister bug that we were discussing
offline.  Reviewing and testing both the pending series.

-- 
Thanks and Regards
Srikar

Re: [PATCH 0/7] uprobes: register/unregister bugfixes

[Oleg Nesterov]

On 10/01, Srikar Dronamraju wrote:
>
> > This is on top of "[PATCH 0/3] uprobes: mprotect fixes" series,
> > which is still waiting for review...
> >
>
> Did you mean [PATCH 0/4] uprobes: remove is_swbp_at_addr() from
> register/unregister?

Yes, sorry for confusion,

> I have already reviewed and acked "[PATCH 0/3] uprobes: mprotect fixes"
> series".

Yes, and I already added your acks, thanks.

> Got distracted by the register/unregister bug that we were discussing
> offline.

Yes. 2-6 try to fix the bugs I found by the code inspection, but
I do not think they can fix that bug, I guess there is yet another
problem we need to fix.

Oleg.

Re: [PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race

[Oleg Nesterov]

On 09/30, Oleg Nesterov wrote:
>
> +	if (uprobe && unlikely(!(uprobe->flags & UPROBE_COPY_INSN))) {
> +		uprobe = NULL;
> +		*is_swbp = 0;
> +	}

OOPS. this obvioulsy needs put_uprobe(uprobe). I updated this patch
in my tree.

-------------------------------------------------------------------
[PATCH] uprobes: Fix handle_swbp() vs unregister() + register() race

Strictly speaking this race was added by me in 56bb4cf6. However
I think that this bug is just another indication that we should
move copy_insn/uprobe_analyze_insn code from install_breakpoint()
to uprobe_register(), there are a lot of other reasons for that.
Until then, add a hack to close the race.

A task can hit uprobe U1, but before it calls find_uprobe() this
uprobe can be unregistered *AND* another uprobe U2 can be added to
uprobes_tree at the same inode/offset. In this case handle_swbp()
will use the not-fully-initialized U2, in particular its arch.insn
for xol.

Add the additional !UPROBE_COPY_INSN check into find_active_uprobe,
if this flag is not set we simply restart as if the new uprobe was
not inserted yet. This is not very nice, we need barriers, but we
will remove this hack when we change uprobe_register().

Note: with or without this patch install_breakpoint() can race with
itself, yet another reson to kill UPROBE_COPY_INSN altogether. And
even the usage of uprobe->flags is not safe. See the next patches.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 6058231..5c77137 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -596,6 +596,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 		BUG_ON((uprobe->offset & ~PAGE_MASK) +
 				UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
 
+		smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
 		uprobe->flags |= UPROBE_COPY_INSN;
 	}
 
@@ -1391,6 +1392,17 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 	if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
 		mmf_recalc_uprobes(mm);
 	up_read(&mm->mmap_sem);
+	/*
+	 * TODO: move copy_insn/etc into _register and remove this hack.
+	 * After we hit the bp, _unregister + _register can install the
+	 * new and not-yet-analyzed uprobe at the same address, restart.
+	 */
+	smp_rmb(); /* pairs with wmb() in install_breakpoint() */
+	if (uprobe && unlikely(!(uprobe->flags & UPROBE_COPY_INSN))) {
+		put_uprobe(uprobe);
+		uprobe = NULL;
+		*is_swbp = 0;
+	}
 
 	return uprobe;
 }
-- 
1.5.5.1

Re: [PATCH 7/7] uprobes: Fix the racy uprobe->flags manipulation

[Anton Arapov]

On Sun, Sep 30, 2012 at 09:42:27PM +0200, Oleg Nesterov wrote:
> Multiple threads can manipulate uprobe->flags, this is obviously
> unsafe. For example mmap can set UPROBE_COPY_INSN while register
> tries to set UPROBE_RUN_HANDLER, the latter can also race with
> can_skip_sstep() which clears UPROBE_SKIP_SSTEP.
> 
> Change this code to use bitops.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c |   28 ++++++++++++++--------------
>  1 files changed, 14 insertions(+), 14 deletions(-)

  Re-generating this one because of updated patch 4/7, so that it
will apply cleanly.

Anton
-------------------------------------------------------------------
From: Oleg Nesterov <oleg@redhat.com>
Date: Sun, 30 Sep 2012 21:42:27 +0200
Subject: [PATCH] uprobes: Fix the racy uprobe->flags manipulation

Multiple threads can manipulate uprobe->flags, this is obviously
unsafe. For example mmap can set UPROBE_COPY_INSN while register
tries to set UPROBE_RUN_HANDLER, the latter can also race with
can_skip_sstep() which clears UPROBE_SKIP_SSTEP.

Change this code to use bitops.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 72067e6..7a8690e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -79,11 +79,11 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
 static atomic_t uprobe_events = ATOMIC_INIT(0);
 
 /* Have a copy of original instruction */
-#define UPROBE_COPY_INSN	0x1
+#define UPROBE_COPY_INSN	0
 /* Dont run handlers when first register/ last unregister in progress*/
-#define UPROBE_RUN_HANDLER	0x2
+#define UPROBE_RUN_HANDLER	1
 /* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP	0x4
+#define UPROBE_SKIP_SSTEP	2
 
 struct uprobe {
 	struct rb_node		rb_node;	/* node in the rb tree */
@@ -94,7 +94,7 @@ struct uprobe {
 	struct uprobe_consumer	*consumers;
 	struct inode		*inode;		/* Also hold a ref to inode */
 	loff_t			offset;
-	int			flags;
+	unsigned long		flags;
 	struct arch_uprobe	arch;
 };
 
@@ -423,7 +423,7 @@ static struct uprobe *insert_uprobe(struct uprobe *uprobe)
 	spin_unlock(&uprobes_treelock);
 
 	/* For now assume that the instruction need not be single-stepped */
-	uprobe->flags |= UPROBE_SKIP_SSTEP;
+	__set_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
 
 	return u;
 }
@@ -466,7 +466,7 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 {
 	struct uprobe_consumer *uc;
 
-	if (!(uprobe->flags & UPROBE_RUN_HANDLER))
+	if (!test_bit(UPROBE_RUN_HANDLER, &uprobe->flags))
 		return;
 
 	down_read(&uprobe->consumer_rwsem);
@@ -577,11 +577,11 @@ static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
 {
 	int ret = 0;
 
-	if (uprobe->flags & UPROBE_COPY_INSN)
+	if (test_bit(UPROBE_COPY_INSN, &uprobe->flags))
 		return ret;
 
 	mutex_lock(&uprobe->copy_mutex);
-	if (uprobe->flags & UPROBE_COPY_INSN)
+	if (test_bit(UPROBE_COPY_INSN, &uprobe->flags))
 		goto out;
 
 	ret = copy_insn(uprobe, file);
@@ -601,7 +601,7 @@ static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
 			UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
 
 	smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
-	uprobe->flags |= UPROBE_COPY_INSN;
+	set_bit(UPROBE_COPY_INSN, &uprobe->flags);
 	ret = 0;
  out:
 	mutex_unlock(&uprobe->copy_mutex);
@@ -852,7 +852,7 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
 			uprobe->consumers = NULL;
 			__uprobe_unregister(uprobe);
 		} else {
-			uprobe->flags |= UPROBE_RUN_HANDLER;
+			set_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
 		}
 	}
 
@@ -885,7 +885,7 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
 	if (consumer_del(uprobe, uc)) {
 		if (!uprobe->consumers) {
 			__uprobe_unregister(uprobe);
-			uprobe->flags &= ~UPROBE_RUN_HANDLER;
+			clear_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
 		}
 	}
 
@@ -1346,10 +1346,10 @@ bool uprobe_deny_signal(void)
  */
 static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)
 {
-	if (uprobe->flags & UPROBE_SKIP_SSTEP) {
+	if (test_bit(UPROBE_SKIP_SSTEP, &uprobe->flags)) {
 		if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
 			return true;
-		uprobe->flags &= ~UPROBE_SKIP_SSTEP;
+		clear_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
 	}
 	return false;
 }
@@ -1428,7 +1428,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 	 * new and not-yet-analyzed uprobe at the same address, restart.
 	 */
 	smp_rmb(); /* pairs with wmb() in install_breakpoint() */
-	if (uprobe && unlikely(!(uprobe->flags & UPROBE_COPY_INSN))) {
+	if (uprobe && unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags))) {
 		put_uprobe(uprobe);
 		uprobe = NULL;
 		*is_swbp = 0;
-- 
1.7.11.7

Re: [PATCH 1/7] uprobes/x86: Only rep+nop can be emulated correctly

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-30 21:41:58]:

> __skip_sstep() correctly detects the "nontrivial" nop insns,
> but since it doesn't update regs->ip we can not really skip
> "0x0f 0x1f | 0x0f 0x19 | 0x87 0xc0", the probed application
> is killed by SIGILL'ed handle_swbp().
> 
> Remove these additional checks. If we want to implement this
> correctly we need to know the full insn length to update ->ip.
> 
> rep* + nop is fine even without updating ->ip.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---

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

Re: [PATCH 2/7] uprobes: Don't return success if alloc_uprobe() fails

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-30 21:42:01]:

> If alloc_uprobe() fails uprobe_register() should return ENOMEM, not 0.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---

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

Re: [PATCH 3/7] uprobes: Do not delete uprobe if uprobe_unregister() fails

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-30 21:42:04]:

> delete_uprobe() must not be called if register_for_each_vma(false)
> fails to remove all breakpoints, __uprobe_unregister() is correct.
> The problem is that register_for_each_vma(false) always returns 0
> and thus this logic does not work.
> 
> 1. Change verify_opcode() to return 0 rather than -EINVAL when
>    unregister detects the !is_swbp insn, we can treat this case
>    as success and currently unregister paths ignore the error
>    code anyway.
> 
> 2. Change remove_breakpoint() to propagate the error code from
>    write_opcode().
> 
> 3. Change register_for_each_vma(is_register => false) to remove
>    as much breakpoints as possible but return non-zero if
>    remove_breakpoint() fails at least once.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

Re: [PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-30 21:42:11]:

> Strictly speaking this race was added by me in 56bb4cf6. However
> I think that this bug is just another indication that we should
> move copy_insn/uprobe_analyze_insn code from install_breakpoint()
> to uprobe_register(), there are a lot of other reasons for that.
> Until then, add a hack to close the race.
> 
> A task can hit uprobe U1, but before it calls find_uprobe() this
> uprobe can be unregistered *AND* another uprobe U2 can be added to
> uprobes_tree at the same inode/offset. In this case handle_swbp()
> will use the not-fully-initialized U2, in particular its arch.insn
> for xol.
> 
> Add the additional !UPROBE_COPY_INSN check into find_active_uprobe,
> if this flag is not set we simply restart as if the new uprobe was
> not inserted yet. This is not very nice, we need barriers, but we
> will remove this hack when we change uprobe_register().
> 
> Note: with or without this patch install_breakpoint() can race with
> itself, yet another reson to kill UPROBE_COPY_INSN altogether. And
> even the usage of uprobe->flags is not safe. See the next patches.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 6058231..a81080f 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -596,6 +596,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  		BUG_ON((uprobe->offset & ~PAGE_MASK) +
>  				UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
> 
> +		smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
>  		uprobe->flags |= UPROBE_COPY_INSN;
>  	}
> 
> @@ -1391,6 +1392,16 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
>  	if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
>  		mmf_recalc_uprobes(mm);
>  	up_read(&mm->mmap_sem);
> +	/*
> +	 * TODO: move copy_insn/etc into _register and remove this hack.
> +	 * After we hit the bp, _unregister + _register can install the
> +	 * new and not-yet-analyzed uprobe at the same address, restart.
> +	 */
> +	smp_rmb(); /* pairs with wmb() in install_breakpoint() */
> +	if (uprobe && unlikely(!(uprobe->flags & UPROBE_COPY_INSN))) {
> +		uprobe = NULL;
> +		*is_swbp = 0;
> +	}
> 
>  	return uprobe;
>  }

Should we be adding this check handle_swbp() around can_skip_step()?

The earliest we access arch.insn is in can_skip_step. So we give some
more time for the instruction to be copied.

Also it will probably be a little cleaner, (Not having to drop a uprobe
reference, not having to reset is_swbp.)

-- 
Thanks and Regards
Srikar

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-30 21:42:17]:

> Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
> from install_breakpoint() into the new helper, uprobe_copy_insn().
> 
> And move uprobe->flags defines from uprobes.h to uprobes.c, nobody
> else can use them anyway.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  include/linux/uprobes.h |   10 --------
>  kernel/events/uprobes.c |   60 ++++++++++++++++++++++++++++++++---------------
>  2 files changed, 41 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 18d839d..2459457 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -35,16 +35,6 @@ struct inode;
>  # include <asm/uprobes.h>
>  #endif
> 
> -/* flags that denote/change uprobes behaviour */
> -
> -/* Have a copy of original instruction */
> -#define UPROBE_COPY_INSN	0x1
> -
> -/* Dont run handlers when first register/ last unregister in progress*/
> -#define UPROBE_RUN_HANDLER	0x2
> -/* Can skip singlestep */
> -#define UPROBE_SKIP_SSTEP	0x4
> -
>  struct uprobe_consumer {
>  	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
>  	/*
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index a81080f..5c0c1b0 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
>   */
>  static atomic_t uprobe_events = ATOMIC_INIT(0);
> 
> +/* Have a copy of original instruction */
> +#define UPROBE_COPY_INSN	0x1
> +/* Dont run handlers when first register/ last unregister in progress*/
> +#define UPROBE_RUN_HANDLER	0x2
> +/* Can skip singlestep */
> +#define UPROBE_SKIP_SSTEP	0x4
> +
>  struct uprobe {
>  	struct rb_node		rb_node;	/* node in the rb tree */
>  	atomic_t		ref;
> @@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file *filp)
>  	return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, uprobe->offset);
>  }
> 
> +static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
> +				struct mm_struct *mm, unsigned long vaddr)
> +{
> +	int ret = 0;
> +
> +	if (uprobe->flags & UPROBE_COPY_INSN)
> +		return ret;
> +
> +	ret = copy_insn(uprobe, file);
> +	if (ret)
> +		goto out;
> +
> +	ret = -ENOTSUPP;
> +	if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> +		goto out;
> +
> +	ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
> +	if (ret)
> +		goto out;
> +
> +	/* write_opcode() assumes we don't cross page boundary */
> +	BUG_ON((uprobe->offset & ~PAGE_MASK) +
> +			UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
> +
> +	smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
> +	uprobe->flags |= UPROBE_COPY_INSN;
> +	ret = 0;
> + out:
> +	return ret;
> +}
> +

2 nits: 
 why do we need to reset ret before out label? I think its redudant.
 arch_uprobe_analyze_insn() should have set it to 0 already. No?

blank line above out:

Currently only extern functions start with uprobe_ but we already have
copy_insn, and __copy_insn, So can think of any names for
uprobe_copy_insn. Not sure test_and_copy_insn() is a good alternative.

-- 
thanks and regards
Srikar

Re: [PATCH 6/7] uprobes: Fix uprobe_copy_insn() race with itself

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-30 21:42:24]:

> install_breakpoint() is called under mm->mmap_sem, this protects
> set_swbp() but not uprobe_copy_insn(). Two or more different tasks
> can call install_breakpoint()->uprobe_copy_insn() at the same time,
> this leads to numerous problems if UPROBE_COPY_INSN is not set.
> 
> Just for example, the second copy_insn() can corrupt the already
> analyzed/fixuped uprobe->arch.insn and race with handle_swbp().
> 
> This patch simply adds uprobe->copy_mutex to serialize this code.
> We could probably reuse ->consumer_rwsem, but this would mean that
> consumer->handler() can not use mm->mmap_sem, not good.
> 
> Note: this is another temporary ugly hack until we move this logic
> into uprobe_register().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Given that we copy just for the first install, and register not being  a
performance path, Can we use a single mutex instead of a per-uprobe
mutex.

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

Re: [PATCH 7/7] uprobes: Fix the racy uprobe->flags manipulation

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-30 21:42:27]:

> Multiple threads can manipulate uprobe->flags, this is obviously
> unsafe. For example mmap can set UPROBE_COPY_INSN while register
> tries to set UPROBE_RUN_HANDLER, the latter can also race with
> can_skip_sstep() which clears UPROBE_SKIP_SSTEP.
> 
> Change this code to use bitops.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

[Oleg Nesterov]

On 10/06, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-09-30 21:42:17]:
>
> > +static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
> > +				struct mm_struct *mm, unsigned long vaddr)
> > +{
> > +	int ret = 0;
> > +
> > +	if (uprobe->flags & UPROBE_COPY_INSN)
> > +		return ret;
> > +
> > +	ret = copy_insn(uprobe, file);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = -ENOTSUPP;
> > +	if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> > +		goto out;
> > +
> > +	ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
> > +	if (ret)
> > +		goto out;
> > +
> > +	/* write_opcode() assumes we don't cross page boundary */
> > +	BUG_ON((uprobe->offset & ~PAGE_MASK) +
> > +			UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
> > +
> > +	smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
> > +	uprobe->flags |= UPROBE_COPY_INSN;
> > +	ret = 0;
> > + out:
> > +	return ret;
> > +}
> > +
>
> 2 nits:
>  why do we need to reset ret before out label? I think its redudant.
>  arch_uprobe_analyze_insn() should have set it to 0 already. No?

OK,

> blank line above out:

OK, see the updated patch below.

> Currently only extern functions start with uprobe_ but we already have
> copy_insn, and __copy_insn, So can think of any names for
> uprobe_copy_insn.

Yes ;) plus it is not only "copy", it does _analyze

> Not sure test_and_copy_insn() is a good alternative.

perhaps prepare_uprobe()... But I agree with any naming, just tell me
what you prefer and I'll update the patch again.

==============================================================================
[PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
from install_breakpoint() into the new helper, uprobe_copy_insn().

And move uprobe->flags defines from uprobes.h to uprobes.c, nobody
else can use them anyway.

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

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 18d839d..2459457 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -35,16 +35,6 @@ struct inode;
 # include <asm/uprobes.h>
 #endif
 
-/* flags that denote/change uprobes behaviour */
-
-/* Have a copy of original instruction */
-#define UPROBE_COPY_INSN	0x1
-
-/* Dont run handlers when first register/ last unregister in progress*/
-#define UPROBE_RUN_HANDLER	0x2
-/* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP	0x4
-
 struct uprobe_consumer {
 	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
 	/*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index dd44311..98b60bc 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
  */
 static atomic_t uprobe_events = ATOMIC_INIT(0);
 
+/* Have a copy of original instruction */
+#define UPROBE_COPY_INSN	0x1
+/* Dont run handlers when first register/ last unregister in progress*/
+#define UPROBE_RUN_HANDLER	0x2
+/* Can skip singlestep */
+#define UPROBE_SKIP_SSTEP	0x4
+
 struct uprobe {
 	struct rb_node		rb_node;	/* node in the rb tree */
 	atomic_t		ref;
@@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file *filp)
 	return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, uprobe->offset);
 }
 
+static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
+				struct mm_struct *mm, unsigned long vaddr)
+{
+	int ret = 0;
+
+	if (uprobe->flags & UPROBE_COPY_INSN)
+		return ret;
+
+	ret = copy_insn(uprobe, file);
+	if (ret)
+		goto out;
+
+	ret = -ENOTSUPP;
+	if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
+		goto out;
+
+	ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
+	if (ret)
+		goto out;
+
+	/* write_opcode() assumes we don't cross page boundary */
+	BUG_ON((uprobe->offset & ~PAGE_MASK) +
+			UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
+
+	smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
+	uprobe->flags |= UPROBE_COPY_INSN;
+
+ out:
+	return ret;
+}
+
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 			struct vm_area_struct *vma, unsigned long vaddr)
@@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 	if (!uprobe->consumers)
 		return 0;
 
-	if (!(uprobe->flags & UPROBE_COPY_INSN)) {
-		ret = copy_insn(uprobe, vma->vm_file);
-		if (ret)
-			return ret;
-
-		if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
-			return -ENOTSUPP;
-
-		ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
-		if (ret)
-			return ret;
-
-		/* write_opcode() assumes we don't cross page boundary */
-		BUG_ON((uprobe->offset & ~PAGE_MASK) +
-				UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
-
-		smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
-		uprobe->flags |= UPROBE_COPY_INSN;
-	}
+	ret = uprobe_copy_insn(uprobe, vma->vm_file, mm, vaddr);
+	if (ret)
+		return ret;
 
 	/*
 	 * set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
-- 
1.5.5.1

Re: [PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race

[Oleg Nesterov]

On 10/06, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-09-30 21:42:11]:
>
> > @@ -1391,6 +1392,16 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> >  	if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
> >  		mmf_recalc_uprobes(mm);
> >  	up_read(&mm->mmap_sem);
> > +	/*
> > +	 * TODO: move copy_insn/etc into _register and remove this hack.
> > +	 * After we hit the bp, _unregister + _register can install the
> > +	 * new and not-yet-analyzed uprobe at the same address, restart.
> > +	 */
> > +	smp_rmb(); /* pairs with wmb() in install_breakpoint() */
> > +	if (uprobe && unlikely(!(uprobe->flags & UPROBE_COPY_INSN))) {
> > +		uprobe = NULL;
> > +		*is_swbp = 0;
> > +	}
> >
> >  	return uprobe;
> >  }
>
> Should we be adding this check handle_swbp() around can_skip_step()?
>
> The earliest we access arch.insn is in can_skip_step. So we give some
> more time for the instruction to be copied.

handle_swbp:

	if (can_skip_sstep(uprobe, regs))
		goto out;

but if we hit a non-UPROBE_COPY_INSN uprobe, we need "goto restart".

> Also it will probably be a little cleaner, (Not having to drop a uprobe
> reference, not having to reset is_swbp.)

We can change handler_chain() (which also checks UPROBE_RUN_HANDLER)
to return "bool restart", not sure this will be more clean.

And if we change handler_chain(), I think it should return bitmask,

	for (uc = uprobe->consumers; uc; uc = uc->next)
		ret |= uc->handler(...);

	return ret;

for the future changes... (say, we can remove bp if consumers do not
want to trace this task). Not sure it makes sense to change it right
now.

So. Should I leave this patch as is? Or do you want me to move this
check into handler_chain() and make it return "bool restart"?

Oleg.

Re: [PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race

[Srikar Dronamraju]

> 
> for the future changes... (say, we can remove bp if consumers do not
> want to trace this task). Not sure it makes sense to change it right
> now.
> 
> So. Should I leave this patch as is? Or do you want me to move this
> check into handler_chain() and make it return "bool restart"?

Lets keep it as is for now.

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

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

[Srikar Dronamraju]

> > uprobe_copy_insn.
> 
> Yes ;) plus it is not only "copy", it does _analyze
> 
> > Not sure test_and_copy_insn() is a good alternative.
> 
> perhaps prepare_uprobe()... But I agree with any naming, just tell me
> what you prefer and I'll update the patch again.


Yeah prepare_uprobe() looks good for me. 



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

> 
> ==============================================================================
> [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()
> 
> Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
> from install_breakpoint() into the new helper, uprobe_copy_insn().
> 
> And move uprobe->flags defines from uprobes.h to uprobes.c, nobody
> else can use them anyway.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  include/linux/uprobes.h |   10 --------
>  kernel/events/uprobes.c |   60 ++++++++++++++++++++++++++++++++---------------
>  2 files changed, 41 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 18d839d..2459457 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -35,16 +35,6 @@ struct inode;
>  # include <asm/uprobes.h>
>  #endif
> 
> -/* flags that denote/change uprobes behaviour */
> -
> -/* Have a copy of original instruction */
> -#define UPROBE_COPY_INSN	0x1
> -
> -/* Dont run handlers when first register/ last unregister in progress*/
> -#define UPROBE_RUN_HANDLER	0x2
> -/* Can skip singlestep */
> -#define UPROBE_SKIP_SSTEP	0x4
> -
>  struct uprobe_consumer {
>  	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
>  	/*
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index dd44311..98b60bc 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
>   */
>  static atomic_t uprobe_events = ATOMIC_INIT(0);
> 
> +/* Have a copy of original instruction */
> +#define UPROBE_COPY_INSN	0x1
> +/* Dont run handlers when first register/ last unregister in progress*/
> +#define UPROBE_RUN_HANDLER	0x2
> +/* Can skip singlestep */
> +#define UPROBE_SKIP_SSTEP	0x4
> +
>  struct uprobe {
>  	struct rb_node		rb_node;	/* node in the rb tree */
>  	atomic_t		ref;
> @@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file *filp)
>  	return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, uprobe->offset);
>  }
> 
> +static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
> +				struct mm_struct *mm, unsigned long vaddr)
> +{
> +	int ret = 0;
> +
> +	if (uprobe->flags & UPROBE_COPY_INSN)
> +		return ret;
> +
> +	ret = copy_insn(uprobe, file);
> +	if (ret)
> +		goto out;
> +
> +	ret = -ENOTSUPP;
> +	if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> +		goto out;
> +
> +	ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
> +	if (ret)
> +		goto out;
> +
> +	/* write_opcode() assumes we don't cross page boundary */
> +	BUG_ON((uprobe->offset & ~PAGE_MASK) +
> +			UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
> +
> +	smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
> +	uprobe->flags |= UPROBE_COPY_INSN;
> +
> + out:
> +	return ret;
> +}
> +
>  static int
>  install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  			struct vm_area_struct *vma, unsigned long vaddr)
> @@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  	if (!uprobe->consumers)
>  		return 0;
> 
> -	if (!(uprobe->flags & UPROBE_COPY_INSN)) {
> -		ret = copy_insn(uprobe, vma->vm_file);
> -		if (ret)
> -			return ret;
> -
> -		if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> -			return -ENOTSUPP;
> -
> -		ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
> -		if (ret)
> -			return ret;
> -
> -		/* write_opcode() assumes we don't cross page boundary */
> -		BUG_ON((uprobe->offset & ~PAGE_MASK) +
> -				UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
> -
> -		smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
> -		uprobe->flags |= UPROBE_COPY_INSN;
> -	}
> +	ret = uprobe_copy_insn(uprobe, vma->vm_file, mm, vaddr);
> +	if (ret)
> +		return ret;
> 
>  	/*
>  	 * set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
> -- 
> 1.5.5.1
> 
> 
> 

Re: [PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race

[Oleg Nesterov]

On 10/06, Srikar Dronamraju wrote:
>
> >
> > for the future changes... (say, we can remove bp if consumers do not
> > want to trace this task). Not sure it makes sense to change it right
> > now.
> >
> > So. Should I leave this patch as is? Or do you want me to move this
> > check into handler_chain() and make it return "bool restart"?
>
> Lets keep it as is for now.
>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thanks...

But I am starting to think that I misunderstood your comment, you
did not suggest to add this check into skip_sstep() as I wrongly
thought.

And yes, I agree it would be more clean to move it out from
find_active_uprobe() and avoid put_uprobe && clear_swbp....

So how about v2 below?

------------------------------------------------------------------------------
[PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race

Strictly speaking this race was added by me in 56bb4cf6. However
I think that this bug is just another indication that we should
move copy_insn/uprobe_analyze_insn code from install_breakpoint()
to uprobe_register(), there are a lot of other reasons for that.
Until then, add a hack to close the race.

A task can hit uprobe U1, but before it calls find_uprobe() this
uprobe can be unregistered *AND* another uprobe U2 can be added to
uprobes_tree at the same inode/offset. In this case handle_swbp()
will use the not-fully-initialized U2, in particular its arch.insn
for xol.

Add the additional !UPROBE_COPY_INSN check into handle_swbp(),
if this flag is not set we simply restart as if the new uprobe was
not inserted yet. This is not very nice, we need barriers, but we
will remove this hack when we change uprobe_register().

Note: with or without this patch install_breakpoint() can race with
itself, yet another reson to kill UPROBE_COPY_INSN altogether. And
even the usage of uprobe->flags is not safe. See the next patches.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 kernel/events/uprobes.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index cfa22c4..dbbca3a 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -596,6 +596,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 		BUG_ON((uprobe->offset & ~PAGE_MASK) +
 				UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
 
+		smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
 		uprobe->flags |= UPROBE_COPY_INSN;
 	}
 
@@ -1436,6 +1437,14 @@ static void handle_swbp(struct pt_regs *regs)
 		}
 		return;
 	}
+	/*
+	 * TODO: move copy_insn/etc into _register and remove this hack.
+	 * After we hit the bp, _unregister + _register can install the
+	 * new and not-yet-analyzed uprobe at the same address, restart.
+	 */
+	smp_rmb(); /* pairs with wmb() in install_breakpoint() */
+	if (unlikely(!(uprobe->flags & UPROBE_COPY_INSN)))
+		goto restart;
 
 	utask = current->utask;
 	if (!utask) {
-- 
1.5.5.1

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

[Oleg Nesterov]

On 10/06, Srikar Dronamraju wrote:
>
> Yeah prepare_uprobe() looks good for me.
>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

OK, renamed.

The next patches updated accordinly, I hope I can keep your acks.

------------------------------------------------------------------------------
[PATCH] uprobes: Introduce prepare_uprobe()

Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
from install_breakpoint() into the new helper, prepare_uprobe().

And move uprobe->flags defines from uprobes.h to uprobes.c, nobody
else can use them anyway.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 include/linux/uprobes.h |   10 --------
 kernel/events/uprobes.c |   60 ++++++++++++++++++++++++++++++++---------------
 2 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 18d839d..2459457 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -35,16 +35,6 @@ struct inode;
 # include <asm/uprobes.h>
 #endif
 
-/* flags that denote/change uprobes behaviour */
-
-/* Have a copy of original instruction */
-#define UPROBE_COPY_INSN	0x1
-
-/* Dont run handlers when first register/ last unregister in progress*/
-#define UPROBE_RUN_HANDLER	0x2
-/* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP	0x4
-
 struct uprobe_consumer {
 	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
 	/*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index dbbca3a..d6fa332 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
  */
 static atomic_t uprobe_events = ATOMIC_INIT(0);
 
+/* Have a copy of original instruction */
+#define UPROBE_COPY_INSN	0x1
+/* Dont run handlers when first register/ last unregister in progress*/
+#define UPROBE_RUN_HANDLER	0x2
+/* Can skip singlestep */
+#define UPROBE_SKIP_SSTEP	0x4
+
 struct uprobe {
 	struct rb_node		rb_node;	/* node in the rb tree */
 	atomic_t		ref;
@@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file *filp)
 	return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, uprobe->offset);
 }
 
+static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
+				struct mm_struct *mm, unsigned long vaddr)
+{
+	int ret = 0;
+
+	if (uprobe->flags & UPROBE_COPY_INSN)
+		return ret;
+
+	ret = copy_insn(uprobe, file);
+	if (ret)
+		goto out;
+
+	ret = -ENOTSUPP;
+	if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
+		goto out;
+
+	ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
+	if (ret)
+		goto out;
+
+	/* write_opcode() assumes we don't cross page boundary */
+	BUG_ON((uprobe->offset & ~PAGE_MASK) +
+			UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
+
+	smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
+	uprobe->flags |= UPROBE_COPY_INSN;
+
+ out:
+	return ret;
+}
+
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 			struct vm_area_struct *vma, unsigned long vaddr)
@@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 	if (!uprobe->consumers)
 		return 0;
 
-	if (!(uprobe->flags & UPROBE_COPY_INSN)) {
-		ret = copy_insn(uprobe, vma->vm_file);
-		if (ret)
-			return ret;
-
-		if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
-			return -ENOTSUPP;
-
-		ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
-		if (ret)
-			return ret;
-
-		/* write_opcode() assumes we don't cross page boundary */
-		BUG_ON((uprobe->offset & ~PAGE_MASK) +
-				UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
-
-		smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
-		uprobe->flags |= UPROBE_COPY_INSN;
-	}
+	ret = prepare_uprobe(uprobe, vma->vm_file, mm, vaddr);
+	if (ret)
+		return ret;
 
 	/*
 	 * set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
-- 
1.5.5.1

Re: [PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-10-06 20:53:37]:

> On 10/06, Srikar Dronamraju wrote:
> >
> > >
> > > for the future changes... (say, we can remove bp if consumers do not
> > > want to trace this task). Not sure it makes sense to change it right
> > > now.
> > >
> > > So. Should I leave this patch as is? Or do you want me to move this
> > > check into handler_chain() and make it return "bool restart"?
> >
> > Lets keep it as is for now.
> >
> > Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> 
> Thanks...
> 
> But I am starting to think that I misunderstood your comment, you
> did not suggest to add this check into skip_sstep() as I wrongly
> thought.
> 
> And yes, I agree it would be more clean to move it out from
> find_active_uprobe() and avoid put_uprobe && clear_swbp....
> 
> So how about v2 below?

Yes, this is what I meant. Thanks for the relooking into it.
This will mean, change in one hunk in patch 7/7.

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

> 
> ------------------------------------------------------------------------------
> [PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race
> 
> Strictly speaking this race was added by me in 56bb4cf6. However
> I think that this bug is just another indication that we should
> move copy_insn/uprobe_analyze_insn code from install_breakpoint()
> to uprobe_register(), there are a lot of other reasons for that.
> Until then, add a hack to close the race.
> 
> A task can hit uprobe U1, but before it calls find_uprobe() this
> uprobe can be unregistered *AND* another uprobe U2 can be added to
> uprobes_tree at the same inode/offset. In this case handle_swbp()
> will use the not-fully-initialized U2, in particular its arch.insn
> for xol.
> 
> Add the additional !UPROBE_COPY_INSN check into handle_swbp(),
> if this flag is not set we simply restart as if the new uprobe was
> not inserted yet. This is not very nice, we need barriers, but we
> will remove this hack when we change uprobe_register().
> 
> Note: with or without this patch install_breakpoint() can race with
> itself, yet another reson to kill UPROBE_COPY_INSN altogether. And
> even the usage of uprobe->flags is not safe. See the next patches.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  kernel/events/uprobes.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index cfa22c4..dbbca3a 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -596,6 +596,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  		BUG_ON((uprobe->offset & ~PAGE_MASK) +
>  				UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
> 
> +		smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
>  		uprobe->flags |= UPROBE_COPY_INSN;
>  	}
> 
> @@ -1436,6 +1437,14 @@ static void handle_swbp(struct pt_regs *regs)
>  		}
>  		return;
>  	}
> +	/*
> +	 * TODO: move copy_insn/etc into _register and remove this hack.
> +	 * After we hit the bp, _unregister + _register can install the
> +	 * new and not-yet-analyzed uprobe at the same address, restart.
> +	 */
> +	smp_rmb(); /* pairs with wmb() in install_breakpoint() */
> +	if (unlikely(!(uprobe->flags & UPROBE_COPY_INSN)))
> +		goto restart;
> 
>  	utask = current->utask;
>  	if (!utask) {
> -- 
> 1.5.5.1
> 
> 

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-10-06 20:59:49]:

> On 10/06, Srikar Dronamraju wrote:
> >
> > Yeah prepare_uprobe() looks good for me.
> >
> > Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> 
> OK, renamed.
> 
> The next patches updated accordinly, I hope I can keep your acks.

Yes, please keep my acks as I understand that the hunks in the rest of
the patches will have trivial modifications.

-- 
Thanks and Regards
Srikar

> 
> ------------------------------------------------------------------------------
> [PATCH] uprobes: Introduce prepare_uprobe()
> 
> Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
> from install_breakpoint() into the new helper, prepare_uprobe().
> 
> And move uprobe->flags defines from uprobes.h to uprobes.c, nobody
> else can use them anyway.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  include/linux/uprobes.h |   10 --------
>  kernel/events/uprobes.c |   60 ++++++++++++++++++++++++++++++++---------------
>  2 files changed, 41 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 18d839d..2459457 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -35,16 +35,6 @@ struct inode;
>  # include <asm/uprobes.h>
>  #endif
> 
> -/* flags that denote/change uprobes behaviour */
> -
> -/* Have a copy of original instruction */
> -#define UPROBE_COPY_INSN	0x1
> -
> -/* Dont run handlers when first register/ last unregister in progress*/
> -#define UPROBE_RUN_HANDLER	0x2
> -/* Can skip singlestep */
> -#define UPROBE_SKIP_SSTEP	0x4
> -
>  struct uprobe_consumer {
>  	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
>  	/*
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index dbbca3a..d6fa332 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
>   */
>  static atomic_t uprobe_events = ATOMIC_INIT(0);
> 
> +/* Have a copy of original instruction */
> +#define UPROBE_COPY_INSN	0x1
> +/* Dont run handlers when first register/ last unregister in progress*/
> +#define UPROBE_RUN_HANDLER	0x2
> +/* Can skip singlestep */
> +#define UPROBE_SKIP_SSTEP	0x4
> +
>  struct uprobe {
>  	struct rb_node		rb_node;	/* node in the rb tree */
>  	atomic_t		ref;
> @@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file *filp)
>  	return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, uprobe->offset);
>  }
> 
> +static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
> +				struct mm_struct *mm, unsigned long vaddr)
> +{
> +	int ret = 0;
> +
> +	if (uprobe->flags & UPROBE_COPY_INSN)
> +		return ret;
> +
> +	ret = copy_insn(uprobe, file);
> +	if (ret)
> +		goto out;
> +
> +	ret = -ENOTSUPP;
> +	if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> +		goto out;
> +
> +	ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
> +	if (ret)
> +		goto out;
> +
> +	/* write_opcode() assumes we don't cross page boundary */
> +	BUG_ON((uprobe->offset & ~PAGE_MASK) +
> +			UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
> +
> +	smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
> +	uprobe->flags |= UPROBE_COPY_INSN;
> +
> + out:
> +	return ret;
> +}
> +
>  static int
>  install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  			struct vm_area_struct *vma, unsigned long vaddr)
> @@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  	if (!uprobe->consumers)
>  		return 0;
> 
> -	if (!(uprobe->flags & UPROBE_COPY_INSN)) {
> -		ret = copy_insn(uprobe, vma->vm_file);
> -		if (ret)
> -			return ret;
> -
> -		if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> -			return -ENOTSUPP;
> -
> -		ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
> -		if (ret)
> -			return ret;
> -
> -		/* write_opcode() assumes we don't cross page boundary */
> -		BUG_ON((uprobe->offset & ~PAGE_MASK) +
> -				UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
> -
> -		smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
> -		uprobe->flags |= UPROBE_COPY_INSN;
> -	}
> +	ret = prepare_uprobe(uprobe, vma->vm_file, mm, vaddr);
> +	if (ret)
> +		return ret;
> 
>  	/*
>  	 * set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
> -- 
> 1.5.5.1
> 
>