Skip to contentSkip to conversation

uprobes: kill uprobes_state->count, add MMF_HAS_UPROBES

PATCH In review Oleg Nesterov 2 participants 17 messages Aug 8, 2012 – Aug 13, 2012

Subsystems (5)
perf events MM core Scheduler exec and binfmt +1 more subsystems
lore
Share & tools
lore

Oleg Nesterov

author maintainer
#0 lore raw

cover uprobes: kill uprobes_state->count, add MMF_HAS_UPROBES

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

PATCH 1/7 uprobes: kill uprobes_state->count

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 <[email protected]>
---
 include/linux/uprobes.h |    2 +-
 kernel/events/uprobes.c |   38 ++------------------------------------
 2 files changed, 3 insertions(+), 37 deletions(-)
include/linux/uprobes.h +1 -1
index efe4b33..03ae547 100644
@@ -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);
kernel/events/uprobes.c +2 -36
index 940005d..f0bb387 100644
@@ -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;
Signature · 1 line
1.5.5.1
Acked-by on v1 1/7
#2 lore raw

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

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

Quoted 5 lines

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 <[email protected]>
Acked-by Srikar Dronamraju <[email protected]>
author maintainer
#3 lore raw

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

In reply to 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 <[email protected]>
---
 kernel/events/uprobes.c |   30 +++---------------------------
 kernel/fork.c           |    3 ---
 2 files changed, 3 insertions(+), 30 deletions(-)
kernel/events/uprobes.c +3 -27
index f0bb387..08ea627 100644
@@ -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 */
kernel/fork.c +0 -3
index 54bb88a..0e419ab 100644
@@ -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);
Signature · 1 line
1.5.5.1
Acked-by on v1 2/7
#4 lore raw

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

Quoted 8 lines · 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().

Quoted 8 lines

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.

Quoted 2 lines
Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Srikar Dronamraju <[email protected]>
author maintainer
#5 lore raw

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

In reply to 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 <[email protected]>
---
 kernel/events/uprobes.c |   27 ++++++---------------------
 1 files changed, 6 insertions(+), 21 deletions(-)
kernel/events/uprobes.c +6 -21
index 08ea627..2e269d1 100644
@@ -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;
 }
 
 /*
Signature · 1 line
1.5.5.1
Acked-by on v1 3/7
#6 lore raw

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

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

Quoted 4 lines

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 <[email protected]>
Acked-by Srikar Dronamraju <[email protected]>
author maintainer
#7 lore raw

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

In reply to 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 <[email protected]>
---
 kernel/events/uprobes.c |   16 +++++-----------
 1 files changed, 5 insertions(+), 11 deletions(-)
kernel/events/uprobes.c +5 -11
index 2e269d1..309309e 100644
@@ -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:
Signature · 1 line
1.5.5.1
author maintainer
#9 lore raw

PATCH 5/7 uprobes: introduce MMF_HAS_UPROBES

In reply to 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 <[email protected]>
---
 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(-)
include/linux/sched.h +2 -0
index cbf3a71..c0fcfb7 100644
@@ -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 {
include/linux/uprobes.h +5 -0
index 03ae547..4a37ab1 100644
@@ -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)
 {
 }
kernel/events/uprobes.c +21 -1
index 309309e..2a42319 100644
@@ -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;
kernel/fork.c +1 -0
index 0e419ab..ab970c3 100644
@@ -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:
 	 */
Signature · 1 line
1.5.5.1
#10 lore raw

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

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

Quoted 5 lines

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.

Quoted 75 lines
Signed-off-by Oleg Nesterov <[email protected]>
---
 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(-)
include/linux/sched.h +2 -0
index cbf3a71..c0fcfb7 100644
@@ -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 {
include/linux/uprobes.h +5 -0
index 03ae547..4a37ab1 100644
@@ -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)
 {
 }
kernel/events/uprobes.c +11 -0
index 309309e..2a42319 100644
@@ -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;
 }

kernel/events/uprobes.c void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon · line 1050

Quoted 6 lines
kernel/events/uprobes.c +3 -0
@@ -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

kernel/events/uprobes.c void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon · line 1051

Quoted 1 line
kernel/events/uprobes.c +0 -0
 	/* TODO: unmapping uprobe(s) will need more work */

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

Quoted 39 lines
kernel/events/uprobes.c +7 -1
 }
 

@@ -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;
kernel/fork.c +1 -0
index 0e419ab..ab970c3 100644
@@ -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:
 	 */
Signature · 1 line
1.5.5.1
author maintainer
#11 lore raw

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

Quoted 19 lines · Srikar Dronamraju
Quoted 10 lines · 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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 12 lines

kernel/events/uprobes.c void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon · line 1050

Quoted 7 lines
kernel/events/uprobes.c +3 -0
@@ -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

Quoted 3 lines

kernel/events/uprobes.c void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon · line 1051

Quoted 1 line
kernel/events/uprobes.c +0 -0
 	/* 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.

Acked-by on v1 5/7
#12 lore raw

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

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

Quoted 6 lines

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 <[email protected]>
Acked-by Srikar Dronamraju <[email protected]>
author maintainer
#13 lore raw

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

In reply to 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 <[email protected]>
---
 include/linux/uprobes.h |    4 ----
 kernel/events/uprobes.c |   10 ++--------
 kernel/fork.c           |    2 --
 3 files changed, 2 insertions(+), 14 deletions(-)
include/linux/uprobes.h +0 -4
index 4a37ab1..30297f9 100644
@@ -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 */
kernel/events/uprobes.c +2 -8
index 2a42319..81fb2d8 100644
@@ -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);
 }
kernel/fork.c +0 -2
index ab970c3..5555f51 100644
@@ -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;
Signature · 1 line
1.5.5.1
author maintainer
#15 lore raw

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

In reply to 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 <[email protected]>
---
 include/linux/uprobes.h |    2 +-
 kernel/events/uprobes.c |   20 +++++++++-----------
 2 files changed, 10 insertions(+), 12 deletions(-)
include/linux/uprobes.h +1 -1
index 30297f9..6d4fe79 100644
@@ -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);
kernel/events/uprobes.c +9 -11
index 81fb2d8..25c0d74 100644
@@ -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);
 }
 
 /*
Signature · 1 line
1.5.5.1
Acked-by on v1 7/7
#16 lore raw

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

Quoted 5 lines · 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 <[email protected]>
Acked-by Srikar Dronamraju <[email protected]>
Quoted 67 lines
---
 include/linux/uprobes.h |    2 +-
 kernel/events/uprobes.c |   20 +++++++++-----------
 2 files changed, 10 insertions(+), 12 deletions(-)
include/linux/uprobes.h +1 -1
index 30297f9..6d4fe79 100644
@@ -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);
kernel/events/uprobes.c +9 -11
index 81fb2d8..25c0d74 100644
@@ -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);
 }
 
 /*
Signature · 1 line
1.5.5.1