Bisected, with rfc/patch - was Re: BUG: unable to handle kernel NULL pointer dereference at sysfs_do_create_link_sd (after mdadm)

Commit Message

NeilBrown April 24, 2015, 2:09 a.m. UTC

On Thu, 23 Apr 2015 18:10:51 +0200 Christoph Hellwig <[e-mail]> wrote:

> On Thu, Apr 23, 2015 at 06:03:14PM +1000, NeilBrown wrote:
> > On Thu, 23 Apr 2015 09:37:24 +0200 Christoph Hellwig <[e-mail]> wrote:
> > 
> > > Plase fix your device name lifetimes.
> > 
> > Any chance you could be more explicit?
> >
> > The commit you identified doesn't seem to help much - md and dm are quite
> > different in this area.
> > 
> > It seems that it is no longer safe to call 'add_disk' between calling
> > 'del_gendisk' and bdi_destroy being called.  How can I find out if I am in
> > that window, or wait for bdi_destroy to be called?
> 
> The bdi is only around if the device is open, either through a device
> node, or through a blkdev_get from a file system.  If you get duplicate
> names that means you're trying to allocate a new gendisk while the old
> one is still around.
> 
> In theory you're fine once the device gets ->release called.

In practice .... the put_disk() shortly after the ->release call in
__blkdev_put() is what ultimately releases the name of the bdi - at least in
the cases where I get a crash.

> 
> Except that we can hold sysfs reference to the qeue, eww.  So for now
> try to follow the dm model, but I'll need to add a callback to the
> queue called once the request_queue actually is released for this.

I'm pretty sure that the md code is already as close to the "dm model" as it
meaningfully can be.

If I move bdi_destroy out of blk_release_queue (which really think is too
later) and place it in blk_cleanup_queue (which seems a credible place for
it), and then move the blk_cleanup_queue call in md_free up before the
del_gendisk() call (which is probably the right thing to do anyway, though dm
has the same order that md currently has) then I don't get any crashes and
I'm almost convince it is correct...

Thoughts?

Thanks,
NeilBrown
--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Christoph Hellwig April 24, 2015, 8:27 a.m. UTC | #1

On Fri, Apr 24, 2015 at 12:09:32PM +1000, NeilBrown wrote:
> I'm pretty sure that the md code is already as close to the "dm model" as it
> meaningfully can be.
> 
> If I move bdi_destroy out of blk_release_queue (which really think is too
> later) and place it in blk_cleanup_queue (which seems a credible place for
> it), and then move the blk_cleanup_queue call in md_free up before the
> del_gendisk() call (which is probably the right thing to do anyway, though dm
> has the same order that md currently has) then I don't get any crashes and
> I'm almost convince it is correct...

This looks reasonable to me, thanks.

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Patch

diff --git a/block/blk-core.c b/block/blk-core.c
index 794c3e7f01cf..66406474f0c4 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -552,6 +552,8 @@  void blk_cleanup_queue(struct request_queue *q)
 		q->queue_lock = &q->__queue_lock;
 	spin_unlock_irq(lock);
 
+	bdi_destroy(&q->backing_dev_info);
+
 	/* @q is and will stay empty, shutdown and put */
 	blk_put_queue(q);
 }
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index faaf36ade7eb..2b8fd302f677 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -522,8 +522,6 @@  static void blk_release_queue(struct kobject *kobj)
 
 	blk_trace_shutdown(q);
 
-	bdi_destroy(&q->backing_dev_info);
-
 	ida_simple_remove(&blk_queue_ida, q->id);
 	call_rcu(&q->rcu_head, blk_free_queue_rcu);
 }
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d4f31e195e26..593a02476c78 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4818,12 +4818,12 @@  static void md_free(struct kobject *ko)
 	if (mddev->sysfs_state)
 		sysfs_put(mddev->sysfs_state);
 
+	if (mddev->queue)
+		blk_cleanup_queue(mddev->queue);
 	if (mddev->gendisk) {
 		del_gendisk(mddev->gendisk);
 		put_disk(mddev->gendisk);
 	}
-	if (mddev->queue)
-		blk_cleanup_queue(mddev->queue);
 
 	kfree(mddev);
 }