uprobes: misc, resend

PATCH New Oleg Nesterov 1 participant 10 messages Jul 13, 2012

Subsystems (4)

Memory management perf events Memory mapping uprobes

lore

Share & tools

lore

Oleg Nesterov

author maintainer

2012-07-13 16:48 UTC #0 lore raw

cover uprobes: misc, resend

Hello.

Ingo, I am re-sending the patches which were already discussed and acked.

Just in case, I will be offline during the next week.

Oleg.

Oleg Nesterov

author maintainer

2012-07-13 16:49 UTC #1 lore raw

PATCH 1/9 uprobes: don't recheck vma/f_mapping in write_opcode()

write_opcode() rechecks valid_vma() and ->f_mapping, this is pointless. The caller, register_for_each_vma() or uprobe_mmap(), has already done these checks under mmap_sem.

To clarify, uprobe_mmap() checks valid_vma() only, but we can rely on build_probe_list(vm_file->f_mapping->host).

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

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

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

kernel/events/uprobes.c +1 ~~-18~~

index d4d9f02..8b3603a 100644
@@ -206,33 +206,16 @@ static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
 			unsigned long vaddr, uprobe_opcode_t opcode)
 {
 	struct page *old_page, *new_page;
-	struct address_space *mapping;
 	void *vaddr_old, *vaddr_new;
 	struct vm_area_struct *vma;
-	struct uprobe *uprobe;
 	int ret;
+
 retry:
 	/* Read the page with vaddr into memory */
 	ret = get_user_pages(NULL, mm, vaddr, 1, 0, 0, &old_page, &vma);
 	if (ret <= 0)
 		return ret;
 
-	ret = -EINVAL;
-
-	/*
-	 * We are interested in text pages only. Our pages of interest
-	 * should be mapped for read and execute only. We desist from
-	 * adding probes in write mapped pages since the breakpoints
-	 * might end up in the file copy.
-	 */
-	if (!valid_vma(vma, is_swbp_insn(&opcode)))
-		goto put_out;
-
-	uprobe = container_of(auprobe, struct uprobe, arch);
-	mapping = uprobe->inode->i_mapping;
-	if (mapping != vma->vm_file->f_mapping)
-		goto put_out;
-
 	ret = -ENOMEM;
 	new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, vaddr);
 	if (!new_page)

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2012-07-13 16:49 UTC #2 lore raw

PATCH 2/9 uprobes: __replace_page() should not use page_address_in_vma()

In reply to Oleg Nesterov

page_address_in_vma(old_page) in __replace_page() is ugly and wrong. The caller already knows the correct virtual address, this page was found by get_user_pages(vaddr).

However, page_address_in_vma() can actually fail if page->mapping was cleared by __delete_from_page_cache() after get_user_pages() returns. But this means the race with page reclaim, write_opcode() should not fail, it should retry and read this page again. Probably the race with remove_mapping() is not possible due to page_freeze_refs() logic, but afaics at least shmem_writepage()->shmem_delete_from_page_cache() can clear ->mapping.

We could change __replace_page() to return -EAGAIN in this case, but it would be better to simply use the caller's vaddr and rely on page_check_address().

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

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

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

kernel/events/uprobes.c +4 -7

index 8b3603a..fbb4188 100644
@@ -127,22 +127,19 @@ static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
  * based on replace_page in mm/ksm.c
  *
  * @vma:      vma that holds the pte pointing to page
+ * @addr:     address the old @page is mapped at
  * @page:     the cowed page we are replacing by kpage
  * @kpage:    the modified page we replace page by
  *
  * Returns 0 on success, -EFAULT on failure.
  */
-static int __replace_page(struct vm_area_struct *vma, struct page *page, struct page *kpage)
+static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
+				struct page *page, struct page *kpage)
 {
 	struct mm_struct *mm = vma->vm_mm;
-	unsigned long addr;
 	spinlock_t *ptl;
 	pte_t *ptep;
 
-	addr = page_address_in_vma(page, vma);
-	if (addr == -EFAULT)
-		return -EFAULT;
-
 	ptep = page_check_address(page, mm, addr, &ptl, 0);
 	if (!ptep)
 		return -EAGAIN;

@@ -243,7 +240,7 @@ retry:
 		goto unlock_out;
 
 	lock_page(new_page);
-	ret = __replace_page(vma, old_page, new_page);
+	ret = __replace_page(vma, vaddr, old_page, new_page);
 	unlock_page(new_page);
 
 unlock_out:

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2012-07-13 16:51 UTC #3 lore raw

PATCH 3/9 uprobes: kill write_opcode()->lock_page(new_page)

In reply to Oleg Nesterov

write_opcode() does lock_page(new_page) for no reason. Nobody can see this page until __replace_page() exposes it under ptl lock, and we do nothing with this page after pte_unmap_unlock().

If nothing else, the similar code in do_wp_page() doesn't lock the new page for page_add_new_anon_rmap/set_pte_at_notify.

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

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

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

kernel/events/uprobes.c +0 -2

index fbb4188..dff8da2 100644
@@ -239,9 +239,7 @@ retry:
 	if (ret)
 		goto unlock_out;
 
-	lock_page(new_page);
 	ret = __replace_page(vma, vaddr, old_page, new_page);
-	unlock_page(new_page);
 
 unlock_out:
 	unlock_page(old_page);

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2012-07-13 16:51 UTC #4 lore raw

PATCH 4/9 uprobes: cleanup and document write_opcode()->lock_page(old_page)

In reply to Oleg Nesterov

The comment above write_opcode()->lock_page(old_page) tells about the race with do_wp_page(). I don't really understand which exactly race it means, but afaics this lock_page() was not enough to close all races with do_wp_page().

Anyway, since 77fc4af1 this code is always called with ->mmap_sem hold for writing so we can forget about do_wp_page().

However, we can't simply remove this lock_page(), and the only (afaics) reason is __replace_page()->try_to_free_swap().

Nothing in write_opcode() needs it, move it into __replace_page() and fix the comment.

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

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

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

kernel/events/uprobes.c +14 ~~-13~~

index dff8da2..67697db 100644
@@ -139,10 +139,15 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
 	struct mm_struct *mm = vma->vm_mm;
 	spinlock_t *ptl;
 	pte_t *ptep;
+	int err;
 
+	/* freeze PageSwapCache() for try_to_free_swap() below */
+	lock_page(page);
+
+	err = -EAGAIN;
 	ptep = page_check_address(page, mm, addr, &ptl, 0);
 	if (!ptep)
-		return -EAGAIN;
+		goto unlock;
 
 	get_page(kpage);
 	page_add_new_anon_rmap(kpage, vma, addr);

@@ -162,7 +167,10 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
 	put_page(page);
 	pte_unmap_unlock(ptep, ptl);
 
-	return 0;
+	err = 0;
+ unlock:
+	unlock_page(page);
+	return err;
 }
 
 /**

@@ -216,15 +224,10 @@ retry:
 	ret = -ENOMEM;
 	new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, vaddr);
 	if (!new_page)
-		goto put_out;
+		goto put_old;
 
 	__SetPageUptodate(new_page);
 
-	/*
-	 * lock page will serialize against do_wp_page()'s
-	 * PageAnon() handling
-	 */
-	lock_page(old_page);
 	/* copy the page now that we've got it stable */
 	vaddr_old = kmap_atomic(old_page);
 	vaddr_new = kmap_atomic(new_page);

@@ -237,15 +240,13 @@ retry:
 
 	ret = anon_vma_prepare(vma);
 	if (ret)
-		goto unlock_out;
+		goto put_new;
 
 	ret = __replace_page(vma, vaddr, old_page, new_page);
 
-unlock_out:
-	unlock_page(old_page);
+put_new:
 	page_cache_release(new_page);
-
-put_out:
+put_old:
 	put_page(old_page);
 
 	if (unlikely(ret == -EAGAIN))

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2012-07-13 16:49 UTC #5 lore raw

PATCH 5/9 uprobes: uprobe_mmap/munmap needs list_for_each_entry_safe()

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

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

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

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

Oleg Nesterov

author maintainer

2012-07-13 16:49 UTC #6 lore raw

PATCH 6/9 uprobes: suppress uprobe_munmap() from mmput()

In reply to 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]>

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

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

Oleg Nesterov

author maintainer

2012-07-13 16:49 UTC #7 lore raw

PATCH 7/9 uprobes: fix overflow in vma_address/find_active_uprobe

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

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

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

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2012-07-13 16:49 UTC #8 lore raw

PATCH 8/9 uprobes: kill copy_vma()->uprobe_mmap()

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

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

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

Oleg Nesterov

author maintainer

2012-07-13 16:50 UTC #9 lore raw

PATCH 9/9 uprobes: kill insert_vm_struct()->uprobe_mmap()

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

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

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