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

[Yu Kuai <yukuai1@xxxxxxxxxxxxxxx>]

Hi,

在 2024/05/23 15:34, Zhu Yanjun 写道:
> 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?

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;