Re: [PATCH 2/9] drm/amdkfd: amdkfd_free_gtt_mem clear the correct pointer

[Felix Kuehling <felix.kuehling@xxxxxxx>]

On 2024-07-15 08:34, Philip Yang wrote:
> 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@xxxxxxx>

Reviewed-by: Felix Kuehling <felix.kuehling@xxxxxxx>


> ---
>   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 03205e3c3746..c272461d70a9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -364,15 +364,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 66b1c72c81e5..6e591280774b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -235,7 +235,7 @@ int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
>   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);
>   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 1d9b21628be7..823f245dc7d0 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -423,7 +423,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_bind_process:
>   err_pdd:
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index f4d20adaa068..6619028dd58b 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -907,7 +907,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:
>   	dev_err(kfd_device,
>   		"device %x:%x NOT added due to errors\n",
> @@ -925,7 +925,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);
>   	}
>   
>   	kfree(kfd);
> 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 4f48507418d2..420444eb8e98 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -2621,7 +2621,7 @@ static void deallocate_hiq_sdma_mqd(struct kfd_node *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 50a81da43ce1..d9ae854b6908 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
> @@ -225,7 +225,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 d65974f3b34d..70d6359bb5a3 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -1048,7 +1048,7 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
>   
>   		if (pdd->dev->kfd->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 21f5a1fb3bf8..36f0460cbffe 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> @@ -204,9 +204,9 @@ static void pqm_clean_queue_resource(struct process_queue_manager *pqm,
>   	}
>   
>   	if (dev->kfd->shared_resources.enable_mes) {
> -		amdgpu_amdkfd_free_gtt_mem(dev->adev, pqn->q->gang_ctx_bo);
> +		amdgpu_amdkfd_free_gtt_mem(dev->adev, &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);
>   	}
>   }
>