Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Concurrent GPU ring timeouts cause list corruption due to unprotected modification of `adev->reset_list`. -- --- Patch [2]: [PATCH v2 2/5] drm/amdgpu: honor gpu_recovery_allowed in amdgpu_job_timedout --- commit 2bc1f829e1f5f1741410cfa00dfab26ce6099f90 Author: Pierre-Eric Pelloux-Prayer <[email protected]> drm/amdgpu: honor gpu_recovery_allowed in amdgpu_job_timedout This commit introduces a single can_reset variable in amdgpu_job_timedout that takes both the global module parameter and the per-device gpu_recovery_allowed flag into account. It uses this new variable to gate the soft-recovery, per-ring-reset, and IP-block-reset attempts. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > index d97cf1212e0f9..43511e0419a12 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c [ ... ] > @@ -111,7 +112,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct > drm_sched_job *s_job) > if (!amdgpu_sriov_vf(adev)) > amdgpu_job_core_dump(adev, job); [Severity: High] This is a pre-existing issue, but can this lead to list corruption if multiple GPU rings timeout concurrently? When a timeout occurs, the code calls amdgpu_job_core_dump(). If the device does not have an XGMI hive (hive is NULL), amdgpu_job_core_dump() links the shared adev->reset_list into a local stack-allocated list: drivers/gpu/drm/amd/amdgpu/amdgpu_job.c:amdgpu_job_core_dump() { ... if (!amdgpu_sriov_vf(adev) && (adev->gmc.xgmi.num_physical_nodes > 1) && hive) { ... } else { list_add_tail(&adev->reset_list, &device_list); device_list_handle = &device_list; } ... } Since adev->reset_list is shared, if an untrusted userspace application submits hanging shaders on two different rings concurrently, wouldn't both threads race to modify adev->reset_list without any lock protection? This looks like it could lead to wild pointer dereferences during list iteration or when exiting the stack frame on concurrent executions of amdgpu_job_timedout() for the same adev. > - if (amdgpu_gpu_recovery && > + if (can_reset && > amdgpu_ring_is_reset_type_supported(ring, > AMDGPU_RESET_TYPE_SOFT_RECOVERY) && > amdgpu_ring_soft_recovery(ring, job->vmid, s_job->s_fence->parent)) > { > dev_err(adev->dev, "ring %s timeout, but soft recovered\n", -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
