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

["李佑鸿 " <dayou5941@163.com>]

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