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

[Zhihao Cheng]

在 2026/9/18 16:20, Abd-Alrhman Masalkhi 写道:
> On Fri, Sep 18, 2026 at 15:38 +0800, Zhihao Cheng wrote:
> > 在 2026/9/18 15:29, Abd-Alrhman Masalkhi 写道:
> > > Hi Zhihao,
> > >
> > > On Fri, Sep 18, 2026 at 15:09 +0800, Zhihao Cheng wrote:
> > > > 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.
> > > > The caller dm_table_destroy() is guaranteed being invoked with device
> > > > suspended, so raid_dtr() could keep using mddev_lock_nointr().
> > > > The caller array_state_store() is guaranteed by the check
> > > > mddev_set_closing_and_sync_blockdev(mddev, 0). For example, someone open
> > > > /dev/mdx, write something and close /dev/mdx, it won't trigger the
> > > > problem, all dirty pages can be flushed before mddev->openers decrement.
> > > > Besides, fail the submitting IO in md_handle_request() if the
> > > > 'mddev->pers' becomes NULL.
> > > >
> > > > Fetch a reproducer in https://bugzilla.kernel.org/show_bug.cgi?id=222020
> > > >
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Reported-by: syzbot+3fe892ea5fc292e1353f@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > Closes: https://syzkaller.appspot.com/bug?extid=3fe892ea5fc292e1353f
> > > > Signed-off-by: Zhihao Cheng <chengzhihao1@xxxxxxxxxx>
> > > > ---
> > > >    v1->v2:
> > > >     1. Add 'mddev->pers != NULL' check before make_request
> > > >     2. Delete dm-raid caller(->dtr) modifications
> > > >     3. Move memalloc_noio_restore after mddev_unlock_and_resume
> > > >    v2->v3:
> > > >     1. Remove modifications in array_state_store()
> > > >     2. update commit msg
> > > >    drivers/md/md.c | 15 +++++++++++++++
> > > >    1 file changed, 15 insertions(+)
> > > >
> > > > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > > > index 680b34a63cb3..e73338c26d53 100644
> > > > --- a/drivers/md/md.c
> > > > +++ b/drivers/md/md.c
> > > > @@ -414,6 +414,20 @@ bool md_handle_request(struct mddev *mddev, struct bio *bio)
> > > >    		if (!percpu_ref_tryget_live(&mddev->active_io))
> > > >    			goto check_suspended;
> > > >    	}
> > > > +	if (!mddev->pers) {
> > > Isn't this case already handled by md_submit_bio(). take a look in
> > > md_submit_bio()
> > Hi,Abd-Alrhman
> > I understand that 'mddev == NULL || mddev->pers == NULL' in
> > md_submit_bio() is a qiuck check, there still exists a small race window:
> >        P1                     P2
> > md_submit_bio
> >    if (mddev == NULL || mddev->pers == NULL)
> >                           md_ioctl
> >                            mddev_suspend_and_lock
> >                            do_md_stop->__md_stop
> >                            // set mddev->pers/private as NULL
> >                            mddev_unlock_and_resume
> >    md_handle_request
> >     if (is_suspended(mddev, bio))
> >     percpu_ref_tryget_live(&mddev->active_io)
> >     mddev->pers // null-ptr-deref
>
> It makes sense. It would solve only the null pointer reference, but I
> see another issue. Look at the comment before calling
> mddev_set_closing_and_sync_blockdev() in md_ioctl(). it says "Need to
> flush page cache, and ensure no-one else opens and writes". The
> suspending happens after flushing the page cache, in this case,
> there might be other writes in-flight.

At present, the current implementation does not align with the expected 
annotations. Yu Kai privately gave me a feasible suggestion, which is to 
have md_ioctl operate on a character device(like /dev/mapper/control) 
instead of an md block device. This fix patch will serve as a temporary 
solution before the official implementation is merged. By the way, I 
don't have much time to implment it, do you have time to deal with the 
official solution?
> > > > +		/*
> > > > +		 * The __md_stop() sets 'mddev->private' to NULL during
> > > > +		 * the IO submitting, check 'mddev->pers' before the IO
> > > > +		 * being processed by specific driver to avoid the
> > > > +		 * null-ptr-deref of 'mddev-><member>'. The check is
> > > > +		 * safe because the IO has got the 'mddev->active_io'
> > > > +		 * reference, and all __md_stop() callers will wait for
> > > > +		 * the reference to be zero.
> > > > +		 */
> > > > +		bio_io_error(bio);
> > > > +		percpu_ref_put(&mddev->active_io);
> > > > +		return true;
> > > > +	}
> > > >    	if (!mddev->pers->make_request(mddev, bio)) {
> > > >    		percpu_ref_put(&mddev->active_io);
> > > >    		if (mddev_is_dm(mddev) && mddev->pers->prepare_suspend)
> > > > @@ -8299,6 +8313,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