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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Concurrent execution of lockless list traversal and `remove_aca_handle` leads to TOCTOU use-after-free or hard infinite loops due to `list_del_init()`.
--

--- Patch [3]: [PATCH v5 3/4] drm/amdgpu/aca: Fix inverted validation logic and list cleanup ---
commit 2db17afc321c2a877e8d6ebfef5f2cce5ce3142f
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

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

This commit fixes aca_handle_is_valid() to properly check for valid handles 
rather than empty nodes. It also changes remove_aca_handle() to use 
list_del_init() before destroying the error cache, ensuring the handle is 
recognized as unregistered before its resources are freed.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> index c76664af9902e..d48f2c4d6c845 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
[ ... ]
> @@ -599,8 +596,8 @@ int amdgpu_aca_get_error_data(struct amdgpu_device *adev, struct aca_handle *han
>  	if (!handle || !err_data)
>  		return -EINVAL;
>  
> -	if (aca_handle_is_valid(handle))
> -		return -EOPNOTSUPP;
> +	if (!aca_handle_is_valid(handle))
> +		return 0;

[Severity: High]
Does this lockless validity check introduce a time-of-check to time-of-use
(TOCTOU) race window?

During driver teardown, amdgpu_ras_fini() calls amdgpu_aca_fini() (which 
eventually destroys handles and their mutexes) before it cancels 
ras_counte_delay_work.

If the worker calls this function concurrently, it could pass this lockless 
validity check right before remove_aca_handle() destroys the error cache and
its lock. This would result in a use-after-free or an attempt to lock a 
destroyed mutex when the worker attempts to use the handle in 
aca_log_aca_error().

>  	if ((type < 0) || (!(BIT(type) & handle->mask)))
>  		return  0;
[ ... ]
> @@ -716,9 +713,10 @@ 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);
>  	mgr->nr_handles--;
> +	aca_fini_error_cache(handle);
> +
>  }

[Severity: High]
Could the use of list_del_init() here cause an infinite loop for concurrent 
list iterators?

When list_del_init() removes the node, it reinitializes the node by pointing 
its next and prev pointers to itself. 

There are functions like aca_dispatch_bank() that iterate over mgr->list 
locklessly. If a concurrent iterator is on this node exactly when it is 
removed, following the self-referential next pointer will cause the iterator 
to loop on this node infinitely, leading to an RCU stall or hard lockup.

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