Re: [PATCH 5/7] xen-gntdev: Add reference counting to maps

[Jeremy Fitzhardinge <jeremy@goop.org>]

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 <dgdegra@tycho.nsa.gov>
> ---
>  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?

>  	int is_mapped:1;
>  	int is_ro:1;
>  	struct page **pages;
>  	struct granted_page pginfo[0];
>  };
>  
> +static void unmap_grant_pages(struct grant_map *map, int offset, int pages);
> +
>  /* ------------------------------------------------------------------ */
>  
>  static void gntdev_print_maps(struct gntdev_priv *priv,
> @@ -112,6 +114,7 @@ static struct grant_map *gntdev_alloc_map(int count,
>  	}
>  
>  	add->index = 0;
> +	atomic_set(&add->users, 1);
>  	add->count = count;
>  	for(i = 0; i < count; i++)
>  		add->pginfo[i].target = grants[i];
> @@ -144,7 +147,6 @@ static void gntdev_add_map(struct gntdev_priv *priv, struct grant_map *add)
>  	list_add_tail(&add->next, &priv->maps);
>  
>  done:
> -	add->priv = priv;
>  	if (debug)
>  		gntdev_print_maps(priv, "[new]", add->index);
>  	spin_unlock(&priv->lock);
> @@ -165,33 +167,26 @@ static struct grant_map *gntdev_find_map_index(struct gntdev_priv *priv, int ind
>  	return NULL;
>  }
>  
> -static int gntdev_del_map(struct grant_map *map)
> +static void gntdev_put_map(struct grant_map *map)
>  {
>  	int i;
>  
> -	if (map->vma)
> -		return -EBUSY;
> -	if (map->is_mapped)
> -		for (i = 0; i < map->count; i++)
> -			if (map->pginfo[i].handle)
> -				return -EBUSY;
> +	if (!map)
> +		return;
>  
> -	atomic_sub(map->count, &pages_mapped);
> -	list_del(&map->next);
> -	return 0;
> -}
> +	if (!atomic_dec_and_test(&map->users))
> +		return;
>  
> -static void gntdev_free_map(struct grant_map *map)
> -{
> -	unsigned i;
> +	atomic_sub(map->count, &pages_mapped);
>  
> -	if (!map)
> -		return;
> +	if (!use_ptemod)
> +		unmap_grant_pages(map, 0, map->count);
>  
>  	for (i = 0; i < map->count; i++) {
>  		if (map->pages[i])
>  			__free_page(map->pages[i]);
>  	}
> +	kfree(map->pages);
>  	kfree(map);
>  }
>  
> @@ -310,6 +305,7 @@ static void gntdev_vma_close(struct vm_area_struct *vma)
>  	map->is_mapped = 0;
>  	map->vma = NULL;
>  	vma->vm_private_data = NULL;
> +	gntdev_put_map(map);
>  }
>  
>  static int gntdev_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> @@ -425,7 +421,6 @@ static int gntdev_release(struct inode *inode, struct file *flip)
>  {
>  	struct gntdev_priv *priv = flip->private_data;
>  	struct grant_map *map;
> -	int err;
>  
>  	if (debug)
>  		printk("%s: priv %p\n", __FUNCTION__, priv);
> @@ -433,10 +428,8 @@ static int gntdev_release(struct inode *inode, struct file *flip)
>  	spin_lock(&priv->lock);
>  	while (!list_empty(&priv->maps)) {
>  		map = list_entry(priv->maps.next, struct grant_map, next);
> -		err = gntdev_del_map(map);
> -		if (WARN_ON(err))
> -			gntdev_free_map(map);
> -
> +		list_del(&map->next);
> +		gntdev_put_map(map);
>  	}
>  	spin_unlock(&priv->lock);
>  
> @@ -487,18 +480,14 @@ static long gntdev_ioctl_map_grant_ref(struct gntdev_priv *priv,
>  
>  	if (copy_to_user(u, &op, sizeof(op))) {
>  		err = -EFAULT;
> -		goto out_remove;
> +		goto out_free;
>  	}
>  	err = 0;
>  out_free:
>  	kfree(grants);
>  	return err;
> -out_remove:
> -	spin_lock(&priv->lock);
> -	gntdev_del_map(map);
> -	spin_unlock(&priv->lock);
>  out_free_map:
> -	gntdev_free_map(map);
> +	gntdev_put_map(map);
>  	goto out_free;
>  }
>  
> @@ -507,7 +496,7 @@ static long gntdev_ioctl_unmap_grant_ref(struct gntdev_priv *priv,
>  {
>  	struct ioctl_gntdev_unmap_grant_ref op;
>  	struct grant_map *map;
> -	int err = -EINVAL;
> +	int err;
>  
>  	if (copy_from_user(&op, u, sizeof(op)) != 0)
>  		return -EFAULT;
> @@ -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?

>  	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;
>  	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