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