[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 at 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);
>>    
>>