Re: [PATCH v7 3.2-rc2 3/30] uprobes: register/unregister probes.
From: Peter Zijlstra
Date: Thu Dec 01 2011 - 08:21:43 EST
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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/