| Message ID | 20240523144536.1376904-1-yukuai1@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues' | expand |
On 23.05.24 16:45, 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 configs. > > 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> > --- > 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;
Hi, 在 2024/05/23 15:34, Zhu Yanjun 写道: > > On 23.05.24 16:45, 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 configs. >> >> 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> >> --- >> 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; >
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;