diff mbox series

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

Message ID 20240523153934.1937851-1-yukuai1@huaweicloud.com (mailing list archive)
State New
Headers show
Series [v2] null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues' | expand

Commit Message

Yu Kuai May 23, 2024, 3:39 p.m. UTC 
From: Yu Kuai <yukuai3@huawei.com>

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: 45919fbfe1c4 ("null_blk: Enable modifying 'submit_queues' after an instance has been configured")
Reported-and-tested-by: Yi Zhang <yi.zhang@redhat.com>
Closes: https://lore.kernel.org/all/CAHj4cs9LgsHLnjg8z06LQ3Pr5cax-+Ps+xT7AP7TPnEjStuwZA@mail.gmail.com/
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
Changes in v2:
 - remove the unrelated code.

 drivers/block/null_blk/main.c | 40 +++++++++++++++++++++++------------
 2 files changed, 27 insertions(+), 15 deletions(-)

Comments

Zhu Yanjun May 23, 2024, 7:47 a.m. UTC | #1 
On 23.05.24 17:39, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
>
> 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: 45919fbfe1c4 ("null_blk: Enable modifying 'submit_queues' after an instance has been configured")
> Reported-and-tested-by: Yi Zhang <yi.zhang@redhat.com>
> Closes: https://lore.kernel.org/all/CAHj4cs9LgsHLnjg8z06LQ3Pr5cax-+Ps+xT7AP7TPnEjStuwZA@mail.gmail.com/
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
> Changes in v2:
>   - remove the unrelated code.

Thanks. I am fine with it.

Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Zhu Yanjun

>
>   drivers/block/null_blk/main.c | 40 +++++++++++++++++++++++------------
>   2 files changed, 27 insertions(+), 15 deletions(-)
>
> 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;
Jens Axboe May 25, 2024, 4:09 p.m. UTC | #2 
On Thu, 23 May 2024 23:39:34 +0800, Yu Kuai wrote:
> 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
> 
> [...]

Applied, thanks!

[1/1] null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues'
      commit: a2db328b0839312c169eb42746ec46fc1ab53ed2

Best regards,
Markus Elfring May 26, 2024, 12:57 p.m. UTC | #3 
…
> Fix this problem by resuing the global mutex to protect
> nullb_device_power_store() and nullb_update_nr_hw_queues() from configfs.
>
> Fixes: 45919fbfe1c4 ("null_blk: Enable modifying 'submit_queues' after an instance has been configured")
…
> +++ 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;
>  }
…

How do you think about to increase the application of scope-based resource management here?
https://elixir.bootlin.com/linux/v6.9.1/source/include/linux/cleanup.h#L124

Will development interests grow for the usage of a statement like “guard(mutex)(&lock);”?

Regards,
Markus
Johannes Thumshirn May 27, 2024, 7:38 a.m. UTC | #4 
On 26.05.24 14:58, Markus Elfring wrote:
> Will development interests grow for the usage of a statement like “guard(mutex)(&lock);”?

No
diff mbox series

Patch

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;