From: Yu Kuai <[email protected]>
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 <[email protected]>
Closes: https://lore.kernel.org/all/CAHj4cs9LgsHLnjg8z06LQ3Pr5cax-+Ps+xT7AP7TPnEjStuwZA@mail.gmail.com/
Signed-off-by: Yu Kuai <[email protected]>
---
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;
}
-
+
/* 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;
--
2.39.2
On 23.05.24 16:45, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> 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 <[email protected]>
> Closes: https://lore.kernel.org/all/CAHj4cs9LgsHLnjg8z06LQ3Pr5cax-+Ps+xT7AP7TPnEjStuwZA@mail.gmail.com/
> Signed-off-by: Yu Kuai <[email protected]>
> ---
> 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
Hi,
在 2024/05/23 15:34, Zhu Yanjun 写道:
>
> On 23.05.24 16:45, Yu Kuai wrote:
>> From: Yu Kuai <[email protected]>
>>
>> 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 <[email protected]>
>> Closes:
>> https://lore.kernel.org/all/CAHj4cs9LgsHLnjg8z06LQ3Pr5cax-+Ps+xT7AP7TPnEjStuwZA@mail.gmail.com/
>>
>> Signed-off-by: Yu Kuai <[email protected]>
>> ---
>> 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?
Of course this should be removed... it's tail space here, however,
it's not related to this patch, and I somehow merged into this patch...
Will send a v2.
Thanks,
Kuai
>
> 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;
>