Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Using `list_del_init()` on a locklessly-iterated list causes a potential kernel infinite loop if a handle is removed concurrently with list traversal. Pre-existing issues: - [High] A TOCTOU race condition exists between background RAS querying and device teardown, leading to a use-after-free of the ACA error cache and its mutex. -- --- Patch [3]: [PATCH v3 3/6] drm/amdgpu/aca: Fix inverted validation logic and list cleanup --- commit 0f8631e3d7835b4beff6c092382f39cf69067a82 Author: Sreeraj S Kurup <[email protected]> drm/amdgpu/aca: Fix inverted validation logic and list cleanup This commit fixes the validation logic in aca_handle_is_valid() by ensuring the handle is non-NULL, contains a valid mask, and is actively registered in the list. It also updates amdgpu_aca_get_error_data() to return 0 for invalid handles, and modifies remove_aca_handle() to use list_del_init() to prevent potential use-after-free issues. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c > index c76664af9902e..06cb3ad1ce623 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c [ ... ] > @@ -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 the ordering in this function create a race condition with the background RAS query thread? If the background thread evaluates aca_handle_is_valid() concurrently during teardown, it might see a valid node because the background work is cancelled after the handles are torn down: drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:amdgpu_ras_fini() { ... amdgpu_aca_fini(adev); ... cancel_delayed_work_sync(&con->ras_counte_delay_work); ... } Because aca_fini_error_cache() is called before the node is removed from the list, could the background thread see a valid node and then attempt to acquire a destroyed mutex here? drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:aca_log_aca_error() { ... mutex_lock(&aerr->lock); ... } [Severity: High] Does switching to list_del_init() here create a risk of an infinite loop during list traversal? The handle list appears to be iterated locklessly here: drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:aca_dispatch_bank() { ... list_for_each_entry(handle, &mgr->list, node) { ... } If a handle is removed concurrently with list traversal, list_del_init() will set the node's next pointer to itself. Will this cause the lockless iterator in aca_dispatch_bank() to get stuck repeatedly visiting the same removed node? > mgr->nr_handles--; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
