[PATCH v2 0/4] btrfs-progs: small bug fixes

[PATCH v2 0/4] btrfs-progs: small bug fixes

[Qu Wenruo]

3 fixes for 3 github issues.

Meanwhile the last one fix a test failure which always fails on my VM
(cause by one bug inside the test case), but it never fails on the
github CI (as the kernel commits two transactions, causing slot change).

[Pull request]
https://github.com/kdave/btrfs-progs/pull/807

[Changelog]
v2:
- Concentrate all small fixes into a patchset
- Update the last patch to handle multiple transactions

Qu Wenruo (4):
  btrfs-progs: corrupt-block: fix memory leak in debug_corrupt_sector()
  btrfs-progs: print-tree: do sanity checks for dir items
  btrfs-progs: error out immediately if an unknown backref type is hit
  btrfs-progs: fix misc/038 test cases

 btrfs-corrupt-block.c                            |  7 ++++---
 kernel-shared/backref.c                          |  3 ++-
 kernel-shared/print-tree.c                       |  5 +++++
 .../038-backup-root-corruption/test.sh           | 16 ++++++++++++----
 4 files changed, 23 insertions(+), 8 deletions(-)

--
2.45.2

[PATCH v2 1/4] btrfs-progs: corrupt-block: fix memory leak in debug_corrupt_sector()

[Qu Wenruo]

ASAN build (make D=asan) would cause memory leak for
btrfs-corrupt-block inside debug_corrupt_sector().

This can be reproduced by fsck/013 test case.

The cause is pretty simple, we just malloc a sector and forgot to free
it.

Issue: #806
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 btrfs-corrupt-block.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 124597333771..e88319891910 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -70,7 +70,7 @@ static int debug_corrupt_sector(struct btrfs_root *root, u64 logical, int mirror
 			if (ret < 0) {
 				errno = -ret;
 				error("cannot read bytenr %llu: %m", logical);
-				return ret;
+				goto out;
 			}
 			printf("corrupting %llu copy %d\n", logical, mirror_num);
 			memset(buf, 0, sectorsize);
@@ -78,7 +78,7 @@ static int debug_corrupt_sector(struct btrfs_root *root, u64 logical, int mirror
 			if (ret < 0) {
 				errno = -ret;
 				error("cannot write bytenr %llu: %m", logical);
-				return ret;
+				goto out;
 			}
 		}
 
@@ -90,7 +90,8 @@ static int debug_corrupt_sector(struct btrfs_root *root, u64 logical, int mirror
 		if (mirror_num > num_copies)
 			break;
 	}
-
+out:
+	free(buf);
 	return 0;
 }
 
-- 
2.45.2

[PATCH v2 2/4] btrfs-progs: print-tree: do sanity checks for dir items

[Qu Wenruo]

There is a bug report that with UBSAN enabled, fuzz/006 test case would
crash.

It turns out that the image bko-154021-invalid-drop-level.raw has
invalid dir items, that the name/data len is beyond the item.

And if we try to read beyond the eb boundary, UBSAN got triggered.

Normally in kernel tree-checker would reject such metadata in the first
place, but in btrfs-progs we can not go that strict or we can not do a
lot of repair.

So here just enhance print_dir_item() to do extra sanity checks for
data/name len before reading the contents.

Issue: #805
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/print-tree.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
index 1b9386d87a0a..9a72ba39b426 100644
--- a/kernel-shared/print-tree.c
+++ b/kernel-shared/print-tree.c
@@ -78,6 +78,11 @@ static void print_dir_item(struct extent_buffer *eb, u32 size,
 		printf("\n");
 		name_len = btrfs_dir_name_len(eb, di);
 		data_len = btrfs_dir_data_len(eb, di);
+		if (data_len + name_len + cur > size) {
+			error("invalid length, cur=%u name_len=%u data_len=%u size=%u\n",
+				cur, name_len, data_len, size);
+			break;
+		}
 		len = (name_len <= sizeof(namebuf))? name_len: sizeof(namebuf);
 		printf("\t\ttransid %llu data_len %u name_len %u\n",
 				btrfs_dir_transid(eb, di),
-- 
2.45.2

[PATCH v2 3/4] btrfs-progs: error out immediately if an unknown backref type is hit

[Qu Wenruo]

There is a bug report that for fuzzed image
bko-155621-bad-block-group-offset.raw, "btrfs check --mode=lowmem
--repair" would lead to a deadloop.

Unlike original mode, lowmem mode relies on the backref walk to properly
go through each root, but unfortunately inside __add_inline_refs() we
doesn't handle unknown backref types correctly, causing it never moving
forward thus deadloop.

Fix it by erroring out to prevent deadloop.

Issue: #788
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/backref.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel-shared/backref.c b/kernel-shared/backref.c
index 89ccf073fca7..f46f3267e144 100644
--- a/kernel-shared/backref.c
+++ b/kernel-shared/backref.c
@@ -650,7 +650,8 @@ static int __add_inline_refs(struct btrfs_fs_info *fs_info,
 			break;
 		}
 		default:
-			WARN_ON(1);
+			error("invalid backref type: %u", type);
+			ret = -EUCLEAN;
 		}
 		if (ret)
 			return ret;
-- 
2.45.2

[PATCH v2 4/4] btrfs-progs: fix misc/038 test cases

[Qu Wenruo]

The test case always fail in my VM, with the following error:

 $ sudo TEST=038\* make test-misc
    [TEST]   misc-tests.sh
    [TEST/misc]   038-backup-root-corruption
 Backup 2 not overwritten
 test failed for case 038-backup-root-corruption

After more debugging, the it turns out that there is nothing wrong
except the final check:

 [ "$main_root_ptr" -ne "$backup_new_root_ptr" ] || _fail "Backup 2 not overwritten"

The _fail() is only triggered if the previous check returns false, which
is completely the opposite.

Furthermore on the github CI, the kernel would commit 2 instead of 1
transaction, resulting the next slot never to match the current
generation/tree root.

The two bugs combined, resulting github CI always pass the test case,
meanwhile for my VM which does the expected one transaction, it would
always fail.

Fix it by:

- Use a proper "if [] then; fi" block to check the tree root bytenr
- Use the generation diff to calculate the expected backup root slot
- Log the full super block dump for debug usage

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../038-backup-root-corruption/test.sh           | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tests/misc-tests/038-backup-root-corruption/test.sh b/tests/misc-tests/038-backup-root-corruption/test.sh
index 9be0cee36239..28aa5baec91e 100755
--- a/tests/misc-tests/038-backup-root-corruption/test.sh
+++ b/tests/misc-tests/038-backup-root-corruption/test.sh
@@ -41,6 +41,9 @@ slot_num=$(echo $found | cut -f1 -d:)
 # To follow the dump-super output, where backup slot starts at 0.
 slot_num=$(($slot_num - 1))
 
+_log "Original superblock:"
+_log "$(dump_super)"
+
 # Save the backup slot info into the log
 _log "Backup slot $slot_num will be utilized"
 dump_super | run_check grep -A9 "backup $slot_num:"
@@ -56,9 +59,14 @@ run_check_mount_test_dev -o usebackuproot
 run_check_umount_test_dev
 
 main_root_ptr=$(dump_super | awk '/^root\t/{print $2}')
-
-# The next slot should be overwritten
-slot_num=$(( ($slot_num + 1) % 4 ))
+cur_gen=$(dump_super | grep ^generation | awk '{print $2}')
+# The slot to be used is based on how many transaction committed.
+slot_num=$(( ($slot_num + $cur_gen - $backup_gen) % 4 ))
 backup_new_root_ptr=$(dump_super | grep -A1 "backup $slot_num" | grep backup_tree_root | awk '{print $2}')
 
-[ "$main_root_ptr" -ne "$backup_new_root_ptr" ] || _fail "Backup 2 not overwritten"
+_log "After the backup usage:"
+_log "$(dump_super)"
+
+if [ "$main_root_ptr" -ne "$backup_new_root_ptr" ]; then
+	_fail "Backup ${slot_num} not overwritten"
+fi
-- 
2.45.2

Re: [PATCH v2 0/4] btrfs-progs: small bug fixes

[David Sterba]

On Wed, Jun 05, 2024 at 09:13:40AM +0930, Qu Wenruo wrote:
> 3 fixes for 3 github issues.
> 
> Meanwhile the last one fix a test failure which always fails on my VM
> (cause by one bug inside the test case), but it never fails on the
> github CI (as the kernel commits two transactions, causing slot change).
> 
> [Pull request]
> https://github.com/kdave/btrfs-progs/pull/807
> 
> [Changelog]
> v2:
> - Concentrate all small fixes into a patchset
> - Update the last patch to handle multiple transactions
> 
> Qu Wenruo (4):
>   btrfs-progs: corrupt-block: fix memory leak in debug_corrupt_sector()
>   btrfs-progs: print-tree: do sanity checks for dir items
>   btrfs-progs: error out immediately if an unknown backref type is hit
>   btrfs-progs: fix misc/038 test cases

Added to devel, thanks.