[PATCH v2] btrfs: fix devid 0 without a replace item by failing the mount

[PATCH v2] btrfs: fix devid 0 without a replace item by failing the mount

[Anand Jain]

If there is a BTRFS_DEV_REPLACE_DEVID without a replace item, then
it means some device is trying to attack or may be corrupted. Fail the
mount so that the user can remove the attacking or fix the corrupted
device.

As of now if BTRFS_DEV_REPLACE_DEVID is present without the replace
item, then in __btrfs_free_extra_devids() we determine that there is an
extra device, and free those extra devices but continue to mount the
device.
However, we were wrong in keeping tack of the rw_devices so the syzbot
testcase failed as below [1].

[1]
WARNING: CPU: 1 PID: 3612 at fs/btrfs/volumes.c:1166 close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 3612 Comm: syz-executor.2 Not tainted 5.9.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x198/0x1fd lib/dump_stack.c:118
 panic+0x347/0x7c0 kernel/panic.c:231
 __warn.cold+0x20/0x46 kernel/panic.c:600
 report_bug+0x1bd/0x210 lib/bug.c:198
 handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234
 exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254
 asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536
RIP: 0010:close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Code: 0f b6 04 02 84 c0 74 02 7e 33 48 8b 44 24 18 c6 80 30 01 00 00 00 48 83 c4 30 5b 5d 41 5c 41 5d 41 5e 41 5f c3 e8 99 ce 6a fe <0f> 0b e9 71 ff ff ff e8 8d ce 6a fe 0f 0b e9 20 ff ff ff e8 d1 d5
RSP: 0018:ffffc900091777e0 EFLAGS: 00010246
RAX: 0000000000040000 RBX: ffffffffffffffff RCX: ffffc9000c8b7000
RDX: 0000000000040000 RSI: ffffffff83097f47 RDI: 0000000000000007
RBP: dffffc0000000000 R08: 0000000000000001 R09: ffff8880988a187f
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88809593a130
R13: ffff88809593a1ec R14: ffff8880988a1908 R15: ffff88809593a050
 close_fs_devices fs/btrfs/volumes.c:1193 [inline]
 btrfs_close_devices+0x95/0x1f0 fs/btrfs/volumes.c:1179
 open_ctree+0x4984/0x4a2d fs/btrfs/disk-io.c:3434
 btrfs_fill_super fs/btrfs/super.c:1316 [inline]
 btrfs_mount_root.cold+0x14/0x165 fs/btrfs/super.c:1672

The fix here is, when we determine that there isn't a replace item
then fail the mount if there is a replace target device (devid 0).

The reproducer looks like this.
 mkfs.btrfs -fq /dev/sda
 dd if=/dev/sda of=/dev/sdb bs=1 seek=64K skip=64K count=4096
 btrfs-sv-mod /dev/sdb devid=0
 mount -o device=/dev/sdb /dev/sda /btrfs

Reported-by: syzbot+4cfe71a4da060be47502@syzkaller.appspotmail.com
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
Depends on the patches
 btrfs: drop never met condition of disk_total_bytes == 0
 btrfs: fix btrfs_find_device unused arg seed
If these patches aren't integrated yet, then please add the last arg in
the function btrfs_find_device(). Any value is fine as it doesn't care.

fstest case will follow.

v2: changed title
    old: btrfs: fix rw_devices count in __btrfs_free_extra_devids

    In btrfs_init_dev_replace() try to match the presence of replace_item
    with the BTRFS_DEV_REPLACE_DEVID device. If fails then fail the
    mount. So drop the similar check in __btrfs_free_extra_devids().

 fs/btrfs/dev-replace.c | 26 ++++++++++++++++++++++++--
 fs/btrfs/volumes.c     | 26 +++++++-------------------
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 02a54177c0df..7a96380a9f1e 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -91,6 +91,17 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
 	if (ret) {
 no_valid_dev_replace_entry_found:
+		/*
+		 * We don't have a replace item or it's corrupted.
+		 * If there is a replace target, fail the mount.
+		 */
+		if (btrfs_find_device(fs_info->fs_devices,
+				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+			btrfs_err(fs_info,
+			"found replace target device without a replace item");
+			ret = -EIO;
+			goto out;
+		}
 		ret = 0;
 		dev_replace->replace_state =
 			BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
@@ -143,8 +154,19 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
-		dev_replace->srcdev = NULL;
-		dev_replace->tgtdev = NULL;
+		/*
+		 * We don't have an active replace item but if there is a
+		 * replace target, fail the mount.
+		 */
+		if (btrfs_find_device(fs_info->fs_devices,
+				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+			btrfs_err(fs_info,
+			"replace devid present without an active replace item");
+			ret = -EIO;
+		} else {
+			dev_replace->srcdev = NULL;
+			dev_replace->tgtdev = NULL;
+		}
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a88655d60a94..0c6049f9ace3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1056,22 +1056,13 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 			continue;
 		}
 
-		if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
-			/*
-			 * In the first step, keep the device which has
-			 * the correct fsid and the devid that is used
-			 * for the dev_replace procedure.
-			 * In the second step, the dev_replace state is
-			 * read from the device tree and it is known
-			 * whether the procedure is really active or
-			 * not, which means whether this device is
-			 * used or whether it should be removed.
-			 */
-			if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-						  &device->dev_state)) {
-				continue;
-			}
-		}
+		/*
+		 * We have already validated the presence of BTRFS_DEV_REPLACE_DEVID,
+		 * in btrfs_init_dev_replace() so just continue.
+		 */
+		if (device->devid == BTRFS_DEV_REPLACE_DEVID)
+			continue;
+
 		if (device->bdev) {
 			blkdev_put(device->bdev, device->mode);
 			device->bdev = NULL;
@@ -1080,9 +1071,6 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 		if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
 			list_del_init(&device->dev_alloc_list);
 			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
-			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-				      &device->dev_state))
-				fs_devices->rw_devices--;
 		}
 		list_del_init(&device->dev_list);
 		fs_devices->num_devices--;
-- 
2.25.1

[PATCH v2 add prerequisite-patch-id] btrfs: fix devid 0 without a replace item by failing the mount

[Anand Jain]

If there is a BTRFS_DEV_REPLACE_DEVID without a replace item, then
it means some device is trying to attack or may be corrupted. Fail the
mount so that the user can remove the attacking or fix the corrupted
device.

As of now if BTRFS_DEV_REPLACE_DEVID is present without the replace
item, in __btrfs_free_extra_devids() we determine that there is an
extra device, and free those extra devices but continue to mount the
device.
However, we were wrong in keeping tack of the rw_devices so the syzbot
testcase failed as below [1].

[1]
WARNING: CPU: 1 PID: 3612 at fs/btrfs/volumes.c:1166 close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 3612 Comm: syz-executor.2 Not tainted 5.9.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x198/0x1fd lib/dump_stack.c:118
 panic+0x347/0x7c0 kernel/panic.c:231
 __warn.cold+0x20/0x46 kernel/panic.c:600
 report_bug+0x1bd/0x210 lib/bug.c:198
 handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234
 exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254
 asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536
RIP: 0010:close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Code: 0f b6 04 02 84 c0 74 02 7e 33 48 8b 44 24 18 c6 80 30 01 00 00 00 48 83 c4 30 5b 5d 41 5c 41 5d 41 5e 41 5f c3 e8 99 ce 6a fe <0f> 0b e9 71 ff ff ff e8 8d ce 6a fe 0f 0b e9 20 ff ff ff e8 d1 d5
RSP: 0018:ffffc900091777e0 EFLAGS: 00010246
RAX: 0000000000040000 RBX: ffffffffffffffff RCX: ffffc9000c8b7000
RDX: 0000000000040000 RSI: ffffffff83097f47 RDI: 0000000000000007
RBP: dffffc0000000000 R08: 0000000000000001 R09: ffff8880988a187f
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88809593a130
R13: ffff88809593a1ec R14: ffff8880988a1908 R15: ffff88809593a050
 close_fs_devices fs/btrfs/volumes.c:1193 [inline]
 btrfs_close_devices+0x95/0x1f0 fs/btrfs/volumes.c:1179
 open_ctree+0x4984/0x4a2d fs/btrfs/disk-io.c:3434
 btrfs_fill_super fs/btrfs/super.c:1316 [inline]
 btrfs_mount_root.cold+0x14/0x165 fs/btrfs/super.c:1672

The fix here is, when we determine that there isn't a replace item
then fail the mount if there is a replace target device (devid 0).

Reported-by: syzbot+4cfe71a4da060be47502@syzkaller.appspotmail.com
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
Depends on the patches
 btrfs: drop never met condition of disk_total_bytes == 0
 btrfs: fix btrfs_find_device unused arg seed
If these patches aren't integrated yet, then please add the last arg in
the function btrfs_find_device(). Any value is fine as it doesn't care.

fstest case will follow.

v2: changed title
    old: btrfs: fix rw_devices count in __btrfs_free_extra_devids

    In btrfs_init_dev_replace() try to match the presence of replace_item
    to the BTRFS_DEV_REPLACE_DEVID device. If fails then fail the
    mount. So drop the similar check in __btrfs_free_extra_devids().

 fs/btrfs/dev-replace.c | 26 ++++++++++++++++++++++++--
 fs/btrfs/volumes.c     | 26 +++++++-------------------
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 9340d03661cd..e2b7ae386224 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -91,6 +91,17 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
 	if (ret) {
 no_valid_dev_replace_entry_found:
+		/*
+		 * We don't have a replace item or it's corrupted.
+		 * If there is a replace target, fail the mount.
+		 */
+		if (btrfs_find_device(fs_info->fs_devices,
+				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+			btrfs_err(fs_info,
+			"found replace target device without a replace item");
+			ret = -EIO;
+			goto out;
+		}
 		ret = 0;
 		dev_replace->replace_state =
 			BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
@@ -143,8 +154,19 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
-		dev_replace->srcdev = NULL;
-		dev_replace->tgtdev = NULL;
+		/*
+		 * We don't have an active replace item but if there is a
+		 * replace target, fail the mount.
+		 */
+		if (btrfs_find_device(fs_info->fs_devices,
+				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+			btrfs_err(fs_info,
+			"replace devid present without an active replace item");
+			ret = -EIO;
+		} else {
+			dev_replace->srcdev = NULL;
+			dev_replace->tgtdev = NULL;
+		}
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a88655d60a94..0c6049f9ace3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1056,22 +1056,13 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 			continue;
 		}
 
-		if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
-			/*
-			 * In the first step, keep the device which has
-			 * the correct fsid and the devid that is used
-			 * for the dev_replace procedure.
-			 * In the second step, the dev_replace state is
-			 * read from the device tree and it is known
-			 * whether the procedure is really active or
-			 * not, which means whether this device is
-			 * used or whether it should be removed.
-			 */
-			if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-						  &device->dev_state)) {
-				continue;
-			}
-		}
+		/*
+		 * We have already validated the presence of BTRFS_DEV_REPLACE_DEVID,
+		 * in btrfs_init_dev_replace() so just continue.
+		 */
+		if (device->devid == BTRFS_DEV_REPLACE_DEVID)
+			continue;
+
 		if (device->bdev) {
 			blkdev_put(device->bdev, device->mode);
 			device->bdev = NULL;
@@ -1080,9 +1071,6 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 		if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
 			list_del_init(&device->dev_alloc_list);
 			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
-			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-				      &device->dev_state))
-				fs_devices->rw_devices--;
 		}
 		list_del_init(&device->dev_list);
 		fs_devices->num_devices--;

base-commit: 1fd4033dd011a3525bacddf37ab9eac425d25c4f
prerequisite-patch-id: 0d3416ab45d924135a9095c3d9c68646f7c5e476
prerequisite-patch-id: 51a2e9b4b78bf808279307d03436a33063d42130
-- 
2.25.1

[PATCH RESEND v2 add prerequisite-patch-id] btrfs: fix devid 0 without a replace item by failing the mount

[Anand Jain]

If there is a BTRFS_DEV_REPLACE_DEVID without a replace item, then
it means some device is trying to attack or may be corrupted. Fail the
mount so that the user can remove the attacking or fix the corrupted
device.

As of now if BTRFS_DEV_REPLACE_DEVID is present without the replace
item, in __btrfs_free_extra_devids() we determine that there is an
extra device, and free those extra devices but continue to mount the
device.
However, we were wrong in keeping tack of the rw_devices so the syzbot
testcase failed as below [1].

[1]
WARNING: CPU: 1 PID: 3612 at fs/btrfs/volumes.c:1166 close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 3612 Comm: syz-executor.2 Not tainted 5.9.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x198/0x1fd lib/dump_stack.c:118
 panic+0x347/0x7c0 kernel/panic.c:231
 __warn.cold+0x20/0x46 kernel/panic.c:600
 report_bug+0x1bd/0x210 lib/bug.c:198
 handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234
 exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254
 asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536
RIP: 0010:close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Code: 0f b6 04 02 84 c0 74 02 7e 33 48 8b 44 24 18 c6 80 30 01 00 00 00 48 83 c4 30 5b 5d 41 5c 41 5d 41 5e 41 5f c3 e8 99 ce 6a fe <0f> 0b e9 71 ff ff ff e8 8d ce 6a fe 0f 0b e9 20 ff ff ff e8 d1 d5
RSP: 0018:ffffc900091777e0 EFLAGS: 00010246
RAX: 0000000000040000 RBX: ffffffffffffffff RCX: ffffc9000c8b7000
RDX: 0000000000040000 RSI: ffffffff83097f47 RDI: 0000000000000007
RBP: dffffc0000000000 R08: 0000000000000001 R09: ffff8880988a187f
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88809593a130
R13: ffff88809593a1ec R14: ffff8880988a1908 R15: ffff88809593a050
 close_fs_devices fs/btrfs/volumes.c:1193 [inline]
 btrfs_close_devices+0x95/0x1f0 fs/btrfs/volumes.c:1179
 open_ctree+0x4984/0x4a2d fs/btrfs/disk-io.c:3434
 btrfs_fill_super fs/btrfs/super.c:1316 [inline]
 btrfs_mount_root.cold+0x14/0x165 fs/btrfs/super.c:1672

The fix here is, when we determine that there isn't a replace item
then fail the mount if there is a replace target device (devid 0).

Reported-by: syzbot+4cfe71a4da060be47502@syzkaller.appspotmail.com
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
Depends on the patches
 btrfs: drop never met condition of disk_total_bytes == 0
 btrfs: fix btrfs_find_device unused arg seed
If these patches aren't integrated yet, then please add the last arg in
the function btrfs_find_device(). Any value is fine as it doesn't care.

fstest case will follow.

v2: changed title
    old: btrfs: fix rw_devices count in __btrfs_free_extra_devids

    In btrfs_init_dev_replace() try to match the presence of replace_item
    to the BTRFS_DEV_REPLACE_DEVID device. If fails then fail the
    mount. So drop the similar check in __btrfs_free_extra_devids().

 fs/btrfs/dev-replace.c | 26 ++++++++++++++++++++++++--
 fs/btrfs/volumes.c     | 26 +++++++-------------------
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 9340d03661cd..e2b7ae386224 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -91,6 +91,17 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
 	if (ret) {
 no_valid_dev_replace_entry_found:
+		/*
+		 * We don't have a replace item or it's corrupted.
+		 * If there is a replace target, fail the mount.
+		 */
+		if (btrfs_find_device(fs_info->fs_devices,
+				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+			btrfs_err(fs_info,
+			"found replace target device without a replace item");
+			ret = -EIO;
+			goto out;
+		}
 		ret = 0;
 		dev_replace->replace_state =
 			BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
@@ -143,8 +154,19 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
-		dev_replace->srcdev = NULL;
-		dev_replace->tgtdev = NULL;
+		/*
+		 * We don't have an active replace item but if there is a
+		 * replace target, fail the mount.
+		 */
+		if (btrfs_find_device(fs_info->fs_devices,
+				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+			btrfs_err(fs_info,
+			"replace devid present without an active replace item");
+			ret = -EIO;
+		} else {
+			dev_replace->srcdev = NULL;
+			dev_replace->tgtdev = NULL;
+		}
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a88655d60a94..0c6049f9ace3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1056,22 +1056,13 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 			continue;
 		}
 
-		if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
-			/*
-			 * In the first step, keep the device which has
-			 * the correct fsid and the devid that is used
-			 * for the dev_replace procedure.
-			 * In the second step, the dev_replace state is
-			 * read from the device tree and it is known
-			 * whether the procedure is really active or
-			 * not, which means whether this device is
-			 * used or whether it should be removed.
-			 */
-			if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-						  &device->dev_state)) {
-				continue;
-			}
-		}
+		/*
+		 * We have already validated the presence of BTRFS_DEV_REPLACE_DEVID,
+		 * in btrfs_init_dev_replace() so just continue.
+		 */
+		if (device->devid == BTRFS_DEV_REPLACE_DEVID)
+			continue;
+
 		if (device->bdev) {
 			blkdev_put(device->bdev, device->mode);
 			device->bdev = NULL;
@@ -1080,9 +1071,6 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 		if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
 			list_del_init(&device->dev_alloc_list);
 			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
-			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-				      &device->dev_state))
-				fs_devices->rw_devices--;
 		}
 		list_del_init(&device->dev_list);
 		fs_devices->num_devices--;

base-commit: 1fd4033dd011a3525bacddf37ab9eac425d25c4f
prerequisite-patch-id: 0d3416ab45d924135a9095c3d9c68646f7c5e476
prerequisite-patch-id: 51a2e9b4b78bf808279307d03436a33063d42130
-- 
2.25.1

Re: [PATCH v2 add prerequisite-patch-id] btrfs: fix devid 0 without a replace item by failing the mount

[Anand Jain]

Accidentally this patch went to the stable. Pls, ignore. Sorry for the 
noise. Thanks.


On 12/10/20 1:26 pm, Anand Jain wrote:
> If there is a BTRFS_DEV_REPLACE_DEVID without a replace item, then
> it means some device is trying to attack or may be corrupted. Fail the
> mount so that the user can remove the attacking or fix the corrupted
> device.
> 
> As of now if BTRFS_DEV_REPLACE_DEVID is present without the replace
> item, in __btrfs_free_extra_devids() we determine that there is an
> extra device, and free those extra devices but continue to mount the
> device.
> However, we were wrong in keeping tack of the rw_devices so the syzbot
> testcase failed as below [1].
> 
> [1]
> WARNING: CPU: 1 PID: 3612 at fs/btrfs/volumes.c:1166 close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
> Kernel panic - not syncing: panic_on_warn set ...
> CPU: 1 PID: 3612 Comm: syz-executor.2 Not tainted 5.9.0-rc4-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
>   __dump_stack lib/dump_stack.c:77 [inline]
>   dump_stack+0x198/0x1fd lib/dump_stack.c:118
>   panic+0x347/0x7c0 kernel/panic.c:231
>   __warn.cold+0x20/0x46 kernel/panic.c:600
>   report_bug+0x1bd/0x210 lib/bug.c:198
>   handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234
>   exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254
>   asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536
> RIP: 0010:close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
> Code: 0f b6 04 02 84 c0 74 02 7e 33 48 8b 44 24 18 c6 80 30 01 00 00 00 48 83 c4 30 5b 5d 41 5c 41 5d 41 5e 41 5f c3 e8 99 ce 6a fe <0f> 0b e9 71 ff ff ff e8 8d ce 6a fe 0f 0b e9 20 ff ff ff e8 d1 d5
> RSP: 0018:ffffc900091777e0 EFLAGS: 00010246
> RAX: 0000000000040000 RBX: ffffffffffffffff RCX: ffffc9000c8b7000
> RDX: 0000000000040000 RSI: ffffffff83097f47 RDI: 0000000000000007
> RBP: dffffc0000000000 R08: 0000000000000001 R09: ffff8880988a187f
> R10: 0000000000000000 R11: 0000000000000001 R12: ffff88809593a130
> R13: ffff88809593a1ec R14: ffff8880988a1908 R15: ffff88809593a050
>   close_fs_devices fs/btrfs/volumes.c:1193 [inline]
>   btrfs_close_devices+0x95/0x1f0 fs/btrfs/volumes.c:1179
>   open_ctree+0x4984/0x4a2d fs/btrfs/disk-io.c:3434
>   btrfs_fill_super fs/btrfs/super.c:1316 [inline]
>   btrfs_mount_root.cold+0x14/0x165 fs/btrfs/super.c:1672
> 
> The fix here is, when we determine that there isn't a replace item
> then fail the mount if there is a replace target device (devid 0).
> 
> Reported-by: syzbot+4cfe71a4da060be47502@syzkaller.appspotmail.com
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> Depends on the patches
>   btrfs: drop never met condition of disk_total_bytes == 0
>   btrfs: fix btrfs_find_device unused arg seed
> If these patches aren't integrated yet, then please add the last arg in
> the function btrfs_find_device(). Any value is fine as it doesn't care.
> 
> fstest case will follow.
> 
> v2: changed title
>      old: btrfs: fix rw_devices count in __btrfs_free_extra_devids
> 
>      In btrfs_init_dev_replace() try to match the presence of replace_item
>      to the BTRFS_DEV_REPLACE_DEVID device. If fails then fail the
>      mount. So drop the similar check in __btrfs_free_extra_devids().
> 
>   fs/btrfs/dev-replace.c | 26 ++++++++++++++++++++++++--
>   fs/btrfs/volumes.c     | 26 +++++++-------------------
>   2 files changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 9340d03661cd..e2b7ae386224 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -91,6 +91,17 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
>   	ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
>   	if (ret) {
>   no_valid_dev_replace_entry_found:
> +		/*
> +		 * We don't have a replace item or it's corrupted.
> +		 * If there is a replace target, fail the mount.
> +		 */
> +		if (btrfs_find_device(fs_info->fs_devices,
> +				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
> +			btrfs_err(fs_info,
> +			"found replace target device without a replace item");
> +			ret = -EIO;
> +			goto out;
> +		}
>   		ret = 0;
>   		dev_replace->replace_state =
>   			BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
> @@ -143,8 +154,19 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
>   	case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
>   	case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
>   	case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
> -		dev_replace->srcdev = NULL;
> -		dev_replace->tgtdev = NULL;
> +		/*
> +		 * We don't have an active replace item but if there is a
> +		 * replace target, fail the mount.
> +		 */
> +		if (btrfs_find_device(fs_info->fs_devices,
> +				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
> +			btrfs_err(fs_info,
> +			"replace devid present without an active replace item");
> +			ret = -EIO;
> +		} else {
> +			dev_replace->srcdev = NULL;
> +			dev_replace->tgtdev = NULL;
> +		}
>   		break;
>   	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
>   	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index a88655d60a94..0c6049f9ace3 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1056,22 +1056,13 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
>   			continue;
>   		}
>   
> -		if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
> -			/*
> -			 * In the first step, keep the device which has
> -			 * the correct fsid and the devid that is used
> -			 * for the dev_replace procedure.
> -			 * In the second step, the dev_replace state is
> -			 * read from the device tree and it is known
> -			 * whether the procedure is really active or
> -			 * not, which means whether this device is
> -			 * used or whether it should be removed.
> -			 */
> -			if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
> -						  &device->dev_state)) {
> -				continue;
> -			}
> -		}
> +		/*
> +		 * We have already validated the presence of BTRFS_DEV_REPLACE_DEVID,
> +		 * in btrfs_init_dev_replace() so just continue.
> +		 */
> +		if (device->devid == BTRFS_DEV_REPLACE_DEVID)
> +			continue;
> +
>   		if (device->bdev) {
>   			blkdev_put(device->bdev, device->mode);
>   			device->bdev = NULL;
> @@ -1080,9 +1071,6 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
>   		if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
>   			list_del_init(&device->dev_alloc_list);
>   			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
> -			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
> -				      &device->dev_state))
> -				fs_devices->rw_devices--;
>   		}
>   		list_del_init(&device->dev_list);
>   		fs_devices->num_devices--;
> 
> base-commit: 1fd4033dd011a3525bacddf37ab9eac425d25c4f
> prerequisite-patch-id: 0d3416ab45d924135a9095c3d9c68646f7c5e476
> prerequisite-patch-id: 51a2e9b4b78bf808279307d03436a33063d42130
> 

Re: [PATCH RESEND v2 add prerequisite-patch-id] btrfs: fix devid 0 without a replace item by failing the mount

[kernel test robot]

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

Hi Anand,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-5.10]
[also build test ERROR on kdave/for-next 1fd4033dd011a3525bacddf37ab9eac425d25c4f v5.9 next-20201021]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201021-120412
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-5.10
config: i386-randconfig-a002-20201021 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/22febac7db326006d990e08b9d808cfaabc672bf
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201021-120412
        git checkout 22febac7db326006d990e08b9d808cfaabc672bf
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/btrfs/dev-replace.c: In function 'btrfs_init_dev_replace':
>> fs/btrfs/dev-replace.c:98:7: error: too few arguments to function 'btrfs_find_device'
      98 |   if (btrfs_find_device(fs_info->fs_devices,
         |       ^~~~~~~~~~~~~~~~~
   In file included from fs/btrfs/dev-replace.c:18:
   fs/btrfs/volumes.h:455:22: note: declared here
     455 | struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
         |                      ^~~~~~~~~~~~~~~~~
   fs/btrfs/dev-replace.c:161:7: error: too few arguments to function 'btrfs_find_device'
     161 |   if (btrfs_find_device(fs_info->fs_devices,
         |       ^~~~~~~~~~~~~~~~~
   In file included from fs/btrfs/dev-replace.c:18:
   fs/btrfs/volumes.h:455:22: note: declared here
     455 | struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
         |                      ^~~~~~~~~~~~~~~~~

vim +/btrfs_find_device +98 fs/btrfs/dev-replace.c

    68	
    69	int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
    70	{
    71		struct btrfs_key key;
    72		struct btrfs_root *dev_root = fs_info->dev_root;
    73		struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
    74		struct extent_buffer *eb;
    75		int slot;
    76		int ret = 0;
    77		struct btrfs_path *path = NULL;
    78		int item_size;
    79		struct btrfs_dev_replace_item *ptr;
    80		u64 src_devid;
    81	
    82		path = btrfs_alloc_path();
    83		if (!path) {
    84			ret = -ENOMEM;
    85			goto out;
    86		}
    87	
    88		key.objectid = 0;
    89		key.type = BTRFS_DEV_REPLACE_KEY;
    90		key.offset = 0;
    91		ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
    92		if (ret) {
    93	no_valid_dev_replace_entry_found:
    94			/*
    95			 * We don't have a replace item or it's corrupted.
    96			 * If there is a replace target, fail the mount.
    97			 */
  > 98			if (btrfs_find_device(fs_info->fs_devices,
    99					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
   100				btrfs_err(fs_info,
   101				"found replace target device without a replace item");
   102				ret = -EIO;
   103				goto out;
   104			}
   105			ret = 0;
   106			dev_replace->replace_state =
   107				BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
   108			dev_replace->cont_reading_from_srcdev_mode =
   109			    BTRFS_DEV_REPLACE_ITEM_CONT_READING_FROM_SRCDEV_MODE_ALWAYS;
   110			dev_replace->time_started = 0;
   111			dev_replace->time_stopped = 0;
   112			atomic64_set(&dev_replace->num_write_errors, 0);
   113			atomic64_set(&dev_replace->num_uncorrectable_read_errors, 0);
   114			dev_replace->cursor_left = 0;
   115			dev_replace->committed_cursor_left = 0;
   116			dev_replace->cursor_left_last_write_of_item = 0;
   117			dev_replace->cursor_right = 0;
   118			dev_replace->srcdev = NULL;
   119			dev_replace->tgtdev = NULL;
   120			dev_replace->is_valid = 0;
   121			dev_replace->item_needs_writeback = 0;
   122			goto out;
   123		}
   124		slot = path->slots[0];
   125		eb = path->nodes[0];
   126		item_size = btrfs_item_size_nr(eb, slot);
   127		ptr = btrfs_item_ptr(eb, slot, struct btrfs_dev_replace_item);
   128	
   129		if (item_size != sizeof(struct btrfs_dev_replace_item)) {
   130			btrfs_warn(fs_info,
   131				"dev_replace entry found has unexpected size, ignore entry");
   132			goto no_valid_dev_replace_entry_found;
   133		}
   134	
   135		src_devid = btrfs_dev_replace_src_devid(eb, ptr);
   136		dev_replace->cont_reading_from_srcdev_mode =
   137			btrfs_dev_replace_cont_reading_from_srcdev_mode(eb, ptr);
   138		dev_replace->replace_state = btrfs_dev_replace_replace_state(eb, ptr);
   139		dev_replace->time_started = btrfs_dev_replace_time_started(eb, ptr);
   140		dev_replace->time_stopped =
   141			btrfs_dev_replace_time_stopped(eb, ptr);
   142		atomic64_set(&dev_replace->num_write_errors,
   143			     btrfs_dev_replace_num_write_errors(eb, ptr));
   144		atomic64_set(&dev_replace->num_uncorrectable_read_errors,
   145			     btrfs_dev_replace_num_uncorrectable_read_errors(eb, ptr));
   146		dev_replace->cursor_left = btrfs_dev_replace_cursor_left(eb, ptr);
   147		dev_replace->committed_cursor_left = dev_replace->cursor_left;
   148		dev_replace->cursor_left_last_write_of_item = dev_replace->cursor_left;
   149		dev_replace->cursor_right = btrfs_dev_replace_cursor_right(eb, ptr);
   150		dev_replace->is_valid = 1;
   151	
   152		dev_replace->item_needs_writeback = 0;
   153		switch (dev_replace->replace_state) {
   154		case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
   155		case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
   156		case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
   157			/*
   158			 * We don't have an active replace item but if there is a
   159			 * replace target, fail the mount.
   160			 */
   161			if (btrfs_find_device(fs_info->fs_devices,
   162					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
   163				btrfs_err(fs_info,
   164				"replace devid present without an active replace item");
   165				ret = -EIO;
   166			} else {
   167				dev_replace->srcdev = NULL;
   168				dev_replace->tgtdev = NULL;
   169			}
   170			break;
   171		case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
   172		case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
   173			dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
   174							src_devid, NULL, NULL, true);
   175			dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
   176								BTRFS_DEV_REPLACE_DEVID,
   177								NULL, NULL, true);
   178			/*
   179			 * allow 'btrfs dev replace_cancel' if src/tgt device is
   180			 * missing
   181			 */
   182			if (!dev_replace->srcdev &&
   183			    !btrfs_test_opt(fs_info, DEGRADED)) {
   184				ret = -EIO;
   185				btrfs_warn(fs_info,
   186				   "cannot mount because device replace operation is ongoing and");
   187				btrfs_warn(fs_info,
   188				   "srcdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
   189				   src_devid);
   190			}
   191			if (!dev_replace->tgtdev &&
   192			    !btrfs_test_opt(fs_info, DEGRADED)) {
   193				ret = -EIO;
   194				btrfs_warn(fs_info,
   195				   "cannot mount because device replace operation is ongoing and");
   196				btrfs_warn(fs_info,
   197				   "tgtdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
   198					BTRFS_DEV_REPLACE_DEVID);
   199			}
   200			if (dev_replace->tgtdev) {
   201				if (dev_replace->srcdev) {
   202					dev_replace->tgtdev->total_bytes =
   203						dev_replace->srcdev->total_bytes;
   204					dev_replace->tgtdev->disk_total_bytes =
   205						dev_replace->srcdev->disk_total_bytes;
   206					dev_replace->tgtdev->commit_total_bytes =
   207						dev_replace->srcdev->commit_total_bytes;
   208					dev_replace->tgtdev->bytes_used =
   209						dev_replace->srcdev->bytes_used;
   210					dev_replace->tgtdev->commit_bytes_used =
   211						dev_replace->srcdev->commit_bytes_used;
   212				}
   213				set_bit(BTRFS_DEV_STATE_REPLACE_TGT,
   214					&dev_replace->tgtdev->dev_state);
   215	
   216				WARN_ON(fs_info->fs_devices->rw_devices == 0);
   217				dev_replace->tgtdev->io_width = fs_info->sectorsize;
   218				dev_replace->tgtdev->io_align = fs_info->sectorsize;
   219				dev_replace->tgtdev->sector_size = fs_info->sectorsize;
   220				dev_replace->tgtdev->fs_info = fs_info;
   221				set_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
   222					&dev_replace->tgtdev->dev_state);
   223			}
   224			break;
   225		}
   226	
   227	out:
   228		btrfs_free_path(path);
   229		return ret;
   230	}
   231	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29957 bytes --]

Re: [PATCH RESEND v2 add prerequisite-patch-id] btrfs: fix devid 0 without a replace item by failing the mount

[kernel test robot]

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

Hi Anand,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-5.10]
[also build test ERROR on kdave/for-next 1fd4033dd011a3525bacddf37ab9eac425d25c4f v5.9 next-20201021]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201021-120412
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-5.10
config: i386-randconfig-a002-20201021 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/22febac7db326006d990e08b9d808cfaabc672bf
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201021-120412
        git checkout 22febac7db326006d990e08b9d808cfaabc672bf
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/btrfs/dev-replace.c: In function 'btrfs_init_dev_replace':
>> fs/btrfs/dev-replace.c:98:7: error: too few arguments to function 'btrfs_find_device'
      98 |   if (btrfs_find_device(fs_info->fs_devices,
         |       ^~~~~~~~~~~~~~~~~
   In file included from fs/btrfs/dev-replace.c:18:
   fs/btrfs/volumes.h:455:22: note: declared here
     455 | struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
         |                      ^~~~~~~~~~~~~~~~~
   fs/btrfs/dev-replace.c:161:7: error: too few arguments to function 'btrfs_find_device'
     161 |   if (btrfs_find_device(fs_info->fs_devices,
         |       ^~~~~~~~~~~~~~~~~
   In file included from fs/btrfs/dev-replace.c:18:
   fs/btrfs/volumes.h:455:22: note: declared here
     455 | struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
         |                      ^~~~~~~~~~~~~~~~~

vim +/btrfs_find_device +98 fs/btrfs/dev-replace.c

    68	
    69	int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
    70	{
    71		struct btrfs_key key;
    72		struct btrfs_root *dev_root = fs_info->dev_root;
    73		struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
    74		struct extent_buffer *eb;
    75		int slot;
    76		int ret = 0;
    77		struct btrfs_path *path = NULL;
    78		int item_size;
    79		struct btrfs_dev_replace_item *ptr;
    80		u64 src_devid;
    81	
    82		path = btrfs_alloc_path();
    83		if (!path) {
    84			ret = -ENOMEM;
    85			goto out;
    86		}
    87	
    88		key.objectid = 0;
    89		key.type = BTRFS_DEV_REPLACE_KEY;
    90		key.offset = 0;
    91		ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
    92		if (ret) {
    93	no_valid_dev_replace_entry_found:
    94			/*
    95			 * We don't have a replace item or it's corrupted.
    96			 * If there is a replace target, fail the mount.
    97			 */
  > 98			if (btrfs_find_device(fs_info->fs_devices,
    99					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
   100				btrfs_err(fs_info,
   101				"found replace target device without a replace item");
   102				ret = -EIO;
   103				goto out;
   104			}
   105			ret = 0;
   106			dev_replace->replace_state =
   107				BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
   108			dev_replace->cont_reading_from_srcdev_mode =
   109			    BTRFS_DEV_REPLACE_ITEM_CONT_READING_FROM_SRCDEV_MODE_ALWAYS;
   110			dev_replace->time_started = 0;
   111			dev_replace->time_stopped = 0;
   112			atomic64_set(&dev_replace->num_write_errors, 0);
   113			atomic64_set(&dev_replace->num_uncorrectable_read_errors, 0);
   114			dev_replace->cursor_left = 0;
   115			dev_replace->committed_cursor_left = 0;
   116			dev_replace->cursor_left_last_write_of_item = 0;
   117			dev_replace->cursor_right = 0;
   118			dev_replace->srcdev = NULL;
   119			dev_replace->tgtdev = NULL;
   120			dev_replace->is_valid = 0;
   121			dev_replace->item_needs_writeback = 0;
   122			goto out;
   123		}
   124		slot = path->slots[0];
   125		eb = path->nodes[0];
   126		item_size = btrfs_item_size_nr(eb, slot);
   127		ptr = btrfs_item_ptr(eb, slot, struct btrfs_dev_replace_item);
   128	
   129		if (item_size != sizeof(struct btrfs_dev_replace_item)) {
   130			btrfs_warn(fs_info,
   131				"dev_replace entry found has unexpected size, ignore entry");
   132			goto no_valid_dev_replace_entry_found;
   133		}
   134	
   135		src_devid = btrfs_dev_replace_src_devid(eb, ptr);
   136		dev_replace->cont_reading_from_srcdev_mode =
   137			btrfs_dev_replace_cont_reading_from_srcdev_mode(eb, ptr);
   138		dev_replace->replace_state = btrfs_dev_replace_replace_state(eb, ptr);
   139		dev_replace->time_started = btrfs_dev_replace_time_started(eb, ptr);
   140		dev_replace->time_stopped =
   141			btrfs_dev_replace_time_stopped(eb, ptr);
   142		atomic64_set(&dev_replace->num_write_errors,
   143			     btrfs_dev_replace_num_write_errors(eb, ptr));
   144		atomic64_set(&dev_replace->num_uncorrectable_read_errors,
   145			     btrfs_dev_replace_num_uncorrectable_read_errors(eb, ptr));
   146		dev_replace->cursor_left = btrfs_dev_replace_cursor_left(eb, ptr);
   147		dev_replace->committed_cursor_left = dev_replace->cursor_left;
   148		dev_replace->cursor_left_last_write_of_item = dev_replace->cursor_left;
   149		dev_replace->cursor_right = btrfs_dev_replace_cursor_right(eb, ptr);
   150		dev_replace->is_valid = 1;
   151	
   152		dev_replace->item_needs_writeback = 0;
   153		switch (dev_replace->replace_state) {
   154		case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
   155		case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
   156		case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
   157			/*
   158			 * We don't have an active replace item but if there is a
   159			 * replace target, fail the mount.
   160			 */
   161			if (btrfs_find_device(fs_info->fs_devices,
   162					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
   163				btrfs_err(fs_info,
   164				"replace devid present without an active replace item");
   165				ret = -EIO;
   166			} else {
   167				dev_replace->srcdev = NULL;
   168				dev_replace->tgtdev = NULL;
   169			}
   170			break;
   171		case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
   172		case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
   173			dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
   174							src_devid, NULL, NULL, true);
   175			dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
   176								BTRFS_DEV_REPLACE_DEVID,
   177								NULL, NULL, true);
   178			/*
   179			 * allow 'btrfs dev replace_cancel' if src/tgt device is
   180			 * missing
   181			 */
   182			if (!dev_replace->srcdev &&
   183			    !btrfs_test_opt(fs_info, DEGRADED)) {
   184				ret = -EIO;
   185				btrfs_warn(fs_info,
   186				   "cannot mount because device replace operation is ongoing and");
   187				btrfs_warn(fs_info,
   188				   "srcdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
   189				   src_devid);
   190			}
   191			if (!dev_replace->tgtdev &&
   192			    !btrfs_test_opt(fs_info, DEGRADED)) {
   193				ret = -EIO;
   194				btrfs_warn(fs_info,
   195				   "cannot mount because device replace operation is ongoing and");
   196				btrfs_warn(fs_info,
   197				   "tgtdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
   198					BTRFS_DEV_REPLACE_DEVID);
   199			}
   200			if (dev_replace->tgtdev) {
   201				if (dev_replace->srcdev) {
   202					dev_replace->tgtdev->total_bytes =
   203						dev_replace->srcdev->total_bytes;
   204					dev_replace->tgtdev->disk_total_bytes =
   205						dev_replace->srcdev->disk_total_bytes;
   206					dev_replace->tgtdev->commit_total_bytes =
   207						dev_replace->srcdev->commit_total_bytes;
   208					dev_replace->tgtdev->bytes_used =
   209						dev_replace->srcdev->bytes_used;
   210					dev_replace->tgtdev->commit_bytes_used =
   211						dev_replace->srcdev->commit_bytes_used;
   212				}
   213				set_bit(BTRFS_DEV_STATE_REPLACE_TGT,
   214					&dev_replace->tgtdev->dev_state);
   215	
   216				WARN_ON(fs_info->fs_devices->rw_devices == 0);
   217				dev_replace->tgtdev->io_width = fs_info->sectorsize;
   218				dev_replace->tgtdev->io_align = fs_info->sectorsize;
   219				dev_replace->tgtdev->sector_size = fs_info->sectorsize;
   220				dev_replace->tgtdev->fs_info = fs_info;
   221				set_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
   222					&dev_replace->tgtdev->dev_state);
   223			}
   224			break;
   225		}
   226	
   227	out:
   228		btrfs_free_path(path);
   229		return ret;
   230	}
   231	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 29957 bytes --]

[PATCH add reported by] btrfs: fix rw_devices count in __btrfs_free_extra_devids

[Anand Jain]

syzbot reported a warning [1] in close_fs_devcies() which it reproduces
using a crafted image.

        WARN_ON(fs_devices->rw_devices);

The crafted image successfully creates a replace-device with the devid 0.
But as there isn't any replace-item. We clean the extra the devid 0, at
__btrfs_free_extra_devids().

rw_devices is incremented in btrfs_open_one_device() for all write-able
devices except for devid == BTRFS_DEV_REPLACE_DEVID.
But while we clean up the extra devices in __btrfs_free_extra_devids()
we used the BTRFS_DEV_STATE_REPLACE_TGT flag which isn't set because
there isn't the replace-item. So rw_devices went below zero.

So let __btrfs_free_extra_devids() also depend on the
devid != BTRFS_DEV_REPLACE_DEVID to manage the rw_devices.

[1]
WARNING: CPU: 1 PID: 3612 at fs/btrfs/volumes.c:1166 close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 3612 Comm: syz-executor.2 Not tainted 5.9.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x198/0x1fd lib/dump_stack.c:118
 panic+0x347/0x7c0 kernel/panic.c:231
 __warn.cold+0x20/0x46 kernel/panic.c:600
 report_bug+0x1bd/0x210 lib/bug.c:198
 handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234
 exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254
 asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536
RIP: 0010:close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Code: 0f b6 04 02 84 c0 74 02 7e 33 48 8b 44 24 18 c6 80 30 01 00 00 00 48 83 c4 30 5b 5d 41 5c 41 5d 41 5e 41 5f c3 e8 99 ce 6a fe <0f> 0b e9 71 ff ff ff e8 8d ce 6a fe 0f 0b e9 20 ff ff ff e8 d1 d5
RSP: 0018:ffffc900091777e0 EFLAGS: 00010246
RAX: 0000000000040000 RBX: ffffffffffffffff RCX: ffffc9000c8b7000
RDX: 0000000000040000 RSI: ffffffff83097f47 RDI: 0000000000000007
RBP: dffffc0000000000 R08: 0000000000000001 R09: ffff8880988a187f
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88809593a130
R13: ffff88809593a1ec R14: ffff8880988a1908 R15: ffff88809593a050
 close_fs_devices fs/btrfs/volumes.c:1193 [inline]
 btrfs_close_devices+0x95/0x1f0 fs/btrfs/volumes.c:1179
 open_ctree+0x4984/0x4a2d fs/btrfs/disk-io.c:3434
 btrfs_fill_super fs/btrfs/super.c:1316 [inline]
 btrfs_mount_root.cold+0x14/0x165 fs/btrfs/super.c:1672
 legacy_get_tree+0x105/0x220 fs/fs_context.c:592
 vfs_get_tree+0x89/0x2f0 fs/super.c:1547
 fc_mount fs/namespace.c:978 [inline]
 vfs_kern_mount.part.0+0xd3/0x170 fs/namespace.c:1008
 vfs_kern_mount+0x3c/0x60 fs/namespace.c:995
 btrfs_mount+0x234/0xaa0 fs/btrfs/super.c:1732
 legacy_get_tree+0x105/0x220 fs/fs_context.c:592
 vfs_get_tree+0x89/0x2f0 fs/super.c:1547
 do_new_mount fs/namespace.c:2875 [inline]
 path_mount+0x1387/0x2070 fs/namespace.c:3192
 do_mount fs/namespace.c:3205 [inline]
 __do_sys_mount fs/namespace.c:3413 [inline]
 __se_sys_mount fs/namespace.c:3390 [inline]
 __x64_sys_mount+0x27f/0x300 fs/namespace.c:3390
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x46004a
Code: b8 a6 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 fd 89 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 0f 83 da 89 fb ff c3 66 0f 1f 84 00 00 00 00 00
RSP: 002b:00007f414d78da88 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007f414d78db20 RCX: 000000000046004a
RDX: 0000000020000000 RSI: 0000000020000100 RDI: 00007f414d78dae0
RBP: 00007f414d78dae0 R08: 00007f414d78db20 R09: 0000000020000000
R10: 0000000000000000 R11: 0000000000000202 R12: 0000000020000000
R13: 0000000020000100 R14: 0000000020000200 R15: 000000002001a800

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reported-by: syzbot+4cfe71a4da060be47502@syzkaller.appspotmail.com
---
 fs/btrfs/volumes.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ec9dac40b4f1..2fd73eab6219 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1080,8 +1080,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 		if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
 			list_del_init(&device->dev_alloc_list);
 			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
-			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-				      &device->dev_state))
+			if (device->devid != BTRFS_DEV_REPLACE_DEVID)
 				fs_devices->rw_devices--;
 		}
 		list_del_init(&device->dev_list);
-- 
2.25.1

[PATCH] btrfs: fix devid 0 without a replace item by failing the mount

[Anand Jain]

If there is a BTRFS_DEV_REPLACE_DEVID without a replace item, then
it means some device is trying to attack or may be corrupted. Fail the
mount so that the user can remove the attacking or fix the corrupted
device.

As of now if BTRFS_DEV_REPLACE_DEVID is present without the replace
item, then in __btrfs_free_extra_devids() we determine that there is an
extra device, and free those extra devices but continue to mount the
device.
However, we were wrong in keeping tack of the rw_devices so the syzbot
testcase failed as below [1].

[1]
WARNING: CPU: 1 PID: 3612 at fs/btrfs/volumes.c:1166 close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 3612 Comm: syz-executor.2 Not tainted 5.9.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x198/0x1fd lib/dump_stack.c:118
 panic+0x347/0x7c0 kernel/panic.c:231
 __warn.cold+0x20/0x46 kernel/panic.c:600
 report_bug+0x1bd/0x210 lib/bug.c:198
 handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234
 exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254
 asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536
RIP: 0010:close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Code: 0f b6 04 02 84 c0 74 02 7e 33 48 8b 44 24 18 c6 80 30 01 00 00 00 48 83 c4 30 5b 5d 41 5c 41 5d 41 5e 41 5f c3 e8 99 ce 6a fe <0f> 0b e9 71 ff ff ff e8 8d ce 6a fe 0f 0b e9 20 ff ff ff e8 d1 d5
RSP: 0018:ffffc900091777e0 EFLAGS: 00010246
RAX: 0000000000040000 RBX: ffffffffffffffff RCX: ffffc9000c8b7000
RDX: 0000000000040000 RSI: ffffffff83097f47 RDI: 0000000000000007
RBP: dffffc0000000000 R08: 0000000000000001 R09: ffff8880988a187f
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88809593a130
R13: ffff88809593a1ec R14: ffff8880988a1908 R15: ffff88809593a050
 close_fs_devices fs/btrfs/volumes.c:1193 [inline]
 btrfs_close_devices+0x95/0x1f0 fs/btrfs/volumes.c:1179
 open_ctree+0x4984/0x4a2d fs/btrfs/disk-io.c:3434
 btrfs_fill_super fs/btrfs/super.c:1316 [inline]
 btrfs_mount_root.cold+0x14/0x165 fs/btrfs/super.c:1672

The fix here is, when we determine that there isn't a replace item
then fail the mount if there is a replace target device (devid 0).

The reproducer looks like this.
 mkfs.btrfs -fq /dev/sda
 dd if=/dev/sda of=/dev/sdb bs=1 seek=64K skip=64K count=4096
 btrfs-sv-mod /dev/sdb devid=0
 mount -o device=/dev/sdb /dev/sda /btrfs

Reported-by: syzbot+4cfe71a4da060be47502@syzkaller.appspotmail.com
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
Depends on the patches
 btrfs: drop never met condition of disk_total_bytes == 0
 btrfs: fix btrfs_find_device unused arg seed
If these patches aren't integrated yet, then please add the last arg in
the function btrfs_find_device(). Any value is fine as it doesn't care.

fstest case will follow.

v2: changed title
    old: btrfs: fix rw_devices count in __btrfs_free_extra_devids

    In btrfs_init_dev_replace() try to match the presence of replace_item
    with the BTRFS_DEV_REPLACE_DEVID device. If fails then fail the
    mount. So drop the similar check in __btrfs_free_extra_devids().

 fs/btrfs/dev-replace.c | 26 ++++++++++++++++++++++++--
 fs/btrfs/volumes.c     | 26 +++++++-------------------
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 02a54177c0df..7a96380a9f1e 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -91,6 +91,17 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
 	if (ret) {
 no_valid_dev_replace_entry_found:
+		/*
+		 * We don't have a replace item or it's corrupted.
+		 * If there is a replace target, fail the mount.
+		 */
+		if (btrfs_find_device(fs_info->fs_devices,
+				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+			btrfs_err(fs_info,
+			"found replace target device without a replace item");
+			ret = -EIO;
+			goto out;
+		}
 		ret = 0;
 		dev_replace->replace_state =
 			BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
@@ -143,8 +154,19 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
-		dev_replace->srcdev = NULL;
-		dev_replace->tgtdev = NULL;
+		/*
+		 * We don't have an active replace item but if there is a
+		 * replace target, fail the mount.
+		 */
+		if (btrfs_find_device(fs_info->fs_devices,
+				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+			btrfs_err(fs_info,
+			"replace devid present without an active replace item");
+			ret = -EIO;
+		} else {
+			dev_replace->srcdev = NULL;
+			dev_replace->tgtdev = NULL;
+		}
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a88655d60a94..0c6049f9ace3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1056,22 +1056,13 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 			continue;
 		}
 
-		if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
-			/*
-			 * In the first step, keep the device which has
-			 * the correct fsid and the devid that is used
-			 * for the dev_replace procedure.
-			 * In the second step, the dev_replace state is
-			 * read from the device tree and it is known
-			 * whether the procedure is really active or
-			 * not, which means whether this device is
-			 * used or whether it should be removed.
-			 */
-			if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-						  &device->dev_state)) {
-				continue;
-			}
-		}
+		/*
+		 * We have already validated the presence of BTRFS_DEV_REPLACE_DEVID,
+		 * in btrfs_init_dev_replace() so just continue.
+		 */
+		if (device->devid == BTRFS_DEV_REPLACE_DEVID)
+			continue;
+
 		if (device->bdev) {
 			blkdev_put(device->bdev, device->mode);
 			device->bdev = NULL;
@@ -1080,9 +1071,6 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 		if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
 			list_del_init(&device->dev_alloc_list);
 			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
-			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-				      &device->dev_state))
-				fs_devices->rw_devices--;
 		}
 		list_del_init(&device->dev_list);
 		fs_devices->num_devices--;
-- 
2.25.1

[PATCH v2] btrfs: fix devid 0 without a replace item by failing the mount

[Anand Jain]

If there is a BTRFS_DEV_REPLACE_DEVID without a replace item, then
it means some device is trying to attack or may be corrupted. Fail the
mount so that the user can remove the attacking or fix the corrupted
device.

As of now if BTRFS_DEV_REPLACE_DEVID is present without the replace
item, then in __btrfs_free_extra_devids() we determine that there is an
extra device, and free those extra devices but continue to mount the
device.
However, we were wrong in keeping tack of the rw_devices so the syzbot
testcase failed as below [1].

[1]
WARNING: CPU: 1 PID: 3612 at fs/btrfs/volumes.c:1166 close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 3612 Comm: syz-executor.2 Not tainted 5.9.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x198/0x1fd lib/dump_stack.c:118
 panic+0x347/0x7c0 kernel/panic.c:231
 __warn.cold+0x20/0x46 kernel/panic.c:600
 report_bug+0x1bd/0x210 lib/bug.c:198
 handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234
 exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254
 asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536
RIP: 0010:close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Code: 0f b6 04 02 84 c0 74 02 7e 33 48 8b 44 24 18 c6 80 30 01 00 00 00 48 83 c4 30 5b 5d 41 5c 41 5d 41 5e 41 5f c3 e8 99 ce 6a fe <0f> 0b e9 71 ff ff ff e8 8d ce 6a fe 0f 0b e9 20 ff ff ff e8 d1 d5
RSP: 0018:ffffc900091777e0 EFLAGS: 00010246
RAX: 0000000000040000 RBX: ffffffffffffffff RCX: ffffc9000c8b7000
RDX: 0000000000040000 RSI: ffffffff83097f47 RDI: 0000000000000007
RBP: dffffc0000000000 R08: 0000000000000001 R09: ffff8880988a187f
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88809593a130
R13: ffff88809593a1ec R14: ffff8880988a1908 R15: ffff88809593a050
 close_fs_devices fs/btrfs/volumes.c:1193 [inline]
 btrfs_close_devices+0x95/0x1f0 fs/btrfs/volumes.c:1179
 open_ctree+0x4984/0x4a2d fs/btrfs/disk-io.c:3434
 btrfs_fill_super fs/btrfs/super.c:1316 [inline]
 btrfs_mount_root.cold+0x14/0x165 fs/btrfs/super.c:1672

The fix here is, when we determine that there isn't a replace item
then fail the mount if there is a replace target device (devid 0).

The reproducer looks like this.
 mkfs.btrfs -fq /dev/sda
 dd if=/dev/sda of=/dev/sdb bs=1 seek=64K skip=64K count=4096
 btrfs-sv-mod /dev/sdb devid=0
 mount -o device=/dev/sdb /dev/sda /btrfs

Reported-by: syzbot+4cfe71a4da060be47502@syzkaller.appspotmail.com
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
Depends on the patches
 btrfs: drop never met condition of disk_total_bytes == 0
 btrfs: fix btrfs_find_device unused arg seed
If these patches aren't integrated yet, then please add the last arg in
the function btrfs_find_device(). Any value is fine as it doesn't care.

fstest case will follow.

v2: changed title
    old: btrfs: fix rw_devices count in __btrfs_free_extra_devids

    In btrfs_init_dev_replace() try to match the presence of replace_item
    with the BTRFS_DEV_REPLACE_DEVID device. If fails then fail the
    mount. So drop the similar check in __btrfs_free_extra_devids().

 fs/btrfs/dev-replace.c | 26 ++++++++++++++++++++++++--
 fs/btrfs/volumes.c     | 26 +++++++-------------------
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 02a54177c0df..7a96380a9f1e 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -91,6 +91,17 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
 	if (ret) {
 no_valid_dev_replace_entry_found:
+		/*
+		 * We don't have a replace item or it's corrupted.
+		 * If there is a replace target, fail the mount.
+		 */
+		if (btrfs_find_device(fs_info->fs_devices,
+				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+			btrfs_err(fs_info,
+			"found replace target device without a replace item");
+			ret = -EIO;
+			goto out;
+		}
 		ret = 0;
 		dev_replace->replace_state =
 			BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
@@ -143,8 +154,19 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
-		dev_replace->srcdev = NULL;
-		dev_replace->tgtdev = NULL;
+		/*
+		 * We don't have an active replace item but if there is a
+		 * replace target, fail the mount.
+		 */
+		if (btrfs_find_device(fs_info->fs_devices,
+				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+			btrfs_err(fs_info,
+			"replace devid present without an active replace item");
+			ret = -EIO;
+		} else {
+			dev_replace->srcdev = NULL;
+			dev_replace->tgtdev = NULL;
+		}
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a88655d60a94..0c6049f9ace3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1056,22 +1056,13 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 			continue;
 		}
 
-		if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
-			/*
-			 * In the first step, keep the device which has
-			 * the correct fsid and the devid that is used
-			 * for the dev_replace procedure.
-			 * In the second step, the dev_replace state is
-			 * read from the device tree and it is known
-			 * whether the procedure is really active or
-			 * not, which means whether this device is
-			 * used or whether it should be removed.
-			 */
-			if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-						  &device->dev_state)) {
-				continue;
-			}
-		}
+		/*
+		 * We have already validated the presence of BTRFS_DEV_REPLACE_DEVID,
+		 * in btrfs_init_dev_replace() so just continue.
+		 */
+		if (device->devid == BTRFS_DEV_REPLACE_DEVID)
+			continue;
+
 		if (device->bdev) {
 			blkdev_put(device->bdev, device->mode);
 			device->bdev = NULL;
@@ -1080,9 +1071,6 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 		if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
 			list_del_init(&device->dev_alloc_list);
 			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
-			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-				      &device->dev_state))
-				fs_devices->rw_devices--;
 		}
 		list_del_init(&device->dev_list);
 		fs_devices->num_devices--;
-- 
2.25.1

Re: [PATCH] btrfs: fix devid 0 without a replace item by failing the mount

[kernel test robot]

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

Hi Anand,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on v5.9-rc8 next-20201006]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-randconfig-s001-20201005 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-201-g24bdaac6-dirty
        # https://github.com/0day-ci/linux/commit/ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
        git checkout ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/btrfs/dev-replace.c: In function 'btrfs_init_dev_replace':
>> fs/btrfs/dev-replace.c:98:7: error: too few arguments to function 'btrfs_find_device'
      98 |   if (btrfs_find_device(fs_info->fs_devices,
         |       ^~~~~~~~~~~~~~~~~
   In file included from fs/btrfs/dev-replace.c:18:
   fs/btrfs/volumes.h:455:22: note: declared here
     455 | struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
         |                      ^~~~~~~~~~~~~~~~~
   fs/btrfs/dev-replace.c:161:7: error: too few arguments to function 'btrfs_find_device'
     161 |   if (btrfs_find_device(fs_info->fs_devices,
         |       ^~~~~~~~~~~~~~~~~
   In file included from fs/btrfs/dev-replace.c:18:
   fs/btrfs/volumes.h:455:22: note: declared here
     455 | struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
         |                      ^~~~~~~~~~~~~~~~~

vim +/btrfs_find_device +98 fs/btrfs/dev-replace.c

    68	
    69	int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
    70	{
    71		struct btrfs_key key;
    72		struct btrfs_root *dev_root = fs_info->dev_root;
    73		struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
    74		struct extent_buffer *eb;
    75		int slot;
    76		int ret = 0;
    77		struct btrfs_path *path = NULL;
    78		int item_size;
    79		struct btrfs_dev_replace_item *ptr;
    80		u64 src_devid;
    81	
    82		path = btrfs_alloc_path();
    83		if (!path) {
    84			ret = -ENOMEM;
    85			goto out;
    86		}
    87	
    88		key.objectid = 0;
    89		key.type = BTRFS_DEV_REPLACE_KEY;
    90		key.offset = 0;
    91		ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
    92		if (ret) {
    93	no_valid_dev_replace_entry_found:
    94			/*
    95			 * We don't have a replace item or it's corrupted.
    96			 * If there is a replace target, fail the mount.
    97			 */
  > 98			if (btrfs_find_device(fs_info->fs_devices,
    99					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
   100				btrfs_err(fs_info,
   101				"found replace target device without a replace item");
   102				ret = -EIO;
   103				goto out;
   104			}
   105			ret = 0;
   106			dev_replace->replace_state =
   107				BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
   108			dev_replace->cont_reading_from_srcdev_mode =
   109			    BTRFS_DEV_REPLACE_ITEM_CONT_READING_FROM_SRCDEV_MODE_ALWAYS;
   110			dev_replace->time_started = 0;
   111			dev_replace->time_stopped = 0;
   112			atomic64_set(&dev_replace->num_write_errors, 0);
   113			atomic64_set(&dev_replace->num_uncorrectable_read_errors, 0);
   114			dev_replace->cursor_left = 0;
   115			dev_replace->committed_cursor_left = 0;
   116			dev_replace->cursor_left_last_write_of_item = 0;
   117			dev_replace->cursor_right = 0;
   118			dev_replace->srcdev = NULL;
   119			dev_replace->tgtdev = NULL;
   120			dev_replace->is_valid = 0;
   121			dev_replace->item_needs_writeback = 0;
   122			goto out;
   123		}
   124		slot = path->slots[0];
   125		eb = path->nodes[0];
   126		item_size = btrfs_item_size_nr(eb, slot);
   127		ptr = btrfs_item_ptr(eb, slot, struct btrfs_dev_replace_item);
   128	
   129		if (item_size != sizeof(struct btrfs_dev_replace_item)) {
   130			btrfs_warn(fs_info,
   131				"dev_replace entry found has unexpected size, ignore entry");
   132			goto no_valid_dev_replace_entry_found;
   133		}
   134	
   135		src_devid = btrfs_dev_replace_src_devid(eb, ptr);
   136		dev_replace->cont_reading_from_srcdev_mode =
   137			btrfs_dev_replace_cont_reading_from_srcdev_mode(eb, ptr);
   138		dev_replace->replace_state = btrfs_dev_replace_replace_state(eb, ptr);
   139		dev_replace->time_started = btrfs_dev_replace_time_started(eb, ptr);
   140		dev_replace->time_stopped =
   141			btrfs_dev_replace_time_stopped(eb, ptr);
   142		atomic64_set(&dev_replace->num_write_errors,
   143			     btrfs_dev_replace_num_write_errors(eb, ptr));
   144		atomic64_set(&dev_replace->num_uncorrectable_read_errors,
   145			     btrfs_dev_replace_num_uncorrectable_read_errors(eb, ptr));
   146		dev_replace->cursor_left = btrfs_dev_replace_cursor_left(eb, ptr);
   147		dev_replace->committed_cursor_left = dev_replace->cursor_left;
   148		dev_replace->cursor_left_last_write_of_item = dev_replace->cursor_left;
   149		dev_replace->cursor_right = btrfs_dev_replace_cursor_right(eb, ptr);
   150		dev_replace->is_valid = 1;
   151	
   152		dev_replace->item_needs_writeback = 0;
   153		switch (dev_replace->replace_state) {
   154		case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
   155		case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
   156		case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
   157			/*
   158			 * We don't have an active replace item but if there is a
   159			 * replace target, fail the mount.
   160			 */
   161			if (btrfs_find_device(fs_info->fs_devices,
   162					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
   163				btrfs_err(fs_info,
   164				"replace devid present without an active replace item");
   165				ret = -EIO;
   166			} else {
   167				dev_replace->srcdev = NULL;
   168				dev_replace->tgtdev = NULL;
   169			}
   170			break;
   171		case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
   172		case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
   173			dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
   174							src_devid, NULL, NULL, true);
   175			dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
   176								BTRFS_DEV_REPLACE_DEVID,
   177								NULL, NULL, true);
   178			/*
   179			 * allow 'btrfs dev replace_cancel' if src/tgt device is
   180			 * missing
   181			 */
   182			if (!dev_replace->srcdev &&
   183			    !btrfs_test_opt(fs_info, DEGRADED)) {
   184				ret = -EIO;
   185				btrfs_warn(fs_info,
   186				   "cannot mount because device replace operation is ongoing and");
   187				btrfs_warn(fs_info,
   188				   "srcdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
   189				   src_devid);
   190			}
   191			if (!dev_replace->tgtdev &&
   192			    !btrfs_test_opt(fs_info, DEGRADED)) {
   193				ret = -EIO;
   194				btrfs_warn(fs_info,
   195				   "cannot mount because device replace operation is ongoing and");
   196				btrfs_warn(fs_info,
   197				   "tgtdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
   198					BTRFS_DEV_REPLACE_DEVID);
   199			}
   200			if (dev_replace->tgtdev) {
   201				if (dev_replace->srcdev) {
   202					dev_replace->tgtdev->total_bytes =
   203						dev_replace->srcdev->total_bytes;
   204					dev_replace->tgtdev->disk_total_bytes =
   205						dev_replace->srcdev->disk_total_bytes;
   206					dev_replace->tgtdev->commit_total_bytes =
   207						dev_replace->srcdev->commit_total_bytes;
   208					dev_replace->tgtdev->bytes_used =
   209						dev_replace->srcdev->bytes_used;
   210					dev_replace->tgtdev->commit_bytes_used =
   211						dev_replace->srcdev->commit_bytes_used;
   212				}
   213				set_bit(BTRFS_DEV_STATE_REPLACE_TGT,
   214					&dev_replace->tgtdev->dev_state);
   215	
   216				WARN_ON(fs_info->fs_devices->rw_devices == 0);
   217				dev_replace->tgtdev->io_width = fs_info->sectorsize;
   218				dev_replace->tgtdev->io_align = fs_info->sectorsize;
   219				dev_replace->tgtdev->sector_size = fs_info->sectorsize;
   220				dev_replace->tgtdev->fs_info = fs_info;
   221				set_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
   222					&dev_replace->tgtdev->dev_state);
   223			}
   224			break;
   225		}
   226	
   227	out:
   228		btrfs_free_path(path);
   229		return ret;
   230	}
   231	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37641 bytes --]

Re: [PATCH] btrfs: fix devid 0 without a replace item by failing the mount

[kernel test robot]

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

Hi Anand,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on v5.9-rc8 next-20201006]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-randconfig-s001-20201005 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-201-g24bdaac6-dirty
        # https://github.com/0day-ci/linux/commit/ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
        git checkout ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/btrfs/dev-replace.c: In function 'btrfs_init_dev_replace':
>> fs/btrfs/dev-replace.c:98:7: error: too few arguments to function 'btrfs_find_device'
      98 |   if (btrfs_find_device(fs_info->fs_devices,
         |       ^~~~~~~~~~~~~~~~~
   In file included from fs/btrfs/dev-replace.c:18:
   fs/btrfs/volumes.h:455:22: note: declared here
     455 | struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
         |                      ^~~~~~~~~~~~~~~~~
   fs/btrfs/dev-replace.c:161:7: error: too few arguments to function 'btrfs_find_device'
     161 |   if (btrfs_find_device(fs_info->fs_devices,
         |       ^~~~~~~~~~~~~~~~~
   In file included from fs/btrfs/dev-replace.c:18:
   fs/btrfs/volumes.h:455:22: note: declared here
     455 | struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
         |                      ^~~~~~~~~~~~~~~~~

vim +/btrfs_find_device +98 fs/btrfs/dev-replace.c

    68	
    69	int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
    70	{
    71		struct btrfs_key key;
    72		struct btrfs_root *dev_root = fs_info->dev_root;
    73		struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
    74		struct extent_buffer *eb;
    75		int slot;
    76		int ret = 0;
    77		struct btrfs_path *path = NULL;
    78		int item_size;
    79		struct btrfs_dev_replace_item *ptr;
    80		u64 src_devid;
    81	
    82		path = btrfs_alloc_path();
    83		if (!path) {
    84			ret = -ENOMEM;
    85			goto out;
    86		}
    87	
    88		key.objectid = 0;
    89		key.type = BTRFS_DEV_REPLACE_KEY;
    90		key.offset = 0;
    91		ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
    92		if (ret) {
    93	no_valid_dev_replace_entry_found:
    94			/*
    95			 * We don't have a replace item or it's corrupted.
    96			 * If there is a replace target, fail the mount.
    97			 */
  > 98			if (btrfs_find_device(fs_info->fs_devices,
    99					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
   100				btrfs_err(fs_info,
   101				"found replace target device without a replace item");
   102				ret = -EIO;
   103				goto out;
   104			}
   105			ret = 0;
   106			dev_replace->replace_state =
   107				BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
   108			dev_replace->cont_reading_from_srcdev_mode =
   109			    BTRFS_DEV_REPLACE_ITEM_CONT_READING_FROM_SRCDEV_MODE_ALWAYS;
   110			dev_replace->time_started = 0;
   111			dev_replace->time_stopped = 0;
   112			atomic64_set(&dev_replace->num_write_errors, 0);
   113			atomic64_set(&dev_replace->num_uncorrectable_read_errors, 0);
   114			dev_replace->cursor_left = 0;
   115			dev_replace->committed_cursor_left = 0;
   116			dev_replace->cursor_left_last_write_of_item = 0;
   117			dev_replace->cursor_right = 0;
   118			dev_replace->srcdev = NULL;
   119			dev_replace->tgtdev = NULL;
   120			dev_replace->is_valid = 0;
   121			dev_replace->item_needs_writeback = 0;
   122			goto out;
   123		}
   124		slot = path->slots[0];
   125		eb = path->nodes[0];
   126		item_size = btrfs_item_size_nr(eb, slot);
   127		ptr = btrfs_item_ptr(eb, slot, struct btrfs_dev_replace_item);
   128	
   129		if (item_size != sizeof(struct btrfs_dev_replace_item)) {
   130			btrfs_warn(fs_info,
   131				"dev_replace entry found has unexpected size, ignore entry");
   132			goto no_valid_dev_replace_entry_found;
   133		}
   134	
   135		src_devid = btrfs_dev_replace_src_devid(eb, ptr);
   136		dev_replace->cont_reading_from_srcdev_mode =
   137			btrfs_dev_replace_cont_reading_from_srcdev_mode(eb, ptr);
   138		dev_replace->replace_state = btrfs_dev_replace_replace_state(eb, ptr);
   139		dev_replace->time_started = btrfs_dev_replace_time_started(eb, ptr);
   140		dev_replace->time_stopped =
   141			btrfs_dev_replace_time_stopped(eb, ptr);
   142		atomic64_set(&dev_replace->num_write_errors,
   143			     btrfs_dev_replace_num_write_errors(eb, ptr));
   144		atomic64_set(&dev_replace->num_uncorrectable_read_errors,
   145			     btrfs_dev_replace_num_uncorrectable_read_errors(eb, ptr));
   146		dev_replace->cursor_left = btrfs_dev_replace_cursor_left(eb, ptr);
   147		dev_replace->committed_cursor_left = dev_replace->cursor_left;
   148		dev_replace->cursor_left_last_write_of_item = dev_replace->cursor_left;
   149		dev_replace->cursor_right = btrfs_dev_replace_cursor_right(eb, ptr);
   150		dev_replace->is_valid = 1;
   151	
   152		dev_replace->item_needs_writeback = 0;
   153		switch (dev_replace->replace_state) {
   154		case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
   155		case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
   156		case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
   157			/*
   158			 * We don't have an active replace item but if there is a
   159			 * replace target, fail the mount.
   160			 */
   161			if (btrfs_find_device(fs_info->fs_devices,
   162					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
   163				btrfs_err(fs_info,
   164				"replace devid present without an active replace item");
   165				ret = -EIO;
   166			} else {
   167				dev_replace->srcdev = NULL;
   168				dev_replace->tgtdev = NULL;
   169			}
   170			break;
   171		case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
   172		case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
   173			dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
   174							src_devid, NULL, NULL, true);
   175			dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
   176								BTRFS_DEV_REPLACE_DEVID,
   177								NULL, NULL, true);
   178			/*
   179			 * allow 'btrfs dev replace_cancel' if src/tgt device is
   180			 * missing
   181			 */
   182			if (!dev_replace->srcdev &&
   183			    !btrfs_test_opt(fs_info, DEGRADED)) {
   184				ret = -EIO;
   185				btrfs_warn(fs_info,
   186				   "cannot mount because device replace operation is ongoing and");
   187				btrfs_warn(fs_info,
   188				   "srcdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
   189				   src_devid);
   190			}
   191			if (!dev_replace->tgtdev &&
   192			    !btrfs_test_opt(fs_info, DEGRADED)) {
   193				ret = -EIO;
   194				btrfs_warn(fs_info,
   195				   "cannot mount because device replace operation is ongoing and");
   196				btrfs_warn(fs_info,
   197				   "tgtdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
   198					BTRFS_DEV_REPLACE_DEVID);
   199			}
   200			if (dev_replace->tgtdev) {
   201				if (dev_replace->srcdev) {
   202					dev_replace->tgtdev->total_bytes =
   203						dev_replace->srcdev->total_bytes;
   204					dev_replace->tgtdev->disk_total_bytes =
   205						dev_replace->srcdev->disk_total_bytes;
   206					dev_replace->tgtdev->commit_total_bytes =
   207						dev_replace->srcdev->commit_total_bytes;
   208					dev_replace->tgtdev->bytes_used =
   209						dev_replace->srcdev->bytes_used;
   210					dev_replace->tgtdev->commit_bytes_used =
   211						dev_replace->srcdev->commit_bytes_used;
   212				}
   213				set_bit(BTRFS_DEV_STATE_REPLACE_TGT,
   214					&dev_replace->tgtdev->dev_state);
   215	
   216				WARN_ON(fs_info->fs_devices->rw_devices == 0);
   217				dev_replace->tgtdev->io_width = fs_info->sectorsize;
   218				dev_replace->tgtdev->io_align = fs_info->sectorsize;
   219				dev_replace->tgtdev->sector_size = fs_info->sectorsize;
   220				dev_replace->tgtdev->fs_info = fs_info;
   221				set_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
   222					&dev_replace->tgtdev->dev_state);
   223			}
   224			break;
   225		}
   226	
   227	out:
   228		btrfs_free_path(path);
   229		return ret;
   230	}
   231	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37641 bytes --]

Re: [PATCH] btrfs: fix devid 0 without a replace item by failing the mount

[Anand Jain]

On 6/10/20 10:54 pm, kernel test robot wrote:
> Hi Anand,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on kdave/for-next]
> [also build test ERROR on v5.9-rc8 next-20201006]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> config: i386-randconfig-s001-20201005 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> reproduce:
>          # apt-get install sparse
>          # sparse version: v0.6.2-201-g24bdaac6-dirty
>          # https://github.com/0day-ci/linux/commit/ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
>          git remote add linux-review https://github.com/0day-ci/linux
>          git fetch --no-tags linux-review Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
>          git checkout ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
>          # save the attached .config to linux build tree
>          make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>     fs/btrfs/dev-replace.c: In function 'btrfs_init_dev_replace':
>>> fs/btrfs/dev-replace.c:98:7: error: too few arguments to function 'btrfs_find_device'
>        98 |   if (btrfs_find_device(fs_info->fs_devices,
>           |       ^~~~~~~~~~~~~~~~~
>     In file included from fs/btrfs/dev-replace.c:18:
>     fs/btrfs/volumes.h:455:22: note: declared here
>       455 | struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
>           |                      ^~~~~~~~~~~~~~~~~
>     fs/btrfs/dev-replace.c:161:7: error: too few arguments to function 'btrfs_find_device'
>       161 |   if (btrfs_find_device(fs_info->fs_devices,
>           |       ^~~~~~~~~~~~~~~~~
>     In file included from fs/btrfs/dev-replace.c:18:
>     fs/btrfs/volumes.h:455:22: note: declared here
>       455 | struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
>           |                      ^~~~~~~~~~~~~~~~~
> 

  Is there is a way to mention the patch dependencies, so that 0-Day 
tests would understand. As in the patch's changelog, two dependent 
patches [1] aren't in the misc-next yet.

[1]
https://patchwork.kernel.org/patch/11818635
https://patchwork.kernel.org/patch/11796905

Thanks, Anand


> vim +/btrfs_find_device +98 fs/btrfs/dev-replace.c
> 
>      68	
>      69	int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
>      70	{
>      71		struct btrfs_key key;
>      72		struct btrfs_root *dev_root = fs_info->dev_root;
>      73		struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
>      74		struct extent_buffer *eb;
>      75		int slot;
>      76		int ret = 0;
>      77		struct btrfs_path *path = NULL;
>      78		int item_size;
>      79		struct btrfs_dev_replace_item *ptr;
>      80		u64 src_devid;
>      81	
>      82		path = btrfs_alloc_path();
>      83		if (!path) {
>      84			ret = -ENOMEM;
>      85			goto out;
>      86		}
>      87	
>      88		key.objectid = 0;
>      89		key.type = BTRFS_DEV_REPLACE_KEY;
>      90		key.offset = 0;
>      91		ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
>      92		if (ret) {
>      93	no_valid_dev_replace_entry_found:
>      94			/*
>      95			 * We don't have a replace item or it's corrupted.
>      96			 * If there is a replace target, fail the mount.
>      97			 */
>    > 98			if (btrfs_find_device(fs_info->fs_devices,
>      99					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
>     100				btrfs_err(fs_info,
>     101				"found replace target device without a replace item");
>     102				ret = -EIO;
>     103				goto out;
>     104			}
>     105			ret = 0;
>     106			dev_replace->replace_state =
>     107				BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
>     108			dev_replace->cont_reading_from_srcdev_mode =
>     109			    BTRFS_DEV_REPLACE_ITEM_CONT_READING_FROM_SRCDEV_MODE_ALWAYS;
>     110			dev_replace->time_started = 0;
>     111			dev_replace->time_stopped = 0;
>     112			atomic64_set(&dev_replace->num_write_errors, 0);
>     113			atomic64_set(&dev_replace->num_uncorrectable_read_errors, 0);
>     114			dev_replace->cursor_left = 0;
>     115			dev_replace->committed_cursor_left = 0;
>     116			dev_replace->cursor_left_last_write_of_item = 0;
>     117			dev_replace->cursor_right = 0;
>     118			dev_replace->srcdev = NULL;
>     119			dev_replace->tgtdev = NULL;
>     120			dev_replace->is_valid = 0;
>     121			dev_replace->item_needs_writeback = 0;
>     122			goto out;
>     123		}
>     124		slot = path->slots[0];
>     125		eb = path->nodes[0];
>     126		item_size = btrfs_item_size_nr(eb, slot);
>     127		ptr = btrfs_item_ptr(eb, slot, struct btrfs_dev_replace_item);
>     128	
>     129		if (item_size != sizeof(struct btrfs_dev_replace_item)) {
>     130			btrfs_warn(fs_info,
>     131				"dev_replace entry found has unexpected size, ignore entry");
>     132			goto no_valid_dev_replace_entry_found;
>     133		}
>     134	
>     135		src_devid = btrfs_dev_replace_src_devid(eb, ptr);
>     136		dev_replace->cont_reading_from_srcdev_mode =
>     137			btrfs_dev_replace_cont_reading_from_srcdev_mode(eb, ptr);
>     138		dev_replace->replace_state = btrfs_dev_replace_replace_state(eb, ptr);
>     139		dev_replace->time_started = btrfs_dev_replace_time_started(eb, ptr);
>     140		dev_replace->time_stopped =
>     141			btrfs_dev_replace_time_stopped(eb, ptr);
>     142		atomic64_set(&dev_replace->num_write_errors,
>     143			     btrfs_dev_replace_num_write_errors(eb, ptr));
>     144		atomic64_set(&dev_replace->num_uncorrectable_read_errors,
>     145			     btrfs_dev_replace_num_uncorrectable_read_errors(eb, ptr));
>     146		dev_replace->cursor_left = btrfs_dev_replace_cursor_left(eb, ptr);
>     147		dev_replace->committed_cursor_left = dev_replace->cursor_left;
>     148		dev_replace->cursor_left_last_write_of_item = dev_replace->cursor_left;
>     149		dev_replace->cursor_right = btrfs_dev_replace_cursor_right(eb, ptr);
>     150		dev_replace->is_valid = 1;
>     151	
>     152		dev_replace->item_needs_writeback = 0;
>     153		switch (dev_replace->replace_state) {
>     154		case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
>     155		case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
>     156		case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
>     157			/*
>     158			 * We don't have an active replace item but if there is a
>     159			 * replace target, fail the mount.
>     160			 */
>     161			if (btrfs_find_device(fs_info->fs_devices,
>     162					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
>     163				btrfs_err(fs_info,
>     164				"replace devid present without an active replace item");
>     165				ret = -EIO;
>     166			} else {
>     167				dev_replace->srcdev = NULL;
>     168				dev_replace->tgtdev = NULL;
>     169			}
>     170			break;
>     171		case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
>     172		case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
>     173			dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
>     174							src_devid, NULL, NULL, true);
>     175			dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
>     176								BTRFS_DEV_REPLACE_DEVID,
>     177								NULL, NULL, true);
>     178			/*
>     179			 * allow 'btrfs dev replace_cancel' if src/tgt device is
>     180			 * missing
>     181			 */
>     182			if (!dev_replace->srcdev &&
>     183			    !btrfs_test_opt(fs_info, DEGRADED)) {
>     184				ret = -EIO;
>     185				btrfs_warn(fs_info,
>     186				   "cannot mount because device replace operation is ongoing and");
>     187				btrfs_warn(fs_info,
>     188				   "srcdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
>     189				   src_devid);
>     190			}
>     191			if (!dev_replace->tgtdev &&
>     192			    !btrfs_test_opt(fs_info, DEGRADED)) {
>     193				ret = -EIO;
>     194				btrfs_warn(fs_info,
>     195				   "cannot mount because device replace operation is ongoing and");
>     196				btrfs_warn(fs_info,
>     197				   "tgtdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
>     198					BTRFS_DEV_REPLACE_DEVID);
>     199			}
>     200			if (dev_replace->tgtdev) {
>     201				if (dev_replace->srcdev) {
>     202					dev_replace->tgtdev->total_bytes =
>     203						dev_replace->srcdev->total_bytes;
>     204					dev_replace->tgtdev->disk_total_bytes =
>     205						dev_replace->srcdev->disk_total_bytes;
>     206					dev_replace->tgtdev->commit_total_bytes =
>     207						dev_replace->srcdev->commit_total_bytes;
>     208					dev_replace->tgtdev->bytes_used =
>     209						dev_replace->srcdev->bytes_used;
>     210					dev_replace->tgtdev->commit_bytes_used =
>     211						dev_replace->srcdev->commit_bytes_used;
>     212				}
>     213				set_bit(BTRFS_DEV_STATE_REPLACE_TGT,
>     214					&dev_replace->tgtdev->dev_state);
>     215	
>     216				WARN_ON(fs_info->fs_devices->rw_devices == 0);
>     217				dev_replace->tgtdev->io_width = fs_info->sectorsize;
>     218				dev_replace->tgtdev->io_align = fs_info->sectorsize;
>     219				dev_replace->tgtdev->sector_size = fs_info->sectorsize;
>     220				dev_replace->tgtdev->fs_info = fs_info;
>     221				set_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
>     222					&dev_replace->tgtdev->dev_state);
>     223			}
>     224			break;
>     225		}
>     226	
>     227	out:
>     228		btrfs_free_path(path);
>     229		return ret;
>     230	}
>     231	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 

Re: [PATCH] btrfs: fix devid 0 without a replace item by failing the mount

[Anand Jain]

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

On 6/10/20 10:54 pm, kernel test robot wrote:
> Hi Anand,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on kdave/for-next]
> [also build test ERROR on v5.9-rc8 next-20201006]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> config: i386-randconfig-s001-20201005 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> reproduce:
>          # apt-get install sparse
>          # sparse version: v0.6.2-201-g24bdaac6-dirty
>          # https://github.com/0day-ci/linux/commit/ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
>          git remote add linux-review https://github.com/0day-ci/linux
>          git fetch --no-tags linux-review Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
>          git checkout ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
>          # save the attached .config to linux build tree
>          make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>     fs/btrfs/dev-replace.c: In function 'btrfs_init_dev_replace':
>>> fs/btrfs/dev-replace.c:98:7: error: too few arguments to function 'btrfs_find_device'
>        98 |   if (btrfs_find_device(fs_info->fs_devices,
>           |       ^~~~~~~~~~~~~~~~~
>     In file included from fs/btrfs/dev-replace.c:18:
>     fs/btrfs/volumes.h:455:22: note: declared here
>       455 | struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
>           |                      ^~~~~~~~~~~~~~~~~
>     fs/btrfs/dev-replace.c:161:7: error: too few arguments to function 'btrfs_find_device'
>       161 |   if (btrfs_find_device(fs_info->fs_devices,
>           |       ^~~~~~~~~~~~~~~~~
>     In file included from fs/btrfs/dev-replace.c:18:
>     fs/btrfs/volumes.h:455:22: note: declared here
>       455 | struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
>           |                      ^~~~~~~~~~~~~~~~~
> 

  Is there is a way to mention the patch dependencies, so that 0-Day 
tests would understand. As in the patch's changelog, two dependent 
patches [1] aren't in the misc-next yet.

[1]
https://patchwork.kernel.org/patch/11818635
https://patchwork.kernel.org/patch/11796905

Thanks, Anand


> vim +/btrfs_find_device +98 fs/btrfs/dev-replace.c
> 
>      68	
>      69	int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
>      70	{
>      71		struct btrfs_key key;
>      72		struct btrfs_root *dev_root = fs_info->dev_root;
>      73		struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
>      74		struct extent_buffer *eb;
>      75		int slot;
>      76		int ret = 0;
>      77		struct btrfs_path *path = NULL;
>      78		int item_size;
>      79		struct btrfs_dev_replace_item *ptr;
>      80		u64 src_devid;
>      81	
>      82		path = btrfs_alloc_path();
>      83		if (!path) {
>      84			ret = -ENOMEM;
>      85			goto out;
>      86		}
>      87	
>      88		key.objectid = 0;
>      89		key.type = BTRFS_DEV_REPLACE_KEY;
>      90		key.offset = 0;
>      91		ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
>      92		if (ret) {
>      93	no_valid_dev_replace_entry_found:
>      94			/*
>      95			 * We don't have a replace item or it's corrupted.
>      96			 * If there is a replace target, fail the mount.
>      97			 */
>    > 98			if (btrfs_find_device(fs_info->fs_devices,
>      99					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
>     100				btrfs_err(fs_info,
>     101				"found replace target device without a replace item");
>     102				ret = -EIO;
>     103				goto out;
>     104			}
>     105			ret = 0;
>     106			dev_replace->replace_state =
>     107				BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
>     108			dev_replace->cont_reading_from_srcdev_mode =
>     109			    BTRFS_DEV_REPLACE_ITEM_CONT_READING_FROM_SRCDEV_MODE_ALWAYS;
>     110			dev_replace->time_started = 0;
>     111			dev_replace->time_stopped = 0;
>     112			atomic64_set(&dev_replace->num_write_errors, 0);
>     113			atomic64_set(&dev_replace->num_uncorrectable_read_errors, 0);
>     114			dev_replace->cursor_left = 0;
>     115			dev_replace->committed_cursor_left = 0;
>     116			dev_replace->cursor_left_last_write_of_item = 0;
>     117			dev_replace->cursor_right = 0;
>     118			dev_replace->srcdev = NULL;
>     119			dev_replace->tgtdev = NULL;
>     120			dev_replace->is_valid = 0;
>     121			dev_replace->item_needs_writeback = 0;
>     122			goto out;
>     123		}
>     124		slot = path->slots[0];
>     125		eb = path->nodes[0];
>     126		item_size = btrfs_item_size_nr(eb, slot);
>     127		ptr = btrfs_item_ptr(eb, slot, struct btrfs_dev_replace_item);
>     128	
>     129		if (item_size != sizeof(struct btrfs_dev_replace_item)) {
>     130			btrfs_warn(fs_info,
>     131				"dev_replace entry found has unexpected size, ignore entry");
>     132			goto no_valid_dev_replace_entry_found;
>     133		}
>     134	
>     135		src_devid = btrfs_dev_replace_src_devid(eb, ptr);
>     136		dev_replace->cont_reading_from_srcdev_mode =
>     137			btrfs_dev_replace_cont_reading_from_srcdev_mode(eb, ptr);
>     138		dev_replace->replace_state = btrfs_dev_replace_replace_state(eb, ptr);
>     139		dev_replace->time_started = btrfs_dev_replace_time_started(eb, ptr);
>     140		dev_replace->time_stopped =
>     141			btrfs_dev_replace_time_stopped(eb, ptr);
>     142		atomic64_set(&dev_replace->num_write_errors,
>     143			     btrfs_dev_replace_num_write_errors(eb, ptr));
>     144		atomic64_set(&dev_replace->num_uncorrectable_read_errors,
>     145			     btrfs_dev_replace_num_uncorrectable_read_errors(eb, ptr));
>     146		dev_replace->cursor_left = btrfs_dev_replace_cursor_left(eb, ptr);
>     147		dev_replace->committed_cursor_left = dev_replace->cursor_left;
>     148		dev_replace->cursor_left_last_write_of_item = dev_replace->cursor_left;
>     149		dev_replace->cursor_right = btrfs_dev_replace_cursor_right(eb, ptr);
>     150		dev_replace->is_valid = 1;
>     151	
>     152		dev_replace->item_needs_writeback = 0;
>     153		switch (dev_replace->replace_state) {
>     154		case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
>     155		case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
>     156		case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
>     157			/*
>     158			 * We don't have an active replace item but if there is a
>     159			 * replace target, fail the mount.
>     160			 */
>     161			if (btrfs_find_device(fs_info->fs_devices,
>     162					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
>     163				btrfs_err(fs_info,
>     164				"replace devid present without an active replace item");
>     165				ret = -EIO;
>     166			} else {
>     167				dev_replace->srcdev = NULL;
>     168				dev_replace->tgtdev = NULL;
>     169			}
>     170			break;
>     171		case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
>     172		case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
>     173			dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
>     174							src_devid, NULL, NULL, true);
>     175			dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
>     176								BTRFS_DEV_REPLACE_DEVID,
>     177								NULL, NULL, true);
>     178			/*
>     179			 * allow 'btrfs dev replace_cancel' if src/tgt device is
>     180			 * missing
>     181			 */
>     182			if (!dev_replace->srcdev &&
>     183			    !btrfs_test_opt(fs_info, DEGRADED)) {
>     184				ret = -EIO;
>     185				btrfs_warn(fs_info,
>     186				   "cannot mount because device replace operation is ongoing and");
>     187				btrfs_warn(fs_info,
>     188				   "srcdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
>     189				   src_devid);
>     190			}
>     191			if (!dev_replace->tgtdev &&
>     192			    !btrfs_test_opt(fs_info, DEGRADED)) {
>     193				ret = -EIO;
>     194				btrfs_warn(fs_info,
>     195				   "cannot mount because device replace operation is ongoing and");
>     196				btrfs_warn(fs_info,
>     197				   "tgtdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
>     198					BTRFS_DEV_REPLACE_DEVID);
>     199			}
>     200			if (dev_replace->tgtdev) {
>     201				if (dev_replace->srcdev) {
>     202					dev_replace->tgtdev->total_bytes =
>     203						dev_replace->srcdev->total_bytes;
>     204					dev_replace->tgtdev->disk_total_bytes =
>     205						dev_replace->srcdev->disk_total_bytes;
>     206					dev_replace->tgtdev->commit_total_bytes =
>     207						dev_replace->srcdev->commit_total_bytes;
>     208					dev_replace->tgtdev->bytes_used =
>     209						dev_replace->srcdev->bytes_used;
>     210					dev_replace->tgtdev->commit_bytes_used =
>     211						dev_replace->srcdev->commit_bytes_used;
>     212				}
>     213				set_bit(BTRFS_DEV_STATE_REPLACE_TGT,
>     214					&dev_replace->tgtdev->dev_state);
>     215	
>     216				WARN_ON(fs_info->fs_devices->rw_devices == 0);
>     217				dev_replace->tgtdev->io_width = fs_info->sectorsize;
>     218				dev_replace->tgtdev->io_align = fs_info->sectorsize;
>     219				dev_replace->tgtdev->sector_size = fs_info->sectorsize;
>     220				dev_replace->tgtdev->fs_info = fs_info;
>     221				set_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
>     222					&dev_replace->tgtdev->dev_state);
>     223			}
>     224			break;
>     225		}
>     226	
>     227	out:
>     228		btrfs_free_path(path);
>     229		return ret;
>     230	}
>     231	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
> 

Re: [kbuild-all] Re: [PATCH] btrfs: fix devid 0 without a replace item by failing the mount

[Rong Chen]

On 10/7/20 10:07 AM, Anand Jain wrote:
> On 6/10/20 10:54 pm, kernel test robot wrote:
>> Hi Anand,
>>
>> Thank you for the patch! Yet something to improve:
>>
>> [auto build test ERROR on kdave/for-next]
>> [also build test ERROR on v5.9-rc8 next-20201006]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url: 
>> https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
>> base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git 
>> for-next
>> config: i386-randconfig-s001-20201005 (attached as .config)
>> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>> reproduce:
>>          # apt-get install sparse
>>          # sparse version: v0.6.2-201-g24bdaac6-dirty
>>          # 
>> https://github.com/0day-ci/linux/commit/ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
>>          git remote add linux-review https://github.com/0day-ci/linux
>>          git fetch --no-tags linux-review 
>> Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
>>          git checkout ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
>>          # save the attached .config to linux build tree
>>          make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' 
>> ARCH=i386
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> All errors (new ones prefixed by >>):
>>
>>     fs/btrfs/dev-replace.c: In function 'btrfs_init_dev_replace':
>>>> fs/btrfs/dev-replace.c:98:7: error: too few arguments to function 
>>>> 'btrfs_find_device'
>>        98 |   if (btrfs_find_device(fs_info->fs_devices,
>>           |       ^~~~~~~~~~~~~~~~~
>>     In file included from fs/btrfs/dev-replace.c:18:
>>     fs/btrfs/volumes.h:455:22: note: declared here
>>       455 | struct btrfs_device *btrfs_find_device(struct 
>> btrfs_fs_devices *fs_devices,
>>           |                      ^~~~~~~~~~~~~~~~~
>>     fs/btrfs/dev-replace.c:161:7: error: too few arguments to 
>> function 'btrfs_find_device'
>>       161 |   if (btrfs_find_device(fs_info->fs_devices,
>>           |       ^~~~~~~~~~~~~~~~~
>>     In file included from fs/btrfs/dev-replace.c:18:
>>     fs/btrfs/volumes.h:455:22: note: declared here
>>       455 | struct btrfs_device *btrfs_find_device(struct 
>> btrfs_fs_devices *fs_devices,
>>           |                      ^~~~~~~~~~~~~~~~~
>>
>
>  Is there is a way to mention the patch dependencies, so that 0-Day 
> tests would understand. As in the patch's changelog, two dependent 
> patches [1] aren't in the misc-next yet.
>
> [1]
> https://patchwork.kernel.org/patch/11818635
> https://patchwork.kernel.org/patch/11796905

HI Anand,

The '--base' option in git format-patch may help, from man git format-patch:

BASE TREE INFORMATION
        The base tree information block is used for maintainers or third 
party testers to know the exact state the patch series applies to. It 
consists of the base commit, which is a well-known commit that is part 
of the stable part of the project history everybody else works
        off of, and zero or more prerequisite patches, which are 
well-known patches in flight that is not yet part of the base commit 
that need to be applied on top of base commit in topological order 
before the patches can be applied.

        The base commit is shown as "base-commit: " followed by the 
40-hex of the commit object name. A prerequisite patch is shown as 
"prerequisite-patch-id: " followed by the 40-hex patch id, which can be 
obtained by passing the patch through the git patch-id --stable
        command.

        Imagine that on top of the public commit P, you applied 
well-known patches X, Y and Z from somebody else, and then built your 
three-patch series A, B, C, the history would be like:

            ---P---X---Y---Z---A---B---C

        With git format-patch --base=P -3 C (or variants thereof, e.g. 
with --cover-letter or using Z..C instead of -3 C to specify the range), 
the base tree information block is shown at the end of the first message 
the command outputs (either the first patch, or the cover
        letter), like this:

            base-commit: P
            prerequisite-patch-id: X
            prerequisite-patch-id: Y
            prerequisite-patch-id: Z

        For non-linear topology, such as

            ---P---X---A---M---C
                \         /
                 Y---Z---B

        You can also use git format-patch --base=P -3 C to generate 
patches for A, B and C, and the identifiers for P, X, Y, Z are appended 
at the end of the first message.

        If set --base=auto in cmdline, it will track base commit 
automatically, the base commit will be the merge base of tip commit of 
the remote-tracking branch and revision-range specified in cmdline. For 
a local branch, you need to track a remote branch by git branch
        --set-upstream-to before using this option.

Best Regards,
Rong Chen

Re: [PATCH] btrfs: fix devid 0 without a replace item by failing the mount

[Rong Chen]

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



On 10/7/20 10:07 AM, Anand Jain wrote:
> On 6/10/20 10:54 pm, kernel test robot wrote:
>> Hi Anand,
>>
>> Thank you for the patch! Yet something to improve:
>>
>> [auto build test ERROR on kdave/for-next]
>> [also build test ERROR on v5.9-rc8 next-20201006]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url: 
>> https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
>> base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git 
>> for-next
>> config: i386-randconfig-s001-20201005 (attached as .config)
>> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>> reproduce:
>>          # apt-get install sparse
>>          # sparse version: v0.6.2-201-g24bdaac6-dirty
>>          # 
>> https://github.com/0day-ci/linux/commit/ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
>>          git remote add linux-review https://github.com/0day-ci/linux
>>          git fetch --no-tags linux-review 
>> Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
>>          git checkout ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
>>          # save the attached .config to linux build tree
>>          make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' 
>> ARCH=i386
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> All errors (new ones prefixed by >>):
>>
>>     fs/btrfs/dev-replace.c: In function 'btrfs_init_dev_replace':
>>>> fs/btrfs/dev-replace.c:98:7: error: too few arguments to function 
>>>> 'btrfs_find_device'
>>        98 |   if (btrfs_find_device(fs_info->fs_devices,
>>           |       ^~~~~~~~~~~~~~~~~
>>     In file included from fs/btrfs/dev-replace.c:18:
>>     fs/btrfs/volumes.h:455:22: note: declared here
>>       455 | struct btrfs_device *btrfs_find_device(struct 
>> btrfs_fs_devices *fs_devices,
>>           |                      ^~~~~~~~~~~~~~~~~
>>     fs/btrfs/dev-replace.c:161:7: error: too few arguments to 
>> function 'btrfs_find_device'
>>       161 |   if (btrfs_find_device(fs_info->fs_devices,
>>           |       ^~~~~~~~~~~~~~~~~
>>     In file included from fs/btrfs/dev-replace.c:18:
>>     fs/btrfs/volumes.h:455:22: note: declared here
>>       455 | struct btrfs_device *btrfs_find_device(struct 
>> btrfs_fs_devices *fs_devices,
>>           |                      ^~~~~~~~~~~~~~~~~
>>
>
>  Is there is a way to mention the patch dependencies, so that 0-Day 
> tests would understand. As in the patch's changelog, two dependent 
> patches [1] aren't in the misc-next yet.
>
> [1]
> https://patchwork.kernel.org/patch/11818635
> https://patchwork.kernel.org/patch/11796905

HI Anand,

The '--base' option in git format-patch may help, from man git format-patch:

BASE TREE INFORMATION
        The base tree information block is used for maintainers or third 
party testers to know the exact state the patch series applies to. It 
consists of the base commit, which is a well-known commit that is part 
of the stable part of the project history everybody else works
        off of, and zero or more prerequisite patches, which are 
well-known patches in flight that is not yet part of the base commit 
that need to be applied on top of base commit in topological order 
before the patches can be applied.

        The base commit is shown as "base-commit: " followed by the 
40-hex of the commit object name. A prerequisite patch is shown as 
"prerequisite-patch-id: " followed by the 40-hex patch id, which can be 
obtained by passing the patch through the git patch-id --stable
        command.

        Imagine that on top of the public commit P, you applied 
well-known patches X, Y and Z from somebody else, and then built your 
three-patch series A, B, C, the history would be like:

            ---P---X---Y---Z---A---B---C

        With git format-patch --base=P -3 C (or variants thereof, e.g. 
with --cover-letter or using Z..C instead of -3 C to specify the range), 
the base tree information block is shown at the end of the first message 
the command outputs (either the first patch, or the cover
        letter), like this:

            base-commit: P
            prerequisite-patch-id: X
            prerequisite-patch-id: Y
            prerequisite-patch-id: Z

        For non-linear topology, such as

            ---P---X---A---M---C
                \         /
                 Y---Z---B

        You can also use git format-patch --base=P -3 C to generate 
patches for A, B and C, and the identifiers for P, X, Y, Z are appended 
at the end of the first message.

        If set --base=auto in cmdline, it will track base commit 
automatically, the base commit will be the merge base of tip commit of 
the remote-tracking branch and revision-range specified in cmdline. For 
a local branch, you need to track a remote branch by git branch
        --set-upstream-to before using this option.

Best Regards,
Rong Chen

Re: [PATCH] btrfs: fix devid 0 without a replace item by failing the mount

[kernel test robot]

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

Hi Anand,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on v5.9-rc8 next-20201006]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: x86_64-randconfig-r006-20201005 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 1127662c6dc2a276839c75a42238b11a3ad00f32)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
        git checkout ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> fs/btrfs/dev-replace.c:99:46: error: too few arguments to function call, expected 5, have 4
                                         BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
                                                                            ^
   fs/btrfs/volumes.h:455:22: note: 'btrfs_find_device' declared here
   struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
                        ^
   fs/btrfs/dev-replace.c:162:46: error: too few arguments to function call, expected 5, have 4
                                         BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
                                                                            ^
   fs/btrfs/volumes.h:455:22: note: 'btrfs_find_device' declared here
   struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
                        ^
   2 errors generated.

vim +99 fs/btrfs/dev-replace.c

    68	
    69	int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
    70	{
    71		struct btrfs_key key;
    72		struct btrfs_root *dev_root = fs_info->dev_root;
    73		struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
    74		struct extent_buffer *eb;
    75		int slot;
    76		int ret = 0;
    77		struct btrfs_path *path = NULL;
    78		int item_size;
    79		struct btrfs_dev_replace_item *ptr;
    80		u64 src_devid;
    81	
    82		path = btrfs_alloc_path();
    83		if (!path) {
    84			ret = -ENOMEM;
    85			goto out;
    86		}
    87	
    88		key.objectid = 0;
    89		key.type = BTRFS_DEV_REPLACE_KEY;
    90		key.offset = 0;
    91		ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
    92		if (ret) {
    93	no_valid_dev_replace_entry_found:
    94			/*
    95			 * We don't have a replace item or it's corrupted.
    96			 * If there is a replace target, fail the mount.
    97			 */
    98			if (btrfs_find_device(fs_info->fs_devices,
  > 99					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
   100				btrfs_err(fs_info,
   101				"found replace target device without a replace item");
   102				ret = -EIO;
   103				goto out;
   104			}
   105			ret = 0;
   106			dev_replace->replace_state =
   107				BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
   108			dev_replace->cont_reading_from_srcdev_mode =
   109			    BTRFS_DEV_REPLACE_ITEM_CONT_READING_FROM_SRCDEV_MODE_ALWAYS;
   110			dev_replace->time_started = 0;
   111			dev_replace->time_stopped = 0;
   112			atomic64_set(&dev_replace->num_write_errors, 0);
   113			atomic64_set(&dev_replace->num_uncorrectable_read_errors, 0);
   114			dev_replace->cursor_left = 0;
   115			dev_replace->committed_cursor_left = 0;
   116			dev_replace->cursor_left_last_write_of_item = 0;
   117			dev_replace->cursor_right = 0;
   118			dev_replace->srcdev = NULL;
   119			dev_replace->tgtdev = NULL;
   120			dev_replace->is_valid = 0;
   121			dev_replace->item_needs_writeback = 0;
   122			goto out;
   123		}
   124		slot = path->slots[0];
   125		eb = path->nodes[0];
   126		item_size = btrfs_item_size_nr(eb, slot);
   127		ptr = btrfs_item_ptr(eb, slot, struct btrfs_dev_replace_item);
   128	
   129		if (item_size != sizeof(struct btrfs_dev_replace_item)) {
   130			btrfs_warn(fs_info,
   131				"dev_replace entry found has unexpected size, ignore entry");
   132			goto no_valid_dev_replace_entry_found;
   133		}
   134	
   135		src_devid = btrfs_dev_replace_src_devid(eb, ptr);
   136		dev_replace->cont_reading_from_srcdev_mode =
   137			btrfs_dev_replace_cont_reading_from_srcdev_mode(eb, ptr);
   138		dev_replace->replace_state = btrfs_dev_replace_replace_state(eb, ptr);
   139		dev_replace->time_started = btrfs_dev_replace_time_started(eb, ptr);
   140		dev_replace->time_stopped =
   141			btrfs_dev_replace_time_stopped(eb, ptr);
   142		atomic64_set(&dev_replace->num_write_errors,
   143			     btrfs_dev_replace_num_write_errors(eb, ptr));
   144		atomic64_set(&dev_replace->num_uncorrectable_read_errors,
   145			     btrfs_dev_replace_num_uncorrectable_read_errors(eb, ptr));
   146		dev_replace->cursor_left = btrfs_dev_replace_cursor_left(eb, ptr);
   147		dev_replace->committed_cursor_left = dev_replace->cursor_left;
   148		dev_replace->cursor_left_last_write_of_item = dev_replace->cursor_left;
   149		dev_replace->cursor_right = btrfs_dev_replace_cursor_right(eb, ptr);
   150		dev_replace->is_valid = 1;
   151	
   152		dev_replace->item_needs_writeback = 0;
   153		switch (dev_replace->replace_state) {
   154		case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
   155		case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
   156		case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
   157			/*
   158			 * We don't have an active replace item but if there is a
   159			 * replace target, fail the mount.
   160			 */
   161			if (btrfs_find_device(fs_info->fs_devices,
   162					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
   163				btrfs_err(fs_info,
   164				"replace devid present without an active replace item");
   165				ret = -EIO;
   166			} else {
   167				dev_replace->srcdev = NULL;
   168				dev_replace->tgtdev = NULL;
   169			}
   170			break;
   171		case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
   172		case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
   173			dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
   174							src_devid, NULL, NULL, true);
   175			dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
   176								BTRFS_DEV_REPLACE_DEVID,
   177								NULL, NULL, true);
   178			/*
   179			 * allow 'btrfs dev replace_cancel' if src/tgt device is
   180			 * missing
   181			 */
   182			if (!dev_replace->srcdev &&
   183			    !btrfs_test_opt(fs_info, DEGRADED)) {
   184				ret = -EIO;
   185				btrfs_warn(fs_info,
   186				   "cannot mount because device replace operation is ongoing and");
   187				btrfs_warn(fs_info,
   188				   "srcdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
   189				   src_devid);
   190			}
   191			if (!dev_replace->tgtdev &&
   192			    !btrfs_test_opt(fs_info, DEGRADED)) {
   193				ret = -EIO;
   194				btrfs_warn(fs_info,
   195				   "cannot mount because device replace operation is ongoing and");
   196				btrfs_warn(fs_info,
   197				   "tgtdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
   198					BTRFS_DEV_REPLACE_DEVID);
   199			}
   200			if (dev_replace->tgtdev) {
   201				if (dev_replace->srcdev) {
   202					dev_replace->tgtdev->total_bytes =
   203						dev_replace->srcdev->total_bytes;
   204					dev_replace->tgtdev->disk_total_bytes =
   205						dev_replace->srcdev->disk_total_bytes;
   206					dev_replace->tgtdev->commit_total_bytes =
   207						dev_replace->srcdev->commit_total_bytes;
   208					dev_replace->tgtdev->bytes_used =
   209						dev_replace->srcdev->bytes_used;
   210					dev_replace->tgtdev->commit_bytes_used =
   211						dev_replace->srcdev->commit_bytes_used;
   212				}
   213				set_bit(BTRFS_DEV_STATE_REPLACE_TGT,
   214					&dev_replace->tgtdev->dev_state);
   215	
   216				WARN_ON(fs_info->fs_devices->rw_devices == 0);
   217				dev_replace->tgtdev->io_width = fs_info->sectorsize;
   218				dev_replace->tgtdev->io_align = fs_info->sectorsize;
   219				dev_replace->tgtdev->sector_size = fs_info->sectorsize;
   220				dev_replace->tgtdev->fs_info = fs_info;
   221				set_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
   222					&dev_replace->tgtdev->dev_state);
   223			}
   224			break;
   225		}
   226	
   227	out:
   228		btrfs_free_path(path);
   229		return ret;
   230	}
   231	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36313 bytes --]

Re: [PATCH] btrfs: fix devid 0 without a replace item by failing the mount

[kernel test robot]

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

Hi Anand,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on v5.9-rc8 next-20201006]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: x86_64-randconfig-r006-20201005 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 1127662c6dc2a276839c75a42238b11a3ad00f32)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anand-Jain/btrfs-fix-devid-0-without-a-replace-item-by-failing-the-mount/20201006-210957
        git checkout ed4ebb4eb3f213f048ea5f6a2ed80f6bd728c9e1
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> fs/btrfs/dev-replace.c:99:46: error: too few arguments to function call, expected 5, have 4
                                         BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
                                                                            ^
   fs/btrfs/volumes.h:455:22: note: 'btrfs_find_device' declared here
   struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
                        ^
   fs/btrfs/dev-replace.c:162:46: error: too few arguments to function call, expected 5, have 4
                                         BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
                                                                            ^
   fs/btrfs/volumes.h:455:22: note: 'btrfs_find_device' declared here
   struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
                        ^
   2 errors generated.

vim +99 fs/btrfs/dev-replace.c

    68	
    69	int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
    70	{
    71		struct btrfs_key key;
    72		struct btrfs_root *dev_root = fs_info->dev_root;
    73		struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
    74		struct extent_buffer *eb;
    75		int slot;
    76		int ret = 0;
    77		struct btrfs_path *path = NULL;
    78		int item_size;
    79		struct btrfs_dev_replace_item *ptr;
    80		u64 src_devid;
    81	
    82		path = btrfs_alloc_path();
    83		if (!path) {
    84			ret = -ENOMEM;
    85			goto out;
    86		}
    87	
    88		key.objectid = 0;
    89		key.type = BTRFS_DEV_REPLACE_KEY;
    90		key.offset = 0;
    91		ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
    92		if (ret) {
    93	no_valid_dev_replace_entry_found:
    94			/*
    95			 * We don't have a replace item or it's corrupted.
    96			 * If there is a replace target, fail the mount.
    97			 */
    98			if (btrfs_find_device(fs_info->fs_devices,
  > 99					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
   100				btrfs_err(fs_info,
   101				"found replace target device without a replace item");
   102				ret = -EIO;
   103				goto out;
   104			}
   105			ret = 0;
   106			dev_replace->replace_state =
   107				BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
   108			dev_replace->cont_reading_from_srcdev_mode =
   109			    BTRFS_DEV_REPLACE_ITEM_CONT_READING_FROM_SRCDEV_MODE_ALWAYS;
   110			dev_replace->time_started = 0;
   111			dev_replace->time_stopped = 0;
   112			atomic64_set(&dev_replace->num_write_errors, 0);
   113			atomic64_set(&dev_replace->num_uncorrectable_read_errors, 0);
   114			dev_replace->cursor_left = 0;
   115			dev_replace->committed_cursor_left = 0;
   116			dev_replace->cursor_left_last_write_of_item = 0;
   117			dev_replace->cursor_right = 0;
   118			dev_replace->srcdev = NULL;
   119			dev_replace->tgtdev = NULL;
   120			dev_replace->is_valid = 0;
   121			dev_replace->item_needs_writeback = 0;
   122			goto out;
   123		}
   124		slot = path->slots[0];
   125		eb = path->nodes[0];
   126		item_size = btrfs_item_size_nr(eb, slot);
   127		ptr = btrfs_item_ptr(eb, slot, struct btrfs_dev_replace_item);
   128	
   129		if (item_size != sizeof(struct btrfs_dev_replace_item)) {
   130			btrfs_warn(fs_info,
   131				"dev_replace entry found has unexpected size, ignore entry");
   132			goto no_valid_dev_replace_entry_found;
   133		}
   134	
   135		src_devid = btrfs_dev_replace_src_devid(eb, ptr);
   136		dev_replace->cont_reading_from_srcdev_mode =
   137			btrfs_dev_replace_cont_reading_from_srcdev_mode(eb, ptr);
   138		dev_replace->replace_state = btrfs_dev_replace_replace_state(eb, ptr);
   139		dev_replace->time_started = btrfs_dev_replace_time_started(eb, ptr);
   140		dev_replace->time_stopped =
   141			btrfs_dev_replace_time_stopped(eb, ptr);
   142		atomic64_set(&dev_replace->num_write_errors,
   143			     btrfs_dev_replace_num_write_errors(eb, ptr));
   144		atomic64_set(&dev_replace->num_uncorrectable_read_errors,
   145			     btrfs_dev_replace_num_uncorrectable_read_errors(eb, ptr));
   146		dev_replace->cursor_left = btrfs_dev_replace_cursor_left(eb, ptr);
   147		dev_replace->committed_cursor_left = dev_replace->cursor_left;
   148		dev_replace->cursor_left_last_write_of_item = dev_replace->cursor_left;
   149		dev_replace->cursor_right = btrfs_dev_replace_cursor_right(eb, ptr);
   150		dev_replace->is_valid = 1;
   151	
   152		dev_replace->item_needs_writeback = 0;
   153		switch (dev_replace->replace_state) {
   154		case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
   155		case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
   156		case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
   157			/*
   158			 * We don't have an active replace item but if there is a
   159			 * replace target, fail the mount.
   160			 */
   161			if (btrfs_find_device(fs_info->fs_devices,
   162					      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
   163				btrfs_err(fs_info,
   164				"replace devid present without an active replace item");
   165				ret = -EIO;
   166			} else {
   167				dev_replace->srcdev = NULL;
   168				dev_replace->tgtdev = NULL;
   169			}
   170			break;
   171		case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
   172		case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
   173			dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
   174							src_devid, NULL, NULL, true);
   175			dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
   176								BTRFS_DEV_REPLACE_DEVID,
   177								NULL, NULL, true);
   178			/*
   179			 * allow 'btrfs dev replace_cancel' if src/tgt device is
   180			 * missing
   181			 */
   182			if (!dev_replace->srcdev &&
   183			    !btrfs_test_opt(fs_info, DEGRADED)) {
   184				ret = -EIO;
   185				btrfs_warn(fs_info,
   186				   "cannot mount because device replace operation is ongoing and");
   187				btrfs_warn(fs_info,
   188				   "srcdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
   189				   src_devid);
   190			}
   191			if (!dev_replace->tgtdev &&
   192			    !btrfs_test_opt(fs_info, DEGRADED)) {
   193				ret = -EIO;
   194				btrfs_warn(fs_info,
   195				   "cannot mount because device replace operation is ongoing and");
   196				btrfs_warn(fs_info,
   197				   "tgtdev (devid %llu) is missing, need to run 'btrfs dev scan'?",
   198					BTRFS_DEV_REPLACE_DEVID);
   199			}
   200			if (dev_replace->tgtdev) {
   201				if (dev_replace->srcdev) {
   202					dev_replace->tgtdev->total_bytes =
   203						dev_replace->srcdev->total_bytes;
   204					dev_replace->tgtdev->disk_total_bytes =
   205						dev_replace->srcdev->disk_total_bytes;
   206					dev_replace->tgtdev->commit_total_bytes =
   207						dev_replace->srcdev->commit_total_bytes;
   208					dev_replace->tgtdev->bytes_used =
   209						dev_replace->srcdev->bytes_used;
   210					dev_replace->tgtdev->commit_bytes_used =
   211						dev_replace->srcdev->commit_bytes_used;
   212				}
   213				set_bit(BTRFS_DEV_STATE_REPLACE_TGT,
   214					&dev_replace->tgtdev->dev_state);
   215	
   216				WARN_ON(fs_info->fs_devices->rw_devices == 0);
   217				dev_replace->tgtdev->io_width = fs_info->sectorsize;
   218				dev_replace->tgtdev->io_align = fs_info->sectorsize;
   219				dev_replace->tgtdev->sector_size = fs_info->sectorsize;
   220				dev_replace->tgtdev->fs_info = fs_info;
   221				set_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
   222					&dev_replace->tgtdev->dev_state);
   223			}
   224			break;
   225		}
   226	
   227	out:
   228		btrfs_free_path(path);
   229		return ret;
   230	}
   231	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 36313 bytes --]

[PATCH] btrfs: fix rw_devices count in __btrfs_free_extra_devids

[Anand Jain]

syzbot reported a warning [1] in close_fs_devcies() which it reproduces
using a crafted image.

        WARN_ON(fs_devices->rw_devices);

The crafted image successfully creates a replace-device with the devid 0.
But as there isn't any replace-item. We clean the extra the devid 0, at
__btrfs_free_extra_devids().

rw_devices is incremented in btrfs_open_one_device() for all write-able
devices except for devid == BTRFS_DEV_REPLACE_DEVID.
But while we clean up the extra devices in __btrfs_free_extra_devids()
we used the BTRFS_DEV_STATE_REPLACE_TGT flag which isn't set because
there isn't the replace-item. So rw_devices went below zero.

So let __btrfs_free_extra_devids() also depend on the
devid != BTRFS_DEV_REPLACE_DEVID to manage the rw_devices.

[1]
WARNING: CPU: 1 PID: 3612 at fs/btrfs/volumes.c:1166 close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 3612 Comm: syz-executor.2 Not tainted 5.9.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x198/0x1fd lib/dump_stack.c:118
 panic+0x347/0x7c0 kernel/panic.c:231
 __warn.cold+0x20/0x46 kernel/panic.c:600
 report_bug+0x1bd/0x210 lib/bug.c:198
 handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234
 exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254
 asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536
RIP: 0010:close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Code: 0f b6 04 02 84 c0 74 02 7e 33 48 8b 44 24 18 c6 80 30 01 00 00 00 48 83 c4 30 5b 5d 41 5c 41 5d 41 5e 41 5f c3 e8 99 ce 6a fe <0f> 0b e9 71 ff ff ff e8 8d ce 6a fe 0f 0b e9 20 ff ff ff e8 d1 d5
RSP: 0018:ffffc900091777e0 EFLAGS: 00010246
RAX: 0000000000040000 RBX: ffffffffffffffff RCX: ffffc9000c8b7000
RDX: 0000000000040000 RSI: ffffffff83097f47 RDI: 0000000000000007
RBP: dffffc0000000000 R08: 0000000000000001 R09: ffff8880988a187f
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88809593a130
R13: ffff88809593a1ec R14: ffff8880988a1908 R15: ffff88809593a050
 close_fs_devices fs/btrfs/volumes.c:1193 [inline]
 btrfs_close_devices+0x95/0x1f0 fs/btrfs/volumes.c:1179
 open_ctree+0x4984/0x4a2d fs/btrfs/disk-io.c:3434
 btrfs_fill_super fs/btrfs/super.c:1316 [inline]
 btrfs_mount_root.cold+0x14/0x165 fs/btrfs/super.c:1672
 legacy_get_tree+0x105/0x220 fs/fs_context.c:592
 vfs_get_tree+0x89/0x2f0 fs/super.c:1547
 fc_mount fs/namespace.c:978 [inline]
 vfs_kern_mount.part.0+0xd3/0x170 fs/namespace.c:1008
 vfs_kern_mount+0x3c/0x60 fs/namespace.c:995
 btrfs_mount+0x234/0xaa0 fs/btrfs/super.c:1732
 legacy_get_tree+0x105/0x220 fs/fs_context.c:592
 vfs_get_tree+0x89/0x2f0 fs/super.c:1547
 do_new_mount fs/namespace.c:2875 [inline]
 path_mount+0x1387/0x2070 fs/namespace.c:3192
 do_mount fs/namespace.c:3205 [inline]
 __do_sys_mount fs/namespace.c:3413 [inline]
 __se_sys_mount fs/namespace.c:3390 [inline]
 __x64_sys_mount+0x27f/0x300 fs/namespace.c:3390
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x46004a
Code: b8 a6 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 fd 89 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 0f 83 da 89 fb ff c3 66 0f 1f 84 00 00 00 00 00
RSP: 002b:00007f414d78da88 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007f414d78db20 RCX: 000000000046004a
RDX: 0000000020000000 RSI: 0000000020000100 RDI: 00007f414d78dae0
RBP: 00007f414d78dae0 R08: 00007f414d78db20 R09: 0000000020000000
R10: 0000000000000000 R11: 0000000000000202 R12: 0000000020000000
R13: 0000000020000100 R14: 0000000020000200 R15: 000000002001a800

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ec9dac40b4f1..2fd73eab6219 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1080,8 +1080,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 		if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
 			list_del_init(&device->dev_alloc_list);
 			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
-			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-				      &device->dev_state))
+			if (device->devid != BTRFS_DEV_REPLACE_DEVID)
 				fs_devices->rw_devices--;
 		}
 		list_del_init(&device->dev_list);
-- 
2.25.1

[PATCH add reported by] btrfs: fix rw_devices count in __btrfs_free_extra_devids

[Anand Jain]

syzbot reported a warning [1] in close_fs_devcies() which it reproduces
using a crafted image.

        WARN_ON(fs_devices->rw_devices);

The crafted image successfully creates a replace-device with the devid 0.
But as there isn't any replace-item. We clean the extra the devid 0, at
__btrfs_free_extra_devids().

rw_devices is incremented in btrfs_open_one_device() for all write-able
devices except for devid == BTRFS_DEV_REPLACE_DEVID.
But while we clean up the extra devices in __btrfs_free_extra_devids()
we used the BTRFS_DEV_STATE_REPLACE_TGT flag which isn't set because
there isn't the replace-item. So rw_devices went below zero.

So let __btrfs_free_extra_devids() also depend on the
devid != BTRFS_DEV_REPLACE_DEVID to manage the rw_devices.

[1]
WARNING: CPU: 1 PID: 3612 at fs/btrfs/volumes.c:1166 close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 3612 Comm: syz-executor.2 Not tainted 5.9.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x198/0x1fd lib/dump_stack.c:118
 panic+0x347/0x7c0 kernel/panic.c:231
 __warn.cold+0x20/0x46 kernel/panic.c:600
 report_bug+0x1bd/0x210 lib/bug.c:198
 handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234
 exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254
 asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536
RIP: 0010:close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Code: 0f b6 04 02 84 c0 74 02 7e 33 48 8b 44 24 18 c6 80 30 01 00 00 00 48 83 c4 30 5b 5d 41 5c 41 5d 41 5e 41 5f c3 e8 99 ce 6a fe <0f> 0b e9 71 ff ff ff e8 8d ce 6a fe 0f 0b e9 20 ff ff ff e8 d1 d5
RSP: 0018:ffffc900091777e0 EFLAGS: 00010246
RAX: 0000000000040000 RBX: ffffffffffffffff RCX: ffffc9000c8b7000
RDX: 0000000000040000 RSI: ffffffff83097f47 RDI: 0000000000000007
RBP: dffffc0000000000 R08: 0000000000000001 R09: ffff8880988a187f
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88809593a130
R13: ffff88809593a1ec R14: ffff8880988a1908 R15: ffff88809593a050
 close_fs_devices fs/btrfs/volumes.c:1193 [inline]
 btrfs_close_devices+0x95/0x1f0 fs/btrfs/volumes.c:1179
 open_ctree+0x4984/0x4a2d fs/btrfs/disk-io.c:3434
 btrfs_fill_super fs/btrfs/super.c:1316 [inline]
 btrfs_mount_root.cold+0x14/0x165 fs/btrfs/super.c:1672
 legacy_get_tree+0x105/0x220 fs/fs_context.c:592
 vfs_get_tree+0x89/0x2f0 fs/super.c:1547
 fc_mount fs/namespace.c:978 [inline]
 vfs_kern_mount.part.0+0xd3/0x170 fs/namespace.c:1008
 vfs_kern_mount+0x3c/0x60 fs/namespace.c:995
 btrfs_mount+0x234/0xaa0 fs/btrfs/super.c:1732
 legacy_get_tree+0x105/0x220 fs/fs_context.c:592
 vfs_get_tree+0x89/0x2f0 fs/super.c:1547
 do_new_mount fs/namespace.c:2875 [inline]
 path_mount+0x1387/0x2070 fs/namespace.c:3192
 do_mount fs/namespace.c:3205 [inline]
 __do_sys_mount fs/namespace.c:3413 [inline]
 __se_sys_mount fs/namespace.c:3390 [inline]
 __x64_sys_mount+0x27f/0x300 fs/namespace.c:3390
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x46004a
Code: b8 a6 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 fd 89 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 0f 83 da 89 fb ff c3 66 0f 1f 84 00 00 00 00 00
RSP: 002b:00007f414d78da88 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007f414d78db20 RCX: 000000000046004a
RDX: 0000000020000000 RSI: 0000000020000100 RDI: 00007f414d78dae0
RBP: 00007f414d78dae0 R08: 00007f414d78db20 R09: 0000000020000000
R10: 0000000000000000 R11: 0000000000000202 R12: 0000000020000000
R13: 0000000020000100 R14: 0000000020000200 R15: 000000002001a800

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reported-by: syzbot+4cfe71a4da060be47502@syzkaller.appspotmail.com
---
 fs/btrfs/volumes.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ec9dac40b4f1..2fd73eab6219 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1080,8 +1080,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 		if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
 			list_del_init(&device->dev_alloc_list);
 			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
-			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-				      &device->dev_state))
+			if (device->devid != BTRFS_DEV_REPLACE_DEVID)
 				fs_devices->rw_devices--;
 		}
 		list_del_init(&device->dev_list);
-- 
2.25.1

Re: [PATCH add reported by] btrfs: fix rw_devices count in __btrfs_free_extra_devids

[Josef Bacik]

On 9/22/20 8:33 AM, Anand Jain wrote:
> syzbot reported a warning [1] in close_fs_devcies() which it reproduces
> using a crafted image.
> 
>          WARN_ON(fs_devices->rw_devices);
> 
> The crafted image successfully creates a replace-device with the devid 0.
> But as there isn't any replace-item. We clean the extra the devid 0, at
> __btrfs_free_extra_devids().
> 
> rw_devices is incremented in btrfs_open_one_device() for all write-able
> devices except for devid == BTRFS_DEV_REPLACE_DEVID.
> But while we clean up the extra devices in __btrfs_free_extra_devids()
> we used the BTRFS_DEV_STATE_REPLACE_TGT flag which isn't set because
> there isn't the replace-item. So rw_devices went below zero.
> 
> So let __btrfs_free_extra_devids() also depend on the
> devid != BTRFS_DEV_REPLACE_DEVID to manage the rw_devices.
> 

This is an invalid state for the fs to be in, I'd rather fix it by detecting we 
have a devid == BTRFS_DEV_REPLACE_DEVID with no corresponding dev_replace item 
and fail out before we get to this point.  Thanks,

Josef

Re: [PATCH add reported by] btrfs: fix rw_devices count in __btrfs_free_extra_devids

[Anand Jain]

On 22/9/20 9:08 pm, Josef Bacik wrote:
> On 9/22/20 8:33 AM, Anand Jain wrote:
>> syzbot reported a warning [1] in close_fs_devcies() which it reproduces
>> using a crafted image.
>>
>>          WARN_ON(fs_devices->rw_devices);
>>
>> The crafted image successfully creates a replace-device with the devid 0.
>> But as there isn't any replace-item. We clean the extra the devid 0, at
>> __btrfs_free_extra_devids().
>>
>> rw_devices is incremented in btrfs_open_one_device() for all write-able
>> devices except for devid == BTRFS_DEV_REPLACE_DEVID.
>> But while we clean up the extra devices in __btrfs_free_extra_devids()
>> we used the BTRFS_DEV_STATE_REPLACE_TGT flag which isn't set because
>> there isn't the replace-item. So rw_devices went below zero.
>>
>> So let __btrfs_free_extra_devids() also depend on the
>> devid != BTRFS_DEV_REPLACE_DEVID to manage the rw_devices.
>>
> 
> This is an invalid state for the fs to be in,

OK, to be more specific. There is an alien device that is pretending to 
be the replace-target (devid = 0).

 > I'd rather fix it by
 > detecting we have a devid == BTRFS_DEV_REPLACE_DEVID with no
 > corresponding dev_replace item and fail out before we get to this
 > point.  Thanks,

Yes. __btrfs_free_extra_devids() is already doing in a way the same.

------------------------------------
1040 static void __btrfs_free_extra_devids(struct btrfs_fs_devices 
*fs_devices,
::
1059 if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
::
1070 if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
1071 &device->dev_state)) {
1072 continue;
1073 }
------------------------------------

OR I did not understand what do you mean.

Thanks, Anand

> 
> Josef

Re: [PATCH add reported by] btrfs: fix rw_devices count in __btrfs_free_extra_devids

[Josef Bacik]

On 9/23/20 12:42 AM, Anand Jain wrote:
> 
> 
> On 22/9/20 9:08 pm, Josef Bacik wrote:
>> On 9/22/20 8:33 AM, Anand Jain wrote:
>>> syzbot reported a warning [1] in close_fs_devcies() which it reproduces
>>> using a crafted image.
>>>
>>>          WARN_ON(fs_devices->rw_devices);
>>>
>>> The crafted image successfully creates a replace-device with the devid 0.
>>> But as there isn't any replace-item. We clean the extra the devid 0, at
>>> __btrfs_free_extra_devids().
>>>
>>> rw_devices is incremented in btrfs_open_one_device() for all write-able
>>> devices except for devid == BTRFS_DEV_REPLACE_DEVID.
>>> But while we clean up the extra devices in __btrfs_free_extra_devids()
>>> we used the BTRFS_DEV_STATE_REPLACE_TGT flag which isn't set because
>>> there isn't the replace-item. So rw_devices went below zero.
>>>
>>> So let __btrfs_free_extra_devids() also depend on the
>>> devid != BTRFS_DEV_REPLACE_DEVID to manage the rw_devices.
>>>
>>
>> This is an invalid state for the fs to be in,
> 
> OK, to be more specific. There is an alien device that is pretending to be the 
> replace-target (devid = 0).
> 
>  > I'd rather fix it by
>  > detecting we have a devid == BTRFS_DEV_REPLACE_DEVID with no
>  > corresponding dev_replace item and fail out before we get to this
>  > point.  Thanks,
> 
> Yes. __btrfs_free_extra_devids() is already doing in a way the same.
> 
> ------------------------------------
> 1040 static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
> ::
> 1059 if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
> ::
> 1070 if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
> 1071 &device->dev_state)) {
> 1072 continue;
> 1073 }
> ------------------------------------
> 
> OR I did not understand what do you mean.
> 

Yeah I mean we do something in btrfs_init_dev_replace(), like when we search for 
the key, we double check to make sure we don't have a devid == 
BTRFS_DEV_REPLACE_DEVID in our devices if we don't find a key.  If we do we 
return -EIO and bail out of the mount.  Thanks,

Josef

Re: [PATCH add reported by] btrfs: fix rw_devices count in __btrfs_free_extra_devids

[Anand Jain]

On 23/9/20 9:42 pm, Josef Bacik wrote:
> On 9/23/20 12:42 AM, Anand Jain wrote:
>>
>>
>> On 22/9/20 9:08 pm, Josef Bacik wrote:
>>> On 9/22/20 8:33 AM, Anand Jain wrote:
>>>> syzbot reported a warning [1] in close_fs_devcies() which it reproduces
>>>> using a crafted image.
>>>>
>>>>          WARN_ON(fs_devices->rw_devices);
>>>>
>>>> The crafted image successfully creates a replace-device with the 
>>>> devid 0.
>>>> But as there isn't any replace-item. We clean the extra the devid 0, at
>>>> __btrfs_free_extra_devids().
>>>>
>>>> rw_devices is incremented in btrfs_open_one_device() for all write-able
>>>> devices except for devid == BTRFS_DEV_REPLACE_DEVID.
>>>> But while we clean up the extra devices in __btrfs_free_extra_devids()
>>>> we used the BTRFS_DEV_STATE_REPLACE_TGT flag which isn't set because
>>>> there isn't the replace-item. So rw_devices went below zero.
>>>>
>>>> So let __btrfs_free_extra_devids() also depend on the
>>>> devid != BTRFS_DEV_REPLACE_DEVID to manage the rw_devices.
>>>>
>>>
>>> This is an invalid state for the fs to be in,
>>
>> OK, to be more specific. There is an alien device that is pretending 
>> to be the replace-target (devid = 0).
>>
>>  > I'd rather fix it by
>>  > detecting we have a devid == BTRFS_DEV_REPLACE_DEVID with no
>>  > corresponding dev_replace item and fail out before we get to this
>>  > point.  Thanks,
>>
>> Yes. __btrfs_free_extra_devids() is already doing in a way the same.
>>
>> ------------------------------------
>> 1040 static void __btrfs_free_extra_devids(struct btrfs_fs_devices 
>> *fs_devices,
>> ::
>> 1059 if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
>> ::
>> 1070 if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
>> 1071 &device->dev_state)) {
>> 1072 continue;
>> 1073 }
>> ------------------------------------
>>
>> OR I did not understand what do you mean.
>>
> 
> Yeah I mean we do something in btrfs_init_dev_replace(), like when we 
> search for the key, we double check to make sure we don't have a devid 
> == BTRFS_DEV_REPLACE_DEVID in our devices if we don't find a key.  If we 
> do we return -EIO and bail out of the mount.  Thanks,

  That's brilliant approach. Let me try.

Thanks, Anand

> 
> Josef

Re: [PATCH add reported by] btrfs: fix rw_devices count in __btrfs_free_extra_devids

[David Sterba]

On Wed, Sep 23, 2020 at 09:42:17AM -0400, Josef Bacik wrote:
> On 9/23/20 12:42 AM, Anand Jain wrote:
> > On 22/9/20 9:08 pm, Josef Bacik wrote:
> >> On 9/22/20 8:33 AM, Anand Jain wrote:

> Yeah I mean we do something in btrfs_init_dev_replace(), like when we search for 
> the key, we double check to make sure we don't have a devid == 
> BTRFS_DEV_REPLACE_DEVID in our devices if we don't find a key.  If we do we 
> return -EIO and bail out of the mount.  Thanks,

From user perspective, then do what? Or do we treat this with minimal
efforts to provide a sane fallback and error handling just to pass
fuzzers (like in many other cases)?

Re: [PATCH add reported by] btrfs: fix rw_devices count in __btrfs_free_extra_devids

[Josef Bacik]

On 9/24/20 7:25 AM, David Sterba wrote:
> On Wed, Sep 23, 2020 at 09:42:17AM -0400, Josef Bacik wrote:
>> On 9/23/20 12:42 AM, Anand Jain wrote:
>>> On 22/9/20 9:08 pm, Josef Bacik wrote:
>>>> On 9/22/20 8:33 AM, Anand Jain wrote:
> 
>> Yeah I mean we do something in btrfs_init_dev_replace(), like when we search for
>> the key, we double check to make sure we don't have a devid ==
>> BTRFS_DEV_REPLACE_DEVID in our devices if we don't find a key.  If we do we
>> return -EIO and bail out of the mount.  Thanks,
> 
>  From user perspective, then do what? Or do we treat this with minimal
> efforts to provide a sane fallback and error handling just to pass
> fuzzers (like in many other cases)?
> 

That's a question for fsck.  I don't want to spend a lot of time chasing 
imaginary cases that fuzzers come up with, I just want them to fail as quickly 
as possible so we can move on with our lives.

If this happened in the real world then it would be because we either

1) Lost the replace item somehow?
2) Got a random corruption that changed the devid to 0

I think for #1 it's impossible to detect really, unless you can tell which 
device was being replaced somehow?  I'm not sure  how you would do that, I'm not 
familiar enough with the replace code to see if we could figure that out.

For #2 it should be straightforward, as long as we can determine that we really 
weren't doing a device replace, then we just change the devid to 1 or something 
and carry on with life?  Thanks,

Josef

Re: [PATCH add reported by] btrfs: fix rw_devices count in __btrfs_free_extra_devids

[Anand Jain]

On 24/9/20 10:02 pm, Josef Bacik wrote:
> On 9/24/20 7:25 AM, David Sterba wrote:
>> On Wed, Sep 23, 2020 at 09:42:17AM -0400, Josef Bacik wrote:
>>> On 9/23/20 12:42 AM, Anand Jain wrote:
>>>> On 22/9/20 9:08 pm, Josef Bacik wrote:
>>>>> On 9/22/20 8:33 AM, Anand Jain wrote:
>>
>>> Yeah I mean we do something in btrfs_init_dev_replace(), like when we 
>>> search for
>>> the key, we double check to make sure we don't have a devid ==
>>> BTRFS_DEV_REPLACE_DEVID in our devices if we don't find a key. 


>>> If we 
>>> do we
>>> return -EIO and bail out of the mount.  Thanks,


I read fast and missed the bailout part before.

If we bailout the mount, it means a btrfs rootfs can fail to boot up.

To recover from it, the user has to remove the trespassing/extra device
manually and reboot.
For a non-rootfs, the user would have to remove the device manually and run
'btrfs dev scan --forget' to free up the extra devices.
What we are doing now is removing the extra/trespassing device
internally.

IMO. The case of trespassing/extra device trying to sabotage the setup
is a bit different from a corrupted device, in the former case
resilience is preferred?

Thanks, Anand


>>
>>  From user perspective, then do what? Or do we treat this with minimal
>> efforts to provide a sane fallback and error handling just to pass
>> fuzzers (like in many other cases)?
>>
> 
> That's a question for fsck.  I don't want to spend a lot of time chasing 
> imaginary cases that fuzzers come up with, I just want them to fail as 
> quickly as possible so we can move on with our lives.
> 
> If this happened in the real world then it would be because we either
> 
> 1) Lost the replace item somehow?
> 2) Got a random corruption that changed the devid to 0
> 
> I think for #1 it's impossible to detect really, unless you can tell 
> which device was being replaced somehow?  I'm not sure  how you would do 
> that, I'm not familiar enough with the replace code to see if we could 
> figure that out.
> 
> For #2 it should be straightforward, as long as we can determine that we 
> really weren't doing a device replace, then we just change the devid to 
> 1 or something and carry on with life?  Thanks,
> 




> Josef

Re: [PATCH add reported by] btrfs: fix rw_devices count in __btrfs_free_extra_devids

[Josef Bacik]

On 9/25/20 6:11 AM, Anand Jain wrote:
> On 24/9/20 10:02 pm, Josef Bacik wrote:
>> On 9/24/20 7:25 AM, David Sterba wrote:
>>> On Wed, Sep 23, 2020 at 09:42:17AM -0400, Josef Bacik wrote:
>>>> On 9/23/20 12:42 AM, Anand Jain wrote:
>>>>> On 22/9/20 9:08 pm, Josef Bacik wrote:
>>>>>> On 9/22/20 8:33 AM, Anand Jain wrote:
>>>
>>>> Yeah I mean we do something in btrfs_init_dev_replace(), like when we search 
>>>> for
>>>> the key, we double check to make sure we don't have a devid ==
>>>> BTRFS_DEV_REPLACE_DEVID in our devices if we don't find a key. 
> 
> 
>>>> If we do we
>>>> return -EIO and bail out of the mount.  Thanks,
> 
> 
> I read fast and missed the bailout part before.
> 
> If we bailout the mount, it means a btrfs rootfs can fail to boot up.
> 
> To recover from it, the user has to remove the trespassing/extra device
> manually and reboot.
> For a non-rootfs, the user would have to remove the device manually and run
> 'btrfs dev scan --forget' to free up the extra devices.
> What we are doing now is removing the extra/trespassing device
> internally.
> 
> IMO. The case of trespassing/extra device trying to sabotage the setup
> is a bit different from a corrupted device, in the former case
> resilience is preferred?
> 

Well this doesn't happen in real life right?  This is purely from a fuzzing 
standpoint, so while resilience should be the first thing we shoot for, I'd 
rather not spend a long time trying to make it work.

In the case of just randomly deleting a device, I don't think that's a decision 
that the kernel can/should make, we should require a user to intervene at that 
point.  That makes failure the best option here, thanks,

Josef

Re: [PATCH add reported by] btrfs: fix rw_devices count in __btrfs_free_extra_devids

[Anand Jain]

On 25/9/20 10:28 pm, Josef Bacik wrote:
> On 9/25/20 6:11 AM, Anand Jain wrote:
>> On 24/9/20 10:02 pm, Josef Bacik wrote:
>>> On 9/24/20 7:25 AM, David Sterba wrote:
>>>> On Wed, Sep 23, 2020 at 09:42:17AM -0400, Josef Bacik wrote:
>>>>> On 9/23/20 12:42 AM, Anand Jain wrote:
>>>>>> On 22/9/20 9:08 pm, Josef Bacik wrote:
>>>>>>> On 9/22/20 8:33 AM, Anand Jain wrote:
>>>>
>>>>> Yeah I mean we do something in btrfs_init_dev_replace(), like when 
>>>>> we search for
>>>>> the key, we double check to make sure we don't have a devid ==
>>>>> BTRFS_DEV_REPLACE_DEVID in our devices if we don't find a key. 
>>
>>
>>>>> If we do we
>>>>> return -EIO and bail out of the mount.  Thanks,
>>
>>
>> I read fast and missed the bailout part before.
>>
>> If we bailout the mount, it means a btrfs rootfs can fail to boot up.
>>
>> To recover from it, the user has to remove the trespassing/extra device
>> manually and reboot.
>> For a non-rootfs, the user would have to remove the device manually 
>> and run
>> 'btrfs dev scan --forget' to free up the extra devices.
>> What we are doing now is removing the extra/trespassing device
>> internally.
>>
>> IMO. The case of trespassing/extra device trying to sabotage the setup
>> is a bit different from a corrupted device, in the former case
>> resilience is preferred?
>>
> 
> Well this doesn't happen in real life right?  This is purely from a 
> fuzzing standpoint, so while resilience should be the first thing we 
> shoot for, I'd rather not spend a long time trying to make it work.
> 
> In the case of just randomly deleting a device, I don't think that's a 
> decision that the kernel can/should make, we should require a user to 
> intervene at that point.  That makes failure the best option here, thanks,
> 

  It makes sense to me, its different from what we had before.
  Made those changes in v2.

Thanks, Anand

> Josef