Re: [PATCH v4 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 3 potential issue(s) to consider:

New issues:
- [High] Invalid memory free causing SLUB corruption.
- [Low] Build failure due to missing `lock` member in `struct aca_handle_manager`.

Pre-existing issues:
- [High] TOCTOU race in `amdgpu_aca_get_error_data` and lockless list traversal in `aca_dispatch_bank`.
--

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

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

This patch fixes inverted logic in aca_handle_is_valid() so that registered
handles evaluate as valid. It also modifies remove_aca_handle() to unlink 
nodes under a lock and explicitly free the handle.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> index c76664af9902e..c35f6be2ef774 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> @@ -586,10 +586,7 @@ static int __aca_get_error_data(struct amdgpu_device *adev, struct aca_handle *h
>  
>  static bool aca_handle_is_valid(struct aca_handle *handle)
>  {
> -	if (!handle->mask || !list_empty(&handle->node))
> -		return false;
> -
> -	return true;
> +	return handle && handle->mask && !list_empty(&handle->node);
>  }
>  
>  int amdgpu_aca_get_error_data(struct amdgpu_device *adev, struct aca_handle *handle,
> @@ -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]
This is a pre-existing issue, but does adding the lock in remove_aca_handle()
actually prevent background queries from referencing destroyed mutexes?

The commit message notes that unlinking under a lock stops background queries
from referencing destroyed error cache mutexes. However, since
amdgpu_aca_get_error_data() still calls aca_handle_is_valid() locklessly, a
background thread can evaluate !list_empty(&handle->node) to true and get
preempted. 

Concurrent driver teardown in remove_aca_handle() can then unlink the handle
and destroy its mutexes. When the background thread resumes and enters
__aca_get_error_data(), won't it still lock a destroyed mutex?

Furthermore, aca_dispatch_bank() iterates mgr->list locklessly, which seems
like it will still race with list modifications in remove_aca_handle().

>  
>  	if ((type < 0) || (!(BIT(type) & handle->mask)))
>  		return  0;

[ ... ]

> @@ -716,9 +713,13 @@ 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);
> +	mutex_lock(&mgr->lock);

[Severity: Low]
Does struct aca_handle_manager actually have a lock field? 

Looking at drivers/gpu/drm/amd/amdgpu/amdgpu_aca.h, the struct only contains
a list head and nr_handles. Attempting to lock mgr->lock appears to cause a
build failure.

> +	list_del_init(&handle->node);
>  	mgr->nr_handles--;
> +	mutex_unlock(&mgr->lock);
> +	aca_fini_error_cache(handle);
> +	kfree(handle);
> +

[Severity: High]
Is it safe to call kfree(handle) here? 
    
Looking at amdgpu_ras_bind_aca() in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c,
the handle is passed as an interior pointer to the embedded aca_handle field
of struct ras_manager:

amdgpu_ras_bind_aca() {
    ...
    return amdgpu_aca_add_handle(adev, &obj->aca_handle, ...);
}

Calling kfree() on an embedded struct rather than a standalone heap allocation
will trigger a SLUB invalid-free kernel panic during driver teardown.

>  }
>  
>  static void remove_aca_sysfs(struct aca_handle *handle)

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