Re: [PATCH] block: revert pushing the final release of request_queue to a workqueue.

[Ming Lei <ming.lei@redhat.com>]

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