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

[sashiko-bot@kernel.org]

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