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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] md: race and hang in stop_sync_thread() after unlocking
  before checking RUNNING
- [Medium] md: uninhibited event_work queueing in md_stop() causes UAF
  in raid_ctr()
- [Medium] md: stop_sync_thread() drops reconfig_mutex in do_md_stop()
  when pers is NULL
--

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

md: fix sync_work vs array teardown races

This commit stops the sync thread in md_stop(), flushes sync_work
unconditionally in stop_sync_thread(), and checks MD_RECOVERY_FROZEN
before queueing sync_work on the read-only path in md_check_recovery().

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 03e35386a8028..5badcffcffb6b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5199,22 +5199,22 @@ static void stop_sync_thread(struct mddev *mddev, bool locked)
>  {
>  	int sync_seq = atomic_read(&mddev->sync_seq);
>  
> -	if (!test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
> -		if (!locked)
> -			mddev_unlock(mddev);
> -		return;
> -	}
> -
>  	mddev_unlock(mddev);

[Severity: Medium]
Can dropping reconfig_mutex unconditionally here let do_md_stop() tear
down a newly started and opened array without MD_CLOSING set or the array
suspended?

When array_state_store() handles "clear" while mddev->pers is NULL, it
skips mddev_set_closing_and_sync_blockdev(), takes reconfig_mutex without
suspending the array, and calls do_md_stop(mddev, 0).  Previously,
stop_sync_thread() saw MD_RECOVERY_RUNNING clear and returned without
releasing reconfig_mutex.  Now it drops reconfig_mutex before do_md_stop()
checks mddev->pers or sets MD_DELETED:

CPU0                              CPU1
array_state_store("clear")
  /* mddev->pers is NULL, skips MD_CLOSING */
  mddev_lock(mddev)
  do_md_stop(mddev, 0)
    stop_sync_thread(mddev, true)
      mddev_unlock(mddev)
                                  do_md_run(mddev)
                                    md_run(mddev)
                                      mddev->pers = pers
                                  md_open()
      mddev_lock_nointr(mddev)
    if (mddev->pers)
      __md_stop_writes(mddev)
      __md_stop(mddev)            <-- stops active, open, unsuspended array

Tearing down the active array while I/O is in flight can cause a NULL
pointer dereference or use-after-free on mddev->private.

> -	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
> +	if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
> +		set_bit(MD_RECOVERY_INTR, &mddev->recovery);
> +		/*
> +		 * Thread might be blocked waiting for metadata update which
> +		 * will now never happen
> +		 */
> +		md_wakeup_thread_directly(&mddev->sync_thread);
> +	}
>  	/*
> -	 * Thread might be blocked waiting for metadata update which will now
> -	 * never happen
> +	 * md_start_sync() may still use mddev after unlocking. Always
> +	 * flush so teardown waits for that work to return; checking
> +	 * work_pending() is not enough once the work is running.
>  	 */
> -	md_wakeup_thread_directly(&mddev->sync_thread);
> -	if (work_pending(&mddev->sync_work))
> -		flush_work(&mddev->sync_work);
> +	flush_work(&mddev->sync_work);
>  
>  	wait_event(resync_wait,
>  		   !test_bit(MD_RECOVERY_RUNNING, &mddev->recovery) ||

[Severity: Medium]
Can unlocking reconfig_mutex before checking MD_RECOVERY_RUNNING cause
md_idle_sync_thread() to block for the entire duration of a newly started
resync, or abort a new sync thread?

md_idle_sync_thread() clears MD_RECOVERY_FROZEN before calling
stop_sync_thread(mddev, true).  If MD_RECOVERY_RUNNING is 0 and
MD_RECOVERY_NEEDED is 1 on entry (for example via raid_message() ->
md_idle_sync_thread()), mddev_unlock() wakes mddev->thread, which can now
start a new resync before stop_sync_thread() reaches wait_event():

CPU0                              CPU1 (mddev->thread)
md_idle_sync_thread(mddev)
  clear_bit(MD_RECOVERY_FROZEN, ...)
  stop_sync_thread(mddev, true)
    sync_seq = atomic_read(&mddev->sync_seq)
    mddev_unlock(mddev)
    if (test_bit(MD_RECOVERY_RUNNING, ...)) /* false */
                                  md_check_recovery(mddev)
                                    set_bit(MD_RECOVERY_RUNNING, ...)
                                    queue_work(..., &mddev->sync_work)
    flush_work(&mddev->sync_work)
      /* md_start_sync() registers sync_thread */
    wait_event(resync_wait, ...)  <-- blocks until entire resync finishes

Because no previous sync_thread was reaped, mddev->sync_seq has not
changed and MD_RECOVERY_RUNNING is now set, so wait_event() blocks until
the newly started resync completes across the whole array.  The same hang
happens if MD_RECOVERY_RUNNING was 1 on entry for a sync_work that exited
via not_running in md_start_sync() without incrementing sync_seq, and
md_check_recovery() starts a new sync before wait_event().  Conversely, if
md_check_recovery() reaps a finished sync_thread and starts a new one
between mddev_unlock() and set_bit(MD_RECOVERY_INTR, ...),
stop_sync_thread() sets MD_RECOVERY_INTR on the new sync_thread and aborts
it.

> @@ -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 calling stop_sync_thread() in md_stop() cause a use-after-free on
struct raid_set when raid_ctr() fails?

In raid_ctr(), md_run() spawns rs->md.thread and sets MD_RECOVERY_NEEDED
on the read-only array while holding reconfig_mutex.  If a later step in
raid_ctr() fails and jumps to bad_unlock, md_stop() now calls
stop_sync_thread(), which drops reconfig_mutex and wakes rs->md.thread.
Even with MD_RECOVERY_FROZEN set, the read-only branch of
md_check_recovery() still calls md_reap_sync_thread(), which queues
mddev->event_work (do_table_event()):

CPU0                              CPU1 (rs->md.thread)
raid_ctr()
  ...
  goto bad_unlock
  md_stop(&rs->md)
    set_bit(MD_RECOVERY_FROZEN, ...)
    stop_sync_thread(&rs->md, true)
      mddev_unlock(&rs->md)
                                  md_check_recovery(&rs->md)
                                    md_reap_sync_thread(&rs->md)
                                      queue_work(..., &rs->md.event_work)
      mddev_lock_nointr(&rs->md)
    __md_stop(&rs->md)
  mddev_unlock(&rs->md)
  raid_set_free(rs)
    kfree(rs)
                                  do_table_event(&rs->md.event_work)
                                    dm_table_event(rs->ti->table) <-- use-after-free

raid_ctr()'s error path frees rs without flushing rs->md.event_work (and
raid_dtr() only checks work_pending(&rs->md.event_work), which is false
once the work callback starts running).

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