[PATCH v3 0/4] md: Fix two raid5/raid10 bugs

[PATCH v3 0/4] md: Fix two raid5/raid10 bugs

[Zhihao Cheng]

Fix two raid5/raid10 bugs in error injection case.

v1->v2:
 Handle pers->run failure in level_store
v2->v3:
 Patch 2: Release sysfs group in 'pers->run' error handling path

Zhihao Cheng (4):
  md/raid5: Hide the origin mddev->thread before takeover
  md: Handle pers->run failure in level_store
  md/raid5: Don't free conf on raid5_run failure
  md/raid10: Don't free conf on raid10_run failure

 drivers/md/md.c     | 29 ++++++++++++++++++++++++++++-
 drivers/md/raid10.c | 22 ++++++++++------------
 drivers/md/raid5.c  | 39 ++++++++++++++++++++++++++++-----------
 3 files changed, 66 insertions(+), 24 deletions(-)

-- 
2.52.0

[PATCH v3 1/4] md/raid5: Hide the origin mddev->thread before takeover

[Zhihao Cheng]

The raid5 takeover invokes setup_conf and allocates strip heads, but
it wakes up the wrong thread, which lefts strip heads in the list
'conf->released_stripes' and not being processed. If raid5_run fails,
the strip heads won't be released, which triggers the following slab
warnings (CONFIG_SLUB_DEBUG):
 BUG raid5-md0 (Not tainted): Objects remaining on __kmem_cache_shutdown()
 Object 0x0000000062fad548 @offset=3968
 Object 0x000000007f74683c @offset=4960
 WARNING: mm/slub.c:1268 at __slab_err+0x31/0x40, CPU#0: bash/865
 RIP: 0010:__slab_err+0x31
 Call Trace:
  __kmem_cache_shutdown.cold+0x15b
  kmem_cache_destroy+0x71
  free_conf+0xf8
  raid5_run.cold+0x463
  level_store+0x64e
  md_attr_store+0xd7

The detailed triggering process is as follows:
 mdadm --create /dev/md0 --level=1 --raid-devices=2 /dev/sda /dev/sdb
 --force --assume-clean # create raid1, mddev->thread is raid1d
 echo 5 > /sys/block/md0/md/level
  level_store
   raid5_takeover_raid1
     setup_conf
      grow_stripes
       grow_one_stripe
        sh = alloc_stripe
        raid5_release_stripe
          md_wakeup_thread(conf->mddev->thread) // wakeup raid1d
     raid5_run
      ENOMEM = raid5_create_ctx_pool
      free_conf
       shrink_stripes
        drop_one_stripe // no strips found from the conf->inactive_list
        kmem_cache_destroy(conf->slab_cache)
	 __kmem_cache_shutdown
	  free_partial
	   list_slab_objects // some entries are not released !

Fix it by hiding the origin mddev->thread before takeover, so that
new allocating strip heads can be put into 'conf->inactive_list',
which can be found by drop_one_stripe().

Fixes: 773ca82fa1ee ("raid5: make release_stripe lockless")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/md/raid5.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index c091bba95c31..7e87e8a60f5f 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -9039,19 +9039,38 @@ static void *raid5_takeover(struct mddev *mddev)
 	 *  raid4 - trivial - just use a raid4 layout.
 	 *  raid6 - Providing it is a *_6 layout
 	 */
-	if (mddev->level == 0)
-		return raid45_takeover_raid0(mddev, 5);
-	if (mddev->level == 1)
-		return raid5_takeover_raid1(mddev);
-	if (mddev->level == 4) {
+	void *ret = ERR_PTR(-EINVAL);
+	struct md_thread *thread;
+
+	thread = rcu_dereference_protected(mddev->thread,
+				lockdep_is_held(&mddev->reconfig_mutex));
+	/*
+	 * Set mddev->thread to NULL before setup_conf() to avoid waking up
+	 * wrong thread(eg. raid1), which can prevent the strips from being
+	 * left unreleased in the error handling path(free_conf) of raid5_run.
+	 */
+	rcu_assign_pointer(mddev->thread, NULL);
+
+	switch (mddev->level) {
+	case 0:
+		ret = raid45_takeover_raid0(mddev, 5);
+		break;
+	case 1:
+		ret = raid5_takeover_raid1(mddev);
+		break;
+	case 4:
 		mddev->new_layout = ALGORITHM_PARITY_N;
 		mddev->new_level = 5;
-		return setup_conf(mddev);
+		ret = setup_conf(mddev);
+		break;
+	case 6:
+		ret = raid5_takeover_raid6(mddev);
+		break;
 	}
-	if (mddev->level == 6)
-		return raid5_takeover_raid6(mddev);
 
-	return ERR_PTR(-EINVAL);
+	rcu_assign_pointer(mddev->thread, thread);
+
+	return ret;
 }
 
 static void *raid4_takeover(struct mddev *mddev)
-- 
2.52.0

[PATCH v3 2/4] md: Handle pers->run failure in level_store

[Zhihao Cheng]

Set 'raid_disks' after raid5_run() failure will trigger an
null-ptr-deref problem:
 BUG: kernel NULL pointer dereference, address: 0000000000000038
 RIP: 0010:raid5_check_reshape+0xad
 Call Trace:
  update_raid_disks+0x124
  raid_disks_store+0x145
  md_attr_store+0xd7
  sysfs_kf_write+0x7c

The trigger process is simple:
 mdadm --create /dev/md0 --level=1 --raid-devices=2 /dev/sda /dev/sdb
 --force --assume-clean # create raid1
 echo 5 > /sys/block/md0/md/level
  level_store
   mddev->pers = pers
   mddev->private = priv
   raid5_run
    fail to abort (eg. raid5_create_ctx_pool fails)
    mddev->private = NULL
 echo 10 > /sys/block/md0/md/raid_disks
  raid_disks_store
   if (mddev->pers) // true
    update_raid_disks
     raid5_check_reshape
      conf = mddev->private
       conf->algorithm = mddev->new_layout // null-ptr-deref !

Similar process exists in do_md_stop->__md_stop_writes->raid5_quiesce.
Similar process exists in raid10 too.

Fix it by handling the error from pers->run, next active-type order will
restart the mddev.

Fixes: 245f46c2c221e ("md: add ->takeover method to support changing the personality managing an array")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/md/md.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 02798f2dbf0e..ff78bf8656ff 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -97,6 +97,7 @@ static struct workqueue_struct *md_misc_wq;
 static int remove_and_add_spares(struct mddev *mddev,
 				 struct md_rdev *this);
 static void mddev_detach(struct mddev *mddev);
+static void __md_stop(struct mddev *mddev);
 static void export_rdev(struct md_rdev *rdev);
 static void md_wakeup_thread_directly(struct md_thread __rcu **thread);
 
@@ -4243,7 +4244,33 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
 		mddev->in_sync = 1;
 		timer_delete_sync(&mddev->safemode_timer);
 	}
-	pers->run(mddev);
+	rv = pers->run(mddev);
+	if (rv) {
+		/*
+		 * ->run() has released the private data of the new personality,
+		 * while the old one has already been released as well. There is
+		 * nothing to fall back to, so stop the array to avoid leaving
+		 * 'mddev->pers' pointing to a personality which has no private
+		 * data, and reminds user to try to active the mddev again.
+		 */
+		pr_warn("md: %s: failed to run %s after takeover, please try to active\n",
+			mdname(mddev), pers->head.name);
+		if (mddev->pers->sync_request && mddev->to_remove == NULL)
+			mddev->to_remove = &md_redundancy_group;
+		if (md_bitmap_enabled(mddev, true))
+			mddev->bitmap_ops->flush(mddev);
+		clear_bit(MD_SERIALIZE_POLICY, &mddev->flags);
+		mddev_destroy_serial_pool(mddev, NULL);
+		__md_stop(mddev);
+		rdev_for_each(rdev, mddev)
+			if (rdev->raid_disk >= 0)
+				sysfs_unlink_rdev(mddev, rdev);
+		set_capacity_and_notify(mddev->gendisk, 0);
+		mddev->changed = 1;
+		md_new_event();
+		sysfs_notify_dirent_safe(mddev->sysfs_state);
+		goto out_unlock;
+	}
 	set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
 	if (!mddev->thread)
 		md_update_sb(mddev, 1);
-- 
2.52.0

[PATCH v3 3/4] md/raid5: Don't free conf on raid5_run failure

[Zhihao Cheng]

Since all pers->run() callers handle the error case, no need to free
conf when raid5_run() fails.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/md/raid5.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 7e87e8a60f5f..913e5a709e35 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -8263,8 +8263,6 @@ static int raid5_run(struct mddev *mddev)
 abort:
 	md_unregister_thread(mddev, &mddev->thread);
 	print_raid5_conf(conf);
-	free_conf(conf);
-	mddev->private = NULL;
 	pr_warn("md/raid:%s: failed to run raid set.\n", mdname(mddev));
 	return ret;
 }
-- 
2.52.0

[PATCH v3 4/4] md/raid10: Don't free conf on raid10_run failure

[Zhihao Cheng]

Since all pers->run() callers handle the error case, no need to free
conf when raid10_run() fails.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/md/raid10.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 1093c798d9dd..9e4d4202f35c 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3972,7 +3972,7 @@ static int raid10_run(struct mddev *mddev)
 		if (fc > 1 || fo > 0) {
 			pr_err("only near layout is supported by clustered"
 				" raid10\n");
-			goto out_free_conf;
+			goto out_unregister_thread;
 		}
 	}
 
@@ -3989,11 +3989,11 @@ static int raid10_run(struct mddev *mddev)
 
 		if (test_bit(Replacement, &rdev->flags)) {
 			if (disk->replacement)
-				goto out_free_conf;
+				goto out_unregister_thread;
 			disk->replacement = rdev;
 		} else {
 			if (disk->rdev)
-				goto out_free_conf;
+				goto out_unregister_thread;
 			disk->rdev = rdev;
 		}
 		diff = (rdev->new_data_offset - rdev->data_offset);
@@ -4013,7 +4013,7 @@ static int raid10_run(struct mddev *mddev)
 
 		if (err) {
 			ret = err;
-			goto out_free_conf;
+			goto out_unregister_thread;
 		}
 	}
 
@@ -4021,17 +4021,17 @@ static int raid10_run(struct mddev *mddev)
 	if (!enough(conf, -1)) {
 		pr_err("md/raid10:%s: not enough operational mirrors.\n",
 		       mdname(mddev));
-		goto out_free_conf;
+		goto out_unregister_thread;
 	}
 
 	if (conf->reshape_progress != MaxSector) {
 		/* must ensure that shape change is supported */
 		if (conf->geo.far_copies != 1 &&
 		    conf->geo.far_offset == 0)
-			goto out_free_conf;
+			goto out_unregister_thread;
 		if (conf->prev.far_copies != 1 &&
 		    conf->prev.far_offset == 0)
-			goto out_free_conf;
+			goto out_unregister_thread;
 	}
 
 	mddev->degraded = 0;
@@ -4081,7 +4081,7 @@ static int raid10_run(struct mddev *mddev)
 	set_bit(MD_FAILFAST_SUPPORTED, &mddev->flags);
 
 	if (md_integrity_register(mddev))
-		goto out_free_conf;
+		goto out_unregister_thread;
 
 	if (conf->reshape_progress != MaxSector) {
 		unsigned long before_length, after_length;
@@ -4094,7 +4094,7 @@ static int raid10_run(struct mddev *mddev)
 		if (max(before_length, after_length) > min_offset_diff) {
 			/* This cannot work */
 			pr_warn("md/raid10: offset difference not enough to continue reshape\n");
-			goto out_free_conf;
+			goto out_unregister_thread;
 		}
 		conf->offset_diff = min_offset_diff;
 
@@ -4106,10 +4106,8 @@ static int raid10_run(struct mddev *mddev)
 
 	return 0;
 
-out_free_conf:
+out_unregister_thread:
 	md_unregister_thread(mddev, &mddev->thread);
-	raid10_free_conf(conf);
-	mddev->private = NULL;
 out:
 	return ret;
 }
-- 
2.52.0

Re: [PATCH v3 2/4] md: Handle pers->run failure in level_store

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] md: NULL pointer dereference in md_handle_request() after pers->run() fails
--

commit 3549464235e3de415ce8ceb834293e195eafad7d
Author: Zhihao Cheng <chengzhihao1@huawei.com>

md: Handle pers->run failure in level_store
    
This commit adds error handling for the pers->run(mddev) call in
level_store(). When pers->run() fails, it stops the array via __md_stop()
and unlinks sysfs nodes before unlocking and returning.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 680b34a63cb3d..0a3252098780b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c

[ ... ]

> @@ -4229,7 +4230,33 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
>  		mddev->in_sync = 1;
>  		timer_delete_sync(&mddev->safemode_timer);
>  	}
> -	pers->run(mddev);
> +	rv = pers->run(mddev);
> +	if (rv) {
> +		/*
> +		 * ->run() has released the private data of the new personality,
> +		 * while the old one has already been released as well. There is
> +		 * nothing to fall back to, so stop the array to avoid leaving
> +		 * 'mddev->pers' pointing to a personality which has no private
> +		 * data, and reminds user to try to active the mddev again.
> +		 */
> +		pr_warn("md: %s: failed to run %s after takeover, please try to active\n",
> +			mdname(mddev), pers->head.name);
> +		if (mddev->pers->sync_request && mddev->to_remove == NULL)
> +			mddev->to_remove = &md_redundancy_group;
> +		if (md_bitmap_enabled(mddev, true))
> +			mddev->bitmap_ops->flush(mddev);
> +		clear_bit(MD_SERIALIZE_POLICY, &mddev->flags);
> +		mddev_destroy_serial_pool(mddev, NULL);
> +		__md_stop(mddev);
> +		rdev_for_each(rdev, mddev)
> +			if (rdev->raid_disk >= 0)
> +				sysfs_unlink_rdev(mddev, rdev);
> +		set_capacity_and_notify(mddev->gendisk, 0);
> +		mddev->changed = 1;
> +		md_new_event();
> +		sysfs_notify_dirent_safe(mddev->sysfs_state);
> +		goto out_unlock;

[Severity: High]
Can this trigger a NULL pointer dereference in md_handle_request() for
concurrent bios?

If pers->run() fails, __md_stop() sets mddev->pers to NULL. The error path
then jumps to out_unlock, which subsequently calls mddev_unlock_and_resume().

When the array is resumed, mddev_unlock_and_resume() resurrects active_io
and wakes up mddev->sb_wait. If there are concurrent bios submitted to the
array, they may be blocked in md_handle_request() waiting on sb_wait:

drivers/md/md.c:md_handle_request() {
	...
		if (is_suspended(mddev, bio)) {
			/* Bail out if REQ_NOWAIT is set for the bio */
			if (bio->bi_opf & REQ_NOWAIT) {
				bio_wouldblock_error(bio);
				return true;
			}
			wait_event(mddev->sb_wait, !is_suspended(mddev, bio));
		}
		if (!percpu_ref_tryget_live(&mddev->active_io))
			goto check_suspended;
	}
	if (!mddev->pers->make_request(mddev, bio)) {
	...
}

Once woken up, they will re-acquire active_io and unconditionally dereference
mddev->pers->make_request. Since mddev->pers was set to NULL by __md_stop(),
will this result in a kernel panic?

> +	}
>  	set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
>  	if (!mddev->thread)
>  		md_update_sb(mddev, 1);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923112159.94175-1-chengzhihao1@huawei.com?part=2

Re: [PATCH v3 2/4] md: Handle pers->run failure in level_store

[Zhihao Cheng]

在 2026/9/23 19:46, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] md: NULL pointer dereference in md_handle_request() after pers->run() fails
> --
> 
> commit 3549464235e3de415ce8ceb834293e195eafad7d
> Author: Zhihao Cheng <chengzhihao1@huawei.com>
> 
> md: Handle pers->run failure in level_store
>      
> This commit adds error handling for the pers->run(mddev) call in
> level_store(). When pers->run() fails, it stops the array via __md_stop()
> and unlinks sysfs nodes before unlocking and returning.
> 
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 680b34a63cb3d..0a3252098780b 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
> 
> [ ... ]
> 
>> @@ -4229,7 +4230,33 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
>>   		mddev->in_sync = 1;
>>   		timer_delete_sync(&mddev->safemode_timer);
>>   	}
>> -	pers->run(mddev);
>> +	rv = pers->run(mddev);
>> +	if (rv) {
>> +		/*
>> +		 * ->run() has released the private data of the new personality,
>> +		 * while the old one has already been released as well. There is
>> +		 * nothing to fall back to, so stop the array to avoid leaving
>> +		 * 'mddev->pers' pointing to a personality which has no private
>> +		 * data, and reminds user to try to active the mddev again.
>> +		 */
>> +		pr_warn("md: %s: failed to run %s after takeover, please try to active\n",
>> +			mdname(mddev), pers->head.name);
>> +		if (mddev->pers->sync_request && mddev->to_remove == NULL)
>> +			mddev->to_remove = &md_redundancy_group;
>> +		if (md_bitmap_enabled(mddev, true))
>> +			mddev->bitmap_ops->flush(mddev);
>> +		clear_bit(MD_SERIALIZE_POLICY, &mddev->flags);
>> +		mddev_destroy_serial_pool(mddev, NULL);
>> +		__md_stop(mddev);
>> +		rdev_for_each(rdev, mddev)
>> +			if (rdev->raid_disk >= 0)
>> +				sysfs_unlink_rdev(mddev, rdev);
>> +		set_capacity_and_notify(mddev->gendisk, 0);
>> +		mddev->changed = 1;
>> +		md_new_event();
>> +		sysfs_notify_dirent_safe(mddev->sysfs_state);
>> +		goto out_unlock;
> 
> [Severity: High]
> Can this trigger a NULL pointer dereference in md_handle_request() for
> concurrent bios?
> 
> If pers->run() fails, __md_stop() sets mddev->pers to NULL. The error path
> then jumps to out_unlock, which subsequently calls mddev_unlock_and_resume().
> 
> When the array is resumed, mddev_unlock_and_resume() resurrects active_io
> and wakes up mddev->sb_wait. If there are concurrent bios submitted to the
> array, they may be blocked in md_handle_request() waiting on sb_wait:
> 
> drivers/md/md.c:md_handle_request() {
> 	...
> 		if (is_suspended(mddev, bio)) {
> 			/* Bail out if REQ_NOWAIT is set for the bio */
> 			if (bio->bi_opf & REQ_NOWAIT) {
> 				bio_wouldblock_error(bio);
> 				return true;
> 			}
> 			wait_event(mddev->sb_wait, !is_suspended(mddev, bio));
> 		}
> 		if (!percpu_ref_tryget_live(&mddev->active_io))
> 			goto check_suspended;
> 	}
> 	if (!mddev->pers->make_request(mddev, bio)) {
> 	...
> }
> 
> Once woken up, they will re-acquire active_io and unconditionally dereference
> mddev->pers->make_request. Since mddev->pers was set to NULL by __md_stop(),
> will this result in a kernel panic?

The problem won't happen after [1] applied.
[1] 
https://lore.kernel.org/lkml/20260922030538.1634904-1-chengzhihao1@huawei.com/
> 
>> +	}
>>   	set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
>>   	if (!mddev->thread)
>>   		md_update_sb(mddev, 1);
> 

Re: [PATCH v3 2/4] md: Handle pers->run failure in level_store

[Bruce Johnston]

Hi. I've been looking into takeover for a little while as well. I was
planning to write up something to post into the mailing list about it
but say your submission and figured I would mention it here. I believe
just handling a failure in pers->run is insufficient to fixing takeover
errors.

IMO the contract for level_store should be that if something fails, we
end up where we were before the function was called. So if we try to go
from 1->5 and something fails, we are back at a working valid level 1.

The current level_store function roughly does the following:
1. Suspend & Validate: Suspends the mddev device and performs initial
   validation checks.
2. Takeover: Invokes pers->takeover().
3. Swap Personality: Swaps the array to the new RAID personality.
4. Free Old State: Frees the old personality configuration and internal
   information.
5. Update sysfs fields from old to new
6. Run Personality: Calls pers->run to initialize the new RAID
   personality.
7. Resume Device: Resumes the mddev device.

By the time we get to run, we have swapped and free'd the old
personality.

The code in md_run around pers->run cleanup is reasonable because if
something fails, the array isn't started. You can't really do anything
after that. But in takeover, on a failed run, we're now in a state where
we have a half swapped personality. How do we recover from that? Are we
just going to stop the array? 

There are also additional issues in level_store where the personality
functions like pers->takeover do things like remove unsupported flags
from the mddev based on the new personality but never repair them on
failures.

I've been investigating possible takeover failure solutions but none of
them are easy. For example, we could try creating a new mddev at the
start of level_store and using that for the pers->foo calls but there
are a lot of shared resources so it becomes tricky. We could try
something like an undo log (each change tell how to undo itself) but
that would need to be passed into the personality functions or attached
to the mddev.

Should we have a discussion on this issue? I feel like we have
some holes here that need fixing.

On Wed, Sep 23, 2026 at 07:21:56PM +0800, Zhihao Cheng wrote:
> Set 'raid_disks' after raid5_run() failure will trigger an
> null-ptr-deref problem:
>  BUG: kernel NULL pointer dereference, address: 0000000000000038
>  RIP: 0010:raid5_check_reshape+0xad
>  Call Trace:
>   update_raid_disks+0x124
>   raid_disks_store+0x145
>   md_attr_store+0xd7
>   sysfs_kf_write+0x7c
> 
> The trigger process is simple:
>  mdadm --create /dev/md0 --level=1 --raid-devices=2 /dev/sda /dev/sdb
>  --force --assume-clean # create raid1
>  echo 5 > /sys/block/md0/md/level
>   level_store
>    mddev->pers = pers
>    mddev->private = priv
>    raid5_run
>     fail to abort (eg. raid5_create_ctx_pool fails)
>     mddev->private = NULL
>  echo 10 > /sys/block/md0/md/raid_disks
>   raid_disks_store
>    if (mddev->pers) // true
>     update_raid_disks
>      raid5_check_reshape
>       conf = mddev->private
>        conf->algorithm = mddev->new_layout // null-ptr-deref !
> 
> Similar process exists in do_md_stop->__md_stop_writes->raid5_quiesce.
> Similar process exists in raid10 too.
> 
> Fix it by handling the error from pers->run, next active-type order will
> restart the mddev.
> 
> Fixes: 245f46c2c221e ("md: add ->takeover method to support changing the personality managing an array")
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
>  drivers/md/md.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 02798f2dbf0e..ff78bf8656ff 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -97,6 +97,7 @@ static struct workqueue_struct *md_misc_wq;
>  static int remove_and_add_spares(struct mddev *mddev,
>  				 struct md_rdev *this);
>  static void mddev_detach(struct mddev *mddev);
> +static void __md_stop(struct mddev *mddev);
>  static void export_rdev(struct md_rdev *rdev);
>  static void md_wakeup_thread_directly(struct md_thread __rcu **thread);
>  
> @@ -4243,7 +4244,33 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
>  		mddev->in_sync = 1;
>  		timer_delete_sync(&mddev->safemode_timer);
>  	}
> -	pers->run(mddev);
> +	rv = pers->run(mddev);
> +	if (rv) {
> +		/*
> +		 * ->run() has released the private data of the new personality,
> +		 * while the old one has already been released as well. There is
> +		 * nothing to fall back to, so stop the array to avoid leaving
> +		 * 'mddev->pers' pointing to a personality which has no private
> +		 * data, and reminds user to try to active the mddev again.
> +		 */
> +		pr_warn("md: %s: failed to run %s after takeover, please try to active\n",
> +			mdname(mddev), pers->head.name);
> +		if (mddev->pers->sync_request && mddev->to_remove == NULL)
> +			mddev->to_remove = &md_redundancy_group;
> +		if (md_bitmap_enabled(mddev, true))
> +			mddev->bitmap_ops->flush(mddev);
> +		clear_bit(MD_SERIALIZE_POLICY, &mddev->flags);
> +		mddev_destroy_serial_pool(mddev, NULL);
> +		__md_stop(mddev);
> +		rdev_for_each(rdev, mddev)
> +			if (rdev->raid_disk >= 0)
> +				sysfs_unlink_rdev(mddev, rdev);
> +		set_capacity_and_notify(mddev->gendisk, 0);
> +		mddev->changed = 1;
> +		md_new_event();
> +		sysfs_notify_dirent_safe(mddev->sysfs_state);
> +		goto out_unlock;
> +	}
>  	set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
>  	if (!mddev->thread)
>  		md_update_sb(mddev, 1);
> -- 
> 2.52.0
> 
> 

Re: [PATCH v3 2/4] md: Handle pers->run failure in level_store

[Zhihao Cheng]

在 2026/9/24 2:42, Bruce Johnston 写道:
Hi Bruce,
> Hi. I've been looking into takeover for a little while as well. I was
> planning to write up something to post into the mailing list about it
> but say your submission and figured I would mention it here. I believe
> just handling a failure in pers->run is insufficient to fixing takeover
> errors.
> 
> IMO the contract for level_store should be that if something fails, we
> end up where we were before the function was called. So if we try to go
> from 1->5 and something fails, we are back at a working valid level 1.
Indeed, this is the most correct implementation for users, the 
level_store operation should be atomic, and my patch is just a simple 
workaround implementation.
> 
> The current level_store function roughly does the following:
> 1. Suspend & Validate: Suspends the mddev device and performs initial
>     validation checks.
> 2. Takeover: Invokes pers->takeover().
> 3. Swap Personality: Swaps the array to the new RAID personality.
> 4. Free Old State: Frees the old personality configuration and internal
>     information.
> 5. Update sysfs fields from old to new
> 6. Run Personality: Calls pers->run to initialize the new RAID
>     personality.
> 7. Resume Device: Resumes the mddev device.
> 
> By the time we get to run, we have swapped and free'd the old
> personality.
> 
> The code in md_run around pers->run cleanup is reasonable because if
> something fails, the array isn't started. You can't really do anything
> after that. But in takeover, on a failed run, we're now in a state where
> we have a half swapped personality. How do we recover from that? Are we
> just going to stop the array?
> 
> There are also additional issues in level_store where the personality
> functions like pers->takeover do things like remove unsupported flags
> from the mddev based on the new personality but never repair them on
> failures.
> 
> I've been investigating possible takeover failure solutions but none of
> them are easy. For example, we could try creating a new mddev at the
> start of level_store and using that for the pers->foo calls but there
> are a lot of shared resources so it becomes tricky.

Separating shared resources (eg. private, thread, bitmap, all_mddevs, 
etc.) is a trivial and error-prone task, and we need to have a clear 
understanding of the lifecycle of these shared resources.

> We could try
> something like an undo log (each change tell how to undo itself) but
> that would need to be passed into the personality functions or attached
> to the mddev.

The undo log solution will also bring significant changes, and 
maintaining the newly added level_store process in the future will be 
relatively difficult.

So far, I cannot find a more suitable solution.
> 
> Should we have a discussion on this issue? I feel like we have
> some holes here that need fixing.
> 
> On Wed, Sep 23, 2026 at 07:21:56PM +0800, Zhihao Cheng wrote:
>> Set 'raid_disks' after raid5_run() failure will trigger an
>> null-ptr-deref problem:
>>   BUG: kernel NULL pointer dereference, address: 0000000000000038
>>   RIP: 0010:raid5_check_reshape+0xad
>>   Call Trace:
>>    update_raid_disks+0x124
>>    raid_disks_store+0x145
>>    md_attr_store+0xd7
>>    sysfs_kf_write+0x7c
>>
>> The trigger process is simple:
>>   mdadm --create /dev/md0 --level=1 --raid-devices=2 /dev/sda /dev/sdb
>>   --force --assume-clean # create raid1
>>   echo 5 > /sys/block/md0/md/level
>>    level_store
>>     mddev->pers = pers
>>     mddev->private = priv
>>     raid5_run
>>      fail to abort (eg. raid5_create_ctx_pool fails)
>>      mddev->private = NULL
>>   echo 10 > /sys/block/md0/md/raid_disks
>>    raid_disks_store
>>     if (mddev->pers) // true
>>      update_raid_disks
>>       raid5_check_reshape
>>        conf = mddev->private
>>         conf->algorithm = mddev->new_layout // null-ptr-deref !
>>
>> Similar process exists in do_md_stop->__md_stop_writes->raid5_quiesce.
>> Similar process exists in raid10 too.
>>
>> Fix it by handling the error from pers->run, next active-type order will
>> restart the mddev.
>>
>> Fixes: 245f46c2c221e ("md: add ->takeover method to support changing the personality managing an array")
>> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
>> ---
>>   drivers/md/md.c | 29 ++++++++++++++++++++++++++++-
>>   1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 02798f2dbf0e..ff78bf8656ff 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -97,6 +97,7 @@ static struct workqueue_struct *md_misc_wq;
>>   static int remove_and_add_spares(struct mddev *mddev,
>>   				 struct md_rdev *this);
>>   static void mddev_detach(struct mddev *mddev);
>> +static void __md_stop(struct mddev *mddev);
>>   static void export_rdev(struct md_rdev *rdev);
>>   static void md_wakeup_thread_directly(struct md_thread __rcu **thread);
>>   
>> @@ -4243,7 +4244,33 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
>>   		mddev->in_sync = 1;
>>   		timer_delete_sync(&mddev->safemode_timer);
>>   	}
>> -	pers->run(mddev);
>> +	rv = pers->run(mddev);
>> +	if (rv) {
>> +		/*
>> +		 * ->run() has released the private data of the new personality,
>> +		 * while the old one has already been released as well. There is
>> +		 * nothing to fall back to, so stop the array to avoid leaving
>> +		 * 'mddev->pers' pointing to a personality which has no private
>> +		 * data, and reminds user to try to active the mddev again.
>> +		 */
>> +		pr_warn("md: %s: failed to run %s after takeover, please try to active\n",
>> +			mdname(mddev), pers->head.name);
>> +		if (mddev->pers->sync_request && mddev->to_remove == NULL)
>> +			mddev->to_remove = &md_redundancy_group;
>> +		if (md_bitmap_enabled(mddev, true))
>> +			mddev->bitmap_ops->flush(mddev);
>> +		clear_bit(MD_SERIALIZE_POLICY, &mddev->flags);
>> +		mddev_destroy_serial_pool(mddev, NULL);
>> +		__md_stop(mddev);
>> +		rdev_for_each(rdev, mddev)
>> +			if (rdev->raid_disk >= 0)
>> +				sysfs_unlink_rdev(mddev, rdev);
>> +		set_capacity_and_notify(mddev->gendisk, 0);
>> +		mddev->changed = 1;
>> +		md_new_event();
>> +		sysfs_notify_dirent_safe(mddev->sysfs_state);
>> +		goto out_unlock;
>> +	}
>>   	set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
>>   	if (!mddev->thread)
>>   		md_update_sb(mddev, 1);
>> -- 
>> 2.52.0
>>
>>
> 
> 
> .
> 

Re: [PATCH v3 2/4] md: Handle pers->run failure in level_store

[Bruce Johnston]

One of the things I have been looking at is how dm-raid does takeover.
As you might expect, it behaves much differently than how md.c does
takeover. In the case of dm-raid, it basically builds up a brand new
array from scratch that is read_only and frozen. This is all done in the
raid_ctr function in dm-raid.c. Then when you want to swap, you
basically suspend the old array, do the appropriate swap code and then
turn the new array to read write and unfreeze the threads.
pers->takeover is never called in this scenario as far as I can tell,
but pers->run is through a call to md_run, but because the threads are
frozen, no I/O happens until the swap is done.  

I'm not saying this is the correct model to follow here, but
it does possibly suggest we might be able to do something similar in
terms of sharing resources.  

I know this is not as easy problem to solve and any fix may require
extensive changes, but I feel like we should try to solve this now
before things become more complicated. 

On Thu, Sep 24, 2026 at 07:53:50PM +0800, Zhihao Cheng wrote:
> 在 2026/9/24 2:42, Bruce Johnston 写道:
> Hi Bruce,
> > Hi. I've been looking into takeover for a little while as well. I was
> > planning to write up something to post into the mailing list about it
> > but say your submission and figured I would mention it here. I believe
> > just handling a failure in pers->run is insufficient to fixing takeover
> > errors.
> > 
> > IMO the contract for level_store should be that if something fails, we
> > end up where we were before the function was called. So if we try to go
> > from 1->5 and something fails, we are back at a working valid level 1.
> Indeed, this is the most correct implementation for users, the level_store
> operation should be atomic, and my patch is just a simple workaround
> implementation.
> > 
> > The current level_store function roughly does the following:
> > 1. Suspend & Validate: Suspends the mddev device and performs initial
> >     validation checks.
> > 2. Takeover: Invokes pers->takeover().
> > 3. Swap Personality: Swaps the array to the new RAID personality.
> > 4. Free Old State: Frees the old personality configuration and internal
> >     information.
> > 5. Update sysfs fields from old to new
> > 6. Run Personality: Calls pers->run to initialize the new RAID
> >     personality.
> > 7. Resume Device: Resumes the mddev device.
> > 
> > By the time we get to run, we have swapped and free'd the old
> > personality.
> > 
> > The code in md_run around pers->run cleanup is reasonable because if
> > something fails, the array isn't started. You can't really do anything
> > after that. But in takeover, on a failed run, we're now in a state where
> > we have a half swapped personality. How do we recover from that? Are we
> > just going to stop the array?
> > 
> > There are also additional issues in level_store where the personality
> > functions like pers->takeover do things like remove unsupported flags
> > from the mddev based on the new personality but never repair them on
> > failures.
> > 
> > I've been investigating possible takeover failure solutions but none of
> > them are easy. For example, we could try creating a new mddev at the
> > start of level_store and using that for the pers->foo calls but there
> > are a lot of shared resources so it becomes tricky.
> 
> Separating shared resources (eg. private, thread, bitmap, all_mddevs, etc.)
> is a trivial and error-prone task, and we need to have a clear understanding
> of the lifecycle of these shared resources.
> 
> > We could try
> > something like an undo log (each change tell how to undo itself) but
> > that would need to be passed into the personality functions or attached
> > to the mddev.
> 
> The undo log solution will also bring significant changes, and maintaining
> the newly added level_store process in the future will be relatively
> difficult.
> 
> So far, I cannot find a more suitable solution.
> > 
> > Should we have a discussion on this issue? I feel like we have
> > some holes here that need fixing.
> > 
> > On Wed, Sep 23, 2026 at 07:21:56PM +0800, Zhihao Cheng wrote:
> > > Set 'raid_disks' after raid5_run() failure will trigger an
> > > null-ptr-deref problem:
> > >   BUG: kernel NULL pointer dereference, address: 0000000000000038
> > >   RIP: 0010:raid5_check_reshape+0xad
> > >   Call Trace:
> > >    update_raid_disks+0x124
> > >    raid_disks_store+0x145
> > >    md_attr_store+0xd7
> > >    sysfs_kf_write+0x7c
> > > 
> > > The trigger process is simple:
> > >   mdadm --create /dev/md0 --level=1 --raid-devices=2 /dev/sda /dev/sdb
> > >   --force --assume-clean # create raid1
> > >   echo 5 > /sys/block/md0/md/level
> > >    level_store
> > >     mddev->pers = pers
> > >     mddev->private = priv
> > >     raid5_run
> > >      fail to abort (eg. raid5_create_ctx_pool fails)
> > >      mddev->private = NULL
> > >   echo 10 > /sys/block/md0/md/raid_disks
> > >    raid_disks_store
> > >     if (mddev->pers) // true
> > >      update_raid_disks
> > >       raid5_check_reshape
> > >        conf = mddev->private
> > >         conf->algorithm = mddev->new_layout // null-ptr-deref !
> > > 
> > > Similar process exists in do_md_stop->__md_stop_writes->raid5_quiesce.
> > > Similar process exists in raid10 too.
> > > 
> > > Fix it by handling the error from pers->run, next active-type order will
> > > restart the mddev.
> > > 
> > > Fixes: 245f46c2c221e ("md: add ->takeover method to support changing the personality managing an array")
> > > Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> > > ---
> > >   drivers/md/md.c | 29 ++++++++++++++++++++++++++++-
> > >   1 file changed, 28 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > > index 02798f2dbf0e..ff78bf8656ff 100644
> > > --- a/drivers/md/md.c
> > > +++ b/drivers/md/md.c
> > > @@ -97,6 +97,7 @@ static struct workqueue_struct *md_misc_wq;
> > >   static int remove_and_add_spares(struct mddev *mddev,
> > >   				 struct md_rdev *this);
> > >   static void mddev_detach(struct mddev *mddev);
> > > +static void __md_stop(struct mddev *mddev);
> > >   static void export_rdev(struct md_rdev *rdev);
> > >   static void md_wakeup_thread_directly(struct md_thread __rcu **thread);
> > > @@ -4243,7 +4244,33 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
> > >   		mddev->in_sync = 1;
> > >   		timer_delete_sync(&mddev->safemode_timer);
> > >   	}
> > > -	pers->run(mddev);
> > > +	rv = pers->run(mddev);
> > > +	if (rv) {
> > > +		/*
> > > +		 * ->run() has released the private data of the new personality,
> > > +		 * while the old one has already been released as well. There is
> > > +		 * nothing to fall back to, so stop the array to avoid leaving
> > > +		 * 'mddev->pers' pointing to a personality which has no private
> > > +		 * data, and reminds user to try to active the mddev again.
> > > +		 */
> > > +		pr_warn("md: %s: failed to run %s after takeover, please try to active\n",
> > > +			mdname(mddev), pers->head.name);
> > > +		if (mddev->pers->sync_request && mddev->to_remove == NULL)
> > > +			mddev->to_remove = &md_redundancy_group;
> > > +		if (md_bitmap_enabled(mddev, true))
> > > +			mddev->bitmap_ops->flush(mddev);
> > > +		clear_bit(MD_SERIALIZE_POLICY, &mddev->flags);
> > > +		mddev_destroy_serial_pool(mddev, NULL);
> > > +		__md_stop(mddev);
> > > +		rdev_for_each(rdev, mddev)
> > > +			if (rdev->raid_disk >= 0)
> > > +				sysfs_unlink_rdev(mddev, rdev);
> > > +		set_capacity_and_notify(mddev->gendisk, 0);
> > > +		mddev->changed = 1;
> > > +		md_new_event();
> > > +		sysfs_notify_dirent_safe(mddev->sysfs_state);
> > > +		goto out_unlock;
> > > +	}
> > >   	set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
> > >   	if (!mddev->thread)
> > >   		md_update_sb(mddev, 1);
> > > -- 
> > > 2.52.0
> > > 
> > > 
> > 
> > 
> > .
> > 
> 

Re: [PATCH v3 1/4] md/raid5: Hide the origin mddev->thread before takeover

[yu kuai]

Hi,

在 2026/9/23 19:21, Zhihao Cheng 写道:
> The raid5 takeover invokes setup_conf and allocates strip heads, but
> it wakes up the wrong thread, which lefts strip heads in the list
> 'conf->released_stripes' and not being processed. If raid5_run fails,
> the strip heads won't be released, which triggers the following slab
> warnings (CONFIG_SLUB_DEBUG):
>   BUG raid5-md0 (Not tainted): Objects remaining on __kmem_cache_shutdown()
>   Object 0x0000000062fad548 @offset=3968
>   Object 0x000000007f74683c @offset=4960
>   WARNING: mm/slub.c:1268 at __slab_err+0x31/0x40, CPU#0: bash/865
>   RIP: 0010:__slab_err+0x31
>   Call Trace:
>    __kmem_cache_shutdown.cold+0x15b
>    kmem_cache_destroy+0x71
>    free_conf+0xf8
>    raid5_run.cold+0x463
>    level_store+0x64e
>    md_attr_store+0xd7

Is this still a problem with following patch?

[PATCH] md/raid5: drain released_stripes before destroying the cache - 
Li Youhong 
<https://lore.kernel.org/all/20260929083916.50620-1-dayou5941@163.com/>

>
> The detailed triggering process is as follows:
>   mdadm --create /dev/md0 --level=1 --raid-devices=2 /dev/sda /dev/sdb
>   --force --assume-clean # create raid1, mddev->thread is raid1d
>   echo 5 > /sys/block/md0/md/level
>    level_store
>     raid5_takeover_raid1
>       setup_conf
>        grow_stripes
>         grow_one_stripe
>          sh = alloc_stripe
>          raid5_release_stripe
>            md_wakeup_thread(conf->mddev->thread) // wakeup raid1d
>       raid5_run
>        ENOMEM = raid5_create_ctx_pool
>        free_conf
>         shrink_stripes
>          drop_one_stripe // no strips found from the conf->inactive_list
>          kmem_cache_destroy(conf->slab_cache)
> 	 __kmem_cache_shutdown
> 	  free_partial
> 	   list_slab_objects // some entries are not released !
>
> Fix it by hiding the origin mddev->thread before takeover, so that
> new allocating strip heads can be put into 'conf->inactive_list',
> which can be found by drop_one_stripe().
>
> Fixes: 773ca82fa1ee ("raid5: make release_stripe lockless")
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
>   drivers/md/raid5.c | 37 ++++++++++++++++++++++++++++---------
>   1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index c091bba95c31..7e87e8a60f5f 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -9039,19 +9039,38 @@ static void *raid5_takeover(struct mddev *mddev)
>   	 *  raid4 - trivial - just use a raid4 layout.
>   	 *  raid6 - Providing it is a *_6 layout
>   	 */
> -	if (mddev->level == 0)
> -		return raid45_takeover_raid0(mddev, 5);
> -	if (mddev->level == 1)
> -		return raid5_takeover_raid1(mddev);
> -	if (mddev->level == 4) {
> +	void *ret = ERR_PTR(-EINVAL);
> +	struct md_thread *thread;
> +
> +	thread = rcu_dereference_protected(mddev->thread,
> +				lockdep_is_held(&mddev->reconfig_mutex));
> +	/*
> +	 * Set mddev->thread to NULL before setup_conf() to avoid waking up
> +	 * wrong thread(eg. raid1), which can prevent the strips from being
> +	 * left unreleased in the error handling path(free_conf) of raid5_run.
> +	 */
> +	rcu_assign_pointer(mddev->thread, NULL);
> +
> +	switch (mddev->level) {
> +	case 0:
> +		ret = raid45_takeover_raid0(mddev, 5);
> +		break;
> +	case 1:
> +		ret = raid5_takeover_raid1(mddev);
> +		break;
> +	case 4:
>   		mddev->new_layout = ALGORITHM_PARITY_N;
>   		mddev->new_level = 5;
> -		return setup_conf(mddev);
> +		ret = setup_conf(mddev);
> +		break;
> +	case 6:
> +		ret = raid5_takeover_raid6(mddev);
> +		break;
>   	}
> -	if (mddev->level == 6)
> -		return raid5_takeover_raid6(mddev);
>   
> -	return ERR_PTR(-EINVAL);
> +	rcu_assign_pointer(mddev->thread, thread);
> +
> +	return ret;
>   }
>   
>   static void *raid4_takeover(struct mddev *mddev)

-- 
Thanks,
Kuai

Re: [PATCH v3 2/4] md: Handle pers->run failure in level_store

[yu kuai]

Hi,

在 2026/9/23 19:21, Zhihao Cheng 写道:
> Set 'raid_disks' after raid5_run() failure will trigger an
> null-ptr-deref problem:
>   BUG: kernel NULL pointer dereference, address: 0000000000000038
>   RIP: 0010:raid5_check_reshape+0xad
>   Call Trace:
>    update_raid_disks+0x124
>    raid_disks_store+0x145
>    md_attr_store+0xd7
>    sysfs_kf_write+0x7c
>
> The trigger process is simple:
>   mdadm --create /dev/md0 --level=1 --raid-devices=2 /dev/sda /dev/sdb
>   --force --assume-clean # create raid1
>   echo 5 > /sys/block/md0/md/level
>    level_store
>     mddev->pers = pers
>     mddev->private = priv
>     raid5_run
>      fail to abort (eg. raid5_create_ctx_pool fails)
>      mddev->private = NULL
>   echo 10 > /sys/block/md0/md/raid_disks
>    raid_disks_store
>     if (mddev->pers) // true
>      update_raid_disks
>       raid5_check_reshape
>        conf = mddev->private
>         conf->algorithm = mddev->new_layout // null-ptr-deref !
>
> Similar process exists in do_md_stop->__md_stop_writes->raid5_quiesce.
> Similar process exists in raid10 too.
>
> Fix it by handling the error from pers->run, next active-type order will
> restart the mddev.
>
> Fixes: 245f46c2c221e ("md: add ->takeover method to support changing the personality managing an array")
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
>   drivers/md/md.c | 29 ++++++++++++++++++++++++++++-
>   1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 02798f2dbf0e..ff78bf8656ff 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -97,6 +97,7 @@ static struct workqueue_struct *md_misc_wq;
>   static int remove_and_add_spares(struct mddev *mddev,
>   				 struct md_rdev *this);
>   static void mddev_detach(struct mddev *mddev);
> +static void __md_stop(struct mddev *mddev);
>   static void export_rdev(struct md_rdev *rdev);
>   static void md_wakeup_thread_directly(struct md_thread __rcu **thread);
>   
> @@ -4243,7 +4244,33 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
>   		mddev->in_sync = 1;
>   		timer_delete_sync(&mddev->safemode_timer);
>   	}
> -	pers->run(mddev);
> +	rv = pers->run(mddev);
> +	if (rv) {
> +		/*
> +		 * ->run() has released the private data of the new personality,
> +		 * while the old one has already been released as well. There is
> +		 * nothing to fall back to, so stop the array to avoid leaving
> +		 * 'mddev->pers' pointing to a personality which has no private
> +		 * data, and reminds user to try to active the mddev again.
> +		 */
> +		pr_warn("md: %s: failed to run %s after takeover, please try to active\n",
> +			mdname(mddev), pers->head.name);

Usually takeover can be performed on a live array, and with a live fs mounted, so
I don't think it's acceptable for the array to be in this state in the case
pers->run failed.

Is it possible to split pers->run into two parts? One for static checking and
memory allocation, so the next pers->run that will never fail. Which means we can
keep the old pers active until we can make sure the new pers activation callback will
not fail.

> +		if (mddev->pers->sync_request && mddev->to_remove == NULL)
> +			mddev->to_remove = &md_redundancy_group;
> +		if (md_bitmap_enabled(mddev, true))
> +			mddev->bitmap_ops->flush(mddev);
> +		clear_bit(MD_SERIALIZE_POLICY, &mddev->flags);
> +		mddev_destroy_serial_pool(mddev, NULL);
> +		__md_stop(mddev);
> +		rdev_for_each(rdev, mddev)
> +			if (rdev->raid_disk >= 0)
> +				sysfs_unlink_rdev(mddev, rdev);
> +		set_capacity_and_notify(mddev->gendisk, 0);
> +		mddev->changed = 1;
> +		md_new_event();
> +		sysfs_notify_dirent_safe(mddev->sysfs_state);
> +		goto out_unlock;
> +	}
>   	set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
>   	if (!mddev->thread)
>   		md_update_sb(mddev, 1);

-- 
Thanks,
Kuai

Re: [PATCH v3 1/4] md/raid5: Hide the origin mddev->thread before takeover

[Zhihao Cheng]

在 2026/10/9 12:31, yu kuai 写道:
> Hi,
> 
> 在 2026/9/23 19:21, Zhihao Cheng 写道:
>> The raid5 takeover invokes setup_conf and allocates strip heads, but
>> it wakes up the wrong thread, which lefts strip heads in the list
>> 'conf->released_stripes' and not being processed. If raid5_run fails,
>> the strip heads won't be released, which triggers the following slab
>> warnings (CONFIG_SLUB_DEBUG):
>>    BUG raid5-md0 (Not tainted): Objects remaining on __kmem_cache_shutdown()
>>    Object 0x0000000062fad548 @offset=3968
>>    Object 0x000000007f74683c @offset=4960
>>    WARNING: mm/slub.c:1268 at __slab_err+0x31/0x40, CPU#0: bash/865
>>    RIP: 0010:__slab_err+0x31
>>    Call Trace:
>>     __kmem_cache_shutdown.cold+0x15b
>>     kmem_cache_destroy+0x71
>>     free_conf+0xf8
>>     raid5_run.cold+0x463
>>     level_store+0x64e
>>     md_attr_store+0xd7
> 
> Is this still a problem with following patch?
> 
> [PATCH] md/raid5: drain released_stripes before destroying the cache -
> Li Youhong
> <https://lore.kernel.org/all/20260929083916.50620-1-dayou5941@163.com/>
Hi,

Youhong's patch could fix the problem, please take it, his solution is 
better.
> 
>>
>> The detailed triggering process is as follows:
>>    mdadm --create /dev/md0 --level=1 --raid-devices=2 /dev/sda /dev/sdb
>>    --force --assume-clean # create raid1, mddev->thread is raid1d
>>    echo 5 > /sys/block/md0/md/level
>>     level_store
>>      raid5_takeover_raid1
>>        setup_conf
>>         grow_stripes
>>          grow_one_stripe
>>           sh = alloc_stripe
>>           raid5_release_stripe
>>             md_wakeup_thread(conf->mddev->thread) // wakeup raid1d
>>        raid5_run
>>         ENOMEM = raid5_create_ctx_pool
>>         free_conf
>>          shrink_stripes
>>           drop_one_stripe // no strips found from the conf->inactive_list
>>           kmem_cache_destroy(conf->slab_cache)
>> 	 __kmem_cache_shutdown
>> 	  free_partial
>> 	   list_slab_objects // some entries are not released !
>>
>> Fix it by hiding the origin mddev->thread before takeover, so that
>> new allocating strip heads can be put into 'conf->inactive_list',
>> which can be found by drop_one_stripe().
>>
>> Fixes: 773ca82fa1ee ("raid5: make release_stripe lockless")
>> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
>> ---
>>    drivers/md/raid5.c | 37 ++++++++++++++++++++++++++++---------
>>    1 file changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index c091bba95c31..7e87e8a60f5f 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -9039,19 +9039,38 @@ static void *raid5_takeover(struct mddev *mddev)
>>    	 *  raid4 - trivial - just use a raid4 layout.
>>    	 *  raid6 - Providing it is a *_6 layout
>>    	 */
>> -	if (mddev->level == 0)
>> -		return raid45_takeover_raid0(mddev, 5);
>> -	if (mddev->level == 1)
>> -		return raid5_takeover_raid1(mddev);
>> -	if (mddev->level == 4) {
>> +	void *ret = ERR_PTR(-EINVAL);
>> +	struct md_thread *thread;
>> +
>> +	thread = rcu_dereference_protected(mddev->thread,
>> +				lockdep_is_held(&mddev->reconfig_mutex));
>> +	/*
>> +	 * Set mddev->thread to NULL before setup_conf() to avoid waking up
>> +	 * wrong thread(eg. raid1), which can prevent the strips from being
>> +	 * left unreleased in the error handling path(free_conf) of raid5_run.
>> +	 */
>> +	rcu_assign_pointer(mddev->thread, NULL);
>> +
>> +	switch (mddev->level) {
>> +	case 0:
>> +		ret = raid45_takeover_raid0(mddev, 5);
>> +		break;
>> +	case 1:
>> +		ret = raid5_takeover_raid1(mddev);
>> +		break;
>> +	case 4:
>>    		mddev->new_layout = ALGORITHM_PARITY_N;
>>    		mddev->new_level = 5;
>> -		return setup_conf(mddev);
>> +		ret = setup_conf(mddev);
>> +		break;
>> +	case 6:
>> +		ret = raid5_takeover_raid6(mddev);
>> +		break;
>>    	}
>> -	if (mddev->level == 6)
>> -		return raid5_takeover_raid6(mddev);
>>    
>> -	return ERR_PTR(-EINVAL);
>> +	rcu_assign_pointer(mddev->thread, thread);
>> +
>> +	return ret;
>>    }
>>    
>>    static void *raid4_takeover(struct mddev *mddev)
>