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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] md: NULL pointer dereference in md_handle_request() after pers->run() fails
--

commit 3549464235e3de415ce8ceb834293e195eafad7d
Author: Zhihao Cheng <chengzhihao1@huawei.com>

md: Handle pers->run failure in level_store
    
This commit adds error handling for the pers->run(mddev) call in
level_store(). When pers->run() fails, it stops the array via __md_stop()
and unlinks sysfs nodes before unlocking and returning.

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

[ ... ]

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

[Severity: High]
Can this trigger a NULL pointer dereference in md_handle_request() for
concurrent bios?

If pers->run() fails, __md_stop() sets mddev->pers to NULL. The error path
then jumps to out_unlock, which subsequently calls mddev_unlock_and_resume().

When the array is resumed, mddev_unlock_and_resume() resurrects active_io
and wakes up mddev->sb_wait. If there are concurrent bios submitted to the
array, they may be blocked in md_handle_request() waiting on sb_wait:

drivers/md/md.c:md_handle_request() {
	...
		if (is_suspended(mddev, bio)) {
			/* Bail out if REQ_NOWAIT is set for the bio */
			if (bio->bi_opf & REQ_NOWAIT) {
				bio_wouldblock_error(bio);
				return true;
			}
			wait_event(mddev->sb_wait, !is_suspended(mddev, bio));
		}
		if (!percpu_ref_tryget_live(&mddev->active_io))
			goto check_suspended;
	}
	if (!mddev->pers->make_request(mddev, bio)) {
	...
}

Once woken up, they will re-acquire active_io and unconditionally dereference
mddev->pers->make_request. Since mddev->pers was set to NULL by __md_stop(),
will this result in a kernel panic?

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

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