Messages in this thread | | | Subject | Re: [PATCH v4 3.0-rc2-tip 7/22] 7: uprobes: mmap and fork hooks. | From | Peter Zijlstra <> | Date | Fri, 24 Jun 2011 09:42:04 +0200 |
| |
On Fri, 2011-06-24 at 07:36 +0530, Srikar Dronamraju wrote: > > > > so I am thinking of a solution that includes most of your ideas along > > with using i_mmap_mutex in mmap_uprobe path. > > > > Addressing Peter's comments given on irc wrt i_mmap_mutex. >
> void _unregister_uprobe(...) > { > if (!del_consumer(...)) { // includes tree removal on last consumer > return; > } > if (uprobe->consumers) > return; > > mutex_lock(&mapping->i_mmap_mutex); //sync with mmap. > vma_prio_tree_foreach() { > // create list > } > > mutex_unlock(&mapping->i_mmap_mutex); > > list_for_each_entry_safe() { > // remove from list > down_read(&mm->mmap_sem); > remove_breakpoint(); // unconditional, if it wasn't there > up_read(&mm->mmap_sem); > } > > mutex_lock(&mapping->i_mmap_mutex); > delete_uprobe(uprobe); > mutex_unlock(&mapping->i_mmap_mutex); > > inode->uprobes_count--; > mutex_unlock(&inode->i_mutex);
Right, so this lonesome unlock got me puzzled for a while, I always find it best not to do asymmetric locking like this, keep the lock and unlock in the same function.
> } > > int register_uprobe(...) > { > uprobe = alloc_uprobe(...); // find or insert in tree > > mutex_lock(&inode->i_mutex); // sync with register/unregister > if (uprobe->consumers) { > add_consumer(); > goto put_unlock; > } > add_consumer(); > inode->uprobes_count++; > mutex_lock(&mapping->i_mmap_mutex); //sync with mmap. > vma_prio_tree_foreach(..) { > // get mm ref, add to list blah blah > } > > mutex_unlock(&mapping->i_mmap_mutex); > list_for_each_entry_safe() { > if (ret) { > // del from list etc.. > // > continue; > } > down_read(mm->mmap_sem); > ret = install_breakpoint(); > up_read(..); > // del from list etc.. > // > if (ret && (ret == -ESRCH || ret == -EEXIST)) > ret = 0; > } > > if (ret) > _unregister_uprobe(); > > put_unlock: > mutex_unlock(&inode->i_mutex);
You see, now this is a double unlock
> put_uprobe(uprobe); > return ret; > } > > void unregister_uprobe(...) > { > mutex_lock(&inode->i_mutex); // sync with register/unregister > uprobe = find_uprobe(); // ref++ > _unregister_uprobe(); > mutex_unlock(&inode->i_mutex);
idem
> put_uprobe(uprobe); > } > > int mmap_uprobe(struct vm_area_struct *vma) > { > struct list_head tmp_list; > struct uprobe *uprobe, *u; > struct mm_struct *mm; > struct inode *inode; > int ret = 0; > > if (!valid_vma(vma)) > return ret; /* Bail-out */ > > mm = vma->vm_mm; > inode = vma->vm_file->f_mapping->host; > if (inode->uprobes_count) > return ret; > __iget(inode); > > INIT_LIST_HEAD(&tmp_list); > > mutex_lock(&mapping->i_mmap_mutex); > add_to_temp_list(vma, inode, &tmp_list); > list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) { > loff_t vaddr; > > list_del(&uprobe->pending_list); > if (ret) > continue; > > vaddr = vma->vm_start + uprobe->offset; > vaddr -= vma->vm_pgoff << PAGE_SHIFT; > ret = install_breakpoint(mm, uprobe, vaddr);
Right, so this is the problem, you cannot do allocations under i_mmap_mutex, however I think you can under i_mutex.
> if (ret && (ret == -ESRCH || ret == -EEXIST)) > ret = 0; > } > > mutex_unlock(&mapping->i_mmap_mutex); > iput(inode); > return ret; > } > > int munmap_uprobe(struct vm_area_struct *vma) > { > struct list_head tmp_list; > struct uprobe *uprobe, *u; > struct mm_struct *mm; > struct inode *inode; > int ret = 0; > > if (!valid_vma(vma)) > return ret; /* Bail-out */ > > mm = vma->vm_mm; > inode = vma->vm_file->f_mapping->host; > if (inode->uprobes_count) > return ret;
Should that be !->uprobes_count?
> // walk thro RB tree and decrement mm->uprobes_count > walk_rbtree_and_dec_uprobes_count(); //hold treelock. > > return ret; > }
| |