diff mbox

drm/rockchip: use drm_gem_mmap helpers

Message ID 1436259816-31090-1-git-send-email-djkurtz@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Kurtz July 7, 2015, 9:03 a.m. UTC 
Rather than (incompletely [0]) re-implementing drm_gem_mmap() and
drm_gem_mmap_obj() helpers, call them directly from the rockchip mmap
routines.

Once the core functions return successfully, the rockchip mmap routines
can still use dma_mmap_attrs() to simply mmap the entire buffer.

[0] Previously, we were performing the mmap() without first taking a
reference on the underlying gem buffer.  This could leak ptes if the gem
object is destroyed while userspace is still holding the mapping.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org

---
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 67 +++++++++++++++--------------
 1 file changed, 34 insertions(+), 33 deletions(-)

Comments

Daniel Vetter July 7, 2015, 12:04 p.m. UTC | #1 
On Tue, Jul 07, 2015 at 05:03:36PM +0800, Daniel Kurtz wrote:
> Rather than (incompletely [0]) re-implementing drm_gem_mmap() and
> drm_gem_mmap_obj() helpers, call them directly from the rockchip mmap
> routines.
> 
> Once the core functions return successfully, the rockchip mmap routines
> can still use dma_mmap_attrs() to simply mmap the entire buffer.
> 
> [0] Previously, we were performing the mmap() without first taking a
> reference on the underlying gem buffer.  This could leak ptes if the gem
> object is destroyed while userspace is still holding the mapping.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stable@vger.kernel.org

Applied to topic/drm-fixes to make sure it won't get lost, but I expect
rockchip maintainers to pick this one up.
-Daniel

> 
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 67 +++++++++++++++--------------
>  1 file changed, 34 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index eb2282c..eba5f8a 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -54,55 +54,56 @@ static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
>  		       &rk_obj->dma_attrs);
>  }
>  
> -int rockchip_gem_mmap_buf(struct drm_gem_object *obj,
> -			  struct vm_area_struct *vma)
> +static int rockchip_drm_gem_object_mmap(struct drm_gem_object *obj,
> +					struct vm_area_struct *vma)
> +
>  {
> +	int ret;
>  	struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
>  	struct drm_device *drm = obj->dev;
> -	unsigned long vm_size;
>  
> -	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> -	vm_size = vma->vm_end - vma->vm_start;
> -
> -	if (vm_size > obj->size)
> -		return -EINVAL;
> +	/*
> +	 * dma_alloc_attrs() allocated a struct page table for rk_obj, so clear
> +	 * VM_PFNMAP flag that was set by drm_gem_mmap_obj()/drm_gem_mmap().
> +	 */
> +	vma->vm_flags &= ~VM_PFNMAP;
>  
> -	return dma_mmap_attrs(drm->dev, vma, rk_obj->kvaddr, rk_obj->dma_addr,
> +	ret = dma_mmap_attrs(drm->dev, vma, rk_obj->kvaddr, rk_obj->dma_addr,
>  			     obj->size, &rk_obj->dma_attrs);
> +	if (ret)
> +		drm_gem_vm_close(vma);
> +
> +	return ret;
>  }
>  
> -/* drm driver mmap file operations */
> -int rockchip_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> +int rockchip_gem_mmap_buf(struct drm_gem_object *obj,
> +			  struct vm_area_struct *vma)
>  {
> -	struct drm_file *priv = filp->private_data;
> -	struct drm_device *dev = priv->minor->dev;
> -	struct drm_gem_object *obj;
> -	struct drm_vma_offset_node *node;
> +	struct drm_device *drm = obj->dev;
>  	int ret;
>  
> -	if (drm_device_is_unplugged(dev))
> -		return -ENODEV;
> +	mutex_lock(&drm->struct_mutex);
> +	ret = drm_gem_mmap_obj(obj, obj->size, vma);
> +	mutex_unlock(&drm->struct_mutex);
> +	if (ret)
> +		return ret;
>  
> -	mutex_lock(&dev->struct_mutex);
> +	return rockchip_drm_gem_object_mmap(obj, vma);
> +}
>  
> -	node = drm_vma_offset_exact_lookup(dev->vma_offset_manager,
> -					   vma->vm_pgoff,
> -					   vma_pages(vma));
> -	if (!node) {
> -		mutex_unlock(&dev->struct_mutex);
> -		DRM_ERROR("failed to find vma node.\n");
> -		return -EINVAL;
> -	} else if (!drm_vma_node_is_allowed(node, filp)) {
> -		mutex_unlock(&dev->struct_mutex);
> -		return -EACCES;
> -	}
> +/* drm driver mmap file operations */
> +int rockchip_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> +	struct drm_gem_object *obj;
> +	int ret;
>  
> -	obj = container_of(node, struct drm_gem_object, vma_node);
> -	ret = rockchip_gem_mmap_buf(obj, vma);
> +	ret = drm_gem_mmap(filp, vma);
> +	if (ret)
> +		return ret;
>  
> -	mutex_unlock(&dev->struct_mutex);
> +	obj = vma->vm_private_data;
>  
> -	return ret;
> +	return rockchip_drm_gem_object_mmap(obj, vma);
>  }
>  
>  struct rockchip_gem_object *
> -- 
> 2.4.3.573.g4eafbef
>
yao mark July 8, 2015, 4:06 a.m. UTC | #2 
On 2015?07?07? 20:04, Daniel Vetter wrote:
> On Tue, Jul 07, 2015 at 05:03:36PM +0800, Daniel Kurtz wrote:
>> Rather than (incompletely [0]) re-implementing drm_gem_mmap() and
>> drm_gem_mmap_obj() helpers, call them directly from the rockchip mmap
>> routines.
>>
>> Once the core functions return successfully, the rockchip mmap routines
>> can still use dma_mmap_attrs() to simply mmap the entire buffer.
>>
>> [0] Previously, we were performing the mmap() without first taking a
>> reference on the underlying gem buffer.  This could leak ptes if the gem
>> object is destroyed while userspace is still holding the mapping.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: stable@vger.kernel.org
> Applied to topic/drm-fixes to make sure it won't get lost, but I expect
> rockchip maintainers to pick this one up.
> -Daniel
I try to pick this patch up, but found it conflicts with patch [0].  Can 
you fix it?

[0]https://patchwork.kernel.org/patch/6226591/

-Mark
>> ---
>>   drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 67 +++++++++++++++--------------
>>   1 file changed, 34 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>> index eb2282c..eba5f8a 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>> @@ -54,55 +54,56 @@ static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
>>   		       &rk_obj->dma_attrs);
>>   }
>>   
>> -int rockchip_gem_mmap_buf(struct drm_gem_object *obj,
>> -			  struct vm_area_struct *vma)
>> +static int rockchip_drm_gem_object_mmap(struct drm_gem_object *obj,
>> +					struct vm_area_struct *vma)
>> +
>>   {
>> +	int ret;
>>   	struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
>>   	struct drm_device *drm = obj->dev;
>> -	unsigned long vm_size;
>>   
>> -	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
>> -	vm_size = vma->vm_end - vma->vm_start;
>> -
>> -	if (vm_size > obj->size)
>> -		return -EINVAL;
>> +	/*
>> +	 * dma_alloc_attrs() allocated a struct page table for rk_obj, so clear
>> +	 * VM_PFNMAP flag that was set by drm_gem_mmap_obj()/drm_gem_mmap().
>> +	 */
>> +	vma->vm_flags &= ~VM_PFNMAP;
>>   
>> -	return dma_mmap_attrs(drm->dev, vma, rk_obj->kvaddr, rk_obj->dma_addr,
>> +	ret = dma_mmap_attrs(drm->dev, vma, rk_obj->kvaddr, rk_obj->dma_addr,
>>   			     obj->size, &rk_obj->dma_attrs);
>> +	if (ret)
>> +		drm_gem_vm_close(vma);
>> +
>> +	return ret;
>>   }
>>   
>> -/* drm driver mmap file operations */
>> -int rockchip_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>> +int rockchip_gem_mmap_buf(struct drm_gem_object *obj,
>> +			  struct vm_area_struct *vma)
>>   {
>> -	struct drm_file *priv = filp->private_data;
>> -	struct drm_device *dev = priv->minor->dev;
>> -	struct drm_gem_object *obj;
>> -	struct drm_vma_offset_node *node;
>> +	struct drm_device *drm = obj->dev;
>>   	int ret;
>>   
>> -	if (drm_device_is_unplugged(dev))
>> -		return -ENODEV;
>> +	mutex_lock(&drm->struct_mutex);
>> +	ret = drm_gem_mmap_obj(obj, obj->size, vma);
>> +	mutex_unlock(&drm->struct_mutex);
>> +	if (ret)
>> +		return ret;
>>   
>> -	mutex_lock(&dev->struct_mutex);
>> +	return rockchip_drm_gem_object_mmap(obj, vma);
>> +}
>>   
>> -	node = drm_vma_offset_exact_lookup(dev->vma_offset_manager,
>> -					   vma->vm_pgoff,
>> -					   vma_pages(vma));
>> -	if (!node) {
>> -		mutex_unlock(&dev->struct_mutex);
>> -		DRM_ERROR("failed to find vma node.\n");
>> -		return -EINVAL;
>> -	} else if (!drm_vma_node_is_allowed(node, filp)) {
>> -		mutex_unlock(&dev->struct_mutex);
>> -		return -EACCES;
>> -	}
>> +/* drm driver mmap file operations */
>> +int rockchip_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>> +{
>> +	struct drm_gem_object *obj;
>> +	int ret;
>>   
>> -	obj = container_of(node, struct drm_gem_object, vma_node);
>> -	ret = rockchip_gem_mmap_buf(obj, vma);
>> +	ret = drm_gem_mmap(filp, vma);
>> +	if (ret)
>> +		return ret;
>>   
>> -	mutex_unlock(&dev->struct_mutex);
>> +	obj = vma->vm_private_data;
>>   
>> -	return ret;
>> +	return rockchip_drm_gem_object_mmap(obj, vma);
>>   }
>>   
>>   struct rockchip_gem_object *
>> -- 
>> 2.4.3.573.g4eafbef
>>
Daniel Vetter July 8, 2015, 7:35 a.m. UTC | #3 
On Wed, Jul 08, 2015 at 12:06:53PM +0800, Mark yao wrote:
> On 2015?07?07? 20:04, Daniel Vetter wrote:
> >On Tue, Jul 07, 2015 at 05:03:36PM +0800, Daniel Kurtz wrote:
> >>Rather than (incompletely [0]) re-implementing drm_gem_mmap() and
> >>drm_gem_mmap_obj() helpers, call them directly from the rockchip mmap
> >>routines.
> >>
> >>Once the core functions return successfully, the rockchip mmap routines
> >>can still use dma_mmap_attrs() to simply mmap the entire buffer.
> >>
> >>[0] Previously, we were performing the mmap() without first taking a
> >>reference on the underlying gem buffer.  This could leak ptes if the gem
> >>object is destroyed while userspace is still holding the mapping.
> >>
> >>Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> >>Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>Cc: stable@vger.kernel.org
> >Applied to topic/drm-fixes to make sure it won't get lost, but I expect
> >rockchip maintainers to pick this one up.
> >-Daniel
> I try to pick this patch up, but found it conflicts with patch [0].  Can you
> fix it?
> 
> [0]https://patchwork.kernel.org/patch/6226591/

Imo this should be the other way round since Daniel's patch fixes a fairly
serious issue: Apply this fix first, rebase&queue the polish for -next.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
index eb2282c..eba5f8a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
@@ -54,55 +54,56 @@  static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
 		       &rk_obj->dma_attrs);
 }
 
-int rockchip_gem_mmap_buf(struct drm_gem_object *obj,
-			  struct vm_area_struct *vma)
+static int rockchip_drm_gem_object_mmap(struct drm_gem_object *obj,
+					struct vm_area_struct *vma)
+
 {
+	int ret;
 	struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
 	struct drm_device *drm = obj->dev;
-	unsigned long vm_size;
 
-	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
-	vm_size = vma->vm_end - vma->vm_start;
-
-	if (vm_size > obj->size)
-		return -EINVAL;
+	/*
+	 * dma_alloc_attrs() allocated a struct page table for rk_obj, so clear
+	 * VM_PFNMAP flag that was set by drm_gem_mmap_obj()/drm_gem_mmap().
+	 */
+	vma->vm_flags &= ~VM_PFNMAP;
 
-	return dma_mmap_attrs(drm->dev, vma, rk_obj->kvaddr, rk_obj->dma_addr,
+	ret = dma_mmap_attrs(drm->dev, vma, rk_obj->kvaddr, rk_obj->dma_addr,
 			     obj->size, &rk_obj->dma_attrs);
+	if (ret)
+		drm_gem_vm_close(vma);
+
+	return ret;
 }
 
-/* drm driver mmap file operations */
-int rockchip_gem_mmap(struct file *filp, struct vm_area_struct *vma)
+int rockchip_gem_mmap_buf(struct drm_gem_object *obj,
+			  struct vm_area_struct *vma)
 {
-	struct drm_file *priv = filp->private_data;
-	struct drm_device *dev = priv->minor->dev;
-	struct drm_gem_object *obj;
-	struct drm_vma_offset_node *node;
+	struct drm_device *drm = obj->dev;
 	int ret;
 
-	if (drm_device_is_unplugged(dev))
-		return -ENODEV;
+	mutex_lock(&drm->struct_mutex);
+	ret = drm_gem_mmap_obj(obj, obj->size, vma);
+	mutex_unlock(&drm->struct_mutex);
+	if (ret)
+		return ret;
 
-	mutex_lock(&dev->struct_mutex);
+	return rockchip_drm_gem_object_mmap(obj, vma);
+}
 
-	node = drm_vma_offset_exact_lookup(dev->vma_offset_manager,
-					   vma->vm_pgoff,
-					   vma_pages(vma));
-	if (!node) {
-		mutex_unlock(&dev->struct_mutex);
-		DRM_ERROR("failed to find vma node.\n");
-		return -EINVAL;
-	} else if (!drm_vma_node_is_allowed(node, filp)) {
-		mutex_unlock(&dev->struct_mutex);
-		return -EACCES;
-	}
+/* drm driver mmap file operations */
+int rockchip_gem_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	struct drm_gem_object *obj;
+	int ret;
 
-	obj = container_of(node, struct drm_gem_object, vma_node);
-	ret = rockchip_gem_mmap_buf(obj, vma);
+	ret = drm_gem_mmap(filp, vma);
+	if (ret)
+		return ret;
 
-	mutex_unlock(&dev->struct_mutex);
+	obj = vma->vm_private_data;
 
-	return ret;
+	return rockchip_drm_gem_object_mmap(obj, vma);
 }
 
 struct rockchip_gem_object *