[PATCH 0/2] btrfs-progs: mkfs: extent-tree-v2 related fixes

[PATCH 0/2] btrfs-progs: mkfs: extent-tree-v2 related fixes

[Qu Wenruo]

Although recently we still have some uncertainty around the on-disk
format for extent-tree-v2, related to how to determine the number
of global roots, most of the on-disk format is fixed.

And even with the uncertain part involved, mkfs.btrfs should not crash
for extent-tree-v2 feature (hidden behind the experimental builds).

There are two bugs involved:

- A crash caused by incorrectly set chunk_objectid for block group item
  As extent-tree-v2 feature reuse that member to indicate which extent
  tree a block group belongs to.

  But the regular fs uses a fixed 256 for that chunk_objectid, and no
  extent-tree-v2 btrfs would have that many global roots.

  This leads to btrfs_extent_root() to return NULL, and cause later
  segfault.

  Fix it by properly setting chunk_objectid.
  This is a regression caused by 1430b41427b5 ("btrfs-progs: separate
  block group tree from extent tree v2").

- A stack-over-flow caused by too long feature string
  With extent-tree-v2 enabled, we have at least 84 bytes long feature
  string (unified features, including compat_ro features likle fst).

  This is beyond the hard-coded 64 bytes limit.

  Fix it by introducing a new macro to indicate a minimal safe buf size,
  and a sanity check to make sure that macro is really large enough.

Qu Wenruo (2):
  btrfs-progs: mkfs: fix a crash when enabling extent-tree-v2
  btrfs-progs: mkfs: fix a stack over-flow when features string are too
    long

 common/fsfeatures.c | 26 ++++++++++++++++++++++++++
 common/fsfeatures.h |  7 +++++++
 convert/main.c      |  3 ++-
 mkfs/common.c       | 14 ++++++++++++--
 mkfs/main.c         |  3 ++-
 5 files changed, 49 insertions(+), 4 deletions(-)

-- 
2.37.3

[PATCH 1/2] btrfs-progs: mkfs: fix a crash when enabling extent-tree-v2

[Qu Wenruo]

[BUG]
When enabling extent-tree-v2 feature at mkfs time (need to enable
experimental features), mkfs.btrfs will crash:

  # ./mkfs.btrfs  -f -O extent-tree-v2 ~/test.img
  btrfs-progs v5.19.1
  See http://btrfs.wiki.kernel.org for more information.

  ERROR: superblock magic doesn't match
  NOTE: several default settings have changed in version 5.15, please make sure
        this does not affect your deployments:
        - DUP for metadata (-m dup)
        - enabled no-holes (-O no-holes)
        - enabled free-space-tree (-R free-space-tree)

  Segmentation fault (core dumped)

[CAUSE]
The block group tree looks like this after make_btrfs() call:

  (gdb) call btrfs_print_tree(root->fs_info->block_group_root->node, 0)
  leaf 1163264 items 1 free space 16234 generation 1 owner BLOCK_GROUP_TREE
  leaf 1163264 flags 0x0() backref revision 1
  checksum stored f137c1ac
  checksum calced f137c1ac
  fs uuid 450d4b15-4954-4574-9801-8c6d248aaec6
  chunk uuid 4c4cc54d-f240-4aa4-b88b-bd487db43444
	item 0 key (1048576 BLOCK_GROUP_ITEM 4194304) itemoff 16259 itemsize 24
		block group used 131072 chunk_objectid 256 flags SYSTEM|single
						       ^^^

This looks completely sane, but notice that chunk_objectid 256.
That 256 value is the expected one for regular non-extent-tree-v2 btrfs,
but for extent-tree-v2, chunk_objectid is reused as the global id of
extent tree where the block group belongs to.

With the old 256 value as chunk_objectid, btrfs will not find an extent
tree root for the block group, and return NULL for btrfs_extent_root()
call, and trigger segfault.

This is a regression caused by commit 1430b41427b5 ("btrfs-progs:
separate block group tree from extent tree v2"), which doesn't take
extent-tree-v2 on-disk format into consideration.

[FIX]
For the initial btrfs created by make_btrfs(), all block group items
will be in extent-tree global id 0, thus we can reset chunk_objectid to
0, if and only if extent-tree-v2 is enabled.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 mkfs/common.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/mkfs/common.c b/mkfs/common.c
index 3a517a503e61..d77688ba584d 100644
--- a/mkfs/common.c
+++ b/mkfs/common.c
@@ -227,12 +227,22 @@ static int create_block_group_tree(int fd, struct btrfs_mkfs_config *cfg,
 				   u64 bg_offset, u64 bg_size, u64 bg_used)
 {
 	int ret;
+	u64 chunk_objectid = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
+
+	/*
+	 * For extent-tree-v2, chunk_objectid of block group item is reused
+	 * to indicate which extent-tree the block group is in.
+	 *
+	 * Thus for the initial image, we should set the chunk_objectid to 0,
+	 * as all initial bgs are in the extent tree with global id 0.
+	 */
+	if (cfg->features.incompat_flags & BTRFS_FEATURE_INCOMPAT_EXTENT_TREE_V2)
+		chunk_objectid = 0;
 
 	memset(buf->data + sizeof(struct btrfs_header), 0,
 		cfg->nodesize - sizeof(struct btrfs_header));
 	write_block_group_item(buf, 0, bg_offset, bg_size, bg_used,
-			       BTRFS_FIRST_CHUNK_TREE_OBJECTID,
-			       cfg->leaf_data_size -
+			       chunk_objectid, cfg->leaf_data_size -
 			       sizeof(struct btrfs_block_group_item));
 	btrfs_set_header_bytenr(buf, cfg->blocks[MKFS_BLOCK_GROUP_TREE]);
 	btrfs_set_header_owner(buf, BTRFS_BLOCK_GROUP_TREE_OBJECTID);
-- 
2.37.3

[PATCH 2/2] btrfs-progs: mkfs: fix a stack over-flow when features string are too long

[Qu Wenruo]

[BUG]
Even with chunk_objectid bug fixed, mkfs.btrfs can still caused stack
overflow when enabling extent-tree-v2 feature (need experimental
features enabled):

  # ./mkfs.btrfs  -f -O extent-tree-v2 ~/test.img
  btrfs-progs v5.19.1
  See http://btrfs.wiki.kernel.org for more information.

  ERROR: superblock magic doesn't match
  NOTE: several default settings have changed in version 5.15, please make sure
        this does not affect your deployments:
        - DUP for metadata (-m dup)
        - enabled no-holes (-O no-holes)
        - enabled free-space-tree (-R free-space-tree)

  Label:              (null)
  UUID:               205c61e7-f58e-4e8f-9dc2-38724f5c554b
  Node size:          16384
  Sector size:        4096
  Filesystem size:    512.00MiB
  Block group profiles:
    Data:             single            8.00MiB
    Metadata:         DUP              32.00MiB
    System:           DUP               8.00MiB
  SSD detected:       no
  Zoned device:       no
  =================================================================
  [... Skip full ASAN output ...]
  ==65655==ABORTING

[CAUSE]
For experimental build, we have unified feature output, but the old
buffer size is only 64 bytes, which is too small to cover the new full
feature string:

  extref, skinny-metadata, no-holes, free-space-tree, block-group-tree, extent-tree-v2

Above feature string is already 84 bytes, over the 64 on-stack memory
size.

This can also be proved by the ASAN output:

  ==65655==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffc4e03b1d0 at pc 0x7ff0fc05fafe bp 0x7ffc4e03ac60 sp 0x7ffc4e03a408
  WRITE of size 17 at 0x7ffc4e03b1d0 thread T0
      #0 0x7ff0fc05fafd in __interceptor_strcat /usr/src/debug/gcc/libsanitizer/asan/asan_interceptors.cpp:377
      #1 0x55cdb7b06ca5 in parse_features_to_string common/fsfeatures.c:316
      #2 0x55cdb7b06ce1 in btrfs_parse_fs_features_to_string common/fsfeatures.c:324
      #3 0x55cdb7a37226 in main mkfs/main.c:1783
      #4 0x7ff0fbe3c28f  (/usr/lib/libc.so.6+0x2328f)
      #5 0x7ff0fbe3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
      #6 0x55cdb7a2cb34 in _start ../sysdeps/x86_64/start.S:115

[FIX]
Introduce a new macro, BTRFS_FEATURE_STRING_BUF_SIZE, along with a new
sanity check helper, btrfs_assert_feature_buf_size().

The problem is I can not find a build time method to verify
BTRFS_FEATURE_STRING_BUF_SIZE is large enough to contain all feature
names, thus have to go the runtime function to do the BUG_ON() to verify
the macro size.

Now the minimal buffer size for experimental build is 138 bytes, just
bump it to 160 for future expansion.

And if further features go beyond that number, mkfs.btrfs/btrfs-convert
will immediately crash at that BUG_ON(), so we can definitely detect it.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 common/fsfeatures.c | 26 ++++++++++++++++++++++++++
 common/fsfeatures.h |  7 +++++++
 convert/main.c      |  3 ++-
 mkfs/main.c         |  3 ++-
 4 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/common/fsfeatures.c b/common/fsfeatures.c
index dc4b346c040a..e4334e3ea6c0 100644
--- a/common/fsfeatures.c
+++ b/common/fsfeatures.c
@@ -251,6 +251,32 @@ static const struct btrfs_feature runtime_features[] = {
 	}
 };
 
+/*
+ * This is a sanity check to make sure BTRFS_FEATURE_STRING_BUF_SIZE is large
+ * enough to contain all strings.
+ *
+ * All callers using btrfs_parse_*_features_to_string() should call this first.
+ */
+void btrfs_assert_feature_buf_size(void)
+{
+	int total_size = 0;
+	int i;
+
+	/*
+	 * This is a little over-calculated, as we include ", list-all".
+	 * But 10 extra bytes should not be a big deal.
+	 */
+	for (i = 0; i < ARRAY_SIZE(mkfs_features); i++)
+		/* The extra 2 bytes are for the ", " prefix. */
+		total_size += strlen(mkfs_features[i].name) + 2;
+	BUG_ON(BTRFS_FEATURE_STRING_BUF_SIZE < total_size);
+
+	total_size = 0;
+	for (i = 0; i < ARRAY_SIZE(runtime_features); i++)
+		total_size += strlen(runtime_features[i].name) + 2;
+	BUG_ON(BTRFS_FEATURE_STRING_BUF_SIZE < total_size);
+}
+
 static size_t get_feature_array_size(enum feature_source source)
 {
 	if (source == FS_FEATURES)
diff --git a/common/fsfeatures.h b/common/fsfeatures.h
index 3b5a915c6012..c4ab704862cd 100644
--- a/common/fsfeatures.h
+++ b/common/fsfeatures.h
@@ -37,6 +37,12 @@ struct btrfs_mkfs_features {
 #define BTRFS_FEATURE_RUNTIME_QUOTA		(1ULL << 0)
 #define BTRFS_FEATURE_RUNTIME_LIST_ALL		(1ULL << 1)
 
+/*
+ * Such buffer size should be able to contain all feature string, with extra
+ * ", " for each feature.
+ */
+#define BTRFS_FEATURE_STRING_BUF_SIZE		(160)
+
 static const struct btrfs_mkfs_features btrfs_mkfs_default_features = {
 	.compat_ro_flags = BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE |
 			   BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE_VALID,
@@ -86,5 +92,6 @@ int btrfs_check_sectorsize(u32 sectorsize);
 int btrfs_check_features(const struct btrfs_mkfs_features *features,
 			 const struct btrfs_mkfs_features *allowed);
 int btrfs_tree_search2_ioctl_supported(int fd);
+void btrfs_assert_feature_buf_size(void);
 
 #endif
diff --git a/convert/main.c b/convert/main.c
index 6bcb0f4876d0..c7be19f4e9bd 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -1147,7 +1147,7 @@ static int do_convert(const char *devname, u32 convert_flags, u32 nodesize,
 	struct btrfs_key key;
 	char subvol_name[SOURCE_FS_NAME_LEN + 8];
 	struct task_ctx ctx;
-	char features_buf[64];
+	char features_buf[BTRFS_FEATURE_STRING_BUF_SIZE];
 	char fsid_str[BTRFS_UUID_UNPARSED_SIZE];
 	struct btrfs_mkfs_config mkfs_cfg;
 	bool btrfs_sb_committed = false;
@@ -1835,6 +1835,7 @@ int BOX_MAIN(convert)(int argc, char *argv[])
 	char fsid[BTRFS_UUID_UNPARSED_SIZE] = {0};
 
 	crc32c_optimization_init();
+	btrfs_assert_feature_buf_size();
 	printf("btrfs-convert from %s\n\n", PACKAGE_STRING);
 
 	while(1) {
diff --git a/mkfs/main.c b/mkfs/main.c
index e5c1aa669828..c4a4e1986f9b 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -1028,6 +1028,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 
 	crc32c_optimization_init();
 	btrfs_config_init();
+	btrfs_assert_feature_buf_size();
 
 	while(1) {
 		int c;
@@ -1750,7 +1751,7 @@ raid_groups:
 		}
 	}
 	if (bconf.verbose) {
-		char features_buf[64];
+		char features_buf[BTRFS_FEATURE_STRING_BUF_SIZE];
 
 		update_chunk_allocation(fs_info, &allocation);
 		printf("Label:              %s\n", label);
-- 
2.37.3

Re: [PATCH 1/2] btrfs-progs: mkfs: fix a crash when enabling extent-tree-v2

[Anand Jain]

On 10/7/22 20:03, Qu Wenruo wrote:
> [BUG]
> When enabling extent-tree-v2 feature at mkfs time (need to enable
> experimental features), mkfs.btrfs will crash:
> 
>    # ./mkfs.btrfs  -f -O extent-tree-v2 ~/test.img
>    btrfs-progs v5.19.1
>    See http://btrfs.wiki.kernel.org for more information.
> 
>    ERROR: superblock magic doesn't match
>    NOTE: several default settings have changed in version 5.15, please make sure
>          this does not affect your deployments:
>          - DUP for metadata (-m dup)
>          - enabled no-holes (-O no-holes)
>          - enabled free-space-tree (-R free-space-tree)
> 
>    Segmentation fault (core dumped)
> 
> [CAUSE]
> The block group tree looks like this after make_btrfs() call:
> 
>    (gdb) call btrfs_print_tree(root->fs_info->block_group_root->node, 0)
>    leaf 1163264 items 1 free space 16234 generation 1 owner BLOCK_GROUP_TREE
>    leaf 1163264 flags 0x0() backref revision 1
>    checksum stored f137c1ac
>    checksum calced f137c1ac
>    fs uuid 450d4b15-4954-4574-9801-8c6d248aaec6
>    chunk uuid 4c4cc54d-f240-4aa4-b88b-bd487db43444
> 	item 0 key (1048576 BLOCK_GROUP_ITEM 4194304) itemoff 16259 itemsize 24
> 		block group used 131072 chunk_objectid 256 flags SYSTEM|single
> 						       ^^^
> 
> This looks completely sane, but notice that chunk_objectid 256.
> That 256 value is the expected one for regular non-extent-tree-v2 btrfs,
> but for extent-tree-v2, chunk_objectid is reused as the global id of
> extent tree where the block group belongs to.
> 
> With the old 256 value as chunk_objectid, btrfs will not find an extent
> tree root for the block group, and return NULL for btrfs_extent_root()
> call, and trigger segfault.
> 
> This is a regression caused by commit 1430b41427b5 ("btrfs-progs:
> separate block group tree from extent tree v2"), which doesn't take
> extent-tree-v2 on-disk format into consideration.
> 
> [FIX]
> For the initial btrfs created by make_btrfs(), all block group items
> will be in extent-tree global id 0, thus we can reset chunk_objectid to
> 0, if and only if extent-tree-v2 is enabled.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>


Reviewed-by: Anand Jain <anand.jain@oracle.com>

Tested-by: Anand Jain <anand.jain@oracle.com>

Re: [PATCH 2/2] btrfs-progs: mkfs: fix a stack over-flow when features string are too long

[Anand Jain]

On 10/7/22 20:03, Qu Wenruo wrote:
> [BUG]
> Even with chunk_objectid bug fixed, mkfs.btrfs can still caused stack
> overflow when enabling extent-tree-v2 feature (need experimental
> features enabled):
> 
>    # ./mkfs.btrfs  -f -O extent-tree-v2 ~/test.img
>    btrfs-progs v5.19.1
>    See http://btrfs.wiki.kernel.org for more information.
> 
>    ERROR: superblock magic doesn't match
>    NOTE: several default settings have changed in version 5.15, please make sure
>          this does not affect your deployments:
>          - DUP for metadata (-m dup)
>          - enabled no-holes (-O no-holes)
>          - enabled free-space-tree (-R free-space-tree)
> 
>    Label:              (null)
>    UUID:               205c61e7-f58e-4e8f-9dc2-38724f5c554b
>    Node size:          16384
>    Sector size:        4096
>    Filesystem size:    512.00MiB
>    Block group profiles:
>      Data:             single            8.00MiB
>      Metadata:         DUP              32.00MiB
>      System:           DUP               8.00MiB
>    SSD detected:       no
>    Zoned device:       no
>    =================================================================
>    [... Skip full ASAN output ...]
>    ==65655==ABORTING
> 
> [CAUSE]
> For experimental build, we have unified feature output, but the old
> buffer size is only 64 bytes, which is too small to cover the new full
> feature string:
> 
>    extref, skinny-metadata, no-holes, free-space-tree, block-group-tree, extent-tree-v2
> 
> Above feature string is already 84 bytes, over the 64 on-stack memory
> size.
> 
> This can also be proved by the ASAN output:
> 
>    ==65655==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffc4e03b1d0 at pc 0x7ff0fc05fafe bp 0x7ffc4e03ac60 sp 0x7ffc4e03a408
>    WRITE of size 17 at 0x7ffc4e03b1d0 thread T0
>        #0 0x7ff0fc05fafd in __interceptor_strcat /usr/src/debug/gcc/libsanitizer/asan/asan_interceptors.cpp:377
>        #1 0x55cdb7b06ca5 in parse_features_to_string common/fsfeatures.c:316
>        #2 0x55cdb7b06ce1 in btrfs_parse_fs_features_to_string common/fsfeatures.c:324
>        #3 0x55cdb7a37226 in main mkfs/main.c:1783
>        #4 0x7ff0fbe3c28f  (/usr/lib/libc.so.6+0x2328f)
>        #5 0x7ff0fbe3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
>        #6 0x55cdb7a2cb34 in _start ../sysdeps/x86_64/start.S:115
> 
> [FIX]
> Introduce a new macro, BTRFS_FEATURE_STRING_BUF_SIZE, along with a new
> sanity check helper, btrfs_assert_feature_buf_size().
> 
> The problem is I can not find a build time method to verify
> BTRFS_FEATURE_STRING_BUF_SIZE is large enough to contain all feature
> names, thus have to go the runtime function to do the BUG_ON() to verify
> the macro size.
> 
> Now the minimal buffer size for experimental build is 138 bytes, just
> bump it to 160 for future expansion.
> 
> And if further features go beyond that number, mkfs.btrfs/btrfs-convert
> will immediately crash at that BUG_ON(), so we can definitely detect it.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>


Reviewed-by: Anand Jain <anand.jain@oracle.com>
Tested-by: Anand Jain <anand.jain@oracle.com>

Re: [PATCH 0/2] btrfs-progs: mkfs: extent-tree-v2 related fixes

[David Sterba]

On Fri, Oct 07, 2022 at 08:02:59PM +0800, Qu Wenruo wrote:
> Although recently we still have some uncertainty around the on-disk
> format for extent-tree-v2, related to how to determine the number
> of global roots, most of the on-disk format is fixed.
> 
> And even with the uncertain part involved, mkfs.btrfs should not crash
> for extent-tree-v2 feature (hidden behind the experimental builds).
> 
> There are two bugs involved:
> 
> - A crash caused by incorrectly set chunk_objectid for block group item
>   As extent-tree-v2 feature reuse that member to indicate which extent
>   tree a block group belongs to.
> 
>   But the regular fs uses a fixed 256 for that chunk_objectid, and no
>   extent-tree-v2 btrfs would have that many global roots.
> 
>   This leads to btrfs_extent_root() to return NULL, and cause later
>   segfault.
> 
>   Fix it by properly setting chunk_objectid.
>   This is a regression caused by 1430b41427b5 ("btrfs-progs: separate
>   block group tree from extent tree v2").
> 
> - A stack-over-flow caused by too long feature string
>   With extent-tree-v2 enabled, we have at least 84 bytes long feature
>   string (unified features, including compat_ro features likle fst).
> 
>   This is beyond the hard-coded 64 bytes limit.
> 
>   Fix it by introducing a new macro to indicate a minimal safe buf size,
>   and a sanity check to make sure that macro is really large enough.
> 
> Qu Wenruo (2):
>   btrfs-progs: mkfs: fix a crash when enabling extent-tree-v2
>   btrfs-progs: mkfs: fix a stack over-flow when features string are too
>     long

Added to devel, thanks.