From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751334Ab1IZNiy (ORCPT ); Mon, 26 Sep 2011 09:38:54 -0400 Received: from e5.ny.us.ibm.com ([32.97.182.145]:34923 "EHLO e5.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750952Ab1IZNix (ORCPT ); Mon, 26 Sep 2011 09:38:53 -0400 Date: Mon, 26 Sep 2011 18:53:37 +0530 From: Srikar Dronamraju To: Peter Zijlstra Cc: Ingo Molnar , Steven Rostedt , Linux-mm , Arnaldo Carvalho de Melo , Linus Torvalds , Jonathan Corbet , Hugh Dickins , Christoph Hellwig , Masami Hiramatsu , Thomas Gleixner , Andi Kleen , Oleg Nesterov , LKML , Jim Keniston , Roland McGrath , Ananth N Mavinakayanahalli , Andrew Morton Subject: Re: [PATCH v5 3.1.0-rc4-tip 3/26] Uprobes: register/unregister probes. Message-ID: <20110926132337.GA13535@linux.vnet.ibm.com> Reply-To: Srikar Dronamraju References: <20110920115938.25326.93059.sendpatchset@srdronam.in.ibm.com> <20110920120022.25326.35868.sendpatchset@srdronam.in.ibm.com> <1317042900.1763.7.camel@twins> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1317042900.1763.7.camel@twins> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Peter Zijlstra [2011-09-26 15:15:00]: > On Tue, 2011-09-20 at 17:30 +0530, Srikar Dronamraju wrote: > > > +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) { > > + 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. > No, There is a possibility of going in a forever loop. Since the the priotree can change when we drop the mapping->mutex, we dont pass the hint to vma_prio_tree_foreach. So we might keep getting the same vma again and again. > > + } > > + } > > + return NULL; > > +} > > + > > +/* > > + * Iterate in the rmap prio tree and find a vma where a probe has not > > + * yet been inserted. > > + */ > > +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. > > > > + 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; > > + 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.. > > > + 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. > Yes, this can be done. > > + 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? > Yes. > > + 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; > > +} > -- Thanks and Regards Srikar