Re: [PATCH v2 3/5] drm/amdgpu/aca: Fix inverted validation logic and handle cleanup

[sashiko-bot@kernel.org]

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

New issues:
- [High] The logic change in `amdgpu_aca_get_error_data()` causes global RAS error count queries to fail with `-EOPNOTSUPP` for non-ACA blocks.

Pre-existing issues:
- [High] Delayed work `ras_counte_delay_work` is cancelled after ACA subsystems are torn down, leading to a TOCTOU race condition and use-after-destroy of a mutex.
--

--- Patch [3]: [PATCH v2 3/5] drm/amdgpu/aca: Fix inverted validation logic and handle cleanup ---
commit e884a156d93ba3896781b055b3f77054754fcf85
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

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

This commit fixes the validation logic in aca_handle_is_valid() and its
caller amdgpu_aca_get_error_data() to properly process valid handles. It
also changes remove_aca_handle() to use list_del_init() to prevent a
potential use-after-free during device teardown.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> index c76664af9902e..b4c1438a56d8c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
[ ... ]
> @@ -599,7 +596,7 @@ 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))
> +	if (!aca_handle_is_valid(handle))
>  		return -EOPNOTSUPP;
[Severity: High]
Does this logic change cause global RAS error count queries to fail for
non-ACA blocks?

Before this patch, zero-initialized handles (where handle->mask is 0) would
cause aca_handle_is_valid() to return false. Since the check here wasn't
negated, the code would bypass the -EOPNOTSUPP return and fall through to:

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

which gracefully returned 0.

With the new negated check, zero-initialized handles for non-ACA blocks (like
HDP) evaluate as invalid and immediately return -EOPNOTSUPP.

Could this overwrite the return value in the loop within
amdgpu_ras_query_error_count() when iterating over RAS blocks?

amdgpu_ras.c:amdgpu_ras_query_error_count() {
    ...
    list_for_each_entry(obj, &con->head, node) {
        ...
        ret = amdgpu_ras_query_error_count_helper(adev, &ce, &ue, &info);
    }
}

If a non-ACA block is queried at the end of the loop, this -EOPNOTSUPP could
propagate up, completely breaking global RAS telemetry and preventing cached
counters from updating.

>  
>  	if ((type < 0) || (!(BIT(type) & handle->mask)))
>  		return  0;
[ ... ]
> @@ -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 this still leave a time-of-check to
time-of-use race condition during device teardown?

Looking at amdgpu_ras_fini(), the delayed work is cancelled after the ACA
subsystems are torn down:

amdgpu_ras.c:amdgpu_ras_fini() {
    ...
    amdgpu_aca_fini();
    ...
    cancel_delayed_work_sync(&con->ras_counte_delay_work);
}

If the delayed work runs concurrently, could it perform the lockless
!list_empty(&handle->node) check in aca_handle_is_valid() and evaluate to
true just before this teardown thread calls remove_aca_handle()?

If remove_aca_handle() then unlinks the node and destroys the handle's mutex,
the worker thread might proceed into __aca_get_error_data() and attempt to
lock a destroyed mutex.

>  	mgr->nr_handles--;
>  }
>

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