[PATCH] blk-mq: don't touch ->tagset in blk_mq_get_sq_hctx

[PATCH] blk-mq: don't touch ->tagset in blk_mq_get_sq_hctx

[Ming Lei]

blk_mq_run_hw_queues() could be run when there isn't queued request and
after queue is cleaned up, at that time tagset is freed, because tagset
lifetime is covered by driver, and often freed after blk_cleanup_queue()
returns.

So don't touch ->tagset for figuring out current default hctx by the mapping
built in request queue, so use-after-free on tagset can be avoided. Meantime
this way should be fast than retrieving mapping from tagset.

Cc: "yukuai (C)" <yukuai3@huawei.com>
Cc: Jan Kara <jack@suse.cz>
Fixes: b6e68ee82585 ("blk-mq: Improve performance of non-mq IO schedulers with multiple HW queues")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index ed1869a305c4..5789e971ac83 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2174,8 +2174,7 @@ static bool blk_mq_has_sqsched(struct request_queue *q)
  */
 static struct blk_mq_hw_ctx *blk_mq_get_sq_hctx(struct request_queue *q)
 {
-	struct blk_mq_hw_ctx *hctx;
-
+	struct blk_mq_ctx *ctx = blk_mq_get_ctx(q);
 	/*
 	 * If the IO scheduler does not respect hardware queues when
 	 * dispatching, we just don't bother with multiple HW queues and
@@ -2183,8 +2182,8 @@ static struct blk_mq_hw_ctx *blk_mq_get_sq_hctx(struct request_queue *q)
 	 * just causes lock contention inside the scheduler and pointless cache
 	 * bouncing.
 	 */
-	hctx = blk_mq_map_queue_type(q, HCTX_TYPE_DEFAULT,
-				     raw_smp_processor_id());
+	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, 0, ctx);
+
 	if (!blk_mq_hctx_stopped(hctx))
 		return hctx;
 	return NULL;
-- 
2.31.1

Re: [PATCH] blk-mq: don't touch ->tagset in blk_mq_get_sq_hctx

[Jan Kara]

On Sun 22-05-22 20:23:50, Ming Lei wrote:
> blk_mq_run_hw_queues() could be run when there isn't queued request and
> after queue is cleaned up, at that time tagset is freed, because tagset
> lifetime is covered by driver, and often freed after blk_cleanup_queue()
> returns.
> 
> So don't touch ->tagset for figuring out current default hctx by the mapping
> built in request queue, so use-after-free on tagset can be avoided. Meantime
> this way should be fast than retrieving mapping from tagset.
> 
> Cc: "yukuai (C)" <yukuai3@huawei.com>
> Cc: Jan Kara <jack@suse.cz>
> Fixes: b6e68ee82585 ("blk-mq: Improve performance of non-mq IO schedulers with multiple HW queues")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Thanks! This indeed looks better. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/blk-mq.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index ed1869a305c4..5789e971ac83 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2174,8 +2174,7 @@ static bool blk_mq_has_sqsched(struct request_queue *q)
>   */
>  static struct blk_mq_hw_ctx *blk_mq_get_sq_hctx(struct request_queue *q)
>  {
> -	struct blk_mq_hw_ctx *hctx;
> -
> +	struct blk_mq_ctx *ctx = blk_mq_get_ctx(q);
>  	/*
>  	 * If the IO scheduler does not respect hardware queues when
>  	 * dispatching, we just don't bother with multiple HW queues and
> @@ -2183,8 +2182,8 @@ static struct blk_mq_hw_ctx *blk_mq_get_sq_hctx(struct request_queue *q)
>  	 * just causes lock contention inside the scheduler and pointless cache
>  	 * bouncing.
>  	 */
> -	hctx = blk_mq_map_queue_type(q, HCTX_TYPE_DEFAULT,
> -				     raw_smp_processor_id());
> +	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, 0, ctx);
> +
>  	if (!blk_mq_hctx_stopped(hctx))
>  		return hctx;
>  	return NULL;
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] blk-mq: don't touch ->tagset in blk_mq_get_sq_hctx

[Jens Axboe]

On Sun, 22 May 2022 20:23:50 +0800, Ming Lei wrote:
> blk_mq_run_hw_queues() could be run when there isn't queued request and
> after queue is cleaned up, at that time tagset is freed, because tagset
> lifetime is covered by driver, and often freed after blk_cleanup_queue()
> returns.
> 
> So don't touch ->tagset for figuring out current default hctx by the mapping
> built in request queue, so use-after-free on tagset can be avoided. Meantime
> this way should be fast than retrieving mapping from tagset.
> 
> [...]

Applied, thanks!

[1/1] blk-mq: don't touch ->tagset in blk_mq_get_sq_hctx
      commit: 5d05426e2d5fd7df8afc866b78c36b37b00188b7

Best regards,
-- 
Jens Axboe