* [PATCH v3] md: fix sync_work vs array teardown races
@ 2026-11-10 6:30 Li Youhong
2026-11-10 6:50 ` sashiko-bot
0 siblings, 1 reply; 2+ messages in thread
From: Li Youhong @ 2026-11-10 6:30 UTC (permalink / raw)
To: song, yukuai
Cc: magiclinan, xiao, linux-raid, linux-kernel, Li Youhong,
syzbot+a53c2918175b36cfd601, stable
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 completion/freeze holes that made that wait
unreliable:
- stop_sync_thread() waits for !MD_RECOVERY_RUNNING before teardown,
but md_start_sync() could still use mddev after unlocking. Clear
RUNNING only after those uses are done, then wake_up(&resync_wait).
- 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>
---
v3:
- Clear MD_RECOVERY_RUNNING only after md_start_sync() finishes using
mddev on the not_running path.
- Honour MD_RECOVERY_FROZEN before queueing sync_work on the
read-only md_check_recovery() path.
- v2: link: 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: link: https://lore.kernel.org/linux-raid/20261106083543.249267-1-dayou5941@163.com
drivers/md/md.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 680b34a63cb3..18d37413ffaf 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7104,6 +7104,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);
}
@@ -10343,7 +10350,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
@@ -10355,11 +10361,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);
}
static void unregister_sync_thread(struct mddev *mddev)
@@ -10494,9 +10505,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
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH v3] md: fix sync_work vs array teardown races
2026-11-10 6:30 [PATCH v3] md: fix sync_work vs array teardown races Li Youhong
@ 2026-11-10 6:50 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-11-10 6:50 UTC (permalink / raw)
To: Li Youhong; +Cc: linux-raid, yukuai
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-11-10 6:50 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-10 6:30 [PATCH v3] md: fix sync_work vs array teardown races Li Youhong
2026-11-10 6:50 ` sashiko-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox