NULL ptr deref at elv_drain_elevator

PATCH In review Jiri Slaby 4 participants 12 messages Nov 3, 2011 – Nov 4, 2011

Subsystems (2)

SCSI Block layer

lore

Share & tools

lore

Jiri Slaby

author

2011-11-03 16:08 UTC #0 lore raw

Hi,

I'm seeing the NULL ptr dereference below on each boot of KVM virtual machine. q->elevator is NULL. This is next-20111025.

I tried to apply Tejun's patch from:
https://lkml.org/lkml/2011/4/30/87
but it doesn't help. Maybe I should revert something?

Scanning for LVM volume groups...
Reading all physical volumes. This may take a while... No volume groups found

BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<ffffffff8125a69c>] elv_drain_elevator+0x1c/0x70
PGD 46176067 PUD 452b5067 PMD 0
Oops: 0000 [#1] SMP
CPU 0
Modules linked in:

Pid: 830, comm: kworker/0:2 Not tainted 3.1.0-next-20111025_64+ #1590 Bochs Bochs

RIP: 0010:[<ffffffff8125a69c>]  [<ffffffff8125a69c>]
elv_drain_elevator+0x1c/0x70
RSP: 0018:ffff8800461abd00  EFLAGS: 00010086
RAX: 0000000000000000 RBX: ffff880046948e00 RCX: 00000001820001ee
RDX: 0000000000000001 RSI: 0000000000000001 RDI: ffff880046948e00
RBP: ffff8800461abd10 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000001
R13: ffff8800469494e0 R14: ffff88004689c450 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff880049600000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000000 CR3: 00000000451f6000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process kworker/0:2 (pid: 830, threadinfo ffff8800461aa000, task
ffff880046f28670)
Stack:
 ffff88004789f888 ffff880046948e00 ffff8800461abd30 ffffffff8125da92
 ffff880046948e00 ffff8800469491f4 ffff8800461abd60 ffffffff8125db90
 ffff8800461abd40 ffff88004689c450 ffff88004689c400 ffff88004789f888
Call Trace:
 [<ffffffff8125da92>] blk_drain_queue+0x42/0x70
 [<ffffffff8125db90>] blk_cleanup_queue+0xd0/0x1c0
 [<ffffffff81469640>] md_free+0x50/0x70
 [<ffffffff8126f43b>] kobject_release+0x8b/0x1d0
 [<ffffffff8126f3b0>] ? kobject_del+0x40/0x40
 [<ffffffff81469380>] ? bb_show+0x20/0x20
 [<ffffffff81270d56>] kref_put+0x36/0xa0
 [<ffffffff8126f2b7>] kobject_put+0x27/0x60
 [<ffffffff814693af>] mddev_delayed_delete+0x2f/0x40
 [<ffffffff81083450>] process_one_work+0x100/0x3b0
 [<ffffffff8108527f>] worker_thread+0x15f/0x3a0
 [<ffffffff81085120>] ? manage_workers.isra.32+0x240/0x240
 [<ffffffff81089937>] kthread+0x87/0x90
 [<ffffffff81621834>] kernel_thread_helper+0x4/0x10
 [<ffffffff810898b0>] ? kthread_worker_fn+0x1a0/0x1a0
 [<ffffffff81621830>] ? gs_change+0xb/0xb
Code: c0 74 02 ff d0 5d c3 0f 1f 84 00 00 00 00 00 55 48 89 e5 53 48 89

fb 48 83 ec 08 0f 1f 40 00 48 8b 43 18 be 01 00 00 00 48 89 df
8b 00 ff 50 28 85 c0 75 ea 8b 93 80 04 00 00 85 d2 74 14 8b
RIP [<ffffffff8125a69c>] elv_drain_elevator+0x1c/0x70
RSP <ffff8800461abd00>
CR2: 0000000000000000

---[ end trace 2b4616ccecf0982b ]---

thanks,

Signature · 2 lines

js
suse labs

Tejun Heo

2011-11-03 16:14 UTC #1 lore raw

Quoted 19 lines · Jiri Slaby

Hi,
I'm seeing the NULL ptr dereference below on each boot of KVM virtual machine. q->elevator is NULL. This is next-20111025.
I tried to apply Tejun's patch from:
https://lkml.org/lkml/2011/4/30/87
but it doesn't help. Maybe I should revert something?
Scanning for LVM volume groups...
Reading all physical volumes. This may take a while... No volume groups found
BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<ffffffff8125a69c>] elv_drain_elevator+0x1c/0x70
PGD 46176067 PUD 452b5067 PMD 0
Oops: 0000 [#1] SMP
CPU 0
Modules linked in:

Quoted 4 lines

Pid: 830, comm: kworker/0:2 Not tainted 3.1.0-next-20111025_64+ #1590 Bochs Bochs
RIP: 0010:[<ffffffff8125a69c>]  [<ffffffff8125a69c>]
elv_drain_elevator+0x1c/0x70

Heh, probably md is tearing down a queue which isn't fully setup. Does the following fix the problem?

Thanks.

block/blk-core.c +2 -1

index f658711..5292e31 100644
@@ -408,7 +408,8 @@ void blk_cleanup_queue(struct request_queue *q)
 	mutex_unlock(&q->sysfs_lock);
 
 	/* drain all requests queued before DEAD marking */
-	blk_drain_queue(q, true);
+	if (q->elevator)
+		blk_drain_queue(q, true);
 
 	/* @q won't process any more request, flush async actions */
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);

Signature · 1 line

tejun

Jiri Slaby

author

2011-11-03 16:16 UTC #2 lore raw

Quoted 20 lines · Tejun Heo

Quoted 19 lines · Jiri Slaby
Hi,
I'm seeing the NULL ptr dereference below on each boot of KVM virtual machine. q->elevator is NULL. This is next-20111025.
I tried to apply Tejun's patch from:
https://lkml.org/lkml/2011/4/30/87
but it doesn't help. Maybe I should revert something?
Scanning for LVM volume groups...
Reading all physical volumes. This may take a while... No volume groups found
BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<ffffffff8125a69c>] elv_drain_elevator+0x1c/0x70
PGD 46176067 PUD 452b5067 PMD 0
Oops: 0000 [#1] SMP
CPU 0
Modules linked in:

Quoted 7 lines

Quoted 4 lines
Pid: 830, comm: kworker/0:2 Not tainted 3.1.0-next-20111025_64+ #1590 Bochs Bochs
RIP: 0010:[<ffffffff8125a69c>]  [<ffffffff8125a69c>]
elv_drain_elevator+0x1c/0x70
Heh, probably md is tearing down a queue which isn't fully setup. Does the following fix the problem?

Yes, it does. Thanks a lot.

Quoted 16 lines

block/blk-core.c +2 -1

index f658711..5292e31 100644
@@ -408,7 +408,8 @@ void blk_cleanup_queue(struct request_queue *q)
 	mutex_unlock(&q->sysfs_lock);
 
 	/* drain all requests queued before DEAD marking */
-	blk_drain_queue(q, true);
+	if (q->elevator)
+		blk_drain_queue(q, true);
 
 	/* @q won't process any more request, flush async actions */
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);

Signature · 2 lines

js
suse labs

Tejun Heo

2011-11-03 16:26 UTC #3 lore raw

PATCH 1/1 block: don't call blk_drain_queue() if elevator is not up

In reply to Tejun Heo

blk_cleanup_queue() may be called before elevator is set up on a queue which triggers the following oops.

 BUG: unable to handle kernel NULL pointer dereference at           (null)
 IP: [<ffffffff8125a69c>] elv_drain_elevator+0x1c/0x70
 ...
 Pid: 830, comm: kworker/0:2 Not tainted 3.1.0-next-20111025_64+ #1590
 Bochs Bochs
 RIP: 0010:[<ffffffff8125a69c>]  [<ffffffff8125a69c>] elv_drain_elevator+0x1c/0x70
 ...
 Call Trace:
  [<ffffffff8125da92>] blk_drain_queue+0x42/0x70
  [<ffffffff8125db90>] blk_cleanup_queue+0xd0/0x1c0
  [<ffffffff81469640>] md_free+0x50/0x70
  [<ffffffff8126f43b>] kobject_release+0x8b/0x1d0
  [<ffffffff81270d56>] kref_put+0x36/0xa0
  [<ffffffff8126f2b7>] kobject_put+0x27/0x60
  [<ffffffff814693af>] mddev_delayed_delete+0x2f/0x40
  [<ffffffff81083450>] process_one_work+0x100/0x3b0
  [<ffffffff8108527f>] worker_thread+0x15f/0x3a0
  [<ffffffff81089937>] kthread+0x87/0x90
  [<ffffffff81621834>] kernel_thread_helper+0x4/0x10

Fix it by making blk_cleanup_queue() check whether q->elevator is set up before invoking blk_drain_queue.

Signed-off-by Tejun Heo <[email protected]>

Reported-and-tested-by Jiri Slaby <[email protected]>

---
 block/blk-core.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

block/blk-core.c +7 -2

index f658711..f43c8a5 100644
@@ -407,8 +407,13 @@ void blk_cleanup_queue(struct request_queue *q)
 	spin_unlock_irq(lock);
 	mutex_unlock(&q->sysfs_lock);
 
-	/* drain all requests queued before DEAD marking */
-	blk_drain_queue(q, true);
+	/*
+	 * Drain all requests queued before DEAD marking.  The caller might
+	 * be trying to tear down @q before its elevator is initialized, in
+	 * which case we don't want to call into draining.
+	 */
+	if (q->elevator)
+		blk_drain_queue(q, true);
 
 	/* @q won't process any more request, flush async actions */
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);

Jens Axboe

maintainer

2011-11-03 17:50 UTC #4 lore raw

Re: [PATCH block/for-next] block: don't call blk_drain_queue() if elevator is not up

Quoted 23 lines · Tejun Heo

blk_cleanup_queue() may be called before elevator is set up on a queue which triggers the following oops.

 BUG: unable to handle kernel NULL pointer dereference at           (null)
 IP: [<ffffffff8125a69c>] elv_drain_elevator+0x1c/0x70
 ...
 Pid: 830, comm: kworker/0:2 Not tainted 3.1.0-next-20111025_64+ #1590
 Bochs Bochs
 RIP: 0010:[<ffffffff8125a69c>]  [<ffffffff8125a69c>] elv_drain_elevator+0x1c/0x70
 ...
 Call Trace:
  [<ffffffff8125da92>] blk_drain_queue+0x42/0x70
  [<ffffffff8125db90>] blk_cleanup_queue+0xd0/0x1c0
  [<ffffffff81469640>] md_free+0x50/0x70
  [<ffffffff8126f43b>] kobject_release+0x8b/0x1d0
  [<ffffffff81270d56>] kref_put+0x36/0xa0
  [<ffffffff8126f2b7>] kobject_put+0x27/0x60
  [<ffffffff814693af>] mddev_delayed_delete+0x2f/0x40
  [<ffffffff81083450>] process_one_work+0x100/0x3b0
  [<ffffffff8108527f>] worker_thread+0x15f/0x3a0
  [<ffffffff81089937>] kthread+0x87/0x90
  [<ffffffff81621834>] kernel_thread_helper+0x4/0x10

Quoted 2 lines

Fix it by making blk_cleanup_queue() check whether q->elevator is set up before invoking blk_drain_queue.

Thanks, glad this got caught before the merge...

Signature · 1 line

Jens Axboe

Tejun Heo

2011-11-03 21:12 UTC #5 lore raw

Re: [PATCH block/for-next] block: don't call blk_drain_queue() if elevator is not up

Quoted 4 lines · Jens Axboe

Quoted 2 lines
Fix it by making blk_cleanup_queue() check whether q->elevator is set up before invoking blk_drain_queue.
Thanks, glad this got caught before the merge...

Heh, yeah, definitely, and just to be paranoid, this whole thing is for the next merge window.

Thanks.

Signature · 1 line

tejun

Jens Axboe

maintainer

2011-11-04 08:12 UTC #6 lore raw

Re: [PATCH block/for-next] block: don't call blk_drain_queue() if elevator is not up

Quoted 8 lines · Tejun Heo

Quoted 4 lines · Jens Axboe
Quoted 2 lines
Fix it by making blk_cleanup_queue() check whether q->elevator is set up before invoking blk_drain_queue.
Thanks, glad this got caught before the merge...
Heh, yeah, definitely, and just to be paranoid, this whole thing is for the next merge window.

Ehm, what parts? If the bug is in for-next, things are queued up for _this_ merge window.

Signature · 1 line

Jens Axboe

Tejun Heo

2011-11-04 14:26 UTC #7 lore raw

Re: [PATCH block/for-next] block: don't call blk_drain_queue() if elevator is not up

Hello,

Quoted 5 lines · Jens Axboe

Quoted 2 lines
Heh, yeah, definitely, and just to be paranoid, this whole thing is for the next merge window.
Ehm, what parts? If the bug is in for-next, things are queued up for _this_ merge window.

So, we had four patchsets - the original drain improvements, updates to drain improvements, cfq locking cleanup, and cfq api cleanup. Currently, the first one is in block tree but the other three are not. I was thinking all four were going mainline in the next merge window.

Hmmm... yeah, the first and second patchsets kinda go together but well the first one definitely is pretty good bug fix without others, so I guess that split isn't too bad either. Alright, no objection.

Thanks.

Signature · 1 line

tejun

Jens Axboe

maintainer

2011-11-04 14:28 UTC #8 lore raw

Re: [PATCH block/for-next] block: don't call blk_drain_queue() if elevator is not up

Quoted 9 lines · Tejun Heo

Hello,
Quoted 5 lines · Jens Axboe
Quoted 2 lines
Heh, yeah, definitely, and just to be paranoid, this whole thing is for the next merge window.
Ehm, what parts? If the bug is in for-next, things are queued up for _this_ merge window.

Quoted 8 lines

So, we had four patchsets - the original drain improvements, updates to drain improvements, cfq locking cleanup, and cfq api cleanup. Currently, the first one is in block tree but the other three are not. I was thinking all four were going mainline in the next merge window.
Hmmm... yeah, the first and second patchsets kinda go together but well the first one definitely is pretty good bug fix without others, so I guess that split isn't too bad either. Alright, no objection.

OK, I'll push off what I have now and then we can queue #2 up shortly. I would probably prefer pushing the cfq locking cleanup to 3.3 to get some more testing time on that, but it all depends on what your level of confidence in it is?

Signature · 1 line

Jens Axboe

Tejun Heo

2011-11-04 14:30 UTC #9 lore raw

Re: [PATCH block/for-next] block: don't call blk_drain_queue() if elevator is not up

Hello,

Quoted 8 lines · Jens Axboe

Quoted 3 lines
Hmmm... yeah, the first and second patchsets kinda go together but well the first one definitely is pretty good bug fix without others, so I guess that split isn't too bad either. Alright, no objection.
OK, I'll push off what I have now and then we can queue #2 up shortly. I would probably prefer pushing the cfq locking cleanup to 3.3 to get some more testing time on that, but it all depends on what your level of confidence in it is?

Not that confident. Those definitely belong to the next window.

Thank you.

Signature · 1 line

tejun

Stefan Richter

2011-11-04 09:40 UTC #10 lore raw

Re: [PATCH block/for-next] block: don't call blk_drain_queue() if elevator is not up

In reply to Tejun Heo

Quoted 2 lines · On Nov 03 Tejun Heo wrote:

blk_cleanup_queue() may be called before elevator is set up on a queue which triggers the following oops.

[...]

Quoted 8 lines

block/blk-core.c +0 -2

@@ -407,8 +407,13 @@ void blk_cleanup_queue(struct request_queue *q)
 	spin_unlock_irq(lock);
 	mutex_unlock(&q->sysfs_lock);
 
-	/* drain all requests queued before DEAD marking */
-	blk_drain_queue(q, true);

block/blk-core.c void blk_cleanup_queue(struct request_queue *q) · line 419

Quoted 10 lines

block/blk-core.c +7 -0

+	/*
+	 * Drain all requests queued before DEAD marking.  The caller might
+	 * be trying to tear down @q before its elevator is initialized, in
+	 * which case we don't want to call into draining.
+	 */
+	if (q->elevator)
+		blk_drain_queue(q, true);
 
 	/* @q won't process any more request, flush async actions */
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);

What prevents elevator_attach to be called and requests to be inserted between 'if (q-elevator)' and 'blk_put_queue(q)'?

Signature · 3 lines

Stefan Richter
-=====-==-== =-== --=--
http://arcgraph.de/sr/

Tejun Heo

2011-11-04 14:38 UTC #11 lore raw

Re: [PATCH block/for-next] block: don't call blk_drain_queue() if elevator is not up

Hello,

Quoted 5 lines · Stefan Richter

Quoted 2 lines
/* @q won't process any more request, flush async actions */ del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);
What prevents elevator_attach to be called and requests to be inserted between 'if (q-elevator)' and 'blk_put_queue(q)'?

The fact that the queue owner has called blk_cleaup_queue()?

Thanks.

Signature · 1 line

tejun