[PATCH 0/5] uprobes: misc fixlets

[PATCH 0/5] uprobes: misc fixlets

[Oleg Nesterov]

Hello,

Misc small fixes.

The 1st one fixes the brown paper bug, I am proud of myself :/

Oleg.

[PATCH 1/5] uprobes: uprobe_mmap/munmap needs list_for_each_entry_safe()

[Oleg Nesterov]

The bug was introduced by me in 449d0d7c "uprobes: Simplify the
usage of uprobe->pending_list".

Yes, we do not care about uprobe->pending_list after return and
nobody can remove the current list entry, but put_uprobe(uprobe)
can actually free it and thus we need list_for_each_safe().

Reported-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 67697db..a93b6df 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1010,7 +1010,7 @@ static void build_probe_list(struct inode *inode, struct list_head *head)
 int uprobe_mmap(struct vm_area_struct *vma)
 {
 	struct list_head tmp_list;
-	struct uprobe *uprobe;
+	struct uprobe *uprobe, *u;
 	struct inode *inode;
 	int ret, count;
 
@@ -1028,7 +1028,7 @@ int uprobe_mmap(struct vm_area_struct *vma)
 	ret = 0;
 	count = 0;
 
-	list_for_each_entry(uprobe, &tmp_list, pending_list) {
+	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
 		if (!ret) {
 			loff_t vaddr = vma_address(vma, uprobe->offset);
 
@@ -1076,7 +1076,7 @@ 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;
+	struct uprobe *uprobe, *u;
 	struct inode *inode;
 
 	if (!atomic_read(&uprobe_events) || !valid_vma(vma, false))
@@ -1093,7 +1093,7 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
 	mutex_lock(uprobes_mmap_hash(inode));
 	build_probe_list(inode, &tmp_list);
 
-	list_for_each_entry(uprobe, &tmp_list, pending_list) {
+	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
 		loff_t vaddr = vma_address(vma, uprobe->offset);
 
 		if (vaddr >= start && vaddr < end) {
-- 
1.5.5.1

Re: [PATCH 1/5] uprobes: uprobe_mmap/munmap needs list_for_each_entry_safe()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-08 22:30:01]:

> The bug was introduced by me in 449d0d7c "uprobes: Simplify the
> usage of uprobe->pending_list".
> 
> Yes, we do not care about uprobe->pending_list after return and
> nobody can remove the current list entry, but put_uprobe(uprobe)
> can actually free it and thus we need list_for_each_safe().
> 
> Reported-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>


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

> ---
>  kernel/events/uprobes.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 67697db..a93b6df 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1010,7 +1010,7 @@ static void build_probe_list(struct inode *inode, struct list_head *head)
>  int uprobe_mmap(struct vm_area_struct *vma)
>  {
>  	struct list_head tmp_list;
> -	struct uprobe *uprobe;
> +	struct uprobe *uprobe, *u;
>  	struct inode *inode;
>  	int ret, count;
> 
> @@ -1028,7 +1028,7 @@ int uprobe_mmap(struct vm_area_struct *vma)
>  	ret = 0;
>  	count = 0;
> 
> -	list_for_each_entry(uprobe, &tmp_list, pending_list) {
> +	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
>  		if (!ret) {
>  			loff_t vaddr = vma_address(vma, uprobe->offset);
> 
> @@ -1076,7 +1076,7 @@ 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;
> +	struct uprobe *uprobe, *u;
>  	struct inode *inode;
> 
>  	if (!atomic_read(&uprobe_events) || !valid_vma(vma, false))
> @@ -1093,7 +1093,7 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
>  	mutex_lock(uprobes_mmap_hash(inode));
>  	build_probe_list(inode, &tmp_list);
> 
> -	list_for_each_entry(uprobe, &tmp_list, pending_list) {
> +	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
>  		loff_t vaddr = vma_address(vma, uprobe->offset);
> 
>  		if (vaddr >= start && vaddr < end) {
> -- 
> 1.5.5.1
> 

[PATCH 2/5] uprobes: suppress uprobe_munmap() from mmput()

[Oleg Nesterov]

uprobe_munmap() does get_user_pages() and it is also called from
the final mmput()->exit_mmap() path. This slows down exit/mmput()
for no reason, and I think  it is simply dangerous/wrong to try to
fault-in a page into the dying mm. If nothing else, this happens
after the last sync_mm_rss(), afaics handle_mm_fault() can change
the task->rss_stat and make the subsequent check_mm() unhappy.

Change uprobe_munmap() to check mm->mm_users != 0.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a93b6df..47c4e24 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1082,6 +1082,9 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
 	if (!atomic_read(&uprobe_events) || !valid_vma(vma, false))
 		return;
 
+	if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
+		return;
+
 	if (!atomic_read(&vma->vm_mm->uprobes_state.count))
 		return;
 
-- 
1.5.5.1

Re: [PATCH 2/5] uprobes: suppress uprobe_munmap() from mmput()

[Peter Zijlstra]

On Sun, 2012-07-08 at 22:30 +0200, Oleg Nesterov wrote:
> uprobe_munmap() does get_user_pages() and it is also called from
> the final mmput()->exit_mmap() path. This slows down exit/mmput()
> for no reason, and I think  it is simply dangerous/wrong to try to
> fault-in a page into the dying mm. If nothing else, this happens
> after the last sync_mm_rss(), afaics handle_mm_fault() can change
> the task->rss_stat and make the subsequent check_mm() unhappy.
> 
> Change uprobe_munmap() to check mm->mm_users != 0.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index a93b6df..47c4e24 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1082,6 +1082,9 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
>  	if (!atomic_read(&uprobe_events) || !valid_vma(vma, false))
>  		return;
>  
> +	if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
> +		return;
> +
>  	if (!atomic_read(&vma->vm_mm->uprobes_state.count))
>  		return;
>  

But won't you leak uprobe refcounts like this? Those aren't tied to the
task (which is dying) but to the vma's mapping the appropriate hunk of
the text. Not doing the munmap will then not put the uprobe->ref..

Or am I missing something here?

Re: [PATCH 2/5] uprobes: suppress uprobe_munmap() from mmput()

[Oleg Nesterov]

On 07/09, Peter Zijlstra wrote:
>
> On Sun, 2012-07-08 at 22:30 +0200, Oleg Nesterov wrote:
> > uprobe_munmap() does get_user_pages() and it is also called from
> > the final mmput()->exit_mmap() path. This slows down exit/mmput()
> > for no reason, and I think  it is simply dangerous/wrong to try to
> > fault-in a page into the dying mm. If nothing else, this happens
> > after the last sync_mm_rss(), afaics handle_mm_fault() can change
> > the task->rss_stat and make the subsequent check_mm() unhappy.
> >
> > Change uprobe_munmap() to check mm->mm_users != 0.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > ---
> >  kernel/events/uprobes.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index a93b6df..47c4e24 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -1082,6 +1082,9 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
> >  	if (!atomic_read(&uprobe_events) || !valid_vma(vma, false))
> >  		return;
> >
> > +	if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
> > +		return;
> > +
> >  	if (!atomic_read(&vma->vm_mm->uprobes_state.count))
> >  		return;
> >
>
> But won't you leak uprobe refcounts like this? Those aren't tied to the
> task (which is dying) but to the vma's mapping the appropriate hunk of
> the text. Not doing the munmap will then not put the uprobe->ref..

No, mmap/munmap do not participate in uprobe refcounting. This code
does put_uprobe() for each uprobe, yes, but only because the counter
was incremented in build_probe_list().

uprobe_munmap() is only needed to decrement mm->uprobes_state.count,
but nobody except uprobe_munmap() itself will use it after mmput().

Oleg.

Re: [PATCH 2/5] uprobes: suppress uprobe_munmap() from mmput()

[Peter Zijlstra]

On Mon, 2012-07-09 at 12:09 +0200, Oleg Nesterov wrote:
> No, mmap/munmap do not participate in uprobe refcounting. This code
> does put_uprobe() for each uprobe, yes, but only because the counter
> was incremented in build_probe_list().
> 
> uprobe_munmap() is only needed to decrement mm->uprobes_state.count,
> but nobody except uprobe_munmap() itself will use it after mmput(). 

OK, that also answers my other question.

Thanks

Re: [PATCH 2/5] uprobes: suppress uprobe_munmap() from mmput()

[Srikar Dronamraju]

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

> On 07/09, Peter Zijlstra wrote:
> >
> > On Sun, 2012-07-08 at 22:30 +0200, Oleg Nesterov wrote:
> > > uprobe_munmap() does get_user_pages() and it is also called from
> > > the final mmput()->exit_mmap() path. This slows down exit/mmput()
> > > for no reason, and I think  it is simply dangerous/wrong to try to
> > > fault-in a page into the dying mm. If nothing else, this happens
> > > after the last sync_mm_rss(), afaics handle_mm_fault() can change
> > > the task->rss_stat and make the subsequent check_mm() unhappy.
> > >
> > > Change uprobe_munmap() to check mm->mm_users != 0.
> > >
> > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > > ---
> > >  kernel/events/uprobes.c |    3 +++
> > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > > index a93b6df..47c4e24 100644
> > > --- a/kernel/events/uprobes.c
> > > +++ b/kernel/events/uprobes.c
> > > @@ -1082,6 +1082,9 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
> > >  	if (!atomic_read(&uprobe_events) || !valid_vma(vma, false))
> > >  		return;
> > >
> > > +	if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
> > > +		return;
> > > +
> > >  	if (!atomic_read(&vma->vm_mm->uprobes_state.count))
> > >  		return;
> > >
> >
> > But won't you leak uprobe refcounts like this? Those aren't tied to the
> > task (which is dying) but to the vma's mapping the appropriate hunk of
> > the text. Not doing the munmap will then not put the uprobe->ref..
> 
> No, mmap/munmap do not participate in uprobe refcounting. This code
> does put_uprobe() for each uprobe, yes, but only because the counter
> was incremented in build_probe_list().
> 

Right

-- 
Thanks and Regards
Srikar

Re: [PATCH 2/5] uprobes: suppress uprobe_munmap() from mmput()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-08 22:30:03]:

> uprobe_munmap() does get_user_pages() and it is also called from
> the final mmput()->exit_mmap() path. This slows down exit/mmput()
> for no reason, and I think  it is simply dangerous/wrong to try to
> fault-in a page into the dying mm. If nothing else, this happens
> after the last sync_mm_rss(), afaics handle_mm_fault() can change
> the task->rss_stat and make the subsequent check_mm() unhappy.
> 
> Change uprobe_munmap() to check mm->mm_users != 0.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  kernel/events/uprobes.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index a93b6df..47c4e24 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1082,6 +1082,9 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
>  	if (!atomic_read(&uprobe_events) || !valid_vma(vma, false))
>  		return;
> 
> +	if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
> +		return;
> +
>  	if (!atomic_read(&vma->vm_mm->uprobes_state.count))
>  		return;
> 
> -- 
> 1.5.5.1
> 

[PATCH 3/5] uprobes: fix overflow in vma_address/find_active_uprobe

[Oleg Nesterov]

vma->vm_pgoff is "unsigned long", it should be promoted to loff_t
before the multiplication to avoid the overflow.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 47c4e24..3826b3b 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -117,7 +117,7 @@ static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
 	loff_t vaddr;
 
 	vaddr = vma->vm_start + offset;
-	vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+	vaddr -= (loff_t)vma->vm_pgoff << PAGE_SHIFT;
 
 	return vaddr;
 }
@@ -1450,7 +1450,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 
 			inode = vma->vm_file->f_mapping->host;
 			offset = bp_vaddr - vma->vm_start;
-			offset += (vma->vm_pgoff << PAGE_SHIFT);
+			offset += ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
 			uprobe = find_uprobe(inode, offset);
 		}
 
-- 
1.5.5.1

Re: [PATCH 3/5] uprobes: fix overflow in vma_address/find_active_uprobe

[Joe Perches]

On Sun, 2012-07-08 at 22:30 +0200, Oleg Nesterov wrote:
> vma->vm_pgoff is "unsigned long", it should be promoted to loff_t
> before the multiplication to avoid the overflow.
[]
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
[]
> @@ -117,7 +117,7 @@ static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
>  	loff_t vaddr;
>  
>  	vaddr = vma->vm_start + offset;
> -	vaddr -= vma->vm_pgoff << PAGE_SHIFT;
> +	vaddr -= (loff_t)vma->vm_pgoff << PAGE_SHIFT;
>  
>  	return vaddr;
>  }
> @@ -1450,7 +1450,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
>  
>  			inode = vma->vm_file->f_mapping->host;
>  			offset = bp_vaddr - vma->vm_start;
> -			offset += (vma->vm_pgoff << PAGE_SHIFT);
> +			offset += ((loff_t)vma->vm_pgoff << PAGE_SHIFT);

It's be nice to take remove the unnecessary parenthesis
and make it consistent with the vaddr use above it too.

Re: [PATCH 3/5] uprobes: fix overflow in vma_address/find_active_uprobe

[Oleg Nesterov]

On 07/08, Joe Perches wrote:
>
> On Sun, 2012-07-08 at 22:30 +0200, Oleg Nesterov wrote:
> > @@ -1450,7 +1450,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> >
> >  			inode = vma->vm_file->f_mapping->host;
> >  			offset = bp_vaddr - vma->vm_start;
> > -			offset += (vma->vm_pgoff << PAGE_SHIFT);
> > +			offset += ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
>
> It's be nice to take remove the unnecessary parenthesis
> and make it consistent with the vaddr use above it too.

OK, please find v2 below.

------------------------------------------------------------------------------
Subject: [PATCH] uprobes: fix overflow in vma_address/find_active_uprobe

vma->vm_pgoff is "unsigned long", it should be promoted to loff_t
before the multiplication to avoid the overflow.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 47c4e24..6194edb 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -117,7 +117,7 @@ static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
 	loff_t vaddr;
 
 	vaddr = vma->vm_start + offset;
-	vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+	vaddr -= (loff_t)vma->vm_pgoff << PAGE_SHIFT;
 
 	return vaddr;
 }
@@ -1450,7 +1450,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 
 			inode = vma->vm_file->f_mapping->host;
 			offset = bp_vaddr - vma->vm_start;
-			offset += (vma->vm_pgoff << PAGE_SHIFT);
+			offset += (loff_t)vma->vm_pgoff << PAGE_SHIFT;
 			uprobe = find_uprobe(inode, offset);
 		}
 
-- 
1.5.5.1

Re: [PATCH 3/5] uprobes: fix overflow in vma_address/find_active_uprobe

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-09 12:54:45]:

> On 07/08, Joe Perches wrote:
> >
> > On Sun, 2012-07-08 at 22:30 +0200, Oleg Nesterov wrote:
> > > @@ -1450,7 +1450,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> > >
> > >  			inode = vma->vm_file->f_mapping->host;
> > >  			offset = bp_vaddr - vma->vm_start;
> > > -			offset += (vma->vm_pgoff << PAGE_SHIFT);
> > > +			offset += ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
> >
> > It's be nice to take remove the unnecessary parenthesis
> > and make it consistent with the vaddr use above it too.
> 
> OK, please find v2 below.
> 
> ------------------------------------------------------------------------------
> Subject: [PATCH] uprobes: fix overflow in vma_address/find_active_uprobe
> 
> vma->vm_pgoff is "unsigned long", it should be promoted to loff_t
> before the multiplication to avoid the overflow.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  kernel/events/uprobes.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 47c4e24..6194edb 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -117,7 +117,7 @@ static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
>  	loff_t vaddr;
> 
>  	vaddr = vma->vm_start + offset;
> -	vaddr -= vma->vm_pgoff << PAGE_SHIFT;
> +	vaddr -= (loff_t)vma->vm_pgoff << PAGE_SHIFT;
> 
>  	return vaddr;
>  }
> @@ -1450,7 +1450,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> 
>  			inode = vma->vm_file->f_mapping->host;
>  			offset = bp_vaddr - vma->vm_start;
> -			offset += (vma->vm_pgoff << PAGE_SHIFT);
> +			offset += (loff_t)vma->vm_pgoff << PAGE_SHIFT;
>  			uprobe = find_uprobe(inode, offset);
>  		}
> 

[PATCH 4/5] uprobes: kill copy_vma()->uprobe_mmap()

[Oleg Nesterov]

Kill copy_vma()->uprobe_mmap(new_vma), it is absolutely wrong.

This new_vma was just initialized to represent the new unmapped area,
[vm_start, vm_end) was returned by get_unmapped_area() in the caller.

This means that uprobe_mmap()->get_user_pages() will fail for sure,
simply because find_vma() can never succeed. And I verified that
sys_mremap()->mremap_to() indeed always fails with the wrong ENOMEM
code if [addr, addr+old_len] is probed.

And why this uprobe_mmap() was added? I believe the intent was wrong.
Note that the caller is going to do move_page_tables(), all registered
uprobes are already faulted in, we only change the virtual addresses.

NOTE: However, somehow we need to close the race with uprobe_register()
which relies on map_info->vaddr. This needs another fix I'll try to do
later. Probably we need uprobe_mmap() in move_vma() but we can not do
this right now, this can confuse uprobes_state.counter (which I still
hope we are going to kill).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 mm/mmap.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 3edfcdf..e5a4614 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2418,9 +2418,6 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
 			if (new_vma->vm_file) {
 				get_file(new_vma->vm_file);
 
-				if (uprobe_mmap(new_vma))
-					goto out_free_mempol;
-
 				if (vma->vm_flags & VM_EXECUTABLE)
 					added_exe_file_vma(mm);
 			}
-- 
1.5.5.1

Re: [PATCH 4/5] uprobes: kill copy_vma()->uprobe_mmap()

[Peter Zijlstra]

On Sun, 2012-07-08 at 22:30 +0200, Oleg Nesterov wrote:
> And why this uprobe_mmap() was added? I believe the intent was wrong.
> Note that the caller is going to do move_page_tables(), all registered
> uprobes are already faulted in, we only change the virtual addresses.
> 

I think it was because of the copy_vma + do_munmap. Since do_munmap()
should be doing a put on the uprobe, we need an extra get to balance.

That said, I cannot actually find the uprobe_munmap() from do_munmap(),
but that might be due to lack of wakefulness etc..

Re: [PATCH 4/5] uprobes: kill copy_vma()->uprobe_mmap()

[Oleg Nesterov]

On 07/09, Peter Zijlstra wrote:
>
> On Sun, 2012-07-08 at 22:30 +0200, Oleg Nesterov wrote:
> > And why this uprobe_mmap() was added? I believe the intent was wrong.
> > Note that the caller is going to do move_page_tables(), all registered
> > uprobes are already faulted in, we only change the virtual addresses.
>
> I think it was because of the copy_vma + do_munmap. Since do_munmap()
> should be doing a put on the uprobe, we need an extra get to balance.

No, please see the previous email, mmap doesn't increment uprobe->ref.

But this doesn't matter. Even if it did, the new vma will not add the
new uprobes, we are going to change the virtual address of the already
existing mapping.

As for mm->uprobes_state.count, move_vma()->do_munmap(old_addr, old_len)
won't change it afaics, is_swbp_at_addr() can't be true after
move_page_tables()->ptep_get_and_clear(old_addr), the page with "int3"
was already moved.

Anyway, this uprobe_mmap() always fails. And we need more fixes, I hope
to send more patches soon.

> That said, I cannot actually find the uprobe_munmap() from do_munmap(),
> but that might be due to lack of wakefulness etc..

do_munmap()->unmap_region()->unmap_vmas()->unmap_single_vma()

Yes, I can't keep in mind this path too ;)

Oleg.

Re: [PATCH 4/5] uprobes: kill copy_vma()->uprobe_mmap()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-08 22:30:08]:

> Kill copy_vma()->uprobe_mmap(new_vma), it is absolutely wrong.
> 
> This new_vma was just initialized to represent the new unmapped area,
> [vm_start, vm_end) was returned by get_unmapped_area() in the caller.
> 
> This means that uprobe_mmap()->get_user_pages() will fail for sure,
> simply because find_vma() can never succeed. And I verified that
> sys_mremap()->mremap_to() indeed always fails with the wrong ENOMEM
> code if [addr, addr+old_len] is probed.
> 
> And why this uprobe_mmap() was added? I believe the intent was wrong.
> Note that the caller is going to do move_page_tables(), all registered
> uprobes are already faulted in, we only change the virtual addresses.
> 
> NOTE: However, somehow we need to close the race with uprobe_register()
> which relies on map_info->vaddr. This needs another fix I'll try to do
> later. Probably we need uprobe_mmap() in move_vma() but we can not do
> this right now, this can confuse uprobes_state.counter (which I still
> hope we are going to kill).
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  mm/mmap.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 3edfcdf..e5a4614 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2418,9 +2418,6 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
>  			if (new_vma->vm_file) {
>  				get_file(new_vma->vm_file);
> 
> -				if (uprobe_mmap(new_vma))
> -					goto out_free_mempol;
> -
>  				if (vma->vm_flags & VM_EXECUTABLE)
>  					added_exe_file_vma(mm);
>  			}
> -- 
> 1.5.5.1
> 

[PATCH 5/5] uprobes: kill insert_vm_struct()->uprobe_mmap()

[Oleg Nesterov]

Kill insert_vm_struct()->uprobe_mmap(). It is not needed, nobody
except arch/ia64/kernel/perfmon.c uses insert_vm_struct(vma) with
vma->vm_file != NULL.

And it is wrong. Again, get_user_pages() can not succeed before
vma_link(vma) makes is visible to find_vma(). And even if this
worked, we must not insert the new bp before this mapping is
visible to vma_prio_tree_foreach() for uprobe_unregister().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 mm/mmap.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index e5a4614..4fe2697 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2345,9 +2345,6 @@ int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma)
 	     security_vm_enough_memory_mm(mm, vma_pages(vma)))
 		return -ENOMEM;
 
-	if (vma->vm_file && uprobe_mmap(vma))
-		return -EINVAL;
-
 	vma_link(mm, vma, prev, rb_link, rb_parent);
 	return 0;
 }
-- 
1.5.5.1

Re: [PATCH 5/5] uprobes: kill insert_vm_struct()->uprobe_mmap()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-08 22:30:11]:

> Kill insert_vm_struct()->uprobe_mmap(). It is not needed, nobody
> except arch/ia64/kernel/perfmon.c uses insert_vm_struct(vma) with
> vma->vm_file != NULL.
> 

Right, but somebody else might start using this later.
I cant think of a use case though.

> And it is wrong. Again, get_user_pages() can not succeed before
> vma_link(vma) makes is visible to find_vma(). And even if this
> worked, we must not insert the new bp before this mapping is
> visible to vma_prio_tree_foreach() for uprobe_unregister().
> 

Agree, we are wrong to do it before vma_link.


> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  mm/mmap.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index e5a4614..4fe2697 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2345,9 +2345,6 @@ int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma)
>  	     security_vm_enough_memory_mm(mm, vma_pages(vma)))
>  		return -ENOMEM;
> 
> -	if (vma->vm_file && uprobe_mmap(vma))
> -		return -EINVAL;
> -
>  	vma_link(mm, vma, prev, rb_link, rb_parent);
>  	return 0;
>  }

Can we do something like:

	vma_link(mm, vma, prev, rb_link, rb_parent);

	if (vma->vm_file && uprobe_mmap(vma)) {
		/* FIXME: dont know if calling unmap_region is fine here */
		unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end); 
		return -EINVAL;
	}

	return 0;

}

Re: [PATCH 5/5] uprobes: kill insert_vm_struct()->uprobe_mmap()

[Oleg Nesterov]

On 07/13, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-07-08 22:30:11]:
>
> > Kill insert_vm_struct()->uprobe_mmap(). It is not needed, nobody
> > except arch/ia64/kernel/perfmon.c uses insert_vm_struct(vma) with
> > vma->vm_file != NULL.
> >
>
> Right, but somebody else might start using this later.

Unlikely, I think...

> I cant think of a use case though.

Yes.

> > And it is wrong. Again, get_user_pages() can not succeed before
> > vma_link(vma) makes is visible to find_vma(). And even if this
> > worked, we must not insert the new bp before this mapping is
> > visible to vma_prio_tree_foreach() for uprobe_unregister().
> >
>
> Agree, we are wrong to do it before vma_link.
>
>
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > ---
> >  mm/mmap.c |    3 ---
> >  1 files changed, 0 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index e5a4614..4fe2697 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2345,9 +2345,6 @@ int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma)
> >  	     security_vm_enough_memory_mm(mm, vma_pages(vma)))
> >  		return -ENOMEM;
> >
> > -	if (vma->vm_file && uprobe_mmap(vma))
> > -		return -EINVAL;
> > -
> >  	vma_link(mm, vma, prev, rb_link, rb_parent);
> >  	return 0;
> >  }
>
> Can we do something like:
>
> 	vma_link(mm, vma, prev, rb_link, rb_parent);
>
> 	if (vma->vm_file && uprobe_mmap(vma)) {
> 		/* FIXME: dont know if calling unmap_region is fine here */
> 		unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
> 		return -EINVAL;
> 	}

Yes, I was thinking about the possible fix, but afaics this is not
enough. At least this needs vm_unacct_memory(). And I am not sure
about unmap_region...

The main problem is that I have no idea how could I test the fix.
Once again, currently this file can't be probed.

So. Can't we kill this obviously wrong and unneeded (at least currently)
code? Currently uprobe_mmap/munmap logic is not correct (I'll try to send
more fixes after I return from vacation), it would be nice to remove the
callsite.

If somebody else will use insert_vm_struct() to mmap the can-be-uprobed
file then yes, we will need to add uprobe_mmap() somewhere. But until
then, the right fix is not clear and not testable.

Oleg.

Re: [PATCH 5/5] uprobes: kill insert_vm_struct()->uprobe_mmap()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-13 15:29:16]:

> On 07/13, Srikar Dronamraju wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> [2012-07-08 22:30:11]:
> >
> > > Kill insert_vm_struct()->uprobe_mmap(). It is not needed, nobody
> > > except arch/ia64/kernel/perfmon.c uses insert_vm_struct(vma) with
> > > vma->vm_file != NULL.
> > >
> >
> > Right, but somebody else might start using this later.
> 
> Unlikely, I think...
> 
> > I cant think of a use case though.
> 
> Yes.
> 
> > > And it is wrong. Again, get_user_pages() can not succeed before
> > > vma_link(vma) makes is visible to find_vma(). And even if this
> > > worked, we must not insert the new bp before this mapping is
> > > visible to vma_prio_tree_foreach() for uprobe_unregister().
> > >
> >
> > Agree, we are wrong to do it before vma_link.
> >
> >
> > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > > ---
> > >  mm/mmap.c |    3 ---
> > >  1 files changed, 0 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index e5a4614..4fe2697 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -2345,9 +2345,6 @@ int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma)
> > >  	     security_vm_enough_memory_mm(mm, vma_pages(vma)))
> > >  		return -ENOMEM;
> > >
> > > -	if (vma->vm_file && uprobe_mmap(vma))
> > > -		return -EINVAL;
> > > -
> > >  	vma_link(mm, vma, prev, rb_link, rb_parent);
> > >  	return 0;
> > >  }
> >
> > Can we do something like:
> >
> > 	vma_link(mm, vma, prev, rb_link, rb_parent);
> >
> > 	if (vma->vm_file && uprobe_mmap(vma)) {
> > 		/* FIXME: dont know if calling unmap_region is fine here */
> > 		unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
> > 		return -EINVAL;
> > 	}
> 
> Yes, I was thinking about the possible fix, but afaics this is not
> enough. At least this needs vm_unacct_memory(). And I am not sure
> about unmap_region...
> 
> The main problem is that I have no idea how could I test the fix.
> Once again, currently this file can't be probed.
> 
> So. Can't we kill this obviously wrong and unneeded (at least currently)
> code? Currently uprobe_mmap/munmap logic is not correct (I'll try to send
> more fixes after I return from vacation), it would be nice to remove the
> callsite.
> 
> If somebody else will use insert_vm_struct() to mmap the can-be-uprobed
> file then yes, we will need to add uprobe_mmap() somewhere. But until
> then, the right fix is not clear and not testable.

Yes, for now your solution should be the way to go.
May be we should probably add a TODO/Fixme comment in insert_vm_struct
saying anybody trying to use insert_vm_struct to look at uprobe_mmap().

-- 
Thanks and Regards
Srikar

Re: [PATCH 5/5] uprobes: kill insert_vm_struct()->uprobe_mmap()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-08 22:30:11]:

> Kill insert_vm_struct()->uprobe_mmap(). It is not needed, nobody
> except arch/ia64/kernel/perfmon.c uses insert_vm_struct(vma) with
> vma->vm_file != NULL.
> 
> And it is wrong. Again, get_user_pages() can not succeed before
> vma_link(vma) makes is visible to find_vma(). And even if this
> worked, we must not insert the new bp before this mapping is
> visible to vma_prio_tree_foreach() for uprobe_unregister().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  mm/mmap.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index e5a4614..4fe2697 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2345,9 +2345,6 @@ int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma)
>  	     security_vm_enough_memory_mm(mm, vma_pages(vma)))
>  		return -ENOMEM;
> 
> -	if (vma->vm_file && uprobe_mmap(vma))
> -		return -EINVAL;
> -
>  	vma_link(mm, vma, prev, rb_link, rb_parent);
>  	return 0;
>  }
> -- 
> 1.5.5.1
>