Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] `aca_handle_is_valid()` incorrectly evaluates removed handles as valid, leading to a Use-After-Free and `mutex_lock` on a destroyed mutex during device teardown. --
--- Patch [3]: [PATCH 3/5] drm/amdgpu/aca: Fix inverted validation logic in amdgpu_aca_get_error_data --- commit c1268f69334da0d7e73d9ad9988d74c06033118e Author: Sreeraj S Kurup <[email protected]> drm/amdgpu/aca: Fix inverted validation logic in amdgpu_aca_get_error_data This commit updates aca_handle_is_valid() to verify handles have non-empty list nodes, and negates the check in amdgpu_aca_get_error_data() to process valid handles properly. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c > index c76664af9902e..1cddacb7c554c 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); [Severity: High] Does this change cause removed handles to be evaluated as valid? When amdgpu_aca_fini() removes handles, it uses list_del() rather than list_del_init(): drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:remove_aca_handle() { ... list_del(&handle->node); ... } Because list_del() poisons the list node, list_empty(&handle->node) will evaluate to false. With this patch, !list_empty(&handle->node) now evaluates to true, causing aca_handle_is_valid() to return true for a removed handle. The original code correctly returned false for poisoned nodes. During device teardown or module unload, if the delayed work amdgpu_ras_counte_dw() executes concurrently, it calls amdgpu_aca_get_error_data(). Since the removed handle is now incorrectly deemed valid, the code would proceed into __aca_get_error_data() and attempt to call mutex_lock(&aerr->lock) on an already destroyed mutex, potentially causing a use-after-free or kernel panic. Should remove_aca_handle() use list_del_init() so that list_empty() can safely identify removed handles? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
