[PATCH 0/4] uprobes: vma_address() fixlets

[PATCH 0/4] uprobes: vma_address() fixlets

[Oleg Nesterov]

Hello,

Another series with small fixes/cleanups, on top of
"uprobes: teach build_probe_list() to consider the range"
(still waiting for review from Srikar).

4/4 was previously nacked by Anton because vma_address()
conflicts with rmap.c. Now that 3/4 renames this helper
it is "safe" to incluse mm/internal.h, so I re-send this
patch (unchanged).

Oleg.

[PATCH 1/4] uprobes: introduce vaddr_to_offset(vma, vaddr)

[Oleg Nesterov]

Add the new helper, vaddr_to_offset(vma, vaddr) which returns the
offset in vma->vm_file this vaddr is mapped at.

Change build_probe_list() and find_active_uprobe() to use the new
helper, the next patch adds another user.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index c825404..5c87042 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -122,6 +122,11 @@ static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
 	return vaddr;
 }
 
+static loff_t vaddr_to_offset(struct vm_area_struct *vma, unsigned long vaddr)
+{
+	return ((loff_t)vma->vm_pgoff << PAGE_SHIFT) + (vaddr - vma->vm_start);
+}
+
 /**
  * __replace_page - replace page in vma by new page.
  * based on replace_page in mm/ksm.c
@@ -978,7 +983,7 @@ static void build_probe_list(struct inode *inode,
 	struct uprobe *u;
 
 	INIT_LIST_HEAD(head);
-	min = ((loff_t)vma->vm_pgoff << PAGE_SHIFT) + start - vma->vm_start;
+	min = vaddr_to_offset(vma, start);
 	max = min + (end - start) - 1;
 
 	spin_lock_irqsave(&uprobes_treelock, flags);
@@ -1442,12 +1447,9 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 	vma = find_vma(mm, bp_vaddr);
 	if (vma && vma->vm_start <= bp_vaddr) {
 		if (valid_vma(vma, false)) {
-			struct inode *inode;
-			loff_t offset;
+			struct inode *inode = vma->vm_file->f_mapping->host;
+			loff_t offset = vaddr_to_offset(vma, bp_vaddr);
 
-			inode = vma->vm_file->f_mapping->host;
-			offset = bp_vaddr - vma->vm_start;
-			offset += (loff_t)vma->vm_pgoff << PAGE_SHIFT;
 			uprobe = find_uprobe(inode, offset);
 		}
 
-- 
1.5.5.1

Re: [PATCH 1/4] uprobes: introduce vaddr_to_offset(vma, vaddr)

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-12 19:10:17]:

> Add the new helper, vaddr_to_offset(vma, vaddr) which returns the
> offset in vma->vm_file this vaddr is mapped at.
> 
> Change build_probe_list() and find_active_uprobe() to use the new
> helper, the next patch adds another user.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

[PATCH 2/4] uprobes: fix register_for_each_vma()->vma_address() check

[Oleg Nesterov]

1. register_for_each_vma() checks that vma_address() == vaddr but
   this is not enough. We should also ensure that vaddr >= vm_start,
   find_vma() guarantees "vaddr < vm_end" only.

2. After the prevous changes, register_for_each_vma() is the only
   reason why vma_address() has to return loff_t, all other users
   know that we have the valid mapping at this offset and thus the
   overflow is not possible.

   Change the code to use vaddr_to_offset() instead, imho this looks
   more clean/understandable and now we can change vma_address().

3. While at it, remove the unnecessary type-cast.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 5c87042..734e199 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -823,12 +823,13 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 			goto free;
 
 		down_write(&mm->mmap_sem);
-		vma = find_vma(mm, (unsigned long)info->vaddr);
-		if (!vma || !valid_vma(vma, is_register))
+		vma = find_vma(mm, info->vaddr);
+		if (!vma || !valid_vma(vma, is_register) ||
+		    vma->vm_file->f_mapping->host != uprobe->inode)
 			goto unlock;
 
-		if (vma->vm_file->f_mapping->host != uprobe->inode ||
-		    vma_address(vma, uprobe->offset) != info->vaddr)
+		if (vma->vm_start > info->vaddr ||
+		    vaddr_to_offset(vma, info->vaddr) != uprobe->offset)
 			goto unlock;
 
 		if (is_register) {
-- 
1.5.5.1

Re: [PATCH 2/4] uprobes: fix register_for_each_vma()->vma_address() check

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-12 19:10:20]:

> 1. register_for_each_vma() checks that vma_address() == vaddr but
>    this is not enough. We should also ensure that vaddr >= vm_start,
>    find_vma() guarantees "vaddr < vm_end" only.
> 
> 2. After the prevous changes, register_for_each_vma() is the only
>    reason why vma_address() has to return loff_t, all other users
>    know that we have the valid mapping at this offset and thus the
>    overflow is not possible.
> 
>    Change the code to use vaddr_to_offset() instead, imho this looks
>    more clean/understandable and now we can change vma_address().
> 
> 3. While at it, remove the unnecessary type-cast.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

[PATCH 3/4] uprobes: rename vma_address() and make it return "unsigned long"

[Oleg Nesterov]

1. vma_address() returns loff_t, this looks confusing and this is
   unnecessary after the previous change. Make it return "ulong",
   all callers truncate the result anyway.

2. Its name conflicts with mm/rmap.c:vma_address(), rename it to
   offset_to_vaddr(), this matches vaddr_to_offset().

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 734e199..fc1df43 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -112,14 +112,9 @@ static bool valid_vma(struct vm_area_struct *vma, bool is_register)
 	return false;
 }
 
-static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
+static unsigned long offset_to_vaddr(struct vm_area_struct *vma, loff_t offset)
 {
-	loff_t vaddr;
-
-	vaddr = vma->vm_start + offset;
-	vaddr -= (loff_t)vma->vm_pgoff << PAGE_SHIFT;
-
-	return vaddr;
+	return vma->vm_start + offset - ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
 }
 
 static loff_t vaddr_to_offset(struct vm_area_struct *vma, unsigned long vaddr)
@@ -775,7 +770,7 @@ build_map_info(struct address_space *mapping, loff_t offset, bool is_register)
 		curr = info;
 
 		info->mm = vma->vm_mm;
-		info->vaddr = vma_address(vma, offset);
+		info->vaddr = offset_to_vaddr(vma, offset);
 	}
 	mutex_unlock(&mapping->i_mmap_mutex);
 
@@ -1042,7 +1037,7 @@ int uprobe_mmap(struct vm_area_struct *vma)
 
 	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
 		if (!ret) {
-			loff_t vaddr = vma_address(vma, uprobe->offset);
+			loff_t vaddr = offset_to_vaddr(vma, uprobe->offset);
 
 			ret = install_breakpoint(uprobe, vma->vm_mm, vma, vaddr);
 			/*
@@ -1103,7 +1098,7 @@ 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) {
-		loff_t vaddr = vma_address(vma, uprobe->offset);
+		loff_t vaddr = offset_to_vaddr(vma, uprobe->offset);
 		/*
 		 * An unregister could have removed the probe before
 		 * unmap. So check before we decrement the count.
-- 
1.5.5.1

Re: [PATCH 3/4] uprobes: rename vma_address() and make it return "unsigned long"

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-12 19:10:22]:

> 1. vma_address() returns loff_t, this looks confusing and this is
>    unnecessary after the previous change. Make it return "ulong",
>    all callers truncate the result anyway.
> 
> 2. Its name conflicts with mm/rmap.c:vma_address(), rename it to
>    offset_to_vaddr(), this matches vaddr_to_offset().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

Re: [PATCH 3/4] uprobes: rename vma_address() and make it return "unsigned long"

[Oleg Nesterov]

On 07/26, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-07-12 19:10:22]:
>
> > 1. vma_address() returns loff_t, this looks confusing and this is
> >    unnecessary after the previous change. Make it return "ulong",
> >    all callers truncate the result anyway.
> >
> > 2. Its name conflicts with mm/rmap.c:vma_address(), rename it to
> >    offset_to_vaddr(), this matches vaddr_to_offset().
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thanks.

Could you please ack v2 below?

I forgot to update "loff_t vaddr" in mmap/munmap. IOW, this is the
same patch plus

	- loff_t vaddr = offset_to_vaddr(...);
	+ unsigned long vaddr = offset_to_vaddr(...);

in uprobe_mmap/munmap.

In fact I was going to do this in
"uprobes: teach build_probe_list() to consider the range" but
forgot as well.

Oleg.

--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -112,14 +112,9 @@ static bool valid_vma(struct vm_area_struct *vma, bool is_register)
 	return false;
 }
 
-static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
+static unsigned long offset_to_vaddr(struct vm_area_struct *vma, loff_t offset)
 {
-	loff_t vaddr;
-
-	vaddr = vma->vm_start + offset;
-	vaddr -= (loff_t)vma->vm_pgoff << PAGE_SHIFT;
-
-	return vaddr;
+	return vma->vm_start + offset - ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
 }
 
 static loff_t vaddr_to_offset(struct vm_area_struct *vma, unsigned long vaddr)
@@ -775,7 +770,7 @@ build_map_info(struct address_space *mapping, loff_t offset, bool is_register)
 		curr = info;
 
 		info->mm = vma->vm_mm;
-		info->vaddr = vma_address(vma, offset);
+		info->vaddr = offset_to_vaddr(vma, offset);
 	}
 	mutex_unlock(&mapping->i_mmap_mutex);
 
@@ -1042,7 +1037,7 @@ int uprobe_mmap(struct vm_area_struct *vma)
 
 	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
 		if (!ret) {
-			loff_t vaddr = vma_address(vma, uprobe->offset);
+			unsigned long vaddr = offset_to_vaddr(vma, uprobe->offset);
 
 			ret = install_breakpoint(uprobe, vma->vm_mm, vma, vaddr);
 			/*
@@ -1103,7 +1098,7 @@ 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) {
-		loff_t vaddr = vma_address(vma, uprobe->offset);
+		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.

Re: [PATCH 3/4] uprobes: rename vma_address() and make it return "unsigned long"

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-26 12:28:12]:

> On 07/26, Srikar Dronamraju wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> [2012-07-12 19:10:22]:
> >
> > > 1. vma_address() returns loff_t, this looks confusing and this is
> > >    unnecessary after the previous change. Make it return "ulong",
> > >    all callers truncate the result anyway.
> > >
> > > 2. Its name conflicts with mm/rmap.c:vma_address(), rename it to
> > >    offset_to_vaddr(), this matches vaddr_to_offset().
> > >
> > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> >
> > Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> 
> Thanks.
> 
> Could you please ack v2 below?
> 
> I forgot to update "loff_t vaddr" in mmap/munmap. IOW, this is the
> same patch plus
> 
> 	- loff_t vaddr = offset_to_vaddr(...);
> 	+ unsigned long vaddr = offset_to_vaddr(...);
> 
> in uprobe_mmap/munmap.
> 

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

> In fact I was going to do this in
> "uprobes: teach build_probe_list() to consider the range" but
> forgot as well.
> 
> Oleg.
> 
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -112,14 +112,9 @@ static bool valid_vma(struct vm_area_struct *vma, bool is_register)
>  	return false;
>  }
> 
> -static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
> +static unsigned long offset_to_vaddr(struct vm_area_struct *vma, loff_t offset)
>  {
> -	loff_t vaddr;
> -
> -	vaddr = vma->vm_start + offset;
> -	vaddr -= (loff_t)vma->vm_pgoff << PAGE_SHIFT;
> -
> -	return vaddr;
> +	return vma->vm_start + offset - ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
>  }
> 
>  static loff_t vaddr_to_offset(struct vm_area_struct *vma, unsigned long vaddr)
> @@ -775,7 +770,7 @@ build_map_info(struct address_space *mapping, loff_t offset, bool is_register)
>  		curr = info;
> 
>  		info->mm = vma->vm_mm;
> -		info->vaddr = vma_address(vma, offset);
> +		info->vaddr = offset_to_vaddr(vma, offset);
>  	}
>  	mutex_unlock(&mapping->i_mmap_mutex);
> 
> @@ -1042,7 +1037,7 @@ int uprobe_mmap(struct vm_area_struct *vma)
> 
>  	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
>  		if (!ret) {
> -			loff_t vaddr = vma_address(vma, uprobe->offset);
> +			unsigned long vaddr = offset_to_vaddr(vma, uprobe->offset);
> 
>  			ret = install_breakpoint(uprobe, vma->vm_mm, vma, vaddr);
>  			/*
> @@ -1103,7 +1098,7 @@ 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) {
> -		loff_t vaddr = vma_address(vma, uprobe->offset);
> +		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.
> 

[PATCH 4/4] uprobes: __replace_page() needs munlock_vma_page()

[Oleg Nesterov]

Like do_wp_page(), __replace_page() should do munlock_vma_page()
for the case when the old page still has other !VM_LOCKED mappings.
Unfortunately this needs mm/internal.h.

Also, move put_page() outside of ptl lock. This doesn't really
matter but looks a bit better.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index fc1df43..aeea41b 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -32,6 +32,7 @@
 #include <linux/swap.h>		/* try_to_free_swap */
 #include <linux/ptrace.h>	/* user_enable_single_step */
 #include <linux/kdebug.h>	/* notifier mechanism */
+#include "../../mm/internal.h"	/* munlock_vma_page */
 
 #include <linux/uprobes.h>
 
@@ -141,7 +142,7 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
 	pte_t *ptep;
 	int err;
 
-	/* freeze PageSwapCache() for try_to_free_swap() below */
+	/* For try_to_free_swap() and munlock_vma_page() below */
 	lock_page(page);
 
 	err = -EAGAIN;
@@ -164,9 +165,12 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
 	page_remove_rmap(page);
 	if (!page_mapped(page))
 		try_to_free_swap(page);
-	put_page(page);
 	pte_unmap_unlock(ptep, ptl);
 
+	if (vma->vm_flags & VM_LOCKED)
+		munlock_vma_page(page);
+	put_page(page);
+
 	err = 0;
  unlock:
 	unlock_page(page);
-- 
1.5.5.1

Re: [PATCH 4/4] uprobes: __replace_page() needs munlock_vma_page()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-12 19:10:25]:

> Like do_wp_page(), __replace_page() should do munlock_vma_page()
> for the case when the old page still has other !VM_LOCKED mappings.
> Unfortunately this needs mm/internal.h.
> 
> Also, move put_page() outside of ptl lock. This doesn't really
> matter but looks a bit better.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

One thing I wanted to check is, should we mlock the new page, i.e the
replacing page. It may not a good idea to mlock the new page
because then we can end up adding too many pages to the unevictable
list.

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

> ---
>  kernel/events/uprobes.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index fc1df43..aeea41b 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -32,6 +32,7 @@
>  #include <linux/swap.h>		/* try_to_free_swap */
>  #include <linux/ptrace.h>	/* user_enable_single_step */
>  #include <linux/kdebug.h>	/* notifier mechanism */
> +#include "../../mm/internal.h"	/* munlock_vma_page */
> 
>  #include <linux/uprobes.h>
> 
> @@ -141,7 +142,7 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
>  	pte_t *ptep;
>  	int err;
> 
> -	/* freeze PageSwapCache() for try_to_free_swap() below */
> +	/* For try_to_free_swap() and munlock_vma_page() below */
>  	lock_page(page);
> 
>  	err = -EAGAIN;
> @@ -164,9 +165,12 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
>  	page_remove_rmap(page);
>  	if (!page_mapped(page))
>  		try_to_free_swap(page);
> -	put_page(page);
>  	pte_unmap_unlock(ptep, ptl);
> 
> +	if (vma->vm_flags & VM_LOCKED)
> +		munlock_vma_page(page);
> +	put_page(page);
> +
>  	err = 0;
>   unlock:
>  	unlock_page(page);
> -- 
> 1.5.5.1
> 

Re: [PATCH 4/4] uprobes: __replace_page() needs munlock_vma_page()

[Oleg Nesterov]

On 07/26, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-07-12 19:10:25]:
>
> > Like do_wp_page(), __replace_page() should do munlock_vma_page()
> > for the case when the old page still has other !VM_LOCKED mappings.
> > Unfortunately this needs mm/internal.h.
> >
> > Also, move put_page() outside of ptl lock. This doesn't really
> > matter but looks a bit better.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>
> One thing I wanted to check is, should we mlock the new page, i.e the
> replacing page.

Yes, currently page_add_new_anon_rmap() makes it Mlocked/Unevictable.

> It may not a good idea to mlock the new page
> because then we can end up adding too many pages to the unevictable
> list.

Perhaps.

But. I think this is not really important. What is more important,
uprobe_register() should simply not add "too many pages", iow we
should re-use the same page if possible. This was another reason
for (buggy) http://marc.info/?l=linux-kernel&m=134013566617717
I sent before. I'll try to return to this later.

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

Thanks!

Oleg.