Re: [PATCH v6.1] drm/amdkfd: amdkfd_free_gtt_mem clear the correct pointer

[Felix Kuehling <felix.kuehling@amd.com>]

On 2024-11-13 07:13, Christian König wrote:
> Am 13.11.24 um 13:10 schrieb Vamsi Krishna Brahmajosyula:
>> From: Philip Yang <Philip.Yang@amd.com>
>>
>> [ Upstream commit c86ad39140bbcb9dc75a10046c2221f657e8083b ]
>>
>> Pass pointer reference to amdgpu_bo_unref to clear the correct pointer,
>> otherwise amdgpu_bo_unref clear the local variable, the original pointer
>> not set to NULL, this could cause use-after-free bug.
>>
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>
>> Acked-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>> Signed-off-by: Vamsi Krishna Brahmajosyula 
>> <vamsi-krishna.brahmajosyula@broadcom.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c         | 14 +++++++-------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h         |  2 +-
>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c           |  2 +-
>>   drivers/gpu/drm/amd/amdkfd/kfd_device.c            |  4 ++--
>>   .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c  |  2 +-
>>   drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c       |  2 +-
>>   drivers/gpu/drm/amd/amdkfd/kfd_process.c           |  2 +-
>>   .../gpu/drm/amd/amdkfd/kfd_process_queue_manager.c |  4 ++--
>>   8 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> index 5d9a34601a1a..c31e5f9d63da 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> @@ -344,15 +344,15 @@ int amdgpu_amdkfd_alloc_gtt_mem(struct 
>> amdgpu_device *adev, size_t size,
>>       return r;
>>   }
>>   -void amdgpu_amdkfd_free_gtt_mem(struct amdgpu_device *adev, void 
>> *mem_obj)
>> +void amdgpu_amdkfd_free_gtt_mem(struct amdgpu_device *adev, void 
>> **mem_obj)
>>   {
>> -    struct amdgpu_bo *bo = (struct amdgpu_bo *) mem_obj;
>> +    struct amdgpu_bo **bo = (struct amdgpu_bo **) mem_obj;
>>   -    amdgpu_bo_reserve(bo, true);
>> -    amdgpu_bo_kunmap(bo);
>> -    amdgpu_bo_unpin(bo);
>> -    amdgpu_bo_unreserve(bo);
>> -    amdgpu_bo_unref(&(bo));
>> +    amdgpu_bo_reserve(*bo, true);
>> +    amdgpu_bo_kunmap(*bo);
>> +    amdgpu_bo_unpin(*bo);
>> +    amdgpu_bo_unreserve(*bo);
>> +    amdgpu_bo_unref(bo);
>>   }
>>     int amdgpu_amdkfd_alloc_gws(struct amdgpu_device *adev, size_t size,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> index 4b694886715c..c7672a1d1560 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> @@ -210,7 +210,7 @@ int amdgpu_amdkfd_evict_userptr(struct kgd_mem 
>> *mem, struct mm_struct *mm)
>>   int amdgpu_amdkfd_alloc_gtt_mem(struct amdgpu_device *adev, size_t 
>> size,
>>                   void **mem_obj, uint64_t *gpu_addr,
>>                   void **cpu_ptr, bool mqd_gfx9);
>> -void amdgpu_amdkfd_free_gtt_mem(struct amdgpu_device *adev, void 
>> *mem_obj);
>> +void amdgpu_amdkfd_free_gtt_mem(struct amdgpu_device *adev, void 
>> **mem_obj);
>
> Why is that a pointer to a void* in the first place? It looks like all 
> callers should work with an amdgpu_bo object as well.

Historical reasons. When KFD was a separate module, mem_obj was opaque 
to KFD, so the call from KFD to KGD used a void*.

Regards,
   Felix


>
> Regards,
> Christian.
>
>>   int amdgpu_amdkfd_alloc_gws(struct amdgpu_device *adev, size_t size,
>>                   void **mem_obj);
>>   void amdgpu_amdkfd_free_gws(struct amdgpu_device *adev, void 
>> *mem_obj);
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> index e3cd66c4d95d..f83574107eb8 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> @@ -408,7 +408,7 @@ static int kfd_ioctl_create_queue(struct file 
>> *filep, struct kfd_process *p,
>>     err_create_queue:
>>       if (wptr_bo)
>> -        amdgpu_amdkfd_free_gtt_mem(dev->adev, wptr_bo);
>> +        amdgpu_amdkfd_free_gtt_mem(dev->adev, (void **)&wptr_bo);
>>   err_wptr_map_gart:
>>   err_alloc_doorbells:
>>   err_bind_process:
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> index 27820f0a282d..e2c055abfea9 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> @@ -673,7 +673,7 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>>   kfd_doorbell_error:
>>       kfd_gtt_sa_fini(kfd);
>>   kfd_gtt_sa_init_error:
>> -    amdgpu_amdkfd_free_gtt_mem(kfd->adev, kfd->gtt_mem);
>> +    amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>   alloc_gtt_mem_failure:
>>       if (kfd->gws)
>>           amdgpu_amdkfd_free_gws(kfd->adev, kfd->gws);
>> @@ -693,7 +693,7 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
>>           kfd_doorbell_fini(kfd);
>>           ida_destroy(&kfd->doorbell_ida);
>>           kfd_gtt_sa_fini(kfd);
>> -        amdgpu_amdkfd_free_gtt_mem(kfd->adev, kfd->gtt_mem);
>> +        amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>           if (kfd->gws)
>>               amdgpu_amdkfd_free_gws(kfd->adev, kfd->gws);
>>       }
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> index 1b7b29426480..3ab0a796af06 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> @@ -2392,7 +2392,7 @@ static void deallocate_hiq_sdma_mqd(struct 
>> kfd_dev *dev,
>>   {
>>       WARN(!mqd, "No hiq sdma mqd trunk to free");
>>   -    amdgpu_amdkfd_free_gtt_mem(dev->adev, mqd->gtt_mem);
>> +    amdgpu_amdkfd_free_gtt_mem(dev->adev, &mqd->gtt_mem);
>>   }
>>     void device_queue_manager_uninit(struct device_queue_manager *dqm)
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
>> index 623ccd227b7d..c733d6888c30 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
>> @@ -204,7 +204,7 @@ void kfd_free_mqd_cp(struct mqd_manager *mm, void 
>> *mqd,
>>             struct kfd_mem_obj *mqd_mem_obj)
>>   {
>>       if (mqd_mem_obj->gtt_mem) {
>> -        amdgpu_amdkfd_free_gtt_mem(mm->dev->adev, 
>> mqd_mem_obj->gtt_mem);
>> +        amdgpu_amdkfd_free_gtt_mem(mm->dev->adev, 
>> &mqd_mem_obj->gtt_mem);
>>           kfree(mqd_mem_obj);
>>       } else {
>>           kfd_gtt_sa_free(mm->dev, mqd_mem_obj);
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> index 5bca6abd55ae..9582c9449fff 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> @@ -1052,7 +1052,7 @@ static void kfd_process_destroy_pdds(struct 
>> kfd_process *p)
>>             if (pdd->dev->shared_resources.enable_mes)
>>               amdgpu_amdkfd_free_gtt_mem(pdd->dev->adev,
>> -                           pdd->proc_ctx_bo);
>> +                           &pdd->proc_ctx_bo);
>>           /*
>>            * before destroying pdd, make sure to report availability
>>            * for auto suspend
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> index 99aa8a8399d6..1918a3c06ac8 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> @@ -441,9 +441,9 @@ int pqm_destroy_queue(struct 
>> process_queue_manager *pqm, unsigned int qid)
>>             if (dev->shared_resources.enable_mes) {
>>               amdgpu_amdkfd_free_gtt_mem(dev->adev,
>> -                           pqn->q->gang_ctx_bo);
>> +                           &pqn->q->gang_ctx_bo);
>>               if (pqn->q->wptr_bo)
>> -                amdgpu_amdkfd_free_gtt_mem(dev->adev, pqn->q->wptr_bo);
>> +                amdgpu_amdkfd_free_gtt_mem(dev->adev, (void 
>> **)&pqn->q->wptr_bo);
>>             }
>>           uninit_queue(pqn->q);
>