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