From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH 5/7] xen-gntdev: Add reference counting to maps Date: Fri, 17 Dec 2010 10:11:12 -0500 Message-ID: <4D0B7D90.7070108@tycho.nsa.gov> References: <1292545063-32107-1-git-send-email-dgdegra@tycho.nsa.gov> <1292545063-32107-6-git-send-email-dgdegra@tycho.nsa.gov> <4D0AB392.6080000@goop.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4D0AB392.6080000@goop.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jeremy Fitzhardinge Cc: xen-devel@lists.xensource.com, Ian.Campbell@citrix.com List-Id: xen-devel@lists.xenproject.org On 12/16/2010 07:49 PM, Jeremy Fitzhardinge wrote: > On 12/16/2010 04:17 PM, Daniel De Graaf wrote: >> This allows userspace to perform mmap() on the gntdev device and then >> immediately close the filehandle or remove the mapping using the >> remove ioctl, with the mapped area remaining valid until unmapped. >> This also fixes an infinite loop when a gntdev device is closed >> without first unmapping all areas. >> >> Signed-off-by: Daniel De Graaf >> --- >> drivers/xen/gntdev.c | 67 +++++++++++++++++++++++-------------------------- >> 1 files changed, 31 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c >> index 6a3c9e4..f1fc8fa 100644 >> --- a/drivers/xen/gntdev.c >> +++ b/drivers/xen/gntdev.c >> @@ -66,16 +66,18 @@ struct granted_page { >> >> struct grant_map { >> struct list_head next; >> - struct gntdev_priv *priv; >> struct vm_area_struct *vma; >> int index; >> int count; >> + atomic_t users; > > Does this need to be atomic? Won't it be happening under spinlock anyway? gntdev_put_map will not be called under spinlock if it is called from an munmap(), especially one that happens after the file is closed. >> @@ -517,11 +506,13 @@ static long gntdev_ioctl_unmap_grant_ref(struct gntdev_priv *priv, >> >> spin_lock(&priv->lock); >> map = gntdev_find_map_index(priv, op.index >> PAGE_SHIFT, op.count); >> - if (map) >> - err = gntdev_del_map(map); >> + if (map) { >> + list_del(&map->next); >> + gntdev_put_map(map); >> + err = 0; >> + } else >> + err = -EINVAL; > > What prevents unmap_grant_ref being called multiple times? gntdev_find_map_index searches in priv->list for the mapping; if found, it removes it from that list. A second search will just return -EINVAL, even if the pages are still mapped. >> spin_unlock(&priv->lock); >> - if (!err) >> - gntdev_free_map(map); >> return err; >> } >> >> @@ -599,13 +590,15 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) >> map = gntdev_find_map_index(priv, index, count); >> if (!map) >> goto unlock_out; >> - if (map->vma) >> + if (use_ptemod && map->vma) >> goto unlock_out; > > Does this depend on the later hvm patch? Whoops, that ended up in the wrong patch. I'll correct the pair. >> if (priv->mm != vma->vm_mm) { >> printk("%s: Huh? Other mm?\n", __FUNCTION__); >> goto unlock_out; >> } >> >> + atomic_inc(&map->users); >> + >> vma->vm_ops = &gntdev_vmops; >> >> vma->vm_flags |= VM_RESERVED; >> @@ -614,7 +607,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) >> vma->vm_flags |= VM_PFNMAP; >> >> vma->vm_private_data = map; >> - map->vma = vma; >> + >> + if (use_ptemod) >> + map->vma = vma; >> >> map->is_ro = !(vma->vm_flags & VM_WRITE); >> > > J > -- Daniel De Graaf National Security Agency