Re: [PATCH 1/6] md/raid5: size the worker group array by nr_node_ids

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

Pre-existing issues:
- [High] Holding reconfig_mutex while waiting for raid5_wq workers to finish can cause an ABBA deadlock if workers wait for MD_SB_CHANGE_PENDING to be cleared.
--

--- Patch [1]: [PATCH 1/6] md/raid5: size the worker group array by nr_node_ids ---
commit 4f7875645a221b114b23eee045960c40b057b078
Author: Hiroshi Nishida <[email protected]>

md/raid5: size the worker group array by nr_node_ids

This patch fixes an out-of-bounds array access when the NUMA node map is
sparse by sizing the worker_groups array using nr_node_ids rather than
num_possible_nodes().

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 0c5c9fb0606ee..d8807114a6932 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -7322,7 +7322,13 @@ static int alloc_thread_groups(struct r5conf *conf, int cnt, int *group_cnt,
>  		*worker_groups = NULL;
>  		return 0;
>  	}
> -	*group_cnt = num_possible_nodes();
> +	/*
> +	 * worker_groups is indexed by cpu_to_group() == cpu_to_node(), a node
> +	 * id, so it must have room for the largest possible id.  Size it by
> +	 * nr_node_ids (one past that id), not num_possible_nodes(), which is
> +	 * only the node count and is smaller on a sparse node map.
> +	 */
> +	*group_cnt = nr_node_ids;

[Severity: High]
This is a pre-existing issue, but while reviewing this allocation change,
I noticed a potential deadlock risk in the caller,
raid5_store_group_thread_cnt(), when configuring these worker groups.

When writing to the group_thread_cnt sysfs attribute,
raid5_store_group_thread_cnt() acquires reconfig_mutex via
mddev_suspend_and_lock() and then blocks waiting for workers in
raid5_wq to finish:

drivers/md/raid5.c:raid5_store_group_thread_cnt() {
    err = mddev_suspend_and_lock(mddev);
    ...
    raid5_quiesce(mddev, true);

    if (new != conf->worker_cnt_per_group) {
        old_groups = conf->worker_groups;
        if (old_groups)
            flush_workqueue(raid5_wq);

        err = alloc_thread_groups(conf, new, &group_cnt, &new_groups);
}

However, the raid5 workers can block waiting for the md_thread to clear
MD_SB_CHANGE_PENDING:

drivers/md/raid5.c:raid5_do_work() {
    ...
    wait_event_lock_irq(mddev->sb_wait,
        !test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags),
        conf->device_lock);
}

The md_thread requires the reconfig_mutex to clear this flag:

drivers/md/md.c:md_check_recovery() {
    ...
    if (mddev_trylock(mddev)) {
        ...
        if (mddev->sb_flags)
            md_update_sb(mddev, 0);
}

Does this sequence create a deadlock where the sysfs thread holds
reconfig_mutex and waits for raid5_wq workers, while the workers wait for
the md_thread to clear MD_SB_CHANGE_PENDING, but the md_thread fails to
acquire reconfig_mutex via mddev_trylock()?

>  	size = sizeof(struct r5worker) * cnt;
>  	workers = kcalloc(size, *group_cnt, GFP_NOIO);
>  	*worker_groups = kzalloc_objs(struct r5worker_group, *group_cnt,

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