[PATCH] drm/amdgpu: user pages array memory leak fix

[PATCH] drm/amdgpu: user pages array memory leak fix

[Yang, Philip]

user_pages array should always be freed after validation regardless if
user pages are changed after bo is created because with HMM change parse
bo always allocate user pages array to get user pages for userptr bo.

Don't need to get user pages while creating uerptr bo because user pages
will only be used while validating after parsing userptr bo.

Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962

v2: remove unused local variable and amend commit

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  4 +---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +----------------------
 2 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 49b767b7238f..961186e7113e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -474,7 +474,6 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
 
 	list_for_each_entry(lobj, validated, tv.head) {
 		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo);
-		bool binding_userptr = false;
 		struct mm_struct *usermm;
 
 		usermm = amdgpu_ttm_tt_get_usermm(bo->tbo.ttm);
@@ -491,14 +490,13 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
 
 			amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm,
 						     lobj->user_pages);
-			binding_userptr = true;
 		}
 
 		r = amdgpu_cs_validate(p, bo);
 		if (r)
 			return r;
 
-		if (binding_userptr) {
+		if (lobj->user_pages) {
 			kvfree(lobj->user_pages);
 			lobj->user_pages = NULL;
 		}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index a828e3d0bfbd..3ccd61d69964 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -283,7 +283,6 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
 int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
 			     struct drm_file *filp)
 {
-	struct ttm_operation_ctx ctx = { true, false };
 	struct amdgpu_device *adev = dev->dev_private;
 	struct drm_amdgpu_gem_userptr *args = data;
 	struct drm_gem_object *gobj;
@@ -326,32 +325,12 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
 			goto release_object;
 	}
 
-	if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) {
-		r = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
-		if (r)
-			goto release_object;
-
-		r = amdgpu_bo_reserve(bo, true);
-		if (r)
-			goto user_pages_done;
-
-		amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
-		r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
-		amdgpu_bo_unreserve(bo);
-		if (r)
-			goto user_pages_done;
-	}
-
 	r = drm_gem_handle_create(filp, gobj, &handle);
 	if (r)
-		goto user_pages_done;
+		goto release_object;
 
 	args->handle = handle;
 
-user_pages_done:
-	if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE)
-		amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
-
 release_object:
 	drm_gem_object_put_unlocked(gobj);
 
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: user pages array memory leak fix

[Yang, Philip]

Thanks Joe for the test, I will add your Tested-by.

Hi Christian,

May you help review? The change removes the get user pages from 
gem_userptr_ioctl, this was done if flags AMDGPU_GEM_USERPTR_VALIDATE is 
set, and delay the get user pages to amdgpu_cs_parser_bos, and check if 
user pages are invalidated when amdgpu_cs_submit. I don't find issue for 
overnight test, but not sure if there is potential side effect.

Thanks,
Philip

On 2019-10-03 3:44 p.m., Yang, Philip wrote:
> user_pages array should always be freed after validation regardless if
> user pages are changed after bo is created because with HMM change parse
> bo always allocate user pages array to get user pages for userptr bo.
> 
> Don't need to get user pages while creating uerptr bo because user pages
> will only be used while validating after parsing userptr bo.
> 
> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962
> 
> v2: remove unused local variable and amend commit
> 
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  4 +---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +----------------------
>   2 files changed, 2 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 49b767b7238f..961186e7113e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -474,7 +474,6 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
>   
>   	list_for_each_entry(lobj, validated, tv.head) {
>   		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo);
> -		bool binding_userptr = false;
>   		struct mm_struct *usermm;
>   
>   		usermm = amdgpu_ttm_tt_get_usermm(bo->tbo.ttm);
> @@ -491,14 +490,13 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
>   
>   			amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm,
>   						     lobj->user_pages);
> -			binding_userptr = true;
>   		}
>   
>   		r = amdgpu_cs_validate(p, bo);
>   		if (r)
>   			return r;
>   
> -		if (binding_userptr) {
> +		if (lobj->user_pages) {
>   			kvfree(lobj->user_pages);
>   			lobj->user_pages = NULL;
>   		}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index a828e3d0bfbd..3ccd61d69964 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -283,7 +283,6 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>   			     struct drm_file *filp)
>   {
> -	struct ttm_operation_ctx ctx = { true, false };
>   	struct amdgpu_device *adev = dev->dev_private;
>   	struct drm_amdgpu_gem_userptr *args = data;
>   	struct drm_gem_object *gobj;
> @@ -326,32 +325,12 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>   			goto release_object;
>   	}
>   
> -	if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) {
> -		r = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
> -		if (r)
> -			goto release_object;
> -
> -		r = amdgpu_bo_reserve(bo, true);
> -		if (r)
> -			goto user_pages_done;
> -
> -		amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
> -		r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> -		amdgpu_bo_unreserve(bo);
> -		if (r)
> -			goto user_pages_done;
> -	}
> -
>   	r = drm_gem_handle_create(filp, gobj, &handle);
>   	if (r)
> -		goto user_pages_done;
> +		goto release_object;
>   
>   	args->handle = handle;
>   
> -user_pages_done:
> -	if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE)
> -		amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
> -
>   release_object:
>   	drm_gem_object_put_unlocked(gobj);
>   
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: user pages array memory leak fix

[Koenig, Christian]

Hi Philip,

Am 04.10.19 um 15:40 schrieb Yang, Philip:
> Thanks Joe for the test, I will add your Tested-by.
>
> Hi Christian,
>
> May you help review? The change removes the get user pages from
> gem_userptr_ioctl, this was done if flags AMDGPU_GEM_USERPTR_VALIDATE is
> set, and delay the get user pages to amdgpu_cs_parser_bos, and check if
> user pages are invalidated when amdgpu_cs_submit. I don't find issue for
> overnight test, but not sure if there is potential side effect.

Yeah, seen that.

The AMDGPU_GEM_USERPTR_VALIDATE was explicitly added to cause a 
validation during BO creation because of some very stupid applications.

I didn't wanted to reject that without offering an alternative, but we 
seriously can't do this or it would break Vulkan/OpenGL.

I need more time to take a closer look,
Christian.

>
> Thanks,
> Philip
>
> On 2019-10-03 3:44 p.m., Yang, Philip wrote:
>> user_pages array should always be freed after validation regardless if
>> user pages are changed after bo is created because with HMM change parse
>> bo always allocate user pages array to get user pages for userptr bo.
>>
>> Don't need to get user pages while creating uerptr bo because user pages
>> will only be used while validating after parsing userptr bo.
>>
>> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962
>>
>> v2: remove unused local variable and amend commit
>>
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  4 +---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +----------------------
>>    2 files changed, 2 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 49b767b7238f..961186e7113e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -474,7 +474,6 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
>>    
>>    	list_for_each_entry(lobj, validated, tv.head) {
>>    		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo);
>> -		bool binding_userptr = false;
>>    		struct mm_struct *usermm;
>>    
>>    		usermm = amdgpu_ttm_tt_get_usermm(bo->tbo.ttm);
>> @@ -491,14 +490,13 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
>>    
>>    			amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm,
>>    						     lobj->user_pages);
>> -			binding_userptr = true;
>>    		}
>>    
>>    		r = amdgpu_cs_validate(p, bo);
>>    		if (r)
>>    			return r;
>>    
>> -		if (binding_userptr) {
>> +		if (lobj->user_pages) {
>>    			kvfree(lobj->user_pages);
>>    			lobj->user_pages = NULL;
>>    		}
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index a828e3d0bfbd..3ccd61d69964 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -283,7 +283,6 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>    int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>    			     struct drm_file *filp)
>>    {
>> -	struct ttm_operation_ctx ctx = { true, false };
>>    	struct amdgpu_device *adev = dev->dev_private;
>>    	struct drm_amdgpu_gem_userptr *args = data;
>>    	struct drm_gem_object *gobj;
>> @@ -326,32 +325,12 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>    			goto release_object;
>>    	}
>>    
>> -	if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) {
>> -		r = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
>> -		if (r)
>> -			goto release_object;
>> -
>> -		r = amdgpu_bo_reserve(bo, true);
>> -		if (r)
>> -			goto user_pages_done;
>> -
>> -		amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
>> -		r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>> -		amdgpu_bo_unreserve(bo);
>> -		if (r)
>> -			goto user_pages_done;
>> -	}
>> -
>>    	r = drm_gem_handle_create(filp, gobj, &handle);
>>    	if (r)
>> -		goto user_pages_done;
>> +		goto release_object;
>>    
>>    	args->handle = handle;
>>    
>> -user_pages_done:
>> -	if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE)
>> -		amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
>> -
>>    release_object:
>>    	drm_gem_object_put_unlocked(gobj);
>>    
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: user pages array memory leak fix

[Joe Barnett]

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

Thanks for looking at this Christian.  Let me know if there's anything I
can do to help.

In the meantime, is there a workaround to avoid the memory leak other than
using a kernel from before the HMM change?

Thanks,
-Joe

On Fri, Oct 4, 2019 at 8:02 AM Koenig, Christian <Christian.Koenig-5C7GfCeVMHo@public.gmane.org>
wrote:

> Hi Philip,
>
> Am 04.10.19 um 15:40 schrieb Yang, Philip:
> > Thanks Joe for the test, I will add your Tested-by.
> >
> > Hi Christian,
> >
> > May you help review? The change removes the get user pages from
> > gem_userptr_ioctl, this was done if flags AMDGPU_GEM_USERPTR_VALIDATE is
> > set, and delay the get user pages to amdgpu_cs_parser_bos, and check if
> > user pages are invalidated when amdgpu_cs_submit. I don't find issue for
> > overnight test, but not sure if there is potential side effect.
>
> Yeah, seen that.
>
> The AMDGPU_GEM_USERPTR_VALIDATE was explicitly added to cause a
> validation during BO creation because of some very stupid applications.
>
> I didn't wanted to reject that without offering an alternative, but we
> seriously can't do this or it would break Vulkan/OpenGL.
>
> I need more time to take a closer look,
> Christian.
>
> >
> > Thanks,
> > Philip
> >
> > On 2019-10-03 3:44 p.m., Yang, Philip wrote:
> >> user_pages array should always be freed after validation regardless if
> >> user pages are changed after bo is created because with HMM change parse
> >> bo always allocate user pages array to get user pages for userptr bo.
> >>
> >> Don't need to get user pages while creating uerptr bo because user pages
> >> will only be used while validating after parsing userptr bo.
> >>
> >> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962
> >>
> >> v2: remove unused local variable and amend commit
> >>
> >> Signed-off-by: Philip Yang <Philip.Yang-5C7GfCeVMHo@public.gmane.org>
> >> ---
> >>    drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  4 +---
> >>    drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +----------------------
> >>    2 files changed, 2 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> >> index 49b767b7238f..961186e7113e 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> >> @@ -474,7 +474,6 @@ static int amdgpu_cs_list_validate(struct
> amdgpu_cs_parser *p,
> >>
> >>      list_for_each_entry(lobj, validated, tv.head) {
> >>              struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo);
> >> -            bool binding_userptr = false;
> >>              struct mm_struct *usermm;
> >>
> >>              usermm = amdgpu_ttm_tt_get_usermm(bo->tbo.ttm);
> >> @@ -491,14 +490,13 @@ static int amdgpu_cs_list_validate(struct
> amdgpu_cs_parser *p,
> >>
> >>                      amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm,
> >>                                                   lobj->user_pages);
> >> -                    binding_userptr = true;
> >>              }
> >>
> >>              r = amdgpu_cs_validate(p, bo);
> >>              if (r)
> >>                      return r;
> >>
> >> -            if (binding_userptr) {
> >> +            if (lobj->user_pages) {
> >>                      kvfree(lobj->user_pages);
> >>                      lobj->user_pages = NULL;
> >>              }
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >> index a828e3d0bfbd..3ccd61d69964 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >> @@ -283,7 +283,6 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev,
> void *data,
> >>    int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
> >>                           struct drm_file *filp)
> >>    {
> >> -    struct ttm_operation_ctx ctx = { true, false };
> >>      struct amdgpu_device *adev = dev->dev_private;
> >>      struct drm_amdgpu_gem_userptr *args = data;
> >>      struct drm_gem_object *gobj;
> >> @@ -326,32 +325,12 @@ int amdgpu_gem_userptr_ioctl(struct drm_device
> *dev, void *data,
> >>                      goto release_object;
> >>      }
> >>
> >> -    if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) {
> >> -            r = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
> >> -            if (r)
> >> -                    goto release_object;
> >> -
> >> -            r = amdgpu_bo_reserve(bo, true);
> >> -            if (r)
> >> -                    goto user_pages_done;
> >> -
> >> -            amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
> >> -            r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> >> -            amdgpu_bo_unreserve(bo);
> >> -            if (r)
> >> -                    goto user_pages_done;
> >> -    }
> >> -
> >>      r = drm_gem_handle_create(filp, gobj, &handle);
> >>      if (r)
> >> -            goto user_pages_done;
> >> +            goto release_object;
> >>
> >>      args->handle = handle;
> >>
> >> -user_pages_done:
> >> -    if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE)
> >> -            amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
> >> -
> >>    release_object:
> >>      drm_gem_object_put_unlocked(gobj);
> >>
> >>
>
>

[-- Attachment #1.2: Type: text/html, Size: 7283 bytes --]

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: user pages array memory leak fix

[Christian König]

Am 03.10.19 um 21:44 schrieb Yang, Philip:
> user_pages array should always be freed after validation regardless if
> user pages are changed after bo is created because with HMM change parse
> bo always allocate user pages array to get user pages for userptr bo.
>
> Don't need to get user pages while creating uerptr bo because user pages
> will only be used while validating after parsing userptr bo.
>
> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962
>
> v2: remove unused local variable and amend commit
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  4 +---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +----------------------
>   2 files changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 49b767b7238f..961186e7113e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -474,7 +474,6 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
>   
>   	list_for_each_entry(lobj, validated, tv.head) {
>   		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo);
> -		bool binding_userptr = false;
>   		struct mm_struct *usermm;
>   
>   		usermm = amdgpu_ttm_tt_get_usermm(bo->tbo.ttm);
> @@ -491,14 +490,13 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
>   
>   			amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm,
>   						     lobj->user_pages);
> -			binding_userptr = true;
>   		}
>   
>   		r = amdgpu_cs_validate(p, bo);
>   		if (r)
>   			return r;
>   
> -		if (binding_userptr) {
> +		if (lobj->user_pages) {
>   			kvfree(lobj->user_pages);
>   			lobj->user_pages = NULL;
>   		}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index a828e3d0bfbd..3ccd61d69964 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -283,7 +283,6 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>   			     struct drm_file *filp)
>   {
> -	struct ttm_operation_ctx ctx = { true, false };
>   	struct amdgpu_device *adev = dev->dev_private;
>   	struct drm_amdgpu_gem_userptr *args = data;
>   	struct drm_gem_object *gobj;
> @@ -326,32 +325,12 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>   			goto release_object;
>   	}
>   
> -	if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) {

We can't drop that handling here, it is mandatory to detect an 
application bug where an application tries to user an userptr with a VMA 
which is not anonymous memory.

This must be detected and rejected as invalid here.

I suggest that we allocate a local pages array similar to how we do it 
during CS and release that after the function is done.

Regards,
Christian.

> -		r = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
> -		if (r)
> -			goto release_object;
> -
> -		r = amdgpu_bo_reserve(bo, true);
> -		if (r)
> -			goto user_pages_done;
> -
> -		amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
> -		r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> -		amdgpu_bo_unreserve(bo);
> -		if (r)
> -			goto user_pages_done;
> -	}
> -
>   	r = drm_gem_handle_create(filp, gobj, &handle);
>   	if (r)
> -		goto user_pages_done;
> +		goto release_object;
>   
>   	args->handle = handle;
>   
> -user_pages_done:
> -	if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE)
> -		amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
> -
>   release_object:
>   	drm_gem_object_put_unlocked(gobj);
>   

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: user pages array memory leak fix

[Yang, Philip]

On 2019-10-11 4:40 a.m., Christian König wrote:
> Am 03.10.19 um 21:44 schrieb Yang, Philip:
>> user_pages array should always be freed after validation regardless if
>> user pages are changed after bo is created because with HMM change parse
>> bo always allocate user pages array to get user pages for userptr bo.
>>
>> Don't need to get user pages while creating uerptr bo because user pages
>> will only be used while validating after parsing userptr bo.
>>
>> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962
>>
>> v2: remove unused local variable and amend commit
>>
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  4 +---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +----------------------
>>   2 files changed, 2 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 49b767b7238f..961186e7113e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -474,7 +474,6 @@ static int amdgpu_cs_list_validate(struct 
>> amdgpu_cs_parser *p,
>>       list_for_each_entry(lobj, validated, tv.head) {
>>           struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo);
>> -        bool binding_userptr = false;
>>           struct mm_struct *usermm;
>>           usermm = amdgpu_ttm_tt_get_usermm(bo->tbo.ttm);
>> @@ -491,14 +490,13 @@ static int amdgpu_cs_list_validate(struct 
>> amdgpu_cs_parser *p,
>>               amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm,
>>                                lobj->user_pages);
>> -            binding_userptr = true;
>>           }
>>           r = amdgpu_cs_validate(p, bo);
>>           if (r)
>>               return r;
>> -        if (binding_userptr) {
>> +        if (lobj->user_pages) {
>>               kvfree(lobj->user_pages);
>>               lobj->user_pages = NULL;
>>           }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index a828e3d0bfbd..3ccd61d69964 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -283,7 +283,6 @@ int amdgpu_gem_create_ioctl(struct drm_device 
>> *dev, void *data,
>>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>                    struct drm_file *filp)
>>   {
>> -    struct ttm_operation_ctx ctx = { true, false };
>>       struct amdgpu_device *adev = dev->dev_private;
>>       struct drm_amdgpu_gem_userptr *args = data;
>>       struct drm_gem_object *gobj;
>> @@ -326,32 +325,12 @@ int amdgpu_gem_userptr_ioctl(struct drm_device 
>> *dev, void *data,
>>               goto release_object;
>>       }
>> -    if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) {
> 
> We can't drop that handling here, it is mandatory to detect an 
> application bug where an application tries to user an userptr with a VMA 
> which is not anonymous memory.
> 
> This must be detected and rejected as invalid here.
> 
> I suggest that we allocate a local pages array similar to how we do it 
> during CS and release that after the function is done.
> 
Thanks for this, we can use bo->tbo.ttm->pages array here to avoid extra 
alloc/free of pages array because CS uses local pages array and update 
to bo->tbo.ttm->pages if user pages are moved. I will submit patch v3 
for review.

> Regards,
> Christian.
> 
>> -        r = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
>> -        if (r)
>> -            goto release_object;
>> -
>> -        r = amdgpu_bo_reserve(bo, true);
>> -        if (r)
>> -            goto user_pages_done;
>> -
>> -        amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
>> -        r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>> -        amdgpu_bo_unreserve(bo);
>> -        if (r)
>> -            goto user_pages_done;
>> -    }
>> -
>>       r = drm_gem_handle_create(filp, gobj, &handle);
>>       if (r)
>> -        goto user_pages_done;
>> +        goto release_object;
>>       args->handle = handle;
>> -user_pages_done:
>> -    if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE)
>> -        amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
>> -
>>   release_object:
>>       drm_gem_object_put_unlocked(gobj);
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: user pages array memory leak fix

[Joe Barnett]

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

I've tested applying v2 of this patch against a v5.3 tagged kernel and it
appears to fix the issue I reported.

Thanks,
-Joe

On Thu, Oct 3, 2019 at 12:07 PM Yang, Philip <Philip.Yang-5C7GfCeVMHo@public.gmane.org> wrote:

> user_pages array should be freed regardless if user pages are
> invalidated after bo is created because HMM change to always allocate
> user pages array to get user pages while parsing user page bo.
>
> Don't need to to get user pages while creating bo because user pages
> will only be used after parsing user page bo.
>
> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962
>
> Signed-off-by: Philip Yang <Philip.Yang-5C7GfCeVMHo@public.gmane.org>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +----------------------
>  2 files changed, 2 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 49b767b7238f..e861de259def 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -498,7 +498,7 @@ static int amdgpu_cs_list_validate(struct
> amdgpu_cs_parser *p,
>                 if (r)
>                         return r;
>
> -               if (binding_userptr) {
> +               if (lobj->user_pages) {
>                         kvfree(lobj->user_pages);
>                         lobj->user_pages = NULL;
>                 }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index a828e3d0bfbd..3ccd61d69964 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -283,7 +283,6 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev,
> void *data,
>  int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>                              struct drm_file *filp)
>  {
> -       struct ttm_operation_ctx ctx = { true, false };
>         struct amdgpu_device *adev = dev->dev_private;
>         struct drm_amdgpu_gem_userptr *args = data;
>         struct drm_gem_object *gobj;
> @@ -326,32 +325,12 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev,
> void *data,
>                         goto release_object;
>         }
>
> -       if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) {
> -               r = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
> -               if (r)
> -                       goto release_object;
> -
> -               r = amdgpu_bo_reserve(bo, true);
> -               if (r)
> -                       goto user_pages_done;
> -
> -               amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
> -               r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> -               amdgpu_bo_unreserve(bo);
> -               if (r)
> -                       goto user_pages_done;
> -       }
> -
>         r = drm_gem_handle_create(filp, gobj, &handle);
>         if (r)
> -               goto user_pages_done;
> +               goto release_object;
>
>         args->handle = handle;
>
> -user_pages_done:
> -       if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE)
> -               amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
> -
>  release_object:
>         drm_gem_object_put_unlocked(gobj);
>
> --
> 2.17.1
>
>

[-- Attachment #1.2: Type: text/html, Size: 4435 bytes --]

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: user pages array memory leak fix

[Yang, Philip]

user_pages array should be freed regardless if user pages are
invalidated after bo is created because HMM change to always allocate
user pages array to get user pages while parsing user page bo.

Don't need to to get user pages while creating bo because user pages
will only be used after parsing user page bo.

Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +----------------------
 2 files changed, 2 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 49b767b7238f..e861de259def 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -498,7 +498,7 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
 		if (r)
 			return r;
 
-		if (binding_userptr) {
+		if (lobj->user_pages) {
 			kvfree(lobj->user_pages);
 			lobj->user_pages = NULL;
 		}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index a828e3d0bfbd..3ccd61d69964 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -283,7 +283,6 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
 int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
 			     struct drm_file *filp)
 {
-	struct ttm_operation_ctx ctx = { true, false };
 	struct amdgpu_device *adev = dev->dev_private;
 	struct drm_amdgpu_gem_userptr *args = data;
 	struct drm_gem_object *gobj;
@@ -326,32 +325,12 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
 			goto release_object;
 	}
 
-	if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) {
-		r = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
-		if (r)
-			goto release_object;
-
-		r = amdgpu_bo_reserve(bo, true);
-		if (r)
-			goto user_pages_done;
-
-		amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
-		r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
-		amdgpu_bo_unreserve(bo);
-		if (r)
-			goto user_pages_done;
-	}
-
 	r = drm_gem_handle_create(filp, gobj, &handle);
 	if (r)
-		goto user_pages_done;
+		goto release_object;
 
 	args->handle = handle;
 
-user_pages_done:
-	if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE)
-		amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
-
 release_object:
 	drm_gem_object_put_unlocked(gobj);
 
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx