lkml.org 
[lkml]   [2020]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] block: revert pushing the final release of request_queue to a workqueue.
    On Thu, Feb 06, 2020 at 07:10:52PM +0800, yu kuai wrote:
    > syzbot is reporting use after free bug in debugfs_remove[1].
    >
    > This is because in request_queue, 'q->debugfs_dir' and
    > 'q->blk_trace->dir' could be the same dir. And in __blk_release_queue(),
    > blk_mq_debugfs_unregister() will remove everything inside the dir.
    >
    > With futher investigation of the reporduce repro, the problem can be
    > reporduced by following procedure:
    >
    > 1. LOOP_CTL_ADD, create a request_queue q1, blk_mq_debugfs_register() will
    > create the dir.
    > 2. LOOP_CTL_REMOVE, blk_release_queue() will add q1 to release queue.
    > 3. LOOP_CTL_ADD, create another request_queue q2,blk_mq_debugfs_register()
    > will fail because the dir aready exist.

    Looks we should have called blk_mq_debugfs_unregister() from
    blk_unregister_queue() because blk-mq debugfs uses disk name as debugfs
    dir. Not sure why blk_mq_debugfs_unregister() is called from queue's
    release handler.


    > 4. BLKTRACESETUP, create two files(msg and dropped) inside the dir.
    > 5. call __blk_release_queue() for q1, debugfs_remove_recursive() will
    > delete the files created in step 4.
    > 6. LOOP_CTL_REMOVE, blk_release_queue() will add q2 to release queue.
    > And when __blk_release_queue() is called for q2, blk_trace_shutdown() will
    > try to release the two files created in step 4, wich are aready released
    > in step 5.
    >
    > |thread1 |kworker |thread2 |
    > | ----------------------- | ------------------------ | -------------------- |
    > |loop_control_ioctl | | |
    > | loop_add | | |
    > | blk_mq_debugfs_register| | |
    > | debugfs_create_dir | | |
    > |loop_control_ioctl | | |
    > | loop_remove | | |
    > | blk_release_queue | | |
    > | schedule_work | | |
    > | | |loop_control_ioctl |
    > | | | loop_add |
    > | | | ... |
    > | | |blk_trace_ioctl |
    > | | | __blk_trace_setup |
    > | | | debugfs_create_file|
    > | |__blk_release_queue | |
    > | | blk_mq_debugfs_unregister| |
    > | | debugfs_remove_recursive| |
    > | | |loop_control_ioctl |
    > | | | loop_remove |
    > | | | ... |
    > | |__blk_release_queue | |
    > | | blk_trace_shutdown | |
    > | | debugfs_remove | |
    >
    > commit dc9edc44de6c ("block: Fix a blk_exit_rl() regression") pushed the
    > final release of request_queue to a workqueue, witch is not necessary
    > since commit 1e9364283764 ("blk-sysfs: Rework documention of
    > __blk_release_queue").
    >
    > [1] https://syzkaller.appspot.com/bug?extid=903b72a010ad6b7a40f2
    > References: CVE-2019-19770

    I guess your test case is more complicated than the above CVE, which
    should be triggered in single queue case.

    > Fixes: commit dc9edc44de6c ("block: Fix a blk_exit_rl() regression")

    As Bart mentioned, the above tag is wrong.

    > Reported-by: syzbot <syz...@syzkaller.appspotmail.com>
    > Signed-off-by: yu kuai <yukuai3@huawei.com>
    > ---
    > block/blk-sysfs.c | 18 +++++-------------
    > include/linux/blkdev.h | 2 --
    > 2 files changed, 5 insertions(+), 15 deletions(-)
    >
    > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
    > index fca9b158f4a0..3f448292099d 100644
    > --- a/block/blk-sysfs.c
    > +++ b/block/blk-sysfs.c
    > @@ -862,8 +862,8 @@ static void blk_exit_queue(struct request_queue *q)
    >
    >
    > /**
    > - * __blk_release_queue - release a request queue
    > - * @work: pointer to the release_work member of the request queue to be released
    > + * blk_release_queue - release a request queue
    > + * @@kobj: the kobj belonging to the request queue to be released
    > *
    > * Description:
    > * This function is called when a block device is being unregistered. The
    > @@ -873,9 +873,10 @@ static void blk_exit_queue(struct request_queue *q)
    > * of the request queue reaches zero, blk_release_queue is called to release
    > * all allocated resources of the request queue.
    > */
    > -static void __blk_release_queue(struct work_struct *work)
    > +static void blk_release_queue(struct kobject *kobj)
    > {
    > - struct request_queue *q = container_of(work, typeof(*q), release_work);
    > + struct request_queue *q =
    > + container_of(kobj, struct request_queue, kobj);
    >
    > if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
    > blk_stat_remove_callback(q, q->poll_cb);
    > @@ -904,15 +905,6 @@ static void __blk_release_queue(struct work_struct *work)
    > call_rcu(&q->rcu_head, blk_free_queue_rcu);
    > }
    >
    > -static void blk_release_queue(struct kobject *kobj)
    > -{
    > - struct request_queue *q =
    > - container_of(kobj, struct request_queue, kobj);
    > -
    > - INIT_WORK(&q->release_work, __blk_release_queue);
    > - schedule_work(&q->release_work);
    > -}
    > -
    > static const struct sysfs_ops queue_sysfs_ops = {
    > .show = queue_attr_show,
    > .store = queue_attr_store,
    > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
    > index 04cfa798a365..dff4d032c78a 100644
    > --- a/include/linux/blkdev.h
    > +++ b/include/linux/blkdev.h
    > @@ -580,8 +580,6 @@ struct request_queue {
    >
    > size_t cmd_size;
    >
    > - struct work_struct release_work;
    > -

    Looks this approach isn't correct:

    1) there are other sleepers in __blk_release_queue(), such blk-mq sysfs
    kobject_put(), or cancel_delayed_work_sync(), ...

    2) wrt. loop, the request queue's release handler may not be called yet
    after loop_remove() returns, so this patch may not avoid the issue in
    your step 3 in which blk_mq_debugfs_register fails when adding new loop
    device. So release not by wq just reduces the chance, instead of fixing
    it completely.

    Thanks,
    Ming

    \
     
     \ /
      Last update: 2020-02-07 10:31    [W:2.458 / U:0.132 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site