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

[Kuehling, Felix]

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.

Regards,
   Felix


>   			kvfree(lobj->user_pages);
>   			lobj->user_pages = NULL;
>   		}