* Re: [PATCH v4] md: fix sync_work vs array teardown races
2026-11-11 1:50 [PATCH v4] md: fix sync_work vs array teardown races Li Youhong
@ 2026-11-11 3:02 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-11-11 3:02 UTC (permalink / raw)
To: Li Youhong; +Cc: linux-raid, yukuai
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
^ permalink raw reply [flat|nested] 2+ messages in thread