Skip to contentSkip to conversation

nbd: fix crash when unmaping nbd device with fs still mounted

RFC PATCH In review Ming Lin 2 participants 6 messages Mar 20, 2017 – Mar 23, 2017

Subsystems (2)
lore
Share & tools
lore

Ming Lin

author
#0 lore raw

cover nbd: fix crash when unmaping nbd device with fs still mounted

Hi all,

I run into a BUG_ON(!buffer_mapped(bh)) crash with below script.

 $ rbd-nbd map mypool/myimg
 $ mkfs.ext4 /dev/nbd0
 $ mount /dev/nbd0 /mnt/
 $ rbd-nbd unmap /dev/nbd0
 $ umount /mnt
[ 1248.870131] kernel BUG at /home/mlin/linux/fs/buffer.c:3103!
[ 1248.871214] invalid opcode: 0000 [#1] SMP
[ 1248.879468] CPU: 0 PID: 2450 Comm: umount Tainted: G            E   4.11.0-rc2+ #2
[ 1248.896579] Call Trace:
[ 1248.897056]  __sync_dirty_buffer+0x6e/0xe0
[ 1248.897870]  ext4_commit_super+0x1eb/0x290 [ext4]
[ 1248.898795]  ext4_put_super+0x2fa/0x3c0 [ext4]
[ 1248.899662]  generic_shutdown_super+0x6f/0x100
[ 1248.900525]  kill_block_super+0x27/0x70
[ 1248.901257]  deactivate_locked_super+0x43/0x70
[ 1248.902112]  deactivate_super+0x46/0x60
[ 1248.902869]  cleanup_mnt+0x3f/0x80
[ 1248.903526]  __cleanup_mnt+0x12/0x20
[ 1248.904218]  task_work_run+0x83/0xb0
[ 1248.904941]  exit_to_usermode_loop+0x59/0x7b
[ 1248.905769]  do_syscall_64+0x165/0x180
[ 1248.907603]  entry_SYSCALL64_slow_path+0x25/0x25

Last year, Ratna posted a patch to fix it.
https://lkml.org/lkml/2016/4/20/257

Ratna's script to reproduce the bug.

 $ qemu-img create -f qcow2 f.img 1G
 $ mkfs.ext4 f.img
 $ qemu-nbd -c /dev/nbd0 f.img
 $ mount /dev/nbd0 dir
 $ killall -KILL qemu-nbd
 $ sleep 1
 $ ls dir
 $ umount dir

I ported Rantna's patch to 4.11-rc2 and confirmed that it fixes the crash.

Jan Kara had some comments about this bug:
http://www.kernelhub.org/?p=2&msg=361407

I hope to fix this bug in the upstream kernel first and then back port it to our production system.

Please see "PATCH 1/1" for detail.

Thanks,
Ming

author
#1 lore raw

RFC PATCH 1/1 nbd: replace kill_bdev() with __invalidate_device()

From: Ratna Manoj Bolla <[email protected]>

When a filesystem is mounted on a nbd device and on a disconnect, because of kill_bdev(), and resetting bdev size to zero, buffer_head mappings are getting destroyed under mounted filesystem.

After a bdev size reset(i.e bdev->bd_inode->i_size = 0) on a disconnect, followed by a sys_umount(),

        generic_shutdown_super()->...
        ->__sync_blockdev()->...
        -blkdev_writepages()->...
        ->do_invalidatepage()->...
        -discard_buffer()   is discarding superblock buffer_head assumed

to be in mapped state by ext4_commit_super().

[mlin: ported to 4.11-rc2]
---
 drivers/block/nbd.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/block/nbd.c +6 -2
index cb4ccfc..a6a3643 100644
@@ -125,7 +125,8 @@ static const char *nbdcmd_to_ascii(int cmd)
 
 static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
 {
-	bd_set_size(bdev, 0);
+	if (bdev->bd_openers <= 1)
+		bd_set_size(bdev, 0);
 	set_capacity(nbd->disk, 0);
 	kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
 
@@ -603,6 +604,8 @@ static void nbd_reset(struct nbd_device *nbd)
 
 static void nbd_bdev_reset(struct block_device *bdev)
 {
+	if (bdev->bd_openers > 1)
+		return;
 	set_device_ro(bdev, false);
 	bdev->bd_inode->i_size = 0;
 	if (max_part > 0) {
@@ -666,7 +669,8 @@ static int nbd_clear_sock(struct nbd_device *nbd, struct block_device *bdev)
 {
 	sock_shutdown(nbd);
 	nbd_clear_que(nbd);
-	kill_bdev(bdev);
+
+	__invalidate_device(bdev, true);
 	nbd_bdev_reset(bdev);
 	/*
 	 * We want to give the run thread a chance to wait for everybody
Signature · 1 line
1.8.3.1
author
#2 lore raw

Re: [Nbd] [RFC PATCH 1/1] nbd: replace kill_bdev() with __invalidate_device()

Quoted 55 lines · Ming Lin

From: Ratna Manoj Bolla <[email protected]>

When a filesystem is mounted on a nbd device and on a disconnect, because of kill_bdev(), and resetting bdev size to zero, buffer_head mappings are getting destroyed under mounted filesystem.

After a bdev size reset(i.e bdev->bd_inode->i_size = 0) on a disconnect, followed by a sys_umount(),

        generic_shutdown_super()->...
        ->__sync_blockdev()->...
        -blkdev_writepages()->...
        ->do_invalidatepage()->...
        -discard_buffer()   is discarding superblock buffer_head assumed

to be in mapped state by ext4_commit_super().

[mlin: ported to 4.11-rc2]
---
 drivers/block/nbd.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/block/nbd.c +6 -3
index cb4ccfc..a6a3643 100644
@@ -125,7 +125,8 @@ static const char *nbdcmd_to_ascii(int cmd)
 
 static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
 {
-       bd_set_size(bdev, 0);
+       if (bdev->bd_openers <= 1)
+               bd_set_size(bdev, 0);
        set_capacity(nbd->disk, 0);
        kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
 
@@ -603,6 +604,8 @@ static void nbd_reset(struct nbd_device *nbd)
 
 static void nbd_bdev_reset(struct block_device *bdev)
 {
+       if (bdev->bd_openers > 1)
+               return;
        set_device_ro(bdev, false);
        bdev->bd_inode->i_size = 0;
        if (max_part > 0) {
@@ -666,7 +669,8 @@ static int nbd_clear_sock(struct nbd_device *nbd, struct block_device *bdev)
 {
        sock_shutdown(nbd);
        nbd_clear_que(nbd);
-       kill_bdev(bdev);
+
+       __invalidate_device(bdev, true);
        nbd_bdev_reset(bdev);
        /*
         * We want to give the run thread a chance to wait for everybody
--
Quoted 1 line

1.8.3.1

Hi Josef,

Any comments?

Thanks,
Ming

#3 lore raw

Re: [Nbd] [RFC PATCH 1/1] nbd: replace kill_bdev() with __invalidate_device()

SGV5IHNvcnJ5IEkganVzdCBnb3QgYmFjayBmcm9tIExTRiwgSeKAmWxsIGxvb2sgYXQgdGhpcyBp biB0aGUgbW9ybmluZy4gIFRoYW5rcywNCg0KSm9zZWYNCg0KT24gMy8yMi8xNywgNDo0OCBQTSwg Ik1pbmcgTGluIiA8bWxpbkBrZXJuZWwub3JnPiB3cm90ZToNCg0KT24gTW9uLCBNYXIgMjAsIDIw MTcgYXQgMzo1OCBQTSwgTWluZyBMaW4gPG1saW5Aa2VybmVsLm9yZz4gd3JvdGU6DQo+IEZyb206 IFJhdG5hIE1hbm9qIEJvbGxhIDxtYW5vai5ickBnbWFpbC5jb20+DQo+DQo+IFdoZW4gYSBmaWxl c3lzdGVtIGlzIG1vdW50ZWQgb24gYSBuYmQgZGV2aWNlIGFuZCBvbiBhIGRpc2Nvbm5lY3QsIGJl Y2F1c2UNCj4gb2Yga2lsbF9iZGV2KCksIGFuZCByZXNldHRpbmcgYmRldiBzaXplIHRvIHplcm8s IGJ1ZmZlcl9oZWFkIG1hcHBpbmdzIGFyZQ0KPiBnZXR0aW5nIGRlc3Ryb3llZCB1bmRlciBtb3Vu dGVkIGZpbGVzeXN0ZW0uDQo+DQo+IEFmdGVyIGEgYmRldiBzaXplIHJlc2V0KGkuZSBiZGV2LT5i ZF9pbm9kZS0+aV9zaXplID0gMCkgb24gYSBkaXNjb25uZWN0LA0KPiBmb2xsb3dlZCBieSBhIHN5 c191bW91bnQoKSwNCj4gICAgICAgICBnZW5lcmljX3NodXRkb3duX3N1cGVyKCktPi4uLg0KPiAg ICAgICAgIC0+X19zeW5jX2Jsb2NrZGV2KCktPi4uLg0KPiAgICAgICAgIC1ibGtkZXZfd3JpdGVw YWdlcygpLT4uLi4NCj4gICAgICAgICAtPmRvX2ludmFsaWRhdGVwYWdlKCktPi4uLg0KPiAgICAg ICAgIC1kaXNjYXJkX2J1ZmZlcigpICAgaXMgZGlzY2FyZGluZyBzdXBlcmJsb2NrIGJ1ZmZlcl9o ZWFkIGFzc3VtZWQNCj4gdG8gYmUgaW4gbWFwcGVkIHN0YXRlIGJ5IGV4dDRfY29tbWl0X3N1cGVy KCkuDQo+DQo+IFttbGluOiBwb3J0ZWQgdG8gNC4xMS1yYzJdDQo+IFNpZ25lZC1vZmYtYnk6IFJh dG5hIE1hbm9qIEJvbGxhIDxtYW5vai5ickBnbWFpbC5jb20NCj4gLS0tDQo+ICBkcml2ZXJzL2Js b2NrL25iZC5jIHwgOCArKysrKystLQ0KPiAgMSBmaWxlIGNoYW5nZWQsIDYgaW5zZXJ0aW9ucygr KSwgMiBkZWxldGlvbnMoLSkNCj4NCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvYmxvY2svbmJkLmMg Yi9kcml2ZXJzL2Jsb2NrL25iZC5jDQo+IGluZGV4IGNiNGNjZmMuLmE2YTM2NDMgMTAwNjQ0DQo+ IC0tLSBhL2RyaXZlcnMvYmxvY2svbmJkLmMNCj4gKysrIGIvZHJpdmVycy9ibG9jay9uYmQuYw0K PiBAQCAtMTI1LDcgKzEyNSw4IEBAIHN0YXRpYyBjb25zdCBjaGFyICpuYmRjbWRfdG9fYXNjaWko aW50IGNtZCkNCj4NCj4gIHN0YXRpYyBpbnQgbmJkX3NpemVfY2xlYXIoc3RydWN0IG5iZF9kZXZp Y2UgKm5iZCwgc3RydWN0IGJsb2NrX2RldmljZSAqYmRldikNCj4gIHsNCj4gLSAgICAgICBiZF9z ZXRfc2l6ZShiZGV2LCAwKTsNCj4gKyAgICAgICBpZiAoYmRldi0+YmRfb3BlbmVycyA8PSAxKQ0K PiArICAgICAgICAgICAgICAgYmRfc2V0X3NpemUoYmRldiwgMCk7DQo+ICAgICAgICAgc2V0X2Nh cGFjaXR5KG5iZC0+ZGlzaywgMCk7DQo+ICAgICAgICAga29iamVjdF91ZXZlbnQoJm5iZF90b19k ZXYobmJkKS0+a29iaiwgS09CSl9DSEFOR0UpOw0KPg0KPiBAQCAtNjAzLDYgKzYwNCw4IEBAIHN0 YXRpYyB2b2lkIG5iZF9yZXNldChzdHJ1Y3QgbmJkX2RldmljZSAqbmJkKQ0KPg0KPiAgc3RhdGlj IHZvaWQgbmJkX2JkZXZfcmVzZXQoc3RydWN0IGJsb2NrX2RldmljZSAqYmRldikNCj4gIHsNCj4g KyAgICAgICBpZiAoYmRldi0+YmRfb3BlbmVycyA+IDEpDQo+ICsgICAgICAgICAgICAgICByZXR1 cm47DQo+ICAgICAgICAgc2V0X2RldmljZV9ybyhiZGV2LCBmYWxzZSk7DQo+ICAgICAgICAgYmRl di0+YmRfaW5vZGUtPmlfc2l6ZSA9IDA7DQo+ICAgICAgICAgaWYgKG1heF9wYXJ0ID4gMCkgew0K PiBAQCAtNjY2LDcgKzY2OSw4IEBAIHN0YXRpYyBpbnQgbmJkX2NsZWFyX3NvY2soc3RydWN0IG5i ZF9kZXZpY2UgKm5iZCwgc3RydWN0IGJsb2NrX2RldmljZSAqYmRldikNCj4gIHsNCj4gICAgICAg ICBzb2NrX3NodXRkb3duKG5iZCk7DQo+ICAgICAgICAgbmJkX2NsZWFyX3F1ZShuYmQpOw0KPiAt ICAgICAgIGtpbGxfYmRldihiZGV2KTsNCj4gKw0KPiArICAgICAgIF9faW52YWxpZGF0ZV9kZXZp Y2UoYmRldiwgdHJ1ZSk7DQo+ICAgICAgICAgbmJkX2JkZXZfcmVzZXQoYmRldik7DQo+ICAgICAg ICAgLyoNCj4gICAgICAgICAgKiBXZSB3YW50IHRvIGdpdmUgdGhlIHJ1biB0aHJlYWQgYSBjaGFu Y2UgdG8gd2FpdCBmb3IgZXZlcnlib2R5DQo+IC0tDQo+IDEuOC4zLjENCg0KSGkgSm9zZWYsDQoN CkFueSBjb21tZW50cz8NCg0KVGhhbmtzLA0KTWluZw0KDQoNCg==

#4 lore raw

Re: [RFC PATCH 1/1] nbd: replace kill_bdev() with __invalidate_device()

In reply to Ming Lin

WWVhaCBJIHRoaW5rIHRoaXMgaXMgb2ssIEnigJlsbCB0aHJvdyBpdCBvbiBteSBxdWV1ZSBmb3Ig Zml4ZXMgZm9yIHRoaXMgY3ljbGUuICBUaGFua3MsDQoNCkpvc2VmDQoNCk9uIDMvMjAvMTcsIDY6 NTggUE0sICJNaW5nIExpbiIgPG1saW5Aa2VybmVsLm9yZz4gd3JvdGU6DQoNCkZyb206IFJhdG5h IE1hbm9qIEJvbGxhIDxtYW5vai5ickBnbWFpbC5jb20+DQoNCldoZW4gYSBmaWxlc3lzdGVtIGlz IG1vdW50ZWQgb24gYSBuYmQgZGV2aWNlIGFuZCBvbiBhIGRpc2Nvbm5lY3QsIGJlY2F1c2UNCm9m IGtpbGxfYmRldigpLCBhbmQgcmVzZXR0aW5nIGJkZXYgc2l6ZSB0byB6ZXJvLCBidWZmZXJfaGVh ZCBtYXBwaW5ncyBhcmUNCmdldHRpbmcgZGVzdHJveWVkIHVuZGVyIG1vdW50ZWQgZmlsZXN5c3Rl bS4NCg0KQWZ0ZXIgYSBiZGV2IHNpemUgcmVzZXQoaS5lIGJkZXYtPmJkX2lub2RlLT5pX3NpemUg PSAwKSBvbiBhIGRpc2Nvbm5lY3QsDQpmb2xsb3dlZCBieSBhIHN5c191bW91bnQoKSwNCiAgICAg ICAgZ2VuZXJpY19zaHV0ZG93bl9zdXBlcigpLT4uLi4NCiAgICAgICAgLT5fX3N5bmNfYmxvY2tk ZXYoKS0+Li4uDQogICAgICAgIC1ibGtkZXZfd3JpdGVwYWdlcygpLT4uLi4NCiAgICAgICAgLT5k b19pbnZhbGlkYXRlcGFnZSgpLT4uLi4NCiAgICAgICAgLWRpc2NhcmRfYnVmZmVyKCkgICBpcyBk aXNjYXJkaW5nIHN1cGVyYmxvY2sgYnVmZmVyX2hlYWQgYXNzdW1lZA0KdG8gYmUgaW4gbWFwcGVk IHN0YXRlIGJ5IGV4dDRfY29tbWl0X3N1cGVyKCkuDQoNClttbGluOiBwb3J0ZWQgdG8gNC4xMS1y YzJdDQpTaWduZWQtb2ZmLWJ5OiBSYXRuYSBNYW5vaiBCb2xsYSA8bWFub2ouYnJAZ21haWwuY29t DQotLS0NCiBkcml2ZXJzL2Jsb2NrL25iZC5jIHwgOCArKysrKystLQ0KIDEgZmlsZSBjaGFuZ2Vk LCA2IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pDQoNCmRpZmYgLS1naXQgYS9kcml2ZXJz L2Jsb2NrL25iZC5jIGIvZHJpdmVycy9ibG9jay9uYmQuYw0KaW5kZXggY2I0Y2NmYy4uYTZhMzY0 MyAxMDA2NDQNCi0tLSBhL2RyaXZlcnMvYmxvY2svbmJkLmMNCisrKyBiL2RyaXZlcnMvYmxvY2sv bmJkLmMNCkBAIC0xMjUsNyArMTI1LDggQEAgc3RhdGljIGNvbnN0IGNoYXIgKm5iZGNtZF90b19h c2NpaShpbnQgY21kKQ0KIA0KIHN0YXRpYyBpbnQgbmJkX3NpemVfY2xlYXIoc3RydWN0IG5iZF9k ZXZpY2UgKm5iZCwgc3RydWN0IGJsb2NrX2RldmljZSAqYmRldikNCiB7DQotCWJkX3NldF9zaXpl KGJkZXYsIDApOw0KKwlpZiAoYmRldi0+YmRfb3BlbmVycyA8PSAxKQ0KKwkJYmRfc2V0X3NpemUo YmRldiwgMCk7DQogCXNldF9jYXBhY2l0eShuYmQtPmRpc2ssIDApOw0KIAlrb2JqZWN0X3VldmVu dCgmbmJkX3RvX2RldihuYmQpLT5rb2JqLCBLT0JKX0NIQU5HRSk7DQogDQpAQCAtNjAzLDYgKzYw NCw4IEBAIHN0YXRpYyB2b2lkIG5iZF9yZXNldChzdHJ1Y3QgbmJkX2RldmljZSAqbmJkKQ0KIA0K IHN0YXRpYyB2b2lkIG5iZF9iZGV2X3Jlc2V0KHN0cnVjdCBibG9ja19kZXZpY2UgKmJkZXYpDQog ew0KKwlpZiAoYmRldi0+YmRfb3BlbmVycyA+IDEpDQorCQlyZXR1cm47DQogCXNldF9kZXZpY2Vf cm8oYmRldiwgZmFsc2UpOw0KIAliZGV2LT5iZF9pbm9kZS0+aV9zaXplID0gMDsNCiAJaWYgKG1h eF9wYXJ0ID4gMCkgew0KQEAgLTY2Niw3ICs2NjksOCBAQCBzdGF0aWMgaW50IG5iZF9jbGVhcl9z b2NrKHN0cnVjdCBuYmRfZGV2aWNlICpuYmQsIHN0cnVjdCBibG9ja19kZXZpY2UgKmJkZXYpDQog ew0KIAlzb2NrX3NodXRkb3duKG5iZCk7DQogCW5iZF9jbGVhcl9xdWUobmJkKTsNCi0Ja2lsbF9i ZGV2KGJkZXYpOw0KKw0KKwlfX2ludmFsaWRhdGVfZGV2aWNlKGJkZXYsIHRydWUpOw0KIAluYmRf YmRldl9yZXNldChiZGV2KTsNCiAJLyoNCiAJICogV2Ugd2FudCB0byBnaXZlIHRoZSBydW4gdGhy ZWFkIGEgY2hhbmNlIHRvIHdhaXQgZm9yIGV2ZXJ5Ym9keQ0KLS0gDQoxLjguMy4xDQoNCg0KDQo=

author
#5 lore raw

Re: [RFC PATCH 1/1] nbd: replace kill_bdev() with __invalidate_device()

Quoted 1 line · Josef Bacik

Yeah I think this is ok, I’ll throw it on my queue for fixes for this cycle. Thanks,

Great. Thanks.

Quoted 2 lines

Josef