lkml.org 
[lkml]   [2012]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 0/3] uprobes: make register/unregister O(n)

I read this code a few times, but I still think I am getting confused
about few scenarios.

So please correct me.

> struct map_info {
> struct map_info *next;
> struct mm_struct *mm;
> loff_t vaddr;
> };
>
> static inline struct map_info *free_map_info(struct map_info *info)
> {
> struct map_info *next = info->next;
> kfree(info);
> return next;
> }
>
> static struct map_info *
> build_map_info(struct address_space *mapping, loff_t offset, bool is_register)
> {
> unsigned long pgoff = offset >> PAGE_SHIFT;
> struct prio_tree_iter iter;
> struct vm_area_struct *vma;
> struct map_info *curr = NULL;
> struct map_info *prev = NULL;
> struct map_info *info;
> int more = 0;
>
> again:
> mutex_lock(&mapping->i_mmap_mutex);
> vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
> if (!valid_vma(vma, is_register))
> continue;
>
> if (!prev) {
> prev = kmalloc(sizeof(struct map_info),
> GFP_NOWAIT | __GFP_NOMEMALLOC | __GFP_NOWARN);
> if (!prev) {
> more++;
> continue;
> }
> prev->next = NULL;
> }
>
> if (!atomic_inc_not_zero(&vma->vm_mm->mm_users))
> continue;
>
> info = prev;
> prev = prev->next;
> info->next = curr;
> curr = info;
>
> info->mm = vma->vm_mm;
> info->vaddr = vma_address(vma, offset);
> }
> mutex_unlock(&mapping->i_mmap_mutex);
>
> if (!more)
> goto out;
>
> prev = curr;
> while (curr) {
> mmput(curr->mm);
> curr = curr->next;
> }
>
> do {
> info = kmalloc(sizeof(struct map_info), GFP_KERNEL);
> if (!info) {
> curr = ERR_PTR(-ENOMEM);
> goto out;
> }
> info->next = prev;
> prev = info;
> } while (--more);
>
> goto again;

This is more theory
If the number of vmas in the priority tree keeps increasing in every
iteration, and the kmalloc(GFP_NOWAIT) fails i.e more is !0, then
dont we end up in a forever loop?

Cant we just restrict this to just 2 iterations? [And depend on
uprobe_mmap() to do the necessary if new vmas come in].

> out:

> while (prev)
> prev = free_map_info(prev);

If we were able to allocate all map_info objects in the first pass but
the last vma belonged to a mm thats at exit, i.e atomic_inc_non_zero
returned 0 , then prev is !NULL and more is 0. Then we seem to clear
all the map_info objects without even decreasing the mm counts for which
atomic_inc_non_zero() was successful. Will curr be proper in this case.

Should this while be an if?

I am sure I am missing something here. Probably I should take a look
again.

> return curr;
> }
>
> static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
> {
> struct map_info *info;
> int err = 0;
>
> info = build_map_info(uprobe->inode->i_mapping,
> uprobe->offset, is_register);
> if (IS_ERR(info))
> return PTR_ERR(info);
>
> while (info) {
> struct mm_struct *mm = info->mm;
> struct vm_area_struct *vma;
> loff_t vaddr;
>
> if (err)
> goto free;
>
> down_write(&mm->mmap_sem);
> vma = find_vma(mm, (unsigned long)info->vaddr);
> if (!vma || !valid_vma(vma, is_register))
> goto unlock;
>
> vaddr = vma_address(vma, uprobe->offset);
> if (vma->vm_file->f_mapping->host != uprobe->inode ||
> vaddr != info->vaddr)
> goto unlock;
>
> if (is_register) {
> err = install_breakpoint(uprobe, mm, vma, info->vaddr);
> /*
> * We can race against uprobe_register(), see the
> * comment near uprobe_hash().
> */
> if (err == -EEXIST)
> err = 0;
> } else {
> remove_breakpoint(uprobe, mm, info->vaddr);
> }
> unlock:
> up_write(&mm->mmap_sem);
> free:
> mmput(mm);
> info = free_map_info(info);
> }
>
> return err;
> }
>



\
 
 \ /
  Last update: 2012-06-04 20:21    [from the cache]
©2003-2011 Jasper Spaans