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

[Yang, Philip]

On 2019-10-11 1:33 p.m., Kuehling, Felix wrote:
> On 2019-10-11 10:36 a.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.
>>
>> v2: remove unused local variable and amend commit
>>
>> v3: add back get user pages in gem_userptr_ioctl, to detect application
>> bug where an userptr VMA is not ananymous memory and reject it.
>>
>> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962
>>
>> Signed-off-by: Philip Yang <Philip.Yang at amd.com>
>> Tested-by: Joe Barnett <thejoe at gmail.com>
>> Reviewed-by: Christian König <christian.koenig at amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +---
>>    1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index c18a153b3d2a..e7b39daa22f6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -476,7 +476,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);
>> @@ -493,14 +492,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) {
> 
> This if is not needed. kvfree should be able to handle NULL pointers,
> and unconditionally setting the pointer to NULL afterwards is not a
> problem either. With that fixed, this commit is
> 
> Reviewed-by: Felix Kuehling <Felix.Kuehling at amd.com>
> 
> However, I don't think this should be the final solution. My concern
> with this solution is, that you end up freeing and regenerating the
> user_pages arrays more frequently than necessary: On every command
> submission, even if there was no MMU notifier since the last command
> submission. I was hoping we could get back to a solution where we can
> maintain the same user_pages array across command submissions, since MMU
> notifiers are rare. That should reduce overhead from doing all thos page
> table walks in HMM on every command submissions when using userptrs.
> 
Yes, I will have another patch to address this using hmm_range_valid, 
the idea is to allow hmm range tracking cross gem_userptr_ioctl and 
cs_ioctl.

Thanks,
Philip

> Regards,
>     Felix
> 
> 
>>    			kvfree(lobj->user_pages);
>>    			lobj->user_pages = NULL;
>>    		}