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