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