uprobes: teach uprobes to clear MMF_HAS_UPROBES

Nobody plays with uprobes_tree/uprobes_treelock in interrupt context, no need to disable irqs.

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

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

kernel/events/uprobes.c +8 -13

index 25c0d74..abe077e 100644
@@ -413,11 +413,10 @@ static struct uprobe *__find_uprobe(struct inode *inode, loff_t offset)
 static struct uprobe *find_uprobe(struct inode *inode, loff_t offset)
 {
 	struct uprobe *uprobe;
-	unsigned long flags;
 
-	spin_lock_irqsave(&uprobes_treelock, flags);
+	spin_lock(&uprobes_treelock);
 	uprobe = __find_uprobe(inode, offset);
-	spin_unlock_irqrestore(&uprobes_treelock, flags);
+	spin_unlock(&uprobes_treelock);
 
 	return uprobe;
 }

@@ -464,12 +463,11 @@ static struct uprobe *__insert_uprobe(struct uprobe *uprobe)
  */
 static struct uprobe *insert_uprobe(struct uprobe *uprobe)
 {
-	unsigned long flags;
 	struct uprobe *u;
 
-	spin_lock_irqsave(&uprobes_treelock, flags);
+	spin_lock(&uprobes_treelock);
 	u = __insert_uprobe(uprobe);
-	spin_unlock_irqrestore(&uprobes_treelock, flags);
+	spin_unlock(&uprobes_treelock);
 
 	/* For now assume that the instruction need not be single-stepped */
 	uprobe->flags |= UPROBE_SKIP_SSTEP;

@@ -707,11 +705,9 @@ remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vad
  */
 static void delete_uprobe(struct uprobe *uprobe)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&uprobes_treelock, flags);
+	spin_lock(&uprobes_treelock);
 	rb_erase(&uprobe->rb_node, &uprobes_tree);
-	spin_unlock_irqrestore(&uprobes_treelock, flags);
+	spin_unlock(&uprobes_treelock);
 	iput(uprobe->inode);
 	put_uprobe(uprobe);
 	atomic_dec(&uprobe_events);

@@ -969,7 +965,6 @@ static void build_probe_list(struct inode *inode,
 				struct list_head *head)
 {
 	loff_t min, max;
-	unsigned long flags;
 	struct rb_node *n, *t;
 	struct uprobe *u;
 

@@ -977,7 +972,7 @@ static void build_probe_list(struct inode *inode,
 	min = vaddr_to_offset(vma, start);
 	max = min + (end - start) - 1;
 
-	spin_lock_irqsave(&uprobes_treelock, flags);
+	spin_lock(&uprobes_treelock);
 	n = find_node_in_range(inode, min, max);
 	if (n) {
 		for (t = n; t; t = rb_prev(t)) {

@@ -995,7 +990,7 @@ static void build_probe_list(struct inode *inode,
 			atomic_inc(&u->ref);
 		}
 	}
-	spin_unlock_irqrestore(&uprobes_treelock, flags);
+	spin_unlock(&uprobes_treelock);
 }
 
 /*

Signature · 1 line

1.5.5.1

Add the new MMF_RECALC_UPROBES flag, it means that MMF_HAS_UPROBES can be false positive after remove_breakpoint() or uprobe_munmap(). It is also set by uprobe_dup_mmap(), this is not optimal but simple. We could add the new hook, uprobe_dup_vma(), to set MMF_HAS_UPROBES only if the new mm actually has uprobes, but I don't think this makes sense.

The next patch will use this flag to clear MMF_HAS_UPROBES.

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

---
 include/linux/sched.h   |    3 ++-
 kernel/events/uprobes.c |   39 +++++++++++++++++++++++++++++++++++----
 2 files changed, 37 insertions(+), 5 deletions(-)

include/linux/sched.h +2 -1

index c0fcfb7..d6899b0 100644
@@ -441,7 +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_HAS_UPROBES		19	/* has uprobes */
+#define MMF_RECALC_UPROBES	20	/* MMF_HAS_UPROBES can be wrong */
 
 #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
 

kernel/events/uprobes.c +35 -4

index abe077e..176de8c 100644
@@ -686,7 +686,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 		set_bit(MMF_HAS_UPROBES, &mm->flags);
 
 	ret = set_swbp(&uprobe->arch, mm, vaddr);
-	if (ret && first_uprobe)
+	if (!ret)
+		clear_bit(MMF_RECALC_UPROBES, &mm->flags);
+	else if (first_uprobe)
 		clear_bit(MMF_HAS_UPROBES, &mm->flags);
 
 	return ret;

@@ -695,6 +697,11 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 static void
 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;
+
+	set_bit(MMF_RECALC_UPROBES, &mm->flags);
 	set_orig_insn(&uprobe->arch, mm, vaddr);
 }
 

@@ -1027,6 +1034,25 @@ int uprobe_mmap(struct vm_area_struct *vma)
 	return 0;
 }
 
+static bool
+vma_has_uprobes(struct vm_area_struct *vma, unsigned long start, unsigned long end)
+{
+	loff_t min, max;
+	struct inode *inode;
+	struct rb_node *n;
+
+	inode = vma->vm_file->f_mapping->host;
+
+	min = vaddr_to_offset(vma, start);
+	max = min + (end - start) - 1;
+
+	spin_lock(&uprobes_treelock);
+	n = find_node_in_range(inode, min, max);
+	spin_unlock(&uprobes_treelock);
+
+	return !!n;
+}
+
 /*
  * Called in context of a munmap of a vma.
  */

@@ -1038,10 +1064,12 @@ 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))
+	if (!test_bit(MMF_HAS_UPROBES, &vma->vm_mm->flags) ||
+	     test_bit(MMF_RECALC_UPROBES, &vma->vm_mm->flags))
 		return;
 
-	/* TODO: unmapping uprobe(s) will need more work */
+	if (vma_has_uprobes(vma, start, end))
+		set_bit(MMF_RECALC_UPROBES, &vma->vm_mm->flags);
 }
 
 /* Slot allocation for XOL */

@@ -1147,8 +1175,11 @@ 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))
+	if (test_bit(MMF_HAS_UPROBES, &oldmm->flags)) {
 		set_bit(MMF_HAS_UPROBES, &newmm->flags);
+		/* unconditionally, dup_mmap() skips VM_DONTCOPY vmas */
+		set_bit(MMF_RECALC_UPROBES, &newmm->flags);
+	}
 }
 
 /*

Signature · 1 line

1.5.5.1

The wrong MMF_HAS_UPROBES doesn't really hurt, just it triggers the "slow" and unnecessary handle_swbp() path if the task hits the non-uprobe breakpoint.

So this patch changes find_active_uprobe() to check every valid vma and clear MMF_HAS_UPROBES if no uprobes were found. This is adds the slow O(n) path, but it is only called in unlikely case when the task hits the normal breakpoint first time after uprobe_unregister().

Note the "not strictly accurate" comment in mmf_recalc_uprobes(). We can fix this, we only need to teach vma_has_uprobes() to return a bit more more info, but I am not sure this worth the trouble.

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

---
 kernel/events/uprobes.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

kernel/events/uprobes.c +3 -0

index 176de8c..0b7918c 100644
@@ -1397,6 +1397,25 @@ static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)
 	return false;
 }
 
+static void mmf_recalc_uprobes(struct mm_struct *mm)
+{
+	struct vm_area_struct *vma;

kernel/events/uprobes.c +9 -0

+			return;
+	}
+
+	clear_bit(MMF_HAS_UPROBES, &mm->flags);
+}
+
 static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 {
 	struct mm_struct *mm = current->mm;

@@ -1418,6 +1437,9 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 	} else {
 		*is_swbp = -EFAULT;
 	}
+
+	if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
+		mmf_recalc_uprobes(mm);
 	up_read(&mm->mmap_sem);
 
 	return uprobe;

Signature · 1 line

1.5.5.1

Quoted 3 lines · Oleg Nesterov

Note the "not strictly accurate" comment in mmf_recalc_uprobes(). We can fix this, we only need to teach vma_has_uprobes() to return a bit more more info, but I am not sure this worth the trouble.

...

kernel/events/uprobes.c static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs) · line 1412

Quoted 6 lines

kernel/events/uprobes.c +6 -0

+		/*
+		 * This is not strictly accurate, we can race with
+		 * uprobe_unregister() and see the already removed
+		 * uprobe if delete_uprobe() was not yet called.
+		 */
+		if (vma_has_uprobes(vma, vma->vm_start, vma->vm_end))

Should we set the MMF_RECALC_UPROBES here?

Its harmless but my thought was if we indeed saw a uprobe that was already deleted, then the next time we hit a non uprobe breakpoint in the same process context, we will not come here because MMF_RECALC_UPROBES is cleared.

Acked-by Srikar Dronamraju <[email protected]>

Hello.

On top of "kill uprobes_state->count, add MMF_HAS_UPROBES" series.

Once again, I am not sure we really need this (except 1/3). Perhaps we could remove uprobe_munmap() instead and forget about the false-positive MMF_HAS_UPROBES's.

OTOH, imho it looks a bit more clean this way. Please comment.

diffstat reports the code bloat, but note that the previous series removed more code.

Oleg.

 include/linux/sched.h   |    3 +-
 kernel/events/uprobes.c |   82 +++++++++++++++++++++++++++++++++++++----------
 2 files changed, 67 insertions(+), 18 deletions(-)

Srikar, this is still waiting for your review ;)

Quoted 2 lines · Oleg Nesterov

Srikar, this is still waiting for your review ;)

I was informed that Srikar is travelling and can't review this seris.

So I think it doesn't make sense to delay the already acked patches, the more testing the better ;) Yes, uprobe_munmap() looks strange, we should either remove it or apply this series. But we can do this later, and uprobes have other problems.

Ingo, please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes

Note: the patch from Sebastian wasn't acked by Srikar, but I hope this cleanup is simple enough.

Oleg Nesterov (7):
uprobes: Kill uprobes_state->count
uprobes: Kill dup_mmap()->uprobe_mmap(), simplify uprobe_mmap/munmap
uprobes: Change uprobe_mmap() to ignore the errors but check fatal_signal_pending()
uprobes: Do not use -EEXIST in install_breakpoint() paths
uprobes: Introduce MMF_HAS_UPROBES
uprobes: Fold uprobe_reset_state() into uprobe_dup_mmap()
uprobes: Remove "verify" argument from set_orig_insn()

Sebastian Andrzej Siewior (1):
uprobes: Remove check for uprobe variable in handle_swbp()

Srikar Dronamraju (1):
uprobes: Remove redundant lock_page/unlock_page

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

Hello.

On top of "kill uprobes_state->count, add MMF_HAS_UPROBES" series.

Once again, I am not sure we really need this (except 1/3). Perhaps we could remove uprobe_munmap() instead and forget about the false-positive MMF_HAS_UPROBES's.

OTOH, imho it looks a bit more clean this way. Please comment.

diffstat reports the code bloat, but note that the previous series removed more code.

Oleg.

 include/linux/sched.h   |    3 +-
 kernel/events/uprobes.c |   82 +++++++++++++++++++++++++++++++++++++----------
 2 files changed, 67 insertions(+), 18 deletions(-)