[PATCH v3 0/6] btrfs-progs: check: Detect invalid dev extents and device items

[PATCH v3 0/6] btrfs-progs: check: Detect invalid dev extents and device items

[Qu Wenruo]

This patchset can be fetch from github:
https://github.com/adam900710/btrfs-progs/tree/dev_extents_check

Hans van Kranenburg reported a case where btrfs DUP chunk allocator
could allocate invalid dev extents, either overlaps with existing dev
extents or beyond device boundary.

This patchset enhances the btrfs-progs side to detect such problems.
With hand crafted test image for it.

Link: https://www.spinics.net/lists/linux-btrfs/msg82370.html

Changelog:
v2:
  Fix a bug in the 1st patch which makes lowmem mode never checks
  overlap dev extents.
  Fix test case bug which never passes due to wrong script.
v3:
  Add btrfs-image fixes to make test cases happy.

Qu Wenruo (6):
  btrfs-progs: image: Use correct device size when restoring
  btrfs-progs: lowmem check: Add check for overlapping dev extents
  btrfs-progs: original check: Add ability to detect bad dev extents
  btrfs-progs: lowmem check: Add dev_item check for used bytes and total
    bytes
  btrfs-progs: original check: Add dev_item check for used bytes and
    total bytes
  btrfs-progs: fsck-tests: Add test image for dev extents beyond device
    boundary

 check/main.c                                  | 105 ++++++++++++++++++
 check/mode-lowmem.c                           |  39 ++++++-
 image/main.c                                  |  48 +++++++-
 .../over_dev_boundary.img.xz                  | Bin 0 -> 1640 bytes
 tests/fsck-tests/036-bad-dev-extents/test.sh  |  20 ++++
 5 files changed, 207 insertions(+), 5 deletions(-)
 create mode 100644 tests/fsck-tests/036-bad-dev-extents/over_dev_boundary.img.xz
 create mode 100755 tests/fsck-tests/036-bad-dev-extents/test.sh

-- 
2.19.1

[PATCH v3 1/6] btrfs-progs: image: Use correct device size when restoring

[Qu Wenruo]

When restoring btrfs image, the total_bytes of device item is not
updated correctly. In fact total_bytes can be left 0 for restored image.

It doesn't trigger any error because btrfs check never checks
total_bytes of dev item.
However this is going to change.

Fix it by populating total_bytes of device item with the end position of
last dev extent to make later btrfs check happy.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 image/main.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/image/main.c b/image/main.c
index 351c5a256938..d5b89bc3149f 100644
--- a/image/main.c
+++ b/image/main.c
@@ -2082,15 +2082,17 @@ static void remap_overlapping_chunks(struct mdrestore_struct *mdres)
 }
 
 static int fixup_devices(struct btrfs_fs_info *fs_info,
-			 struct mdrestore_struct *mdres, off_t dev_size)
+			 struct mdrestore_struct *mdres, int out_fd)
 {
 	struct btrfs_trans_handle *trans;
 	struct btrfs_dev_item *dev_item;
+	struct btrfs_dev_extent *dev_ext;
 	struct btrfs_path path;
 	struct extent_buffer *leaf;
 	struct btrfs_root *root = fs_info->chunk_root;
 	struct btrfs_key key;
 	u64 devid, cur_devid;
+	u64 dev_size; /* Get from last dev extents */
 	int ret;
 
 	trans = btrfs_start_transaction(fs_info->tree_root, 1);
@@ -2101,16 +2103,56 @@ static int fixup_devices(struct btrfs_fs_info *fs_info,
 
 	dev_item = &fs_info->super_copy->dev_item;
 
+	btrfs_init_path(&path);
 	devid = btrfs_stack_device_id(dev_item);
 
+	key.objectid = devid;
+	key.type = BTRFS_DEV_EXTENT_KEY;
+	key.offset = (u64)-1;
+
+	ret = btrfs_search_slot(NULL, fs_info->dev_root, &key, &path, 0, 0);
+	if (ret < 0) {
+		error("failed to locate last dev extent of devid %llu: %s",
+			devid, strerror(-ret));
+		btrfs_release_path(&path);
+		return ret;
+	}
+	if (ret == 0) {
+		error("found invalid dev extent devid %llu offset -1",
+			devid);
+		btrfs_release_path(&path);
+		return -EUCLEAN;
+	}
+	ret = btrfs_previous_item(fs_info->dev_root, &path, devid,
+				  BTRFS_DEV_EXTENT_KEY);
+	if (ret > 0)
+		ret = -ENOENT;
+	if (ret < 0) {
+		error("failed to locate last dev extent of devid %llu: %s",
+			devid, strerror(-ret));
+		btrfs_release_path(&path);
+		return ret;
+	}
+
+	btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]);
+	dev_ext = btrfs_item_ptr(path.nodes[0], path.slots[0],
+				 struct btrfs_dev_extent);
+	dev_size = key.offset + btrfs_dev_extent_length(path.nodes[0], dev_ext);
+	btrfs_release_path(&path);
+
 	btrfs_set_stack_device_total_bytes(dev_item, dev_size);
 	btrfs_set_stack_device_bytes_used(dev_item, mdres->alloced_chunks);
+	/* Don't forget to enlarge the real file */
+	ret = ftruncate64(out_fd, dev_size);
+	if (ret < 0) {
+		error("failed to enlarge result image: %s", strerror(errno));
+		return -errno;
+	}
 
 	key.objectid = BTRFS_DEV_ITEMS_OBJECTID;
 	key.type = BTRFS_DEV_ITEM_KEY;
 	key.offset = 0;
 
-	btrfs_init_path(&path);
 
 again:
 	ret = btrfs_search_slot(trans, root, &key, &path, -1, 1);
@@ -2275,7 +2317,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 			return 1;
 		}
 
-		ret = fixup_devices(info, &mdrestore, st.st_size);
+		ret = fixup_devices(info, &mdrestore, fileno(out));
 		close_ctree(info->chunk_root);
 		if (ret)
 			goto out;
-- 
2.19.1

[PATCH v3 2/6] btrfs-progs: lowmem check: Add check for overlapping dev extents

[Qu Wenruo]

Add such check at check_dev_item(), since at that timing we're also
iterating dev extents for dev item accounting.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/mode-lowmem.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 1bce44f5658a..07c03cad77af 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -4065,6 +4065,8 @@ static int check_dev_item(struct btrfs_fs_info *fs_info,
 	u64 dev_id;
 	u64 used;
 	u64 total = 0;
+	u64 prev_devid = 0;
+	u64 prev_dev_ext_end = 0;
 	int ret;
 
 	dev_item = btrfs_item_ptr(eb, slot, struct btrfs_dev_item);
@@ -4086,8 +4088,16 @@ static int check_dev_item(struct btrfs_fs_info *fs_info,
 		return REFERENCER_MISSING;
 	}
 
-	/* Iterate dev_extents to calculate the used space of a device */
+	/*
+	 * Iterate dev_extents to calculate the used space of a device
+	 *
+	 * Also make sure no dev extents overlap and end beyond device boundary
+	 */
 	while (1) {
+		u64 devid;
+		u64 physical_offset;
+		u64 physical_len;
+
 		if (path.slots[0] >= btrfs_header_nritems(path.nodes[0]))
 			goto next;
 
@@ -4099,7 +4109,27 @@ static int check_dev_item(struct btrfs_fs_info *fs_info,
 
 		ptr = btrfs_item_ptr(path.nodes[0], path.slots[0],
 				     struct btrfs_dev_extent);
-		total += btrfs_dev_extent_length(path.nodes[0], ptr);
+		devid = key.objectid;
+		physical_offset = key.offset;
+		physical_len = btrfs_dev_extent_length(path.nodes[0], ptr);
+
+		if (prev_devid == devid && physical_offset < prev_dev_ext_end) {
+			error(
+"dev extent devid %llu offset %llu len %llu overlap with previous dev extent end %llu",
+			      devid, physical_offset, physical_len,
+			      prev_dev_ext_end);
+			return ACCOUNTING_MISMATCH;
+		}
+		if (physical_offset + physical_len > total_bytes) {
+			error(
+"dev extent devid %llu offset %llu len %llu is beyond device boundary %llu",
+			      devid, physical_offset, physical_len,
+			      total_bytes);
+			return ACCOUNTING_MISMATCH;
+		}
+		prev_devid = devid;
+		prev_dev_ext_end = physical_offset + physical_len;
+		total += physical_len;
 next:
 		ret = btrfs_next_item(dev_root, &path);
 		if (ret)
-- 
2.19.1

[PATCH v3 3/6] btrfs-progs: original check: Add ability to detect bad dev extents

[Qu Wenruo]

Unlike lowmem mode check, we don't have good place for original mode to
check overlap dev extents.

So this patch introduces a new function, btrfs_check_dev_extents(), to
handle possible bad dev extents.

Reported-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/check/main.c b/check/main.c
index bc2ee22f7943..ff9a785ce555 100644
--- a/check/main.c
+++ b/check/main.c
@@ -8224,6 +8224,99 @@ out:
 	return ret;
 }
 
+/*
+ * Check if all dev extents are valid (not overlap nor beyond device
+ * boundary).
+ *
+ * Dev extents <-> chunk cross checking is already done in check_chunks().
+ */
+static int check_dev_extents(struct btrfs_fs_info *fs_info)
+{
+	struct btrfs_path path;
+	struct btrfs_key key;
+	struct btrfs_root *dev_root = fs_info->dev_root;
+	int ret;
+	u64 prev_devid = 0;
+	u64 prev_dev_ext_end = 0;
+
+	btrfs_init_path(&path);
+
+	key.objectid = 1;
+	key.type = BTRFS_DEV_EXTENT_KEY;
+	key.offset = 0;
+
+	ret = btrfs_search_slot(NULL, dev_root, &key, &path, 0, 0);
+	if (ret < 0) {
+		error("failed to search device tree: %s", strerror(-ret));
+		goto out;
+	}
+	if (path.slots[0] >= btrfs_header_nritems(path.nodes[0])) {
+		ret = btrfs_next_leaf(dev_root, &path);
+		if (ret < 0) {
+			error("failed to find next leaf: %s", strerror(-ret));
+			goto out;
+		}
+		if (ret > 0) {
+			ret = 0;
+			goto out;
+		}
+	}
+
+	while (1) {
+		struct btrfs_dev_extent *dev_ext;
+		struct btrfs_device *dev;
+		u64 devid;
+		u64 physical_offset;
+		u64 physical_len;
+
+		btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]);
+		if (key.type != BTRFS_DEV_EXTENT_KEY)
+			break;
+		dev_ext = btrfs_item_ptr(path.nodes[0], path.slots[0],
+					 struct btrfs_dev_extent);
+		devid = key.objectid;
+		physical_offset = key.offset;
+		physical_len = btrfs_dev_extent_length(path.nodes[0], dev_ext);
+
+		dev = btrfs_find_device(fs_info, devid, NULL, NULL);
+		if (!dev) {
+			error("failed to find device with devid %llu", devid);
+			ret = -EUCLEAN;
+			goto out;
+		}
+		if (prev_devid == devid && prev_dev_ext_end > physical_offset) {
+			error(
+"dev extent devid %llu physical offset %llu overlap with previous dev extent end %llu",
+			      devid, physical_offset, prev_dev_ext_end);
+			ret = -EUCLEAN;
+			goto out;
+		}
+		if (physical_offset + physical_len > dev->total_bytes) {
+			error(
+"dev extent devid %llu physical offset %llu len %llu is beyond device boudnary %llu",
+			      devid, physical_offset, physical_len,
+			      dev->total_bytes);
+			ret = -EUCLEAN;
+			goto out;
+		}
+		prev_devid = devid;
+		prev_dev_ext_end = physical_offset + physical_len;
+
+		ret = btrfs_next_item(dev_root, &path);
+		if (ret < 0) {
+			error("failed to find next leaf: %s", strerror(-ret));
+			goto out;
+		}
+		if (ret > 0) {
+			ret = 0;
+			break;
+		}
+	}
+out:
+	btrfs_release_path(&path);
+	return ret;
+}
+
 static int check_chunks_and_extents(struct btrfs_fs_info *fs_info)
 {
 	struct rb_root dev_cache;
@@ -8318,6 +8411,12 @@ again:
 		goto out;
 	}
 
+	ret = check_dev_extents(fs_info);
+	if (ret < 0) {
+		err = ret;
+		goto out;
+	}
+
 	ret = check_chunks(&chunk_cache, &block_group_cache,
 			   &dev_extent_cache, NULL, NULL, NULL, 0);
 	if (ret) {
-- 
2.19.1

[PATCH v3 4/6] btrfs-progs: lowmem check: Add dev_item check for used bytes and total bytes

[Qu Wenruo]

Obviously, used bytes can't be larger than total bytes.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/mode-lowmem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 07c03cad77af..1173b963b8f3 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -4074,6 +4074,11 @@ static int check_dev_item(struct btrfs_fs_info *fs_info,
 	used = btrfs_device_bytes_used(eb, dev_item);
 	total_bytes = btrfs_device_total_bytes(eb, dev_item);
 
+	if (used > total_bytes) {
+		error("device %llu has incorrect used bytes %llu > total bytes %llu",
+			dev_id, used, total_bytes);
+		return ACCOUNTING_MISMATCH;
+	}
 	key.objectid = dev_id;
 	key.type = BTRFS_DEV_EXTENT_KEY;
 	key.offset = 0;
-- 
2.19.1

[PATCH v3 5/6] btrfs-progs: original check: Add dev_item check for used bytes and total bytes

[Qu Wenruo]

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/check/main.c b/check/main.c
index ff9a785ce555..12f12e18a83f 100644
--- a/check/main.c
+++ b/check/main.c
@@ -7938,6 +7938,12 @@ static int check_device_used(struct device_record *dev_rec,
 	struct device_extent_record *dev_extent_rec;
 	u64 total_byte = 0;
 
+	if (dev_rec->byte_used > dev_rec->total_byte) {
+		error("device %llu has incorrect used bytes %llu > total bytes %llu",
+		      dev_rec->devid, dev_rec->byte_used, dev_rec->total_byte);
+		return -EUCLEAN;
+	}
+
 	cache = search_cache_extent2(&dext_cache->tree, dev_rec->devid, 0);
 	while (cache) {
 		dev_extent_rec = container_of(cache,
-- 
2.19.1

[PATCH v3 6/6] btrfs-progs: fsck-tests: Add test image for dev extents beyond device boundary

[Qu Wenruo]

Now two locations can detect such problem, either by device item
used/total bytes check, or by early dev extents check against device
boundary.

The image is hand-crafted image which uses DATA SINGLE chunk to feed
btrfs check.
As expected, as long as block group item, chunk item, device used bytes
matches, older btrfs check can't detect such problem.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../over_dev_boundary.img.xz                  | Bin 0 -> 1640 bytes
 tests/fsck-tests/036-bad-dev-extents/test.sh  |  20 ++++++++++++++++++
 2 files changed, 20 insertions(+)
 create mode 100644 tests/fsck-tests/036-bad-dev-extents/over_dev_boundary.img.xz
 create mode 100755 tests/fsck-tests/036-bad-dev-extents/test.sh

diff --git a/tests/fsck-tests/036-bad-dev-extents/over_dev_boundary.img.xz b/tests/fsck-tests/036-bad-dev-extents/over_dev_boundary.img.xz
new file mode 100644
index 0000000000000000000000000000000000000000..47cb2a707b0097e369dc088ed0549f847995f136
GIT binary patch
literal 1640
zcmV-u2ABE$H+ooF000E$*0e?f03iVu0001VFXf})3;zZsT>wRyj;C3^v%$$4d1oRm
zhA1@4%tH=9jYF%IQSpIUKDpjXLRl?q4p$q;1zsY^#9_Lx=#tbGm>@S#e2aqt!?0}y
z2BPO%4~c9Q5)jKFD7}DURarKL)`^j{f&YXRZFN7?s>sEHdzmSvX^98%kGi<&Wr9_8
zMn<v|L-7(}Rd54?!s&KC;)cj02cGeRIr%lcuq9CieJ^uw#gN*DXHbJJ_cG<`)tzRZ
z7iiTIj!JKmA%K$<jld>XynsC*B7}KE(<VQ-{-;Dq!XKG7h7GX&7)6g)x&zR#3}<?`
zerVp{I4Mf*_^y@gB;xwKVA$2}#YP`5&9z#2nY9o9cf1ycyoVvllLj)oQduMo;#&{T
zIb!}$))B*sP4(pSQi9Q{J{(5)Q<QJ$OUITVpN^z#lp9T;6uVqfPz2ufgR@hYgxVDk
z$x`_6#%t^+I@^z?>6w>*wfdb|tw$kt^y>W+TB*?pon1P+)#u#?6bSIG)TooJx~u$#
zf^+}xKw`BfI}6=717S~Q%LW1kwY`pz9H{`uNNNFOk2w!VauNEaMfoLj)Z<)!1?F60
zJ+OEA|4$a=9W#XX*l{EG!j^s}p|<!K+xHDwKHbAw($8~rad$^EE6=y6k#GR;ztm<w
z#ahHUL<!32tL3o;`MDc-;{87F5HD*@m)7W=TJ3!a%;uIK&c5L=OTnVyOUO^UUIHB>
z0i#_%$Q}d)-EE8#8O5<!nULUUAp@}=Z3Fh99&W?=NxIvgTZoJI!`e>^x$&V$8Y0l2
zc19e0Et3O3m`pMOqEkasL8VGE+u~2lZn>sRCRj169Z6mQ3*+`D+C#F1V7POV%<anZ
z6pCKQ@RVSrcwcVrOLZ?$xtD*57PoeA63&N-%XnJ(lp#DsxA)DEe4#pIJYN4L?KKfP
zZ}8xx1}Yb8^sa~PQ{dhJnXpW^iQQ1BG8H;hb)OypOm}Iq9~CEBz4Gap9nv3LG_$mV
zuM<Bz1a#)PJc(dveSQp!nMsohFT14>lx(9cB{WN*9OP%Zbd1VDn(S4HX^ad4-b~#H
z@9eUP4AAU`)yRf!k+rrrLSYfBSEi6RE#HtbqyPl<Sul{VKSX-iNqlnSZIysM_ebUb
z+~Xr5U&)ibJOQ1TBtnlIOb`Bkw2_-U5}W;WbJF=|BPlxM45+#vgP+{|6lPI<iIY$2
zG@MwL0dl;En#ny@be99^sS^qL;@t&ZhuFHzGD{dL39T{^4<C^+pb+1`aF>11S>RCH
zqvJbt22t`FmU^tmTb+7LtpybCB-x1lGSlrpVQ9|6WNBs~q*M-to<L(Acj@$J6%B0l
z9Z>1gD1l41oiy~}+O?!{68Jvim2*%BznW)@B=3<jH~jhjz|ZCMWv6gWeQlz-(KTzF
z7VI7(No$qk2aLjuYY$0hK-g)7yzAX9CKq>IH<h+68}6;%x(9&vmLt}L<5`-ww7;yQ
zm6u{#hw;;>)Xi1795q{#>sF*y^0T2@b$`Wqwch&z?BgN}IR9Ui&GZjmedlGizBd0T
z;!cs&L)hJFGsJmFaiUsYrN$c0^BLU^n-B%fagn+jR{?Dq+K%VyMG<I@0%=3<4suF^
z+%tHH6zc&0NeZ$t3@x+?%%xrBeXnAtt0~N?4^g>@pmAOShFY)k8zBxm@7Y<iJ#}$8
z5HO92kagkd`}eTFMbYHXeVZDQ@eRuAkeEv-e%8D9)<yk(Ez|qR1*ydekW&XPuTZ>D
zb^ZXU;<`+_B<hPLATaU}Ag4&`<8MrKV#`=!NVGD=jjWs!gw+dT#7~C>+IV{+A~1Ku
zWggqWQd{E<Q#S2aj1fMk8XA86;5b|mA_;!hpUN0QJ*Y?jrtXmDo_GK&$59E$0cwGr
z_$LwlZjpqrHefLrab;~b&y9-po!d<p<?(p{28(FQ{9snXm)a7~Ou+6=_!MzIPnklU
z?nMI-Yw`o)9vH>%hF}W2ArPwJ33zWP>MIe;YDN8WV+|+a4_c>yFN#ZGN00G#%3HYi
z4pePTnc{8k5CjwuN6hudRFcBkvB^&y3;pSFufzaaD`-;mPgJ~wr(<glZK${QAL#%9
m0001a5yH9Ybh`Bb0l^G_7ytl1uze%3#Ao{g000001X)@i`Wb5g

literal 0
HcmV?d00001

diff --git a/tests/fsck-tests/036-bad-dev-extents/test.sh b/tests/fsck-tests/036-bad-dev-extents/test.sh
new file mode 100755
index 000000000000..fe2d05673bbe
--- /dev/null
+++ b/tests/fsck-tests/036-bad-dev-extents/test.sh
@@ -0,0 +1,20 @@
+#!/bin/bash
+#
+# Due to DUP chunk allocator bugs, we could allocate DUP chunks while its dev
+# extents could exist beyond device boundary.
+# And since all related items (block group, chunk, device used bytes) are all
+# valid, btrfs check won't report any error.
+#
+# This test case contain hand crafted minimal image, to test if btrfs check can
+# detect and report such error.
+
+source "$TEST_TOP/common"
+
+check_prereq btrfs
+
+check_image() {
+	run_mustfail "btrfs check failed to detect invalid dev extents" \
+		"$TOP/btrfs" check "$1"
+}
+
+check_all_images
-- 
2.19.1

Re: [PATCH v3 4/6] btrfs-progs: lowmem check: Add dev_item check for used bytes and total bytes

[Hans van Kranenburg]

On 10/08/2018 02:30 PM, Qu Wenruo wrote:
> Obviously, used bytes can't be larger than total bytes.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  check/mode-lowmem.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 07c03cad77af..1173b963b8f3 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -4074,6 +4074,11 @@ static int check_dev_item(struct btrfs_fs_info *fs_info,
>  	used = btrfs_device_bytes_used(eb, dev_item);
>  	total_bytes = btrfs_device_total_bytes(eb, dev_item);
>  
> +	if (used > total_bytes) {
> +		error("device %llu has incorrect used bytes %llu > total bytes %llu",
> +			dev_id, used, total_bytes);
> +		return ACCOUNTING_MISMATCH;

The message and return code point at an error in accounting logic.

However, if you have a fully allocated device and a DUP chunk ending
beyond device, then having used > total_bytes is expected...

So maybe there's two possibilities... There's an error in the accounting
logic, or there's an "over-allocation", which is another type of issue
which produces used > total with correct accounting logic.

> +	}
>  	key.objectid = dev_id;
>  	key.type = BTRFS_DEV_EXTENT_KEY;
>  	key.offset = 0;
> 


-- 
Hans van Kranenburg

Re: [PATCH v3 4/6] btrfs-progs: lowmem check: Add dev_item check for used bytes and total bytes

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1822 bytes --]



On 2018/10/9 上午6:20, Hans van Kranenburg wrote:
> On 10/08/2018 02:30 PM, Qu Wenruo wrote:
>> Obviously, used bytes can't be larger than total bytes.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  check/mode-lowmem.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
>> index 07c03cad77af..1173b963b8f3 100644
>> --- a/check/mode-lowmem.c
>> +++ b/check/mode-lowmem.c
>> @@ -4074,6 +4074,11 @@ static int check_dev_item(struct btrfs_fs_info *fs_info,
>>  	used = btrfs_device_bytes_used(eb, dev_item);
>>  	total_bytes = btrfs_device_total_bytes(eb, dev_item);
>>  
>> +	if (used > total_bytes) {
>> +		error("device %llu has incorrect used bytes %llu > total bytes %llu",
>> +			dev_id, used, total_bytes);
>> +		return ACCOUNTING_MISMATCH;
> 
> The message and return code point at an error in accounting logic.

One of the biggest problem in lowmem is we don't always have the error
code we really want.

And that's the case for this error message.
It's indeed not an accounting error, as in that case (just like that
test case image) the used bytes is correct accounted.

The good news is, the return value is never really used to classify the
error.
So as long as the error message makes sense, it's not a big problem.

Thanks,
Qu

> 
> However, if you have a fully allocated device and a DUP chunk ending
> beyond device, then having used > total_bytes is expected...
> 
> So maybe there's two possibilities... There's an error in the accounting
> logic, or there's an "over-allocation", which is another type of issue
> which produces used > total with correct accounting logic.
> 
>> +	}
>>  	key.objectid = dev_id;
>>  	key.type = BTRFS_DEV_EXTENT_KEY;
>>  	key.offset = 0;
>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/6] btrfs-progs: lowmem check: Add check for overlapping dev extents

[Su Yue]

On 10/8/18 8:30 PM, Qu Wenruo wrote:
> Add such check at check_dev_item(), since at that timing we're also
> iterating dev extents for dev item accounting.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

LGTM.
Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>   check/mode-lowmem.c | 34 ++++++++++++++++++++++++++++++++--
>   1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 1bce44f5658a..07c03cad77af 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -4065,6 +4065,8 @@ static int check_dev_item(struct btrfs_fs_info *fs_info,
>   	u64 dev_id;
>   	u64 used;
>   	u64 total = 0;
> +	u64 prev_devid = 0;
> +	u64 prev_dev_ext_end = 0;
>   	int ret;
>   
>   	dev_item = btrfs_item_ptr(eb, slot, struct btrfs_dev_item);
> @@ -4086,8 +4088,16 @@ static int check_dev_item(struct btrfs_fs_info *fs_info,
>   		return REFERENCER_MISSING;
>   	}
>   
> -	/* Iterate dev_extents to calculate the used space of a device */
> +	/*
> +	 * Iterate dev_extents to calculate the used space of a device
> +	 *
> +	 * Also make sure no dev extents overlap and end beyond device boundary
> +	 */
>   	while (1) {
> +		u64 devid;
> +		u64 physical_offset;
> +		u64 physical_len;
> +
>   		if (path.slots[0] >= btrfs_header_nritems(path.nodes[0]))
>   			goto next;
>   
> @@ -4099,7 +4109,27 @@ static int check_dev_item(struct btrfs_fs_info *fs_info,
>   
>   		ptr = btrfs_item_ptr(path.nodes[0], path.slots[0],
>   				     struct btrfs_dev_extent);
> -		total += btrfs_dev_extent_length(path.nodes[0], ptr);
> +		devid = key.objectid;
> +		physical_offset = key.offset;
> +		physical_len = btrfs_dev_extent_length(path.nodes[0], ptr);
> +
> +		if (prev_devid == devid && physical_offset < prev_dev_ext_end) {
> +			error(
> +"dev extent devid %llu offset %llu len %llu overlap with previous dev extent end %llu",
> +			      devid, physical_offset, physical_len,
> +			      prev_dev_ext_end);
> +			return ACCOUNTING_MISMATCH;
> +		}
> +		if (physical_offset + physical_len > total_bytes) {
> +			error(
> +"dev extent devid %llu offset %llu len %llu is beyond device boundary %llu",
> +			      devid, physical_offset, physical_len,
> +			      total_bytes);
> +			return ACCOUNTING_MISMATCH;
> +		}
> +		prev_devid = devid;
> +		prev_dev_ext_end = physical_offset + physical_len;
> +		total += physical_len;
>   next:
>   		ret = btrfs_next_item(dev_root, &path);
>   		if (ret)
> 

Re: [PATCH v3 3/6] btrfs-progs: original check: Add ability to detect bad dev extents

[Su Yue]

On 10/8/18 8:30 PM, Qu Wenruo wrote:
> Unlike lowmem mode check, we don't have good place for original mode to
> check overlap dev extents.
> 
> So this patch introduces a new function, btrfs_check_dev_extents(), to
> handle possible bad dev extents.
> 
> Reported-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>   check/main.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 99 insertions(+)
> 
> diff --git a/check/main.c b/check/main.c
> index bc2ee22f7943..ff9a785ce555 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -8224,6 +8224,99 @@ out:
>   	return ret;
>   }
>   
> +/*
> + * Check if all dev extents are valid (not overlap nor beyond device
> + * boundary).
> + *
> + * Dev extents <-> chunk cross checking is already done in check_chunks().
> + */
> +static int check_dev_extents(struct btrfs_fs_info *fs_info)
> +{
> +	struct btrfs_path path;
> +	struct btrfs_key key;
> +	struct btrfs_root *dev_root = fs_info->dev_root;
> +	int ret;
> +	u64 prev_devid = 0;
> +	u64 prev_dev_ext_end = 0;
> +
> +	btrfs_init_path(&path);
> +
> +	key.objectid = 1;
> +	key.type = BTRFS_DEV_EXTENT_KEY;
> +	key.offset = 0;
> +
> +	ret = btrfs_search_slot(NULL, dev_root, &key, &path, 0, 0);
> +	if (ret < 0) {
> +		error("failed to search device tree: %s", strerror(-ret));
> +		goto out;
> +	}
> +	if (path.slots[0] >= btrfs_header_nritems(path.nodes[0])) {
> +		ret = btrfs_next_leaf(dev_root, &path);
> +		if (ret < 0) {
> +			error("failed to find next leaf: %s", strerror(-ret));
> +			goto out;
> +		}
> +		if (ret > 0) {
> +			ret = 0;
> +			goto out;
> +		}
> +	}
> +
> +	while (1) {
> +		struct btrfs_dev_extent *dev_ext;
> +		struct btrfs_device *dev;
> +		u64 devid;
> +		u64 physical_offset;
> +		u64 physical_len;
> +
> +		btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]);
> +		if (key.type != BTRFS_DEV_EXTENT_KEY)
> +			break;
> +		dev_ext = btrfs_item_ptr(path.nodes[0], path.slots[0],
> +					 struct btrfs_dev_extent);
> +		devid = key.objectid;
> +		physical_offset = key.offset;
> +		physical_len = btrfs_dev_extent_length(path.nodes[0], dev_ext);
> +
> +		dev = btrfs_find_device(fs_info, devid, NULL, NULL);
> +		if (!dev) {
> +			error("failed to find device with devid %llu", devid);
> +			ret = -EUCLEAN;
> +			goto out;
> +		}
> +		if (prev_devid == devid && prev_dev_ext_end > physical_offset) {
> +			error(
> +"dev extent devid %llu physical offset %llu overlap with previous dev extent end %llu",
> +			      devid, physical_offset, prev_dev_ext_end);
> +			ret = -EUCLEAN;
> +			goto out;
> +		}
> +		if (physical_offset + physical_len > dev->total_bytes) {
> +			error(
> +"dev extent devid %llu physical offset %llu len %llu is beyond device boudnary %llu",
> +			      devid, physical_offset, physical_len,
> +			      dev->total_bytes);
> +			ret = -EUCLEAN;
> +			goto out;
> +		}
> +		prev_devid = devid;
> +		prev_dev_ext_end = physical_offset + physical_len;
> +
> +		ret = btrfs_next_item(dev_root, &path);
> +		if (ret < 0) {
> +			error("failed to find next leaf: %s", strerror(-ret));
> +			goto out;
> +		}
> +		if (ret > 0) {
> +			ret = 0;
> +			break;
> +		}
> +	}
> +out:
> +	btrfs_release_path(&path);
> +	return ret;
> +}
> +
>   static int check_chunks_and_extents(struct btrfs_fs_info *fs_info)
>   {
>   	struct rb_root dev_cache;
> @@ -8318,6 +8411,12 @@ again:
>   		goto out;
>   	}
>   
> +	ret = check_dev_extents(fs_info);
> +	if (ret < 0) {
> +		err = ret;
> +		goto out;
> +	}
> +
>   	ret = check_chunks(&chunk_cache, &block_group_cache,
>   			   &dev_extent_cache, NULL, NULL, NULL, 0);
>   	if (ret) {
> 

Re: [PATCH v3 4/6] btrfs-progs: lowmem check: Add dev_item check for used bytes and total bytes

[Hans van Kranenburg]

On 10/09/2018 03:14 AM, Qu Wenruo wrote:
> 
> 
> On 2018/10/9 上午6:20, Hans van Kranenburg wrote:
>> On 10/08/2018 02:30 PM, Qu Wenruo wrote:
>>> Obviously, used bytes can't be larger than total bytes.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  check/mode-lowmem.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
>>> index 07c03cad77af..1173b963b8f3 100644
>>> --- a/check/mode-lowmem.c
>>> +++ b/check/mode-lowmem.c
>>> @@ -4074,6 +4074,11 @@ static int check_dev_item(struct btrfs_fs_info *fs_info,
>>>  	used = btrfs_device_bytes_used(eb, dev_item);
>>>  	total_bytes = btrfs_device_total_bytes(eb, dev_item);
>>>  
>>> +	if (used > total_bytes) {
>>> +		error("device %llu has incorrect used bytes %llu > total bytes %llu",
>>> +			dev_id, used, total_bytes);
>>> +		return ACCOUNTING_MISMATCH;
>>
>> The message and return code point at an error in accounting logic.
> 
> One of the biggest problem in lowmem is we don't always have the error
> code we really want.
> 
> And that's the case for this error message.
> It's indeed not an accounting error, as in that case (just like that
> test case image) the used bytes is correct accounted.
> 
> The good news is, the return value is never really used to classify the
> error.
> So as long as the error message makes sense, it's not a big problem.

Aha. Clear, thanks for explaining.

> 
> Thanks,
> Qu
> 
>>
>> However, if you have a fully allocated device and a DUP chunk ending
>> beyond device, then having used > total_bytes is expected...
>>
>> So maybe there's two possibilities... There's an error in the accounting
>> logic, or there's an "over-allocation", which is another type of issue
>> which produces used > total with correct accounting logic.
>>
>>> +	}
>>>  	key.objectid = dev_id;
>>>  	key.type = BTRFS_DEV_EXTENT_KEY;
>>>  	key.offset = 0;
>>>
>>
>>
> 


-- 
Hans van Kranenburg

Re: [PATCH v3 1/6] btrfs-progs: image: Use correct device size when restoring

[Nikolay Borisov]

On  8.10.2018 15:30, Qu Wenruo wrote:
> When restoring btrfs image, the total_bytes of device item is not
> updated correctly. In fact total_bytes can be left 0 for restored image.
> 
> It doesn't trigger any error because btrfs check never checks
> total_bytes of dev item.
> However this is going to change.
> 
> Fix it by populating total_bytes of device item with the end position of
> last dev extent to make later btrfs check happy.

'Setting it to the end position' really means "setting the total device
size to the allocated space on the device". Is it more clear if stated
as the second way ?

In any case:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  image/main.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/image/main.c b/image/main.c
> index 351c5a256938..d5b89bc3149f 100644
> --- a/image/main.c
> +++ b/image/main.c
> @@ -2082,15 +2082,17 @@ static void remap_overlapping_chunks(struct mdrestore_struct *mdres)
>  }
>  
>  static int fixup_devices(struct btrfs_fs_info *fs_info,
> -			 struct mdrestore_struct *mdres, off_t dev_size)
> +			 struct mdrestore_struct *mdres, int out_fd)
>  {
>  	struct btrfs_trans_handle *trans;
>  	struct btrfs_dev_item *dev_item;
> +	struct btrfs_dev_extent *dev_ext;
>  	struct btrfs_path path;
>  	struct extent_buffer *leaf;
>  	struct btrfs_root *root = fs_info->chunk_root;
>  	struct btrfs_key key;
>  	u64 devid, cur_devid;
> +	u64 dev_size; /* Get from last dev extents */
>  	int ret;
>  
>  	trans = btrfs_start_transaction(fs_info->tree_root, 1);
> @@ -2101,16 +2103,56 @@ static int fixup_devices(struct btrfs_fs_info *fs_info,
>  
>  	dev_item = &fs_info->super_copy->dev_item;
>  
> +	btrfs_init_path(&path);
>  	devid = btrfs_stack_device_id(dev_item);
>  
> +	key.objectid = devid;
> +	key.type = BTRFS_DEV_EXTENT_KEY;
> +	key.offset = (u64)-1;
> +
> +	ret = btrfs_search_slot(NULL, fs_info->dev_root, &key, &path, 0, 0);
> +	if (ret < 0) {
> +		error("failed to locate last dev extent of devid %llu: %s",
> +			devid, strerror(-ret));
> +		btrfs_release_path(&path);
> +		return ret;
> +	}
> +	if (ret == 0) {

nit: I'd prefer if this is an else if since it emphasizes the fact that
the if/elseif construct works on a single value.

> +		error("found invalid dev extent devid %llu offset -1",
> +			devid);
> +		btrfs_release_path(&path);
> +		return -EUCLEAN;
> +	}
> +	ret = btrfs_previous_item(fs_info->dev_root, &path, devid,
> +				  BTRFS_DEV_EXTENT_KEY);
> +	if (ret > 0)
> +		ret = -ENOENT;
> +	if (ret < 0) {

ditto

> +		error("failed to locate last dev extent of devid %llu: %s",
> +			devid, strerror(-ret));
> +		btrfs_release_path(&path);
> +		return ret;
> +	}
> +
> +	btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]);
> +	dev_ext = btrfs_item_ptr(path.nodes[0], path.slots[0],
> +				 struct btrfs_dev_extent);
> +	dev_size = key.offset + btrfs_dev_extent_length(path.nodes[0], dev_ext);
> +	btrfs_release_path(&path);
> +
>  	btrfs_set_stack_device_total_bytes(dev_item, dev_size);
>  	btrfs_set_stack_device_bytes_used(dev_item, mdres->alloced_chunks);
> +	/* Don't forget to enlarge the real file */
> +	ret = ftruncate64(out_fd, dev_size);
> +	if (ret < 0) {
> +		error("failed to enlarge result image: %s", strerror(errno));
> +		return -errno;
> +	}
>  
>  	key.objectid = BTRFS_DEV_ITEMS_OBJECTID;
>  	key.type = BTRFS_DEV_ITEM_KEY;
>  	key.offset = 0;
>  
> -	btrfs_init_path(&path);
>  
>  again:
>  	ret = btrfs_search_slot(trans, root, &key, &path, -1, 1);
> @@ -2275,7 +2317,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
>  			return 1;
>  		}
>  
> -		ret = fixup_devices(info, &mdrestore, st.st_size);
> +		ret = fixup_devices(info, &mdrestore, fileno(out));
>  		close_ctree(info->chunk_root);
>  		if (ret)
>  			goto out;
> 

Re: [PATCH v3 1/6] btrfs-progs: image: Use correct device size when restoring

[Qu Wenruo]

On 2018/10/11 下午8:07, Nikolay Borisov wrote:
> 
> 
> On  8.10.2018 15:30, Qu Wenruo wrote:
>> When restoring btrfs image, the total_bytes of device item is not
>> updated correctly. In fact total_bytes can be left 0 for restored image.
>>
>> It doesn't trigger any error because btrfs check never checks
>> total_bytes of dev item.
>> However this is going to change.
>>
>> Fix it by populating total_bytes of device item with the end position of
>> last dev extent to make later btrfs check happy.
> 
> 'Setting it to the end position' really means "setting the total device
> size to the allocated space on the device". Is it more clear if stated
> as the second way ?

Not exactly.

Don't forget that we have 0~1M reserved, and it's possible to have dev
extent holes.
So it's not "allocated space" but really "the end position of the last
dev extent"

Thanks,
Qu

> 
> In any case:
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  image/main.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 45 insertions(+), 3 deletions(-)
>>
>> diff --git a/image/main.c b/image/main.c
>> index 351c5a256938..d5b89bc3149f 100644
>> --- a/image/main.c
>> +++ b/image/main.c
>> @@ -2082,15 +2082,17 @@ static void remap_overlapping_chunks(struct mdrestore_struct *mdres)
>>  }
>>  
>>  static int fixup_devices(struct btrfs_fs_info *fs_info,
>> -			 struct mdrestore_struct *mdres, off_t dev_size)
>> +			 struct mdrestore_struct *mdres, int out_fd)
>>  {
>>  	struct btrfs_trans_handle *trans;
>>  	struct btrfs_dev_item *dev_item;
>> +	struct btrfs_dev_extent *dev_ext;
>>  	struct btrfs_path path;
>>  	struct extent_buffer *leaf;
>>  	struct btrfs_root *root = fs_info->chunk_root;
>>  	struct btrfs_key key;
>>  	u64 devid, cur_devid;
>> +	u64 dev_size; /* Get from last dev extents */
>>  	int ret;
>>  
>>  	trans = btrfs_start_transaction(fs_info->tree_root, 1);
>> @@ -2101,16 +2103,56 @@ static int fixup_devices(struct btrfs_fs_info *fs_info,
>>  
>>  	dev_item = &fs_info->super_copy->dev_item;
>>  
>> +	btrfs_init_path(&path);
>>  	devid = btrfs_stack_device_id(dev_item);
>>  
>> +	key.objectid = devid;
>> +	key.type = BTRFS_DEV_EXTENT_KEY;
>> +	key.offset = (u64)-1;
>> +
>> +	ret = btrfs_search_slot(NULL, fs_info->dev_root, &key, &path, 0, 0);
>> +	if (ret < 0) {
>> +		error("failed to locate last dev extent of devid %llu: %s",
>> +			devid, strerror(-ret));
>> +		btrfs_release_path(&path);
>> +		return ret;
>> +	}
>> +	if (ret == 0) {
> 
> nit: I'd prefer if this is an else if since it emphasizes the fact that
> the if/elseif construct works on a single value.
> 
>> +		error("found invalid dev extent devid %llu offset -1",
>> +			devid);
>> +		btrfs_release_path(&path);
>> +		return -EUCLEAN;
>> +	}
>> +	ret = btrfs_previous_item(fs_info->dev_root, &path, devid,
>> +				  BTRFS_DEV_EXTENT_KEY);
>> +	if (ret > 0)
>> +		ret = -ENOENT;
>> +	if (ret < 0) {
> 
> ditto
> 
>> +		error("failed to locate last dev extent of devid %llu: %s",
>> +			devid, strerror(-ret));
>> +		btrfs_release_path(&path);
>> +		return ret;
>> +	}
>> +
>> +	btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]);
>> +	dev_ext = btrfs_item_ptr(path.nodes[0], path.slots[0],
>> +				 struct btrfs_dev_extent);
>> +	dev_size = key.offset + btrfs_dev_extent_length(path.nodes[0], dev_ext);
>> +	btrfs_release_path(&path);
>> +
>>  	btrfs_set_stack_device_total_bytes(dev_item, dev_size);
>>  	btrfs_set_stack_device_bytes_used(dev_item, mdres->alloced_chunks);
>> +	/* Don't forget to enlarge the real file */
>> +	ret = ftruncate64(out_fd, dev_size);
>> +	if (ret < 0) {
>> +		error("failed to enlarge result image: %s", strerror(errno));
>> +		return -errno;
>> +	}
>>  
>>  	key.objectid = BTRFS_DEV_ITEMS_OBJECTID;
>>  	key.type = BTRFS_DEV_ITEM_KEY;
>>  	key.offset = 0;
>>  
>> -	btrfs_init_path(&path);
>>  
>>  again:
>>  	ret = btrfs_search_slot(trans, root, &key, &path, -1, 1);
>> @@ -2275,7 +2317,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
>>  			return 1;
>>  		}
>>  
>> -		ret = fixup_devices(info, &mdrestore, st.st_size);
>> +		ret = fixup_devices(info, &mdrestore, fileno(out));
>>  		close_ctree(info->chunk_root);
>>  		if (ret)
>>  			goto out;
>>

Re: [PATCH v3 0/6] btrfs-progs: check: Detect invalid dev extents and device items

[David Sterba]

On Mon, Oct 08, 2018 at 08:30:38PM +0800, Qu Wenruo wrote:
> This patchset can be fetch from github:
> https://github.com/adam900710/btrfs-progs/tree/dev_extents_check
> 
> Hans van Kranenburg reported a case where btrfs DUP chunk allocator
> could allocate invalid dev extents, either overlaps with existing dev
> extents or beyond device boundary.
> 
> This patchset enhances the btrfs-progs side to detect such problems.
> With hand crafted test image for it.
> 
> Link: https://www.spinics.net/lists/linux-btrfs/msg82370.html
> 
> Changelog:
> v2:
>   Fix a bug in the 1st patch which makes lowmem mode never checks
>   overlap dev extents.
>   Fix test case bug which never passes due to wrong script.
> v3:
>   Add btrfs-image fixes to make test cases happy.
> 
> Qu Wenruo (6):
>   btrfs-progs: image: Use correct device size when restoring
>   btrfs-progs: lowmem check: Add check for overlapping dev extents
>   btrfs-progs: original check: Add ability to detect bad dev extents
>   btrfs-progs: lowmem check: Add dev_item check for used bytes and total
>     bytes
>   btrfs-progs: original check: Add dev_item check for used bytes and
>     total bytes
>   btrfs-progs: fsck-tests: Add test image for dev extents beyond device
>     boundary

Added to devel with some fixups, thanks.