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

在 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:

quoted

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

.