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

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

[Li Youhong]

From: Li Youhong <liyouhong@kylinos.cn>

dm-raid tears down arrays via md_stop(), which cleared mddev->pers
without waiting for previously queued sync_work. That work could
then call remove_spares() with pers == NULL, or keep using mddev
after raid_dtr() frees it.

do_md_stop()/md_stop_writes() already call stop_sync_thread() before
teardown. Do the same in md_stop().

Also fix two related holes that made that wait unreliable:

- md_start_sync() may still use mddev after unlocking. Checking
  work_pending() in stop_sync_thread() is not enough once the work
  is running, so always flush_work() and wait for it to return
  before teardown.

- The read-only path in md_check_recovery() queued sync_work without
  honouring MD_RECOVERY_FROZEN, unlike the read-write path. Check
  FROZEN so freeze/stop cannot race with a new queue while
  stop_sync_thread() drops reconfig_mutex.

Fixes: db5e653d7c9f ("md: delay choosing sync action to md_start_sync()")
Fixes: 81e2ce1b3d5a ("md: delay remove_and_add_spares() for read only array to md_start_sync()")
Reported-by: syzbot+a53c2918175b36cfd601@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=a53c2918175b36cfd601
Cc: stable@vger.kernel.org
Signed-off-by: Li Youhong <liyouhong@kylinos.cn>
---
v4:
- Always flush_work(&sync_work) in stop_sync_thread(); do not delay
  clearing MD_RECOVERY_RUNNING (that can drop MD_RECOVERY_NEEDED via
  md_check_recovery, and does not cover the successful sync_thread
  registration path).
- v3: https://lore.kernel.org/linux-raid/20261110063045.912703-1-dayou5941@163.com
v3:
- Honour MD_RECOVERY_FROZEN before queueing sync_work on the
  read-only md_check_recovery() path.
- (superseded) delayed MD_RECOVERY_RUNNING clear on not_running.
- v2: https://lore.kernel.org/linux-raid/20261109023055.2164333-1-dayou5941@163.com
v2:
- Drop the pers NULL check in md_start_sync().
- Call stop_sync_thread() from md_stop() before clearing pers,
  matching do_md_stop()/md_stop_writes().
- v1: https://lore.kernel.org/linux-raid/20261106083543.249267-1-dayou5941@163.com

 drivers/md/md.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 680b34a63cb3..68a72513e884 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5184,22 +5184,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);
 
-	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) ||
@@ -7101,6 +7101,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);
+
 	/* stop the array and free an attached data structures.
 	 * This is called from dm-raid
 	 */
@@ -10494,9 +10502,11 @@ void md_check_recovery(struct mddev *mddev)
 
 			/*
 			 * Let md_start_sync() to remove and add rdevs to the
-			 * array.
+			 * array. Honour MD_RECOVERY_FROZEN like the read-write
+			 * path so freeze/stop does not race with a new queue.
 			 */
-			if (md_spares_need_change(mddev)) {
+			if (md_spares_need_change(mddev) &&
+			    !test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) {
 				set_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
 				queue_work(md_misc_wq, &mddev->sync_work);
 			}
-- 
2.25.1

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

[sashiko-bot]

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