Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] A TOCTOU race in sysfs/debugfs handlers can lead to a NULL pointer dereference because `con->eh_data` is freed in `amdgpu_ras_recovery_fini()` before the sysfs and debugfs interfaces are removed. - [High] The delayed work `ras_counte_delay_work` can iterate over the `con->head` list while it is being concurrently modified without locks by `amdgpu_ras_disable_all_features()`. - [High] `amdgpu_ras_critical_region_init()` is incorrectly called inside the teardown function `amdgpu_ras_recovery_fini()`, rendering the feature non-functional and needlessly allocating memory that is immediately freed. - [High] The MCA notifier is unregistered at the end of `amdgpu_ras_recovery_fini`, after locks are destroyed and data is freed, allowing use-after-free and locking violations. -- --- Patch [4]: [PATCH v5 4/4] drm/amdgpu/ras: Fix delayed work cancellation order during teardown --- commit 1c0d53c82e9e39018aa03ebc14a76ad3fb030607 Author: Sreeraj S Kurup <[email protected]> drm/amdgpu/ras: Fix delayed work cancellation order during teardown In amdgpu_ras_recovery_fini(), mutex_destroy(&con->page_rsv_lock) was called prior to cancel_work_sync(&con->recovery_work) and cancel_delayed_work_sync(&con->page_retirement_dwork). If pending work ran during teardown, it could attempt to acquire page_rsv_lock after its destruction, leading to a locking violation and potential use-after-free. Fix this by moving cancel_work_sync(&con->recovery_work) and cancel_delayed_work_sync(&con->page_retirement_dwork) before mutex_destroy(&con->page_rsv_lock), ensuring both work items are synchronously canceled while the lock remains intact. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c > index 764cd49504083..1d6b4300d80aa 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c > @@ -3994,15 +3994,14 @@ static int amdgpu_ras_recovery_fini(struct > amdgpu_device *adev) > if (con->page_retirement_thread) > kthread_stop(con->page_retirement_thread); > > + cancel_work_sync(&con->recovery_work); > + cancel_delayed_work_sync(&con->page_retirement_dwork); > + > atomic_set(&con->page_retirement_req_cnt, 0); > atomic_set(&con->poison_creation_count, 0); > > mutex_destroy(&con->page_rsv_lock); > > - cancel_work_sync(&con->recovery_work); > - > - cancel_delayed_work_sync(&con->page_retirement_dwork); > - > amdgpu_ras_ecc_log_fini(&con->umc_ecc_log); > > mutex_lock(&con->recovery_lock); [Severity: High] This isn't a bug introduced by this patch, but could this lead to a NULL pointer dereference during driver unload? amdgpu_ras_recovery_fini() sets con->eh_data to NULL and frees it just after this lock is acquired. However, sysfs interfaces are not removed until amdgpu_ras_fs_fini() is called later. If a concurrent read in amdgpu_ras_badpages_read() checks con->eh_data outside the lock, blocks on recovery_lock, and resumes after the teardown function has freed it: amdgpu_ras_badpages_read() { ... mutex_lock(&con->recovery_lock); data = con->eh_data; if (start < data->count) { ... } It will dereference the now-NULL con->eh_data when evaluating start < data->count. [Severity: High] This is a pre-existing issue, but does the unregistration of the MCA notifier happen too late? Right after this diff context, amdgpu_ras_recovery_fini() unregisters the MCA notifier via amdgpu_unregister_bad_pages_mca_notifier(). This happens after con->page_rsv_lock is destroyed and con->eh_data is freed. If a hardware Machine Check Exception (MCE) occurs during this window, amdgpu_bad_page_notifier() executes and calls amdgpu_ras_add_bad_pages(), which attempts to acquire the already-destroyed con->page_rsv_lock in amdgpu_ras_reserve_page(): amdgpu_ras_reserve_page() { ... mutex_lock(&con->page_rsv_lock); ... } [Severity: High] This isn't a bug introduced by this patch, but is it intentional that amdgpu_ras_critical_region_init() is called during teardown? Just after the diff context in amdgpu_ras_recovery_fini(), we have: amdgpu_ras_critical_region_init(adev); #ifdef CONFIG_X86_MCE_AMD amdgpu_unregister_bad_pages_mca_notifier(adev); #endif This function allocates memory for a critical region and adds it to con->critical_region_head. Because it is called during teardown, the critical region list remains empty during normal operation, causing amdgpu_ras_check_critical_address() to always return false. When the driver unloads, the memory is pointlessly allocated and then freed shortly after. [Severity: High] This is a pre-existing issue, but could ras_counte_delay_work cause a use-after-free or list corruption during driver teardown? During teardown, amdgpu_ras_disable_all_features() locklessly iterates over con->head using list_for_each_entry_safe() and removes nodes. However, ras_counte_delay_work is not synchronously canceled until after this occurs in amdgpu_ras_fini(). If the delayed work executes concurrently, it calls amdgpu_ras_query_error_count(), which locklessly iterates over con->head: amdgpu_ras_query_error_count() { ... list_for_each_entry(obj, &con->head, node) { ... } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
