Re: [PATCH v4 1/6] drm/amdkfd: avoid NULL kfd_process_device in kfd_profiler_pmc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/amdkfd: un-reverted lock state on NULL pdd in
  kfd_profiler_pmc()
- [Low] drm/amdkfd: missing Fixes trailer in kfd_profiler_pmc() commit
--

commit 04f061e589d38a636c5063f9828ed5587bde69f6
Author: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

> drm/amdkfd: avoid NULL kfd_process_device in kfd_profiler_pmc
>
> All other callers of kfd_process_device_data_by_id deal with
> a possible NULL return value except kfd_profiler_pmc.
>
> Check it after the mutex unlock and return -EINVAL as others
> place do.
>
> Reported-by: Sashiko-bot <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20261029152000.22001-1-pierre-eric.pelloux-prayer@amd.com?part=4
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

[Severity: Low]
Since this fixes a NULL pointer dereference in kfd_profiler_pmc(), should
the commit message include a Fixes tag?

  Fixes: a789761de305 ("amd/amdkfd: Add kfd_ioctl_profiler to contain profiler kernel driver changes")

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index d65ae53c81a5c..95921369abb26 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -3463,8 +3463,10 @@ static inline int kfd_profiler_pmc(struct kfd_process *p,
>  	/* Enable/disable perfcount if requested */
>  	mutex_lock(&p->mutex);
>  	pdd = kfd_process_device_data_by_id(p, args->gpu_id);
> -	dqm = pdd->dev->dqm;
>  	mutex_unlock(&p->mutex);
> +	if (!pdd)
> +		return -EINVAL;
> +	dqm = pdd->dev->dqm;
>  
>  	dqm->ops.set_perfcount(dqm, args->perfcount_enable);
>  	return status;

[Severity: Medium]
Does returning -EINVAL here leave the device profiler lock and PTL disable
request leaked when args->lock == 1?

profile_lock_device() has already looked up pdd under p->mutex, set
kfd->profiler_process = p, and called kfd_ptl_disable_request(pdd, p).
Because p->mutex is dropped between the two lookups, a concurrent
criu_restore_devices() call updating pdd->user_gpu_id (or the device
becoming wedged later in this series) can cause this second
kfd_process_device_data_by_id() call to return NULL.  Returning -EINVAL at
that point leaves kfd->profiler_process and the PTL disable state held
when locking, so later profiling attempts fail with -EALREADY or -EBUSY
(or skips updating perfcount on the queues after unlocking when
args->lock == 0).

Looking up pdd once or passing pdd from profile_lock_device() would avoid
the second lookup and the un-reverted state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109171333.4329-1-pierre-eric.pelloux-prayer@amd.com?part=1