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

[sashiko-bot@kernel.org]

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 <sreekuttan2156239@gmail.com>

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/20260809144451.35431-1-sreekuttan2156239@gmail.com?part=4