[PATCH V3 0/6] blk-mq: update_nr_hw_queues related improvement & bugfix

[PATCH V3 0/6] blk-mq: update_nr_hw_queues related improvement & bugfix

[Ming Lei]

Hi,

The 1st patch figures out correct numa node for each kind of hw queue.

The 2nd patch simplifies reallocation of q->queue_hw_ctx a bit.

The 3rd patch re-configures poll capability after queue map is changed.

The 4th patch changes mtip32xx to avoid to refer to q->queue_hw_ctx
directly.

The 5th & 6th patches fix use-after-free on q->queue_hw_ctx.

V3:
	- fix smatch warnings reported by kernel test robot (5/6)

V2:
	- use xa_for_each() to implement queue_for_each_hw_ctx(5/6 6/6) 
	- patch style change(1/6)
	- rename as suggested by Christoph(3/6)

Ming Lei (6):
  blk-mq: figure out correct numa node for hw queue
  blk-mq: simplify reallocation of hw ctxs a bit
  blk-mq: reconfigure poll after queue map is changed
  block: mtip32xx: don't touch q->queue_hw_ctx
  blk-mq: prepare for implementing hctx table via xarray
  blk-mq: manage hctx map via xarray

 block/blk-mq-debugfs.c            |   6 +-
 block/blk-mq-sched.c              |   9 +-
 block/blk-mq-sysfs.c              |  16 +--
 block/blk-mq-tag.c                |   4 +-
 block/blk-mq.c                    | 158 +++++++++++++++++-------------
 block/blk-mq.h                    |   2 +-
 drivers/block/mtip32xx/mtip32xx.c |   4 +-
 drivers/block/rnbd/rnbd-clt.c     |   2 +-
 include/linux/blk-mq.h            |   3 +-
 include/linux/blkdev.h            |   2 +-
 10 files changed, 116 insertions(+), 90 deletions(-)

-- 
2.31.1

[PATCH V3 1/6] blk-mq: figure out correct numa node for hw queue

[Ming Lei]

The current code always uses default queue map and hw queue index
for figuring out the numa node for hw queue, this way isn't correct
because blk-mq supports three queue maps, and the correct queue map
should be used for the specified hw queue.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index a05ce7725031..5d25abf9e551 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3107,15 +3107,41 @@ void blk_mq_free_rq_map(struct blk_mq_tags *tags)
 	blk_mq_free_tags(tags);
 }
 
+static enum hctx_type hctx_idx_to_type(struct blk_mq_tag_set *set,
+		unsigned int hctx_idx)
+{
+	int i;
+
+	for (i = 0; i < set->nr_maps; i++) {
+		unsigned int start = set->map[i].queue_offset;
+		unsigned int end = start + set->map[i].nr_queues;
+
+		if (hctx_idx >= start && hctx_idx < end)
+			break;
+	}
+
+	if (i >= set->nr_maps)
+		i = HCTX_TYPE_DEFAULT;
+
+	return i;
+}
+
+static int blk_mq_get_hctx_node(struct blk_mq_tag_set *set,
+		unsigned int hctx_idx)
+{
+	enum hctx_type type = hctx_idx_to_type(set, hctx_idx);
+
+	return blk_mq_hw_queue_to_node(&set->map[type], hctx_idx);
+}
+
 static struct blk_mq_tags *blk_mq_alloc_rq_map(struct blk_mq_tag_set *set,
 					       unsigned int hctx_idx,
 					       unsigned int nr_tags,
 					       unsigned int reserved_tags)
 {
+	int node = blk_mq_get_hctx_node(set, hctx_idx);
 	struct blk_mq_tags *tags;
-	int node;
 
-	node = blk_mq_hw_queue_to_node(&set->map[HCTX_TYPE_DEFAULT], hctx_idx);
 	if (node == NUMA_NO_NODE)
 		node = set->numa_node;
 
@@ -3164,10 +3190,9 @@ static int blk_mq_alloc_rqs(struct blk_mq_tag_set *set,
 			    unsigned int hctx_idx, unsigned int depth)
 {
 	unsigned int i, j, entries_per_page, max_order = 4;
+	int node = blk_mq_get_hctx_node(set, hctx_idx);
 	size_t rq_size, left;
-	int node;
 
-	node = blk_mq_hw_queue_to_node(&set->map[HCTX_TYPE_DEFAULT], hctx_idx);
 	if (node == NUMA_NO_NODE)
 		node = set->numa_node;
 
@@ -3941,10 +3966,9 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 	/* protect against switching io scheduler  */
 	mutex_lock(&q->sysfs_lock);
 	for (i = 0; i < set->nr_hw_queues; i++) {
-		int node;
+		int node = blk_mq_get_hctx_node(set, i);
 		struct blk_mq_hw_ctx *hctx;
 
-		node = blk_mq_hw_queue_to_node(&set->map[HCTX_TYPE_DEFAULT], i);
 		/*
 		 * If the hw queue has been mapped to another numa node,
 		 * we need to realloc the hctx. If allocation fails, fallback
-- 
2.31.1

Re: [PATCH V3 1/6] blk-mq: figure out correct numa node for hw queue

[Hannes Reinecke]

On 3/7/22 07:43, Ming Lei wrote:
> The current code always uses default queue map and hw queue index
> for figuring out the numa node for hw queue, this way isn't correct
> because blk-mq supports three queue maps, and the correct queue map
> should be used for the specified hw queue.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-mq.c | 36 ++++++++++++++++++++++++++++++------
>   1 file changed, 30 insertions(+), 6 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH V3 2/6] blk-mq: simplify reallocation of hw ctxs a bit

[Ming Lei]

blk_mq_alloc_and_init_hctx() has already taken reuse into account, so
no need to do it outside, then we can simplify blk_mq_realloc_hw_ctxs().

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5d25abf9e551..64fddb36c93c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3966,29 +3966,24 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 	/* protect against switching io scheduler  */
 	mutex_lock(&q->sysfs_lock);
 	for (i = 0; i < set->nr_hw_queues; i++) {
+		int old_node;
 		int node = blk_mq_get_hctx_node(set, i);
-		struct blk_mq_hw_ctx *hctx;
+		struct blk_mq_hw_ctx *old_hctx = hctxs[i];
 
-		/*
-		 * If the hw queue has been mapped to another numa node,
-		 * we need to realloc the hctx. If allocation fails, fallback
-		 * to use the previous one.
-		 */
-		if (hctxs[i] && (hctxs[i]->numa_node == node))
-			continue;
+		if (old_hctx) {
+			old_node = old_hctx->numa_node;
+			blk_mq_exit_hctx(q, set, old_hctx, i);
+		}
 
-		hctx = blk_mq_alloc_and_init_hctx(set, q, i, node);
-		if (hctx) {
-			if (hctxs[i])
-				blk_mq_exit_hctx(q, set, hctxs[i], i);
-			hctxs[i] = hctx;
-		} else {
-			if (hctxs[i])
-				pr_warn("Allocate new hctx on node %d fails,\
-						fallback to previous one on node %d\n",
-						node, hctxs[i]->numa_node);
-			else
+		hctxs[i] = blk_mq_alloc_and_init_hctx(set, q, i, node);
+		if (!hctxs[i]) {
+			if (!old_hctx)
 				break;
+			pr_warn("Allocate new hctx on node %d fails, fallback to previous one on node %d\n",
+					node, old_node);
+			hctxs[i] = blk_mq_alloc_and_init_hctx(set, q, i,
+					old_node);
+			WARN_ON_ONCE(!hctxs[i]);
 		}
 	}
 	/*
-- 
2.31.1

Re: [PATCH V3 2/6] blk-mq: simplify reallocation of hw ctxs a bit

[Hannes Reinecke]

On 3/7/22 07:43, Ming Lei wrote:
> blk_mq_alloc_and_init_hctx() has already taken reuse into account, so
> no need to do it outside, then we can simplify blk_mq_realloc_hw_ctxs().
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-mq.c | 33 ++++++++++++++-------------------
>   1 file changed, 14 insertions(+), 19 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH V3 3/6] blk-mq: reconfigure poll after queue map is changed

[Ming Lei]

queue map can be changed when updating nr_hw_queues, so we need to
reconfigure queue's poll capability. Add one helper for doing this job.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 64fddb36c93c..57ae9df0f4dc 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4010,6 +4010,17 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 	mutex_unlock(&q->sysfs_lock);
 }
 
+static void blk_mq_update_poll_flag(struct request_queue *q)
+{
+	struct blk_mq_tag_set *set = q->tag_set;
+
+	if (set->nr_maps > HCTX_TYPE_POLL &&
+	    set->map[HCTX_TYPE_POLL].nr_queues)
+		blk_queue_flag_set(QUEUE_FLAG_POLL, q);
+	else
+		blk_queue_flag_clear(QUEUE_FLAG_POLL, q);
+}
+
 int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 		struct request_queue *q)
 {
@@ -4044,9 +4055,7 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 	q->tag_set = set;
 
 	q->queue_flags |= QUEUE_FLAG_MQ_DEFAULT;
-	if (set->nr_maps > HCTX_TYPE_POLL &&
-	    set->map[HCTX_TYPE_POLL].nr_queues)
-		blk_queue_flag_set(QUEUE_FLAG_POLL, q);
+	blk_mq_update_poll_flag(q);
 
 	INIT_DELAYED_WORK(&q->requeue_work, blk_mq_requeue_work);
 	INIT_LIST_HEAD(&q->requeue_list);
@@ -4512,6 +4521,7 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 	blk_mq_update_queue_map(set);
 	list_for_each_entry(q, &set->tag_list, tag_set_list) {
 		blk_mq_realloc_hw_ctxs(set, q);
+		blk_mq_update_poll_flag(q);
 		if (q->nr_hw_queues != set->nr_hw_queues) {
 			int i = prev_nr_hw_queues;
 
-- 
2.31.1

Re: [PATCH V3 3/6] blk-mq: reconfigure poll after queue map is changed

[Christoph Hellwig]

On Mon, Mar 07, 2022 at 02:43:58PM +0800, Ming Lei wrote:
> queue map can be changed when updating nr_hw_queues, so we need to
> reconfigure queue's poll capability. Add one helper for doing this job.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Eventually we'd want to reuse this for nvme-multipath as already pointed
out.  But I'm fine with just fixing this up later, so:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH V3 3/6] blk-mq: reconfigure poll after queue map is changed

[Hannes Reinecke]

On 3/7/22 07:43, Ming Lei wrote:
> queue map can be changed when updating nr_hw_queues, so we need to
> reconfigure queue's poll capability. Add one helper for doing this job.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-mq.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH V3 4/6] block: mtip32xx: don't touch q->queue_hw_ctx

[Ming Lei]

q->queue_hw_ctx is really one blk-mq internal structure for retrieving
hctx via its index, not supposed to be used by drivers. Meantime drivers
can get the tags structure easily from tagset.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/mtip32xx/mtip32xx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index cba956881d55..2d43ab5b5cc0 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -160,9 +160,7 @@ static bool mtip_check_surprise_removal(struct driver_data *dd)
 static struct mtip_cmd *mtip_cmd_from_tag(struct driver_data *dd,
 					  unsigned int tag)
 {
-	struct blk_mq_hw_ctx *hctx = dd->queue->queue_hw_ctx[0];
-
-	return blk_mq_rq_to_pdu(blk_mq_tag_to_rq(hctx->tags, tag));
+	return blk_mq_rq_to_pdu(blk_mq_tag_to_rq(dd->tags.tags[0], tag));
 }
 
 /*
-- 
2.31.1

Re: [PATCH V3 4/6] block: mtip32xx: don't touch q->queue_hw_ctx

[Hannes Reinecke]

On 3/7/22 07:43, Ming Lei wrote:
> q->queue_hw_ctx is really one blk-mq internal structure for retrieving
> hctx via its index, not supposed to be used by drivers. Meantime drivers
> can get the tags structure easily from tagset.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   drivers/block/mtip32xx/mtip32xx.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH V3 5/6] blk-mq: prepare for implementing hctx table via xarray

[Ming Lei]

It is inevitable to cause use-after-free on q->queue_hw_ctx between
queue_for_each_hw_ctx() and blk_mq_update_nr_hw_queues(). And converting
to xarray can fix the uaf, meantime code gets cleaner.

Prepare for converting q->queue_hctx_ctx into xarray, one thing is that
xa_for_each() can only accept 'unsigned long' as index, so changes type
of hctx index of queue_for_each_hw_ctx() into 'unsigned long'.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-debugfs.c        |  6 +++---
 block/blk-mq-sched.c          |  9 +++++----
 block/blk-mq-sysfs.c          | 16 ++++++++++------
 block/blk-mq-tag.c            |  2 +-
 block/blk-mq.c                | 30 ++++++++++++++++--------------
 drivers/block/rnbd/rnbd-clt.c |  2 +-
 6 files changed, 36 insertions(+), 29 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 3a790eb4995c..e2880f6deb34 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -707,7 +707,7 @@ static void debugfs_create_files(struct dentry *parent, void *data,
 void blk_mq_debugfs_register(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	debugfs_create_files(q->debugfs_dir, q, blk_mq_debugfs_queue_attrs);
 
@@ -780,7 +780,7 @@ void blk_mq_debugfs_unregister_hctx(struct blk_mq_hw_ctx *hctx)
 void blk_mq_debugfs_register_hctxs(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	queue_for_each_hw_ctx(q, hctx, i)
 		blk_mq_debugfs_register_hctx(q, hctx);
@@ -789,7 +789,7 @@ void blk_mq_debugfs_register_hctxs(struct request_queue *q)
 void blk_mq_debugfs_unregister_hctxs(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	queue_for_each_hw_ctx(q, hctx, i)
 		blk_mq_debugfs_unregister_hctx(hctx);
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 55488ba97823..e6ad8f761474 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -515,7 +515,7 @@ static void blk_mq_exit_sched_shared_tags(struct request_queue *queue)
 static void blk_mq_sched_tags_teardown(struct request_queue *q, unsigned int flags)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	queue_for_each_hw_ctx(q, hctx, i) {
 		if (hctx->sched_tags) {
@@ -550,9 +550,10 @@ static int blk_mq_init_sched_shared_tags(struct request_queue *queue)
 
 int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
 {
-	unsigned int i, flags = q->tag_set->flags;
+	unsigned int flags = q->tag_set->flags;
 	struct blk_mq_hw_ctx *hctx;
 	struct elevator_queue *eq;
+	unsigned long i;
 	int ret;
 
 	if (!e) {
@@ -618,7 +619,7 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
 void blk_mq_sched_free_rqs(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	if (blk_mq_is_shared_tags(q->tag_set->flags)) {
 		blk_mq_free_rqs(q->tag_set, q->sched_shared_tags,
@@ -635,7 +636,7 @@ void blk_mq_sched_free_rqs(struct request_queue *q)
 void blk_mq_exit_sched(struct request_queue *q, struct elevator_queue *e)
 {
 	struct blk_mq_hw_ctx *hctx;
-	unsigned int i;
+	unsigned long i;
 	unsigned int flags = 0;
 
 	queue_for_each_hw_ctx(q, hctx, i) {
diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
index 674786574075..c08426975856 100644
--- a/block/blk-mq-sysfs.c
+++ b/block/blk-mq-sysfs.c
@@ -206,7 +206,7 @@ static int blk_mq_register_hctx(struct blk_mq_hw_ctx *hctx)
 void blk_mq_unregister_dev(struct device *dev, struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	lockdep_assert_held(&q->sysfs_dir_lock);
 
@@ -255,7 +255,8 @@ void blk_mq_sysfs_init(struct request_queue *q)
 int __blk_mq_register_dev(struct device *dev, struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int ret, i;
+	unsigned long i, j;
+	int ret;
 
 	WARN_ON_ONCE(!q->kobj.parent);
 	lockdep_assert_held(&q->sysfs_dir_lock);
@@ -278,8 +279,10 @@ int __blk_mq_register_dev(struct device *dev, struct request_queue *q)
 	return ret;
 
 unreg:
-	while (--i >= 0)
-		blk_mq_unregister_hctx(q->queue_hw_ctx[i]);
+	queue_for_each_hw_ctx(q, hctx, j) {
+		if (j < i)
+			blk_mq_unregister_hctx(hctx);
+	}
 
 	kobject_uevent(q->mq_kobj, KOBJ_REMOVE);
 	kobject_del(q->mq_kobj);
@@ -290,7 +293,7 @@ int __blk_mq_register_dev(struct device *dev, struct request_queue *q)
 void blk_mq_sysfs_unregister(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	mutex_lock(&q->sysfs_dir_lock);
 	if (!q->mq_sysfs_init_done)
@@ -306,7 +309,8 @@ void blk_mq_sysfs_unregister(struct request_queue *q)
 int blk_mq_sysfs_register(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i, ret = 0;
+	unsigned long i;
+	int ret = 0;
 
 	mutex_lock(&q->sysfs_dir_lock);
 	if (!q->mq_sysfs_init_done)
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index 0fd409b8e86e..1850a4225e12 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -515,7 +515,7 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_tag_iter_fn *fn,
 		bt_for_each(NULL, q, btags, fn, priv, false);
 	} else {
 		struct blk_mq_hw_ctx *hctx;
-		int i;
+		unsigned long i;
 
 		queue_for_each_hw_ctx(q, hctx, i) {
 			struct blk_mq_tags *tags = hctx->tags;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 57ae9df0f4dc..bffdd71c670d 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -312,7 +312,7 @@ EXPORT_SYMBOL_GPL(blk_mq_unquiesce_queue);
 void blk_mq_wake_waiters(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
-	unsigned int i;
+	unsigned long i;
 
 	queue_for_each_hw_ctx(q, hctx, i)
 		if (blk_mq_hw_queue_mapped(hctx))
@@ -1442,7 +1442,7 @@ static void blk_mq_timeout_work(struct work_struct *work)
 		container_of(work, struct request_queue, timeout_work);
 	unsigned long next = 0;
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	/* A deadlock might occur if a request is stuck requiring a
 	 * timeout at the same time a queue freeze is waiting
@@ -2143,7 +2143,7 @@ static struct blk_mq_hw_ctx *blk_mq_get_sq_hctx(struct request_queue *q)
 void blk_mq_run_hw_queues(struct request_queue *q, bool async)
 {
 	struct blk_mq_hw_ctx *hctx, *sq_hctx;
-	int i;
+	unsigned long i;
 
 	sq_hctx = NULL;
 	if (blk_mq_has_sqsched(q))
@@ -2171,7 +2171,7 @@ EXPORT_SYMBOL(blk_mq_run_hw_queues);
 void blk_mq_delay_run_hw_queues(struct request_queue *q, unsigned long msecs)
 {
 	struct blk_mq_hw_ctx *hctx, *sq_hctx;
-	int i;
+	unsigned long i;
 
 	sq_hctx = NULL;
 	if (blk_mq_has_sqsched(q))
@@ -2209,7 +2209,7 @@ EXPORT_SYMBOL(blk_mq_delay_run_hw_queues);
 bool blk_mq_queue_stopped(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	queue_for_each_hw_ctx(q, hctx, i)
 		if (blk_mq_hctx_stopped(hctx))
@@ -2248,7 +2248,7 @@ EXPORT_SYMBOL(blk_mq_stop_hw_queue);
 void blk_mq_stop_hw_queues(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	queue_for_each_hw_ctx(q, hctx, i)
 		blk_mq_stop_hw_queue(hctx);
@@ -2266,7 +2266,7 @@ EXPORT_SYMBOL(blk_mq_start_hw_queue);
 void blk_mq_start_hw_queues(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	queue_for_each_hw_ctx(q, hctx, i)
 		blk_mq_start_hw_queue(hctx);
@@ -2286,7 +2286,7 @@ EXPORT_SYMBOL_GPL(blk_mq_start_stopped_hw_queue);
 void blk_mq_start_stopped_hw_queues(struct request_queue *q, bool async)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	queue_for_each_hw_ctx(q, hctx, i)
 		blk_mq_start_stopped_hw_queue(hctx, async);
@@ -3446,7 +3446,7 @@ static void blk_mq_exit_hw_queues(struct request_queue *q,
 		struct blk_mq_tag_set *set, int nr_queue)
 {
 	struct blk_mq_hw_ctx *hctx;
-	unsigned int i;
+	unsigned long i;
 
 	queue_for_each_hw_ctx(q, hctx, i) {
 		if (i == nr_queue)
@@ -3637,7 +3637,8 @@ static void __blk_mq_free_map_and_rqs(struct blk_mq_tag_set *set,
 
 static void blk_mq_map_swqueue(struct request_queue *q)
 {
-	unsigned int i, j, hctx_idx;
+	unsigned int j, hctx_idx;
+	unsigned long i;
 	struct blk_mq_hw_ctx *hctx;
 	struct blk_mq_ctx *ctx;
 	struct blk_mq_tag_set *set = q->tag_set;
@@ -3744,7 +3745,7 @@ static void blk_mq_map_swqueue(struct request_queue *q)
 static void queue_set_hctx_shared(struct request_queue *q, bool shared)
 {
 	struct blk_mq_hw_ctx *hctx;
-	int i;
+	unsigned long i;
 
 	queue_for_each_hw_ctx(q, hctx, i) {
 		if (shared) {
@@ -3844,7 +3845,7 @@ static int blk_mq_alloc_ctxs(struct request_queue *q)
 void blk_mq_release(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx, *next;
-	int i;
+	unsigned long i;
 
 	queue_for_each_hw_ctx(q, hctx, i)
 		WARN_ON_ONCE(hctx && list_empty(&hctx->hctx_list));
@@ -4362,7 +4363,8 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)
 {
 	struct blk_mq_tag_set *set = q->tag_set;
 	struct blk_mq_hw_ctx *hctx;
-	int i, ret;
+	int ret;
+	unsigned long i;
 
 	if (!set)
 		return -EINVAL;
@@ -4738,7 +4740,7 @@ void blk_mq_cancel_work_sync(struct request_queue *q)
 {
 	if (queue_is_mq(q)) {
 		struct blk_mq_hw_ctx *hctx;
-		int i;
+		unsigned long i;
 
 		cancel_delayed_work_sync(&q->requeue_work);
 
diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
index c08971de369f..58304f978e10 100644
--- a/drivers/block/rnbd/rnbd-clt.c
+++ b/drivers/block/rnbd/rnbd-clt.c
@@ -1343,7 +1343,7 @@ static inline void rnbd_init_hw_queue(struct rnbd_clt_dev *dev,
 
 static void rnbd_init_mq_hw_queues(struct rnbd_clt_dev *dev)
 {
-	int i;
+	unsigned long i;
 	struct blk_mq_hw_ctx *hctx;
 	struct rnbd_queue *q;
 
-- 
2.31.1

Re: [PATCH V3 5/6] blk-mq: prepare for implementing hctx table via xarray

[Christoph Hellwig]

Even if I still think this would make more sense as part of the next
patch, the mechanical changes do look fine to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH V3 5/6] blk-mq: prepare for implementing hctx table via xarray

[Hannes Reinecke]

On 3/7/22 07:44, Ming Lei wrote:
> It is inevitable to cause use-after-free on q->queue_hw_ctx between
> queue_for_each_hw_ctx() and blk_mq_update_nr_hw_queues(). And converting
> to xarray can fix the uaf, meantime code gets cleaner.
> 
> Prepare for converting q->queue_hctx_ctx into xarray, one thing is that
> xa_for_each() can only accept 'unsigned long' as index, so changes type
> of hctx index of queue_for_each_hw_ctx() into 'unsigned long'.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-mq-debugfs.c        |  6 +++---
>   block/blk-mq-sched.c          |  9 +++++----
>   block/blk-mq-sysfs.c          | 16 ++++++++++------
>   block/blk-mq-tag.c            |  2 +-
>   block/blk-mq.c                | 30 ++++++++++++++++--------------
>   drivers/block/rnbd/rnbd-clt.c |  2 +-
>   6 files changed, 36 insertions(+), 29 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH V3 6/6] blk-mq: manage hctx map via xarray

[Ming Lei]

Firstly code becomes more clean by switching to xarray from plain array.

Secondly use-after-free on q->queue_hw_ctx can be fixed because
queue_for_each_hw_ctx() may be run when updating nr_hw_queues is
in-progress. With this patch, q->hctx_table is defined as xarray, and
this structure will share same lifetime with request queue, so
queue_for_each_hw_ctx() can use q->hctx_table to lookup hctx reliably.

Reported-by: Yu Kuai <yukuai3@huawei.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-tag.c     |  2 +-
 block/blk-mq.c         | 55 ++++++++++++++++++------------------------
 block/blk-mq.h         |  2 +-
 include/linux/blk-mq.h |  3 +--
 include/linux/blkdev.h |  2 +-
 5 files changed, 28 insertions(+), 36 deletions(-)

diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index 1850a4225e12..68ac23d0b640 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -498,7 +498,7 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_tag_iter_fn *fn,
 		void *priv)
 {
 	/*
-	 * __blk_mq_update_nr_hw_queues() updates nr_hw_queues and queue_hw_ctx
+	 * __blk_mq_update_nr_hw_queues() updates nr_hw_queues and hctx_table
 	 * while the queue is frozen. So we can use q_usage_counter to avoid
 	 * racing with it.
 	 */
diff --git a/block/blk-mq.c b/block/blk-mq.c
index bffdd71c670d..a15d12fb227c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -71,7 +71,8 @@ static int blk_mq_poll_stats_bkt(const struct request *rq)
 static inline struct blk_mq_hw_ctx *blk_qc_to_hctx(struct request_queue *q,
 		blk_qc_t qc)
 {
-	return q->queue_hw_ctx[(qc & ~BLK_QC_T_INTERNAL) >> BLK_QC_T_SHIFT];
+	return xa_load(&q->hctx_table,
+			(qc & ~BLK_QC_T_INTERNAL) >> BLK_QC_T_SHIFT);
 }
 
 static inline struct request *blk_qc_to_rq(struct blk_mq_hw_ctx *hctx,
@@ -573,7 +574,7 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
 	 * If not tell the caller that it should skip this queue.
 	 */
 	ret = -EXDEV;
-	data.hctx = q->queue_hw_ctx[hctx_idx];
+	data.hctx = xa_load(&q->hctx_table, hctx_idx);
 	if (!blk_mq_hw_queue_mapped(data.hctx))
 		goto out_queue_exit;
 	cpu = cpumask_first_and(data.hctx->cpumask, cpu_online_mask);
@@ -3437,6 +3438,8 @@ static void blk_mq_exit_hctx(struct request_queue *q,
 
 	blk_mq_remove_cpuhp(hctx);
 
+	xa_erase(&q->hctx_table, hctx_idx);
+
 	spin_lock(&q->unused_hctx_lock);
 	list_add(&hctx->hctx_list, &q->unused_hctx_list);
 	spin_unlock(&q->unused_hctx_lock);
@@ -3476,8 +3479,15 @@ static int blk_mq_init_hctx(struct request_queue *q,
 	if (blk_mq_init_request(set, hctx->fq->flush_rq, hctx_idx,
 				hctx->numa_node))
 		goto exit_hctx;
+
+	if (xa_insert(&q->hctx_table, hctx_idx, hctx, GFP_KERNEL))
+		goto exit_flush_rq;
+
 	return 0;
 
+ exit_flush_rq:
+	if (set->ops->exit_request)
+		set->ops->exit_request(set, hctx->fq->flush_rq, hctx_idx);
  exit_hctx:
 	if (set->ops->exit_hctx)
 		set->ops->exit_hctx(hctx, hctx_idx);
@@ -3856,7 +3866,7 @@ void blk_mq_release(struct request_queue *q)
 		kobject_put(&hctx->kobj);
 	}
 
-	kfree(q->queue_hw_ctx);
+	xa_destroy(&q->hctx_table);
 
 	/*
 	 * release .mq_kobj and sw queue's kobject now because
@@ -3946,45 +3956,28 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 						struct request_queue *q)
 {
 	int i, j, end;
-	struct blk_mq_hw_ctx **hctxs = q->queue_hw_ctx;
-
-	if (q->nr_hw_queues < set->nr_hw_queues) {
-		struct blk_mq_hw_ctx **new_hctxs;
-
-		new_hctxs = kcalloc_node(set->nr_hw_queues,
-				       sizeof(*new_hctxs), GFP_KERNEL,
-				       set->numa_node);
-		if (!new_hctxs)
-			return;
-		if (hctxs)
-			memcpy(new_hctxs, hctxs, q->nr_hw_queues *
-			       sizeof(*hctxs));
-		q->queue_hw_ctx = new_hctxs;
-		kfree(hctxs);
-		hctxs = new_hctxs;
-	}
 
 	/* protect against switching io scheduler  */
 	mutex_lock(&q->sysfs_lock);
 	for (i = 0; i < set->nr_hw_queues; i++) {
 		int old_node;
 		int node = blk_mq_get_hctx_node(set, i);
-		struct blk_mq_hw_ctx *old_hctx = hctxs[i];
+		struct blk_mq_hw_ctx *old_hctx = xa_load(&q->hctx_table, i);
 
 		if (old_hctx) {
 			old_node = old_hctx->numa_node;
 			blk_mq_exit_hctx(q, set, old_hctx, i);
 		}
 
-		hctxs[i] = blk_mq_alloc_and_init_hctx(set, q, i, node);
-		if (!hctxs[i]) {
+		if (!blk_mq_alloc_and_init_hctx(set, q, i, node)) {
+			struct blk_mq_hw_ctx *hctx;
+
 			if (!old_hctx)
 				break;
 			pr_warn("Allocate new hctx on node %d fails, fallback to previous one on node %d\n",
 					node, old_node);
-			hctxs[i] = blk_mq_alloc_and_init_hctx(set, q, i,
-					old_node);
-			WARN_ON_ONCE(!hctxs[i]);
+			hctx = blk_mq_alloc_and_init_hctx(set, q, i, old_node);
+			WARN_ON_ONCE(!hctx);
 		}
 	}
 	/*
@@ -4001,12 +3994,10 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 	}
 
 	for (; j < end; j++) {
-		struct blk_mq_hw_ctx *hctx = hctxs[j];
+		struct blk_mq_hw_ctx *hctx = xa_load(&q->hctx_table, j);
 
-		if (hctx) {
+		if (hctx)
 			blk_mq_exit_hctx(q, set, hctx, j);
-			hctxs[j] = NULL;
-		}
 	}
 	mutex_unlock(&q->sysfs_lock);
 }
@@ -4046,6 +4037,8 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 	INIT_LIST_HEAD(&q->unused_hctx_list);
 	spin_lock_init(&q->unused_hctx_lock);
 
+	xa_init(&q->hctx_table);
+
 	blk_mq_realloc_hw_ctxs(set, q);
 	if (!q->nr_hw_queues)
 		goto err_hctxs;
@@ -4075,7 +4068,7 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 	return 0;
 
 err_hctxs:
-	kfree(q->queue_hw_ctx);
+	xa_destroy(&q->hctx_table);
 	q->nr_hw_queues = 0;
 	blk_mq_sysfs_deinit(q);
 err_poll:
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 948791ea2a3e..2615bd58bad3 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -83,7 +83,7 @@ static inline struct blk_mq_hw_ctx *blk_mq_map_queue_type(struct request_queue *
 							  enum hctx_type type,
 							  unsigned int cpu)
 {
-	return q->queue_hw_ctx[q->tag_set->map[type].mq_map[cpu]];
+	return xa_load(&q->hctx_table, q->tag_set->map[type].mq_map[cpu]);
 }
 
 static inline enum hctx_type blk_mq_get_hctx_type(unsigned int flags)
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 3a41d50b85d3..7aa5c54901a9 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -917,8 +917,7 @@ static inline void *blk_mq_rq_to_pdu(struct request *rq)
 }
 
 #define queue_for_each_hw_ctx(q, hctx, i)				\
-	for ((i) = 0; (i) < (q)->nr_hw_queues &&			\
-	     ({ hctx = (q)->queue_hw_ctx[i]; 1; }); (i)++)
+	xa_for_each(&(q)->hctx_table, (i), (hctx))
 
 #define hctx_for_each_ctx(hctx, ctx, i)					\
 	for ((i) = 0; (i) < (hctx)->nr_ctx &&				\
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index f757f9c2871f..a53ae40aaded 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -355,7 +355,7 @@ struct request_queue {
 	unsigned int		queue_depth;
 
 	/* hw dispatch queues */
-	struct blk_mq_hw_ctx	**queue_hw_ctx;
+	struct xarray		hctx_table;
 	unsigned int		nr_hw_queues;
 
 	/*
-- 
2.31.1

Re: [PATCH V3 6/6] blk-mq: manage hctx map via xarray

[Christoph Hellwig]

On Mon, Mar 07, 2022 at 02:44:01PM +0800, Ming Lei wrote:
> Firstly code becomes more clean by switching to xarray from plain array.
> 
> Secondly use-after-free on q->queue_hw_ctx can be fixed because

Not a native speaker, but shouldn't this read First and Second?

>  	mutex_lock(&q->sysfs_lock);
>  	for (i = 0; i < set->nr_hw_queues; i++) {
>  		int old_node;
>  		int node = blk_mq_get_hctx_node(set, i);
> -		struct blk_mq_hw_ctx *old_hctx = hctxs[i];
> +		struct blk_mq_hw_ctx *old_hctx = xa_load(&q->hctx_table, i);

This should cand can xa_for_each_range.

>  	for (; j < end; j++) {
> -		struct blk_mq_hw_ctx *hctx = hctxs[j];
> +		struct blk_mq_hw_ctx *hctx = xa_load(&q->hctx_table, j);
>  
> -		if (hctx) {
> +		if (hctx)
>  			blk_mq_exit_hctx(q, set, hctx, j);
> -			hctxs[j] = NULL;
> -		}

Same here.

Re: [PATCH V3 6/6] blk-mq: manage hctx map via xarray

[Ming Lei]

On Mon, Mar 07, 2022 at 08:13:17AM +0100, Christoph Hellwig wrote:
> On Mon, Mar 07, 2022 at 02:44:01PM +0800, Ming Lei wrote:
> > Firstly code becomes more clean by switching to xarray from plain array.
> > 
> > Secondly use-after-free on q->queue_hw_ctx can be fixed because
> 
> Not a native speaker, but shouldn't this read First and Second?

OK, will fix it in next version.

> 
> >  	mutex_lock(&q->sysfs_lock);
> >  	for (i = 0; i < set->nr_hw_queues; i++) {
> >  		int old_node;
> >  		int node = blk_mq_get_hctx_node(set, i);
> > -		struct blk_mq_hw_ctx *old_hctx = hctxs[i];
> > +		struct blk_mq_hw_ctx *old_hctx = xa_load(&q->hctx_table, i);
> 
> This should cand can xa_for_each_range.

It may not work here since xa_for_each_range() breaks if NULL entry is
found. Even two loops can't work too because we need old numa node
for reallocation.

> 
> >  	for (; j < end; j++) {
> > -		struct blk_mq_hw_ctx *hctx = hctxs[j];
> > +		struct blk_mq_hw_ctx *hctx = xa_load(&q->hctx_table, j);
> >  
> > -		if (hctx) {
> > +		if (hctx)
> >  			blk_mq_exit_hctx(q, set, hctx, j);
> > -			hctxs[j] = NULL;
> > -		}

This one can be converted directly.


Thanks,
Ming

Re: [PATCH V3 6/6] blk-mq: manage hctx map via xarray

[Hannes Reinecke]

On 3/7/22 07:44, Ming Lei wrote:
> Firstly code becomes more clean by switching to xarray from plain array.
> 
> Secondly use-after-free on q->queue_hw_ctx can be fixed because
> queue_for_each_hw_ctx() may be run when updating nr_hw_queues is
> in-progress. With this patch, q->hctx_table is defined as xarray, and
> this structure will share same lifetime with request queue, so
> queue_for_each_hw_ctx() can use q->hctx_table to lookup hctx reliably.
> 
> Reported-by: Yu Kuai <yukuai3@huawei.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-mq-tag.c     |  2 +-
>   block/blk-mq.c         | 55 ++++++++++++++++++------------------------
>   block/blk-mq.h         |  2 +-
>   include/linux/blk-mq.h |  3 +--
>   include/linux/blkdev.h |  2 +-
>   5 files changed, 28 insertions(+), 36 deletions(-)
> 
> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> index 1850a4225e12..68ac23d0b640 100644
> --- a/block/blk-mq-tag.c
> +++ b/block/blk-mq-tag.c
> @@ -498,7 +498,7 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_tag_iter_fn *fn,
>   		void *priv)
>   {
>   	/*
> -	 * __blk_mq_update_nr_hw_queues() updates nr_hw_queues and queue_hw_ctx
> +	 * __blk_mq_update_nr_hw_queues() updates nr_hw_queues and hctx_table
>   	 * while the queue is frozen. So we can use q_usage_counter to avoid
>   	 * racing with it.
>   	 */
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index bffdd71c670d..a15d12fb227c 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -71,7 +71,8 @@ static int blk_mq_poll_stats_bkt(const struct request *rq)
>   static inline struct blk_mq_hw_ctx *blk_qc_to_hctx(struct request_queue *q,
>   		blk_qc_t qc)
>   {
> -	return q->queue_hw_ctx[(qc & ~BLK_QC_T_INTERNAL) >> BLK_QC_T_SHIFT];
> +	return xa_load(&q->hctx_table,
> +			(qc & ~BLK_QC_T_INTERNAL) >> BLK_QC_T_SHIFT);
>   }
>   
>   static inline struct request *blk_qc_to_rq(struct blk_mq_hw_ctx *hctx,
> @@ -573,7 +574,7 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
>   	 * If not tell the caller that it should skip this queue.
>   	 */
>   	ret = -EXDEV;
> -	data.hctx = q->queue_hw_ctx[hctx_idx];
> +	data.hctx = xa_load(&q->hctx_table, hctx_idx);
>   	if (!blk_mq_hw_queue_mapped(data.hctx))
>   		goto out_queue_exit;
>   	cpu = cpumask_first_and(data.hctx->cpumask, cpu_online_mask);
> @@ -3437,6 +3438,8 @@ static void blk_mq_exit_hctx(struct request_queue *q,
>   
>   	blk_mq_remove_cpuhp(hctx);
>   
> +	xa_erase(&q->hctx_table, hctx_idx);
> +
>   	spin_lock(&q->unused_hctx_lock);
>   	list_add(&hctx->hctx_list, &q->unused_hctx_list);
>   	spin_unlock(&q->unused_hctx_lock);
> @@ -3476,8 +3479,15 @@ static int blk_mq_init_hctx(struct request_queue *q,
>   	if (blk_mq_init_request(set, hctx->fq->flush_rq, hctx_idx,
>   				hctx->numa_node))
>   		goto exit_hctx;
> +
> +	if (xa_insert(&q->hctx_table, hctx_idx, hctx, GFP_KERNEL))
> +		goto exit_flush_rq;
> +
>   	return 0;
>    > + exit_flush_rq:
> +	if (set->ops->exit_request)
> +		set->ops->exit_request(set, hctx->fq->flush_rq, hctx_idx);

Why is this here? It's not directly related to the xarray conversion, so 
it should rather go into a separate patch.

>    exit_hctx:
>   	if (set->ops->exit_hctx)
>   		set->ops->exit_hctx(hctx, hctx_idx);
> @@ -3856,7 +3866,7 @@ void blk_mq_release(struct request_queue *q)
>   		kobject_put(&hctx->kobj);
>   	}
>   
> -	kfree(q->queue_hw_ctx);
> +	xa_destroy(&q->hctx_table);
>   
>   	/*
>   	 * release .mq_kobj and sw queue's kobject now because
> @@ -3946,45 +3956,28 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
>   						struct request_queue *q)
>   {
>   	int i, j, end;
> -	struct blk_mq_hw_ctx **hctxs = q->queue_hw_ctx;
> -
> -	if (q->nr_hw_queues < set->nr_hw_queues) {
> -		struct blk_mq_hw_ctx **new_hctxs;
> -
> -		new_hctxs = kcalloc_node(set->nr_hw_queues,
> -				       sizeof(*new_hctxs), GFP_KERNEL,
> -				       set->numa_node);
> -		if (!new_hctxs)
> -			return;
> -		if (hctxs)
> -			memcpy(new_hctxs, hctxs, q->nr_hw_queues *
> -			       sizeof(*hctxs));
> -		q->queue_hw_ctx = new_hctxs;
> -		kfree(hctxs);
> -		hctxs = new_hctxs;
> -	}
>   
>   	/* protect against switching io scheduler  */
>   	mutex_lock(&q->sysfs_lock);
>   	for (i = 0; i < set->nr_hw_queues; i++) {
>   		int old_node;
>   		int node = blk_mq_get_hctx_node(set, i);
> -		struct blk_mq_hw_ctx *old_hctx = hctxs[i];
> +		struct blk_mq_hw_ctx *old_hctx = xa_load(&q->hctx_table, i);
>   
>   		if (old_hctx) {
>   			old_node = old_hctx->numa_node;
>   			blk_mq_exit_hctx(q, set, old_hctx, i);
>   		}
>   
> -		hctxs[i] = blk_mq_alloc_and_init_hctx(set, q, i, node);
> -		if (!hctxs[i]) {
> +		if (!blk_mq_alloc_and_init_hctx(set, q, i, node)) {
> +			struct blk_mq_hw_ctx *hctx;
> +
>   			if (!old_hctx)
>   				break;
>   			pr_warn("Allocate new hctx on node %d fails, fallback to previous one on node %d\n",
>   					node, old_node);
> -			hctxs[i] = blk_mq_alloc_and_init_hctx(set, q, i,
> -					old_node);
> -			WARN_ON_ONCE(!hctxs[i]);
> +			hctx = blk_mq_alloc_and_init_hctx(set, q, i, old_node);
> +			WARN_ON_ONCE(!hctx);
>   		}
>   	}
>   	/*
> @@ -4001,12 +3994,10 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
>   	}
>   
>   	for (; j < end; j++) {
> -		struct blk_mq_hw_ctx *hctx = hctxs[j];
> +		struct blk_mq_hw_ctx *hctx = xa_load(&q->hctx_table, j);
>   
> -		if (hctx) {
> +		if (hctx)
>   			blk_mq_exit_hctx(q, set, hctx, j);
> -			hctxs[j] = NULL;
> -		}

Do you need to call 'xa_load' here? Isn't it sufficient to call
blk_mq_exit_hctx() and have it skip any non-present entries?

>   	}
>   	mutex_unlock(&q->sysfs_lock);
>   }
> @@ -4046,6 +4037,8 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
>   	INIT_LIST_HEAD(&q->unused_hctx_list);
>   	spin_lock_init(&q->unused_hctx_lock);
>   
> +	xa_init(&q->hctx_table);
> +
>   	blk_mq_realloc_hw_ctxs(set, q);
>   	if (!q->nr_hw_queues)
>   		goto err_hctxs;
> @@ -4075,7 +4068,7 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
>   	return 0;
>   
>   err_hctxs:
> -	kfree(q->queue_hw_ctx);
> +	xa_destroy(&q->hctx_table);
>   	q->nr_hw_queues = 0;
>   	blk_mq_sysfs_deinit(q);
>   err_poll:
> diff --git a/block/blk-mq.h b/block/blk-mq.h
> index 948791ea2a3e..2615bd58bad3 100644
> --- a/block/blk-mq.h
> +++ b/block/blk-mq.h
> @@ -83,7 +83,7 @@ static inline struct blk_mq_hw_ctx *blk_mq_map_queue_type(struct request_queue *
>   							  enum hctx_type type,
>   							  unsigned int cpu)
>   {
> -	return q->queue_hw_ctx[q->tag_set->map[type].mq_map[cpu]];
> +	return xa_load(&q->hctx_table, q->tag_set->map[type].mq_map[cpu]);
>   }
>   
>   static inline enum hctx_type blk_mq_get_hctx_type(unsigned int flags)
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 3a41d50b85d3..7aa5c54901a9 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -917,8 +917,7 @@ static inline void *blk_mq_rq_to_pdu(struct request *rq)
>   }
>   
>   #define queue_for_each_hw_ctx(q, hctx, i)				\
> -	for ((i) = 0; (i) < (q)->nr_hw_queues &&			\
> -	     ({ hctx = (q)->queue_hw_ctx[i]; 1; }); (i)++)
> +	xa_for_each(&(q)->hctx_table, (i), (hctx))
>   
>   #define hctx_for_each_ctx(hctx, ctx, i)					\
>   	for ((i) = 0; (i) < (hctx)->nr_ctx &&				\
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index f757f9c2871f..a53ae40aaded 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -355,7 +355,7 @@ struct request_queue {
>   	unsigned int		queue_depth;
>   
>   	/* hw dispatch queues */
> -	struct blk_mq_hw_ctx	**queue_hw_ctx;
> +	struct xarray		hctx_table;
>   	unsigned int		nr_hw_queues;
>   
>   	/*

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH V3 6/6] blk-mq: manage hctx map via xarray

[Ming Lei]

On Mon, Mar 07, 2022 at 08:44:24AM +0100, Hannes Reinecke wrote:
> On 3/7/22 07:44, Ming Lei wrote:
> > Firstly code becomes more clean by switching to xarray from plain array.
> > 
> > Secondly use-after-free on q->queue_hw_ctx can be fixed because
> > queue_for_each_hw_ctx() may be run when updating nr_hw_queues is
> > in-progress. With this patch, q->hctx_table is defined as xarray, and
> > this structure will share same lifetime with request queue, so
> > queue_for_each_hw_ctx() can use q->hctx_table to lookup hctx reliably.
> > 
> > Reported-by: Yu Kuai <yukuai3@huawei.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >   block/blk-mq-tag.c     |  2 +-
> >   block/blk-mq.c         | 55 ++++++++++++++++++------------------------
> >   block/blk-mq.h         |  2 +-
> >   include/linux/blk-mq.h |  3 +--
> >   include/linux/blkdev.h |  2 +-
> >   5 files changed, 28 insertions(+), 36 deletions(-)
> > 
> > diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> > index 1850a4225e12..68ac23d0b640 100644
> > --- a/block/blk-mq-tag.c
> > +++ b/block/blk-mq-tag.c
> > @@ -498,7 +498,7 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_tag_iter_fn *fn,
> >   		void *priv)
> >   {
> >   	/*
> > -	 * __blk_mq_update_nr_hw_queues() updates nr_hw_queues and queue_hw_ctx
> > +	 * __blk_mq_update_nr_hw_queues() updates nr_hw_queues and hctx_table
> >   	 * while the queue is frozen. So we can use q_usage_counter to avoid
> >   	 * racing with it.
> >   	 */
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index bffdd71c670d..a15d12fb227c 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -71,7 +71,8 @@ static int blk_mq_poll_stats_bkt(const struct request *rq)
> >   static inline struct blk_mq_hw_ctx *blk_qc_to_hctx(struct request_queue *q,
> >   		blk_qc_t qc)
> >   {
> > -	return q->queue_hw_ctx[(qc & ~BLK_QC_T_INTERNAL) >> BLK_QC_T_SHIFT];
> > +	return xa_load(&q->hctx_table,
> > +			(qc & ~BLK_QC_T_INTERNAL) >> BLK_QC_T_SHIFT);
> >   }
> >   static inline struct request *blk_qc_to_rq(struct blk_mq_hw_ctx *hctx,
> > @@ -573,7 +574,7 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
> >   	 * If not tell the caller that it should skip this queue.
> >   	 */
> >   	ret = -EXDEV;
> > -	data.hctx = q->queue_hw_ctx[hctx_idx];
> > +	data.hctx = xa_load(&q->hctx_table, hctx_idx);
> >   	if (!blk_mq_hw_queue_mapped(data.hctx))
> >   		goto out_queue_exit;
> >   	cpu = cpumask_first_and(data.hctx->cpumask, cpu_online_mask);
> > @@ -3437,6 +3438,8 @@ static void blk_mq_exit_hctx(struct request_queue *q,
> >   	blk_mq_remove_cpuhp(hctx);
> > +	xa_erase(&q->hctx_table, hctx_idx);
> > +
> >   	spin_lock(&q->unused_hctx_lock);
> >   	list_add(&hctx->hctx_list, &q->unused_hctx_list);
> >   	spin_unlock(&q->unused_hctx_lock);
> > @@ -3476,8 +3479,15 @@ static int blk_mq_init_hctx(struct request_queue *q,
> >   	if (blk_mq_init_request(set, hctx->fq->flush_rq, hctx_idx,
> >   				hctx->numa_node))
> >   		goto exit_hctx;
> > +
> > +	if (xa_insert(&q->hctx_table, hctx_idx, hctx, GFP_KERNEL))
> > +		goto exit_flush_rq;
> > +
> >   	return 0;
> >    > + exit_flush_rq:
> > +	if (set->ops->exit_request)
> > +		set->ops->exit_request(set, hctx->fq->flush_rq, hctx_idx);
> 
> Why is this here? It's not directly related to the xarray conversion, so it
> should rather go into a separate patch.

This new error handling is only needed if xa_insert() fails.

> 
> >    exit_hctx:
> >   	if (set->ops->exit_hctx)
> >   		set->ops->exit_hctx(hctx, hctx_idx);
> > @@ -3856,7 +3866,7 @@ void blk_mq_release(struct request_queue *q)
> >   		kobject_put(&hctx->kobj);
> >   	}
> > -	kfree(q->queue_hw_ctx);
> > +	xa_destroy(&q->hctx_table);
> >   	/*
> >   	 * release .mq_kobj and sw queue's kobject now because
> > @@ -3946,45 +3956,28 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
> >   						struct request_queue *q)
> >   {
> >   	int i, j, end;
> > -	struct blk_mq_hw_ctx **hctxs = q->queue_hw_ctx;
> > -
> > -	if (q->nr_hw_queues < set->nr_hw_queues) {
> > -		struct blk_mq_hw_ctx **new_hctxs;
> > -
> > -		new_hctxs = kcalloc_node(set->nr_hw_queues,
> > -				       sizeof(*new_hctxs), GFP_KERNEL,
> > -				       set->numa_node);
> > -		if (!new_hctxs)
> > -			return;
> > -		if (hctxs)
> > -			memcpy(new_hctxs, hctxs, q->nr_hw_queues *
> > -			       sizeof(*hctxs));
> > -		q->queue_hw_ctx = new_hctxs;
> > -		kfree(hctxs);
> > -		hctxs = new_hctxs;
> > -	}
> >   	/* protect against switching io scheduler  */
> >   	mutex_lock(&q->sysfs_lock);
> >   	for (i = 0; i < set->nr_hw_queues; i++) {
> >   		int old_node;
> >   		int node = blk_mq_get_hctx_node(set, i);
> > -		struct blk_mq_hw_ctx *old_hctx = hctxs[i];
> > +		struct blk_mq_hw_ctx *old_hctx = xa_load(&q->hctx_table, i);
> >   		if (old_hctx) {
> >   			old_node = old_hctx->numa_node;
> >   			blk_mq_exit_hctx(q, set, old_hctx, i);
> >   		}
> > -		hctxs[i] = blk_mq_alloc_and_init_hctx(set, q, i, node);
> > -		if (!hctxs[i]) {
> > +		if (!blk_mq_alloc_and_init_hctx(set, q, i, node)) {
> > +			struct blk_mq_hw_ctx *hctx;
> > +
> >   			if (!old_hctx)
> >   				break;
> >   			pr_warn("Allocate new hctx on node %d fails, fallback to previous one on node %d\n",
> >   					node, old_node);
> > -			hctxs[i] = blk_mq_alloc_and_init_hctx(set, q, i,
> > -					old_node);
> > -			WARN_ON_ONCE(!hctxs[i]);
> > +			hctx = blk_mq_alloc_and_init_hctx(set, q, i, old_node);
> > +			WARN_ON_ONCE(!hctx);
> >   		}
> >   	}
> >   	/*
> > @@ -4001,12 +3994,10 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
> >   	}
> >   	for (; j < end; j++) {
> > -		struct blk_mq_hw_ctx *hctx = hctxs[j];
> > +		struct blk_mq_hw_ctx *hctx = xa_load(&q->hctx_table, j);
> > -		if (hctx) {
> > +		if (hctx)
> >   			blk_mq_exit_hctx(q, set, hctx, j);
> > -			hctxs[j] = NULL;
> > -		}
> 
> Do you need to call 'xa_load' here? Isn't it sufficient to call
> blk_mq_exit_hctx() and have it skip any non-present entries?

As Christoph suggested, xa_for_each_range() can be used here for
exiting any present entry.


Thanks,
Ming