Re: [PATCH v2 2/5] drm/amdgpu: honor gpu_recovery_allowed in amdgpu_job_timedout

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