Re: [PATCH v2 3/5] drm/amdgpu: add wedge event implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] NULL pointer dereference in `kfd_profiler_pmc` due to TOCTOU race with asynchronous wedge state.
- [High] Concurrent GPU recovery races with wedge state application in `amdgpu_job_timedout`, leading to PM counter underflow and broken bus master state.
- [High] Top-level ioctl wedge check blocks core DRM cleanup ioctls, leading to kernel resource leaks.
--

--- Patch [3]: [PATCH v2 3/5] drm/amdgpu: add wedge event implementation ---
commit e3bfcebac5b33ee92580699914c3fa3365f9d94f
Author: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Subject: drm/amdgpu: add wedge event implementation

This commit enables the drm wedge framework for amdgpu. When a hang is
detected, the GPU isn't reset but pending fences are signalled and new
work is blocked. Runtime PM is also disabled when the device is wedged.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 0ab380ca7e645..399e935df7b29 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
[ ... ]
> @@ -3006,10 +3006,15 @@ long amdgpu_drm_ioctl(struct file *filp,
>  		      unsigned int cmd, unsigned long arg)
>  {
>  	struct drm_file *file_priv = filp->private_data;
> +	struct amdgpu_device *adev;
>  	struct drm_device *dev;
>  	long ret;
>  
>  	dev = file_priv->minor->dev;
> +	adev = drm_to_adev(dev);
> +	if (amdgpu_device_is_wedged(adev))
> +		return -ENODEV;
> +

[Severity: High]
Does this block core resource cleanup ioctls? By returning -ENODEV here,
it bypasses drm_ioctl() entirely. 

If a Wayland compositor or X server tries to gracefully clean up a client's
resources after a GPU hang, won't standard cleanup interfaces like
DRM_IOCTL_GEM_CLOSE or DRM_IOCTL_SYNCOBJ_DESTROY fail and leak memory in
the kernel?

>  	ret = pm_runtime_get_sync(dev->dev);
>  	if (ret < 0)
>  		goto out;

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 43511e0419a12..41d083646da59 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
[ ... ]
> @@ -185,9 +186,35 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
>  		if (r)
>  			dev_err(adev->dev, "GPU Recovery Failed: %d\n", r);
>  	} else {
> -		drm_sched_suspend_timeout(&ring->sched);
> -		if (amdgpu_sriov_vf(adev))
> +		if (amdgpu_sriov_vf(adev)) {
> +			drm_sched_suspend_timeout(&ring->sched);
>  			adev->virt.tdr_debug = true;
> +		} else {
> +			struct drm_gpu_scheduler *sched;
> +			struct amdgpu_fence *guilty_fence;
> +
> +			/* Declare the device as wedged if it's not already. */
> +			if (!atomic_xchg(&adev->wedge_status, 1)) {
> +				pm_runtime_get_sync(adev->dev);
> +
> +				pci_clear_master(adev->pdev);
> +

[Severity: High]
Can this race with concurrent GPU recovery since it is not holding the
adev->reset_domain->sem lock?

If a user manually triggers a GPU reset concurrently, the reset thread in
amdgpu_device_gpu_recover() could clear wedge_status to 0 and call
pm_runtime_put_autosuspend(). 

If this happens while amdgpu_job_timedout() is preempted before executing
pm_runtime_get_sync(), will the PM usage count underflow? 

Furthermore, when amdgpu_job_timedout() resumes, won't it execute
pci_clear_master() on the newly-recovered healthy GPU, breaking its state?

> +				drm_dev_wedged_event(&adev->ddev, DRM_WEDGE_RECOVERY_REBIND |
> +						DRM_WEDGE_RECOVERY_BUS_RESET, NULL);
> +			}
> +
> +			guilty_fence = to_amdgpu_job(s_job)->hw_fence;

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 0a7c1900da959..544dc960833b2 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
[ ... ]
> @@ -2433,8 +2433,12 @@ struct kfd_process_device *kfd_process_device_data_by_id(struct kfd_process *p,
>  		for (i = 0; i < p->n_pdds; i++) {
>  			struct kfd_process_device *pdd = p->pdds[i];
>  
> -			if (pdd->user_gpu_id == gpu_id)
> +			if (pdd->user_gpu_id == gpu_id) {
> +				if (amdgpu_device_is_wedged(pdd->dev->adev))
> +					return NULL;
> +

[Severity: High]
Does returning NULL dynamically here introduce a TOCTOU race leading to a
NULL pointer dereference?

Looking at kfd_profiler_pmc() in kfd_chardev.c, it acquires the pdd pointer
and immediately dereferences it without checking for NULL:

drivers/gpu/drm/amd/amdkfd/kfd_chardev.c:kfd_profiler_pmc() {
    ...
    mutex_lock(&p->mutex);
    pdd = kfd_process_device_data_by_id(p, args->gpu_id);
    dqm = pdd->dev->dqm;
    mutex_unlock(&p->mutex);
    ...
}

Since the device can enter the wedged state asynchronously on a job timeout,
could this second call to kfd_process_device_data_by_id() return NULL and
cause a kernel crash when pdd->dev is accessed?

Additionally, does this abort unmapping loops and leak mappings on healthy
GPUs?

In kfd_ioctl_unmap_memory_from_gpu(), if kfd_process_device_data_by_id()
returns NULL for a wedged GPU, the loop aborts and goes to 
get_mem_obj_from_handle_failed, leaking mappings on any subsequent healthy
GPUs in the array.

>  				return pdd;
> +			}
>  		}
>  	}
>  	return NULL;

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