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

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

[ghuicao]

From: Cao Guanghui <caoguanghui@kylinos.cn>

In setup_conf(), conf->disks is allocated with max_disks slots and
extra_page is allocated for each slot.  However, pool_size remains 0
(uninitialized from kzalloc) until grow_stripes() sets it later.  If
any allocation or initialization between the extra_page loop and
grow_stripes() fails and jumps to abort, free_conf() iterates
pool_size (= 0) times and skips the extra_page freeing loop entirely,
leaking max_disks pages.

Set pool_size to max_disks right after the disks array allocation
succeeds, so that free_conf() correctly frees all allocated
extra_page entries on any error path.  This is safe because:

- If kzalloc_objs(disks) fails, pool_size stays 0 and free_conf
  skips the loop (kfree(NULL) is safe).
- grow_stripes() later sets pool_size = devs, which equals max_disks,
  so the early assignment does not change the final value.
- resize_stripes() only updates pool_size on success and reallocates
  the disks array in lockstep.

Fixes: d7bd398e97f2 ("md/r5cache: handle alloc_page failure")
Cc: stable@vger.kernel.org
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---
 drivers/md/raid5.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7732,8 +7732,9 @@ static struct r5conf *setup_conf(struct mddev *mddev)
 	conf->disks = kzalloc_objs(struct disk_info, max_disks);
 
 	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);
 		if (!conf->disks[i].extra_page)
-- 
2.34.1

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

[sashiko-bot]

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 <caoguanghui@kylinos.cn>

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: stable@vger.kernel.org

> 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/20260827023803.36602-1-ghuicao@163.com?part=1

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

[ghuicao]

From: Cao Guanghui <caoguanghui@kylinos.cn>

In setup_conf(), conf->disks is allocated with max_disks slots and
extra_page is allocated for each slot.  However, pool_size remains 0
(uninitialized from kzalloc) until grow_stripes() sets it later.  If
any allocation or initialization between the extra_page loop and
grow_stripes() fails and jumps to abort, free_conf() iterates
pool_size (= 0) times and skips the extra_page freeing loop entirely,
leaking max_disks pages.

Set pool_size to max_disks right after the disks array allocation
succeeds, so that free_conf() correctly frees all allocated
extra_page entries on any error path.  This is safe because:

- If kzalloc_objs(disks) fails, pool_size stays 0 and free_conf
  skips the loop (kfree(NULL) is safe).
- grow_stripes() later sets pool_size = devs, which equals max_disks,
  so the early assignment does not change the final value.
- resize_stripes() only updates pool_size on success and reallocates
  the disks array in lockstep.

Fixes: d7bd398e97f2 ("md/r5cache: handle alloc_page failure")
Cc: stable@vger.kernel.org
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---
 drivers/md/raid5.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7732,8 +7732,9 @@ static struct r5conf *setup_conf(struct mddev *mddev)
 	conf->disks = kzalloc_objs(struct disk_info, max_disks);
 
 	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);
 		if (!conf->disks[i].extra_page)
-- 
2.34.1

[PATCH v2 2/3] md/raid5: fix leak and use-after-free in resize_stripes error path

[ghuicao]

From: Cao Guanghui <caoguanghui@kylinos.cn>

resize_stripes() has two issues in how conf->disks is replaced:

1. Memory leak: conf->disks is replaced with ndisks in Step 3, but
   pool_size is only updated at the end with "if (!err)".  If Step 4
   (allocating pages for new stripe slots) fails, pool_size retains the
   old value.  On teardown, free_conf() iterates only pool_size entries,
   leaking (newsize - pool_size) extra_page allocations.

2. Use-after-free: conf->disks is freed and replaced without holding
   mddev->lock, while raid5_status() (called from /proc/mdstat via
   md_seq_show) reads conf->disks[i].rdev under mddev->lock.  The
   freeing and replacement happen under reconfig_mutex and
   cache_size_mutex, which do not exclude mddev->lock holders.

Fix both by deferring the conf->disks replacement until after Step 4
succeeds, and performing the pointer swap under mddev->lock so that
concurrent readers in raid5_status() see either the old or new array,
never a freed one.  If Step 4 fails, ndisks is freed instead.

This also preserves the original retry behavior: pool_size is only
updated on full success, so check_reshape() correctly calls
resize_stripes() again on retry.

Fixes: ad01c9e3752f ("[PATCH] md: Allow stripes to be expanded in preparation for expanding an array")
Cc: stable@vger.kernel.org
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---

Changes in v2:
  - Defer conf->disks replacement to after Step 4 instead of setting
    pool_size early, which would break reshape retry logic (Sashiko)
  - Add spinlock protection around the pointer swap to fix a concurrent
    use-after-free in raid5_status() (Sashiko)

 drivers/md/raid5.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2639,9 +2639,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
 				if (ndisks[i].extra_page)
 					put_page(ndisks[i].extra_page);
 			kfree(ndisks);
-		} else {
-			kfree(conf->disks);
-			conf->disks = ndisks;
+			ndisks = NULL;
 		}
 	} else
 		err = -ENOMEM;
@@ -2685,8 +2683,20 @@ static int resize_stripes(struct r5conf *conf, int newsize)
 	}
 	/* critical section pass, GFP_NOIO no longer needed */
 
-	if (!err)
+	if (!err && ndisks) {
+		struct disk_info *old_disks = conf->disks;
+
+		spin_lock_irq(&conf->mddev->lock);
+		conf->disks = ndisks;
+		spin_unlock_irq(&conf->mddev->lock);
+		kfree(old_disks);
 		conf->pool_size = newsize;
+	} else if (ndisks) {
+		for (i = conf->pool_size; i < newsize; i++)
+			if (ndisks[i].extra_page)
+				put_page(ndisks[i].extra_page);
+		kfree(ndisks);
+	}
 	mutex_unlock(&conf->cache_size_mutex);
 
 	return err;
-- 
2.34.1

Re: [PATCH v2 2/3] md/raid5: fix leak and use-after-free in resize_stripes error path

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Concurrent lockless read of `conf->disks` in IO and error paths races with synchronous `kfree(old_disks)`.
- [High] Memory leak of `stripe_head` pages (`nsh->dev[i].page`) when Step 4 of `resize_stripes()` fails.
--

commit 42a858726c79b14d53c892c406bd10d16e4932f8
Author: Cao Guanghui <caoguanghui@kylinos.cn>

md/raid5: fix leak and use-after-free in resize_stripes error path

This commit attempts to fix a memory leak and a use-after-free issue
in the resize_stripes error path. It defers the conf->disks replacement
and performs it under mddev->lock to prevent concurrent reads in
raid5_status from seeing a freed array.

Cc: stable@vger.kernel.org

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 4efaef622fc05..e481c48b45093 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c

[ ... ]

> @@ -2685,8 +2683,20 @@ static int resize_stripes(struct r5conf *conf, int newsize)
>  	}
>  	/* critical section pass, GFP_NOIO no longer needed */
>  
> -	if (!err)
> +	if (!err && ndisks) {
> +		struct disk_info *old_disks = conf->disks;
> +
> +		spin_lock_irq(&conf->mddev->lock);
> +		conf->disks = ndisks;
> +		spin_unlock_irq(&conf->mddev->lock);
> +		kfree(old_disks);

[Severity: Critical]
This is a pre-existing issue, but the patch's attempt to fix the conf->disks
use-after-free is incomplete and leaves critical race windows open. This
kfree() is synchronous and doesn't wait for an RCU grace period.

Because check_reshape() invokes resize_stripes() before reshape_position is
updated, chunk_aligned_read is still enabled. Does this allow
raid5_read_one_chunk() to run concurrently and read the freed conf->disks
array completely locklessly?

drivers/md/raid5.c:raid5_read_one_chunk() {
    ...
    rdev = conf->disks[dd_idx].replacement;
    if (!rdev || test_bit(Faulty, &rdev->flags) ||
        READ_ONCE(rdev->recovery_offset) < end_sector) {
        rdev = conf->disks[dd_idx].rdev;
    ...
}

Additionally, raid5_calc_degraded() iterates over conf->disks holding only
rcu_read_lock(). Will this synchronous kfree() cause a use-after-free for
those RCU readers as well?

drivers/md/raid5.c:raid5_calc_degraded() {
    ...
    struct md_rdev *rdev = READ_ONCE(conf->disks[i].rdev);
    ...
}

>  		conf->pool_size = newsize;
> +	} else if (ndisks) {

[Severity: High]
This isn't a bug introduced by this patch, but the patch attempts to fix
leaks in this error path and structurally misses this one. When Step 4 of
resize_stripes() fails to allocate a page for the new stripe slots
(nsh->dev[i].page), err is set to -ENOMEM and the pool_size update is
skipped here.

During array teardown, shrink_buffers() iterates only up to
sh->raid_conf->pool_size:

drivers/md/raid5.c:shrink_buffers() {
    ...
    int num = sh->raid_conf->pool_size;

#if PAGE_SIZE == DEFAULT_STRIPE_SIZE
    for (i = 0; i < num ; i++) {
    ...
}

Are the successfully allocated pages at indices >= pool_size permanently
leaked when free_stripe() destroys the slab object, because the cleanup loop
misses them?

> +		for (i = conf->pool_size; i < newsize; i++)
> +			if (ndisks[i].extra_page)
> +				put_page(ndisks[i].extra_page);
> +		kfree(ndisks);
> +	}
>  	mutex_unlock(&conf->cache_size_mutex);
>  
>  	return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827060554.77849-1-ghuicao@163.com?part=2

[PATCH v2 3/3] md/raid5: fix NULL pointer dereference in raid5_free_percpu

[ghuicao]

From: Cao Guanghui <caoguanghui@kylinos.cn>

If cpuhp_state_add_instance() fails in raid5_alloc_percpu() (e.g., the
startup callback raid456_cpu_up_prepare fails due to an allocation
failure), conf->node is never added to the cpuhp instance list and
its pprev remains NULL (from kzalloc initialization).

When setup_conf() then jumps to abort, free_conf() calls
raid5_free_percpu(), which checks conf->percpu (non-NULL, since it was
allocated before the cpuhp failure) and proceeds to call
cpuhp_state_remove_instance().  This calls hlist_del() on the unhashed
node, which dereferences node->pprev (NULL), causing a kernel panic.

Guard the removal with hlist_unhashed() so that the cpuhp instance is
only removed if it was actually added.

Fixes: 29c6d1bbd7a2 ("md/raid5: Convert to hotplug state machine")
Cc: stable@vger.kernel.org
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---
 drivers/md/raid5.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7539,7 +7539,8 @@ static void raid5_free_percpu(struct r5conf *conf)
 {
 	if (!conf->percpu)
 		return;
 
-	cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
+	if (!hlist_unhashed(&conf->node))
+		cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
 	free_percpu(conf->percpu);
 }
-- 
2.34.1

Re: [PATCH v2 3/3] md/raid5: fix NULL pointer dereference in raid5_free_percpu

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Data race when checking hlist_unhashed(&conf->node) locklessly.
--

commit f000e30dfd8bae5a7346baf7add33f6d2b5316e6
Author: Cao Guanghui <caoguanghui@kylinos.cn>

md/raid5: fix NULL pointer dereference in raid5_free_percpu

This commit adds a check in raid5_free_percpu() to only remove the CPU
hotplug instance if conf->node is hashed, preventing a NULL pointer
dereference on its pprev pointer during error teardown when cpuhp
registration fails.

Cc: stable@vger.kernel.org

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index e481c48b45093..537225c14a236 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -7548,7 +7548,8 @@ static void raid5_free_percpu(struct r5conf *conf)
>  	if (!conf->percpu)
>  		return;
>  
> -	cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
> +	if (!hlist_unhashed(&conf->node))

[Severity: Medium]
Does checking hlist_unhashed() locklessly here introduce a data race?

If one RAID array is being destroyed, it calls raid5_free_percpu() and
evaluates !hlist_unhashed(&conf->node) without holding the
cpuhp_state_mutex.

Concurrently, if another array is being created, it might take the lock
and call hlist_add_head() on the shared CPUHP_MD_RAID5_PREPARE list.
If the destroying array happens to be at the head, hlist_add_head()
updates its pprev pointer via WRITE_ONCE().

This concurrent modification can violate the kernel memory model and
trigger KCSAN load-tearing warnings. Could hlist_unhashed_lockless(),
which uses READ_ONCE(), be used here instead to safely handle concurrent
list operations?

> +		cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
>  	free_percpu(conf->percpu);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827060554.77849-1-ghuicao@163.com?part=3

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

[sashiko-bot]

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

Pre-existing issues:
- [High] NULL pointer dereference in log_exit() when early allocations in setup_conf() fail before conf->mddev is initialized.
--

commit 9327d7b2c0b735410bac76b4b55b188fa14875d3
Author: Cao Guanghui <caoguanghui@kylinos.cn>

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

    In setup_conf(), conf->disks is allocated with max_disks slots and
    extra_page is allocated for each slot. However, pool_size remains 0
    (uninitialized from kzalloc) until grow_stripes() sets it later. This
    fixes a memory leak by setting pool_size earlier.

> 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 is a pre-existing issue, but I noticed a potential NULL pointer
dereference on these early error paths.

If any early memory allocation in setup_conf() fails (such as
conf->pending_data, conf->disks, or alloc_page() for extra pages here),
the code jumps to the abort label and calls free_conf(conf). This happens
before conf->mddev is assigned.

free_conf() unconditionally calls log_exit(conf). Because conf->log is NULL
on these paths, log_exit() checks raid5_has_ppl():

drivers/md/raid5-log.h:log_exit() {
	if (conf->log)
		r5l_exit_log(conf);
	else if (raid5_has_ppl(conf))
		ppl_exit_log(conf);
}

This function evaluates test_bit(MD_HAS_PPL, &conf->mddev->flags), which
dereferences the uninitialized conf->mddev pointer:

drivers/md/raid5-log.h:raid5_has_ppl() {
	return test_bit(MD_HAS_PPL, &conf->mddev->flags);
}

Can conf->mddev = mddev; be assigned immediately after conf is allocated in
setup_conf() to avoid this crash during early aborts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827060554.77849-1-ghuicao@163.com?part=1

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

[ghuicao]

From: Cao Guanghui <caoguanghui@kylinos.cn>

In setup_conf(), conf->disks is allocated with max_disks slots and
extra_page is allocated for each slot.  However, pool_size remains 0
(uninitialized from kzalloc) until grow_stripes() sets it later.  If
any allocation or initialization between the extra_page loop and
grow_stripes() fails and jumps to abort, free_conf() iterates
pool_size (= 0) times and skips the extra_page freeing loop entirely,
leaking max_disks pages.

Set pool_size to max_disks right after the disks array allocation
succeeds, so that free_conf() correctly frees all allocated
extra_page entries on any error path.  This is safe because:

- If kzalloc_objs(disks) fails, pool_size stays 0 and free_conf
  skips the loop (kfree(NULL) is safe).
- grow_stripes() later sets pool_size = devs, which equals max_disks,
  so the early assignment does not change the final value.
- resize_stripes() only updates pool_size on success and reallocates
  the disks array in lockstep.

Fixes: d7bd398e97f2 ("md/r5cache: handle alloc_page failure")
Cc: stable@vger.kernel.org
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---
 drivers/md/raid5.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7732,8 +7732,9 @@ static struct r5conf *setup_conf(struct mddev *mddev)
 	conf->disks = kzalloc_objs(struct disk_info, max_disks);
 
 	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);
 		if (!conf->disks[i].extra_page)
-- 
2.34.1

[PATCH v3 2/3] md/raid5: fix leak and use-after-free in resize_stripes error path

[ghuicao]

From: Cao Guanghui <caoguanghui@kylinos.cn>

resize_stripes() has two issues in how conf->disks is replaced:

1. Memory leak: conf->disks is replaced with ndisks in Step 3, but
   pool_size is only updated at the end with "if (!err)".  If Step 4
   (allocating pages for new stripe slots) fails, pool_size retains the
   old value.  On teardown, free_conf() iterates only pool_size entries,
   leaking (newsize - pool_size) extra_page allocations.

2. Use-after-free: conf->disks is freed and replaced without holding
   mddev->lock, while raid5_status() (called from /proc/mdstat via
   md_seq_show) reads conf->disks[i].rdev under mddev->lock.  The
   freeing and replacement happen under reconfig_mutex and
   cache_size_mutex, which do not exclude mddev->lock holders.

Fix both by deferring the conf->disks replacement until after Step 4
succeeds, and performing the pointer swap under mddev->lock so that
concurrent readers in raid5_status() see either the old or new array,
never a freed one.  If Step 4 fails, ndisks is freed instead.

This also preserves the original retry behavior: pool_size is only
updated on full success, so check_reshape() correctly calls
resize_stripes() again on retry.

Fixes: ad01c9e3752f ("[PATCH] md: Allow stripes to be expanded in preparation for expanding an array")
Cc: stable@vger.kernel.org
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---

Changes in v2:
  - Defer conf->disks replacement to after Step 4 instead of setting
    pool_size early, which would break reshape retry logic (Sashiko)
  - Add spinlock protection around the pointer swap to fix a concurrent
    use-after-free in raid5_status() (Sashiko)

 drivers/md/raid5.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2639,9 +2639,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
 				if (ndisks[i].extra_page)
 					put_page(ndisks[i].extra_page);
 			kfree(ndisks);
-		} else {
-			kfree(conf->disks);
-			conf->disks = ndisks;
+			ndisks = NULL;
 		}
 	} else
 		err = -ENOMEM;
@@ -2685,8 +2683,20 @@ static int resize_stripes(struct r5conf *conf, int newsize)
 	}
 	/* critical section pass, GFP_NOIO no longer needed */
 
-	if (!err)
+	if (!err && ndisks) {
+		struct disk_info *old_disks = conf->disks;
+
+		spin_lock_irq(&conf->mddev->lock);
+		conf->disks = ndisks;
+		spin_unlock_irq(&conf->mddev->lock);
+		kfree(old_disks);
 		conf->pool_size = newsize;
+	} else if (ndisks) {
+		for (i = conf->pool_size; i < newsize; i++)
+			if (ndisks[i].extra_page)
+				put_page(ndisks[i].extra_page);
+		kfree(ndisks);
+	}
 	mutex_unlock(&conf->cache_size_mutex);
 
 	return err;
-- 
2.34.1

Re: [PATCH v3 2/3] md/raid5: fix leak and use-after-free in resize_stripes error path

[sashiko-bot]

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

New issues:
- [Critical] Moving the replacement and freeing of `conf->disks` to after Step 4 introduces a severe Use-After-Free race with the main IO thread (`raid5d`).

Pre-existing issues:
- [High] Incomplete UAF fix: `conf->disks` is still vulnerable to a concurrent use-after-free by `raid5_error()` and `raid5_calc_degraded()`.
--

commit bd1f3c6216c041887763e5099827c3cd4343c423
Author: Cao Guanghui <caoguanghui@kylinos.cn>

md/raid5: fix leak and use-after-free in resize_stripes error path

This commit addresses a memory leak when stripe slot allocation fails, and
attempts to fix a concurrent use-after-free by swapping the conf->disks array
under mddev->lock. The array replacement is deferred until after new stripe
slots are successfully allocated.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 4efaef622fc05..e481c48b45093 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c

[ ... ]

> @@ -2685,8 +2683,20 @@ static int resize_stripes(struct r5conf *conf, int newsize)
>  	}
>  	/* critical section pass, GFP_NOIO no longer needed */
>  
> -	if (!err)
> +	if (!err && ndisks) {
> +		struct disk_info *old_disks = conf->disks;
> +
> +		spin_lock_irq(&conf->mddev->lock);
> +		conf->disks = ndisks;
> +		spin_unlock_irq(&conf->mddev->lock);
> +		kfree(old_disks);
                ^^^^^^^^^^^^^^^^^

[Severity: Critical]
By deferring the kfree() of old_disks until after Step 4, does this code
introduce a race with the main IO thread?

During Step 4 (just above this code), resize_stripes() calls
raid5_release_stripe(), which returns stripes to service and wakes up the
raid5d IO thread.

Once awakened, raid5d resumes processing stripes and will locklessly access
the disks array:

raid5d()
  handle_stripe()
    analyse_stripe()
      rdev = conf->disks[i].replacement;

Because raid5d does not acquire mddev->lock, won't it dereference the freed
old_disks array when this code executes concurrently?


[Severity: High]
This isn't a bug introduced by this patch, but does protecting the pointer
swap with mddev->lock completely prevent concurrent use-after-free of
conf->disks?

The commit message notes that raid5_status() reads conf->disks under
mddev->lock. However, a concurrent block IO error can trigger raid5_error(),
which calls raid5_calc_degraded():

raid5_error()
  raid5_calc_degraded()
    struct md_rdev *rdev = READ_ONCE(conf->disks[i].rdev);

In raid5_error(), this is protected by conf->device_lock, not mddev->lock.
If raid5_error() executes concurrently with this kfree(), won't it
dereference the freed old_disks array?

>  		conf->pool_size = newsize;
> +	} else if (ndisks) {
> +		for (i = conf->pool_size; i < newsize; i++)
> +			if (ndisks[i].extra_page)
> +				put_page(ndisks[i].extra_page);
> +		kfree(ndisks);
> +	}
>  	mutex_unlock(&conf->cache_size_mutex);
>  
>  	return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827063235.85375-1-ghuicao@163.com?part=2

[PATCH v3 3/3] md/raid5: fix NULL pointer dereference in raid5_free_percpu

[ghuicao]

From: Cao Guanghui <caoguanghui@kylinos.cn>

If cpuhp_state_add_instance() fails in raid5_alloc_percpu() (e.g., the
startup callback raid456_cpu_up_prepare fails due to an allocation
failure), conf->node is never added to the cpuhp instance list and
its pprev remains NULL (from kzalloc initialization).

When setup_conf() then jumps to abort, free_conf() calls
raid5_free_percpu(), which checks conf->percpu (non-NULL, since it was
allocated before the cpuhp failure) and proceeds to call
cpuhp_state_remove_instance().  This calls hlist_del() on the unhashed
node, which dereferences node->pprev (NULL), causing a kernel panic.

Guard the removal with hlist_unhashed_lockless() so that the cpuhp
instance is only removed if it was actually added.  The lockless variant
uses READ_ONCE() for the pprev read, which is safe here because the
actual removal is synchronized by cpuhp_state_mutex inside
cpuhp_state_remove_instance().

Fixes: 29c6d1bbd7a2 ("md/raid5: Convert to hotplug state machine")
Cc: stable@vger.kernel.org
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---

Changes in v2:
  - Use hlist_unhashed_lockless() instead of hlist_unhashed() to avoid
    a KCSAN data race warning when another array is concurrently added
    to the same cpuhp instance list (Sashiko)

 drivers/md/raid5.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7539,7 +7539,8 @@ static void raid5_free_percpu(struct r5conf *conf)
 {
 	if (!conf->percpu)
 		return;
 
-	cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
+	if (!hlist_unhashed_lockless(&conf->node))
+		cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
 	free_percpu(conf->percpu);
 }
-- 
2.34.1

Re: [PATCH v3 3/3] md/raid5: fix NULL pointer dereference in raid5_free_percpu

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing workqueue flush before freeing worker structures during array teardown.
--

commit 7455143ee1b52af0e50fb9ef4f12b3a32d0dc0c2
Author: Cao Guanghui <caoguanghui@kylinos.cn>

    md/raid5: fix NULL pointer dereference in raid5_free_percpu
    
    If cpuhp_state_add_instance() fails in raid5_alloc_percpu(), conf->node is
    never added to the cpuhp instance list and its pprev remains NULL. This
    patch guards the removal with hlist_unhashed_lockless() so that the cpuhp
    instance is only removed if it was actually added, preventing a kernel panic.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index e481c48b45093..31417c91ab248 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -7548,7 +7548,8 @@ static void raid5_free_percpu(struct r5conf *conf)
>  	if (!conf->percpu)
>  		return;
>  
> -	cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
> +	if (!hlist_unhashed_lockless(&conf->node))
> +		cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
>  	free_percpu(conf->percpu);
>  }

While reviewing the array teardown path modified here, I noticed a separate
issue in the nearby free_thread_groups() function:

drivers/md/raid5.c:free_thread_groups() {
    ...
	if (conf->worker_groups)
		kfree(conf->worker_groups[0].workers);
	kfree(conf->worker_groups);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is a pre-existing issue.

Does this sequence miss a workqueue flush before freeing the worker structures?

During array teardown in md_stop(), raid5_quiesce() waits for active_stripes
to reach 0. This decrement and the subsequent wake_up(&conf->wait_for_quiescent)
can occur inside release_inactive_stripe_list(). This is called by
handle_active_stripes() (executed by raid5_do_work() on the worker thread)
after temporarily dropping conf->device_lock.

Once woken, raid5_quiesce() finishes and teardown proceeds to
free_thread_groups(). This function frees the memory backing worker->work
without calling flush_workqueue() or cancel_work_sync().

Meanwhile, the worker thread in raid5_do_work() could still be executing its
trailing loop and workqueue core cleanup code. Could this result in a
use-after-free of the worker structures, leading to memory corruption or a
kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827063235.85375-1-ghuicao@163.com?part=3

[PATCH v4 1/2] md/raid5: track disks array size to fix extra_page leak on error paths

[ghuicao]

From: Cao Guanghui <caoguanghui@kylinos.cn>

free_conf() iterates conf->pool_size entries to free extra_page
allocations, but pool_size may not reflect the actual size of the
conf->disks array.  Two scenarios cause a mismatch:

1. setup_conf() early abort: pool_size is 0 (not yet set by
   grow_stripes) but conf->disks has max_disks entries with
   extra_page allocated.  The loop iterates 0 times, leaking all
   pages.

2. resize_stripes() Step 4 failure: conf->disks was replaced with
   a newsize-entry array in Step 3, but pool_size is only updated
   on success.  The loop iterates pool_size (old, smaller value)
   times, leaking (newsize - pool_size) pages.

Add a dedicated disks_cnt field to track the actual number of
entries in conf->disks.  Set it immediately after each allocation
or replacement (in setup_conf and resize_stripes Step 3, where the
array is safely stalled with no concurrent access), and use it in
free_conf() instead of pool_size.

This leaves pool_size untouched, preserving the check_reshape()
retry behavior that depends on pool_size only being updated on
full success.

Fixes: d7bd398e97f2 ("md/r5cache: handle alloc_page failure")
Cc: stable@vger.kernel.org
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---
 drivers/md/raid5.c | 4 +++-
 drivers/md/raid5.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2642,6 +2642,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
 		} else {
 			kfree(conf->disks);
 			conf->disks = ndisks;
+			conf->disks_cnt = newsize;
 		}
 	} else
 		err = -ENOMEM;
@@ -7552,7 +7553,7 @@ static void free_conf(struct r5conf *conf)
 	free_thread_groups(conf);
 	shrink_stripes(conf);
 	raid5_free_percpu(conf);
-	for (i = 0; i < conf->pool_size; i++)
+	for (i = 0; i < conf->disks_cnt; i++)
 		if (conf->disks[i].extra_page)
 			put_page(conf->disks[i].extra_page);
 	kfree(conf->disks);
@@ -7733,7 +7734,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
 
 	if (!conf->disks)
 		goto abort;
-
+	conf->disks_cnt = max_disks;
 	for (i = 0; i < max_disks; i++) {
 		conf->disks[i].extra_page = alloc_page(GFP_KERNEL);
 		if (!conf->disks[i].extra_page)
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -667,6 +667,7 @@ struct r5conf {
 	unsigned long		cache_state;
 	struct shrinker		*shrinker;
 	int			pool_size; /* number of disks in stripeheads in pool */
+	int			disks_cnt; /* number of entries in disks[] array */
 	spinlock_t		device_lock;
 	struct disk_info	*disks;
 	struct bio_set		bio_split;
-- 
2.34.1

[PATCH v4 2/2] md/raid5: fix NULL pointer dereference in raid5_free_percpu

[ghuicao]

From: Cao Guanghui <caoguanghui@kylinos.cn>

If cpuhp_state_add_instance() fails in raid5_alloc_percpu() (e.g., the
startup callback raid456_cpu_up_prepare fails due to an allocation
failure), conf->node is never added to the cpuhp instance list and
its pprev remains NULL (from kzalloc initialization).

When setup_conf() then jumps to abort, free_conf() calls
raid5_free_percpu(), which checks conf->percpu (non-NULL, since it was
allocated before the cpuhp failure) and proceeds to call
cpuhp_state_remove_instance().  This calls hlist_del() on the unhashed
node, which dereferences node->pprev (NULL), causing a kernel panic.

Guard the removal with hlist_unhashed_lockless() so that the cpuhp
instance is only removed if it was actually added.  The lockless variant
uses READ_ONCE() for the pprev read, which is safe here because the
actual removal is synchronized by cpuhp_state_mutex inside
cpuhp_state_remove_instance().

Fixes: 29c6d1bbd7a2 ("md/raid5: Convert to hotplug state machine")
Cc: stable@vger.kernel.org
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---
 drivers/md/raid5.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7539,7 +7539,8 @@ static void raid5_free_percpu(struct r5conf *conf)
 {
 	if (!conf->percpu)
 		return;
 
-	cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
+	if (!hlist_unhashed_lockless(&conf->node))
+		cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
 	free_percpu(conf->percpu);
 }
-- 
2.34.1

Re: [PATCH v4 1/2] md/raid5: track disks array size to fix extra_page leak on error paths

[sashiko-bot]

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

Pre-existing issues:
- [High] Retrying `resize_stripes()` after a partial failure leaks `extra_page` allocations from the previous attempt.
- [High] `resize_stripes()` leaks `stripe_head` pages if Step 4 fails (on architectures where PAGE_SIZE == DEFAULT_STRIPE_SIZE).
- [High] Use-After-Free race condition on `conf->disks` array between `raid5_status()` and `resize_stripes()`
--

commit c7f0ab9943c7136150bf0d1225ef069780dedbe5
Author: Cao Guanghui <caoguanghui@kylinos.cn>

md/raid5: track disks array size to fix extra_page leak on error paths

This commit introduces disks_cnt to struct r5conf to track the actual number
of entries in conf->disks and uses it in free_conf() to fix memory leaks
when pool_size doesn't match the actual allocated array size.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index b91545ce090de..e5838bf516e38 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -2642,6 +2642,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
>  		} else {
>  			kfree(conf->disks);

[Severity: High]
This is a pre-existing issue, but can freeing the disks array directly here 
cause a use-after-free race with raid5_status()?

raid5_status() runs with only mddev->lock held and iterates over
conf->disks[i].rdev:

drivers/md/raid5.c:raid5_status() {
    ...
    struct md_rdev *rdev = READ_ONCE(conf->disks[i].rdev);
    ...
}

Would calling kfree() here (without kfree_rcu or synchronize_rcu) allow the
array to be freed while raid5_status() is actively reading from it?

[Severity: High]
This is also a pre-existing issue, but will this kfree() leak the 
extra_page allocations on retry if a previous reshape failed in Step 4?

If resize_stripes() succeeds in Step 3 (updating conf->disks) but fails
in Step 4, conf->pool_size is not updated. On a retry, the earlier copy
loop only copies elements up to the outdated conf->pool_size:

drivers/md/raid5.c:resize_stripes() {
    ...
    for (i = 0; i < conf->pool_size; i++)
        ndisks[i] = conf->disks[i];
    ...
}

Will this cause the extra_page allocations at indices >= pool_size from
the previous attempt to be permanently abandoned and leaked when this
kfree(conf->disks) is called?

>  			conf->disks = ndisks;
> +			conf->disks_cnt = newsize;

[Severity: High]
This isn't a bug introduced by this patch, but could resize_stripes()
leak stripe_head pages if a failure happens later in Step 4?

In Step 4, pages are allocated for the new stripe size:

drivers/md/raid5.c:resize_stripes() {
    ...
    for (i = conf->raid_disks; i < newsize; i++)
        if (nsh->dev[i].page == NULL) {
            struct page *p = alloc_page(GFP_NOIO);
            nsh->dev[i].page = p;
    ...
}

If an allocation fails, err = -ENOMEM is set, but the loop continues and
previously allocated pages remain attached. Because err is set,
conf->pool_size is never updated.

Later, shrink_buffers() bounds itself strictly to pool_size:

drivers/md/raid5.c:shrink_buffers() {
    ...
    int num = sh->raid_conf->pool_size;
    
    #if PAGE_SIZE == DEFAULT_STRIPE_SIZE
    for (i = 0; i < num ; i++) {
    ...
}

Will this unconditionally leak all orphaned pages residing at higher
indices (>= pool_size) when the stripe_head structures are dropped?

>  		}
>  	} else
>  		err = -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827063235.85375-1-ghuicao@163.com?part=1