* [PATCH v5] md: fix sync_work vs array teardown races
@ 2026-11-12 1:35 Li Youhong
2026-11-12 2:07 ` sashiko-bot
0 siblings, 1 reply; 3+ messages in thread
From: Li Youhong @ 2026-11-12 1:35 UTC (permalink / raw)
To: song, yukuai, agk, snitzer, mpatocka, bmarzins
Cc: dm-devel, 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(). 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v5] md: fix sync_work vs array teardown races
2026-11-12 1:35 [PATCH v5] md: fix sync_work vs array teardown races Li Youhong
@ 2026-11-12 2:07 ` sashiko-bot
2026-11-12 7:43 ` 李佑鸿
0 siblings, 1 reply; 3+ messages in thread
From: sashiko-bot @ 2026-11-12 2:07 UTC (permalink / raw)
To: Li Youhong, song, yukuai, agk, snitzer, mpatocka, bmarzins
Cc: dm-devel, magiclinan, xiao, linux-raid, Li Youhong,
syzbot+a53c2918175b36cfd601
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re:Re: [PATCH v5] md: fix sync_work vs array teardown races
2026-11-12 2:07 ` sashiko-bot
@ 2026-11-12 7:43 ` 李佑鸿
0 siblings, 0 replies; 3+ messages in thread
From: 李佑鸿 @ 2026-11-12 7:43 UTC (permalink / raw)
To: sashiko-reviews
Cc: song, yukuai, agk, snitzer, mpatocka, bmarzins, dm-devel,
magiclinan, xiao, linux-raid, Li Youhong,
syzbot+a53c2918175b36cfd601
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-11-12 7:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-12 1:35 [PATCH v5] md: fix sync_work vs array teardown races Li Youhong
2026-11-12 2:07 ` sashiko-bot
2026-11-12 7:43 ` 李佑鸿
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox