diff mbox

[1/3] nbd: del_gendisk after we cleanup the queue

Message ID 20180413160334.5496-1-josef@toxicpanda.com (mailing list archive)
State New, archived
Headers show

Commit Message

Josef Bacik April 13, 2018, 4:03 p.m. UTC 
From: Josef Bacik <jbacik@fb.com>

This fixes a use after free bug, we need to do the del_gendisk after we
cleanup the queue on the device.

Fixes: c6a4759ea0c9 ("nbd: add device refcounting")
cc: stable@vger.kernel.org
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 drivers/block/nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bart Van Assche April 13, 2018, 4:16 p.m. UTC | #1 
On Fri, 2018-04-13 at 12:03 -0400, Josef Bacik wrote:
> This fixes a use after free bug, we need to do the del_gendisk after we

> cleanup the queue on the device.


Hello Josef,

Which use-after-free bug does this patch fix? Are you aware that most block
drivers call blk_cleanup_queue() before calling del_gendisk()? Why do you
think that the nbd driver should use the opposite order?

Thanks,

Bart.
Josef Bacik April 13, 2018, 4:21 p.m. UTC | #2 
On Fri, Apr 13, 2018 at 04:16:18PM +0000, Bart Van Assche wrote:
> On Fri, 2018-04-13 at 12:03 -0400, Josef Bacik wrote:
> > This fixes a use after free bug, we need to do the del_gendisk after we
> > cleanup the queue on the device.
> 
> Hello Josef,
> 
> Which use-after-free bug does this patch fix? Are you aware that most block
> drivers call blk_cleanup_queue() before calling del_gendisk()? Why do you
> think that the nbd driver should use the opposite order?
> 

I'm confused, that's what this patch does.  Before I had del_gendisk() first and
then the blk_cleanup_queue(), which was bugging out when I was testing stuff
with a null pointer deref whenever I rmmod'ed the nbd.  Swapping it to the way
everybody else did it fixed the problem.  Thanks,

Josef
Bart Van Assche April 13, 2018, 4:25 p.m. UTC | #3 
On Fri, 2018-04-13 at 12:21 -0400, Josef Bacik wrote:
> On Fri, Apr 13, 2018 at 04:16:18PM +0000, Bart Van Assche wrote:

> > On Fri, 2018-04-13 at 12:03 -0400, Josef Bacik wrote:

> > > This fixes a use after free bug, we need to do the del_gendisk after we

> > > cleanup the queue on the device.

> > 

> > Hello Josef,

> > 

> > Which use-after-free bug does this patch fix? Are you aware that most block

> > drivers call blk_cleanup_queue() before calling del_gendisk()? Why do you

> > think that the nbd driver should use the opposite order?

> 

> I'm confused, that's what this patch does.  Before I had del_gendisk() first and

> then the blk_cleanup_queue(), which was bugging out when I was testing stuff

> with a null pointer deref whenever I rmmod'ed the nbd.  Swapping it to the way

> everybody else did it fixed the problem.  Thanks,


Hello Josef,

Oops, I swapped "blk_cleanup_queue()" and "del_gendisk()" in my e-mail.

Can you share the call stack of the NULL pointer deref and also the translation
of the crash address into a source code line?

Thanks,

Bart.
diff mbox

Patch

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 86258b00a1d4..e33da3e6aa20 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -174,8 +174,8 @@  static void nbd_dev_remove(struct nbd_device *nbd)
 {
 	struct gendisk *disk = nbd->disk;
 	if (disk) {
-		del_gendisk(disk);
 		blk_cleanup_queue(disk->queue);
+		del_gendisk(disk);
 		blk_mq_free_tag_set(&nbd->tag_set);
 		disk->private_data = NULL;
 		put_disk(disk);