Re: [PATCH v3 3/6] drm/amdgpu/aca: Fix inverted validation logic and list cleanup

[sashiko-bot@kernel.org]

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

New issues:
- [High] Using `list_del_init()` on a locklessly-iterated list causes a potential kernel infinite loop if a handle is removed concurrently with list traversal.

Pre-existing issues:
- [High] A TOCTOU race condition exists between background RAS querying and device teardown, leading to a use-after-free of the ACA error cache and its mutex.
--

--- Patch [3]: [PATCH v3 3/6] drm/amdgpu/aca: Fix inverted validation logic and list cleanup ---
commit 0f8631e3d7835b4beff6c092382f39cf69067a82
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

drm/amdgpu/aca: Fix inverted validation logic and list cleanup

This commit fixes the validation logic in aca_handle_is_valid() by ensuring
the handle is non-NULL, contains a valid mask, and is actively registered
in the list. It also updates amdgpu_aca_get_error_data() to return 0 for
invalid handles, and modifies remove_aca_handle() to use list_del_init()
to prevent potential use-after-free issues.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> index c76664af9902e..06cb3ad1ce623 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
[ ... ]
> @@ -717,7 +714,7 @@ static void remove_aca_handle(struct aca_handle *handle)
>  	struct aca_handle_manager *mgr = handle->mgr;
>  
>  	aca_fini_error_cache(handle);
> -	list_del(&handle->node);
> +	list_del_init(&handle->node);

[Severity: High]
This is a pre-existing issue, but does the ordering in this function create
a race condition with the background RAS query thread?

If the background thread evaluates aca_handle_is_valid() concurrently
during teardown, it might see a valid node because the background work is
cancelled after the handles are torn down:

drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:amdgpu_ras_fini() {
	...
	amdgpu_aca_fini(adev);
	...
	cancel_delayed_work_sync(&con->ras_counte_delay_work);
	...
}

Because aca_fini_error_cache() is called before the node is removed from
the list, could the background thread see a valid node and then attempt to
acquire a destroyed mutex here?

drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:aca_log_aca_error() {
	...
	mutex_lock(&aerr->lock);
	...
}

[Severity: High]
Does switching to list_del_init() here create a risk of an infinite loop
during list traversal?

The handle list appears to be iterated locklessly here:

drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:aca_dispatch_bank() {
	...
	list_for_each_entry(handle, &mgr->list, node) {
	...
}

If a handle is removed concurrently with list traversal, list_del_init()
will set the node's next pointer to itself. Will this cause the lockless
iterator in aca_dispatch_bank() to get stuck repeatedly visiting the same
removed node?

>  	mgr->nr_handles--;
>  }

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