Re: [PATCH v3] md: fix sync_work vs array teardown races

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] md: post-unlock mddev UAF in md_start_sync() on normal sync
  startup path
- [Medium] md: clearing MD_RECOVERY_RUNNING unlocked in md_start_sync()
  loses recovery
--

commit 20468c000d2b07f235e4306894a52f2c0937b056
Author: Li Youhong <liyouhong@kylinos.cn>

md: fix sync_work vs array teardown races

This commit calls stop_sync_thread() in md_stop(), delays clearing
MD_RECOVERY_RUNNING on the not_running path of md_start_sync(), and checks
MD_RECOVERY_FROZEN before queueing sync_work on read-only arrays.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 03e35386a8028..38df5aababac7 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7133,6 +7133,13 @@ void md_stop(struct mddev *mddev)
>  	/* stop the array and free an attached data structures.
>  	 * This is called from dm-raid
>  	 */
> +	/*
> +	 * 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);
>  	__md_stop_writes(mddev);
>  	__md_stop(mddev);
>  }

[Severity: Medium]
Can md_start_sync() still suffer a use-after-free on mddev on its normal
sync startup path?

When md_start_sync() succeeds in registering sync_thread, it unlocks
reconfig_mutex and continues accessing mddev in __mddev_resume(),
md_wakeup_thread(mddev->sync_thread), and
sysfs_notify_dirent_safe(mddev->sysfs_action):

drivers/md/md.c:md_start_sync() {
	...
	mddev_unlock(mddev);
	if (suspend) {
		memalloc_noio_restore(noio_flags);
		__mddev_resume(mddev, false);
	}
	md_wakeup_thread(mddev->sync_thread);
	sysfs_notify_dirent_safe(mddev->sysfs_action);
	md_new_event();
	return;
}

Because process_one_work() clears WORK_STRUCT_PENDING_BIT before invoking
md_start_sync(), work_pending(&mddev->sync_work) is false in
stop_sync_thread(), so flush_work(&mddev->sync_work) is skipped.  Once
sync_thread is woken (either inside __mddev_resume() before unlocking
suspend_mutex, by md_wakeup_thread(), or by md_wakeup_thread_directly() in
stop_sync_thread()), it can exit and be reaped before md_start_sync()
returns:

CPU0 (kworker)                    CPU1 (teardown / md_thread)
md_start_sync()
  rcu_assign_pointer(sync_thread, ...)
  mddev_unlock(mddev)
                                  raid_dtr() -> md_stop()
                                    set_bit(MD_RECOVERY_FROZEN, ...)
                                    stop_sync_thread()
                                      /* work_pending(sync_work) is false */
                                      md_wakeup_thread_directly(sync_thread)
                                  md_do_sync()
                                    /* sees FROZEN/INTR, sets DONE */
                                  md_check_recovery()
                                    md_reap_sync_thread()
                                      clear_bit(MD_RECOVERY_RUNNING, ...)
                                      wake_up(&resync_wait)
                                    /* stop_sync_thread() returns */
                                  raid_set_free()
                                    kfree(rs)
  __mddev_resume(mddev, false)
  md_wakeup_thread(mddev->sync_thread)
    <-- use-after-free on mddev

Calling flush_work(&mddev->sync_work) unconditionally in
stop_sync_thread() after wait_event(resync_wait, ...) would ensure
md_start_sync() has completely returned before array teardown proceeds.

> @@ -10369,7 +10376,6 @@ static void md_start_sync(struct work_struct *ws)
>  	clear_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
>  	clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
>  	clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> -	clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
>  	mddev_unlock(mddev);
>  	/*
>  	 * md_start_sync was triggered by MD_RECOVERY_NEEDED, so we should
> @@ -10381,11 +10387,16 @@ static void md_start_sync(struct work_struct *ws)
>  		memalloc_noio_restore(noio_flags);
>  		__mddev_resume(mddev, false);
>  	}
> -
> -	wake_up(&resync_wait);
>  	if (test_and_clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery) &&
>  	    mddev->sysfs_action)
>  		sysfs_notify_dirent_safe(mddev->sysfs_action);
> +	/*
> +	 * stop_sync_thread() waits for !RUNNING and may then tear down
> +	 * the array. This path still uses mddev after unlocking, so
> +	 * clear RUNNING only after those uses are done.
> +	 */
> +	clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
> +	wake_up(&resync_wait);
>  }

[Severity: Medium]
Does clearing MD_RECOVERY_RUNNING after mddev_unlock() and
__mddev_resume() cause md_check_recovery() to drop pending
MD_RECOVERY_NEEDED events?

Both mddev_unlock() and __mddev_resume() wake mddev->thread after
releasing reconfig_mutex while MD_RECOVERY_RUNNING is still set and
MD_RECOVERY_DONE is clear:

CPU0 (kworker)                    CPU1 (mddev->thread)
md_start_sync()
  /* not_running: */
  mddev_unlock(mddev)
    mutex_unlock(&reconfig_mutex)
    md_wakeup_thread(mddev->thread)
                                  md_check_recovery()
                                    mddev_trylock(mddev)
                                    if (test_bit(MD_RECOVERY_RUNNING, ...))
                                      unregister_sync_thread(mddev)
                                        /* !MD_RECOVERY_DONE */
                                        clear_bit(MD_RECOVERY_NEEDED, ...)
  clear_bit(MD_RECOVERY_RUNNING, ...)

Because no sync_thread is running to later re-set MD_RECOVERY_NEEDED in
md_reap_sync_thread(), any MD_RECOVERY_NEEDED set before that point (for
example by md_error() or spare changes) is permanently lost, stalling
recovery or faulty device removal.  Reconfiguration callers acquiring
reconfig_mutex in that window (such as slot_store() or
remove_and_add_spares()) can also observe MD_RECOVERY_RUNNING still set
and fail with -EBUSY.

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