dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm: Set vm_ops to GEM object's values during mmap
@ 2021-01-15  8:39 Thomas Zimmermann
  2021-01-15  9:17 ` Kieran Bingham
  0 siblings, 1 reply; 4+ messages in thread
From: Thomas Zimmermann @ 2021-01-15  8:39 UTC (permalink / raw)
  To: daniel, airlied, mripard, maarten.lankhorst, kieran.bingham+renesas
  Cc: Thomas Zimmermann, dri-devel

The GEM mmap code relies on the GEM object's mmap callback to set the
VMA's vm_ops field. This is easily forgotten and lead to a memory leak
in the CMA helpers. Instead set the vm_ops field in the DRM core code
to the GEM object's value. Drivers with different needs can override
this in their mmap callback.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: f5ca8eb6f9bd ("drm/cma-helper: Implement mmap as GEM CMA object functions")
Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Eric Anholt <eric@anholt.net>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_gem.c   | 23 ++++++++++++-----------
 drivers/gpu/drm/drm_prime.c |  4 ++++
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 34b2f111c01c..54d95621fcbb 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1069,27 +1069,28 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 
 	vma->vm_private_data = obj;
 
+	if (!obj->funcs->vm_ops) {
+		ret = -EINVAL;
+		goto err_drm_gem_object_put;
+	}
+	vma->vm_ops = obj->funcs->vm_ops;
+
 	if (obj->funcs->mmap) {
 		ret = obj->funcs->mmap(obj, vma);
-		if (ret) {
-			drm_gem_object_put(obj);
-			return ret;
-		}
+		if (ret)
+			goto err_drm_gem_object_put;
 		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
 	} else {
-		if (obj->funcs->vm_ops)
-			vma->vm_ops = obj->funcs->vm_ops;
-		else {
-			drm_gem_object_put(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));
 		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
 	}
 
 	return 0;
+
+err_drm_gem_object_put:
+	drm_gem_object_put(obj);
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_mmap_obj);
 
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 683aa29ecd3b..0549ec190583 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -717,6 +717,10 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 	vma->vm_pgoff += drm_vma_node_start(&obj->vma_node);
 
 	if (obj->funcs && obj->funcs->mmap) {
+		if (!obj->funcs->vm_ops)
+			return -EINVAL;
+		vma->vm_ops = obj->funcs->vm_ops;
+
 		ret = obj->funcs->mmap(obj, vma);
 		if (ret)
 			return ret;
-- 
2.29.2

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm: Set vm_ops to GEM object's values during mmap
  2021-01-15  8:39 [PATCH] drm: Set vm_ops to GEM object's values during mmap Thomas Zimmermann
@ 2021-01-15  9:17 ` Kieran Bingham
  2021-01-15  9:28   ` Thomas Zimmermann
  0 siblings, 1 reply; 4+ messages in thread
From: Kieran Bingham @ 2021-01-15  9:17 UTC (permalink / raw)
  To: Thomas Zimmermann, daniel, airlied, mripard, maarten.lankhorst; +Cc: dri-devel

Hi Thomas,

On 15/01/2021 08:39, Thomas Zimmermann wrote:
> The GEM mmap code relies on the GEM object's mmap callback to set the
> VMA's vm_ops field. This is easily forgotten and lead to a memory leak

 s/lead/can lead/

> in the CMA helpers. Instead set the vm_ops field in the DRM core code
> to the GEM object's value. Drivers with different needs can override
> this in their mmap callback.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: f5ca8eb6f9bd ("drm/cma-helper: Implement mmap as GEM CMA object functions")
> Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

This also works here.
 - https://paste.ubuntu.com/p/S2Q586rgwT/

Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_gem.c   | 23 ++++++++++++-----------
>  drivers/gpu/drm/drm_prime.c |  4 ++++
>  2 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 34b2f111c01c..54d95621fcbb 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1069,27 +1069,28 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  
>  	vma->vm_private_data = obj;
>  
> +	if (!obj->funcs->vm_ops) {
> +		ret = -EINVAL;
> +		goto err_drm_gem_object_put;
> +	}
> +	vma->vm_ops = obj->funcs->vm_ops;
> +
>  	if (obj->funcs->mmap) {
>  		ret = obj->funcs->mmap(obj, vma);
> -		if (ret) {
> -			drm_gem_object_put(obj);
> -			return ret;
> -		}
> +		if (ret)
> +			goto err_drm_gem_object_put;
>  		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
>  	} else {
> -		if (obj->funcs->vm_ops)
> -			vma->vm_ops = obj->funcs->vm_ops;
> -		else {
> -			drm_gem_object_put(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));
>  		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>  	}
>  
>  	return 0;
> +
> +err_drm_gem_object_put:
> +	drm_gem_object_put(obj);
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_gem_mmap_obj);
>  
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 683aa29ecd3b..0549ec190583 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -717,6 +717,10 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  	vma->vm_pgoff += drm_vma_node_start(&obj->vma_node);
>  
>  	if (obj->funcs && obj->funcs->mmap) {
> +		if (!obj->funcs->vm_ops)
> +			return -EINVAL;
> +		vma->vm_ops = obj->funcs->vm_ops;
> +
>  		ret = obj->funcs->mmap(obj, vma);
>  		if (ret)
>  			return ret;
> 

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm: Set vm_ops to GEM object's values during mmap
  2021-01-15  9:17 ` Kieran Bingham
@ 2021-01-15  9:28   ` Thomas Zimmermann
  2021-01-15  9:32     ` Kieran Bingham
  0 siblings, 1 reply; 4+ messages in thread
From: Thomas Zimmermann @ 2021-01-15  9:28 UTC (permalink / raw)
  To: kieran.bingham+renesas, daniel, airlied, mripard, maarten.lankhorst
  Cc: dri-devel


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

Hi

Am 15.01.21 um 10:17 schrieb Kieran Bingham:
> Hi Thomas,
> 
> On 15/01/2021 08:39, Thomas Zimmermann wrote:
>> The GEM mmap code relies on the GEM object's mmap callback to set the
>> VMA's vm_ops field. This is easily forgotten and lead to a memory leak
> 
>   s/lead/can lead/
> 
>> in the CMA helpers. Instead set the vm_ops field in the DRM core code
>> to the GEM object's value. Drivers with different needs can override
>> this in their mmap callback.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: f5ca8eb6f9bd ("drm/cma-helper: Implement mmap as GEM CMA object functions")
>> Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> This also works here.
>   - https://paste.ubuntu.com/p/S2Q586rgwT/
> 
> Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Great! I have to send out another iteration of the patch, but I think 
the change is such that I can keep the Tested-by.

Best regards
Thomas

> 
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/drm_gem.c   | 23 ++++++++++++-----------
>>   drivers/gpu/drm/drm_prime.c |  4 ++++
>>   2 files changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index 34b2f111c01c..54d95621fcbb 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -1069,27 +1069,28 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>>   
>>   	vma->vm_private_data = obj;
>>   
>> +	if (!obj->funcs->vm_ops) {
>> +		ret = -EINVAL;
>> +		goto err_drm_gem_object_put;
>> +	}
>> +	vma->vm_ops = obj->funcs->vm_ops;
>> +
>>   	if (obj->funcs->mmap) {
>>   		ret = obj->funcs->mmap(obj, vma);
>> -		if (ret) {
>> -			drm_gem_object_put(obj);
>> -			return ret;
>> -		}
>> +		if (ret)
>> +			goto err_drm_gem_object_put;
>>   		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
>>   	} else {
>> -		if (obj->funcs->vm_ops)
>> -			vma->vm_ops = obj->funcs->vm_ops;
>> -		else {
>> -			drm_gem_object_put(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));
>>   		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>>   	}
>>   
>>   	return 0;
>> +
>> +err_drm_gem_object_put:
>> +	drm_gem_object_put(obj);
>> +	return ret;
>>   }
>>   EXPORT_SYMBOL(drm_gem_mmap_obj);
>>   
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 683aa29ecd3b..0549ec190583 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -717,6 +717,10 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>>   	vma->vm_pgoff += drm_vma_node_start(&obj->vma_node);
>>   
>>   	if (obj->funcs && obj->funcs->mmap) {
>> +		if (!obj->funcs->vm_ops)
>> +			return -EINVAL;
>> +		vma->vm_ops = obj->funcs->vm_ops;
>> +
>>   		ret = obj->funcs->mmap(obj, vma);
>>   		if (ret)
>>   			return ret;
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
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: 840 bytes --]

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

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm: Set vm_ops to GEM object's values during mmap
  2021-01-15  9:28   ` Thomas Zimmermann
@ 2021-01-15  9:32     ` Kieran Bingham
  0 siblings, 0 replies; 4+ messages in thread
From: Kieran Bingham @ 2021-01-15  9:32 UTC (permalink / raw)
  To: Thomas Zimmermann, daniel, airlied, mripard, maarten.lankhorst; +Cc: dri-devel

On 15/01/2021 09:28, Thomas Zimmermann wrote:
> Hi
> 
> Am 15.01.21 um 10:17 schrieb Kieran Bingham:
>> Hi Thomas,
>>
>> On 15/01/2021 08:39, Thomas Zimmermann wrote:
>>> The GEM mmap code relies on the GEM object's mmap callback to set the
>>> VMA's vm_ops field. This is easily forgotten and lead to a memory leak
>>
>>   s/lead/can lead/
>>
>>> in the CMA helpers. Instead set the vm_ops field in the DRM core code
>>> to the GEM object's value. Drivers with different needs can override
>>> this in their mmap callback.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Fixes: f5ca8eb6f9bd ("drm/cma-helper: Implement mmap as GEM CMA
>>> object functions")
>>> Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> This also works here.
>>   - https://paste.ubuntu.com/p/S2Q586rgwT/
>>
>> Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Great! I have to send out another iteration of the patch, but I think
> the change is such that I can keep the Tested-by.

I have this test automated now, so I'll kick off a validation run on the
new patch as well.

--
Kieran


> 
> Best regards
> Thomas
> 
>>
>>> Cc: Maxime Ripard <mripard@kernel.org>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: David Airlie <airlied@linux.ie>
>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>> Cc: Eric Anholt <eric@anholt.net>
>>> Cc: dri-devel@lists.freedesktop.org
>>> ---
>>>   drivers/gpu/drm/drm_gem.c   | 23 ++++++++++++-----------
>>>   drivers/gpu/drm/drm_prime.c |  4 ++++
>>>   2 files changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>> index 34b2f111c01c..54d95621fcbb 100644
>>> --- a/drivers/gpu/drm/drm_gem.c
>>> +++ b/drivers/gpu/drm/drm_gem.c
>>> @@ -1069,27 +1069,28 @@ int drm_gem_mmap_obj(struct drm_gem_object
>>> *obj, unsigned long obj_size,
>>>         vma->vm_private_data = obj;
>>>   +    if (!obj->funcs->vm_ops) {
>>> +        ret = -EINVAL;
>>> +        goto err_drm_gem_object_put;
>>> +    }
>>> +    vma->vm_ops = obj->funcs->vm_ops;
>>> +
>>>       if (obj->funcs->mmap) {
>>>           ret = obj->funcs->mmap(obj, vma);
>>> -        if (ret) {
>>> -            drm_gem_object_put(obj);
>>> -            return ret;
>>> -        }
>>> +        if (ret)
>>> +            goto err_drm_gem_object_put;
>>>           WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
>>>       } else {
>>> -        if (obj->funcs->vm_ops)
>>> -            vma->vm_ops = obj->funcs->vm_ops;
>>> -        else {
>>> -            drm_gem_object_put(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));
>>>           vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>>>       }
>>>         return 0;
>>> +
>>> +err_drm_gem_object_put:
>>> +    drm_gem_object_put(obj);
>>> +    return ret;
>>>   }
>>>   EXPORT_SYMBOL(drm_gem_mmap_obj);
>>>   diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>> index 683aa29ecd3b..0549ec190583 100644
>>> --- a/drivers/gpu/drm/drm_prime.c
>>> +++ b/drivers/gpu/drm/drm_prime.c
>>> @@ -717,6 +717,10 @@ int drm_gem_prime_mmap(struct drm_gem_object
>>> *obj, struct vm_area_struct *vma)
>>>       vma->vm_pgoff += drm_vma_node_start(&obj->vma_node);
>>>         if (obj->funcs && obj->funcs->mmap) {
>>> +        if (!obj->funcs->vm_ops)
>>> +            return -EINVAL;
>>> +        vma->vm_ops = obj->funcs->vm_ops;
>>> +
>>>           ret = obj->funcs->mmap(obj, vma);
>>>           if (ret)
>>>               return ret;
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> 

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-15  9:32 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-15  8:39 [PATCH] drm: Set vm_ops to GEM object's values during mmap Thomas Zimmermann
2021-01-15  9:17 ` Kieran Bingham
2021-01-15  9:28   ` Thomas Zimmermann
2021-01-15  9:32     ` Kieran Bingham

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).