linux-kernel - Re: [PATCH 1/2] Disk hot removal causing oopses and fixes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20091103115444.GA8507@angel.research.nokia.com>
Date:	Tue, 3 Nov 2009 13:54:44 +0200
From:	Jarkko Lavinen <jarkko.lavinen@...ia.com>
To:	ext Stefan Richter <stefanr@...6.in-berlin.de>
Cc:	Jens Axboe <axboe@...nel.dk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>
Subject: Re: [PATCH 1/2] Disk hot removal causing oopses and fixes

Hi Steven

Sorry for late reply.

> It has to reference-count its objects so that they are not freed as long
> as they are used by upper layers,

The block layer and device removal seems to be designed from
top-down approach.  Althouh disc is referenced from
__blkdev_get(), disc's request queue is not.  Also
blk_cleanup_queue() calls elevator_exit() without caring if
anyone still uses the elevator.

A remedy would be to take reference of the request queue in
__blkdev_get() and move elevator_exit() from blk_cleanup_queue()
to blk_release_queue().

I've tried this and it works fine. I am unable to cause oops within
the elevator or anywhere else with hot card removal.  This is how
it should be, since first the queue is marked dead and no new
requests are added into queue and old requests are flushed with
elevator_exit before releasing it and the dead status takes care
they don't even reach to requst issue function.

The caveat is ihat when request queue is initialzed with
blk_init_queue(), caller provides a pointer to spinlock.  When
blk_cleanup_queue() is called, drivers release the lock before
elevator_exit() is finished.  In mmc driver the struct containing
queue_lock is released immediately after returning blk_cleanup_queue().
Then later when request queue is released, elevator_exit() tries to
use the released spinlock.

Kind of hack workaround is to reset the queue_lock pointer to
request queue's internal lock in blk_cleanup_queue().

Jarkko Lavvinen
---
>From 8ccee6132f376d78bb6e355016ecc06c9ca47b9f Mon Sep 17 00:00:00 2001
From: Jarkko Lavinen <jarkko.lavinen@...ia.com>
Date: Tue, 3 Nov 2009 09:48:00 +0200
Subject: [PATCH] block: Move elevator exit from blk_cleanup_queue() to blk_release_queue()

If block_cleanup_queue() should not remove elevator if it is still
in use. Better call the elevator exit in blk_release_queue() when
all the reference to queue are gone.  Since drivers could use queue
locks which are freed after returning from blk_cleanup_queue(),
switch the queue lock to internal one.

Signed-off-by: Jarkko Lavinen <jarkko.lavinen@...ia.com>
---
 block/blk-core.c  |    8 ++++++--
 block/blk-sysfs.c |    3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 9daf621..8d7d5aa 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -449,6 +449,8 @@ void blk_put_queue(struct request_queue *q)

 void blk_cleanup_queue(struct request_queue *q)
 {
+	spinlock_t *oldlock;
+
 	/*
 	 * We know we have process context here, so we can be a little
 	 * cautious and ensure that pending block actions on this device
@@ -461,8 +463,10 @@ void blk_cleanup_queue(struct request_queue *q)
 	queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q);
 	mutex_unlock(&q->sysfs_lock);

-	if (q->elevator)
-		elevator_exit(q->elevator);
+	oldlock = q->queue_lock;
+	spin_lock_irq(oldlock);
+	q->queue_lock = &q->__queue_lock;
+	spin_unlock_irq(oldlock);

 	blk_put_queue(q);
 }
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 21e275d..8ac3e5b 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -304,6 +304,9 @@ static void blk_release_queue(struct kobject *kobj)
 		container_of(kobj, struct request_queue, kobj);
 	struct request_list *rl = &q->rq;

+	if (q->elevator)
+		elevator_exit(q->elevator);
+
 	blk_sync_queue(q);

 	if (rl->rq_pool)
-- 
1.6.5

View attachment "0002-block-Move-elevator-exit-from-blk_cleanup_queue-to-b.patch" of type "text/x-diff" (1874 bytes)