Re: [PATCH] drm/amdkfd: Dereference null return value

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

On 2024-11-28 11:26, Andrew Martin wrote:
> In the function pqm_uninit there is a call-assignment of "pdd =
> kfd_get_process_device_data" which could be null, and this value was
> later dereferenced without checking.
> 
> Signed-off-by: Andrew Martin <Andrew.Martin@amd.com>

This seems to fix a bug introduced by a previous patch. Please add a Fixes tag for that:

Fixes: fb91065851cd ("drm/amdkfd: Refactor queue wptr_bo GART mapping")

> ---
>  .../drm/amd/amdkfd/kfd_process_queue_manager.c   | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> 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 c76db22a1000..808c447879c0 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> @@ -212,13 +212,21 @@ static void pqm_clean_queue_resource(struct process_queue_manager *pqm,
>  void pqm_uninit(struct process_queue_manager *pqm)
>  {
>  	struct process_queue_node *pqn, *next;
> -	struct kfd_process_device *pdd;
>  
>  	list_for_each_entry_safe(pqn, next, &pqm->queues, process_queue_list) {
>  		if (pqn->q) {
> -			pdd = kfd_get_process_device_data(pqn->q->device, pqm->process);
> -			kfd_queue_unref_bo_vas(pdd, &pqn->q->properties);
> -			kfd_queue_release_buffers(pdd, &pqn->q->properties);
> +			struct kfd_process_device *pdd =
> +				kfd_get_process_device_data(
> +					pqn->q->device,
> +					pqm->process);
> +			if (pdd) {

checkpatch.pl will complain without an empty line after the variable declaration.

> +				kfd_queue_unref_bo_vas(
> +					pdd,
> +					&pqn->q->properties);
> +				kfd_queue_release_buffers(
> +					pdd,
> +					&pqn->q->properties);
> +			}

I think it should be impossible for pdd to be NULL here. Without a PDD, the queue could not have been created in the first place. If we want to add a NULL check here, we should make that a WARN or WARN_ON because it would indicate some serious bug somewhere. The result would be a memory leak here, so we need to warn about that.

Regards,
  Felix

>  			pqm_clean_queue_resource(pqm, pqn);
>  		}
>