* Re: [PATCH] btrfs-progs: check for invalid free space tree entries
2022-07-29 21:08 [PATCH] btrfs-progs: check for invalid free space tree entries Josef Bacik
@ 2022-07-30 3:23 ` Qu Wenruo
2022-08-03 19:33 ` David Sterba
2022-08-30 4:46 ` Qu Wenruo
2 siblings, 0 replies; 5+ messages in thread
From: Qu Wenruo @ 2022-07-30 3:23 UTC (permalink / raw)
To: Josef Bacik, linux-btrfs, kernel-team
On 2022/7/30 05:08, Josef Bacik wrote:
> While testing some changes to how we reclaim block groups I started
> hitting failures with my TEST_DEV. This occurred because I had a bug
> and failed to properly remove a block groups free space tree entries.
> However this wasn't caught in testing when it happened because
> btrfs check only checks that the free space cache for the existing block
> groups is valid, it doesn't check for free space entries that don't have
> a corresponding block group.
>
> Fix this by checking for free space entries that don't have a
> corresponding block group. Additionally add a test image to validate
> this fix.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
> check/main.c | 79 ++++++++++++++++++
> .../corrupt-free-space-tree.img.xz | Bin 0 -> 1368 bytes
> 2 files changed, 79 insertions(+)
> create mode 100644 tests/fsck-tests/058-bad-free-space-tree-entry/corrupt-free-space-tree.img.xz
>
> diff --git a/check/main.c b/check/main.c
> index 4f7ab8b2..fb119bfe 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -5729,6 +5729,83 @@ static int verify_space_cache(struct btrfs_root *root,
> return ret;
> }
>
> +static int check_free_space_tree(struct btrfs_root *root)
> +{
> + struct btrfs_key key = {};
> + struct btrfs_path *path;
> + int ret = 0;
> +
> + path = btrfs_alloc_path();
> + if (!path)
> + return -ENOMEM;
> +
> + while (1) {
> + struct btrfs_block_group *bg;
> + u64 cur_start = key.objectid;
> +
> + ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> + if (ret < 0)
> + goto out;
> +
> + /*
> + * We should be landing on an item, so if we're above the
> + * nritems we know we hit the end of the tree.
> + */
> + if (path->slots[0] >= btrfs_header_nritems(path->nodes[0]))
> + break;
> +
> + btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> +
> + if (key.type != BTRFS_FREE_SPACE_INFO_KEY) {
> + fprintf(stderr, "Failed to find a space info key at %llu [%llu %u %llu]\n",
> + cur_start, key.objectid, key.type, key.offset);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + bg = btrfs_lookup_first_block_group(gfs_info, key.objectid);
> + if (!bg) {
> + fprintf(stderr, "We have a space info key for a block group that doesn't exist\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + btrfs_release_path(path);
> + key.objectid += key.offset;
> + key.offset = 0;
> + }
> + ret = 0;
> +out:
> + btrfs_free_path(path);
> + return ret;
> +}
> +
> +static int check_free_space_trees(struct btrfs_root *root)
> +{
> + struct btrfs_root *free_space_root;
> + struct rb_node *n;
> + struct btrfs_key key = {
> + .objectid = BTRFS_FREE_SPACE_TREE_OBJECTID,
> + .type = BTRFS_ROOT_ITEM_KEY,
> + .offset = 0,
> + };
> + int ret = 0;
> +
> + free_space_root = btrfs_global_root(gfs_info, &key);
> + while (1) {
> + ret = check_free_space_tree(free_space_root);
> + if (ret)
> + break;
> + n = rb_next(&root->rb_node);
> + if (!n)
> + break;
> + free_space_root = rb_entry(n, struct btrfs_root, rb_node);
> + if (root->root_key.objectid != BTRFS_FREE_SPACE_TREE_OBJECTID)
> + break;
> + }
> + return ret;
> +}
> +
> static int check_space_cache(struct btrfs_root *root)
> {
> struct extent_io_tree used;
> @@ -10121,6 +10198,8 @@ static int validate_free_space_cache(struct btrfs_root *root)
> }
>
> ret = check_space_cache(root);
> + if (!ret && btrfs_fs_compat_ro(gfs_info, FREE_SPACE_TREE))
> + ret = check_free_space_trees(root);
> if (ret && btrfs_fs_compat_ro(gfs_info, FREE_SPACE_TREE) &&
> repair) {
> ret = do_clear_free_space_cache(2);
> diff --git a/tests/fsck-tests/058-bad-free-space-tree-entry/corrupt-free-space-tree.img.xz b/tests/fsck-tests/058-bad-free-space-tree-entry/corrupt-free-space-tree.img.xz
> new file mode 100644
> index 0000000000000000000000000000000000000000..93280b82749c87183c8222740f15f6abca3d9bbf
> GIT binary patch
> literal 1368
> zcmV-e1*iJ`H+ooF000E$*0e?f03iVu0001VFXf}+Q~w1OT>wRyj;C3^v%$$4d1ocf
> zjjaF1$3^@a-*Xj3r-#5<9#Wqg%5NYWtuFyaX3{;HIsTkK=A=aNB3|l5$%+=o8g1{`
> z9>n_MeGBmNWMc2O8%=OwNuQkT%DC=u6%>3ias*)m2w2Vj{<X`j2OnKGMEZ%xR1cy`
> zv?-{L(P2Vkw0p<|MXz$0(HMoM8$Cs<!9}Ebp|h0L_u(K6y=q5|H5~9~lfr>K%U40+
> z_*HaH3>s3Be3<P8!YiHA`cw6c01&vER1Oyfl&3F9`o@wse1epc9c+bHS%moHfGbi0
> za>G|qceZM^9=!MAA{x7B*DdZw`=MZbP>g&&UJ78w*_xFJ*ymY);<Mbk211PVqr9pY
> z2$a*{$ddCoWl0v${~c<P0kv>=&F%~Em~&)LIgI!eG5|{o17mJxTHbL0LzMS}X7Rn(
> z8aG+sA}om=lMvs%yH^30B*fHO+M3JW%myi2Q@oISpjzVQL->PHN#F;kkTDP9m3d&G
> zATJRzI9%32{Q#bQRBPs+qO=%P(RJuP|KXmbBKz&TwxJq`c>bGV$H_w?rL#W~CBA&Y
> zTVrF&)Lgdkg=t8HuJHV%5gY<|+kT6=fq0y7i5Dq~A`8gp{)hlOigUZY*~kzbW*nyy
> zPy`p*ZYH)a7M@cM%W+<aS#c!NBv6M#k&B?|Gp>@{OMl<t$~kLNW5L+}or+3uMfZCa
> zPdfQpi-M`W(}|V|bGaH}H$OWtV5FtcB!t8htp{EN<x2hz!>(-bc0W+2Ou$DqMhHg0
> zAXXfJ)lD^OQ+ZsozgJ|ED^p@%Xk(nRRA=1*@@0`G!v_f=eu`$4g=+OcYHd?o<s{k<
> zxnc&Y__V)N3e2C_ckEzS<C+G7Nmfx90n%C0*tw3Kx?CrF*A4nznbUo#(uhw;yi{b&
> zUFA$8;MagqpNk9<F7lTE&f5YUW5zkYknyP}6_&g7maa>n#o^)Go`Vsk4wWALj%6xo
> zaptcSD`V9YyHG^8)iRj!Joyn!>AZaZYq|%`TqWRNiYO4%%UH-fOt<QXMD?{|hGUbp
> zrYY)<e8>}&<1eT_XhMooY9PF)hWGE81Aws&1!uWAsqM;Ila>-d{V}&$g0ds)zg&5W
> zy@oE>Vku@9rAuL@*wgdz7LuW?v&?fv^VEo(ELFU8)sSplTO|-_Q>g)KAl2D8@Wv4Y
> zy}hHu5?eK*DQHnJhNk1<@fb?xFkdSAlPxV;TKl{n;f9EkWjJt73KuO}-qG&G+*R)4
> zhwOeTHj!?vZ|(K2a(js@blT`!xJhTI{G8PmIgngOpf5HKr4%f_C@7a5O$XsR>`ph3
> zJz=xXUxXe6<DAbu*W&a8RFF;`p>>7gvsh$Ew3X_(ssfEfMDkER1cYqYxN3}-w)g0`
> z`SCbd(vQevQ;+U<8TtQb{OE;fs!)YC&?A1H9|)Br;s1wxo&K&9?UjEYW|mKVQq6tH
> zm(NruvFH7=%Q?*I)g6tb0-kV?<>qj-__Hc`D^QsGKI9`d+nL^1g*N?EU%c~9&I=R?
> zBW6edh^W^U!u{1k%mq`Fcp#QU4}(<wxlpXBWvSX#&huX4Ijqe+-iDRro04Xy+{kR)
> z(^+iDSztUr)`280d<1=uO{~1UM1%4>BZvQQ4zC#B;#@cKzqPk)OV4xGUgcG@RzDoN
> zvL!%GwW?EsbyiH<;7;8+b{X<zmZ8HfcL|V@C=a>Xdph+>?Zp590000pn{%NX14{`2
> a0kH~zs0jcD4TH0>#Ao{g000001X)^gHlHK_
>
> literal 0
> HcmV?d00001
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs-progs: check for invalid free space tree entries
2022-07-29 21:08 [PATCH] btrfs-progs: check for invalid free space tree entries Josef Bacik
2022-07-30 3:23 ` Qu Wenruo
2022-08-03 19:33 ` David Sterba
@ 2022-08-30 4:46 ` Qu Wenruo
2022-08-30 17:11 ` David Sterba
2 siblings, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2022-08-30 4:46 UTC (permalink / raw)
To: Josef Bacik, linux-btrfs, kernel-team, David Sterba
On 2022/7/30 05:08, Josef Bacik wrote:
> While testing some changes to how we reclaim block groups I started
> hitting failures with my TEST_DEV. This occurred because I had a bug
> and failed to properly remove a block groups free space tree entries.
> However this wasn't caught in testing when it happened because
> btrfs check only checks that the free space cache for the existing block
> groups is valid, it doesn't check for free space entries that don't have
> a corresponding block group.
>
> Fix this by checking for free space entries that don't have a
> corresponding block group. Additionally add a test image to validate
> this fix.
David, something is wrong with the patch in upstream, but not in the
patch in the mailing list.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> check/main.c | 79 ++++++++++++++++++
> .../corrupt-free-space-tree.img.xz | Bin 0 -> 1368 bytes
> 2 files changed, 79 insertions(+)
> create mode 100644 tests/fsck-tests/058-bad-free-space-tree-entry/corrupt-free-space-tree.img.xz
>
> diff --git a/check/main.c b/check/main.c
> index 4f7ab8b2..fb119bfe 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -5729,6 +5729,83 @@ static int verify_space_cache(struct btrfs_root *root,
> return ret;
> }
>
> +static int check_free_space_tree(struct btrfs_root *root)
> +{
> + struct btrfs_key key = {};
> + struct btrfs_path *path;
> + int ret = 0;
> +
> + path = btrfs_alloc_path();
> + if (!path)
> + return -ENOMEM;
> +
> + while (1) {
> + struct btrfs_block_group *bg;
> + u64 cur_start = key.objectid;
> +
> + ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> + if (ret < 0)
> + goto out;
Here, the original code just goto out tag, and there we call
btrfs_free_path() and free everything.
But in devel branch, we're using on-stack path, and out tag no longer
has the btrfs_release_path(), but just return.
This leaks the path, causing every valid btrfs to cause eb leakage:
[adam@btrfs-vm btrfs-progs]$ ./btrfs check ~/test.img
Opening filesystem to check...
Checking filesystem on /home/adam/test.img
UUID: 956ba692-d951-4c63-8c50-73541d01a6f3
[1/7] checking root items
[2/7] checking extents
[3/7] checking free space tree
[4/7] checking fs roots
[5/7] checking only csums items (without verifying data)
[6/7] checking root refs
[7/7] checking quota groups skipped (not enabled on this FS)
found 16461824 bytes used, no error found
total csum bytes: 13836
total tree bytes: 2293760
total fs tree bytes: 2129920
total extent tree bytes: 65536
btree space waste bytes: 441740
file data blocks allocated: 14168064
referenced 14168064
extent buffer leak: start 33062912 len 16384
In my case, above eb is the fst root node.
I'll send out a fix for it since it's already in the v5.19 release.
Thanks,
Qu
> +
> + /*
> + * We should be landing on an item, so if we're above the
> + * nritems we know we hit the end of the tree.
> + */
> + if (path->slots[0] >= btrfs_header_nritems(path->nodes[0]))
> + break;
> +
> + btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> +
> + if (key.type != BTRFS_FREE_SPACE_INFO_KEY) {
> + fprintf(stderr, "Failed to find a space info key at %llu [%llu %u %llu]\n",
> + cur_start, key.objectid, key.type, key.offset);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + bg = btrfs_lookup_first_block_group(gfs_info, key.objectid);
> + if (!bg) {
> + fprintf(stderr, "We have a space info key for a block group that doesn't exist\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + btrfs_release_path(path);
> + key.objectid += key.offset;
> + key.offset = 0;
> + }
> + ret = 0;
> +out:
> + btrfs_free_path(path);
> + return ret;
> +}
> +
> +static int check_free_space_trees(struct btrfs_root *root)
> +{
> + struct btrfs_root *free_space_root;
> + struct rb_node *n;
> + struct btrfs_key key = {
> + .objectid = BTRFS_FREE_SPACE_TREE_OBJECTID,
> + .type = BTRFS_ROOT_ITEM_KEY,
> + .offset = 0,
> + };
> + int ret = 0;
> +
> + free_space_root = btrfs_global_root(gfs_info, &key);
> + while (1) {
> + ret = check_free_space_tree(free_space_root);
> + if (ret)
> + break;
> + n = rb_next(&root->rb_node);
> + if (!n)
> + break;
> + free_space_root = rb_entry(n, struct btrfs_root, rb_node);
> + if (root->root_key.objectid != BTRFS_FREE_SPACE_TREE_OBJECTID)
> + break;
> + }
> + return ret;
> +}
> +
> static int check_space_cache(struct btrfs_root *root)
> {
> struct extent_io_tree used;
> @@ -10121,6 +10198,8 @@ static int validate_free_space_cache(struct btrfs_root *root)
> }
>
> ret = check_space_cache(root);
> + if (!ret && btrfs_fs_compat_ro(gfs_info, FREE_SPACE_TREE))
> + ret = check_free_space_trees(root);
> if (ret && btrfs_fs_compat_ro(gfs_info, FREE_SPACE_TREE) &&
> repair) {
> ret = do_clear_free_space_cache(2);
> diff --git a/tests/fsck-tests/058-bad-free-space-tree-entry/corrupt-free-space-tree.img.xz b/tests/fsck-tests/058-bad-free-space-tree-entry/corrupt-free-space-tree.img.xz
> new file mode 100644
> index 0000000000000000000000000000000000000000..93280b82749c87183c8222740f15f6abca3d9bbf
> GIT binary patch
> literal 1368
> zcmV-e1*iJ`H+ooF000E$*0e?f03iVu0001VFXf}+Q~w1OT>wRyj;C3^v%$$4d1ocf
> zjjaF1$3^@a-*Xj3r-#5<9#Wqg%5NYWtuFyaX3{;HIsTkK=A=aNB3|l5$%+=o8g1{`
> z9>n_MeGBmNWMc2O8%=OwNuQkT%DC=u6%>3ias*)m2w2Vj{<X`j2OnKGMEZ%xR1cy`
> zv?-{L(P2Vkw0p<|MXz$0(HMoM8$Cs<!9}Ebp|h0L_u(K6y=q5|H5~9~lfr>K%U40+
> z_*HaH3>s3Be3<P8!YiHA`cw6c01&vER1Oyfl&3F9`o@wse1epc9c+bHS%moHfGbi0
> za>G|qceZM^9=!MAA{x7B*DdZw`=MZbP>g&&UJ78w*_xFJ*ymY);<Mbk211PVqr9pY
> z2$a*{$ddCoWl0v${~c<P0kv>=&F%~Em~&)LIgI!eG5|{o17mJxTHbL0LzMS}X7Rn(
> z8aG+sA}om=lMvs%yH^30B*fHO+M3JW%myi2Q@oISpjzVQL->PHN#F;kkTDP9m3d&G
> zATJRzI9%32{Q#bQRBPs+qO=%P(RJuP|KXmbBKz&TwxJq`c>bGV$H_w?rL#W~CBA&Y
> zTVrF&)Lgdkg=t8HuJHV%5gY<|+kT6=fq0y7i5Dq~A`8gp{)hlOigUZY*~kzbW*nyy
> zPy`p*ZYH)a7M@cM%W+<aS#c!NBv6M#k&B?|Gp>@{OMl<t$~kLNW5L+}or+3uMfZCa
> zPdfQpi-M`W(}|V|bGaH}H$OWtV5FtcB!t8htp{EN<x2hz!>(-bc0W+2Ou$DqMhHg0
> zAXXfJ)lD^OQ+ZsozgJ|ED^p@%Xk(nRRA=1*@@0`G!v_f=eu`$4g=+OcYHd?o<s{k<
> zxnc&Y__V)N3e2C_ckEzS<C+G7Nmfx90n%C0*tw3Kx?CrF*A4nznbUo#(uhw;yi{b&
> zUFA$8;MagqpNk9<F7lTE&f5YUW5zkYknyP}6_&g7maa>n#o^)Go`Vsk4wWALj%6xo
> zaptcSD`V9YyHG^8)iRj!Joyn!>AZaZYq|%`TqWRNiYO4%%UH-fOt<QXMD?{|hGUbp
> zrYY)<e8>}&<1eT_XhMooY9PF)hWGE81Aws&1!uWAsqM;Ila>-d{V}&$g0ds)zg&5W
> zy@oE>Vku@9rAuL@*wgdz7LuW?v&?fv^VEo(ELFU8)sSplTO|-_Q>g)KAl2D8@Wv4Y
> zy}hHu5?eK*DQHnJhNk1<@fb?xFkdSAlPxV;TKl{n;f9EkWjJt73KuO}-qG&G+*R)4
> zhwOeTHj!?vZ|(K2a(js@blT`!xJhTI{G8PmIgngOpf5HKr4%f_C@7a5O$XsR>`ph3
> zJz=xXUxXe6<DAbu*W&a8RFF;`p>>7gvsh$Ew3X_(ssfEfMDkER1cYqYxN3}-w)g0`
> z`SCbd(vQevQ;+U<8TtQb{OE;fs!)YC&?A1H9|)Br;s1wxo&K&9?UjEYW|mKVQq6tH
> zm(NruvFH7=%Q?*I)g6tb0-kV?<>qj-__Hc`D^QsGKI9`d+nL^1g*N?EU%c~9&I=R?
> zBW6edh^W^U!u{1k%mq`Fcp#QU4}(<wxlpXBWvSX#&huX4Ijqe+-iDRro04Xy+{kR)
> z(^+iDSztUr)`280d<1=uO{~1UM1%4>BZvQQ4zC#B;#@cKzqPk)OV4xGUgcG@RzDoN
> zvL!%GwW?EsbyiH<;7;8+b{X<zmZ8HfcL|V@C=a>Xdph+>?Zp590000pn{%NX14{`2
> a0kH~zs0jcD4TH0>#Ao{g000001X)^gHlHK_
>
> literal 0
> HcmV?d00001
>
^ permalink raw reply [flat|nested] 5+ messages in thread