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

[Zhihao Cheng <chengzhihao1@huawei.com>]

在 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
> 
>> +		/*
>> +		 * 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
>>
>