Re: [PATCH] md: drain active_io before clearing pers on stop

[yu kuai]

Hi,

在 2026/10/3 18:19, Li Youhong 写道:
> From: Li Youhong <liyouhong@xxxxxxxxxx>
>
> STOP_ARRAY clears mddev->pers while a direct bio can already be in
> md_handle_request(). fork() shares the block-device file, so openers
> stays 1 and the stop is allowed, and sync_blockdev() does not wait for
> O_DIRECT. The bio passes the pers check in md_submit_bio(), then pers
> is cleared and md_handle_request() dereferences pers->make_request.
>
> Suspend the array before __md_stop() so percpu_ref_kill(active_io) makes
> later tryget_live fail and the wait runs until every make_request holder
> has dropped the ref. The suspend taken here is resumed after pers is
> cleared, which wakes bios blocked in md_handle_request(). They see
> pers == NULL and complete with bio_io_error(). dm-raid already holds
> its own suspend across md_stop() and that path is unchanged.
>
> reconfig_mutex is dropped around mddev_suspend() and mddev_resume().
> stop_draining is set across that window so mddev_lock() returns
> -EBUSY.
>
> Reported-by: syzbot+89a9ab2a134d092fffae@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=89a9ab2a134d092fffae
> Fixes: 409c57f38017 ("md: enable suspend/resume of md devices.")
> Signed-off-by: Li Youhong <liyouhong@xxxxxxxxxx>
> ---
>   drivers/md/md.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++---
>   drivers/md/md.h |  7 ++++++
>   2 files changed, 66 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 680b34a63cb3..85158a99277f 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -395,6 +395,8 @@ static bool is_suspended(struct mddev *mddev, struct bio *bio)
>   
>   bool md_handle_request(struct mddev *mddev, struct bio *bio)
>   {
> +	struct md_personality *pers;
> +
>   check_suspended:
>   	if (unlikely(md_cloned_bio(mddev, bio))) {
>   		/*
> @@ -402,6 +404,12 @@ bool md_handle_request(struct mddev *mddev, struct bio *bio)
>   		 * active_io reference, so percpu_ref_get() is safe here.
>   		 */
>   		percpu_ref_get(&mddev->active_io);
> +		pers = READ_ONCE(mddev->pers);
> +		if (!pers) {
> +			percpu_ref_put(&mddev->active_io);
> +			bio_io_error(bio);
> +			return true;
> +		}
>   	} else {
>   		if (is_suspended(mddev, bio)) {
>   			/* Bail out if REQ_NOWAIT is set for the bio */
> @@ -409,14 +417,20 @@ bool md_handle_request(struct mddev *mddev, struct bio *bio)
>   				bio_wouldblock_error(bio);
>   				return true;
>   			}
> -			wait_event(mddev->sb_wait, !is_suspended(mddev, bio));
> +			wait_event(mddev->sb_wait, !is_suspended(mddev, bio) ||
> +				   !READ_ONCE(mddev->pers));
> +		}
> +		pers = READ_ONCE(mddev->pers);
> +		if (!pers) {
> +			bio_io_error(bio);
> +			return true;
>   		}
>   		if (!percpu_ref_tryget_live(&mddev->active_io))
>   			goto check_suspended;
>   	}
> -	if (!mddev->pers->make_request(mddev, bio)) {
> +	if (!pers->make_request(mddev, bio)) {
>   		percpu_ref_put(&mddev->active_io);
> -		if (mddev_is_dm(mddev) && mddev->pers->prepare_suspend)
> +		if (mddev_is_dm(mddev) && pers->prepare_suspend)
>   			return false;
>   		goto check_suspended;
>   	}
> @@ -7097,6 +7111,32 @@ static void __md_stop(struct mddev *mddev)
>   	clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>   }
>   
> +/* Drop reconfig_mutex around suspend so in-flight IO can update the superblock. */
> +static int md_suspend_for_stop(struct mddev *mddev, bool interruptible)
> +{
> +	int err;
> +
> +	lockdep_assert_held(&mddev->reconfig_mutex);
> +
> +	WRITE_ONCE(mddev->stop_draining, 1);
> +	mutex_unlock(&mddev->reconfig_mutex);

Creating a new wheel will just make code more complicated. Check following patch for the
correct fix:

[PATCH v5] md: Fix the null-ptr-deref of 'mddev->private' while 
submitting IO - Zhihao Cheng <https://lore.kernel.org/all/20260922030538.1634904-1-chengzhihao1@xxxxxxxxxx/>

> +	err = mddev_suspend(mddev, interruptible);
> +	mddev_lock_nointr(mddev);
> +	WRITE_ONCE(mddev->stop_draining, 0);
> +	return err;
> +}
> +
> +static void md_resume_for_stop(struct mddev *mddev)
> +{
> +	lockdep_assert_held(&mddev->reconfig_mutex);
> +
> +	WRITE_ONCE(mddev->stop_draining, 1);
> +	mutex_unlock(&mddev->reconfig_mutex);
> +	__mddev_resume(mddev, false);
> +	mddev_lock_nointr(mddev);
> +	WRITE_ONCE(mddev->stop_draining, 0);
> +}
> +
>   void md_stop(struct mddev *mddev)
>   {
>   	lockdep_assert_held(&mddev->reconfig_mutex);
> @@ -7164,6 +7204,8 @@ static int do_md_stop(struct mddev *mddev, int mode)
>   	struct gendisk *disk = mddev->gendisk;
>   	struct md_rdev *rdev;
>   	int did_freeze = 0;
> +	int err = 0;
> +	int resume = 0;
>   
>   	if (!test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) {
>   		did_freeze = 1;
> @@ -7181,6 +7223,18 @@ static int do_md_stop(struct mddev *mddev, int mode)
>   		}
>   		return -EBUSY;
>   	}
> +
> +	if (mddev->pers) {
> +		err = md_suspend_for_stop(mddev, true);
> +		if (err) {
> +			if (did_freeze) {
> +				clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
> +				set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> +			}
> +			return err;
> +		}
> +		resume = 1;
> +	}
>   	if (mddev->pers) {
>   		if (!md_is_rdwr(mddev))
>   			set_disk_ro(disk, 0);
> @@ -7205,6 +7259,8 @@ static int do_md_stop(struct mddev *mddev, int mode)
>   		if (!md_is_rdwr(mddev))
>   			mddev->ro = MD_RDWR;
>   	}
> +	if (resume)
> +		md_resume_for_stop(mddev);
>   	/*
>   	 * Free resources if final stop
>   	 */
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index b6d2e8929a0f..745f0c57f669 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -414,6 +414,8 @@ struct mddev {
>   	unsigned long			sb_flags;
>   
>   	int				suspended;
> +	/* Set while do_md_stop() has dropped reconfig_mutex to drain IO. */
> +	int				stop_draining;
>   	struct mutex			suspend_mutex;
>   	struct percpu_ref		active_io;
>   	int				ro;
> @@ -719,6 +721,11 @@ static inline int __must_check mddev_lock(struct mddev *mddev)
>   		ret = -ENODEV;
>   		mutex_unlock(&mddev->reconfig_mutex);
>   	}
> +	/* do_md_stop() is waiting for active_io with the mutex dropped. */
> +	if (!ret && READ_ONCE(mddev->stop_draining)) {
> +		ret = -EBUSY;
> +		mutex_unlock(&mddev->reconfig_mutex);
> +	}
>   
>   	return ret;
>   }

-- 
Thanks,
Kuai