Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Low] The added NULL check for the `banks` parameter is dead code and introduces unnecessary branching because the parameter is structurally guaranteed to never be NULL. - [Low] The `lock` member referenced in `remove_aca_handle()` does not exist in `struct aca_handle_manager`, resulting in a compilation failure. Pre-existing issues: - [High] Concurrent traversal and modification of `mgr->list` without holding `mgr->lock` on the reader side causes a severe data race. -- --- Patch [4]: [PATCH v4 4/6] drm/amdgpu/aca: Add missing NULL check for banks parameter in aca_banks_add_bank --- commit 613bad660f79f7955828e89d88180e64def84678 Author: Sreeraj S Kurup <[email protected]> drm/amdgpu/aca: Add missing NULL check for banks parameter in aca_banks_add_bank aca_banks_add_bank() verified that the 'bank' parameter was non-NULL, but passed 'banks' directly into list_add_tail(&node->node, &banks->list) and incremented 'banks->nr_banks' without validating whether 'banks' was NULL. Add a NULL check for 'banks' to prevent a kernel NULL pointer dereference if an invalid pointer is passed by a caller. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c > index c35f6be2ef774..dcddeb3816285 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c > @@ -49,7 +49,7 @@ static int aca_banks_add_bank(struct aca_banks *banks, > struct aca_bank *bank) > { > struct aca_bank_node *node; > > - if (!bank) > + if (!banks || !bank) [Severity: Low] Is this NULL check for banks necessary? The parameter banks in aca_banks_add_bank() is always the address of a local stack variable (&banks or &de_banks) in all of its calling paths (e.g., from aca_banks_update() and aca_banks_generate_cper()), which means it can never be NULL. Does this introduce unnecessary dead code and branching? > return -EINVAL; > > node = kvzalloc_obj(*node); [ ... ] There are also a couple of other issues in this area related to the aca handle list: [Severity: Low] Can this code compile successfully? The lock member referenced in remove_aca_handle() does not exist in struct aca_handle_manager: drivers/gpu/drm/amd/amdgpu/amdgpu_aca.h: struct aca_handle_manager { struct list_head list; int nr_handles; }; Attempting to reference mgr->lock here: drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:remove_aca_handle() { ... mutex_lock(&mgr->lock); list_del_init(&handle->node); mgr->nr_handles--; mutex_unlock(&mgr->lock); ... } appears to result in a compilation failure since lock is not defined in the struct. [Severity: High] This isn't a bug introduced by this patch, but there is an explicit attempt to fix it here. Could this concurrent traversal cause a data race? While the writer side in remove_aca_handle() attempts to use mgr->lock: drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:remove_aca_handle() { ... mutex_lock(&mgr->lock); list_del_init(&handle->node); mgr->nr_handles--; mutex_unlock(&mgr->lock); ... } The reader side traverses mgr->list without holding any lock or using RCU primitives: drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:aca_dispatch_bank() { ... if (list_empty(&mgr->list)) return 0; list_for_each_entry(handle, &mgr->list, node) { if (!aca_bank_is_valid(handle, bank, type)) continue; ... } If a background RAS query or sysfs read (which calls aca_dispatch_bank()) traverses mgr->list while remove_aca_handle() concurrently unlinks and frees a node during teardown, could this lead to a use-after-free or list corruption and a kernel panic? The fix appears incomplete because it only syntactically locks the writer side without adding synchronization to the reader side. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
