Re: [PATCH v4 6/6] drm/amdgpu/ras: Fix delayed work cancellation order during teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Widened race condition allowing delayed work to be re-queued, leading to a Use-After-Free.
--

--- Patch [6]: [PATCH v4 6/6] drm/amdgpu/ras: Fix delayed work cancellation order during teardown ---
commit 0a6b2c827639f23d781a308f6731a9c602763c4a
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

drm/amdgpu/ras: Fix delayed work cancellation order during teardown

This commit reorders the cancellation of delayed work in amdgpu_ras_fini()
and amdgpu_ras_recovery_fini(). It moves cancel_delayed_work_sync() and
cancel_work_sync() before mutex destruction and subsystem teardown to
prevent work handlers from attempting to acquire destroyed locks.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index 764cd49504083..23b9844d9f1ca 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
[ ... ]
> @@ -4751,6 +4750,8 @@ int amdgpu_ras_fini(struct amdgpu_device *adev)
>  	if (!adev->ras_enabled || !con)
>  		return 0;
>  
> +	cancel_delayed_work_sync(&con->ras_counte_delay_work);
> +

[Severity: Critical]
This isn't a bug introduced by this patch, but by moving this
cancel_delayed_work_sync() earlier in amdgpu_ras_fini(), does this widen a race
condition that could lead to a Use-After-Free?

If a userspace process calls the AMDGPU_CTX_OP_QUERY_STATE2 ioctl concurrently
with device teardown, amdgpu_ctx_query2() can asynchronously re-queue the work:

drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c:amdgpu_ctx_query2() {
    ...
    schedule_delayed_work(&con->ras_counte_delay_work,
                          msecs_to_jiffies(AMDGPU_RAS_COUNTE_DELAY_MS));
    ...
}

Because the ioctl lacks drm_dev_enter() synchronization, it can re-arm the work
after this cancellation but before the end of amdgpu_ras_fini().

When kfree(con) is called later in amdgpu_ras_fini(), will the re-armed
ras_counte_delay_work access the freed con structure?

>  	amdgpu_ras_critical_region_fini(adev);
>  	mutex_destroy(&con->critical_region_lock);
>  
[ ... ]
> @@ -4785,8 +4786,6 @@ int amdgpu_ras_fini(struct amdgpu_device *adev)
>  	if (AMDGPU_RAS_GET_FEATURES(con->features))
>  		amdgpu_ras_disable_all_features(adev, 0);
>  
> -	cancel_delayed_work_sync(&con->ras_counte_delay_work);
> -
>  	amdgpu_ras_set_context(adev, NULL);
>  	kfree(con);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809084735.9743-1-sreekuttan2156239@gmail.com?part=6