[PATCH] block: quiesce queue before freeing queue

There is a race beteewn blk_mq_run_hw_queue() and cleanup queue,
which can cause use-after-free as following:

cpu1                              cpu2
queue_state_write()
                                  blk_release_queue
                                    blk_exit_queue
  blk_mq_run_hw_queue
    blk_mq_hctx_has_pending
      e = q->elevator
                                    q->elevator = NULL
                                    free(q->elevator)
      e && e->type->ops.has_work //use-after-free

Fix this bug by adding quiesce before freeing queue. Then, anyone
who tries to run hw queue will be safe.

This is basically revert of 662156641bc4 ("block: don't drain
in-progress dispatch in blk_cleanup_queue()")

Fixes: 662156641bc4 ("block: don't drain in-progress dispatch in blk_cleanup_queue()")
Signed-off-by: Yufen Yu <redacted>
---
 block/blk-core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index 7663a9b94b80..f8a038d19c89 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c

@@ -392,6 +392,18 @@ void blk_cleanup_queue(struct request_queue *q)
 
 	blk_queue_flag_set(QUEUE_FLAG_DEAD, q);
 
+	/*
+	 * make sure all in-progress dispatch are completed because
+	 * blk_freeze_queue() can only complete all requests, and
+	 * dispatch may still be in-progress since we dispatch requests
+	 * from more than one contexts.
+	 *
+	 * We rely on driver to deal with the race in case that queue
+	 * initialization isn't done.
+	 */
+	if (queue_is_mq(q) && blk_queue_init_done(q))
+		blk_mq_quiesce_queue(q);
+
 	/* for synchronous bio-based driver finish in-flight integrity i/o */
 	blk_flush_integrity();
 

-- 
2.25.4

On Fri, Jan 15, 2021 at 01:43:52AM -0500, Yufen Yu wrote:

There is a race beteewn blk_mq_run_hw_queue() and cleanup queue,
which can cause use-after-free as following:

cpu1                              cpu2
queue_state_write()
                                  blk_release_queue
                                    blk_exit_queue
  blk_mq_run_hw_queue
    blk_mq_hctx_has_pending
      e = q->elevator
                                    q->elevator = NULL
                                    free(q->elevator)
      e && e->type->ops.has_work //use-after-free

Fix this bug by adding quiesce before freeing queue. Then, anyone
who tries to run hw queue will be safe.

It is blk_mq_run_hw_queue() caller's responsibility to make sure that
the request queue won't disappear, so please fix the caller if there is
such issue.

No, please don't do that. We had several slow boot reports before caused by
by synchronize_rcu() in blk_cleanup_queue(), since blk_cleanup_queue()
is called for in-existed queues, which number can be quite big.

Thanks,
Ming

On 2021/1/15 15:35, Ming Lei wrote:

On Fri, Jan 15, 2021 at 01:43:52AM -0500, Yufen Yu wrote:

quoted

There is a race beteewn blk_mq_run_hw_queue() and cleanup queue,
which can cause use-after-free as following:

quoted

diff --git a/block/blk-core.c b/block/blk-core.c
index 7663a9b94b80..f8a038d19c89 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c

@@ -392,6 +392,18 @@ void blk_cleanup_queue(struct request_queue *q)
  
  	blk_queue_flag_set(QUEUE_FLAG_DEAD, q);
  
+	/*
+	 * make sure all in-progress dispatch are completed because
+	 * blk_freeze_queue() can only complete all requests, and
+	 * dispatch may still be in-progress since we dispatch requests
+	 * from more than one contexts.
+	 *
+	 * We rely on driver to deal with the race in case that queue
+	 * initialization isn't done.
+	 */
+	if (queue_is_mq(q) && blk_queue_init_done(q))
+		blk_mq_quiesce_queue(q);

No, please don't do that. We had several slow boot reports before caused by
by synchronize_rcu() in blk_cleanup_queue(), since blk_cleanup_queue()
is called for in-existed queues, which number can be quite big.

Thanks to point out this. But, can't blk_queue_init_done will avoid calling
synchronize_rcu() for inexistent queues?

Thanks,
Yufen