[PATCH 0/3] btrfs-progs: make convert to generate chunks aligned to stripe boundary

[PATCH 0/3] btrfs-progs: make convert to generate chunks aligned to stripe boundary

[Qu Wenruo]

There is a recent report about scrub use-after-free, which is caused by
unaligned chunk length (only aligned to sectorsize, but not to
BTRFS_STRIPE_LEN).

Although the bug would soon be fixed in kernel, there is no hard to make
convert to generate data chunks with both start and length aligned to
BTRFS_STRIPE_LEN.

Thankfully the start bytenr is already aligned to 64K, we only need to
make the length aligned.

Furthermore, allow "btrfs check" to detect such unaligned chunks and
gives a warning (but not consider it as an error).
For selftests, we would utilize the debug environment variable,
BTRFS_PROGS_DEBUG_STRICT_CHUNK_ALIGNMENT, to convert the warning to an
error.

Qu Wenruo (3):
  btrfs-progs: convert: make sure the length of data chunks are also
    stripe aligned
  btrfs-progs: add extra chunk alignment checks
  btrfs-progs: tests: enable strict chunk alignment check

 check/common.h       |  1 +
 check/main.c         | 20 ++++++++++++++++++++
 check/mode-lowmem.c  | 11 +++++++++++
 common/utils.c       | 19 +++++++++++++++++++
 common/utils.h       |  1 +
 convert/main.c       |  3 ++-
 tests/common         |  2 ++
 tests/common.convert |  2 ++
 8 files changed, 58 insertions(+), 1 deletion(-)

--
2.43.0

[PATCH 1/3] btrfs-progs: convert: make sure the length of data chunks are also stripe aligned

[Qu Wenruo]

Although scrub code is updated to handle the unaligned chunk length,
there is also no harm if we can alloc data chunk with both start and
length aligned.

This patch would handle this by rounding up the end bytenr when
allocating data chunks for the conversion.

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

diff --git a/convert/main.c b/convert/main.c
index c9e50c036f92..77b7c0516ae5 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -984,7 +984,8 @@ static int make_convert_data_block_groups(struct btrfs_trans_handle *trans,
 			u64 cur_backup = cur;
 
 			len = min(max_chunk_size,
-				  cache->start + cache->size - cur);
+				  round_up(cache->start + cache->size,
+					   BTRFS_STRIPE_LEN) - cur);
 			ret = btrfs_alloc_data_chunk(trans, fs_info, &cur_backup, len);
 			if (ret < 0)
 				break;
-- 
2.43.0

[PATCH 2/3] btrfs-progs: add extra chunk alignment checks

[Qu Wenruo]

Recently we have a scrub use-after-free caused by unaligned chunk
length, although the fix is submitted, we may want to do extra checks
for a chunk's alignment.

This patch would add such check for the starting bytenr and length of a
chunk, to make sure they are properly aligned to 64K stripe boundary.

By default, the check would only lead to a warning but not treated as an
error, as we expect kernel to handle such unalignment without any
problem.

But if the new debug environmental variable,
BTRFS_PROGS_DEBUG_STRICT_CHUNK_ALIGNMENT, is specified, then we would
treat it as an error.
So that we can detect unexpected chunks from btrfs-progs, and fix them
before reaching the end users.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/common.h      |  1 +
 check/main.c        | 20 ++++++++++++++++++++
 check/mode-lowmem.c | 11 +++++++++++
 common/utils.c      | 19 +++++++++++++++++++
 common/utils.h      |  1 +
 5 files changed, 52 insertions(+)

diff --git a/check/common.h b/check/common.h
index 2d5db2131ba5..d1c8e8de4af9 100644
--- a/check/common.h
+++ b/check/common.h
@@ -78,6 +78,7 @@ struct chunk_record {
 	u32 io_align;
 	u32 io_width;
 	u32 sector_size;
+	bool unaligned;
 	struct stripe stripes[0];
 };
 
diff --git a/check/main.c b/check/main.c
index 108818688132..99643f0ad7e0 100644
--- a/check/main.c
+++ b/check/main.c
@@ -5265,6 +5265,9 @@ struct chunk_record *btrfs_new_chunk_record(struct extent_buffer *leaf,
 	rec->num_stripes = num_stripes;
 	rec->sub_stripes = btrfs_chunk_sub_stripes(leaf, ptr);
 
+	if (!IS_ALIGNED(rec->cache.start, BTRFS_STRIPE_LEN)||
+	    !IS_ALIGNED(rec->cache.size, BTRFS_STRIPE_LEN))
+		rec->unaligned = true;
 	for (i = 0; i < rec->num_stripes; ++i) {
 		rec->stripes[i].devid =
 			btrfs_stripe_devid_nr(leaf, ptr, i);
@@ -8386,6 +8389,7 @@ int check_chunks(struct cache_tree *chunk_cache,
 	struct chunk_record *chunk_rec;
 	struct block_group_record *bg_rec;
 	struct device_extent_record *dext_rec;
+	bool strict_alignment = get_env_bool("BTRFS_DEBUG_STRICT_CHUNK_ALIGNMENT");
 	int err;
 	int ret = 0;
 
@@ -8393,6 +8397,22 @@ int check_chunks(struct cache_tree *chunk_cache,
 	while (chunk_item) {
 		chunk_rec = container_of(chunk_item, struct chunk_record,
 					 cache);
+		if (chunk_rec->unaligned && !silent) {
+			if (strict_alignment) {
+				error(
+		"chunk[%llu %llu) is not fully aligned to BTRFS_STRIPE_LEN(%u)",
+				      chunk_rec->cache.start,
+				      chunk_rec->cache.start + chunk_rec->cache.size,
+				      BTRFS_STRIPE_LEN);
+				ret = -EINVAL;
+			} else {
+				warning(
+		"chunk[%llu %llu) is not fully aligned to BTRFS_STRIPE_LEN(%u)",
+				      chunk_rec->cache.start,
+				      chunk_rec->cache.start + chunk_rec->cache.size,
+				      BTRFS_STRIPE_LEN);
+			}
+		}
 		err = check_chunk_refs(chunk_rec, block_group_cache,
 				       dev_extent_cache, silent);
 		if (err < 0)
diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 19b7b1a72a1f..39944c5430ec 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -4719,6 +4719,17 @@ static int check_chunk_item(struct extent_buffer *eb, int slot)
 	chunk = btrfs_item_ptr(eb, slot, struct btrfs_chunk);
 	length = btrfs_chunk_length(eb, chunk);
 	chunk_end = chunk_key.offset + length;
+	if (!IS_ALIGNED(chunk_key.offset, BTRFS_STRIPE_LEN) ||
+	    !IS_ALIGNED(length, BTRFS_STRIPE_LEN)) {
+		if (get_env_bool("BTRFS_PROGS_DEBUG_STRICT_CHUNK_ALIGNMENT")) {
+			error("chunk[%llu %llu) is not fully aligned to BTRFS_STRIPE_LEN(%u)",
+				chunk_key.offset, length, BTRFS_STRIPE_LEN);
+			err |= BYTES_UNALIGNED;
+			goto out;
+		}
+		warning("chunk[%llu %llu) is not fully aligned to BTRFS_STRIPE_LEN(%u)",
+			chunk_key.offset, length, BTRFS_STRIPE_LEN);
+	}
 	ret = btrfs_check_chunk_valid(eb, chunk, chunk_key.offset);
 	if (ret < 0) {
 		error("chunk[%llu %llu) is invalid", chunk_key.offset,
diff --git a/common/utils.c b/common/utils.c
index e6070791f5cc..68fa95ece6f8 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -1018,6 +1018,25 @@ int get_env_u64(const char *env_name, u64 *value_ret)
 	return 0;
 }
 
+/*
+ * Parse a boolean value from an environment variable.
+ *
+ * As long as the environment variable is not set to "0", "n" or "\0",
+ * it would return true.
+ */
+bool get_env_bool(const char *env_name)
+{
+	char *env_value_str;
+
+	env_value_str = getenv(env_name);
+	if (!env_value_str)
+		return false;
+	if (env_value_str[0] == '0' || env_value_str[0] == 'n' ||
+	    env_value_str[0] == '\0')
+		return false;
+	return true;
+}
+
 void btrfs_config_init(void)
 {
 	bconf.output_format = CMD_FORMAT_TEXT;
diff --git a/common/utils.h b/common/utils.h
index 30c75339b05b..5bdeeab44bb4 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -133,6 +133,7 @@ u64 rand_u64(void);
 unsigned int rand_range(unsigned int upper);
 void init_rand_seed(u64 seed);
 int get_env_u64(const char *env_name, u64 *value_ret);
+bool get_env_bool(const char *env_name);
 
 char *btrfs_test_for_multiple_profiles(int fd);
 int btrfs_warn_multiple_profiles(int fd);
-- 
2.43.0

[PATCH 3/3] btrfs-progs: tests: enable strict chunk alignment check

[Qu Wenruo]

The strict check is enabled for both check_image() and
convert_test_do_convert() to detect chunk alignment related problems.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/common         | 2 ++
 tests/common.convert | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/tests/common b/tests/common
index 771acaa339e8..1f880adead6d 100644
--- a/tests/common
+++ b/tests/common
@@ -441,6 +441,7 @@ check_image()
 
 	image=$1
 	echo "testing image $(basename $image)" >> "$RESULTS"
+	export BTRFS_PROGS_DEBUG_STRICT_CHUNK_ALIGNMENT=y
 	run_mustfail_stdout "btrfs check should have detected corruption"	\
 		"$TOP/btrfs" check "$image" &> "$tmp_output"
 
@@ -449,6 +450,7 @@ check_image()
 
 	run_check "$TOP/btrfs" check --repair --force "$image"
 	run_check_stdout "$TOP/btrfs" check "$image" &> "$tmp_output"
+	unset BTRFS_PROGS_DEBUG_STRICT_CHUNK_ALIGNMENT
 
 	# Also make sure no subpage related warnings for the repaired image
 	check_test_results "$tmp_output" "$testname"
diff --git a/tests/common.convert b/tests/common.convert
index 9b498ef29f1f..7d1d333895fa 100644
--- a/tests/common.convert
+++ b/tests/common.convert
@@ -147,9 +147,11 @@ convert_test_acl() {
 # $1: features, argument of -O, can be empty
 # $2: nodesize, argument of -N, can be empty
 convert_test_do_convert() {
+	export BTRFS_PROGS_DEBUG_STRICT_CHUNK_ALIGNMENT=y
 	run_check "$TOP/btrfs-convert" ${1:+-O "$1"} ${2:+-N "$2"} "$TEST_DEV"
 	run_check "$TOP/btrfs" check "$TEST_DEV"
 	run_check "$TOP/btrfs" inspect-internal dump-super -Ffa "$TEST_DEV"
+	unset BTRFS_PROGS_DEBUG_STRICT_CHUNK_ALIGNMENT
 }
 
 # post conversion check, verify file permissions.
-- 
2.43.0

Re: [PATCH 0/3] btrfs-progs: make convert to generate chunks aligned to stripe boundary

[David Sterba]

On Tue, Jan 16, 2024 at 02:01:23PM +1030, Qu Wenruo wrote:
> There is a recent report about scrub use-after-free, which is caused by
> unaligned chunk length (only aligned to sectorsize, but not to
> BTRFS_STRIPE_LEN).
> 
> Although the bug would soon be fixed in kernel, there is no hard to make
> convert to generate data chunks with both start and length aligned to
> BTRFS_STRIPE_LEN.
> 
> Thankfully the start bytenr is already aligned to 64K, we only need to
> make the length aligned.
> 
> Furthermore, allow "btrfs check" to detect such unaligned chunks and
> gives a warning (but not consider it as an error).
> For selftests, we would utilize the debug environment variable,
> BTRFS_PROGS_DEBUG_STRICT_CHUNK_ALIGNMENT, to convert the warning to an
> error.
> 
> Qu Wenruo (3):
>   btrfs-progs: convert: make sure the length of data chunks are also
>     stripe aligned
>   btrfs-progs: add extra chunk alignment checks
>   btrfs-progs: tests: enable strict chunk alignment check

Added to devel, with some minor fixups, thanks. The environment variable
name is long but it's just for us and it's descriptive, so OK.