Re: [PATCH] md: fix NULL dereference of pers in md_start_sync()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] md: use-after-free of mddev in md_start_sync() after md_stop()
--

commit 430c0b94e66563acd0f6769283c9a65888eb4dba
Author: Li Youhong <liyouhong@kylinos.cn>

md: fix NULL dereference of pers in md_start_sync()

This commit adds a NULL check for mddev->pers in md_start_sync() to bail
out via not_running if md_stop() has already cleared pers.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 03e35386a8028..9cafa8d673e82 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -10312,6 +10312,13 @@ static void md_start_sync(struct work_struct *ws)
>  		noio_flags = memalloc_noio_save();
>  	}
>  
> +	/*
> +	 * md_stop() clears pers. This work may have been queued earlier and
> +	 * can run after that, so pers may already be NULL here.
> +	 */
> +	if (!mddev->pers)
> +		goto not_running;

[Severity: High]
Can md_start_sync() still use mddev after it is freed when dm-raid calls
md_stop()?

Standard md teardown in do_md_stop() calls stop_sync_thread(), which flushes
mddev->sync_work before __md_stop() clears mddev->pers.  In contrast,
dm-raid's raid_dtr() calls md_stop(), unlocks reconfig_mutex, flushes only
event_work, and immediately destroys and frees rs without cancelling or
flushing sync_work:

CPU0                                  CPU1 (kworker)
raid_dtr()                            md_start_sync()
  mddev_lock_nointr(&rs->md)
  md_stop(&rs->md)
    __md_stop(&rs->md)
      mddev->pers = NULL
  mddev_unlock(&rs->md)
                                        mddev_lock_nointr(mddev)
                                        if (!mddev->pers)
                                          goto not_running;
                                        ...
                                        mddev_unlock(mddev)
  raid_set_free(rs)
    mddev_destroy(&rs->md)
      percpu_ref_exit(&mddev->active_io)
    kfree(rs)
                                        __mddev_resume(mddev, false)
                                        test_and_clear_bit(..., &mddev->recovery)
                                          <-- use-after-free

If md_start_sync() starts after raid_dtr() unlocks reconfig_mutex, it can
also access freed mddev memory before reaching the !mddev->pers check, and
the check itself sits after the conditional unlock-and-suspend block that
can drop reconfig_mutex and suspend an already-stopped array.

Should md_stop() (or __md_stop_writes()) flush or cancel mddev->sync_work
before __md_stop() clears mddev->pers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106083543.249267-1-dayou5941@163.com?part=1