uprobes: misc fixlets

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 <[email protected]>

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

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

kernel/events/uprobes.c +4 -4

index 67697db..a93b6df 100644
@@ -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) {

Signature · 1 line

1.5.5.1

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 <[email protected]>

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

kernel/events/uprobes.c +0 -0

index a93b6df..47c4e24 100644

Quoted 22 lines · 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 <[email protected]>

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

kernel/events/uprobes.c +0 -0

index a93b6df..47c4e24 100644
@@ -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;
 

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

Quoted 6 lines

kernel/events/uprobes.c +3 -0

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

Quoted 24 lines · Peter Zijlstra

Quoted 22 lines · 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 <[email protected]>

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

kernel/events/uprobes.c +0 -0

index a93b6df..47c4e24 100644
@@ -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;
 

Quoted 10 lines

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

Quoted 6 lines

kernel/events/uprobes.c +3 -0

+	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() 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 <[email protected]>

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

kernel/events/uprobes.c +3 -0

index a93b6df..47c4e24 100644
@@ -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;
 

Signature · 1 line

1.5.5.1

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

kernel/events/uprobes.c +1 -1

@@ -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;
 }

Review static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) · line 1453

Quoted 6 lines · Oleg Nesterov

(quoted diff) +1 -1

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

Quoted 11 lines · Joe Perches

kernel/events/uprobes.c static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) · line 1453

Quoted 6 lines · Oleg Nesterov

kernel/events/uprobes.c +1 -1

@@ -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 <[email protected]>

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

kernel/events/uprobes.c +2 -2

index 47c4e24..6194edb 100644
@@ -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);
 		}
 

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.

Quoted 3 lines · Oleg Nesterov

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

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 <[email protected]>

---
 mm/mmap.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

mm/mmap.c +0 -3

index 3edfcdf..e5a4614 100644
@@ -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);
 			}

Signature · 1 line

1.5.5.1

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 <[email protected]>

---
 mm/mmap.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

mm/mmap.c +0 -0

index e5a4614..4fe2697 100644

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

Right, but somebody else might start using this later.

I cant think of a use case though.

Quoted 5 lines

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.

Quoted 10 lines

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

---
 mm/mmap.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

mm/mmap.c +0 -0

index e5a4614..4fe2697 100644

mm/mmap.c int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma) · line 2350

Quoted 9 lines

mm/mmap.c +0 -3

@@ -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;
	}

Quoted 9 lines · Srikar Dronamraju

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

Right, but somebody else might start using this later.

Unlikely, I think...

Quoted 1 line

I cant think of a use case though.

Yes.

Quoted 31 lines

Quoted 5 lines

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.

Quoted 10 lines

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

---
 mm/mmap.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

mm/mmap.c +0 -0

index e5a4614..4fe2697 100644

mm/mmap.c int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma) · line 2350

Quoted 9 lines

mm/mmap.c +0 -3

@@ -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:

Quoted 7 lines

	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.

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 <[email protected]>

---
 mm/mmap.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

mm/mmap.c +0 -3

index e5a4614..4fe2697 100644
@@ -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;
 }

Signature · 1 line

1.5.5.1