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

["yu kuai" <yukuai@fygo.io>]

Hi,

在 2026/9/18 14:19, Zhihao Cheng 写道:
> 在 2026/9/18 14:06, yu kuai 写道:
>> Hi,
>>
>> 在 2026/9/17 10:42, 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. The caller dm_table_destroy() is guaranteed
>>> being invoked with device suspended, so raid_dtr() could keep
>>> using mddev_lock_nointr(). 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@syzkaller.appspotmail.com
>>> Closes: https://syzkaller.appspot.com/bug?extid=3fe892ea5fc292e1353f
>>> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
>>> ---
>>>    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
>>>    drivers/md/md.c | 32 +++++++++++++++++++++++++++++---
>>>    1 file changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>> index 680b34a63cb3..3dfc34caa2dc 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) {
>>> +        /*
>>> +         * 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)
>>> @@ -4657,6 +4671,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 +4682,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 +4720,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);
>>
>> Unlike ioctl path, where the checking of opener is just 1, 
>> mddev_set_closing_and_sync_blockdev() here
>> already check there is no opener, so there can't be any IO inflight.
>
> Hi, yukuai
> I thought of a scene. If someone open '/dev/md0, write(buffer) 4K, and 
> close the corresponding fd. Then writeback kworker submit IO, 
> array_state_store will pass the mddev_set_closing_and_sync_blockdev() 
> check, which could lead to the similar problem.
This should not happen, that last one to close /dev/md0 will wait for 
full disk flush to be done.
>>
>>>        if (err)
>>>            return err;
>>> +    if (suspend)
>>> +        noio_flags = memalloc_noio_save();
>>>           switch (st) {
>>>        case inactive:
>>> @@ -4775,7 +4795,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) {
>>> +        mddev_unlock_and_resume(mddev);
>>> +        memalloc_noio_restore(noio_flags);
>>> +    } else {
>>> +        mddev_unlock(mddev);
>>> +    }
>>>           if (st == readonly || st == read_auto || st == inactive ||
>>>            (err && st == clear))
>>> @@ -8299,6 +8324,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;
>>
>
-- 
Thanks,
Kuai