[PATCH] md: Fix the null-ptr-deref of 'mddev->private' while submitting IO

[PATCH] md: Fix the null-ptr-deref of 'mddev->private' while submitting IO

[Zhihao Cheng]

Concurrent processes md_stop and IO submitting could trigger a
null-ptr-deref of 'mddev->private':

 BUG: kernel NULL pointer dereference, address: 0000000000000070
 RIP: 0010:_wait_barrier+0x2f/0x250
 Call Trace:
  raid1_make_request+0x150/0xf50
  md_handle_request+0x104/0x530
  md_submit_bio+0x76/0x130
  submit_bio+0xdd/0x250
  submit_bio_wait+0x1f/0x40
  __blkdev_direct_IO_simple+0x1f6/0x370
  blkdev_write_iter+0x3b2/0x520
  ksys_write+0x7d/0x190

              P1
  fd = open(/dev/md0, O_RDWR)
	                      P2 (forked from P1, fd' <= fd)
  write(fd)
   submit_bio
    md_handle_request
     raid1_make_request
      raid1_write_request
                                  ioctl(fd, STOP_ARRAY)
		                   mddev_set_closing_and_sync_blockdev
				   // check passed, mddev->openers = 1,
				   // because md_open() is only called
				   // once in P1->open
				   do_md_stop
				    __md_stop
				     mddev->private = NULL

       conf = mddev->private // NULL
        wait_barrier(conf, sector) // null-ptr-deref !

It is a common problem for raid0/1/10/5, and __md_stop could be
triggered by several paths(eg. ioctl, sysfs, ->dtr). Fix it by
replacing mddev_lock() with mddev_suspend_and_lock() for all
__md_stop() callers.

Fetch a reproducer in https://bugzilla.kernel.org/show_bug.cgi?id=222020

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+3fe892ea5fc292e1353f@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=3fe892ea5fc292e1353f
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/md/dm-raid.c |  7 +++++--
 drivers/md/md.c      | 18 +++++++++++++++---
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 8f5a5e1342a9..3dda494fe172 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -3326,12 +3326,15 @@ static int raid_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 static void raid_dtr(struct dm_target *ti)
 {
+	unsigned int noio_flags = 0;
 	struct raid_set *rs = ti->private;
 
-	mddev_lock_nointr(&rs->md);
+	mddev_suspend_and_lock_nointr(&rs->md);
+	noio_flags = memalloc_noio_save();
 	md_stop(&rs->md);
 	rs->md.dm_gendisk = NULL;
-	mddev_unlock(&rs->md);
+	memalloc_noio_restore(noio_flags);
+	mddev_unlock_and_resume(&rs->md);
 
 	if (work_pending(&rs->md.event_work))
 		flush_work(&rs->md.event_work);
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 680b34a63cb3..e292acd54bb0 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4657,6 +4657,8 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
 {
 	int err = 0;
 	enum array_state st = match_word(buf, array_states);
+	unsigned int noio_flags = 0;
+	bool suspend = false;
 
 	/* No lock dependent actions */
 	switch (st) {
@@ -4666,9 +4668,11 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
 	case broken:		/* cannot be set */
 	case bad_word:
 		return -EINVAL;
+	case inactive:
 	case clear:
+		suspend = true;
+		fallthrough;
 	case readonly:
-	case inactive:
 	case read_auto:
 		if (!mddev->pers || !md_is_rdwr(mddev))
 			break;
@@ -4702,9 +4706,11 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
 		spin_unlock(&mddev->lock);
 		return err ?: len;
 	}
-	err = mddev_lock(mddev);
+	err = suspend ? mddev_suspend_and_lock(mddev) : mddev_lock(mddev);
 	if (err)
 		return err;
+	if (suspend)
+		noio_flags = memalloc_noio_save();
 
 	switch (st) {
 	case inactive:
@@ -4775,7 +4781,12 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
 			mddev->hold_active = 0;
 		sysfs_notify_dirent_safe(mddev->sysfs_state);
 	}
-	mddev_unlock(mddev);
+	if (suspend) {
+		memalloc_noio_restore(noio_flags);
+		mddev_unlock_and_resume(mddev);
+	} else {
+		mddev_unlock(mddev);
+	}
 
 	if (st == readonly || st == read_auto || st == inactive ||
 	    (err && st == clear))
@@ -8299,6 +8310,7 @@ static bool md_ioctl_need_suspend(unsigned int cmd)
 	case HOT_REMOVE_DISK:
 	case SET_BITMAP_FILE:
 	case SET_ARRAY_INFO:
+	case STOP_ARRAY:
 		return true;
 	default:
 		return false;
-- 
2.52.0

Re: [PATCH] md: Fix the null-ptr-deref of 'mddev->private' while submitting IO

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] md: NULL pointer dereference in md_handle_request() on array teardown
- [High] md: memory reclaim deadlock due to premature memalloc_noio_restore
--

commit 86c9448b538e3d50a408040bfd47956ef0397e1a
Author: Zhihao Cheng <chengzhihao1@huawei.com>

md: Fix the null-ptr-deref of 'mddev->private' while submitting IO

This patch replaces mddev_lock() with mddev_suspend_and_lock() across
__md_stop() callers. It aims to prevent concurrent IO submissions from
dereferencing mddev->private while the array is being torn down.

Closes: https://syzkaller.appspot.com/bug?extid=3fe892ea5fc292e1353f

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 680b34a63cb3d..e292acd54bb00 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
[ ... ]
> @@ -4775,7 +4781,12 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
>  			mddev->hold_active = 0;
>  		sysfs_notify_dirent_safe(mddev->sysfs_state);
>  	}
> -	mddev_unlock(mddev);
> +	if (suspend) {
> +		memalloc_noio_restore(noio_flags);
> +		mddev_unlock_and_resume(mddev);

[Severity: Critical]
Could unconditionally resuming the array here lead to a NULL pointer
dereference for trapped IOs?

If an IO is submitted concurrently with array teardown (e.g. sysfs inactive
or STOP_ARRAY ioctl), it is trapped in md_handle_request() by wait_event()
waiting for the array to not be suspended.

The teardown thread sets mddev->pers = NULL in md_stop() and then calls
mddev_unlock_and_resume() here. The resumption resurrects active_io and wakes
up the trapped IO, which then blindly dereferences mddev->pers:

drivers/md/md.c:md_handle_request() {
        ...
        if (!percpu_ref_tryget_live(&mddev->active_io))
                goto check_suspended;
        }
        if (!mddev->pers->make_request(mddev, bio)) {
        ...
}

[Severity: High]
Is there a risk of a memory reclaim deadlock due to the NOIO restoration order
here?

By restoring NOIO flags before calling mddev_unlock_and_resume(), we run
mddev_unlock() without NOIO protection while the array is still suspended:

drivers/md/md.h:mddev_unlock_and_resume() {
        mddev_unlock(mddev);
        mddev_resume(mddev);
}

If memory reclaim is triggered by allocations inside mddev_unlock() and tries
to submit IO to this suspended array, it will block indefinitely waiting for
the array to resume, preventing this thread from ever reaching mddev_resume().

> +	} else {
> +		mddev_unlock(mddev);
> +	}
>  
>  	if (st == readonly || st == read_auto || st == inactive ||
>  	    (err && st == clear))
[ ... ]
> @@ -8299,6 +8310,7 @@ static bool md_ioctl_need_suspend(unsigned int cmd)
>  	case HOT_REMOVE_DISK:
>  	case SET_BITMAP_FILE:
>  	case SET_ARRAY_INFO:
> +	case STOP_ARRAY:

[Severity: Critical]
Might unconditionally suspending and then resuming the array during STOP_ARRAY
lead to a similar NULL pointer dereference for trapped IOs?

Because STOP_ARRAY now returns true here, the existing logic in md_ioctl()
will suspend the array, call do_md_stop(), and then unconditionally restore
NOIO flags and call mddev_unlock_and_resume(mddev).

This allows trapped IOs waiting in md_handle_request() to wake up after
mddev->pers has been cleared, resulting in a NULL pointer dereference when
they attempt to call mddev->pers->make_request().

[Severity: High]
Will this NOIO restoration order cause the same memory reclaim deadlock in the
ioctl path?

As in the sysfs path, md_ioctl() now suspends and later restores NOIO before
mddev_unlock_and_resume(mddev), risking a deadlock if allocations within
mddev_unlock() trigger reclaim that submits IO to the suspended array:

drivers/md/md.c:md_ioctl() {
        ...
        if (suspend) {
                memalloc_noio_restore(noio_flags);
                mddev_unlock_and_resume(mddev);
        ...
}

>  		return true;
>  	default:
>  		return false;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916121120.1242281-1-chengzhihao1@huawei.com?part=1