[PATCH 0/7] uprobes: kill uprobes_state->count, add MMF_HAS_UPROBES

[PATCH 0/7] uprobes: kill uprobes_state->count, add MMF_HAS_UPROBES

[Oleg Nesterov]

Hello.

Srikar, please review.

This is the first part. The next series will teach uprobes to
clear MMF_HAS_UPROBES, but perhaps we should simply remove
uprobe_munmap() instead. A wrong MMF_HAS_UPROBES doesn't hurt
unless the task hits the non-uprobe "int3", not sure it really
makes sense to try to speedup this case. We will see.

7/7 is offtopic.

Oleg.

 include/linux/sched.h   |    2 +
 include/linux/uprobes.h |   13 ++--
 kernel/events/uprobes.c |  141 ++++++++++++----------------------------------
 kernel/fork.c           |    6 +--
 4 files changed, 47 insertions(+), 115 deletions(-)

[PATCH 1/7] uprobes: kill uprobes_state->count

[Oleg Nesterov]

uprobes_state->count is only needed to avoid the slow path in
uprobe_pre_sstep_notifier(). It is also checked in uprobe_munmap()
but ironically its only goal to decrement this counter. However,
it is very broken. Just some examples:

- uprobe_mmap() can race with uprobe_unregister() and wrongly
  increment the counter if it hits the non-uprobe "int3". Note
  that install_breakpoint() checks ->consumers first and returns
  -EEXIST if it is NULL.

  "atomic_sub() if error" in uprobe_mmap() looks obviously wrong
  too.

- uprobe_munmap() can race with uprobe_register() and wrongly
  decrement the counter by the same reason.

- Suppose an appication tries to increase the mmapped area via
  sys_mremap(). vma_adjust() does uprobe_munmap(whole_vma) first,
  this can nullify the counter temporarily and race with another
  thread which can hit the bp, the application will be killed by
  SIGTRAP.

- Suppose an application mmaps 2 consecutive areas in the same file
  and one (or both) of these areas has uprobes. In the likely case
  mmap_region()->vma_merge() suceeds. Like above, this leads to
  uprobe_munmap/uprobe_mmap from vma_merge()->vma_adjust() but then
  mmap_region() does another uprobe_mmap(resulting_vma) and doubles
  the counter.

This patch only removes this counter and fixes the compile errors,
then we will try to cleanup the changed code and add something else
instead.

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

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index efe4b33..03ae547 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -99,8 +99,8 @@ struct xol_area {
 
 struct uprobes_state {
 	struct xol_area		*xol_area;
-	atomic_t		count;
 };
+
 extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
 extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm,  unsigned long vaddr, bool verify);
 extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 940005d..f0bb387 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -680,18 +680,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 		uprobe->flags |= UPROBE_COPY_INSN;
 	}
 
-	/*
-	 * Ideally, should be updating the probe count after the breakpoint
-	 * has been successfully inserted. However a thread could hit the
-	 * breakpoint we just inserted even before the probe count is
-	 * incremented. If this is the first breakpoint placed, breakpoint
-	 * notifier might ignore uprobes and pass the trap to the thread.
-	 * Hence increment before and decrement on failure.
-	 */
-	atomic_inc(&mm->uprobes_state.count);
 	ret = set_swbp(&uprobe->arch, mm, vaddr);
-	if (ret)
-		atomic_dec(&mm->uprobes_state.count);
 
 	return ret;
 }
@@ -699,8 +688,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 static void
 remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr)
 {
-	if (!set_orig_insn(&uprobe->arch, mm, vaddr, true))
-		atomic_dec(&mm->uprobes_state.count);
+	set_orig_insn(&uprobe->arch, mm, vaddr, true);
 }
 
 /*
@@ -1053,13 +1041,6 @@ int uprobe_mmap(struct vm_area_struct *vma)
 
 				if (!is_swbp_at_addr(vma->vm_mm, vaddr))
 					continue;
-
-				/*
-				 * Unable to insert a breakpoint, but
-				 * breakpoint lies underneath. Increment the
-				 * probe count.
-				 */
-				atomic_inc(&vma->vm_mm->uprobes_state.count);
 			}
 
 			if (!ret)
@@ -1070,9 +1051,6 @@ int uprobe_mmap(struct vm_area_struct *vma)
 
 	mutex_unlock(uprobes_mmap_hash(inode));
 
-	if (ret)
-		atomic_sub(count, &vma->vm_mm->uprobes_state.count);
-
 	return ret;
 }
 
@@ -1091,9 +1069,6 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
 	if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
 		return;
 
-	if (!atomic_read(&vma->vm_mm->uprobes_state.count))
-		return;
-
 	inode = vma->vm_file->f_mapping->host;
 	if (!inode)
 		return;
@@ -1102,13 +1077,6 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
 	build_probe_list(inode, vma, start, end, &tmp_list);
 
 	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
-		unsigned long vaddr = offset_to_vaddr(vma, uprobe->offset);
-		/*
-		 * An unregister could have removed the probe before
-		 * unmap. So check before we decrement the count.
-		 */
-		if (is_swbp_at_addr(vma->vm_mm, vaddr) == 1)
-			atomic_dec(&vma->vm_mm->uprobes_state.count);
 		put_uprobe(uprobe);
 	}
 	mutex_unlock(uprobes_mmap_hash(inode));
@@ -1219,7 +1187,6 @@ void uprobe_clear_state(struct mm_struct *mm)
 void uprobe_reset_state(struct mm_struct *mm)
 {
 	mm->uprobes_state.xol_area = NULL;
-	atomic_set(&mm->uprobes_state.count, 0);
 }
 
 /*
@@ -1589,8 +1556,7 @@ int uprobe_pre_sstep_notifier(struct pt_regs *regs)
 {
 	struct uprobe_task *utask;
 
-	if (!current->mm || !atomic_read(&current->mm->uprobes_state.count))
-		/* task is currently not uprobed */
+	if (!current->mm)
 		return 0;
 
 	utask = current->utask;
-- 
1.5.5.1

[PATCH 2/7] uprobes: kill dup_mmap()->uprobe_mmap(), simplify uprobe_mmap/munmap

[Oleg Nesterov]

1. Kill dup_mmap()->uprobe_mmap(), it was only needed to calculate
   new_mm->uprobes_state.count removed by the previous patch.

   If the forking process has a pending uprobe (int3) in vma, it will
   be copied by copy_page_range(), note that it checks vma->anon_vma
   so "Don't copy ptes" is not possible after install_breakpoint()
   which does anon_vma_prepare().

2. Remove is_swbp_at_addr() and "int count" in uprobe_mmap(). Again,
   this was needed for uprobes_state.count.

   As a side effect this fixes the bug pointed out by Srikar,
   this code lacked the necessary put_uprobe().

3. uprobe_munmap() becomes a nop after the previous patch. Remove the
   meaningless code but do not remove the helper, we will need it.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f0bb387..08ea627 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1012,7 +1012,7 @@ int uprobe_mmap(struct vm_area_struct *vma)
 	struct list_head tmp_list;
 	struct uprobe *uprobe, *u;
 	struct inode *inode;
-	int ret, count;
+	int ret;
 
 	if (!atomic_read(&uprobe_events) || !valid_vma(vma, true))
 		return 0;
@@ -1025,8 +1025,6 @@ int uprobe_mmap(struct vm_area_struct *vma)
 	build_probe_list(inode, vma, vma->vm_start, vma->vm_end, &tmp_list);
 
 	ret = 0;
-	count = 0;
-
 	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
 		if (!ret) {
 			unsigned long vaddr = offset_to_vaddr(vma, uprobe->offset);
@@ -1036,19 +1034,11 @@ int uprobe_mmap(struct vm_area_struct *vma)
 			 * We can race against uprobe_register(), see the
 			 * comment near uprobe_hash().
 			 */
-			if (ret == -EEXIST) {
+			if (ret == -EEXIST)
 				ret = 0;
-
-				if (!is_swbp_at_addr(vma->vm_mm, vaddr))
-					continue;
-			}
-
-			if (!ret)
-				count++;
 		}
 		put_uprobe(uprobe);
 	}
-
 	mutex_unlock(uprobes_mmap_hash(inode));
 
 	return ret;
@@ -1059,27 +1049,13 @@ int uprobe_mmap(struct vm_area_struct *vma)
  */
 void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end)
 {
-	struct list_head tmp_list;
-	struct uprobe *uprobe, *u;
-	struct inode *inode;
-
 	if (!atomic_read(&uprobe_events) || !valid_vma(vma, false))
 		return;
 
 	if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
 		return;
 
-	inode = vma->vm_file->f_mapping->host;
-	if (!inode)
-		return;
-
-	mutex_lock(uprobes_mmap_hash(inode));
-	build_probe_list(inode, vma, start, end, &tmp_list);
-
-	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
-		put_uprobe(uprobe);
-	}
-	mutex_unlock(uprobes_mmap_hash(inode));
+	/* TODO: unmapping uprobe(s) will need more work */
 }
 
 /* Slot allocation for XOL */
diff --git a/kernel/fork.c b/kernel/fork.c
index 54bb88a..0e419ab 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -453,9 +453,6 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
 
 		if (retval)
 			goto out;
-
-		if (file)
-			uprobe_mmap(tmp);
 	}
 	/* a new mm has just been created */
 	arch_dup_mmap(oldmm, mm);
-- 
1.5.5.1

[PATCH 3/7] uprobes: change uprobe_mmap() to ignore the errors but check fatal_signal_pending()

[Oleg Nesterov]

Once install_breakpoint() fails uprobe_mmap() "ignores" all other
uprobes and returns the error.

It was never really needed to to stop after the first error, and
in fact it was always wrong at least in -ENOTSUPP case.

Change uprobe_mmap() to ignore the errors and always return 0.
This is not what we want in the long term, but until we teach
the callers to handle the failure it would be better to remove
the pointless complications. And this doesn't look too bad, the
only "reasonable" error is ENOMEM but in this case the caller
should be oom-killed in the likely case or the system has more
serious problems.

However it makes sense to stop if fatal_signal_pending() == T.
In particular this helps if the task was oom-killed.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 08ea627..2e269d1 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -996,23 +996,16 @@ static void build_probe_list(struct inode *inode,
 }
 
 /*
- * Called from mmap_region.
- * called with mm->mmap_sem acquired.
+ * Called from mmap_region/vma_adjust with mm->mmap_sem acquired.
  *
- * Return -ve no if we fail to insert probes and we cannot
- * bail-out.
- * Return 0 otherwise. i.e:
- *
- *	- successful insertion of probes
- *	- (or) no possible probes to be inserted.
- *	- (or) insertion of probes failed but we can bail-out.
+ * Currently we ignore all errors and always return 0, the callers
+ * can't handle the failure anyway.
  */
 int uprobe_mmap(struct vm_area_struct *vma)
 {
 	struct list_head tmp_list;
 	struct uprobe *uprobe, *u;
 	struct inode *inode;
-	int ret;
 
 	if (!atomic_read(&uprobe_events) || !valid_vma(vma, true))
 		return 0;
@@ -1024,24 +1017,16 @@ int uprobe_mmap(struct vm_area_struct *vma)
 	mutex_lock(uprobes_mmap_hash(inode));
 	build_probe_list(inode, vma, vma->vm_start, vma->vm_end, &tmp_list);
 
-	ret = 0;
 	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
-		if (!ret) {
+		if (!fatal_signal_pending(current)) {
 			unsigned long vaddr = offset_to_vaddr(vma, uprobe->offset);
-
-			ret = install_breakpoint(uprobe, vma->vm_mm, vma, vaddr);
-			/*
-			 * We can race against uprobe_register(), see the
-			 * comment near uprobe_hash().
-			 */
-			if (ret == -EEXIST)
-				ret = 0;
+			install_breakpoint(uprobe, vma->vm_mm, vma, vaddr);
 		}
 		put_uprobe(uprobe);
 	}
 	mutex_unlock(uprobes_mmap_hash(inode));
 
-	return ret;
+	return 0;
 }
 
 /*
-- 
1.5.5.1

[PATCH 4/7] uprobes: do not use -EEXIST in install_breakpoint() paths

[Oleg Nesterov]

-EEXIST from install_breakpoint() no longer makes sense, all
callers should simply treat it as "success". Change the code
to return zero and simplify register_for_each_vma().

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 2e269d1..309309e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -334,7 +334,7 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned
 	 */
 	result = is_swbp_at_addr(mm, vaddr);
 	if (result == 1)
-		return -EEXIST;
+		return 0;
 
 	if (result)
 		return result;
@@ -659,7 +659,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 	 * Hence behave as if probe already existed.
 	 */
 	if (!uprobe->consumers)
-		return -EEXIST;
+		return 0;
 
 	if (!(uprobe->flags & UPROBE_COPY_INSN)) {
 		ret = copy_insn(uprobe, vma->vm_file);
@@ -819,17 +819,11 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 		    vaddr_to_offset(vma, info->vaddr) != uprobe->offset)
 			goto unlock;
 
-		if (is_register) {
+		if (is_register)
 			err = install_breakpoint(uprobe, mm, vma, info->vaddr);
-			/*
-			 * We can race against uprobe_mmap(), see the
-			 * comment near uprobe_hash().
-			 */
-			if (err == -EEXIST)
-				err = 0;
-		} else {
+		else
 			remove_breakpoint(uprobe, mm, info->vaddr);
-		}
+
  unlock:
 		up_write(&mm->mmap_sem);
  free:
-- 
1.5.5.1

[PATCH 5/7] uprobes: introduce MMF_HAS_UPROBES

[Oleg Nesterov]

Add the new MMF_HAS_UPROBES flag. It is set by install_breakpoint()
and it is copied by dup_mmap(), uprobe_pre_sstep_notifier() checks
it to avoid the slow path if the task was never probed. Perhaps it
makes sense to check it in valid_vma(is_register => false) as well.

This needs the new dup_mmap()->uprobe_dup_mmap() hook. We can't use
uprobe_reset_state() or put MMF_HAS_UPROBES into MMF_INIT_MASK, we
need oldmm->mmap_sem to avoid the race with uprobe_register() or
mmap() from another thread.

Currently we never clear this bit, it can be false-positive after
uprobe_unregister() or uprobe_munmap() or if dup_mmap() hits the
probed VM_DONTCOPY vma. But this is fine correctness-wise and has
no effect unless the task hits the non-uprobe breakpoint.

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

diff --git a/include/linux/sched.h b/include/linux/sched.h
index cbf3a71..c0fcfb7 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -441,6 +441,8 @@ extern int get_dumpable(struct mm_struct *mm);
 #define MMF_VM_HUGEPAGE		17	/* set when VM_HUGEPAGE is set on vma */
 #define MMF_EXE_FILE_CHANGED	18	/* see prctl_set_mm_exe_file() */
 
+#define MMF_HAS_UPROBES		19	/* might have uprobes */
+
 #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
 
 struct sighand_struct {
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 03ae547..4a37ab1 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -108,6 +108,7 @@ extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_con
 extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern int uprobe_mmap(struct vm_area_struct *vma);
 extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end);
+extern void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm);
 extern void uprobe_free_utask(struct task_struct *t);
 extern void uprobe_copy_process(struct task_struct *t);
 extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
@@ -138,6 +139,10 @@ static inline void
 uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end)
 {
 }
+static inline void
+uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
+{
+}
 static inline void uprobe_notify_resume(struct pt_regs *regs)
 {
 }
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 309309e..2a42319 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -649,6 +649,7 @@ static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 			struct vm_area_struct *vma, unsigned long vaddr)
 {
+	bool first_uprobe;
 	int ret;
 
 	/*
@@ -680,7 +681,17 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 		uprobe->flags |= UPROBE_COPY_INSN;
 	}
 
+	/*
+	 * set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
+	 * the task can hit this breakpoint right after __replace_page().
+	 */
+	first_uprobe = !test_bit(MMF_HAS_UPROBES, &mm->flags);
+	if (first_uprobe)
+		set_bit(MMF_HAS_UPROBES, &mm->flags);
+
 	ret = set_swbp(&uprobe->arch, mm, vaddr);
+	if (ret && first_uprobe)
+		clear_bit(MMF_HAS_UPROBES, &mm->flags);
 
 	return ret;
 }
@@ -1034,6 +1045,9 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
 	if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
 		return;
 
+	if (!test_bit(MMF_HAS_UPROBES, &vma->vm_mm->flags))
+		return;
+
 	/* TODO: unmapping uprobe(s) will need more work */
 }
 
@@ -1144,6 +1158,12 @@ void uprobe_reset_state(struct mm_struct *mm)
 	mm->uprobes_state.xol_area = NULL;
 }
 
+void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
+{
+	if (test_bit(MMF_HAS_UPROBES, &oldmm->flags))
+		set_bit(MMF_HAS_UPROBES, &newmm->flags);
+}
+
 /*
  *  - search for a free slot.
  */
@@ -1511,7 +1531,7 @@ int uprobe_pre_sstep_notifier(struct pt_regs *regs)
 {
 	struct uprobe_task *utask;
 
-	if (!current->mm)
+	if (!current->mm || !test_bit(MMF_HAS_UPROBES, &current->mm->flags))
 		return 0;
 
 	utask = current->utask;
diff --git a/kernel/fork.c b/kernel/fork.c
index 0e419ab..ab970c3 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -350,6 +350,7 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
 
 	down_write(&oldmm->mmap_sem);
 	flush_cache_dup_mm(oldmm);
+	uprobe_dup_mmap(oldmm, mm);
 	/*
 	 * Not linked in yet - no deadlock potential:
 	 */
-- 
1.5.5.1

[PATCH 6/7] uprobes: fold uprobe_reset_state() into uprobe_dup_mmap()

[Oleg Nesterov]

Now that we have uprobe_dup_mmap() we can fold uprobe_reset_state()
into the new hook and remove it. mmput()->uprobe_clear_state() can't
be called before dup_mmap().

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

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 4a37ab1..30297f9 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -118,7 +118,6 @@ extern void uprobe_notify_resume(struct pt_regs *regs);
 extern bool uprobe_deny_signal(void);
 extern bool __weak arch_uprobe_skip_sstep(struct arch_uprobe *aup, struct pt_regs *regs);
 extern void uprobe_clear_state(struct mm_struct *mm);
-extern void uprobe_reset_state(struct mm_struct *mm);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
@@ -163,8 +162,5 @@ static inline void uprobe_copy_process(struct task_struct *t)
 static inline void uprobe_clear_state(struct mm_struct *mm)
 {
 }
-static inline void uprobe_reset_state(struct mm_struct *mm)
-{
-}
 #endif /* !CONFIG_UPROBES */
 #endif	/* _LINUX_UPROBES_H */
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 2a42319..81fb2d8 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1150,16 +1150,10 @@ void uprobe_clear_state(struct mm_struct *mm)
 	kfree(area);
 }
 
-/*
- * uprobe_reset_state - Free the area allocated for slots.
- */
-void uprobe_reset_state(struct mm_struct *mm)
-{
-	mm->uprobes_state.xol_area = NULL;
-}
-
 void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
 {
+	newmm->uprobes_state.xol_area = NULL;
+
 	if (test_bit(MMF_HAS_UPROBES, &oldmm->flags))
 		set_bit(MMF_HAS_UPROBES, &newmm->flags);
 }
diff --git a/kernel/fork.c b/kernel/fork.c
index ab970c3..5555f51 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -836,8 +836,6 @@ struct mm_struct *dup_mm(struct task_struct *tsk)
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 	mm->pmd_huge_pte = NULL;
 #endif
-	uprobe_reset_state(mm);
-
 	if (!mm_init(mm, tsk))
 		goto fail_nomem;
 
-- 
1.5.5.1

[PATCH 7/7] uprobes: remove "verify" argument from set_orig_insn()

[Oleg Nesterov]

Nobody does set_orig_insn(verify => false), and I think nobody will.
Remove this argument. IIUC set_orig_insn(verify => false) was needed
to single-step without xol area.

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

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 30297f9..6d4fe79 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -102,7 +102,7 @@ struct uprobes_state {
 };
 
 extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
-extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm,  unsigned long vaddr, bool verify);
+extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
 extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
 extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 81fb2d8..25c0d74 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -347,24 +347,22 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned
  * @mm: the probed process address space.
  * @auprobe: arch specific probepoint information.
  * @vaddr: the virtual address to insert the opcode.
- * @verify: if true, verify existance of breakpoint instruction.
  *
  * For mm @mm, restore the original opcode (opcode) at @vaddr.
  * Return 0 (success) or a negative errno.
  */
 int __weak
-set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr, bool verify)
+set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr)
 {
-	if (verify) {
-		int result;
+	int result;
+
+	result = is_swbp_at_addr(mm, vaddr);
+	if (!result)
+		return -EINVAL;
 
-		result = is_swbp_at_addr(mm, vaddr);
-		if (!result)
-			return -EINVAL;
+	if (result != 1)
+		return result;
 
-		if (result != 1)
-			return result;
-	}
 	return write_opcode(auprobe, mm, vaddr, *(uprobe_opcode_t *)auprobe->insn);
 }
 
@@ -699,7 +697,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 static void
 remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr)
 {
-	set_orig_insn(&uprobe->arch, mm, vaddr, true);
+	set_orig_insn(&uprobe->arch, mm, vaddr);
 }
 
 /*
-- 
1.5.5.1

Re: [PATCH 5/7] uprobes: introduce MMF_HAS_UPROBES

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-08-08 19:37:47]:

> Add the new MMF_HAS_UPROBES flag. It is set by install_breakpoint()
> and it is copied by dup_mmap(), uprobe_pre_sstep_notifier() checks
> it to avoid the slow path if the task was never probed. Perhaps it
> makes sense to check it in valid_vma(is_register => false) as well.
> 
> This needs the new dup_mmap()->uprobe_dup_mmap() hook. We can't use
> uprobe_reset_state() or put MMF_HAS_UPROBES into MMF_INIT_MASK, we
> need oldmm->mmap_sem to avoid the race with uprobe_register() or
> mmap() from another thread.
> 
> Currently we never clear this bit, it can be false-positive after
> uprobe_unregister() or uprobe_munmap() or if dup_mmap() hits the
> probed VM_DONTCOPY vma. But this is fine correctness-wise and has
> no effect unless the task hits the non-uprobe breakpoint.
> 

In which case, cant we just delete uprobe_munmap() altogether.

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  include/linux/sched.h   |    2 ++
>  include/linux/uprobes.h |    5 +++++
>  kernel/events/uprobes.c |   22 +++++++++++++++++++++-
>  kernel/fork.c           |    1 +
>  4 files changed, 29 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index cbf3a71..c0fcfb7 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -441,6 +441,8 @@ extern int get_dumpable(struct mm_struct *mm);
>  #define MMF_VM_HUGEPAGE		17	/* set when VM_HUGEPAGE is set on vma */
>  #define MMF_EXE_FILE_CHANGED	18	/* see prctl_set_mm_exe_file() */
> 
> +#define MMF_HAS_UPROBES		19	/* might have uprobes */
> +
>  #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
> 
>  struct sighand_struct {
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 03ae547..4a37ab1 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -108,6 +108,7 @@ extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_con
>  extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
>  extern int uprobe_mmap(struct vm_area_struct *vma);
>  extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end);
> +extern void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm);
>  extern void uprobe_free_utask(struct task_struct *t);
>  extern void uprobe_copy_process(struct task_struct *t);
>  extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
> @@ -138,6 +139,10 @@ static inline void
>  uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end)
>  {
>  }
> +static inline void
> +uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
> +{
> +}
>  static inline void uprobe_notify_resume(struct pt_regs *regs)
>  {
>  }
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 309309e..2a42319 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -649,6 +649,7 @@ static int
>  install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  			struct vm_area_struct *vma, unsigned long vaddr)
>  {
> +	bool first_uprobe;
>  	int ret;
> 
>  	/*
> @@ -680,7 +681,17 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  		uprobe->flags |= UPROBE_COPY_INSN;
>  	}
> 
> +	/*
> +	 * set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
> +	 * the task can hit this breakpoint right after __replace_page().
> +	 */
> +	first_uprobe = !test_bit(MMF_HAS_UPROBES, &mm->flags);
> +	if (first_uprobe)
> +		set_bit(MMF_HAS_UPROBES, &mm->flags);
> +
>  	ret = set_swbp(&uprobe->arch, mm, vaddr);
> +	if (ret && first_uprobe)
> +		clear_bit(MMF_HAS_UPROBES, &mm->flags);
> 
>  	return ret;
>  }
> @@ -1034,6 +1045,9 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
>  	if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
>  		return;
> 
> +	if (!test_bit(MMF_HAS_UPROBES, &vma->vm_mm->flags))
> +		return;
> +

I am not sure whats the purpose of the above test 


>  	/* TODO: unmapping uprobe(s) will need more work */

and I am unable to think what more we would want to do here.

>  }
> 
> @@ -1144,6 +1158,12 @@ void uprobe_reset_state(struct mm_struct *mm)
>  	mm->uprobes_state.xol_area = NULL;
>  }
> 
> +void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
> +{
> +	if (test_bit(MMF_HAS_UPROBES, &oldmm->flags))
> +		set_bit(MMF_HAS_UPROBES, &newmm->flags);
> +}
> +
>  /*
>   *  - search for a free slot.
>   */
> @@ -1511,7 +1531,7 @@ int uprobe_pre_sstep_notifier(struct pt_regs *regs)
>  {
>  	struct uprobe_task *utask;
> 
> -	if (!current->mm)
> +	if (!current->mm || !test_bit(MMF_HAS_UPROBES, &current->mm->flags))
>  		return 0;
> 
>  	utask = current->utask;
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 0e419ab..ab970c3 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -350,6 +350,7 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
> 
>  	down_write(&oldmm->mmap_sem);
>  	flush_cache_dup_mm(oldmm);
> +	uprobe_dup_mmap(oldmm, mm);
>  	/*
>  	 * Not linked in yet - no deadlock potential:
>  	 */
> -- 
> 1.5.5.1
> 

Re: [PATCH 7/7] uprobes: remove "verify" argument from set_orig_insn()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-08-08 19:37:52]:

> Nobody does set_orig_insn(verify => false), and I think nobody will.
> Remove this argument. IIUC set_orig_insn(verify => false) was needed
> to single-step without xol area.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  include/linux/uprobes.h |    2 +-
>  kernel/events/uprobes.c |   20 +++++++++-----------
>  2 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 30297f9..6d4fe79 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -102,7 +102,7 @@ struct uprobes_state {
>  };
> 
>  extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> -extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm,  unsigned long vaddr, bool verify);
> +extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
>  extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
>  extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
>  extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 81fb2d8..25c0d74 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -347,24 +347,22 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned
>   * @mm: the probed process address space.
>   * @auprobe: arch specific probepoint information.
>   * @vaddr: the virtual address to insert the opcode.
> - * @verify: if true, verify existance of breakpoint instruction.
>   *
>   * For mm @mm, restore the original opcode (opcode) at @vaddr.
>   * Return 0 (success) or a negative errno.
>   */
>  int __weak
> -set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr, bool verify)
> +set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr)
>  {
> -	if (verify) {
> -		int result;
> +	int result;
> +
> +	result = is_swbp_at_addr(mm, vaddr);
> +	if (!result)
> +		return -EINVAL;
> 
> -		result = is_swbp_at_addr(mm, vaddr);
> -		if (!result)
> -			return -EINVAL;
> +	if (result != 1)
> +		return result;
> 
> -		if (result != 1)
> -			return result;
> -	}
>  	return write_opcode(auprobe, mm, vaddr, *(uprobe_opcode_t *)auprobe->insn);
>  }
> 
> @@ -699,7 +697,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  static void
>  remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr)
>  {
> -	set_orig_insn(&uprobe->arch, mm, vaddr, true);
> +	set_orig_insn(&uprobe->arch, mm, vaddr);
>  }
> 
>  /*
> -- 
> 1.5.5.1
> 

Re: [PATCH 5/7] uprobes: introduce MMF_HAS_UPROBES

[Oleg Nesterov]

On 08/09, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-08-08 19:37:47]:
>
> > Add the new MMF_HAS_UPROBES flag. It is set by install_breakpoint()
> > and it is copied by dup_mmap(), uprobe_pre_sstep_notifier() checks
> > it to avoid the slow path if the task was never probed. Perhaps it
> > makes sense to check it in valid_vma(is_register => false) as well.
> >
> > This needs the new dup_mmap()->uprobe_dup_mmap() hook. We can't use
> > uprobe_reset_state() or put MMF_HAS_UPROBES into MMF_INIT_MASK, we
> > need oldmm->mmap_sem to avoid the race with uprobe_register() or
> > mmap() from another thread.
> >
> > Currently we never clear this bit, it can be false-positive after
> > uprobe_unregister() or uprobe_munmap() or if dup_mmap() hits the
> > probed VM_DONTCOPY vma. But this is fine correctness-wise and has
> > no effect unless the task hits the non-uprobe breakpoint.
> >
>
> In which case, cant we just delete uprobe_munmap() altogether.

>From 0/7:

	The next series will teach uprobes to
	clear MMF_HAS_UPROBES, but perhaps we should simply remove
	uprobe_munmap() instead.

Yes, after this series uprobe_munmap() is nop, but see below.

> > @@ -1034,6 +1045,9 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
> >  	if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
> >  		return;
> >
> > +	if (!test_bit(MMF_HAS_UPROBES, &vma->vm_mm->flags))
> > +		return;
> > +
>
> I am not sure whats the purpose of the above test
>
>
>
> >  	/* TODO: unmapping uprobe(s) will need more work */
>
> and I am unable to think what more we would want to do here.

The next series will add MMF_UPROBE_RECALC, this bits indicates that
MMF_HAS_UPROBES can be false-positive. uprobe_munmap() will roughly do

	if (find_node_in_range(start, end))
		set_bit(MMF_UPROBE_RECALC);

Once again, I am not sure we really need more complications, we will
discuss this later and decide. If we do not want them, we can kill
uprobe_munmap().

Just in case... uprobe_dup_mmap() is very simple but "sub-optimal".
We can improve this logic if we add uprobe_dup_vma() instead which
does

	if (test_bit(MMF_HAS_UPROBES))
		return;
	if (find_node_in_range(...))
		set_bit(MMF_HAS_UPROBES);

But again, it would be better to discuss this later.

Oleg.

Re: [PATCH 1/7] uprobes: kill uprobes_state->count

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-08-08 19:37:37]:

> uprobes_state->count is only needed to avoid the slow path in
> uprobe_pre_sstep_notifier(). It is also checked in uprobe_munmap()
> but ironically its only goal to decrement this counter. However,
> it is very broken. Just some examples:
> 
> - uprobe_mmap() can race with uprobe_unregister() and wrongly
>   increment the counter if it hits the non-uprobe "int3". Note
>   that install_breakpoint() checks ->consumers first and returns
>   -EEXIST if it is NULL.
> 
>   "atomic_sub() if error" in uprobe_mmap() looks obviously wrong
>   too.
> 
> - uprobe_munmap() can race with uprobe_register() and wrongly
>   decrement the counter by the same reason.
> 
> - Suppose an appication tries to increase the mmapped area via
>   sys_mremap(). vma_adjust() does uprobe_munmap(whole_vma) first,
>   this can nullify the counter temporarily and race with another
>   thread which can hit the bp, the application will be killed by
>   SIGTRAP.
> 
> - Suppose an application mmaps 2 consecutive areas in the same file
>   and one (or both) of these areas has uprobes. In the likely case
>   mmap_region()->vma_merge() suceeds. Like above, this leads to
>   uprobe_munmap/uprobe_mmap from vma_merge()->vma_adjust() but then
>   mmap_region() does another uprobe_mmap(resulting_vma) and doubles
>   the counter.
> 
> This patch only removes this counter and fixes the compile errors,
> then we will try to cleanup the changed code and add something else
> instead.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

Re: [PATCH 2/7] uprobes: kill dup_mmap()->uprobe_mmap(), simplify uprobe_mmap/munmap

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-08-08 19:37:39]:

> 1. Kill dup_mmap()->uprobe_mmap(), it was only needed to calculate
>    new_mm->uprobes_state.count removed by the previous patch.
> 
>    If the forking process has a pending uprobe (int3) in vma, it will
>    be copied by copy_page_range(), note that it checks vma->anon_vma
>    so "Don't copy ptes" is not possible after install_breakpoint()
>    which does anon_vma_prepare().
> 
> 2. Remove is_swbp_at_addr() and "int count" in uprobe_mmap(). Again,
>    this was needed for uprobes_state.count.
> 
>    As a side effect this fixes the bug pointed out by Srikar,
>    this code lacked the necessary put_uprobe().
> 
> 3. uprobe_munmap() becomes a nop after the previous patch. Remove the
>    meaningless code but do not remove the helper, we will need it.

We could have removed uprobe_munmap() for now and reintroduced it. But
its fine to keep it this way for now.

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

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

Re: [PATCH 3/7] uprobes: change uprobe_mmap() to ignore the errors but check fatal_signal_pending()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-08-08 19:37:42]:

> Once install_breakpoint() fails uprobe_mmap() "ignores" all other
> uprobes and returns the error.
> 
> It was never really needed to to stop after the first error, and
> in fact it was always wrong at least in -ENOTSUPP case.
> 
> Change uprobe_mmap() to ignore the errors and always return 0.
> This is not what we want in the long term, but until we teach
> the callers to handle the failure it would be better to remove
> the pointless complications. And this doesn't look too bad, the
> only "reasonable" error is ENOMEM but in this case the caller
> should be oom-killed in the likely case or the system has more
> serious problems.
> 
> However it makes sense to stop if fatal_signal_pending() == T.
> In particular this helps if the task was oom-killed.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Re: [PATCH 4/7] uprobes: do not use -EEXIST in install_breakpoint() paths

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-08-08 19:37:44]:

> -EEXIST from install_breakpoint() no longer makes sense, all
> callers should simply treat it as "success". Change the code
> to return zero and simplify register_for_each_vma().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Re: [PATCH 5/7] uprobes: introduce MMF_HAS_UPROBES

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-08-08 19:37:47]:

> Add the new MMF_HAS_UPROBES flag. It is set by install_breakpoint()
> and it is copied by dup_mmap(), uprobe_pre_sstep_notifier() checks
> it to avoid the slow path if the task was never probed. Perhaps it
> makes sense to check it in valid_vma(is_register => false) as well.
> 
> This needs the new dup_mmap()->uprobe_dup_mmap() hook. We can't use
> uprobe_reset_state() or put MMF_HAS_UPROBES into MMF_INIT_MASK, we
> need oldmm->mmap_sem to avoid the race with uprobe_register() or
> mmap() from another thread.
> 
> Currently we never clear this bit, it can be false-positive after
> uprobe_unregister() or uprobe_munmap() or if dup_mmap() hits the
> probed VM_DONTCOPY vma. But this is fine correctness-wise and has
> no effect unless the task hits the non-uprobe breakpoint.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Re: [PATCH 6/7] uprobes: fold uprobe_reset_state() into uprobe_dup_mmap()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-08-08 19:37:49]:

> Now that we have uprobe_dup_mmap() we can fold uprobe_reset_state()
> into the new hook and remove it. mmput()->uprobe_clear_state() can't
> be called before dup_mmap().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>