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