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

[PATCH v5] 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(). Because md_start_sync() may still
use mddev after unlocking / clearing MD_RECOVERY_RUNNING, flush
sync_work whenever it is busy rather than only when work_pending().

Calling stop_sync_thread() from md_stop() can drop reconfig_mutex,
which introduces two follow-on issues:

- When nothing is in flight, keep reconfig_mutex if the caller held
  it (gate the idle early-return with work_busy()). Otherwise clear
  of an inactive array can race with do_md_run().

- While the mutex is dropped, mddev->thread may run
  md_reap_sync_thread() and queue event_work. Flush event_work on
  the raid_ctr() error path before raid_set_free().

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

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>
---
v5:
- Restore idle early-return in stop_sync_thread() gated by work_busy(),
  so locked callers keep reconfig_mutex and do not race with do_md_run().
- Flush event_work before raid_set_free() on the raid_ctr error path
  (md_stop() may drop the mutex and let md_reap queue it).
- v4: https://lore.kernel.org/linux-raid/20261111015027.4029184-1-dayou5941@163.com

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/dm-raid.c |  7 +++++++
 drivers/md/md.c      | 41 ++++++++++++++++++++++++++++++++---------
 2 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index ac019a0f5a92..f82c09d858de 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -3319,6 +3319,13 @@ static int raid_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	md_stop(&rs->md);
 	mddev_unlock(&rs->md);
 bad:
+	/*
+	 * md_stop() may drop reconfig_mutex so mddev->thread can run
+	 * md_reap_sync_thread() and queue event_work; wait for it before
+	 * freeing raid_set. Skip if INIT_WORK has not run yet.
+	 */
+	if (rs->md.event_work.func)
+		flush_work(&rs->md.event_work);
 	raid_set_free(rs);
 
 	return r;
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 680b34a63cb3..79c564320f18 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5184,7 +5184,14 @@ 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 nothing is in flight, keep reconfig_mutex when locked so
+	 * callers such as do_md_stop() cannot race with do_md_run().
+	 * work_busy() is required: md_start_sync() may clear RUNNING and
+	 * still use mddev after unlocking.
+	 */
+	if (!test_bit(MD_RECOVERY_RUNNING, &mddev->recovery) &&
+	    !work_busy(&mddev->sync_work)) {
 		if (!locked)
 			mddev_unlock(mddev);
 		return;
@@ -5192,14 +5199,20 @@ static void stop_sync_thread(struct mddev *mddev, bool locked)
 
 	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 +7114,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 +10515,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 v5] md: fix sync_work vs array teardown races

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] dm-raid: use-after-free in raid_dtr() due to work_pending()
  check before flush_work()
- [Medium] md: stop_sync_thread() hangs on new sync when md_start_sync()
  aborts
--

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

md: fix sync_work vs array teardown races

This commit stops the sync thread and flushes sync_work during md_stop(),
flushes event_work on the raid_ctr() error path, and checks
MD_RECOVERY_FROZEN on the read-only path in md_check_recovery().

> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> index ac019a0f5a929..f82c09d858de3 100644
> --- a/drivers/md/dm-raid.c
> +++ b/drivers/md/dm-raid.c
> @@ -3319,6 +3319,13 @@ static int raid_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  	md_stop(&rs->md);
>  	mddev_unlock(&rs->md);
>  bad:
> +	/*
> +	 * md_stop() may drop reconfig_mutex so mddev->thread can run
> +	 * md_reap_sync_thread() and queue event_work; wait for it before
> +	 * freeing raid_set. Skip if INIT_WORK has not run yet.
> +	 */
> +	if (rs->md.event_work.func)
> +		flush_work(&rs->md.event_work);
>  	raid_set_free(rs);
>  
>  	return r;

[Severity: High]
Does raid_dtr() also need an unconditional flush_work(&rs->md.event_work)
instead of gating it on work_pending()?

drivers/md/dm-raid.c:raid_dtr() {
	...
	if (work_pending(&rs->md.event_work))
		flush_work(&rs->md.event_work);
	raid_set_free(rs);
}

When md_stop() drops reconfig_mutex in raid_dtr(), mddev->thread can run
md_reap_sync_thread() and queue event_work.  Once a worker dequeues
event_work to run do_table_event(), WORK_STRUCT_PENDING_BIT is cleared:

CPU0                              kworker (md_misc_wq)
raid_dtr()
  md_stop(&rs->md)
    stop_sync_thread()
      mddev_unlock()
      /* md_reap_sync_thread()
         queues event_work */
                                  process_one_work()
                                    /* clears PENDING_BIT */
                                    do_table_event()
  if (work_pending(&event_work))
    /* false, skips flush_work */
  raid_set_free(rs)
    kfree(rs)
                                      dm_table_event(rs->ti->table)
                                        <-- use-after-free

Calling flush_work(&rs->md.event_work) unconditionally in raid_dtr() avoids
freeing rs while do_table_event() is still running.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 03e35386a8028..a33075f1b8324 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5199,7 +5199,14 @@ 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 nothing is in flight, keep reconfig_mutex when locked so
> +	 * callers such as do_md_stop() cannot race with do_md_run().
> +	 * work_busy() is required: md_start_sync() may clear RUNNING and
> +	 * still use mddev after unlocking.
> +	 */
> +	if (!test_bit(MD_RECOVERY_RUNNING, &mddev->recovery) &&
> +	    !work_busy(&mddev->sync_work)) {
>  		if (!locked)
>  			mddev_unlock(mddev);
>  		return;
> @@ -5207,14 +5214,20 @@ static void stop_sync_thread(struct mddev *mddev, bool locked)
>  
>  	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) ||

[Severity: Medium]
Can stop_sync_thread() block in wait_event() for the entire duration of a
newly started sync when an in-flight md_start_sync() exits via not_running?

Unlike md_reap_sync_thread(), the not_running path in md_start_sync() clears
MD_RECOVERY_RUNNING and unlocks reconfig_mutex without incrementing
mddev->sync_seq:

drivers/md/md.c:md_start_sync() {
	...
not_running:
	clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
	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);
	...
}

When a caller such as raid_message() -> md_idle_sync_thread() enters
stop_sync_thread() with MD_RECOVERY_FROZEN clear while sync_work is still
busy on the not_running path, stop_sync_thread() drops reconfig_mutex and
waits in flush_work(&mddev->sync_work).  Once md_start_sync() unlocks,
mddev->thread can run md_check_recovery() and start a new sync before
stop_sync_thread() reaches wait_event():

CPU0 (raid_message "idle")        kworker               mddev->thread
md_idle_sync_thread()
  stop_sync_thread()
    sync_seq = ...
    /* work_busy(sync_work) */
    mddev_unlock()
    flush_work(&sync_work)
                                  md_start_sync()
                                  not_running:
                                    clear RUNNING
                                    mddev_unlock()
                                                        md_check_recovery()
                                                          set RUNNING
                                                          clear INTR
                                                          queue sync_work
    wait_event(resync_wait, ...)
      <-- blocks until new sync finishes

Because MD_RECOVERY_RUNNING is set again, MD_RECOVERY_INTR was cleared by
md_check_recovery(), and sync_seq never changed, stop_sync_thread() waits
for the new sync operation to complete instead of returning once the
aborted sync_work finishes.

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

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

[李佑鸿]

At 2026-11-12 10:07:37, sashiko-bot@kernel.org wrote:
>Thank you for your contribution! Sashiko AI review found 2 potential
>issues to consider:
>- [High] dm-raid: use-after-free in raid_dtr() due to work_pending()
>  check before flush_work()
>- [Medium] md: stop_sync_thread() hangs on new sync when md_start_sync()
>  aborts
>--
>
>commit 232add857e22becb764a00281d703c45b875fced
>Author: Li Youhong <liyouhong@kylinos.cn>
>
>md: fix sync_work vs array teardown races
>
>This commit stops the sync thread and flushes sync_work during md_stop(),
>flushes event_work on the raid_ctr() error path, and checks
>MD_RECOVERY_FROZEN on the read-only path in md_check_recovery().
>
>> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
>> index ac019a0f5a929..f82c09d858de3 100644
>> --- a/drivers/md/dm-raid.c
>> +++ b/drivers/md/dm-raid.c
>> @@ -3319,6 +3319,13 @@ static int raid_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>>  	md_stop(&rs->md);
>>  	mddev_unlock(&rs->md);
>>  bad:
>> +	/*
>> +	 * md_stop() may drop reconfig_mutex so mddev->thread can run
>> +	 * md_reap_sync_thread() and queue event_work; wait for it before
>> +	 * freeing raid_set. Skip if INIT_WORK has not run yet.
>> +	 */
>> +	if (rs->md.event_work.func)
>> +		flush_work(&rs->md.event_work);
>>  	raid_set_free(rs);
>>  
>>  	return r;
>
>[Severity: High]
>Does raid_dtr() also need an unconditional flush_work(&rs->md.event_work)
>instead of gating it on work_pending()?
>
>drivers/md/dm-raid.c:raid_dtr() {
>	...
>	if (work_pending(&rs->md.event_work))
>		flush_work(&rs->md.event_work);
>	raid_set_free(rs);
>}
>
>When md_stop() drops reconfig_mutex in raid_dtr(), mddev->thread can run
>md_reap_sync_thread() and queue event_work.  Once a worker dequeues
>event_work to run do_table_event(), WORK_STRUCT_PENDING_BIT is cleared:
>
>CPU0                              kworker (md_misc_wq)
>raid_dtr()
>  md_stop(&rs->md)
>    stop_sync_thread()
>      mddev_unlock()
>      /* md_reap_sync_thread()
>         queues event_work */
>                                  process_one_work()
>                                    /* clears PENDING_BIT */
>                                    do_table_event()
>  if (work_pending(&event_work))
>    /* false, skips flush_work */
>  raid_set_free(rs)
>    kfree(rs)
>                                      dm_table_event(rs->ti->table)
>                                        <-- use-after-free
>
>Calling flush_work(&rs->md.event_work) unconditionally in raid_dtr() avoids
>freeing rs while do_table_event() is still running.
>
Agreed. raid_dtr() should not rely on work_pending() because the work may already be
running with the PENDING bit cleared. I will send a separate patch to flush event_work
whenever it has been initialized, aligned with the same check in raid_ctr().

>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 03e35386a8028..a33075f1b8324 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -5199,7 +5199,14 @@ 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 nothing is in flight, keep reconfig_mutex when locked so
>> +	 * callers such as do_md_stop() cannot race with do_md_run().
>> +	 * work_busy() is required: md_start_sync() may clear RUNNING and
>> +	 * still use mddev after unlocking.
>> +	 */
>> +	if (!test_bit(MD_RECOVERY_RUNNING, &mddev->recovery) &&
>> +	    !work_busy(&mddev->sync_work)) {
>>  		if (!locked)
>>  			mddev_unlock(mddev);
>>  		return;
>> @@ -5207,14 +5214,20 @@ static void stop_sync_thread(struct mddev *mddev, bool locked)
>>  
>>  	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) ||
>
>[Severity: Medium]
>Can stop_sync_thread() block in wait_event() for the entire duration of a
>newly started sync when an in-flight md_start_sync() exits via not_running?
>
>Unlike md_reap_sync_thread(), the not_running path in md_start_sync() clears
>MD_RECOVERY_RUNNING and unlocks reconfig_mutex without incrementing
>mddev->sync_seq:
>
>drivers/md/md.c:md_start_sync() {
>	...
>not_running:
>	clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
>	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);
>	...
>}
>
>When a caller such as raid_message() -> md_idle_sync_thread() enters
>stop_sync_thread() with MD_RECOVERY_FROZEN clear while sync_work is still
>busy on the not_running path, stop_sync_thread() drops reconfig_mutex and
>waits in flush_work(&mddev->sync_work).  Once md_start_sync() unlocks,
>mddev->thread can run md_check_recovery() and start a new sync before
>stop_sync_thread() reaches wait_event():
>
>CPU0 (raid_message "idle")        kworker               mddev->thread
>md_idle_sync_thread()
>  stop_sync_thread()
>    sync_seq = ...
>    /* work_busy(sync_work) */
>    mddev_unlock()
>    flush_work(&sync_work)
>                                  md_start_sync()
>                                  not_running:
>                                    clear RUNNING
>                                    mddev_unlock()
>                                                        md_check_recovery()
>                                                          set RUNNING
>                                                          clear INTR
>                                                          queue sync_work
>    wait_event(resync_wait, ...)
>      <-- blocks until new sync finishes
>
>Because MD_RECOVERY_RUNNING is set again, MD_RECOVERY_INTR was cleared by
>md_check_recovery(), and sync_seq never changed, stop_sync_thread() waits
>for the new sync operation to complete instead of returning once the
>aborted sync_work finishes.
>

Agreed. The sync_seq-based wait condition can cause md_idle_sync_thread() to block until a newly
started sync completes. I will send a separate patch to simplify the wait condition and remove
the now-unused sync_seq field.

Both issues are independent of the core v5 fix and will be addressed in separate patches sent to the
respective maintainers.


Thanks,
Li Youhong