public inbox for linux-block@vger.kernel.org
 help / color / mirror / Atom feed
From: yu kuai <yukuai3@huawei.com>
To: <axboe@kernel.dk>, <ming.lei@redhat.com>,
	<chaitanya.kulkarni@wdc.com>, <damien.lemoal@wdc.com>,
	<bvanassche@acm.org>, <dhowells@redhat.com>,
	<asml.silence@gmail.com>, <ajay.joshi@wdc.com>
Cc: <linux-block@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<yukuai3@huawei.com>, <yi.zhang@huawei.com>,
	<zhangxiaoxu5@huawei.com>, <luoshijie1@huawei.com>
Subject: [PATCH] block: revert pushing the final release of request_queue to a workqueue.
Date: Thu, 6 Feb 2020 19:10:52 +0800	[thread overview]
Message-ID: <20200206111052.45356-1-yukuai3@huawei.com> (raw)

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.
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
Fixes: commit dc9edc44de6c ("block: Fix a blk_exit_rl() regression")
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;
-
 #define BLK_MAX_WRITE_HINTS	5
 	u64			write_hints[BLK_MAX_WRITE_HINTS];
 };
-- 
2.17.2


             reply	other threads:[~2020-02-06 11:12 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-06 11:10 yu kuai [this message]
2020-02-07  4:09 ` [PATCH] block: revert pushing the final release of request_queue to a workqueue Bart Van Assche
2020-02-07  6:02   ` yukuai (C)
2020-02-07  7:10     ` yukuai (C)
2020-02-07  9:30 ` Ming Lei
2020-02-07 10:26   ` yukuai (C)
2020-02-07 12:24     ` yukuai (C)
2020-02-07 13:04       ` Ming Lei
2020-02-08  6:12         ` yukuai (C)
2020-02-10  1:00 ` Bart Van Assche
2020-02-10  2:13   ` yukuai (C)
2020-02-10  2:32     ` Ming Lei
2020-02-10  3:14     ` Bart Van Assche
2020-02-10  8:49       ` yukuai (C)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200206111052.45356-1-yukuai3@huawei.com \
    --to=yukuai3@huawei.com \
    --cc=ajay.joshi@wdc.com \
    --cc=asml.silence@gmail.com \
    --cc=axboe@kernel.dk \
    --cc=bvanassche@acm.org \
    --cc=chaitanya.kulkarni@wdc.com \
    --cc=damien.lemoal@wdc.com \
    --cc=dhowells@redhat.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luoshijie1@huawei.com \
    --cc=ming.lei@redhat.com \
    --cc=yi.zhang@huawei.com \
    --cc=zhangxiaoxu5@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox