maillist inclusion category: bugfix bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=222020 Reference: https://lore.kernel.org/linux-raid/20260917024205.2331019-1-chengzhihao1@hua... -------------------------------- 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> Signed-off-by: Zhou Minqiang <zhouminqiang2@huawei.com> --- drivers/md/md.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 2 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index d00ceffb8812..54109499bd7b 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -445,7 +445,21 @@ void md_handle_request(struct mddev *mddev, struct bio *bio) } atomic_inc(&mddev->active_io); rcu_read_unlock(); - + 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); + if (atomic_dec_and_test(&mddev->active_io) && mddev->suspended) + wake_up(&mddev->sb_wait); + return; + } if (!mddev->pers->make_request(mddev, bio)) { atomic_dec(&mddev->active_io); wake_up(&mddev->sb_wait); @@ -489,6 +503,26 @@ static blk_qc_t md_submit_bio(struct bio *bio) return BLK_QC_T_NONE; } +static void __mddev_suspend(struct mddev *mddev) +{ + struct md_thread *thread = rcu_dereference_protected(mddev->thread, + lockdep_is_held(&mddev->reconfig_mutex)); + + WARN_ON_ONCE(thread && current == thread->tsk); + if (mddev->suspended++) + return; + synchronize_rcu(); + wake_up(&mddev->sb_wait); + set_bit(MD_ALLOW_SB_UPDATE, &mddev->flags); + smp_mb__after_atomic(); + wait_event(mddev->sb_wait, atomic_read(&mddev->active_io) == 0); + clear_bit_unlock(MD_ALLOW_SB_UPDATE, &mddev->flags); + wait_event(mddev->sb_wait, !test_bit(MD_UPDATING_SB, &mddev->flags)); + + /* restrict memory reclaim I/O during raid array is suspend */ + mddev->noio_flag = memalloc_noio_save(); +} + /* mddev_suspend makes sure no new requests are submitted * to the device, and that any requests that have been submitted * are completely handled. @@ -518,6 +552,22 @@ void mddev_suspend(struct mddev *mddev) } EXPORT_SYMBOL_GPL(mddev_suspend); +static void __mddev_resume(struct mddev *mddev) +{ + lockdep_assert_held(&mddev->reconfig_mutex); + if (--mddev->suspended) + return; + + /* entred the memalloc scope from mddev_suspend() */ + memalloc_noio_restore(mddev->noio_flag); + + wake_up(&mddev->sb_wait); + + set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); + md_wakeup_thread(mddev->thread); + md_wakeup_thread(mddev->sync_thread); /* possibly kick off a reshape */ +} + void mddev_resume(struct mddev *mddev) { lockdep_assert_held(&mddev->reconfig_mutex); @@ -4433,6 +4483,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len) { int err = 0; enum array_state st = match_word(buf, array_states); + bool suspend = false; if (mddev->pers && (st == active || st == clean) && mddev->ro != 1) { /* don't take reconfig_mutex when toggling between @@ -4454,9 +4505,16 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len) spin_unlock(&mddev->lock); return err ?: len; } + if (st == inactive || st == clear) + suspend = true; + if (suspend) + __mddev_suspend(mddev); err = mddev_lock(mddev); - if (err) + if (err) { + if (suspend) + __mddev_resume(mddev); return err; + } err = -EINVAL; switch(st) { case bad_word: @@ -4536,6 +4594,8 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len) mddev->hold_active = 0; sysfs_notify_dirent_safe(mddev->sysfs_state); } + if (suspend) + __mddev_resume(mddev); mddev_unlock(mddev); return err ?: len; } @@ -7670,8 +7730,12 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode, mutex_unlock(&mddev->open_mutex); sync_blockdev(bdev); } + if (cmd == STOP_ARRAY) + __mddev_suspend(mddev); err = mddev_lock(mddev); if (err) { + if (cmd == STOP_ARRAY) + __mddev_resume(mddev); pr_debug("md: ioctl lock interrupted, reason %d, cmd %d\n", err, cmd); goto out; @@ -7858,6 +7922,8 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode, err != -EINVAL) mddev->hold_active = 0; mddev_unlock(mddev); + if (cmd == STOP_ARRAY) + __mddev_resume(mddev); out: if(did_set_md_closing) clear_bit(MD_CLOSING, &mddev->flags); -- 2.52.0