Re: [PATCH RESEND] null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues'

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 23.05.24 16:45, Yu Kuai wrote:
From: Yu Kuai <yukuai3@xxxxxxxxxx>

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 configs.

Fixes: 45919fbfe1c4 ("null_blk: Enable modifying 'submit_queues' after an instance has been configured")
Reported-and-tested-by: Yi Zhang <yi.zhang@xxxxxxxxxx>
Closes: https://lore.kernel.org/all/CAHj4cs9LgsHLnjg8z06LQ3Pr5cax-+Ps+xT7AP7TPnEjStuwZA@xxxxxxxxxxxxxx/
Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx>
---
  drivers/block/loop.c          |  2 +-
  drivers/block/null_blk/main.c | 40 +++++++++++++++++++++++------------
  2 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 28a95fd366fe..3e1c4f5ef714 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2140,7 +2140,7 @@ static int loop_control_remove(int idx)
  		pr_warn_once("deleting an unspecified loop device is not supported.\n");
  		return -EINVAL;
  	}
-		
+

This snippet changes nothing. Can we remove it?

Zhu Yanjun

  	/* Hide this loop device for serialization. */
  	ret = mutex_lock_killable(&loop_ctl_mutex);
  	if (ret)
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 5d56ad4ce01a..eb023d267369 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -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,
  				     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,
  				   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);
@@ -468,28 +480,31 @@ static ssize_t nullb_device_power_store(struct config_item *item,
  	if (ret < 0)
  		return ret;
+ ret = count;
+	mutex_lock(&lock);
  	if (!dev->power && newp) {
  		if (test_and_set_bit(NULLB_DEV_FL_UP, &dev->flags))
-			return count;
+			goto out;
+
  		ret = null_add_dev(dev);
  		if (ret) {
  			clear_bit(NULLB_DEV_FL_UP, &dev->flags);
-			return ret;
+			goto out;
  		}
set_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags);
  		dev->power = newp;
  	} else if (dev->power && !newp) {
  		if (test_and_clear_bit(NULLB_DEV_FL_UP, &dev->flags)) {
-			mutex_lock(&lock);
  			dev->power = newp;
  			null_del_dev(dev->nullb);
-			mutex_unlock(&lock);
  		}
  		clear_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags);
  	}
- return count;
+out:
+	mutex_unlock(&lock);
+	return ret;
  }
CONFIGFS_ATTR(nullb_device_, power);
@@ -1932,15 +1947,12 @@ static int null_add_dev(struct nullb_device *dev)
  	nullb->q->queuedata = nullb;
  	blk_queue_flag_set(QUEUE_FLAG_NONROT, nullb->q);
- mutex_lock(&lock);
  	rv = ida_alloc(&nullb_indexes, GFP_KERNEL);
-	if (rv < 0) {
-		mutex_unlock(&lock);
+	if (rv < 0)
  		goto out_cleanup_disk;
-	}
+
  	nullb->index = rv;
  	dev->index = rv;
-	mutex_unlock(&lock);
if (config_item_name(&dev->group.cg_item)) {
  		/* Use configfs dir name as the device name */
@@ -1969,9 +1981,7 @@ static int null_add_dev(struct nullb_device *dev)
  	if (rv)
  		goto out_ida_free;
- mutex_lock(&lock);
  	list_add_tail(&nullb->list, &nullb_list);
-	mutex_unlock(&lock);
pr_info("disk %s created\n", nullb->disk_name); @@ -2020,7 +2030,9 @@ static int null_create_dev(void)
  	if (!dev)
  		return -ENOMEM;
+ mutex_lock(&lock);
  	ret = null_add_dev(dev);
+	mutex_unlock(&lock);
  	if (ret) {
  		null_free_dev(dev);
  		return ret;

--
Best Regards,
Yanjun.Zhu





[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux