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

[Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>]

Hi Kuai,

On Fri, Sep 18, 2026 at 17:30 +0800, yu kuai wrote:
> Hi,
>
> 在 2026/9/18 16:54, Abd-Alrhman Masalkhi 写道:
>> 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@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
>>>>>>>     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?
>
> I think the following procedures(details should be considered more):
>
> 1) add a new char device in mdraid to issue ioctl to array;
> 2) update mdadm to use the new ioctl;
> 3) a kernel message to warn the old block device ioctl is deprecated, and suggest
> user to upgrade mdadm;
> 4) Since the new ioctl will introduce UAPI change, we should wait for a long time,
> perhaps more than 1 year, before the deprecated old ioctl code in kernel can be removed.
>

Thanks, I’ll start working on the implementation.

>>
>>>>>>> +		/*
>>>>>>> +		 * 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
>>>>>>>
> -- 
> Thanks,
> Kuai

-- 
Best Regards,
Abd-Alrhman