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

Posted by ghuicao@163.com 1 month ago

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 1/3] md/raid5: set pool_size before extra_page allocation to fix leak on error path

Posted by ghuicao@163.com 1 month ago

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 v4 1/2] md/raid5: track disks array size to fix extra_page leak on error paths

Posted by ghuicao@163.com 1 month ago

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

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

Posted by yu kuai 3 weeks, 2 days ago

在 2026/8/27 16:03, ghuicao@163.com 写道:

> 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(-)
Applied v4 to md-7.3.

-- 
Thanks,
Kuai

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

Posted by ghuicao@163.com 1 month ago

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

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

Posted by ghuicao@163.com 1 month ago

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

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

Posted by ghuicao@163.com 1 month ago

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

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

Posted by ghuicao@163.com 1 month ago

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

Posted by ghuicao@163.com 1 month ago

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

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

Posted by ghuicao@163.com 1 month ago

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