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

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

[Li Youhong]

From: Li Youhong <liyouhong@kylinos.cn>

md_start_sync() is queued on md_misc_wq to start resync or to
manage spare disks. On a read-only array it calls
remove_and_add_spares(), which ends up in remove_spares() and
invokes pers->hot_remove_disk.

md_start_sync() does not check whether mddev->pers is still
valid before using it. dm-raid stops the array through
md_stop(), which clears pers. If the queued work then runs,
remove_spares() dereferences pers->hot_remove_disk and crashes.
The read-write path has the same exposure in
md_choose_sync_action().

Return via not_running if pers is already gone.

Fixes: db5e653d7c9f ("md: delay choosing sync action 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>
---
 drivers/md/md.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 680b34a63cb3..b7e162f52408 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -10286,6 +10286,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;
+
 	if (!md_is_rdwr(mddev)) {
 		/*
 		 * On a read-only array we can:
-- 
2.25.1

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

[sashiko-bot]

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

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

[yu kuai]

Hi,

在 2026/11/6 16:35, Li Youhong 写道:
> From: Li Youhong <liyouhong@kylinos.cn>
>
> md_start_sync() is queued on md_misc_wq to start resync or to
> manage spare disks. On a read-only array it calls
> remove_and_add_spares(), which ends up in remove_spares() and
> invokes pers->hot_remove_disk.
>
> md_start_sync() does not check whether mddev->pers is still
> valid before using it. dm-raid stops the array through
> md_stop(), which clears pers. If the queued work then runs,
> remove_spares() dereferences pers->hot_remove_disk and crashes.
> The read-write path has the same exposure in
> md_choose_sync_action().
>
> Return via not_running if pers is already gone.
>
> Fixes: db5e653d7c9f ("md: delay choosing sync action 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>
> ---
>   drivers/md/md.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 680b34a63cb3..b7e162f52408 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -10286,6 +10286,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;

The plain NULL checking is not a correct fix, as md_start_sync() should not be running
after md_stop(), I think the root cause is that dm-raid is missing a call like
stop_sync_thread() to flush the sync_work.

> +
>   	if (!md_is_rdwr(mddev)) {
>   		/*
>   		 * On a read-only array we can:

-- 
Thanks,
Kuai