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

[Zhihao Cheng <chengzhihao1@huawei.com>]

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