Re: [PATCH v3] md: Fix the null-ptr-deref of 'mddev->private' while submitting IO
From: Abd-Alrhman Masalkhi
Date: Fri Sep 18 2026 - 04:56:23 EST
Hi Zhihao,
On Fri, Sep 18, 2026 at 16:34 +0800, Zhihao Cheng wrote:
> 在 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?
Thanks for the summary. I can take on the official implementation.
Before I get started, could we align on the interface details and any
other relevant considerations?
>>
>>>>
>>>>> + /*
>>>>> + * 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
>>>>>
>>>>
>>>
>>
>
--
Best Regards,
Abd-Alrhman