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

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.

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