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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] md: NULL pointer dereference in md_handle_request() when resuming array after pers->run() failure
- [Medium] md: sysfs group md_redundancy_group leaked on pers->run() failure
--

commit 4ab3c278fcfe04083eb0fb2b801da26eb96c6cec
Author: Zhihao Cheng <chengzhihao1@huawei.com>

md: Handle pers->run failure in level_store

This commit fixes a NULL pointer dereference that occurs when raid_disks
is set after raid5_run() fails during array conversion. It handles the
error by stopping the array if the new personality fails to run, allowing
a subsequent active order to restart it.

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

[ ... ]

> @@ -4229,7 +4230,31 @@ 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 (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);

[Severity: High]
Does calling __md_stop() here introduce a NULL pointer dereference for in-flight IOs?

If level_store() is called while IOs are active, mddev_suspend_and_lock() will
force them to wait in md_handle_request(). __md_stop() sets mddev->pers = NULL.

When the error path later reaches out_unlock and calls mddev_unlock_and_resume(),
will the trapped IOs wake up and unconditionally call mddev->pers->make_request()
while mddev->pers is NULL, panicking the kernel?

> +		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: Medium]
When converting from a non-redundancy array to a redundancy array,
sysfs_create_group() is called earlier in level_store() to add md_redundancy_group.

Does this error path leak the sysfs group and its kernfs_node references, since
mddev->to_remove is not set to &md_redundancy_group to trigger cleanup in
mddev_unlock()?

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

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