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 <[email protected]> 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/[email protected]?part=3
