[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 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) {

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