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 <[email protected]> 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/[email protected]?part=3
