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

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

[Josef Bacik]

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(-)

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);
-- 
2.14.3

[PATCH 2/3] nbd: update size when connected

[Josef Bacik]

From: Josef Bacik <jbacik@fb.com>

I messed up changing the size of an NBD device while it was connected by
not actually updating the device or doing the uevent.  Fix this by
updating everything if we're connected and we change the size.

cc: stable@vger.kernel.org
Fixes: 639812a ("nbd: don't set the device size until we're connected")
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 drivers/block/nbd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index e33da3e6aa20..1520383b12f6 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -243,6 +243,8 @@ static void nbd_size_set(struct nbd_device *nbd, loff_t blocksize,
 	struct nbd_config *config = nbd->config;
 	config->blksize = blocksize;
 	config->bytesize = blocksize * nr_blocks;
+	if (nbd->task_recv != NULL)
+		nbd_size_update(nbd);
 }
 
 static void nbd_complete_rq(struct request *req)
-- 
2.14.3

[PATCH 3/3] nbd: use bd_set_size when updating disk size

[Josef Bacik]

From: Josef Bacik <jbacik@fb.com>

When we stopped relying on the bdev everywhere I broke updating the
block device size on the fly, which ceph relies on.  We can't just do
set_capacity, we also have to do bd_set_size so things like parted will
notice the device size change.

Fixes: 29eaadc ("nbd: stop using the bdev everywhere")
cc: stable@vger.kernel.org
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 drivers/block/nbd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 1520383b12f6..61dd95aa47fa 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -231,9 +231,15 @@ static void nbd_size_clear(struct nbd_device *nbd)
 static void nbd_size_update(struct nbd_device *nbd)
 {
 	struct nbd_config *config = nbd->config;
+	struct block_device *bdev = bdget_disk(nbd->disk, 0);
+
 	blk_queue_logical_block_size(nbd->disk->queue, config->blksize);
 	blk_queue_physical_block_size(nbd->disk->queue, config->blksize);
 	set_capacity(nbd->disk, config->bytesize >> 9);
+	if (bdev) {
+		bd_set_size(bdev, config->bytesize);
+		bdput(bdev);
+	}
 	kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
 }
 
@@ -1111,7 +1117,6 @@ static int nbd_start_device_ioctl(struct nbd_device *nbd, struct block_device *b
 	if (ret)
 		return ret;
 
-	bd_set_size(bdev, config->bytesize);
 	if (max_part)
 		bdev->bd_invalidated = 1;
 	mutex_unlock(&nbd->config_lock);
-- 
2.14.3

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

[Bart Van Assche]

T24gRnJpLCAyMDE4LTA0LTEzIGF0IDEyOjAzIC0wNDAwLCBKb3NlZiBCYWNpayB3cm90ZToNCj4g
VGhpcyBmaXhlcyBhIHVzZSBhZnRlciBmcmVlIGJ1Zywgd2UgbmVlZCB0byBkbyB0aGUgZGVsX2dl
bmRpc2sgYWZ0ZXIgd2UNCj4gY2xlYW51cCB0aGUgcXVldWUgb24gdGhlIGRldmljZS4NCg0KSGVs
bG8gSm9zZWYsDQoNCldoaWNoIHVzZS1hZnRlci1mcmVlIGJ1ZyBkb2VzIHRoaXMgcGF0Y2ggZml4
PyBBcmUgeW91IGF3YXJlIHRoYXQgbW9zdCBibG9jaw0KZHJpdmVycyBjYWxsIGJsa19jbGVhbnVw
X3F1ZXVlKCkgYmVmb3JlIGNhbGxpbmcgZGVsX2dlbmRpc2soKT8gV2h5IGRvIHlvdQ0KdGhpbmsg
dGhhdCB0aGUgbmJkIGRyaXZlciBzaG91bGQgdXNlIHRoZSBvcHBvc2l0ZSBvcmRlcj8NCg0KVGhh
bmtzLA0KDQpCYXJ0Lg0KDQo=

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

[Bart Van Assche]

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.

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

[Josef Bacik]

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

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

[Bart Van Assche]

T24gRnJpLCAyMDE4LTA0LTEzIGF0IDEyOjIxIC0wNDAwLCBKb3NlZiBCYWNpayB3cm90ZToNCj4g
T24gRnJpLCBBcHIgMTMsIDIwMTggYXQgMDQ6MTY6MThQTSArMDAwMCwgQmFydCBWYW4gQXNzY2hl
IHdyb3RlOg0KPiA+IE9uIEZyaSwgMjAxOC0wNC0xMyBhdCAxMjowMyAtMDQwMCwgSm9zZWYgQmFj
aWsgd3JvdGU6DQo+ID4gPiBUaGlzIGZpeGVzIGEgdXNlIGFmdGVyIGZyZWUgYnVnLCB3ZSBuZWVk
IHRvIGRvIHRoZSBkZWxfZ2VuZGlzayBhZnRlciB3ZQ0KPiA+ID4gY2xlYW51cCB0aGUgcXVldWUg
b24gdGhlIGRldmljZS4NCj4gPiANCj4gPiBIZWxsbyBKb3NlZiwNCj4gPiANCj4gPiBXaGljaCB1
c2UtYWZ0ZXItZnJlZSBidWcgZG9lcyB0aGlzIHBhdGNoIGZpeD8gQXJlIHlvdSBhd2FyZSB0aGF0
IG1vc3QgYmxvY2sNCj4gPiBkcml2ZXJzIGNhbGwgYmxrX2NsZWFudXBfcXVldWUoKSBiZWZvcmUg
Y2FsbGluZyBkZWxfZ2VuZGlzaygpPyBXaHkgZG8geW91DQo+ID4gdGhpbmsgdGhhdCB0aGUgbmJk
IGRyaXZlciBzaG91bGQgdXNlIHRoZSBvcHBvc2l0ZSBvcmRlcj8NCj4gDQo+IEknbSBjb25mdXNl
ZCwgdGhhdCdzIHdoYXQgdGhpcyBwYXRjaCBkb2VzLiAgQmVmb3JlIEkgaGFkIGRlbF9nZW5kaXNr
KCkgZmlyc3QgYW5kDQo+IHRoZW4gdGhlIGJsa19jbGVhbnVwX3F1ZXVlKCksIHdoaWNoIHdhcyBi
dWdnaW5nIG91dCB3aGVuIEkgd2FzIHRlc3Rpbmcgc3R1ZmYNCj4gd2l0aCBhIG51bGwgcG9pbnRl
ciBkZXJlZiB3aGVuZXZlciBJIHJtbW9kJ2VkIHRoZSBuYmQuICBTd2FwcGluZyBpdCB0byB0aGUg
d2F5DQo+IGV2ZXJ5Ym9keSBlbHNlIGRpZCBpdCBmaXhlZCB0aGUgcHJvYmxlbS4gIFRoYW5rcywN
Cg0KSGVsbG8gSm9zZWYsDQoNCk9vcHMsIEkgc3dhcHBlZCAiYmxrX2NsZWFudXBfcXVldWUoKSIg
YW5kICJkZWxfZ2VuZGlzaygpIiBpbiBteSBlLW1haWwuDQoNCkNhbiB5b3Ugc2hhcmUgdGhlIGNh
bGwgc3RhY2sgb2YgdGhlIE5VTEwgcG9pbnRlciBkZXJlZiBhbmQgYWxzbyB0aGUgdHJhbnNsYXRp
b24NCm9mIHRoZSBjcmFzaCBhZGRyZXNzIGludG8gYSBzb3VyY2UgY29kZSBsaW5lPw0KDQpUaGFu
a3MsDQoNCkJhcnQuDQoNCg0K

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

[Bart Van Assche]

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.

Re: [PATCH 3/3] nbd: use bd_set_size when updating disk size

[Alex Gorbachev]

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

On Fri, Apr 13, 2018 at 12:03 PM, Josef Bacik <josef@toxicpanda.com> wrote:

> From: Josef Bacik <jbacik@fb.com>
>
> When we stopped relying on the bdev everywhere I broke updating the
> block device size on the fly, which ceph relies on.  We can't just do
> set_capacity, we also have to do bd_set_size so things like parted will
> notice the device size change.
>

The three patches Josef submitted today fix the problem with the online
rbd-nbd expansion completely.


Fixes: 29eaadc ("nbd: stop using the bdev everywhere")

> cc: stable@vger.kernel.org
Signed-off-by: Josef Bacik <jbacik@fb.com>
Tested-by: Alex Gorbachev <ag@iss-integration.com>

> ---

>  drivers/block/nbd.c | 7 ++++++-

>  1 file changed, 6 insertions(+), 1 deletion(-)

>
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c

> index 1520383b12f6..61dd95aa47fa 100644

> --- a/drivers/block/nbd.c

> +++ b/drivers/block/nbd.c

> @@ -231,9 +231,15 @@ static void nbd_size_clear(struct nbd_device *nbd)

>  static void nbd_size_update(struct nbd_device *nbd)

>  {

>         struct nbd_config *config = nbd->config;

> +       struct block_device *bdev = bdget_disk(nbd->disk, 0);

> +

>         blk_queue_logical_block_size(nbd->disk->queue, config->blksize);

>         blk_queue_physical_block_size(nbd->disk->queue, config->blksize);

>         set_capacity(nbd->disk, config->bytesize >> 9);

> +       if (bdev) {

> +               bd_set_size(bdev, config->bytesize);

> +               bdput(bdev);

> +       }

>         kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);

>  }

>
@@ -1111,7 +1117,6 @@ static int nbd_start_device_ioctl(struct nbd_device
*nbd, struct block_device *b

>         if (ret)

>                 return ret;

>
-       bd_set_size(bdev, config->bytesize);

>         if (max_part)

>                 bdev->bd_invalidated = 1;

>         mutex_unlock(&nbd->config_lock);

> --

> 2.14.3

>
>

[-- Attachment #2: Type: text/html, Size: 9183 bytes --]

Re: [PATCH 2/3] nbd: update size when connected

[Alex Gorbachev]

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

On Fri, Apr 13, 2018 at 12:03 PM, Josef Bacik <josef@toxicpanda.com> wrote:

> From: Josef Bacik <jbacik@fb.com>
>
> I messed up changing the size of an NBD device while it was connected by
> not actually updating the device or doing the uevent.  Fix this by
> updating everything if we're connected and we change the size.
>

The three patches Josef submitted today fix the problem with the online
rbd-nbd expansion completely.


cc: stable@vger.kernel.org

> Fixes: 639812a ("nbd: don't set the device size until we're connected")

> Signed-off-by: Josef Bacik <jbacik@fb.com>
Tested-by: Alex Gorbachev <ag@iss-integration.com>

> ---

>  drivers/block/nbd.c | 2 ++

>  1 file changed, 2 insertions(+)

>
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c

> index e33da3e6aa20..1520383b12f6 100644

> --- a/drivers/block/nbd.c

> +++ b/drivers/block/nbd.c

> @@ -243,6 +243,8 @@ static void nbd_size_set(struct nbd_device *nbd,
loff_t blocksize,

>         struct nbd_config *config = nbd->config;

>         config->blksize = blocksize;

>         config->bytesize = blocksize * nr_blocks;

> +       if (nbd->task_recv != NULL)

> +               nbd_size_update(nbd);

>  }

>
 static void nbd_complete_rq(struct request *req)

> --

> 2.14.3

>
>

[-- Attachment #2: Type: text/html, Size: 6922 bytes --]

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

[Alex Gorbachev]

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

On Fri, Apr 13, 2018 at 12:03 PM, Josef Bacik <josef@toxicpanda.com> wrote:

> 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.
>

The three patches Josef submitted today fix the problem with the online
rbd-nbd expansion completely.

Fixes: c6a4759ea0c9 ("nbd: add device refcounting")

> cc: stable@vger.kernel.org

> Signed-off-by: Josef Bacik <jbacik@fb.com>
Tested-by: Alex Gorbachev <ag@iss-integration.com>

> ---

>  drivers/block/nbd.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>
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);

> --

> 2.14.3

>
>

[-- Attachment #2: Type: text/html, Size: 7965 bytes --]