* Oleg Nesterov <oleg@xxxxxxxxxx> [2011-10-05 20:50:08]: > On 10/05, Srikar Dronamraju wrote: > > > > 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; > > This is unneeded, vma_prio_tree_foreach() stops when vma_prio_tree_next() > returns NULL. IOW, you can never see vma == NULL. Agree. > > > if (!valid_vma(vma)) > > continue; > > Yes. > > > > > + 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 > > OOPS ;) may be deserves a comment... will add a comment. > > > > > +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; > > > > + } > > > > + > > > > + 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. > > Yes, but I meant in this case register_uprobe() does the unnecessary > __register_uprobe() because it sees ->consumers == NULL (add_consumer() > returns NULL). 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. > > I guess this is probably harmless because of is_bkpt_insn/-EEXIST > logic, but still. > Agree. > > 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. Okay, will do. > > Oh, wait. I just noticed that the original code does > > (ret != -ESRCH || ret != -EEXIST) > > this expression is always true ;) Right, will correct this. > > Oleg. > -- Thanks and Regards Srikar -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. 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@xxxxxxxxx";> email@xxxxxxxxx </a>