Re: [PATCH] md/raid5: set pool_size before extra_page allocation to fix leak on error path

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

Pre-existing issues:
- [High] Memory leak of extra_page and stripe pages during raid5 reshape error path.
- [High] Kernel panic due to NULL pointer dereference in `raid5_free_percpu()` when handling hotplug callback allocation failures.
--

commit e2a9b5ec799de7e8af41dc93ccf252fa2539b0d8
Author: Cao Guanghui <[email protected]>

md/raid5: set pool_size before extra_page allocation to fix leak on error path

Sets conf->pool_size right after the conf->disks array is allocated in
setup_conf(). This ensures that if subsequent initialization fails and
jumps to the abort label, free_conf() uses the correct pool size to clean
up the allocated extra_page entries, preventing a memory leak.

Cc: [email protected]

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index b91545ce090de..4efaef622fc05 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -7733,6 +7733,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
>  
>  	if (!conf->disks)
>  		goto abort;
> +	conf->pool_size = max_disks;
>  
>  	for (i = 0; i < max_disks; i++) {
>  		conf->disks[i].extra_page = alloc_page(GFP_KERNEL);

[Severity: High]
This isn't a bug introduced by this patch, but it exists in the surrounding
reshape logic. Does this same stale conf->pool_size issue leak memory in
resize_stripes()?

If alloc_page(GFP_NOIO) fails in step 4 of resize_stripes(), the function
returns -ENOMEM but fails to update conf->pool_size because it is gated by
if (!err).

However, conf->disks was already permanently replaced in step 3, and new
pages were already allocated for the newly added slots:

drivers/md/raid5.c:resize_stripes()
		} else {
			kfree(conf->disks);
			conf->disks = ndisks;
		}

Later in the function:
				if (!p)
					err = -ENOMEM;
			}

	if (!err)
		conf->pool_size = newsize;
	mutex_unlock(&conf->cache_size_mutex);

	return err;

Because conf->pool_size is not updated on error, subsequent teardown paths
like free_conf() and shrink_stripes() will use the old boundary and fail to
free the memory allocated for the new slots. Could this exhaust kernel
memory over multiple failed reshape attempts?

[Severity: High]
This is a pre-existing issue on the setup_conf() error path, but could
the abort path trigger a kernel panic due to a null pointer dereference
in raid5_free_percpu()?

If cpuhp_state_add_instance() fails in raid5_alloc_percpu(), the function
returns the error without cleaning up conf->percpu:

drivers/md/raid5.c:raid5_alloc_percpu()
	err = cpuhp_state_add_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
	if (!err) {

When setup_conf() receives the error and jumps to the abort label, free_conf()
calls raid5_free_percpu(). Because conf->percpu is not null, it incorrectly
assumes the hotplug node was hashed:

drivers/md/raid5.c:raid5_free_percpu()
	if (!conf->percpu)
		return;

	cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);

Since the node was never actually added, hlist_del() in the hotplug state
removal blindly unlinks the node, dereferencing node->pprev which is null:

kernel/cpu.c:__cpuhp_state_remove_instance()
remove:
	hlist_del(node);

Can this crash the kernel during raid array initialization or hotplug events
if memory allocation fails in the hotplug subsystem?

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