Re: [PATCH v7 3.2-rc2 3/30] uprobes: register/unregister probes.

[Peter Zijlstra <peterz@infradead.org>]

On Fri, 2011-11-18 at 16:37 +0530, Srikar Dronamraju wrote:
> +static int __register_uprobe(struct inode *inode, loff_t offset,
> +                               struct uprobe *uprobe)
> +{
> +       struct list_head try_list;
> +       struct vm_area_struct *vma;
> +       struct address_space *mapping;
> +       struct vma_info *vi, *tmpvi;
> +       struct mm_struct *mm;
> +       loff_t vaddr;
> +       int ret = 0;
> +
> +       mapping = inode->i_mapping;
> +       INIT_LIST_HEAD(&try_list);
> +       while ((vi = find_next_vma_info(&try_list, offset,
> +                                               mapping, true)) != NULL) {
> +               if (IS_ERR(vi)) {
> +                       ret = -ENOMEM;
> +                       break;
> +               }
> +               mm = vi->mm;
> +               down_read(&mm->mmap_sem);
> +               vma = find_vma(mm, (unsigned long)vi->vaddr);
> +               if (!vma || !valid_vma(vma, true)) {
> +                       list_del(&vi->probe_list);
> +                       kfree(vi);
> +                       up_read(&mm->mmap_sem);
> +                       mmput(mm);
> +                       continue;
> +               }
> +               vaddr = vma->vm_start + offset;
> +               vaddr -= vma->vm_pgoff << PAGE_SHIFT;
> +               if (vma->vm_file->f_mapping->host != inode ||
> +                                               vaddr != vi->vaddr) {
> +                       list_del(&vi->probe_list);
> +                       kfree(vi);
> +                       up_read(&mm->mmap_sem);
> +                       mmput(mm);
> +                       continue;
> +               }
> +               ret = install_breakpoint(mm);
> +               up_read(&mm->mmap_sem);
> +               mmput(mm);
> +               if (ret && ret == -EEXIST)
> +                       ret = 0;
> +               if (!ret)
> +                       break;
> +       }
> +       list_for_each_entry_safe(vi, tmpvi, &try_list, probe_list) {
> +               list_del(&vi->probe_list);
> +               kfree(vi);
> +       }
> +       return ret;
> +}
> +
> +static void __unregister_uprobe(struct inode *inode, loff_t offset,
> +                                               struct uprobe *uprobe)
> +{
> +       struct list_head try_list;
> +       struct address_space *mapping;
> +       struct vma_info *vi, *tmpvi;
> +       struct vm_area_struct *vma;
> +       struct mm_struct *mm;
> +       loff_t vaddr;
> +
> +       mapping = inode->i_mapping;
> +       INIT_LIST_HEAD(&try_list);
> +       while ((vi = find_next_vma_info(&try_list, offset,
> +                                               mapping, false)) != NULL) {
> +               if (IS_ERR(vi))
> +                       break;
> +               mm = vi->mm;
> +               down_read(&mm->mmap_sem);
> +               vma = find_vma(mm, (unsigned long)vi->vaddr);
> +               if (!vma || !valid_vma(vma, false)) {
> +                       list_del(&vi->probe_list);
> +                       kfree(vi);
> +                       up_read(&mm->mmap_sem);
> +                       mmput(mm);
> +                       continue;
> +               }
> +               vaddr = vma->vm_start + offset;
> +               vaddr -= vma->vm_pgoff << PAGE_SHIFT;
> +               if (vma->vm_file->f_mapping->host != inode ||
> +                                               vaddr != vi->vaddr) {
> +                       list_del(&vi->probe_list);
> +                       kfree(vi);
> +                       up_read(&mm->mmap_sem);
> +                       mmput(mm);
> +                       continue;
> +               }
> +               remove_breakpoint(mm);
> +               up_read(&mm->mmap_sem);
> +               mmput(mm);
> +       }
> +
> +       list_for_each_entry_safe(vi, tmpvi, &try_list, probe_list) {
> +               list_del(&vi->probe_list);
> +               kfree(vi);
> +       }
> +       delete_uprobe(uprobe);
> +} 

I already mentioned on IRC that there's a lot of duplication here and
how to 'solve that'...

Something like the below, it lost the delete_uprobe() bit, and it adds a
few XXX marks where we have to deal with -ENOMEM. Also its not been near
a compiler.

---
 kernel/uprobes.c |   78 ++++++++++++++---------------------------------------
 1 files changed, 21 insertions(+), 57 deletions(-)

diff --git a/kernel/uprobes.c b/kernel/uprobes.c
index 2493191..c57284a 100644
--- a/kernel/uprobes.c
+++ b/kernel/uprobes.c
@@ -622,7 +622,7 @@ static int install_breakpoint(struct mm_struct *mm, struct uprobe *uprobe,
 }
 
 static void remove_breakpoint(struct mm_struct *mm, struct uprobe *uprobe,
-							loff_t vaddr)
+			      struct vm_area_struct *vma, loff_t vaddr)
 {
 	if (!set_orig_insn(mm, uprobe, (unsigned long)vaddr, true))
 		atomic_dec(&mm->mm_uprobes_count);
@@ -713,8 +713,10 @@ static struct vma_info *find_next_vma_info(struct list_head *head,
 	return retvi;
 }
 
-static int __register_uprobe(struct inode *inode, loff_t offset,
-				struct uprobe *uprobe)
+typedef int (*vma_func_t)(struct mm_struct *mm, struct uprobe *uprobe,
+			  struct vm_area_struct *vma, unsigned long addr);
+
+static int __for_each_vma(struct uprobe *uprobe, vma_func_t func)
 {
 	struct list_head try_list;
 	struct vm_area_struct *vma;
@@ -724,12 +726,12 @@ static int __register_uprobe(struct inode *inode, loff_t offset,
 	loff_t vaddr;
 	int ret = 0;
 
-	mapping = inode->i_mapping;
+	mapping = uprobe->inode->i_mapping;
 	INIT_LIST_HEAD(&try_list);
-	while ((vi = find_next_vma_info(&try_list, offset,
+	while ((vi = find_next_vma_info(&try_list, uprobe->offset,
 						mapping, true)) != NULL) {
 		if (IS_ERR(vi)) {
-			ret = -ENOMEM;
+			ret = PTR_ERR(vi);
 			break;
 		}
 		mm = vi->mm;
@@ -742,9 +744,9 @@ static int __register_uprobe(struct inode *inode, loff_t offset,
 			mmput(mm);
 			continue;
 		}
-		vaddr = vma->vm_start + offset;
+		vaddr = vma->vm_start + uprobe->offset;
 		vaddr -= vma->vm_pgoff << PAGE_SHIFT;
-		if (vma->vm_file->f_mapping->host != inode ||
+		if (vma->vm_file->f_mapping->host != uprobe->inode ||
 						vaddr != vi->vaddr) {
 			list_del(&vi->probe_list);
 			kfree(vi);
@@ -752,12 +754,12 @@ static int __register_uprobe(struct inode *inode, loff_t offset,
 			mmput(mm);
 			continue;
 		}
-		ret = install_breakpoint(mm, uprobe, vma, vi->vaddr);
+		ret = func(mm, uprobe, vma, vi->vaddr);
 		up_read(&mm->mmap_sem);
 		mmput(mm);
 		if (ret && ret == -EEXIST)
 			ret = 0;
-		if (!ret)
+		if (ret)
 			break;
 	}
 	list_for_each_entry_safe(vi, tmpvi, &try_list, probe_list) {
@@ -767,52 +769,14 @@ static int __register_uprobe(struct inode *inode, loff_t offset,
 	return ret;
 }
 
-static void __unregister_uprobe(struct inode *inode, loff_t offset,
-						struct uprobe *uprobe)
+static int __register_uprobe(struct uprobe *uprobe)
 {
-	struct list_head try_list;
-	struct address_space *mapping;
-	struct vma_info *vi, *tmpvi;
-	struct vm_area_struct *vma;
-	struct mm_struct *mm;
-	loff_t vaddr;
-
-	mapping = inode->i_mapping;
-	INIT_LIST_HEAD(&try_list);
-	while ((vi = find_next_vma_info(&try_list, offset,
-						mapping, false)) != NULL) {
-		if (IS_ERR(vi))
-			break;
-		mm = vi->mm;
-		down_read(&mm->mmap_sem);
-		vma = find_vma(mm, (unsigned long)vi->vaddr);
-		if (!vma || !valid_vma(vma, false)) {
-			list_del(&vi->probe_list);
-			kfree(vi);
-			up_read(&mm->mmap_sem);
-			mmput(mm);
-			continue;
-		}
-		vaddr = vma->vm_start + offset;
-		vaddr -= vma->vm_pgoff << PAGE_SHIFT;
-		if (vma->vm_file->f_mapping->host != inode ||
-						vaddr != vi->vaddr) {
-			list_del(&vi->probe_list);
-			kfree(vi);
-			up_read(&mm->mmap_sem);
-			mmput(mm);
-			continue;
-		}
-		remove_breakpoint(mm, uprobe, vi->vaddr);
-		up_read(&mm->mmap_sem);
-		mmput(mm);
-	}
+	return __for_each_vma(uprobe, install_breakpoint);
+}
 
-	list_for_each_entry_safe(vi, tmpvi, &try_list, probe_list) {
-		list_del(&vi->probe_list);
-		kfree(vi);
-	}
-	delete_uprobe(uprobe);
+static int __unregister_uprobe(struct uprobe *uprobe)
+{
+	return __for_each_vma(uprobe, remove_breakpoint);
 }
 
 /*
@@ -852,10 +816,10 @@ int register_uprobe(struct inode *inode, loff_t offset,
 	mutex_lock(uprobes_hash(inode));
 	uprobe = alloc_uprobe(inode, offset);
 	if (uprobe && !add_consumer(uprobe, consumer)) {
-		ret = __register_uprobe(inode, offset, uprobe);
+		ret = __register_uprobe(uprobe);
 		if (ret) {
 			uprobe->consumers = NULL;
-			__unregister_uprobe(inode, offset, uprobe);
+			__unregister_uprobe(uprobe); // -ENOMEM
 		} else
 			uprobe->flags |= UPROBES_RUN_HANDLER;
 	}
@@ -894,7 +858,7 @@ void unregister_uprobe(struct inode *inode, loff_t offset,
 	}
 
 	if (!uprobe->consumers) {
-		__unregister_uprobe(inode, offset, uprobe);
+		__unregister_uprobe(uprobe); // XXX -ENOMEM
 		uprobe->flags &= ~UPROBES_RUN_HANDLER;
 	}
 	mutex_unlock(uprobes_hash(inode));

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>