LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/2] blk-mq: one revert and one blk-mq kobject fix candidate
@ 2015-01-29 12:17 Ming Lei
  2015-01-29 12:17 ` [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free" Ming Lei
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Ming Lei @ 2015-01-29 12:17 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel; +Cc: Christoph Hellwig, Bart Van Assche, Sasha Levin

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free"
  2015-01-29 12:17 [PATCH 0/2] blk-mq: one revert and one blk-mq kobject fix candidate Ming Lei
@ 2015-01-29 12:17 ` Ming Lei
  2015-02-03 19:13   ` Stefan Seyfried
  2015-01-29 12:17 ` [PATCH 2/2] blk-mq: release mq's kobjects in blk_release_queue() Ming Lei
  2015-01-29 16:31 ` [PATCH 0/2] blk-mq: one revert and one blk-mq kobject fix candidate Jens Axboe
  2 siblings, 1 reply; 9+ messages in thread
From: Ming Lei @ 2015-01-29 12:17 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel
  Cc: Christoph Hellwig, Bart Van Assche, Sasha Levin, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] blk-mq: release mq's kobjects in blk_release_queue()
  2015-01-29 12:17 [PATCH 0/2] blk-mq: one revert and one blk-mq kobject fix candidate Ming Lei
  2015-01-29 12:17 ` [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free" Ming Lei
@ 2015-01-29 12:17 ` Ming Lei
  2015-01-29 12:49   ` Bart Van Assche
  2015-01-29 16:31 ` [PATCH 0/2] blk-mq: one revert and one blk-mq kobject fix candidate Jens Axboe
  2 siblings, 1 reply; 9+ messages in thread
From: Ming Lei @ 2015-01-29 12:17 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel
  Cc: Christoph Hellwig, Bart Van Assche, Sasha Levin, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] blk-mq: release mq's kobjects in blk_release_queue()
  2015-01-29 12:17 ` [PATCH 2/2] blk-mq: release mq's kobjects in blk_release_queue() Ming Lei
@ 2015-01-29 12:49   ` Bart Van Assche
  0 siblings, 0 replies; 9+ messages in thread
From: Bart Van Assche @ 2015-01-29 12:49 UTC (permalink / raw)
  To: Ming Lei, Jens Axboe, linux-kernel; +Cc: Christoph Hellwig, Sasha Levin

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] blk-mq: one revert and one blk-mq kobject fix candidate
  2015-01-29 12:17 [PATCH 0/2] blk-mq: one revert and one blk-mq kobject fix candidate Ming Lei
  2015-01-29 12:17 ` [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free" Ming Lei
  2015-01-29 12:17 ` [PATCH 2/2] blk-mq: release mq's kobjects in blk_release_queue() Ming Lei
@ 2015-01-29 16:31 ` Jens Axboe
  2 siblings, 0 replies; 9+ messages in thread
From: Jens Axboe @ 2015-01-29 16:31 UTC (permalink / raw)
  To: Ming Lei, linux-kernel; +Cc: Christoph Hellwig, Bart Van Assche, Sasha Levin

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free"
  2015-01-29 12:17 ` [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free" Ming Lei
@ 2015-02-03 19:13   ` Stefan Seyfried
  2015-02-03 19:14     ` Jens Axboe
  0 siblings, 1 reply; 9+ messages in thread
From: Stefan Seyfried @ 2015-02-03 19:13 UTC (permalink / raw)
  To: Ming Lei, Jens Axboe, linux-kernel
  Cc: Christoph Hellwig, Bart Van Assche, Sasha Levin

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free"
  2015-02-03 19:13   ` Stefan Seyfried
@ 2015-02-03 19:14     ` Jens Axboe
  2015-02-03 21:50       ` Jens Axboe
  0 siblings, 1 reply; 9+ messages in thread
From: Jens Axboe @ 2015-02-03 19:14 UTC (permalink / raw)
  To: Stefan Seyfried, Ming Lei, linux-kernel
  Cc: Christoph Hellwig, Bart Van Assche, Sasha Levin

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free"
  2015-02-03 19:14     ` Jens Axboe
@ 2015-02-03 21:50       ` Jens Axboe
  2015-02-04  7:43         ` Stefan Seyfried
  0 siblings, 1 reply; 9+ messages in thread
From: Jens Axboe @ 2015-02-03 21:50 UTC (permalink / raw)
  To: Stefan Seyfried, Ming Lei, linux-kernel
  Cc: Christoph Hellwig, Bart Van Assche, Sasha Levin

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free"
  2015-02-03 21:50       ` Jens Axboe
@ 2015-02-04  7:43         ` Stefan Seyfried
  0 siblings, 0 replies; 9+ messages in thread
From: Stefan Seyfried @ 2015-02-04  7:43 UTC (permalink / raw)
  To: Jens Axboe, Ming Lei, linux-kernel
  Cc: Christoph Hellwig, Bart Van Assche, Sasha Levin

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-02-04  7:44 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-29 12:17 [PATCH 0/2] blk-mq: one revert and one blk-mq kobject fix candidate Ming Lei
2015-01-29 12:17 ` [PATCH 1/2] Revert "blk-mq: fix hctx/ctx kobject use-after-free" Ming Lei
2015-02-03 19:13   ` Stefan Seyfried
2015-02-03 19:14     ` Jens Axboe
2015-02-03 21:50       ` Jens Axboe
2015-02-04  7:43         ` Stefan Seyfried
2015-01-29 12:17 ` [PATCH 2/2] blk-mq: release mq's kobjects in blk_release_queue() Ming Lei
2015-01-29 12:49   ` Bart Van Assche
2015-01-29 16:31 ` [PATCH 0/2] blk-mq: one revert and one blk-mq kobject fix candidate Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).