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

[Bruce Johnston <bjohnsto@redhat.com>]

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