[PATCH 0/2] blk-mq: one revert and one blk-mq kobject fix candidate

[PATCH 0/2] blk-mq: one revert and one blk-mq kobject fix candidate

[Ming Lei]

The 1st patch reverts commit "blk-mq: fix hctx/ctx kobject use-after-free"
(commit 76d697d10769048e) which is wrong and causes scsi regression.

The 2nd patch is another candidate/approach for fixing hctx/ctx kobject
use-after-free.

Sorry for causing the mess, and I am happy to see it fixed by either
Bart Van Assche's fix or this patchset, or other better one.

Thanks,
Ming Lei

[PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free"

[Ming Lei]

This reverts commit 76d697d10769048e5721510100bf3a9413a56385.

The commit 76d697d10769048 causes general protection fault
reported from Bart Van Assche:

	https://lkml.org/lkml/2015/1/28/334

Reported-by: Bart Van Assche <bart.vanassche@sandisk.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 block/blk-mq-sysfs.c |   25 ++-----------------------
 block/blk-mq.c       |    6 +++++-
 2 files changed, 7 insertions(+), 24 deletions(-)

diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
index 6774a0e..1630a20 100644
--- a/block/blk-mq-sysfs.c
+++ b/block/blk-mq-sysfs.c
@@ -15,26 +15,6 @@
 
 static void blk_mq_sysfs_release(struct kobject *kobj)
 {
-	struct request_queue *q;
-
-	q = container_of(kobj, struct request_queue, mq_kobj);
-	free_percpu(q->queue_ctx);
-}
-
-static void blk_mq_ctx_release(struct kobject *kobj)
-{
-	struct blk_mq_ctx *ctx;
-
-	ctx = container_of(kobj, struct blk_mq_ctx, kobj);
-	kobject_put(&ctx->queue->mq_kobj);
-}
-
-static void blk_mq_hctx_release(struct kobject *kobj)
-{
-	struct blk_mq_hw_ctx *hctx;
-
-	hctx = container_of(kobj, struct blk_mq_hw_ctx, kobj);
-	kfree(hctx);
 }
 
 struct blk_mq_ctx_sysfs_entry {
@@ -338,13 +318,13 @@ static struct kobj_type blk_mq_ktype = {
 static struct kobj_type blk_mq_ctx_ktype = {
 	.sysfs_ops	= &blk_mq_sysfs_ops,
 	.default_attrs	= default_ctx_attrs,
-	.release	= blk_mq_ctx_release,
+	.release	= blk_mq_sysfs_release,
 };
 
 static struct kobj_type blk_mq_hw_ktype = {
 	.sysfs_ops	= &blk_mq_hw_sysfs_ops,
 	.default_attrs	= default_hw_ctx_attrs,
-	.release	= blk_mq_hctx_release,
+	.release	= blk_mq_sysfs_release,
 };
 
 static void blk_mq_unregister_hctx(struct blk_mq_hw_ctx *hctx)
@@ -375,7 +355,6 @@ static int blk_mq_register_hctx(struct blk_mq_hw_ctx *hctx)
 		return ret;
 
 	hctx_for_each_ctx(hctx, ctx, i) {
-		kobject_get(&q->mq_kobj);
 		ret = kobject_add(&ctx->kobj, &hctx->kobj, "cpu%u", ctx->cpu);
 		if (ret)
 			break;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 51ce53d..a880b6c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1644,8 +1644,10 @@ static void blk_mq_free_hw_queues(struct request_queue *q,
 	struct blk_mq_hw_ctx *hctx;
 	unsigned int i;
 
-	queue_for_each_hw_ctx(q, hctx, i)
+	queue_for_each_hw_ctx(q, hctx, i) {
 		free_cpumask_var(hctx->cpumask);
+		kfree(hctx);
+	}
 }
 
 static int blk_mq_init_hctx(struct request_queue *q,
@@ -2003,9 +2005,11 @@ void blk_mq_free_queue(struct request_queue *q)
 
 	percpu_ref_exit(&q->mq_usage_counter);
 
+	free_percpu(q->queue_ctx);
 	kfree(q->queue_hw_ctx);
 	kfree(q->mq_map);
 
+	q->queue_ctx = NULL;
 	q->queue_hw_ctx = NULL;
 	q->mq_map = NULL;
 
-- 
1.7.9.5

[PATCH 2/2] blk-mq: release mq's kobjects in blk_release_queue()

[Ming Lei]

The kobject memory inside blk-mq hctx/ctx shouldn't have been freed
before the kobject is released because driver core can access it freely
before its release.

We can't do that in all ctx/hctx/mq_kobj's release handler because
it can be run before blk_cleanup_queue().

Given mq_kobj shouldn't have been introduced, this patch simply moves
mq's release into blk_release_queue().

Reported-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 block/blk-mq.c    |   29 ++++++++++++++++++++++-------
 block/blk-mq.h    |    2 ++
 block/blk-sysfs.c |    2 ++
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index a880b6c..9b250da 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1644,10 +1644,8 @@ static void blk_mq_free_hw_queues(struct request_queue *q,
 	struct blk_mq_hw_ctx *hctx;
 	unsigned int i;
 
-	queue_for_each_hw_ctx(q, hctx, i) {
+	queue_for_each_hw_ctx(q, hctx, i)
 		free_cpumask_var(hctx->cpumask);
-		kfree(hctx);
-	}
 }
 
 static int blk_mq_init_hctx(struct request_queue *q,
@@ -1872,6 +1870,27 @@ static void blk_mq_add_queue_tag_set(struct blk_mq_tag_set *set,
 	mutex_unlock(&set->tag_list_lock);
 }
 
+/*
+ * It is the actual release handler for mq, but we do it from
+ * request queue's release handler for avoiding use-after-free
+ * and headache because q->mq_kobj shouldn't have been introduced,
+ * but we can't group ctx/kctx kobj without it.
+ */
+void blk_mq_release(struct request_queue *q)
+{
+	struct blk_mq_hw_ctx *hctx;
+	unsigned int i;
+
+	/* hctx kobj stays in hctx */
+	queue_for_each_hw_ctx(q, hctx, i)
+		kfree(hctx);
+
+	kfree(q->queue_hw_ctx);
+
+	/* ctx kobj stays in queue_ctx */
+	free_percpu(q->queue_ctx);
+}
+
 struct request_queue *blk_mq_init_queue(struct blk_mq_tag_set *set)
 {
 	struct blk_mq_hw_ctx **hctxs;
@@ -2005,12 +2024,8 @@ void blk_mq_free_queue(struct request_queue *q)
 
 	percpu_ref_exit(&q->mq_usage_counter);
 
-	free_percpu(q->queue_ctx);
-	kfree(q->queue_hw_ctx);
 	kfree(q->mq_map);
 
-	q->queue_ctx = NULL;
-	q->queue_hw_ctx = NULL;
 	q->mq_map = NULL;
 
 	mutex_lock(&all_q_mutex);
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 4f4f943..6a48c4c 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -62,6 +62,8 @@ extern void blk_mq_sysfs_unregister(struct request_queue *q);
 
 extern void blk_mq_rq_timed_out(struct request *req, bool reserved);
 
+void blk_mq_release(struct request_queue *q);
+
 /*
  * Basic implementation of sparser bitmap, allowing the user to spread
  * the bits over more cachelines.
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 935ea2a..faaf36a 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -517,6 +517,8 @@ static void blk_release_queue(struct kobject *kobj)
 
 	if (!q->mq_ops)
 		blk_free_flush_queue(q->fq);
+	else
+		blk_mq_release(q);
 
 	blk_trace_shutdown(q);
 
-- 
1.7.9.5

Re: [PATCH 2/2] blk-mq: release mq's kobjects in blk_release_queue()

[Bart Van Assche]

On 01/29/15 13:18, Ming Lei wrote:
> The kobject memory inside blk-mq hctx/ctx shouldn't have been freed
> before the kobject is released because driver core can access it freely
> before its release.
> 
> We can't do that in all ctx/hctx/mq_kobj's release handler because
> it can be run before blk_cleanup_queue().
> 
> Given mq_kobj shouldn't have been introduced, this patch simply moves
> mq's release into blk_release_queue().

Thanks for the quick respin. With these two patches applied my test
passes (SRP SCSI host removal).

Bart.

Re: [PATCH 0/2] blk-mq: one revert and one blk-mq kobject fix candidate

[Jens Axboe]

On 01/29/2015 04:17 AM, Ming Lei wrote:
> The 1st patch reverts commit "blk-mq: fix hctx/ctx kobject use-after-free"
> (commit 76d697d10769048e) which is wrong and causes scsi regression.
>
> The 2nd patch is another candidate/approach for fixing hctx/ctx kobject
> use-after-free.
>
> Sorry for causing the mess, and I am happy to see it fixed by either
> Bart Van Assche's fix or this patchset, or other better one.

Thanks Ming, this looks better...

-- 
Jens Axboe

Re: [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free"

[Stefan Seyfried]

Am 29.01.2015 um 13:17 schrieb Ming Lei:
> This reverts commit 76d697d10769048e5721510100bf3a9413a56385.
> 
> The commit 76d697d10769048 causes general protection fault
> reported from Bart Van Assche:
> 
> 	https://lkml.org/lkml/2015/1/28/334

I bisected the "unplugging my USB stick crashes the kernel" problem
today and came to this very commit.

The revert is not yet in Linus' tree (but it should get there before
3.19 is released, or all USB-stick users will be unhappy).

Best regards,

	Stefan

> Reported-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
-- 
Stefan Seyfried
Linux Consultant & Developer -- GPG Key: 0x731B665B

B1 Systems GmbH
Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de
GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537

Re: [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free"

[Jens Axboe]

On 02/03/2015 12:13 PM, Stefan Seyfried wrote:
> Am 29.01.2015 um 13:17 schrieb Ming Lei:
>> This reverts commit 76d697d10769048e5721510100bf3a9413a56385.
>>
>> The commit 76d697d10769048 causes general protection fault
>> reported from Bart Van Assche:
>>
>> 	https://lkml.org/lkml/2015/1/28/334
>
> I bisected the "unplugging my USB stick crashes the kernel" problem
> today and came to this very commit.
>
> The revert is not yet in Linus' tree (but it should get there before
> 3.19 is released, or all USB-stick users will be unhappy).

It'll go out later today.

-- 
Jens Axboe

Re: [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free"

[Jens Axboe]

On 02/03/2015 12:14 PM, Jens Axboe wrote:
> On 02/03/2015 12:13 PM, Stefan Seyfried wrote:
>> Am 29.01.2015 um 13:17 schrieb Ming Lei:
>>> This reverts commit 76d697d10769048e5721510100bf3a9413a56385.
>>>
>>> The commit 76d697d10769048 causes general protection fault
>>> reported from Bart Van Assche:
>>>
>>>     https://lkml.org/lkml/2015/1/28/334
>>
>> I bisected the "unplugging my USB stick crashes the kernel" problem
>> today and came to this very commit.
>>
>> The revert is not yet in Linus' tree (but it should get there before
>> 3.19 is released, or all USB-stick users will be unhappy).
>
> It'll go out later today.

It's in Linus' tree now.

-- 
Jens Axboe

Re: [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free"

[Stefan Seyfried]

Am 03.02.2015 um 22:50 schrieb Jens Axboe:
> On 02/03/2015 12:14 PM, Jens Axboe wrote:
>> On 02/03/2015 12:13 PM, Stefan Seyfried wrote:
>>> Am 29.01.2015 um 13:17 schrieb Ming Lei:
>>>> This reverts commit 76d697d10769048e5721510100bf3a9413a56385.
>>> The revert is not yet in Linus' tree (but it should get there before
>>> 3.19 is released, or all USB-stick users will be unhappy).
>>
>> It'll go out later today.
> 
> It's in Linus' tree now.

...and works well for my trivial "plug and unplug an USB stick" testcase.
(I did not want to push, just make sure it wasn't forgotten :)

Thanks all,

	Stefan
-- 
Stefan Seyfried
Linux Consultant & Developer -- GPG Key: 0x731B665B

B1 Systems GmbH
Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de
GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537