[PATCH] drm/ttm: fix mmap refcounting

[PATCH] drm/ttm: fix mmap refcounting

[Gerd Hoffmann]

When mapping ttm objects via drm_gem_ttm_mmap() helper
drm_gem_mmap_obj() will take an object reference.  That gets
never released due to ttm having its own reference counting.

Fix that by dropping the gem object reference once the ttm mmap
completed (and ttm refcount got bumped).

For that to work properly the drm_gem_object_get() call in
drm_gem_ttm_mmap() must be moved so it happens before calling
obj->funcs->mmap(), otherwise the gem refcount would go down
to zero.

Fixes: 231927d939f0 ("drm/ttm: add drm_gem_ttm_mmap()")
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/drm_gem.c            | 24 ++++++++++++++----------
 drivers/gpu/drm/drm_gem_ttm_helper.c | 13 ++++++++++++-
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 2f2b889096b0..000fa4a1899f 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1105,21 +1105,33 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 	if (obj_size < vma->vm_end - vma->vm_start)
 		return -EINVAL;
 
+	/* Take a ref for this mapping of the object, so that the fault
+	 * handler can dereference the mmap offset's pointer to the object.
+	 * This reference is cleaned up by the corresponding vm_close
+	 * (which should happen whether the vma was created by this call, or
+	 * by a vm_open due to mremap or partial unmap or whatever).
+	 */
+	drm_gem_object_get(obj);
+
 	if (obj->funcs && obj->funcs->mmap) {
 		/* Remove the fake offset */
 		vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
 
 		ret = obj->funcs->mmap(obj, vma);
-		if (ret)
+		if (ret) {
+			drm_gem_object_put_unlocked(obj);
 			return ret;
+		}
 		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
 	} else {
 		if (obj->funcs && obj->funcs->vm_ops)
 			vma->vm_ops = obj->funcs->vm_ops;
 		else if (dev->driver->gem_vm_ops)
 			vma->vm_ops = dev->driver->gem_vm_ops;
-		else
+		else {
+			drm_gem_object_put_unlocked(obj);
 			return -EINVAL;
+		}
 
 		vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
 		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
@@ -1128,14 +1140,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 
 	vma->vm_private_data = obj;
 
-	/* Take a ref for this mapping of the object, so that the fault
-	 * handler can dereference the mmap offset's pointer to the object.
-	 * This reference is cleaned up by the corresponding vm_close
-	 * (which should happen whether the vma was created by this call, or
-	 * by a vm_open due to mremap or partial unmap or whatever).
-	 */
-	drm_gem_object_get(obj);
-
 	return 0;
 }
 EXPORT_SYMBOL(drm_gem_mmap_obj);
diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
index 7412bfc5c05a..605a8a3da7f9 100644
--- a/drivers/gpu/drm/drm_gem_ttm_helper.c
+++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
@@ -64,8 +64,19 @@ int drm_gem_ttm_mmap(struct drm_gem_object *gem,
 		     struct vm_area_struct *vma)
 {
 	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
+	int ret;
 
-	return ttm_bo_mmap_obj(vma, bo);
+	ret = ttm_bo_mmap_obj(vma, bo);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * ttm has its own object refcounting, so drop gem reference
+	 * to avoid double accounting counting.
+	 */
+	drm_gem_object_put_unlocked(gem);
+
+	return 0;
 }
 EXPORT_SYMBOL(drm_gem_ttm_mmap);
 
-- 
2.18.1

[PATCH] drm/ttm: fix mmap refcounting

[Gerd Hoffmann]

When mapping ttm objects via drm_gem_ttm_mmap() helper
drm_gem_mmap_obj() will take an object reference.  That gets
never released due to ttm having its own reference counting.

Fix that by dropping the gem object reference once the ttm mmap
completed (and ttm refcount got bumped).

For that to work properly the drm_gem_object_get() call in
drm_gem_ttm_mmap() must be moved so it happens before calling
obj->funcs->mmap(), otherwise the gem refcount would go down
to zero.

Fixes: 231927d939f0 ("drm/ttm: add drm_gem_ttm_mmap()")
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/drm_gem.c            | 24 ++++++++++++++----------
 drivers/gpu/drm/drm_gem_ttm_helper.c | 13 ++++++++++++-
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 2f2b889096b0..000fa4a1899f 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1105,21 +1105,33 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 	if (obj_size < vma->vm_end - vma->vm_start)
 		return -EINVAL;
 
+	/* Take a ref for this mapping of the object, so that the fault
+	 * handler can dereference the mmap offset's pointer to the object.
+	 * This reference is cleaned up by the corresponding vm_close
+	 * (which should happen whether the vma was created by this call, or
+	 * by a vm_open due to mremap or partial unmap or whatever).
+	 */
+	drm_gem_object_get(obj);
+
 	if (obj->funcs && obj->funcs->mmap) {
 		/* Remove the fake offset */
 		vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
 
 		ret = obj->funcs->mmap(obj, vma);
-		if (ret)
+		if (ret) {
+			drm_gem_object_put_unlocked(obj);
 			return ret;
+		}
 		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
 	} else {
 		if (obj->funcs && obj->funcs->vm_ops)
 			vma->vm_ops = obj->funcs->vm_ops;
 		else if (dev->driver->gem_vm_ops)
 			vma->vm_ops = dev->driver->gem_vm_ops;
-		else
+		else {
+			drm_gem_object_put_unlocked(obj);
 			return -EINVAL;
+		}
 
 		vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
 		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
@@ -1128,14 +1140,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 
 	vma->vm_private_data = obj;
 
-	/* Take a ref for this mapping of the object, so that the fault
-	 * handler can dereference the mmap offset's pointer to the object.
-	 * This reference is cleaned up by the corresponding vm_close
-	 * (which should happen whether the vma was created by this call, or
-	 * by a vm_open due to mremap or partial unmap or whatever).
-	 */
-	drm_gem_object_get(obj);
-
 	return 0;
 }
 EXPORT_SYMBOL(drm_gem_mmap_obj);
diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
index 7412bfc5c05a..605a8a3da7f9 100644
--- a/drivers/gpu/drm/drm_gem_ttm_helper.c
+++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
@@ -64,8 +64,19 @@ int drm_gem_ttm_mmap(struct drm_gem_object *gem,
 		     struct vm_area_struct *vma)
 {
 	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
+	int ret;
 
-	return ttm_bo_mmap_obj(vma, bo);
+	ret = ttm_bo_mmap_obj(vma, bo);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * ttm has its own object refcounting, so drop gem reference
+	 * to avoid double accounting counting.
+	 */
+	drm_gem_object_put_unlocked(gem);
+
+	return 0;
 }
 EXPORT_SYMBOL(drm_gem_ttm_mmap);
 
-- 
2.18.1

[PATCH] drm/ttm: fix mmap refcounting

[Gerd Hoffmann]

When mapping ttm objects via drm_gem_ttm_mmap() helper
drm_gem_mmap_obj() will take an object reference.  That gets
never released due to ttm having its own reference counting.

Fix that by dropping the gem object reference once the ttm mmap
completed (and ttm refcount got bumped).

For that to work properly the drm_gem_object_get() call in
drm_gem_ttm_mmap() must be moved so it happens before calling
obj->funcs->mmap(), otherwise the gem refcount would go down
to zero.

Fixes: 231927d939f0 ("drm/ttm: add drm_gem_ttm_mmap()")
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/drm_gem.c            | 24 ++++++++++++++----------
 drivers/gpu/drm/drm_gem_ttm_helper.c | 13 ++++++++++++-
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 2f2b889096b0..000fa4a1899f 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1105,21 +1105,33 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 	if (obj_size < vma->vm_end - vma->vm_start)
 		return -EINVAL;
 
+	/* Take a ref for this mapping of the object, so that the fault
+	 * handler can dereference the mmap offset's pointer to the object.
+	 * This reference is cleaned up by the corresponding vm_close
+	 * (which should happen whether the vma was created by this call, or
+	 * by a vm_open due to mremap or partial unmap or whatever).
+	 */
+	drm_gem_object_get(obj);
+
 	if (obj->funcs && obj->funcs->mmap) {
 		/* Remove the fake offset */
 		vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
 
 		ret = obj->funcs->mmap(obj, vma);
-		if (ret)
+		if (ret) {
+			drm_gem_object_put_unlocked(obj);
 			return ret;
+		}
 		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
 	} else {
 		if (obj->funcs && obj->funcs->vm_ops)
 			vma->vm_ops = obj->funcs->vm_ops;
 		else if (dev->driver->gem_vm_ops)
 			vma->vm_ops = dev->driver->gem_vm_ops;
-		else
+		else {
+			drm_gem_object_put_unlocked(obj);
 			return -EINVAL;
+		}
 
 		vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
 		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
@@ -1128,14 +1140,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 
 	vma->vm_private_data = obj;
 
-	/* Take a ref for this mapping of the object, so that the fault
-	 * handler can dereference the mmap offset's pointer to the object.
-	 * This reference is cleaned up by the corresponding vm_close
-	 * (which should happen whether the vma was created by this call, or
-	 * by a vm_open due to mremap or partial unmap or whatever).
-	 */
-	drm_gem_object_get(obj);
-
 	return 0;
 }
 EXPORT_SYMBOL(drm_gem_mmap_obj);
diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
index 7412bfc5c05a..605a8a3da7f9 100644
--- a/drivers/gpu/drm/drm_gem_ttm_helper.c
+++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
@@ -64,8 +64,19 @@ int drm_gem_ttm_mmap(struct drm_gem_object *gem,
 		     struct vm_area_struct *vma)
 {
 	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
+	int ret;
 
-	return ttm_bo_mmap_obj(vma, bo);
+	ret = ttm_bo_mmap_obj(vma, bo);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * ttm has its own object refcounting, so drop gem reference
+	 * to avoid double accounting counting.
+	 */
+	drm_gem_object_put_unlocked(gem);
+
+	return 0;
 }
 EXPORT_SYMBOL(drm_gem_ttm_mmap);
 
-- 
2.18.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ttm: fix mmap refcounting

[Daniel Vetter]

On Wed, Nov 13, 2019 at 02:56:12PM +0100, Gerd Hoffmann wrote:
> When mapping ttm objects via drm_gem_ttm_mmap() helper
> drm_gem_mmap_obj() will take an object reference.  That gets
> never released due to ttm having its own reference counting.
> 
> Fix that by dropping the gem object reference once the ttm mmap
> completed (and ttm refcount got bumped).
> 
> For that to work properly the drm_gem_object_get() call in
> drm_gem_ttm_mmap() must be moved so it happens before calling
> obj->funcs->mmap(), otherwise the gem refcount would go down
> to zero.
> 
> Fixes: 231927d939f0 ("drm/ttm: add drm_gem_ttm_mmap()")

Since the offending patch is in drm-next and we're in the merge window
freeze past -rc6 please remember to apply this to drm-misc-next-fixes.
Otherwise it'll miss the merge window.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

I was wondering whether we'd need a cc: stable, in case someone is really
fast and gets the vm_close in before we finish the mmap. But I think we
should be serialized by mmap_sem here enough ...

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_gem.c            | 24 ++++++++++++++----------
>  drivers/gpu/drm/drm_gem_ttm_helper.c | 13 ++++++++++++-
>  2 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2f2b889096b0..000fa4a1899f 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1105,21 +1105,33 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  	if (obj_size < vma->vm_end - vma->vm_start)
>  		return -EINVAL;
>  
> +	/* Take a ref for this mapping of the object, so that the fault
> +	 * handler can dereference the mmap offset's pointer to the object.
> +	 * This reference is cleaned up by the corresponding vm_close
> +	 * (which should happen whether the vma was created by this call, or
> +	 * by a vm_open due to mremap or partial unmap or whatever).
> +	 */
> +	drm_gem_object_get(obj);
> +
>  	if (obj->funcs && obj->funcs->mmap) {
>  		/* Remove the fake offset */
>  		vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>  
>  		ret = obj->funcs->mmap(obj, vma);
> -		if (ret)
> +		if (ret) {
> +			drm_gem_object_put_unlocked(obj);
>  			return ret;
> +		}
>  		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
>  	} else {
>  		if (obj->funcs && obj->funcs->vm_ops)
>  			vma->vm_ops = obj->funcs->vm_ops;
>  		else if (dev->driver->gem_vm_ops)
>  			vma->vm_ops = dev->driver->gem_vm_ops;
> -		else
> +		else {
> +			drm_gem_object_put_unlocked(obj);
>  			return -EINVAL;
> +		}
>  
>  		vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
>  		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> @@ -1128,14 +1140,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  
>  	vma->vm_private_data = obj;
>  
> -	/* Take a ref for this mapping of the object, so that the fault
> -	 * handler can dereference the mmap offset's pointer to the object.
> -	 * This reference is cleaned up by the corresponding vm_close
> -	 * (which should happen whether the vma was created by this call, or
> -	 * by a vm_open due to mremap or partial unmap or whatever).
> -	 */
> -	drm_gem_object_get(obj);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_mmap_obj);
> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
> index 7412bfc5c05a..605a8a3da7f9 100644
> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> @@ -64,8 +64,19 @@ int drm_gem_ttm_mmap(struct drm_gem_object *gem,
>  		     struct vm_area_struct *vma)
>  {
>  	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
> +	int ret;
>  
> -	return ttm_bo_mmap_obj(vma, bo);
> +	ret = ttm_bo_mmap_obj(vma, bo);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * ttm has its own object refcounting, so drop gem reference
> +	 * to avoid double accounting counting.
> +	 */
> +	drm_gem_object_put_unlocked(gem);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_ttm_mmap);
>  
> -- 
> 2.18.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/ttm: fix mmap refcounting

[Daniel Vetter]

On Wed, Nov 13, 2019 at 02:56:12PM +0100, Gerd Hoffmann wrote:
> When mapping ttm objects via drm_gem_ttm_mmap() helper
> drm_gem_mmap_obj() will take an object reference.  That gets
> never released due to ttm having its own reference counting.
> 
> Fix that by dropping the gem object reference once the ttm mmap
> completed (and ttm refcount got bumped).
> 
> For that to work properly the drm_gem_object_get() call in
> drm_gem_ttm_mmap() must be moved so it happens before calling
> obj->funcs->mmap(), otherwise the gem refcount would go down
> to zero.
> 
> Fixes: 231927d939f0 ("drm/ttm: add drm_gem_ttm_mmap()")

Since the offending patch is in drm-next and we're in the merge window
freeze past -rc6 please remember to apply this to drm-misc-next-fixes.
Otherwise it'll miss the merge window.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

I was wondering whether we'd need a cc: stable, in case someone is really
fast and gets the vm_close in before we finish the mmap. But I think we
should be serialized by mmap_sem here enough ...

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_gem.c            | 24 ++++++++++++++----------
>  drivers/gpu/drm/drm_gem_ttm_helper.c | 13 ++++++++++++-
>  2 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2f2b889096b0..000fa4a1899f 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1105,21 +1105,33 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  	if (obj_size < vma->vm_end - vma->vm_start)
>  		return -EINVAL;
>  
> +	/* Take a ref for this mapping of the object, so that the fault
> +	 * handler can dereference the mmap offset's pointer to the object.
> +	 * This reference is cleaned up by the corresponding vm_close
> +	 * (which should happen whether the vma was created by this call, or
> +	 * by a vm_open due to mremap or partial unmap or whatever).
> +	 */
> +	drm_gem_object_get(obj);
> +
>  	if (obj->funcs && obj->funcs->mmap) {
>  		/* Remove the fake offset */
>  		vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>  
>  		ret = obj->funcs->mmap(obj, vma);
> -		if (ret)
> +		if (ret) {
> +			drm_gem_object_put_unlocked(obj);
>  			return ret;
> +		}
>  		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
>  	} else {
>  		if (obj->funcs && obj->funcs->vm_ops)
>  			vma->vm_ops = obj->funcs->vm_ops;
>  		else if (dev->driver->gem_vm_ops)
>  			vma->vm_ops = dev->driver->gem_vm_ops;
> -		else
> +		else {
> +			drm_gem_object_put_unlocked(obj);
>  			return -EINVAL;
> +		}
>  
>  		vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
>  		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> @@ -1128,14 +1140,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  
>  	vma->vm_private_data = obj;
>  
> -	/* Take a ref for this mapping of the object, so that the fault
> -	 * handler can dereference the mmap offset's pointer to the object.
> -	 * This reference is cleaned up by the corresponding vm_close
> -	 * (which should happen whether the vma was created by this call, or
> -	 * by a vm_open due to mremap or partial unmap or whatever).
> -	 */
> -	drm_gem_object_get(obj);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_mmap_obj);
> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
> index 7412bfc5c05a..605a8a3da7f9 100644
> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> @@ -64,8 +64,19 @@ int drm_gem_ttm_mmap(struct drm_gem_object *gem,
>  		     struct vm_area_struct *vma)
>  {
>  	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
> +	int ret;
>  
> -	return ttm_bo_mmap_obj(vma, bo);
> +	ret = ttm_bo_mmap_obj(vma, bo);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * ttm has its own object refcounting, so drop gem reference
> +	 * to avoid double accounting counting.
> +	 */
> +	drm_gem_object_put_unlocked(gem);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_ttm_mmap);
>  
> -- 
> 2.18.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ttm: fix mmap refcounting

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4017 bytes --]



Am 13.11.19 um 14:56 schrieb Gerd Hoffmann:
> When mapping ttm objects via drm_gem_ttm_mmap() helper
> drm_gem_mmap_obj() will take an object reference.  That gets
> never released due to ttm having its own reference counting.
> 
> Fix that by dropping the gem object reference once the ttm mmap
> completed (and ttm refcount got bumped).
> 
> For that to work properly the drm_gem_object_get() call in
> drm_gem_ttm_mmap() must be moved so it happens before calling
> obj->funcs->mmap(), otherwise the gem refcount would go down
> to zero.
> 
> Fixes: 231927d939f0 ("drm/ttm: add drm_gem_ttm_mmap()")
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Tested-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>  drivers/gpu/drm/drm_gem.c            | 24 ++++++++++++++----------
>  drivers/gpu/drm/drm_gem_ttm_helper.c | 13 ++++++++++++-
>  2 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2f2b889096b0..000fa4a1899f 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1105,21 +1105,33 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  	if (obj_size < vma->vm_end - vma->vm_start)
>  		return -EINVAL;
>  
> +	/* Take a ref for this mapping of the object, so that the fault
> +	 * handler can dereference the mmap offset's pointer to the object.
> +	 * This reference is cleaned up by the corresponding vm_close
> +	 * (which should happen whether the vma was created by this call, or
> +	 * by a vm_open due to mremap or partial unmap or whatever).
> +	 */
> +	drm_gem_object_get(obj);
> +
>  	if (obj->funcs && obj->funcs->mmap) {
>  		/* Remove the fake offset */
>  		vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>  
>  		ret = obj->funcs->mmap(obj, vma);
> -		if (ret)
> +		if (ret) {
> +			drm_gem_object_put_unlocked(obj);
>  			return ret;
> +		}
>  		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
>  	} else {
>  		if (obj->funcs && obj->funcs->vm_ops)
>  			vma->vm_ops = obj->funcs->vm_ops;
>  		else if (dev->driver->gem_vm_ops)
>  			vma->vm_ops = dev->driver->gem_vm_ops;
> -		else
> +		else {
> +			drm_gem_object_put_unlocked(obj);
>  			return -EINVAL;
> +		}
>  
>  		vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
>  		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> @@ -1128,14 +1140,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  
>  	vma->vm_private_data = obj;
>  
> -	/* Take a ref for this mapping of the object, so that the fault
> -	 * handler can dereference the mmap offset's pointer to the object.
> -	 * This reference is cleaned up by the corresponding vm_close
> -	 * (which should happen whether the vma was created by this call, or
> -	 * by a vm_open due to mremap or partial unmap or whatever).
> -	 */
> -	drm_gem_object_get(obj);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_mmap_obj);
> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
> index 7412bfc5c05a..605a8a3da7f9 100644
> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> @@ -64,8 +64,19 @@ int drm_gem_ttm_mmap(struct drm_gem_object *gem,
>  		     struct vm_area_struct *vma)
>  {
>  	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
> +	int ret;
>  
> -	return ttm_bo_mmap_obj(vma, bo);
> +	ret = ttm_bo_mmap_obj(vma, bo);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * ttm has its own object refcounting, so drop gem reference
> +	 * to avoid double accounting counting.
> +	 */
> +	drm_gem_object_put_unlocked(gem);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_ttm_mmap);
>  
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] drm/ttm: fix mmap refcounting

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4017 bytes --]



Am 13.11.19 um 14:56 schrieb Gerd Hoffmann:
> When mapping ttm objects via drm_gem_ttm_mmap() helper
> drm_gem_mmap_obj() will take an object reference.  That gets
> never released due to ttm having its own reference counting.
> 
> Fix that by dropping the gem object reference once the ttm mmap
> completed (and ttm refcount got bumped).
> 
> For that to work properly the drm_gem_object_get() call in
> drm_gem_ttm_mmap() must be moved so it happens before calling
> obj->funcs->mmap(), otherwise the gem refcount would go down
> to zero.
> 
> Fixes: 231927d939f0 ("drm/ttm: add drm_gem_ttm_mmap()")
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Tested-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>  drivers/gpu/drm/drm_gem.c            | 24 ++++++++++++++----------
>  drivers/gpu/drm/drm_gem_ttm_helper.c | 13 ++++++++++++-
>  2 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2f2b889096b0..000fa4a1899f 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1105,21 +1105,33 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  	if (obj_size < vma->vm_end - vma->vm_start)
>  		return -EINVAL;
>  
> +	/* Take a ref for this mapping of the object, so that the fault
> +	 * handler can dereference the mmap offset's pointer to the object.
> +	 * This reference is cleaned up by the corresponding vm_close
> +	 * (which should happen whether the vma was created by this call, or
> +	 * by a vm_open due to mremap or partial unmap or whatever).
> +	 */
> +	drm_gem_object_get(obj);
> +
>  	if (obj->funcs && obj->funcs->mmap) {
>  		/* Remove the fake offset */
>  		vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>  
>  		ret = obj->funcs->mmap(obj, vma);
> -		if (ret)
> +		if (ret) {
> +			drm_gem_object_put_unlocked(obj);
>  			return ret;
> +		}
>  		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
>  	} else {
>  		if (obj->funcs && obj->funcs->vm_ops)
>  			vma->vm_ops = obj->funcs->vm_ops;
>  		else if (dev->driver->gem_vm_ops)
>  			vma->vm_ops = dev->driver->gem_vm_ops;
> -		else
> +		else {
> +			drm_gem_object_put_unlocked(obj);
>  			return -EINVAL;
> +		}
>  
>  		vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
>  		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> @@ -1128,14 +1140,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  
>  	vma->vm_private_data = obj;
>  
> -	/* Take a ref for this mapping of the object, so that the fault
> -	 * handler can dereference the mmap offset's pointer to the object.
> -	 * This reference is cleaned up by the corresponding vm_close
> -	 * (which should happen whether the vma was created by this call, or
> -	 * by a vm_open due to mremap or partial unmap or whatever).
> -	 */
> -	drm_gem_object_get(obj);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_mmap_obj);
> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
> index 7412bfc5c05a..605a8a3da7f9 100644
> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> @@ -64,8 +64,19 @@ int drm_gem_ttm_mmap(struct drm_gem_object *gem,
>  		     struct vm_area_struct *vma)
>  {
>  	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
> +	int ret;
>  
> -	return ttm_bo_mmap_obj(vma, bo);
> +	ret = ttm_bo_mmap_obj(vma, bo);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * ttm has its own object refcounting, so drop gem reference
> +	 * to avoid double accounting counting.
> +	 */
> +	drm_gem_object_put_unlocked(gem);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_ttm_mmap);
>  
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel