[PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES

[PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES

[Oleg Nesterov]

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(-)

[PATCH 1/3] uprobes: uprobes_treelock should not disable irqs

[Oleg Nesterov]

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

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 25c0d74..abe077e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -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);
 }
 
 /*
-- 
1.5.5.1

[PATCH 2/3] uprobes: introduce MMF_RECALC_UPROBES

[Oleg Nesterov]

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 <oleg@redhat.com>
---
 include/linux/sched.h   |    3 ++-
 kernel/events/uprobes.c |   39 +++++++++++++++++++++++++++++++++++----
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index c0fcfb7..d6899b0 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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)
 
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index abe077e..176de8c 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -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);
+	}
 }
 
 /*
-- 
1.5.5.1

[PATCH 3/3] uprobes: teach find_active_uprobe() to clear MMF_HAS_UPROBES

[Oleg Nesterov]

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 <oleg@redhat.com>
---
 kernel/events/uprobes.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 176de8c..0b7918c 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -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;
+
+	for (vma = mm->mmap; vma; vma = vma->vm_next) {
+		if (!valid_vma(vma, false))
+			continue;
+		/*
+		 * 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))
+			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;
-- 
1.5.5.1

Re: [PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES

[Oleg Nesterov]

Srikar, this is still waiting for your review ;)

FYI, I created the tree to accumulate the acked patches,
kernel.org:/pub/scm/linux/kernel/git/oleg/misc

Currently it has:

	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

You didn't ack Sebastian's patch, but it looks simple. If you
have any objections please let me know.

On 08/19, Oleg Nesterov wrote:
>
> 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(-)

[GIT PULL] (Was: uprobes: teach uprobes to clear MMF_HAS_UPROBES)

[Oleg Nesterov]

On 08/27, Oleg Nesterov wrote:
>
> 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(-)

Re: [GIT PULL] (Was: uprobes: teach uprobes to clear MMF_HAS_UPROBES)

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 08/27, Oleg Nesterov wrote:
> >
> > 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(-)

Pulled into tip:perf/core for v3.7, thanks a lot Oleg!

	Ingo

Re: [PATCH 1/3] uprobes: uprobes_treelock should not disable irqs

[Srikar Dronamraju]

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

> Nobody plays with uprobes_tree/uprobes_treelock in interrupt context,
> no need to disable irqs.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  kernel/events/uprobes.c |   21 ++++++++-------------
>  1 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 25c0d74..abe077e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -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);
>  }
> 
>  /*
> -- 
> 1.5.5.1
> 

Re: [PATCH 2/3] uprobes: introduce MMF_RECALC_UPROBES

[Srikar Dronamraju]

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

> 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 <oleg@redhat.com>

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

> ---
>  include/linux/sched.h   |    3 ++-
>  kernel/events/uprobes.c |   39 +++++++++++++++++++++++++++++++++++----
>  2 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index c0fcfb7..d6899b0 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -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)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index abe077e..176de8c 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -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);
> +	}
>  }
> 
>  /*
> -- 
> 1.5.5.1
> 

Re: [PATCH 3/3] uprobes: teach find_active_uprobe() to clear MMF_HAS_UPROBES

[Srikar Dronamraju]

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

> 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 <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 176de8c..0b7918c 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -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;
> +
> +	for (vma = mm->mmap; vma; vma = vma->vm_next) {
> +		if (!valid_vma(vma, false))
> +			continue;
> +		/*
> +		 * 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.

The rest looks fine. 


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


> +			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;
> -- 
> 1.5.5.1
> 

Re: [PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES

[Srikar Dronamraju]

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

> 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.


I am agree with the current approach.

The other approach could be to forget about MMF_RECALC_UPROBES and
uprobe_munmap() (essentially patch 2). But in patch 3 we will end up
calling mmf_recalc_uprobes() everytime a process with MM_HAS_UPROBES
hits a non-uprobe breakpoint.


> 
> 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(-)
> 

Re: [PATCH 3/3] uprobes: teach find_active_uprobe() to clear MMF_HAS_UPROBES

[Oleg Nesterov]

On 09/14, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-08-19 18:40:42]:
>
> > 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.
> ...
> > +		/*
> > +		 * 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?

Assuming uprobe was removed - yes.

> 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.

Yes, this is what the comment/changelog tries to document.

vma_has_uprobes() needs to check uprobe->consumers under uprobes_treelock
and return no|yes|maybe. Or it can do rb_prev/rb_entry until it finds the
live uprobe. We can do this later to improve this logic. And we can do
other improvements, say, fork() can be much more accurate but with more
complications.

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

Thanks,

Oleg.