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

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

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

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