revert: loop: queue_lock NULL pointer derefence in blk_throtl_exit V1

revert: loop: queue_lock NULL pointer derefence in blk_throtl_exit V1

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

Hello Jens, Andrew

We have accidentally commit "loop: queue_lock NULL pointer 
derefence in blk_throtl_exit V1" applied: ee71a968672a9951aee6014c55511007596425bc

It's not a huge problem, but I think, it's better to have V3 applied 
https://lkml.org/lkml/2011/1/21/158

since V3 targets all the throttle users, not only loop (by moving 
`fall-back' from loop_free to blk_release_queue).

V3 is in Andrew's mmotm as for now 
loop-queue_lock-null-pointer-derefence-in-blk_throtl_exit-v3.patch

Could we please revert ee71a968672a9951aee6014c55511007596425bc when 
loop-queue_lock-null-pointer-derefence-in-blk_throtl_exit-v3 will land 
in the kernel?

Sorry.

Thanks in advance,

        Sergey

[-- Attachment #2: Type: application/pgp-signature, Size: 316 bytes --]

Re: revert: loop: queue_lock NULL pointer derefence in blk_throtl_exit V1

[Jens Axboe]

On 2011-02-11 13:59, Sergey Senozhatsky wrote:
> Hello Jens, Andrew
> 
> We have accidentally commit "loop: queue_lock NULL pointer 
> derefence in blk_throtl_exit V1" applied: ee71a968672a9951aee6014c55511007596425bc
> 
> It's not a huge problem, but I think, it's better to have V3 applied 
> https://lkml.org/lkml/2011/1/21/158
> 
> since V3 targets all the throttle users, not only loop (by moving 
> `fall-back' from loop_free to blk_release_queue).
> 
> V3 is in Andrew's mmotm as for now 
> loop-queue_lock-null-pointer-derefence-in-blk_throtl_exit-v3.patch
> 
> Could we please revert ee71a968672a9951aee6014c55511007596425bc when 
> loop-queue_lock-null-pointer-derefence-in-blk_throtl_exit-v3 will land 
> in the kernel?

Send an incremental patch if there's overlap, otherwise we'll do the
revert. I'm on my way out the door and I'll be away from the computer
all of next week, so perhaps Andrew can assist you in upstreaming such a
patch?

-- 
Jens Axboe

[PATCH] block: fix queue_lock NULL pointer derefence in blk_throtl_exit (v4)

[Sergey Senozhatsky]

block: fix queue_lock NULL pointer derefence in blk_throtl_exit

BUG: unable to handle kernel NULL pointer dereference at 0000000000000004
 IP: [<ffffffff812479d4>] do_raw_spin_lock+0x14/0x122
 Process modprobe (pid: 6189, threadinfo ffff88009a898000, task
 ffff880154a88000)                                                        
 Call Trace:                                         
  [<ffffffff81486788>] _raw_spin_lock_irq+0x4a/0x51                         
  [<ffffffff8123404b>] ? blk_throtl_exit+0x3b/0xa0
  [<ffffffff8105b120>] ? cancel_delayed_work_sync+0xd/0xf
  [<ffffffff8123404b>] blk_throtl_exit+0x3b/0xa0   
  [<ffffffff81229bc8>] blk_release_queue+0x21/0x65
  [<ffffffff8123bb06>] kobject_release+0x51/0x66         
  [<ffffffff8123bab5>] ? kobject_release+0x0/0x66
  [<ffffffff8123ce1e>] kref_put+0x43/0x4d         
  [<ffffffff8123ba27>] kobject_put+0x47/0x4b    
  [<ffffffff8122717c>] blk_cleanup_queue+0x56/0x5b
  [<ffffffffa01c3824>] loop_exit+0x68/0x844 [loop]
  [<ffffffff8107cccc>] sys_delete_module+0x1e8/0x25b
  [<ffffffff814864c9>] ? trace_hardirqs_on_thunk+0x3a/0x3f
  [<ffffffff81002112>] system_call_fastpath+0x16/0x1b

because of an attempt to acquire NULL queue_lock.   
Added the same lines as in blk_queue_make_request: 
'fall back to embedded per-queue lock' - when call blk_release_queue on
allocated but never initialized queue.

v4:
Incremental patch to move fall back to embedded per-queue lock from loop_free
to blk_release_queue, because v1 patch was accidentally applied.

v3: According to comment by Vivek Goyal, queue_lock NULL check and fix moved
out from loop driver code to blk_release_queue.


Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

---

 block/blk-sysfs.c    |    6 ++++++
 drivers/block/loop.c |    3 ---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 41fb691..2b426ac 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -471,6 +471,12 @@ static void blk_release_queue(struct kobject *kobj)
 
 	blk_sync_queue(q);
 
+	/* It's possible that blk_release_queue will be called on allocated
+	 * but never initialized queue. Fall back to our embedded per-queue 
+	 * locks in this case. */
+	if (!q->queue_lock)
+		q->queue_lock = &q->__queue_lock;
+
 	blk_throtl_exit(q);
 
 	if (rl->rq_pool)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 49e6a54..44e18c0 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1641,9 +1641,6 @@ out:
 
 static void loop_free(struct loop_device *lo)
 {
-	if (!lo->lo_queue->queue_lock)
-		lo->lo_queue->queue_lock = &lo->lo_queue->__queue_lock;
-
 	blk_cleanup_queue(lo->lo_queue);
 	put_disk(lo->lo_disk);
 	list_del(&lo->lo_list);