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

> [High] kzalloc_node(size, GFP_NOIO, i) -- i is a sequential index, not a node ID
> [High, pre-existing] worker_groups sized by num_possible_nodes() but indexed
> by node ID -> OOB on sparse NUMA

Both are valid.
The kzalloc_node(..., i) is wrong on sparse NUMA node IDs -- i runs
0..num_possible_nodes()-1, which only equals the node ID when nodes
are dense -- and it rides on the pre-existing assumption that the
worker_groups array (sized by the node *count*) can be indexed by
cpu_to_node().

The NUMA-locality win here is multi-socket-only and I don't have a
measurement to justify it, so rather than grow this into a sparse-NUMA
correctness fix I'll
drop this patch from v2.
The pre-existing worker_groups sizing/indexing issue is real and
separate; thanks for catching it.

Thanks,


2026年6月24日(水) 9:07 <[email protected]>:
>
> 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



-- 
Hiroshi Nishida
[email protected]