Re: [PATCH 6/8] md/raid5: allocate worker groups per NUMA node

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch incorrectly uses a sequential loop index `i` as the NUMA node ID parameter to `kzalloc_node()`.

Pre-existing issues:
- [High] The `worker_groups` array is sized using `num_possible_nodes()` but is indexed by the raw NUMA node ID, leading to out-of-bounds accesses on systems with sparse NUMA node IDs.
--

commit bce08bf6ce446b00da625ff4028f5782b756e17c
Author: Hiroshi Nishida <[email protected]>

md/raid5: allocate worker groups per NUMA node

alloc_thread_groups() previously allocated all r5worker arrays in a
single kcalloc() block, assigning workers for NUMA node N from node 0
memory.  On multi-socket systems this causes remote memory traffic on
every worker->work and worker->temp_inactive_list access.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 8e9edaaca667b..c8787ab7b309c 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
[ ... ]
> @@ -7336,7 +7340,6 @@ static int alloc_thread_groups(struct r5conf *conf, int cnt, int *group_cnt,
[ ... ]
>  	*group_cnt = num_possible_nodes();
> -	size = sizeof(struct r5worker) * cnt;
> -	workers = kcalloc(size, *group_cnt, GFP_NOIO);
>  	*worker_groups = kzalloc_objs(struct r5worker_group, *group_cnt,
>  				      GFP_NOIO);

[Severity: High]
This isn't a bug introduced by this patch, but could this array sizing cause
an out-of-bounds access on systems with sparse NUMA nodes?

We size the worker_groups array using num_possible_nodes(), which is a count.
On systems with sparse NUMA node IDs (for example, nodes 0 and 8), the array
would only have 2 elements. Later, in raid5_wakeup_stripe_thread(), we index
into this array using cpu_to_group(cpu) which expands to cpu_to_node(cpu).
If a CPU belongs to node 8, we would access conf->worker_groups[8], exceeding
the allocated size and corrupting memory.

> -	if (!*worker_groups || !workers) {
> -		kfree(workers);
> -		kfree(*worker_groups);
> +	if (!*worker_groups)
>  		return -ENOMEM;
> -	}
>  
> +	size = sizeof(struct r5worker) * cnt;
>  	for (i = 0; i < *group_cnt; i++) {
> -		struct r5worker_group *group;
> +		struct r5worker_group *group = &(*worker_groups)[i];
> +		struct r5worker *workers;
> +
> +		workers = kzalloc_node(size, GFP_NOIO, i);

[Severity: High]
Could passing the sequential loop index i directly to kzalloc_node cause a
NULL pointer dereference on systems with sparse NUMA nodes?

Since num_possible_nodes returns a count rather than an upper bound on IDs,
on a system with nodes 0 and 8, i would be evaluated as 1. Passing an
unpopulated node ID to kzalloc_node without validation can lead to a crash
when accessing NODE_DATA(nid). Should this loop use
for_each_node_state(nid, N_MEMORY) or similar to get the actual node ID
instead of using the loop counter?

> +		if (!workers)
> +			goto out_free;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6