Re: [PATCH v4 4/6] drm/amdgpu/aca: Add missing NULL check for banks parameter in aca_banks_add_bank

[sashiko-bot@kernel.org]

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 <sreekuttan2156239@gmail.com>

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/20260809084735.9743-1-sreekuttan2156239@gmail.com?part=4