Re: [PATCH v2] md: stop sync_work in md_stop() before clearing pers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] md: stop_sync_thread() in md_stop() races with running
  md_start_sync()
- [Medium] md: md_check_recovery() can re-queue sync_work during
  md_stop()
--

commit 1d5026b108bd24d6a0ff2d1fe48dedbb1f84d4d9
Author: Li Youhong <liyouhong@kylinos.cn>

md: stop sync_work in md_stop() before clearing pers

This commit sets MD_RECOVERY_FROZEN and calls stop_sync_thread() in
md_stop() so that pending sync_work finishes before __md_stop() clears
mddev->pers.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 03e35386a8028..6f0fb2bb1fc2b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7130,6 +7130,14 @@ void md_stop(struct mddev *mddev)
>  {
>  	lockdep_assert_held(&mddev->reconfig_mutex);
>  
> +	/*
> +	 * md_check_recovery() may have queued sync_work; that work accesses
> +	 * pers. Wait for it here before __md_stop() clears pers, as
> +	 * do_md_stop()/md_stop_writes() do.
> +	 */
> +	set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
> +	stop_sync_thread(mddev, true);

[Severity: Medium]
Can stop_sync_thread() return while md_start_sync() is still accessing
mddev after dropping reconfig_mutex?

Once a worker dequeues mddev->sync_work and starts md_start_sync(),
work_pending(&mddev->sync_work) is false, so stop_sync_thread() skips
flush_work(&mddev->sync_work).  In the not_running path of
md_start_sync() (such as on a read-only dm-raid array), md_start_sync()
clears MD_RECOVERY_RUNNING and drops reconfig_mutex before resuming the
device and touching mddev:

CPU0 (raid_dtr)                   CPU1 (kworker: md_start_sync)
                                  clear_bit(MD_RECOVERY_RUNNING, ...)
                                  mddev_unlock(mddev)
mddev_lock_nointr(&rs->md)
md_stop(&rs->md)
  stop_sync_thread(mddev, true)
    !test_bit(MD_RECOVERY_RUNNING)
    return
  __md_stop(mddev)
mddev_unlock(&rs->md)
raid_set_free(rs)
  kfree(rs)
                                  __mddev_resume(mddev, false)
                                  test_and_clear_bit(..., &mddev->recovery)
                                    <-- use-after-free

Unconditionally calling flush_work(&mddev->sync_work) after
__md_stop_writes() or after mddev_detach() stops mddev->thread would
ensure md_start_sync() has completely finished before raid_dtr() frees
struct raid_set.

[Severity: Medium]
Can md_check_recovery() re-queue mddev->sync_work while
stop_sync_thread() drops reconfig_mutex in md_stop()?

stop_sync_thread() is called before __md_stop() -> mddev_detach() stops
mddev->thread.  When MD_RECOVERY_RUNNING is set on entry,
stop_sync_thread() unlocks reconfig_mutex (waking mddev->thread) and
waits for MD_RECOVERY_RUNNING to clear before re-locking reconfig_mutex.

On a read-only array (such as dm-raid during raid_dtr() after
raid_postsuspend()), the !md_is_rdwr(mddev) branch of
md_check_recovery() does not check MD_RECOVERY_FROZEN before queuing
sync_work:

drivers/md/md.c:md_check_recovery() {
    ...
		if (!md_is_rdwr(mddev)) {
			...
			if (md_spares_need_change(mddev)) {
				set_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
				queue_work(md_misc_wq, &mddev->sync_work);
			}
    ...
}

This allows mddev->thread to queue a new sync_work before
stop_sync_thread() re-acquires reconfig_mutex:

CPU0 (raid_dtr)                   CPU1 (mddev->thread)
md_stop(&rs->md)
  set_bit(MD_RECOVERY_FROZEN, ...)
  stop_sync_thread(mddev, true)
    mddev_unlock(mddev)
    wait_event(resync_wait, ...)
                                  /* previous sync finishes */
                                  md_check_recovery()
                                    mddev_trylock(mddev)
                                    set_bit(MD_RECOVERY_RUNNING, ...)
                                    queue_work(..., &mddev->sync_work)
                                    mddev_unlock(mddev)
    mddev_lock_nointr(mddev)
  __md_stop(mddev)
    mddev->pers = NULL
raid_set_free(rs)
                                  /* kworker runs md_start_sync() */
                                  remove_spares()
                                    mddev->pers->hot_remove_disk()
                                      <-- NULL pointer dereference / UAF

Checking MD_RECOVERY_FROZEN in the !md_is_rdwr(mddev) path of
md_check_recovery() (matching the read-write path), or stopping
sync_work after mddev->thread has been stopped, would close this race.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109023055.2164333-1-dayou5941@163.com?part=1