Uprobes patchset with perf probe support

kernel/uprobes.c line 173

Quoted 9 lines · Srikar Dronamraju

kernel/uprobes.c new file +9 -0

+static void delete_uprobe(struct uprobe *uprobe)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&uprobes_treelock, flags);
+	rb_erase(&uprobe->rb_node, &uprobes_tree);
+	spin_unlock_irqrestore(&uprobes_treelock, flags);
+	put_uprobe(uprobe);
+	iput(uprobe->inode);

Use-after-free when put_uprobe() kfrees() the uprobe?

Quoted 12 lines · Stefan Hajnoczi

kernel/uprobes.c line 173

Quoted 9 lines · Srikar Dronamraju

kernel/uprobes.c new file +9 -0

+static void delete_uprobe(struct uprobe *uprobe)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&uprobes_treelock, flags);
+	rb_erase(&uprobe->rb_node, &uprobes_tree);
+	spin_unlock_irqrestore(&uprobes_treelock, flags);
+	put_uprobe(uprobe);
+	iput(uprobe->inode);

Use-after-free when put_uprobe() kfrees() the uprobe?

I suspect the caller still has one, and this was the reference for being part of the tree. But yes, that could do with a comment.

The comment near atomic_set() in __insert_uprobe() isn't too clear either. /* get access + drop ref */, would naively seem +1 -1 = 0, instead of +1 +1 = 2.

kernel/uprobes.c new file +19 -0

+static struct uprobe *__insert_uprobe(struct uprobe *uprobe)
+{
+       struct rb_node **p = &uprobes_tree.rb_node;
+       struct rb_node *parent = NULL;
+       struct uprobe *u;
+       int match;
+
+       while (*p) {
+               parent = *p;
+               u = rb_entry(parent, struct uprobe, rb_node);
+               match = match_uprobe(uprobe, u);
+               if (!match) {
+                       atomic_inc(&u->ref);
+                       return u;
+               }
+
+               if (match < 0)
+                       p = &parent->rb_left;
+               else

Quoted 24 lines · Srikar Dronamraju

kernel/uprobes.c new file +24 -0

+static struct uprobe *__insert_uprobe(struct uprobe *uprobe)
+{
+       struct rb_node **p = &uprobes_tree.rb_node;
+       struct rb_node *parent = NULL;
+       struct uprobe *u;
+       int match;
+
+       while (*p) {
+               parent = *p;
+               u = rb_entry(parent, struct uprobe, rb_node);
+               match = match_uprobe(uprobe, u);
+               if (!match) {
+                       atomic_inc(&u->ref);
+                       return u;
+               }
+
+               if (match < 0)
+                       p = &parent->rb_left;
+               else
+                       p = &parent->rb_right;
+
+       }
+       u = NULL;
+       rb_link_node(&uprobe->rb_node, parent, p);

kernel/uprobes.c line 115

Quoted 5 lines

kernel/uprobes.c new file +5 -0

+       rb_insert_color(&uprobe->rb_node, &uprobes_tree);
+       /* get access + drop ref */
+       atomic_set(&uprobe->ref, 2);
+       return u;
+} 

If you ever want to make a 'lockless' lookup work you need to set the refcount of the new object before its fully visible, instead of after.

Now much of a problem now since its fully serialized by that uprobes_treelock thing.

+static struct uprobe *__find_uprobe(struct inode * inode, loff_t offset)

Quoted 1 line · Srikar Dronamraju

+static struct uprobe *__find_uprobe(struct inode * inode, loff_t offset)

Here and elsewhere, your whitespace is off, it should read:

struct inode *inode

I think checkpatch will inform you of this, but I didn't check.

kernel/uprobes.c +7 -0

+int register_uprobe(struct inode *inode, loff_t offset,
+				struct uprobe_consumer *consumer)
+{
+	struct uprobe *uprobe;
+	int ret = 0;
+
+	inode = igrab(inode);

kernel/uprobes.c +10 -0

+void unregister_uprobe(struct inode *inode, loff_t offset,
+				struct uprobe_consumer *consumer)
+{
+	struct uprobe *uprobe;
+
+	inode = igrab(inode);
+	if (!inode || !consumer)
+		return;
+
+	if (offset > inode->i_size)

Quoted 12 lines · Srikar Dronamraju

kernel/uprobes.c +12 -0

+int register_uprobe(struct inode *inode, loff_t offset,
+				struct uprobe_consumer *consumer)
+{
+	struct uprobe *uprobe;
+	int ret = 0;
+
+	inode = igrab(inode);
+	if (!inode || !consumer || consumer->next)
+		return -EINVAL;
+
+	if (offset > inode->i_size)
+		return -EINVAL;

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 430

Quoted 5 lines

kernel/uprobes.c +5 -0

+
+	mutex_lock(&inode->i_mutex);
+	uprobe = alloc_uprobe(inode, offset);
+	if (!uprobe)
+		return -ENOMEM;

The error returns above don't iput(inode). And inode->i_mutex stays locked on this return.

Quoted 15 lines

kernel/uprobes.c +14 -0

+void unregister_uprobe(struct inode *inode, loff_t offset,
+				struct uprobe_consumer *consumer)
+{
+	struct uprobe *uprobe;
+
+	inode = igrab(inode);
+	if (!inode || !consumer)
+		return;
+
+	if (offset > inode->i_size)
+		return;
+
+	uprobe = find_uprobe(inode, offset);
+	if (!uprobe)

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 469

Quoted 6 lines

kernel/uprobes.c +6 -0

+		return;
+
+	if (!del_consumer(uprobe, consumer)) {
+		put_uprobe(uprobe);
+		return;
+	}

More returns that do not iput(inode).

kernel/uprobes.c +20 -0

+static struct vma_info *__find_next_vma_info(struct list_head *head,
+			loff_t offset, struct address_space *mapping,
+			struct vma_info *vi)
+{
+	struct prio_tree_iter iter;
+	struct vm_area_struct *vma;
+	struct vma_info *tmpvi;
+	loff_t vaddr;
+	unsigned long pgoff = offset >> PAGE_SHIFT;
+	int existing_vma;
+
+	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
+		if (!vma || !valid_vma(vma))
+			return NULL;
+
+		existing_vma = 0;
+		vaddr = vma->vm_start + offset;
+		vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+		list_for_each_entry(tmpvi, head, probe_list) {
+			if (tmpvi->mm == vma->vm_mm && tmpvi->vaddr == vaddr) {

kernel/uprobes.c +8 -0

+		}
+	}
+	return NULL;
+}
+
+/*
+ * Iterate in the rmap prio tree  and find a vma where a probe has not
+ * yet been inserted.

kernel/uprobes.c +17 -0

+	mutex_lock(&mapping->i_mmap_mutex);
+	retvi = __find_next_vma_info(head, offset, mapping, vi);
+	mutex_unlock(&mapping->i_mmap_mutex);
+
+	if (!retvi)
+		kfree(vi);
+	return retvi;
+}
+
+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;

Quoted 20 lines · Srikar Dronamraju

kernel/uprobes.c +20 -0

+static struct vma_info *__find_next_vma_info(struct list_head *head,
+			loff_t offset, struct address_space *mapping,
+			struct vma_info *vi)
+{
+	struct prio_tree_iter iter;
+	struct vm_area_struct *vma;
+	struct vma_info *tmpvi;
+	loff_t vaddr;
+	unsigned long pgoff = offset >> PAGE_SHIFT;
+	int existing_vma;
+
+	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
+		if (!vma || !valid_vma(vma))
+			return NULL;
+
+		existing_vma = 0;
+		vaddr = vma->vm_start + offset;
+		vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+		list_for_each_entry(tmpvi, head, probe_list) {
+			if (tmpvi->mm == vma->vm_mm && tmpvi->vaddr == vaddr) {

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 287

Quoted 10 lines

kernel/uprobes.c +10 -0

+				existing_vma = 1;
+				break;
+			}
+		}
+		if (!existing_vma &&
+				atomic_inc_not_zero(&vma->vm_mm->mm_users)) {
+			vi->mm = vma->vm_mm;
+			vi->vaddr = vaddr;
+			list_add(&vi->probe_list, head);
+			return vi;

The the sole purpose of actually having that list is the above linear was to test if we've already had this one?

Does that really matter? After all, if the probe is already installed installing it again will return with -EEXIST, which should be easy enough to deal with.

Quoted 8 lines

kernel/uprobes.c +8 -0

+		}
+	}
+	return NULL;
+}
+
+/*
+ * Iterate in the rmap prio tree  and find a vma where a probe has not
+ * yet been inserted.

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 305

Quoted 10 lines

kernel/uprobes.c +10 -0

+ */
+static struct vma_info *find_next_vma_info(struct list_head *head,
+			loff_t offset, struct address_space *mapping)
+{
+	struct vma_info *vi, *retvi;
+	vi = kzalloc(sizeof(struct vma_info), GFP_KERNEL);
+	if (!vi)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&vi->probe_list);

weird place for the INIT_LIST_HEAD, I would have expected it near where the rest of vi is initialized, although it looks to be superfluous anyway, since list_add() can handle an uninitialized entry.

Quoted 17 lines

kernel/uprobes.c +17 -0

+	mutex_lock(&mapping->i_mmap_mutex);
+	retvi = __find_next_vma_info(head, offset, mapping, vi);
+	mutex_unlock(&mapping->i_mmap_mutex);
+
+	if (!retvi)
+		kfree(vi);
+	return retvi;
+}
+
+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;

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 332

Quoted 10 lines

kernel/uprobes.c +10 -0

+	int ret = 0;
+
+	mapping = inode->i_mapping;
+	INIT_LIST_HEAD(&try_list);
+	while ((vi = find_next_vma_info(&try_list, offset,
+							mapping)) != NULL) {
+		if (IS_ERR(vi)) {
+			ret = -ENOMEM;
+			break;
+		}

Here we hold neither i_mmap_mutex nor mmap_sem, so everything can change under our feet. See below..

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 336

Quoted 4 lines

kernel/uprobes.c +4 -0

+		mm = vi->mm;
+		down_read(&mm->mmap_sem);
+		vma = find_vma(mm, (unsigned long) vi->vaddr);
+		if (!vma || !valid_vma(vma)) {

No validation if its indeed the same vma you found earlier? At the very least we should validate the vma returned from find_vma() is indeed a mapping of the inode we're after and that the offset is still to be found at vaddr.

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 348

Quoted 12 lines

kernel/uprobes.c +12 -0

+			list_del(&vi->probe_list);
+			kfree(vi);
+			up_read(&mm->mmap_sem);
+			mmput(mm);
+			continue;
+		}
+		ret = install_breakpoint(mm);
+		if (ret && (ret != -ESRCH || ret != -EEXIST)) {
+			up_read(&mm->mmap_sem);
+			mmput(mm);
+			break;
+		}

Right, so you already deal with -EEXIST, so why do we need that list at all then?

Aah, its to make fwd progress, without it we would keep retrying the same vma over and over,.. hmm?

Quoted 10 lines

kernel/uprobes.c +10 -0

+		ret = 0;
+		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);
+	}
+	return ret;
+}

kernel/uprobes.c +4 -0

+static struct vma_info *__find_next_vma_info(struct list_head *head,
+			loff_t offset, struct address_space *mapping,
+			struct vma_info *vi)
+{

kernel/uprobes.c +11 -0

+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;
+	int ret = 0;
+
+	mapping = inode->i_mapping;

kernel/uprobes.c +8 -0

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

kernel/uprobes.c +14 -0

+void unregister_uprobe(struct inode *inode, loff_t offset,
+				struct uprobe_consumer *consumer)
+{
+	struct uprobe *uprobe;
+
+	inode = igrab(inode);
+	if (!inode || !consumer)
+		return;
+
+	if (offset > inode->i_size)
+		return;
+
+	uprobe = find_uprobe(inode, offset);
+	if (!uprobe)

Quoted 5 lines · Srikar Dronamraju

kernel/uprobes.c +4 -0

+static struct vma_info *__find_next_vma_info(struct list_head *head,
+			loff_t offset, struct address_space *mapping,
+			struct vma_info *vi)
+{

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 271

Quoted 10 lines

kernel/uprobes.c +10 -0

+	struct prio_tree_iter iter;
+	struct vm_area_struct *vma;
+	struct vma_info *tmpvi;
+	loff_t vaddr;
+	unsigned long pgoff = offset >> PAGE_SHIFT;
+	int existing_vma;
+
+	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
+		if (!vma || !valid_vma(vma))
+			return NULL;

!vma is not possible.

But I can't understand the !valid_vma(vma) check... We shouldn't return, we should ignore this vma and continue, no? Otherwise, I can't see how this can work if someone does, say, mmap(PROT_READ).

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 283

Quoted 12 lines

kernel/uprobes.c +11 -0

+		existing_vma = 0;
+		vaddr = vma->vm_start + offset;
+		vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+		list_for_each_entry(tmpvi, head, probe_list) {
+			if (tmpvi->mm == vma->vm_mm && tmpvi->vaddr == vaddr) {
+				existing_vma = 1;
+				break;
+			}
+		}
+		if (!existing_vma &&
+				atomic_inc_not_zero(&vma->vm_mm->mm_users)) {

This looks suspicious. If atomic_inc_not_zero() can fail, iow if we can see ->mm_users == 0, then why it is safe to touch this counter/memory? How we can know ->mm_count != 0 ?

I _think_ this is probably correct, ->mm_users == 0 means we are racing mmput(), ->i_mmap_mutex and the fact we found this vma guarantees that mmput() can't pass unlink_file_vma() and thus mmdrop() is not possible. May be needs a comment...

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 305

Quoted 9 lines

kernel/uprobes.c +9 -0

+static struct vma_info *find_next_vma_info(struct list_head *head,
+			loff_t offset, struct address_space *mapping)
+{
+	struct vma_info *vi, *retvi;
+	vi = kzalloc(sizeof(struct vma_info), GFP_KERNEL);
+	if (!vi)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&vi->probe_list);

Looks unneeded.

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 308

Quoted 3 lines

kernel/uprobes.c +3 -0

+	mutex_lock(&mapping->i_mmap_mutex);
+	retvi = __find_next_vma_info(head, offset, mapping, vi);
+	mutex_unlock(&mapping->i_mmap_mutex);

It is not clear why we can't race with mmap() after find_next_vma_info() returns NULL. I guess this is solved by the next patches.

Quoted 17 lines

kernel/uprobes.c +16 -0

+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;
+	int ret = 0;
+
+	mapping = inode->i_mapping;
+	INIT_LIST_HEAD(&try_list);
+	while ((vi = find_next_vma_info(&try_list, offset,
+							mapping)) != NULL) {
+		if (IS_ERR(vi)) {
+			ret = -ENOMEM;

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 335

Quoted 5 lines

kernel/uprobes.c +5 -0

+			break;
+		}
+		mm = vi->mm;
+		down_read(&mm->mmap_sem);
+		vma = find_vma(mm, (unsigned long) vi->vaddr);

But we can't trust find_vma? The original vma found by find_next_vma_info() could go away, at least we should verify vi->vaddr >= vm_start.

And worse, I do not understand how we can trust ->vaddr. Can't we race with sys_mremap() ?

Quoted 8 lines

kernel/uprobes.c +8 -0

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

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 377

Quoted 10 lines

kernel/uprobes.c +10 -0

+
+	mapping = inode->i_mapping;
+	INIT_LIST_HEAD(&try_list);
+	while ((vi = find_next_vma_info(&try_list, offset,
+							mapping)) != NULL) {
+		if (IS_ERR(vi))
+			break;
+		mm = vi->mm;
+		down_read(&mm->mmap_sem);
+		vma = find_vma(mm, (unsigned long) vi->vaddr);

Same problems...

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 342

Quoted 7 lines

kernel/uprobes.c +7 -0

+		if (!vma || !valid_vma(vma)) {
+			list_del(&vi->probe_list);
+			kfree(vi);
+			up_read(&mm->mmap_sem);
+			mmput(mm);
+			continue;
+		}

Not sure about !valid_vma() (and note that __find_next_vma_info does() this check too).

Suppose that register_uprobe() succeeds. After that unregister_ should work even if user-space does mprotect() which can make valid_vma() == F, right?

Quoted 6 lines

kernel/uprobes.c +6 -0

+int register_uprobe(struct inode *inode, loff_t offset,
+				struct uprobe_consumer *consumer)
+{
+	struct uprobe *uprobe;
+	int ret = 0;
+

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 425

Quoted 6 lines

kernel/uprobes.c +6 -0

+	inode = igrab(inode);
+	if (!inode || !consumer || consumer->next)
+		return -EINVAL;
+
+	if (offset > inode->i_size)
+		return -EINVAL;

I guess this needs i_size_read().

And every "return" in register/unregister leaks something.

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 428

Quoted 4 lines

kernel/uprobes.c +3 -0

+
+	mutex_lock(&inode->i_mutex);
+	uprobe = alloc_uprobe(inode, offset);

Looks like, alloc_uprobe() doesn't need ->i_mutex.

OTOH,

Quoted 20 lines

kernel/uprobes.c +20 -0

+void unregister_uprobe(struct inode *inode, loff_t offset,
+				struct uprobe_consumer *consumer)
+{
+	struct uprobe *uprobe;
+
+	inode = igrab(inode);
+	if (!inode || !consumer)
+		return;
+
+	if (offset > inode->i_size)
+		return;
+
+	uprobe = find_uprobe(inode, offset);
+	if (!uprobe)
+		return;
+
+	if (!del_consumer(uprobe, consumer)) {
+		put_uprobe(uprobe);
+		return;
+	}

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 473

Quoted 4 lines

kernel/uprobes.c +4 -0

+
+	mutex_lock(&inode->i_mutex);
+	if (!uprobe->consumers)
+		__unregister_uprobe(inode, offset, uprobe);

It seemes that del_consumer() should be done under ->i_mutex. If it removes the last consumer, we can race with register_uprobe() which takes ->i_mutex before us and does another __register_uprobe(), no?

Oleg.

Agree. Infact I encountered this problem last week and had fixed it. In mycase, I had mapped the file read and write while trying to insert probes.
The changed code looks like this

	if (!vma)
		return NULL;

	if (!valid_vma(vma))
		continue;

Quoted 4 lines

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 428

Quoted 2 lines

kernel/uprobes.c +2 -0

+	mutex_lock(&inode->i_mutex);
+	uprobe = alloc_uprobe(inode, offset);

Looks like, alloc_uprobe() doesn't need ->i_mutex.

Actually this was pointed out by you in the last review.
https://lkml.org/lkml/2011/7/24/91

Quoted 20 lines

Quoted 20 lines

kernel/uprobes.c +20 -0

+void unregister_uprobe(struct inode *inode, loff_t offset,
+				struct uprobe_consumer *consumer)
+{
+	struct uprobe *uprobe;
+
+	inode = igrab(inode);
+	if (!inode || !consumer)
+		return;
+
+	if (offset > inode->i_size)
+		return;
+
+	uprobe = find_uprobe(inode, offset);
+	if (!uprobe)
+		return;
+
+	if (!del_consumer(uprobe, consumer)) {
+		put_uprobe(uprobe);
+		return;
+	}

Quoted 8 lines

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 473

Quoted 4 lines

kernel/uprobes.c +4 -0

+
+	mutex_lock(&inode->i_mutex);
+	if (!uprobe->consumers)
+		__unregister_uprobe(inode, offset, uprobe);

It seemes that del_consumer() should be done under ->i_mutex. If it removes the last consumer, we can race with register_uprobe() which takes ->i_mutex before us and does another __register_uprobe(), no?

We should still be okay, because we check for the consumers before we do the actual unregister in form of __unregister_uprobe. since the consumer is again added by the time we get the lock, we dont do the actual unregistration and go as if del_consumer deleted one consumer but not the last.

Quoted 6 lines · Srikar Dronamraju

Agree. Infact I encountered this problem last week and had fixed it. In mycase, I had mapped the file read and write while trying to insert probes.
The changed code looks like this

Quoted 2 lines

	if (!vma)
		return NULL;

This is unneeded, vma_prio_tree_foreach() stops when vma_prio_tree_next() returns NULL. IOW, you can never see vma == NULL.

Quoted 3 lines

	if (!valid_vma(vma))
		continue;

Yes.

Quoted 6 lines

Quoted 4 lines

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 428

Quoted 2 lines

kernel/uprobes.c +2 -0

+	mutex_lock(&inode->i_mutex);
+	uprobe = alloc_uprobe(inode, offset);

Looks like, alloc_uprobe() doesn't need ->i_mutex.

Quoted 2 lines

Actually this was pointed out by you in the last review.
https://lkml.org/lkml/2011/7/24/91

OOPS ;) may be deserves a comment...

Quoted 30 lines

Quoted 21 lines

Quoted 21 lines

kernel/uprobes.c +20 -0

+void unregister_uprobe(struct inode *inode, loff_t offset,
+				struct uprobe_consumer *consumer)
+{
+	struct uprobe *uprobe;
+
+	inode = igrab(inode);
+	if (!inode || !consumer)
+		return;
+
+	if (offset > inode->i_size)
+		return;
+
+	uprobe = find_uprobe(inode, offset);
+	if (!uprobe)
+		return;
+
+	if (!del_consumer(uprobe, consumer)) {
+		put_uprobe(uprobe);
+		return;
+	}

Quoted 8 lines

kernel/uprobes.c static void delete_uprobe(struct uprobe *uprobe) · line 473

Quoted 4 lines

kernel/uprobes.c +4 -0

+
+	mutex_lock(&inode->i_mutex);
+	if (!uprobe->consumers)
+		__unregister_uprobe(inode, offset, uprobe);

It seemes that del_consumer() should be done under ->i_mutex. If it removes the last consumer, we can race with register_uprobe() which takes ->i_mutex before us and does another __register_uprobe(), no?

Quoted 5 lines

We should still be okay, because we check for the consumers before we do the actual unregister in form of __unregister_uprobe. since the consumer is again added by the time we get the lock, we dont do the actual unregistration and go as if del_consumer deleted one consumer but not the last.

Yes, but I meant in this case register_uprobe() does the unnecessary __register_uprobe() because it sees ->consumers == NULL (add_consumer() returns NULL).

I guess this is probably harmless because of is_bkpt_insn/-EEXIST logic, but still.

Btw. __register_uprobe() does

		ret = install_breakpoint(mm, uprobe, vma, vi->vaddr);
		if (ret && (ret != -ESRCH || ret != -EEXIST)) {
			up_read(&mm->mmap_sem);
			mmput(mm);
			break;
		}
		ret = 0;
		up_read(&mm->mmap_sem);
		mmput(mm);

Yes, this is cosmetic, but why do we duplicate up_read/mmput ?

Up to you, but

		ret = install_breakpoint(mm, uprobe, vma, vi->vaddr);
		up_read(&mm->mmap_sem);
		mmput(mm);

		if (ret) {
			if (ret != -ESRCH && ret != -EEXIST)
				break;
			ret = 0;
		}

Looks a bit simpler.

Oh, wait. I just noticed that the original code does

	(ret != -ESRCH || ret != -EEXIST)

this expression is always true ;)

Oleg.

* Oleg Nesterov <[email protected]> [2011-10-05 20:50:08]:

yes we might be doing an unnecessary __register_uprobe() but because it raced with unregister_uprobe() and got the lock, we would avoid doing a __unregister_uprobe().

However I am okay to move the lock before del_consumer().

Please let me
know how you prefer this.

kernel/uprobes.c void unregister_uprobe(struct inode *inode, loff_t offset, · line 512

Quoted 8 lines · Srikar Dronamraju

kernel/uprobes.c +8 -0

+static void build_probe_list(struct inode *inode, struct list_head *head)
+{
+	struct uprobe *uprobe;
+	struct rb_node *n;
+	unsigned long flags;
+
+	n = uprobes_tree.rb_node;
+	spin_lock_irqsave(&uprobes_treelock, flags);

Not sure whether grabbing root.rb_node outside the spinlock is safe? If the tree is rotated on another CPU you could catch and out-of-date node?

kernel/uprobes.c void unregister_uprobe(struct inode *inode, loff_t offset, · line 588

Quoted 9 lines

kernel/uprobes.c +9 -0

+static void dec_mm_uprobes_count(struct vm_area_struct *vma,
+		struct inode *inode)
+{
+	struct uprobe *uprobe;
+	struct rb_node *n;
+	unsigned long flags;
+
+	n = uprobes_tree.rb_node;
+	spin_lock_irqsave(&uprobes_treelock, flags);

Same here.

kernel/uprobes.c +13 -2

-static int match_uprobe(struct uprobe *l, struct uprobe *r)
+static int match_uprobe(struct uprobe *l, struct uprobe *r, int *match_inode)
 {
+	/*
+	 * if match_inode is non NULL then indicate if the
+	 * inode atleast match.
+	 */
+	if (match_inode)
+		*match_inode = 0;
+
 	if (l->inode < r->inode)
 		return -1;
 	if (l->inode > r->inode)
 		return 1;
 	else {
+		if (match_inode)
+			*match_inode = 1;
+
 		if (l->offset < r->offset)
 			return -1;

@@ -75,16 +86,20 @@ static int match_uprobe(struct uprobe *l, struct uprobe *r)
 	return 0;
 }
 
-static struct uprobe *__find_uprobe(struct inode * inode, loff_t offset)
+static struct uprobe *__find_uprobe(struct inode * inode, loff_t offset,
+					struct rb_node **close_match)
 {
 	struct uprobe u = { .inode = inode, .offset = offset };
 	struct rb_node *n = uprobes_tree.rb_node;

kernel/uprobes.c +10 -0

+/*
+ * For a given inode, build a list of probes that need to be inserted.
+ */
+static void build_probe_list(struct inode *inode, struct list_head *head)
+{
+	struct uprobe *uprobe;
+	struct rb_node *n;
+	unsigned long flags;
+
+	n = uprobes_tree.rb_node;

kernel/uprobes.c +94 -0

+
+/*
+ * Called from mmap_region.
+ * called with mm->mmap_sem acquired.
+ *
+ * Return -ve no if we fail to insert probes and we cannot
+ * bail-out.
+ * Return 0 otherwise. i.e :
+ *	- successful insertion of probes
+ *	- (or) no possible probes to be inserted.
+ *	- (or) insertion of probes failed but we can bail-out.
+ */
+int mmap_uprobe(struct vm_area_struct *vma)
+{
+	struct list_head tmp_list;
+	struct uprobe *uprobe, *u;
+	struct inode *inode;
+	int ret = 0;
+
+	if (!valid_vma(vma))
+		return ret;	/* Bail-out */
+
+	inode = igrab(vma->vm_file->f_mapping->host);
+	if (!inode)
+		return ret;
+
+	INIT_LIST_HEAD(&tmp_list);
+	mutex_lock(&uprobes_mmap_mutex);
+	build_probe_list(inode, &tmp_list);
+	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
+		loff_t vaddr;
+
+		list_del(&uprobe->pending_list);
+		if (!ret && uprobe->consumers) {
+			vaddr = vma->vm_start + uprobe->offset;
+			vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+			if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
+				continue;
+			ret = install_breakpoint(vma->vm_mm, uprobe);
+
+			if (ret && (ret == -ESRCH || ret == -EEXIST))
+				ret = 0;
+		}
+		put_uprobe(uprobe);
+	}
+
+	mutex_unlock(&uprobes_mmap_mutex);
+	iput(inode);
+	return ret;
+}
+
+static void dec_mm_uprobes_count(struct vm_area_struct *vma,
+		struct inode *inode)
+{
+	struct uprobe *uprobe;
+	struct rb_node *n;
+	unsigned long flags;
+
+	n = uprobes_tree.rb_node;
+	spin_lock_irqsave(&uprobes_treelock, flags);
+	uprobe = __find_uprobe(inode, 0, &n);
+
+	/*
+	 * If indeed there is a probe for the inode and with offset zero,
+	 * then lets release its reference. (ref got thro __find_uprobe)
+	 */
+	if (uprobe)
+		put_uprobe(uprobe);
+	for (; n; n = rb_next(n)) {
+		loff_t vaddr;
+
+		uprobe = rb_entry(n, struct uprobe, rb_node);
+		if (uprobe->inode != inode)
+			break;
+		vaddr = vma->vm_start + uprobe->offset;
+		vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+		if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
+			continue;
+		atomic_dec(&vma->vm_mm->mm_uprobes_count);
+	}
+	spin_unlock_irqrestore(&uprobes_treelock, flags);
+}
+
+/*
+ * Called in context of a munmap of a vma.
+ */
+void munmap_uprobe(struct vm_area_struct *vma)
+{
+	struct inode *inode;
+
+	if (!valid_vma(vma))
+		return;		/* Bail-out */
+
+	if (!atomic_read(&vma->vm_mm->mm_uprobes_count))

Quoted 7 lines

kernel/uprobes.c +2 -1

-static struct uprobe *__find_uprobe(struct inode * inode, loff_t offset)
+static struct uprobe *__find_uprobe(struct inode * inode, loff_t offset,
+					struct rb_node **close_match)
 {
 	struct uprobe u = { .inode = inode, .offset = offset };
 	struct rb_node *n = uprobes_tree.rb_node;

kernel/uprobes.c static int match_uprobe(struct uprobe *l, struct uprobe *r) · line 101

Quoted 10 lines

kernel/uprobes.c +4 -2

 	struct uprobe *uprobe;
-	int match;
+	int match, match_inode;
 
 	while (n) {
 		uprobe = rb_entry(n, struct uprobe, rb_node);
-		match = match_uprobe(&u, uprobe);
+		match = match_uprobe(&u, uprobe, &match_inode);
+		if (close_match && match_inode)
+			*close_match = n;

Because:

		if (close_match && uprobe->inode == inode)

Isn't good enough? Also, returning an rb_node just seems iffy..

kernel/uprobes.c static int match_uprobe(struct uprobe *l, struct uprobe *r) · line 105

Quoted 4 lines

kernel/uprobes.c +0 -0

 		if (!match) {
 			atomic_inc(&uprobe->ref);
 			return uprobe;

Why not something like:

+static struct uprobe *__find_uprobe(struct inode * inode, loff_t offset,
bool inode_only)

kernel/uprobes.c +3 -0

+{
        struct uprobe u = { .inode = inode, .offset = inode_only ? 0 : offset };
+       struct rb_node *n = uprobes_tree.rb_node;
+       struct uprobe *uprobe;

	struct uprobe *ret = NULL;

(quoted diff) +6 -0

+       int match;
+
+       while (n) {
+               uprobe = rb_entry(n, struct uprobe, rb_node);
+               match = match_uprobe(&u, uprobe);
+               if (!match) {

			if (!inode_only)
	                       atomic_inc(&uprobe->ref);

(quoted diff) +2 -0

+                       return uprobe;
+               }

		if (inode_only && uprobe->inode == inode)
			ret = uprobe;

Quoted 11 lines

kernel/uprobes.c +10 -0

+/*
+ * For a given inode, build a list of probes that need to be inserted.
+ */
+static void build_probe_list(struct inode *inode, struct list_head *head)
+{
+	struct uprobe *uprobe;
+	struct rb_node *n;
+	unsigned long flags;
+
+	n = uprobes_tree.rb_node;

kernel/uprobes.c void unregister_uprobe(struct inode *inode, loff_t offset, · line 519

Quoted 10 lines

kernel/uprobes.c +8 -0

+	spin_lock_irqsave(&uprobes_treelock, flags);
+	uprobe = __find_uprobe(inode, 0, &n);
 
 
+	/*
+	 * If indeed there is a probe for the inode and with offset zero,
+	 * then lets release its reference. (ref got thro __find_uprobe)
+	 */
+	if (uprobe)
+		put_uprobe(uprobe);

The above would make this ^ unneeded.

	n = &uprobe->rb_node;

kernel/uprobes.c void unregister_uprobe(struct inode *inode, loff_t offset, · line 528

Quoted 9 lines

kernel/uprobes.c +9 -0

+	for (; n; n = rb_next(n)) {
+		uprobe = rb_entry(n, struct uprobe, rb_node);
+		if (uprobe->inode != inode)
+			break;
+		list_add(&uprobe->pending_list, head);
+		atomic_inc(&uprobe->ref);
+	}
+	spin_unlock_irqrestore(&uprobes_treelock, flags);
+}

If this ever gets to be a latency issue (linear lookup under spinlock) you can use a double lock (mutex+spinlock) and require that modification acquires both but lookups can get away with either.

That way you can do the linear search using a mutex instead of the spinlock.

Quoted 99 lines

kernel/uprobes.c +99 -0

+
+/*
+ * Called from mmap_region.
+ * called with mm->mmap_sem acquired.
+ *
+ * Return -ve no if we fail to insert probes and we cannot
+ * bail-out.
+ * Return 0 otherwise. i.e :
+ *	- successful insertion of probes
+ *	- (or) no possible probes to be inserted.
+ *	- (or) insertion of probes failed but we can bail-out.
+ */
+int mmap_uprobe(struct vm_area_struct *vma)
+{
+	struct list_head tmp_list;
+	struct uprobe *uprobe, *u;
+	struct inode *inode;
+	int ret = 0;
+
+	if (!valid_vma(vma))
+		return ret;	/* Bail-out */
+
+	inode = igrab(vma->vm_file->f_mapping->host);
+	if (!inode)
+		return ret;
+
+	INIT_LIST_HEAD(&tmp_list);
+	mutex_lock(&uprobes_mmap_mutex);
+	build_probe_list(inode, &tmp_list);
+	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
+		loff_t vaddr;
+
+		list_del(&uprobe->pending_list);
+		if (!ret && uprobe->consumers) {
+			vaddr = vma->vm_start + uprobe->offset;
+			vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+			if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
+				continue;
+			ret = install_breakpoint(vma->vm_mm, uprobe);
+
+			if (ret && (ret == -ESRCH || ret == -EEXIST))
+				ret = 0;
+		}
+		put_uprobe(uprobe);
+	}
+
+	mutex_unlock(&uprobes_mmap_mutex);
+	iput(inode);
+	return ret;
+}
+
+static void dec_mm_uprobes_count(struct vm_area_struct *vma,
+		struct inode *inode)
+{
+	struct uprobe *uprobe;
+	struct rb_node *n;
+	unsigned long flags;
+
+	n = uprobes_tree.rb_node;
+	spin_lock_irqsave(&uprobes_treelock, flags);
+	uprobe = __find_uprobe(inode, 0, &n);
+
+	/*
+	 * If indeed there is a probe for the inode and with offset zero,
+	 * then lets release its reference. (ref got thro __find_uprobe)
+	 */
+	if (uprobe)
+		put_uprobe(uprobe);
+	for (; n; n = rb_next(n)) {
+		loff_t vaddr;
+
+		uprobe = rb_entry(n, struct uprobe, rb_node);
+		if (uprobe->inode != inode)
+			break;
+		vaddr = vma->vm_start + uprobe->offset;
+		vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+		if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
+			continue;
+		atomic_dec(&vma->vm_mm->mm_uprobes_count);
+	}
+	spin_unlock_irqrestore(&uprobes_treelock, flags);
+}
+
+/*
+ * Called in context of a munmap of a vma.
+ */
+void munmap_uprobe(struct vm_area_struct *vma)
+{
+	struct inode *inode;
+
+	if (!valid_vma(vma))
+		return;		/* Bail-out */
+
+	if (!atomic_read(&vma->vm_mm->mm_uprobes_count))
+		return;
+
+	inode = igrab(vma->vm_file->f_mapping->host);
+	if (!inode)
+		return;

kernel/uprobes.c void unregister_uprobe(struct inode *inode, loff_t offset, · line 632

Quoted 5 lines

kernel/uprobes.c +5 -0

+
+	dec_mm_uprobes_count(vma, inode);
+	iput(inode);
+	return;
+}

One has to wonder why mmap_uprobe() can be one function but munmap_uprobe() cannot.

Quoted 100 lines

Quoted 100 lines

kernel/uprobes.c +99 -0

+
+/*
+ * Called from mmap_region.
+ * called with mm->mmap_sem acquired.
+ *
+ * Return -ve no if we fail to insert probes and we cannot
+ * bail-out.
+ * Return 0 otherwise. i.e :
+ * - successful insertion of probes
+ * - (or) no possible probes to be inserted.
+ * - (or) insertion of probes failed but we can bail-out.
+ */
+int mmap_uprobe(struct vm_area_struct *vma)
+{
+   struct list_head tmp_list;
+   struct uprobe *uprobe, *u;
+   struct inode *inode;
+   int ret = 0;
+
+   if (!valid_vma(vma))
+           return ret;     /* Bail-out */
+
+   inode = igrab(vma->vm_file->f_mapping->host);
+   if (!inode)
+           return ret;
+
+   INIT_LIST_HEAD(&tmp_list);
+   mutex_lock(&uprobes_mmap_mutex);
+   build_probe_list(inode, &tmp_list);
+   list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
+           loff_t vaddr;
+
+           list_del(&uprobe->pending_list);
+           if (!ret && uprobe->consumers) {
+                   vaddr = vma->vm_start + uprobe->offset;
+                   vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+                   if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
+                           continue;
+                   ret = install_breakpoint(vma->vm_mm, uprobe);
+
+                   if (ret && (ret == -ESRCH || ret == -EEXIST))
+                           ret = 0;
+           }
+           put_uprobe(uprobe);
+   }
+
+   mutex_unlock(&uprobes_mmap_mutex);
+   iput(inode);
+   return ret;
+}
+
+static void dec_mm_uprobes_count(struct vm_area_struct *vma,
+           struct inode *inode)
+{
+   struct uprobe *uprobe;
+   struct rb_node *n;
+   unsigned long flags;
+
+   n = uprobes_tree.rb_node;
+   spin_lock_irqsave(&uprobes_treelock, flags);
+   uprobe = __find_uprobe(inode, 0, &n);
+
+   /*
+    * If indeed there is a probe for the inode and with offset zero,
+    * then lets release its reference. (ref got thro __find_uprobe)
+    */
+   if (uprobe)
+           put_uprobe(uprobe);
+   for (; n; n = rb_next(n)) {
+           loff_t vaddr;
+
+           uprobe = rb_entry(n, struct uprobe, rb_node);
+           if (uprobe->inode != inode)
+                   break;
+           vaddr = vma->vm_start + uprobe->offset;
+           vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+           if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
+                   continue;
+           atomic_dec(&vma->vm_mm->mm_uprobes_count);
+   }
+   spin_unlock_irqrestore(&uprobes_treelock, flags);
+}
+
+/*
+ * Called in context of a munmap of a vma.
+ */
+void munmap_uprobe(struct vm_area_struct *vma)
+{
+   struct inode *inode;
+
+   if (!valid_vma(vma))
+           return;         /* Bail-out */
+
+   if (!atomic_read(&vma->vm_mm->mm_uprobes_count))
+           return;
+
+   inode = igrab(vma->vm_file->f_mapping->host);
+   if (!inode)
+           return;

Quoted 9 lines

kernel/uprobes.c void unregister_uprobe(struct inode *inode, loff_t offset, · line 632

Quoted 5 lines

kernel/uprobes.c +5 -0

+
+   dec_mm_uprobes_count(vma, inode);
+   iput(inode);
+   return;
+}

One has to wonder why mmap_uprobe() can be one function but munmap_uprobe() cannot.

I didnt understand this comment, Can you please elaborate? mmap_uprobe uses build_probe_list and munmap_uprobe uses dec_mm_uprobes_count.

Quoted 110 lines · Srikar Dronamraju

Quoted 100 lines

Quoted 100 lines

kernel/uprobes.c +99 -0

+
+/*
+ * Called from mmap_region.
+ * called with mm->mmap_sem acquired.
+ *
+ * Return -ve no if we fail to insert probes and we cannot
+ * bail-out.
+ * Return 0 otherwise. i.e :
+ * - successful insertion of probes
+ * - (or) no possible probes to be inserted.
+ * - (or) insertion of probes failed but we can bail-out.
+ */
+int mmap_uprobe(struct vm_area_struct *vma)
+{
+   struct list_head tmp_list;
+   struct uprobe *uprobe, *u;
+   struct inode *inode;
+   int ret = 0;
+
+   if (!valid_vma(vma))
+           return ret;     /* Bail-out */
+
+   inode = igrab(vma->vm_file->f_mapping->host);
+   if (!inode)
+           return ret;
+
+   INIT_LIST_HEAD(&tmp_list);
+   mutex_lock(&uprobes_mmap_mutex);
+   build_probe_list(inode, &tmp_list);
+   list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
+           loff_t vaddr;
+
+           list_del(&uprobe->pending_list);
+           if (!ret && uprobe->consumers) {
+                   vaddr = vma->vm_start + uprobe->offset;
+                   vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+                   if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
+                           continue;
+                   ret = install_breakpoint(vma->vm_mm, uprobe);
+
+                   if (ret && (ret == -ESRCH || ret == -EEXIST))
+                           ret = 0;
+           }
+           put_uprobe(uprobe);
+   }
+
+   mutex_unlock(&uprobes_mmap_mutex);
+   iput(inode);
+   return ret;
+}
+
+static void dec_mm_uprobes_count(struct vm_area_struct *vma,
+           struct inode *inode)
+{
+   struct uprobe *uprobe;
+   struct rb_node *n;
+   unsigned long flags;
+
+   n = uprobes_tree.rb_node;
+   spin_lock_irqsave(&uprobes_treelock, flags);
+   uprobe = __find_uprobe(inode, 0, &n);
+
+   /*
+    * If indeed there is a probe for the inode and with offset zero,
+    * then lets release its reference. (ref got thro __find_uprobe)
+    */
+   if (uprobe)
+           put_uprobe(uprobe);
+   for (; n; n = rb_next(n)) {
+           loff_t vaddr;
+
+           uprobe = rb_entry(n, struct uprobe, rb_node);
+           if (uprobe->inode != inode)
+                   break;
+           vaddr = vma->vm_start + uprobe->offset;
+           vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+           if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
+                   continue;
+           atomic_dec(&vma->vm_mm->mm_uprobes_count);
+   }
+   spin_unlock_irqrestore(&uprobes_treelock, flags);
+}
+
+/*
+ * Called in context of a munmap of a vma.
+ */
+void munmap_uprobe(struct vm_area_struct *vma)
+{
+   struct inode *inode;
+
+   if (!valid_vma(vma))
+           return;         /* Bail-out */
+
+   if (!atomic_read(&vma->vm_mm->mm_uprobes_count))
+           return;
+
+   inode = igrab(vma->vm_file->f_mapping->host);
+   if (!inode)
+           return;

Quoted 9 lines

kernel/uprobes.c void unregister_uprobe(struct inode *inode, loff_t offset, · line 632

Quoted 5 lines

kernel/uprobes.c +5 -0

+
+   dec_mm_uprobes_count(vma, inode);
+   iput(inode);
+   return;
+}

One has to wonder why mmap_uprobe() can be one function but munmap_uprobe() cannot.

Quoted 3 lines

I didnt understand this comment, Can you please elaborate? mmap_uprobe uses build_probe_list and munmap_uprobe uses dec_mm_uprobes_count.

Ah, I missed build_probe_list(), but I didn't see a reason for the existence of dec_mm_uprobe_count(), the name doesn't make sense and the content is 'small' enough to just put in munmap_uprobe.

To me it looks similar to the list iteration you have in mmap_uprobe(), you didn't split that out into another function either.

Quoted 27 lines

Why not something like:

(quoted diff) +17 -0

+static struct uprobe *__find_uprobe(struct inode * inode, loff_t offset,
                                      bool inode_only)
+{
        struct uprobe u = { .inode = inode, .offset = inode_only ? 0 : offset };
+       struct rb_node *n = uprobes_tree.rb_node;
+       struct uprobe *uprobe;
      struct uprobe *ret = NULL;
+       int match;
+
+       while (n) {
+               uprobe = rb_entry(n, struct uprobe, rb_node);
+               match = match_uprobe(&u, uprobe);
+               if (!match) {
                      if (!inode_only)
                             atomic_inc(&uprobe->ref);
+                       return uprobe;
+               }
              if (inode_only && uprobe->inode == inode)
                      ret = uprobe;
+               if (match < 0)
+                       n = n->rb_left;
+               else
+                       n = n->rb_right;
+

include/linux/uprobes.h static inline void unregister_uprobe(struct inode *inode, loff_t offset, · line 74

Quoted 4 lines

include/linux/uprobes.h +2 -0

+       }
        return ret;
+}

I am not comfortable with this change.
find_uprobe() was suppose to return back a uprobe if and only if the inode and offset match,

However with your approach, we end up
returning a uprobe that isnt matching and one that isnt refcounted. Moreover if even if we have a matching uprobe, we end up sending a unrefcounted uprobe back.

Quoted 27 lines · Srikar Dronamraju

Quoted 27 lines

Why not something like:

(quoted diff) +17 -0

+static struct uprobe *__find_uprobe(struct inode * inode, loff_t offset,
                                      bool inode_only)
+{
        struct uprobe u = { .inode = inode, .offset = inode_only ? 0 : offset };
+       struct rb_node *n = uprobes_tree.rb_node;
+       struct uprobe *uprobe;
      struct uprobe *ret = NULL;
+       int match;
+
+       while (n) {
+               uprobe = rb_entry(n, struct uprobe, rb_node);
+               match = match_uprobe(&u, uprobe);
+               if (!match) {
                      if (!inode_only)
                             atomic_inc(&uprobe->ref);
+                       return uprobe;
+               }
              if (inode_only && uprobe->inode == inode)
                      ret = uprobe;
+               if (match < 0)
+                       n = n->rb_left;
+               else
+                       n = n->rb_right;
+

Quoted 8 lines

include/linux/uprobes.h static inline void unregister_uprobe(struct inode *inode, loff_t offset, · line 74

Quoted 4 lines

include/linux/uprobes.h +2 -0

+       }
        return ret;
+}

I am not comfortable with this change.
find_uprobe() was suppose to return back a uprobe if and only if the inode and offset match,

And it will, because find_uprobe() will never expose that third argument.

Quoted 4 lines

However with your approach, we end up
returning a uprobe that isnt matching and one that isnt refcounted. Moreover if even if we have a matching uprobe, we end up sending a unrefcounted uprobe back.

Because the matching isn't the important part, you want to return the leftmost node matching the specified inode. Also, in that case you explicitly don't want the ref, since the first thing you do on the call-site is drop the ref if there was a match. You don't care about inode:0 in particular, you want a place to start iterating all of inode:*.

Quoted 1 line

Isn't good enough? Also, returning an rb_node just seems iffy..

yup this can be done. can you please elaborate on why passing back an rb_node is an issue?

kernel/uprobes.c +6 -1

-static int match_uprobe(struct uprobe *l, struct uprobe *r)
+static int match_uprobe(struct uprobe *l, struct uprobe *r, int *match_inode)
 {
+	/*
+	 * if match_inode is non NULL then indicate if the
+	 * inode atleast match.
+	 */
+	if (match_inode)

kernel/uprobes.c +17 -0

+int mmap_uprobe(struct vm_area_struct *vma)
+{
+	struct list_head tmp_list;
+	struct uprobe *uprobe, *u;
+	struct inode *inode;
+	int ret = 0;
+
+	if (!valid_vma(vma))
+		return ret;	/* Bail-out */
+
+	inode = igrab(vma->vm_file->f_mapping->host);
+	if (!inode)
+		return ret;
+
+	INIT_LIST_HEAD(&tmp_list);
+	mutex_lock(&uprobes_mmap_mutex);
+	build_probe_list(inode, &tmp_list);

kernel/uprobes.c +18 -0

+static void dec_mm_uprobes_count(struct vm_area_struct *vma,
+		struct inode *inode)
+{
+	struct uprobe *uprobe;
+	struct rb_node *n;
+	unsigned long flags;
+
+	n = uprobes_tree.rb_node;
+	spin_lock_irqsave(&uprobes_treelock, flags);
+	uprobe = __find_uprobe(inode, 0, &n);
+
+	/*
+	 * If indeed there is a probe for the inode and with offset zero,
+	 * then lets release its reference. (ref got thro __find_uprobe)
+	 */
+	if (uprobe)
+		put_uprobe(uprobe);
+	for (; n; n = rb_next(n)) {

kernel/fork.c struct mm_struct *dup_mm(struct task_struct *tsk) · line 745

Quoted 8 lines · Srikar Dronamraju

kernel/fork.c +3 -0

@@ -739,6 +740,10 @@ struct mm_struct *dup_mm(struct task_struct *tsk)
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 	mm->pmd_huge_pte = NULL;
 #endif
+#ifdef CONFIG_UPROBES
+	atomic_set(&mm->mm_uprobes_count,
+			atomic_read(&oldmm->mm_uprobes_count));

Hmm. Why this can't race with install_breakpoint/remove_breakpoint between _read and _set ?

What about VM_DONTCOPY vma's with breakpoints ?

Quoted 9 lines

kernel/uprobes.c +6 -1

-static int match_uprobe(struct uprobe *l, struct uprobe *r)
+static int match_uprobe(struct uprobe *l, struct uprobe *r, int *match_inode)
 {
+	/*
+	 * if match_inode is non NULL then indicate if the
+	 * inode atleast match.
+	 */
+	if (match_inode)

kernel/uprobes.c static bool valid_vma(struct vm_area_struct *vma) · line 78

Quoted 10 lines

kernel/uprobes.c +5 -0

+		*match_inode = 0;
+
 	if (l->inode < r->inode)
 		return -1;
 	if (l->inode > r->inode)
 		return 1;
 	else {
+		if (match_inode)
+			*match_inode = 1;
+

It is very possible I missed something, but imho this looks confusing.

This close_match logic is only needed for build_probe_list() and dec_mm_uprobes_count(), and both do not actually need the returned uprobe.

Instead of complicating match_uprobe() and __find_uprobe(), perhaps it makes sense to add "struct rb_node *__find_close_rb_node(inode)" ?

kernel/uprobes.c static bool del_consumer(struct uprobe *uprobe, · line 256

Quoted 6 lines

kernel/uprobes.c +3 -0

+static int install_breakpoint(struct mm_struct *mm, struct uprobe *uprobe)
 {
 	/* Placeholder: Yet to be implemented */
+	if (!uprobe->consumers)
+		return 0;

How it is possible to see ->consumers == NULL?

OK, afaics it _is_ possible, but only because unregister does del_consumer() without ->i_mutex, but this is bug afaics (see the previous email).

Another user is mmap_uprobe() and it checks ->consumers != NULL itself (but see below).

Quoted 17 lines

kernel/uprobes.c +17 -0

+int mmap_uprobe(struct vm_area_struct *vma)
+{
+	struct list_head tmp_list;
+	struct uprobe *uprobe, *u;
+	struct inode *inode;
+	int ret = 0;
+
+	if (!valid_vma(vma))
+		return ret;	/* Bail-out */
+
+	inode = igrab(vma->vm_file->f_mapping->host);
+	if (!inode)
+		return ret;
+
+	INIT_LIST_HEAD(&tmp_list);
+	mutex_lock(&uprobes_mmap_mutex);
+	build_probe_list(inode, &tmp_list);

kernel/uprobes.c void unregister_uprobe(struct inode *inode, loff_t offset, · line 567

Quoted 10 lines

kernel/uprobes.c +10 -0

+	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
+		loff_t vaddr;
+
+		list_del(&uprobe->pending_list);
+		if (!ret && uprobe->consumers) {
+			vaddr = vma->vm_start + uprobe->offset;
+			vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+			if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
+				continue;
+			ret = install_breakpoint(vma->vm_mm, uprobe);

So. We are adding the new mapping, we should find all breakpoints this file has in the start/end range.

We are holding ->mmap_sem... this seems enough to protect against the races with register/unregister. Except, what if __register_uprobe() fails? In this case __unregister_uprobe() does delete_uprobe() at the very end. What if mmap mmap_uprobe() is called right before delete_?

Quoted 22 lines

kernel/uprobes.c +22 -0

+static void dec_mm_uprobes_count(struct vm_area_struct *vma,
+		struct inode *inode)
+{
+	struct uprobe *uprobe;
+	struct rb_node *n;
+	unsigned long flags;
+
+	n = uprobes_tree.rb_node;
+	spin_lock_irqsave(&uprobes_treelock, flags);
+	uprobe = __find_uprobe(inode, 0, &n);
+
+	/*
+	 * If indeed there is a probe for the inode and with offset zero,
+	 * then lets release its reference. (ref got thro __find_uprobe)
+	 */
+	if (uprobe)
+		put_uprobe(uprobe);
+	for (; n; n = rb_next(n)) {
+		loff_t vaddr;
+
+		uprobe = rb_entry(n, struct uprobe, rb_node);
+		if (uprobe->inode != inode)

kernel/uprobes.c void unregister_uprobe(struct inode *inode, loff_t offset, · line 607

Quoted 6 lines

kernel/uprobes.c +6 -0

+			break;
+		vaddr = vma->vm_start + uprobe->offset;
+		vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+		if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
+			continue;
+		atomic_dec(&vma->vm_mm->mm_uprobes_count);

So, this does atomic_dec() for each bp in this vma?

And the caller is

mm/memory.c unsigned long unmap_vmas(struct mmu_gather *tlb, · line 1342

Quoted 6 lines

mm/memory.c +2 -0

@@ -1337,6 +1338,9 @@ unsigned long unmap_vmas(struct mmu_gather *tlb,
 		if (unlikely(is_pfn_mapping(vma)))
 			untrack_pfn_vma(vma, 0, 0);
 
+		if (vma->vm_file)
+			munmap_uprobe(vma);

Doesn't look right...

munmap_uprobe() assumes that the whole region goes away. This is true in munmap() case afaics, it does __split_vma() if necessary.

But what about truncate() ? In this case this vma is not unmapped, but unmap_vmas() is called anyway and [start, end) can be different. IOW, unless I missed something (this is very possible) we can do more atomic_dec's then needed.

Also, truncate() obviously changes ->i_size. Doesn't this mean unregister_uprobe() should return if offset > i_size ? We need to free uprobes anyway.

MADV_DONTNEED? It calls unmap_vmas() too. And application can do madvise(DONTNEED) in a loop.

Quoted 12 lines · Oleg Nesterov

kernel/fork.c struct mm_struct *dup_mm(struct task_struct *tsk) · line 745

Quoted 8 lines · Srikar Dronamraju

kernel/fork.c +3 -0

@@ -739,6 +740,10 @@ struct mm_struct *dup_mm(struct task_struct *tsk)
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 	mm->pmd_huge_pte = NULL;
 #endif
+#ifdef CONFIG_UPROBES
+	atomic_set(&mm->mm_uprobes_count,
+			atomic_read(&oldmm->mm_uprobes_count));

Hmm. Why this can't race with install_breakpoint/remove_breakpoint between _read and _set ?

At this time the child vmas are not yet created, so I dont see a install_breakpoints/remove_breakpoints from child affecting.

However if install_breakpoints/remove_breakpoints happen from a parent context, from now on till we do a vma_prio_tree_add (actually down_write(oldmm->mmap_sem) in dup_mmap()), then the count in the child may not be the right one.
If you are pointing to this race, then its probably bigger than just between read and set.

Quoted 7 lines

kernel/uprobes.c static bool del_consumer(struct uprobe *uprobe, · line 256

Quoted 5 lines

kernel/uprobes.c +3 -0

+static int install_breakpoint(struct mm_struct *mm, struct uprobe *uprobe)
 {
 	/* Placeholder: Yet to be implemented */
+	if (!uprobe->consumers)
+		return 0;

How it is possible to see ->consumers == NULL?

consumers == NULL check is mostly for the mmap_uprobe path.

_register_uprobe and _unregister_uprobe() use the same lock to serialize so they can check consumers after taking the lock.

Quoted 2 lines

OK, afaics it _is_ possible, but only because unregister does del_consumer() without ->i_mutex, but this is bug afaics (see the previous email).

We have discussed this in the other thread.

Quoted 31 lines

Quoted 17 lines

kernel/uprobes.c +17 -0

+int mmap_uprobe(struct vm_area_struct *vma)
+{
+	struct list_head tmp_list;
+	struct uprobe *uprobe, *u;
+	struct inode *inode;
+	int ret = 0;
+
+	if (!valid_vma(vma))
+		return ret;	/* Bail-out */
+
+	inode = igrab(vma->vm_file->f_mapping->host);
+	if (!inode)
+		return ret;
+
+	INIT_LIST_HEAD(&tmp_list);
+	mutex_lock(&uprobes_mmap_mutex);
+	build_probe_list(inode, &tmp_list);

kernel/uprobes.c void unregister_uprobe(struct inode *inode, loff_t offset, · line 567

Quoted 10 lines

kernel/uprobes.c +10 -0

+	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
+		loff_t vaddr;
+
+		list_del(&uprobe->pending_list);
+		if (!ret && uprobe->consumers) {
+			vaddr = vma->vm_start + uprobe->offset;
+			vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+			if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
+				continue;
+			ret = install_breakpoint(vma->vm_mm, uprobe);

So. We are adding the new mapping, we should find all breakpoints this file has in the start/end range.

Quoted 5 lines

We are holding ->mmap_sem... this seems enough to protect against the races with register/unregister. Except, what if __register_uprobe() fails? In this case __unregister_uprobe() does delete_uprobe() at the very end. What if mmap mmap_uprobe() is called right before delete_?

Because consumers would be NULL before _unregister_uprobe kicks in, we shouldnt have a problem here.

Am I missing something?

Quoted 3 lines

Also, truncate() obviously changes ->i_size. Doesn't this mean unregister_uprobe() should return if offset > i_size ? We need to free uprobes anyway.

Do you mean we shouldnt check for the offset in unregister_uprobe() and just search in the rbtree for the matching uprobe?
Thats also possible to do.

I think this would be taken care of if we move the munmap_uprobe() hook from unmap_vmas to unlink_file_vma().

The other thing that I need to investigate a bit more is if I have handle all cases of mremap correctly.

Quoted 10 lines

Quoted 9 lines

So. We are adding the new mapping, we should find all breakpoints this file has in the start/end range.

We are holding ->mmap_sem... this seems enough to protect against the races with register/unregister. Except, what if __register_uprobe() fails? In this case __unregister_uprobe() does delete_uprobe() at the very end. What if mmap mmap_uprobe() is called right before delete_?

Quoted 2 lines

Because consumers would be NULL before _unregister_uprobe kicks in, we shouldnt have a problem here.

Hmm. But it is not NULL.

Once again, I didn't mean unregister_uprobe(). I meant register_uprobe(). In this case, if __register_uprobe() fails, we are doing __unregister but uprobe->consumer != NULL.

Just suppose that the caller of register_uprobe() gets a (long) preemption right before __unregister_uprobe()->delete_uprobe(). What if mmap() is called at this time?

Quoted 1 line

Am I missing something?

May be you, may be me. Please recheck ;)

Quoted 3 lines

I think this would be taken care of if we move the munmap_uprobe() hook from unmap_vmas to unlink_file_vma().

Probably yes, we should rely on prio_tree locking/changes.

Quoted 2 lines

The other thing that I need to investigate a bit more is if I have handle all cases of mremap correctly.

Yes. May be mmap_uprobe() should be "closer" to vma_prio_tree_add/insert too, but I am not sure.

Oleg.

kernel/uprobes.c +10 -0

+static int __copy_insn(struct address_space *mapping,
+			struct vm_area_struct *vma, char *insn,
+			unsigned long nbytes, unsigned long offset)
+{
+	struct file *filp = vma->vm_file;
+	struct page *page;
+	void *vaddr;
+	unsigned long off1;
+	unsigned long idx;
+

kernel/uprobes.c +6 -2

-static int install_breakpoint(struct mm_struct *mm, struct uprobe *uprobe)
+static int install_breakpoint(struct mm_struct *mm, struct uprobe *uprobe,
+				struct vm_area_struct *vma, loff_t vaddr)
 {
-	/* Placeholder: Yet to be implemented */
+	struct task_struct *tsk;
+	unsigned long addr;
+	int ret = -EINVAL;
+
 	if (!uprobe->consumers)