Commit a2db328b authored by Yu Kuai's avatar Yu Kuai Committed by Jens Axboe

null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues'

Writing 'power' and 'submit_queues' concurrently will trigger kernel
panic:

Test script:

modprobe null_blk nr_devices=0
mkdir -p /sys/kernel/config/nullb/nullb0
while true; do echo 1 > submit_queues; echo 4 > submit_queues; done &
while true; do echo 1 > power; echo 0 > power; done

Test result:

BUG: kernel NULL pointer dereference, address: 0000000000000148
Oops: 0000 [#1] PREEMPT SMP
RIP: 0010:__lock_acquire+0x41d/0x28f0
Call Trace:
 <TASK>
 lock_acquire+0x121/0x450
 down_write+0x5f/0x1d0
 simple_recursive_removal+0x12f/0x5c0
 blk_mq_debugfs_unregister_hctxs+0x7c/0x100
 blk_mq_update_nr_hw_queues+0x4a3/0x720
 nullb_update_nr_hw_queues+0x71/0xf0 [null_blk]
 nullb_device_submit_queues_store+0x79/0xf0 [null_blk]
 configfs_write_iter+0x119/0x1e0
 vfs_write+0x326/0x730
 ksys_write+0x74/0x150

This is because del_gendisk() can concurrent with
blk_mq_update_nr_hw_queues():

nullb_device_power_store	nullb_apply_submit_queues
 null_del_dev
 del_gendisk
				 nullb_update_nr_hw_queues
				  if (!dev->nullb)
				  // still set while gendisk is deleted
				   return 0
				  blk_mq_update_nr_hw_queues
 dev->nullb = NULL

Fix this problem by resuing the global mutex to protect
nullb_device_power_store() and nullb_update_nr_hw_queues() from configfs.

Fixes: 45919fbf ("null_blk: Enable modifying 'submit_queues' after an instance has been configured")
Reported-and-tested-by: default avatarYi Zhang <yi.zhang@redhat.com>
Closes: https://lore.kernel.org/all/CAHj4cs9LgsHLnjg8z06LQ3Pr5cax-+Ps+xT7AP7TPnEjStuwZA@mail.gmail.com/Signed-off-by: default avatarYu Kuai <yukuai3@huawei.com>
Reviewed-by: default avatarZhu Yanjun <yanjun.zhu@linux.dev>
Link: https://lore.kernel.org/r/20240523153934.1937851-1-yukuai1@huaweicloud.comSigned-off-by: default avatarJens Axboe <axboe@kernel.dk>
parent 4a482e69
...@@ -413,13 +413,25 @@ static int nullb_update_nr_hw_queues(struct nullb_device *dev, ...@@ -413,13 +413,25 @@ static int nullb_update_nr_hw_queues(struct nullb_device *dev,
static int nullb_apply_submit_queues(struct nullb_device *dev, static int nullb_apply_submit_queues(struct nullb_device *dev,
unsigned int submit_queues) unsigned int submit_queues)
{ {
return nullb_update_nr_hw_queues(dev, submit_queues, dev->poll_queues); int ret;
mutex_lock(&lock);
ret = nullb_update_nr_hw_queues(dev, submit_queues, dev->poll_queues);
mutex_unlock(&lock);
return ret;
} }
static int nullb_apply_poll_queues(struct nullb_device *dev, static int nullb_apply_poll_queues(struct nullb_device *dev,
unsigned int poll_queues) unsigned int poll_queues)
{ {
return nullb_update_nr_hw_queues(dev, dev->submit_queues, poll_queues); int ret;
mutex_lock(&lock);
ret = nullb_update_nr_hw_queues(dev, dev->submit_queues, poll_queues);
mutex_unlock(&lock);
return ret;
} }
NULLB_DEVICE_ATTR(size, ulong, NULL); NULLB_DEVICE_ATTR(size, ulong, NULL);
...@@ -468,28 +480,31 @@ static ssize_t nullb_device_power_store(struct config_item *item, ...@@ -468,28 +480,31 @@ static ssize_t nullb_device_power_store(struct config_item *item,
if (ret < 0) if (ret < 0)
return ret; return ret;
ret = count;
mutex_lock(&lock);
if (!dev->power && newp) { if (!dev->power && newp) {
if (test_and_set_bit(NULLB_DEV_FL_UP, &dev->flags)) if (test_and_set_bit(NULLB_DEV_FL_UP, &dev->flags))
return count; goto out;
ret = null_add_dev(dev); ret = null_add_dev(dev);
if (ret) { if (ret) {
clear_bit(NULLB_DEV_FL_UP, &dev->flags); clear_bit(NULLB_DEV_FL_UP, &dev->flags);
return ret; goto out;
} }
set_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags); set_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags);
dev->power = newp; dev->power = newp;
} else if (dev->power && !newp) { } else if (dev->power && !newp) {
if (test_and_clear_bit(NULLB_DEV_FL_UP, &dev->flags)) { if (test_and_clear_bit(NULLB_DEV_FL_UP, &dev->flags)) {
mutex_lock(&lock);
dev->power = newp; dev->power = newp;
null_del_dev(dev->nullb); null_del_dev(dev->nullb);
mutex_unlock(&lock);
} }
clear_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags); clear_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags);
} }
return count; out:
mutex_unlock(&lock);
return ret;
} }
CONFIGFS_ATTR(nullb_device_, power); CONFIGFS_ATTR(nullb_device_, power);
...@@ -1932,15 +1947,12 @@ static int null_add_dev(struct nullb_device *dev) ...@@ -1932,15 +1947,12 @@ static int null_add_dev(struct nullb_device *dev)
nullb->q->queuedata = nullb; nullb->q->queuedata = nullb;
blk_queue_flag_set(QUEUE_FLAG_NONROT, nullb->q); blk_queue_flag_set(QUEUE_FLAG_NONROT, nullb->q);
mutex_lock(&lock);
rv = ida_alloc(&nullb_indexes, GFP_KERNEL); rv = ida_alloc(&nullb_indexes, GFP_KERNEL);
if (rv < 0) { if (rv < 0)
mutex_unlock(&lock);
goto out_cleanup_disk; goto out_cleanup_disk;
}
nullb->index = rv; nullb->index = rv;
dev->index = rv; dev->index = rv;
mutex_unlock(&lock);
if (config_item_name(&dev->group.cg_item)) { if (config_item_name(&dev->group.cg_item)) {
/* Use configfs dir name as the device name */ /* Use configfs dir name as the device name */
...@@ -1969,9 +1981,7 @@ static int null_add_dev(struct nullb_device *dev) ...@@ -1969,9 +1981,7 @@ static int null_add_dev(struct nullb_device *dev)
if (rv) if (rv)
goto out_ida_free; goto out_ida_free;
mutex_lock(&lock);
list_add_tail(&nullb->list, &nullb_list); list_add_tail(&nullb->list, &nullb_list);
mutex_unlock(&lock);
pr_info("disk %s created\n", nullb->disk_name); pr_info("disk %s created\n", nullb->disk_name);
...@@ -2020,7 +2030,9 @@ static int null_create_dev(void) ...@@ -2020,7 +2030,9 @@ static int null_create_dev(void)
if (!dev) if (!dev)
return -ENOMEM; return -ENOMEM;
mutex_lock(&lock);
ret = null_add_dev(dev); ret = null_add_dev(dev);
mutex_unlock(&lock);
if (ret) { if (ret) {
null_free_dev(dev); null_free_dev(dev);
return ret; return ret;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment