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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
>> ---
>>  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel