[PATCH 0/2] btrfs-progs: fix the uuid report in "btrfs subvolume list -u"

[PATCH 0/2] btrfs-progs: fix the uuid report in "btrfs subvolume list -u"

[Qu Wenruo]

There is a regression caused by commit d729048be6ef ("btrfs-progs: stop
using btrfs_root_item_v0").

Just fix it and add a simple test case for it.

Qu Wenruo (2):
  btrfs-progs: fix the wrong timestamp and UUID check for root items
  btrfs-progs: misc-tests: add a test case to make sure uuid is
    correctly  reported

 cmds/subvolume-list.c                         | 11 ++++++--
 .../056-subvolume-list-uuid/test.sh           | 28 +++++++++++++++++++
 2 files changed, 37 insertions(+), 2 deletions(-)
 create mode 100755 tests/misc-tests/056-subvolume-list-uuid/test.sh

-- 
2.39.0

[PATCH 1/2] btrfs-progs: fix the wrong timestamp and UUID check for root items

[Qu Wenruo]

[BUG]
Since commit d729048be6ef ("btrfs-progs: stop using
btrfs_root_item_v0"), "btrfs subvolume list" not longer correctly report
UUID nor timestamp, while older (btrfs-progs v6.0.2) still works
correct:

 v6.0.2:
 # btrfs subv list -u  /mnt/btrfs/
 ID 256 gen 12 top level 5 uuid ed4af580-d512-2644-b392-2a71aaeeb99e path subv1
 ID 257 gen 13 top level 5 uuid a22ccba7-0a0a-a94f-af4b-5116ab58bb61 path subv2

 v6.1:
 # ./btrfs subv list -u /mnt/btrfs/
 ID 256 gen 12 top level 5 uuid -                                    path subv1
 ID 257 gen 13 top level 5 uuid -                                    path subv2

[CAUSE]
Commit d729048be6ef ("btrfs-progs: stop using btrfs_root_item_v0")
removed old btrfs_root_item_v0, but incorrectly changed the check for
v0 root item.

Now we will treat v0 root items as latest root items, causing possible
out-of-bound access. while treat current root items as older v0 root
items, ignoring the UUID nor timestamp.

[FIX]
Fix the bug by using correct checks, and add extra comments on the
branches.

Issue: #562
Fixes: d729048be6ef ("btrfs-progs: stop using btrfs_root_item_v0")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/subvolume-list.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/cmds/subvolume-list.c b/cmds/subvolume-list.c
index 6d5ef509ae67..7cdb0402b8e5 100644
--- a/cmds/subvolume-list.c
+++ b/cmds/subvolume-list.c
@@ -870,14 +870,21 @@ static int list_subvol_search(int fd, struct rb_root *root_lookup)
 				ri = (struct btrfs_root_item *)(args.buf + off);
 				gen = btrfs_root_generation(ri);
 				flags = btrfs_root_flags(ri);
-				if(sh.len <
-				   sizeof(struct btrfs_root_item)) {
+				if(sh.len >= sizeof(struct btrfs_root_item)) {
+					/*
+					 * The new full btrfs_root_item with
+					 * timestamp and UUID.
+					 */
 					otime = btrfs_stack_timespec_sec(&ri->otime);
 					ogen = btrfs_root_otransid(ri);
 					memcpy(uuid, ri->uuid, BTRFS_UUID_SIZE);
 					memcpy(puuid, ri->parent_uuid, BTRFS_UUID_SIZE);
 					memcpy(ruuid, ri->received_uuid, BTRFS_UUID_SIZE);
 				} else {
+					/*
+					 * The old v0 root item, which doesn't
+					 * has timestamp nor UUID.
+					 */
 					otime = 0;
 					ogen = 0;
 					memset(uuid, 0, BTRFS_UUID_SIZE);
-- 
2.39.0

Re: [PATCH 1/2] btrfs-progs: fix the wrong timestamp and UUID check for root items

[Anand Jain]

On 12/27/22 13:55, Qu Wenruo wrote:
> [BUG]
> Since commit d729048be6ef ("btrfs-progs: stop using
> btrfs_root_item_v0"), "btrfs subvolume list" not longer correctly report
> UUID nor timestamp, while older (btrfs-progs v6.0.2) still works
> correct:
> 
>   v6.0.2:
>   # btrfs subv list -u  /mnt/btrfs/
>   ID 256 gen 12 top level 5 uuid ed4af580-d512-2644-b392-2a71aaeeb99e path subv1
>   ID 257 gen 13 top level 5 uuid a22ccba7-0a0a-a94f-af4b-5116ab58bb61 path subv2
> 
>   v6.1:
>   # ./btrfs subv list -u /mnt/btrfs/
>   ID 256 gen 12 top level 5 uuid -                                    path subv1
>   ID 257 gen 13 top level 5 uuid -                                    path subv2
> 
> [CAUSE]
> Commit d729048be6ef ("btrfs-progs: stop using btrfs_root_item_v0")
> removed old btrfs_root_item_v0, but incorrectly changed the check for
> v0 root item.
> 
> Now we will treat v0 root items as latest root items, causing possible
> out-of-bound access. while treat current root items as older v0 root
> items, ignoring the UUID nor timestamp.
> 
> [FIX]
> Fix the bug by using correct checks, and add extra comments on the
> branches.
> 
> Issue: #562
> Fixes: d729048be6ef ("btrfs-progs: stop using btrfs_root_item_v0")
> Signed-off-by: Qu Wenruo <wqu@suse.com>



> ---
>   cmds/subvolume-list.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds/subvolume-list.c b/cmds/subvolume-list.c
> index 6d5ef509ae67..7cdb0402b8e5 100644
> --- a/cmds/subvolume-list.c
> +++ b/cmds/subvolume-list.c
> @@ -870,14 +870,21 @@ static int list_subvol_search(int fd, struct rb_root *root_lookup)
>   				ri = (struct btrfs_root_item *)(args.buf + off);
>   				gen = btrfs_root_generation(ri);
>   				flags = btrfs_root_flags(ri);
> -				if(sh.len <
> -				   sizeof(struct btrfs_root_item)) {
> +				if(sh.len >= sizeof(struct btrfs_root_item)) {
> +					/*
> +					 * The new full btrfs_root_item with
> +					 * timestamp and UUID.
> +					 */
>   					otime = btrfs_stack_timespec_sec(&ri->otime);
>   					ogen = btrfs_root_otransid(ri);
>   					memcpy(uuid, ri->uuid, BTRFS_UUID_SIZE);
>   					memcpy(puuid, ri->parent_uuid, BTRFS_UUID_SIZE);
>   					memcpy(ruuid, ri->received_uuid, BTRFS_UUID_SIZE);
>   				} else {
> +					/*
> +					 * The old v0 root item, which doesn't
> +					 * has timestamp nor UUID.
> +					 */

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


>   					otime = 0;
>   					ogen = 0;
>   					memset(uuid, 0, BTRFS_UUID_SIZE);

Re: [PATCH 1/2] btrfs-progs: fix the wrong timestamp and UUID check for root items

[Neal Gompa]

On Tue, Dec 27, 2022 at 1:10 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [BUG]
> Since commit d729048be6ef ("btrfs-progs: stop using
> btrfs_root_item_v0"), "btrfs subvolume list" not longer correctly report
> UUID nor timestamp, while older (btrfs-progs v6.0.2) still works
> correct:
>
>  v6.0.2:
>  # btrfs subv list -u  /mnt/btrfs/
>  ID 256 gen 12 top level 5 uuid ed4af580-d512-2644-b392-2a71aaeeb99e path subv1
>  ID 257 gen 13 top level 5 uuid a22ccba7-0a0a-a94f-af4b-5116ab58bb61 path subv2
>
>  v6.1:
>  # ./btrfs subv list -u /mnt/btrfs/
>  ID 256 gen 12 top level 5 uuid -                                    path subv1
>  ID 257 gen 13 top level 5 uuid -                                    path subv2
>
> [CAUSE]
> Commit d729048be6ef ("btrfs-progs: stop using btrfs_root_item_v0")
> removed old btrfs_root_item_v0, but incorrectly changed the check for
> v0 root item.
>
> Now we will treat v0 root items as latest root items, causing possible
> out-of-bound access. while treat current root items as older v0 root
> items, ignoring the UUID nor timestamp.
>
> [FIX]
> Fix the bug by using correct checks, and add extra comments on the
> branches.
>
> Issue: #562
> Fixes: d729048be6ef ("btrfs-progs: stop using btrfs_root_item_v0")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  cmds/subvolume-list.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/cmds/subvolume-list.c b/cmds/subvolume-list.c
> index 6d5ef509ae67..7cdb0402b8e5 100644
> --- a/cmds/subvolume-list.c
> +++ b/cmds/subvolume-list.c
> @@ -870,14 +870,21 @@ static int list_subvol_search(int fd, struct rb_root *root_lookup)
>                                 ri = (struct btrfs_root_item *)(args.buf + off);
>                                 gen = btrfs_root_generation(ri);
>                                 flags = btrfs_root_flags(ri);
> -                               if(sh.len <
> -                                  sizeof(struct btrfs_root_item)) {
> +                               if(sh.len >= sizeof(struct btrfs_root_item)) {
> +                                       /*
> +                                        * The new full btrfs_root_item with
> +                                        * timestamp and UUID.
> +                                        */
>                                         otime = btrfs_stack_timespec_sec(&ri->otime);
>                                         ogen = btrfs_root_otransid(ri);
>                                         memcpy(uuid, ri->uuid, BTRFS_UUID_SIZE);
>                                         memcpy(puuid, ri->parent_uuid, BTRFS_UUID_SIZE);
>                                         memcpy(ruuid, ri->received_uuid, BTRFS_UUID_SIZE);
>                                 } else {
> +                                       /*
> +                                        * The old v0 root item, which doesn't
> +                                        * has timestamp nor UUID.
> +                                        */
>                                         otime = 0;
>                                         ogen = 0;
>                                         memset(uuid, 0, BTRFS_UUID_SIZE);
> --
> 2.39.0
>

Resolves: rhbz#2156710

Reviewed-by: Neal Gompa <neal@gompa.dev>


-- 
真実はいつも一つ！/ Always, there's only one truth!

[PATCH 2/2] btrfs-progs: misc-tests: add a test case to make sure uuid is correctly reported

[Qu Wenruo]

The new test case will execute "btrfs subvolume list -u" on the newly
create btrfs.

Since the v0 root item is already deprecated for a long time, newly
created btrfs should be already using the new root item, thus "btrfs
subvolume list -u" should always report the correct uuid.

The test case relies on external program "uuidparse" which should be
provided by util-linux.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../056-subvolume-list-uuid/test.sh           | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100755 tests/misc-tests/056-subvolume-list-uuid/test.sh

diff --git a/tests/misc-tests/056-subvolume-list-uuid/test.sh b/tests/misc-tests/056-subvolume-list-uuid/test.sh
new file mode 100755
index 000000000000..45f4f956c25f
--- /dev/null
+++ b/tests/misc-tests/056-subvolume-list-uuid/test.sh
@@ -0,0 +1,28 @@
+#!/bin/bash
+#
+# Make sure "btrfs subvolume list -u" shows uuid correctly
+
+source "$TEST_TOP/common"
+
+check_prereq mkfs.btrfs
+check_prereq btrfs
+check_global_prereq uuidparse
+
+setup_root_helper
+prepare_test_dev
+
+tmp=$(_mktemp_dir list_uuid)
+
+run_check_mkfs_test_dev
+run_check_mount_test_dev
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1"
+run_check_stdout $SUDO_HELPER "$TOP/btrfs" subvolume list -u "$TEST_MNT" |\
+	cut -d\  -f9 > "$tmp/output"
+
+result=$(cat "$tmp/output" | uuidparse -o TYPE -n)
+rm -rf -- "$tmp"
+
+if [ "$result" == "invalid" ]; then
+	_fail "subvolume list failed to report uuid"
+fi
+run_check_umount_test_dev
-- 
2.39.0

Re: [PATCH 2/2] btrfs-progs: misc-tests: add a test case to make sure uuid is correctly reported

[Neal Gompa]

On Tue, Dec 27, 2022 at 1:10 AM Qu Wenruo <wqu@suse.com> wrote:
>
> The new test case will execute "btrfs subvolume list -u" on the newly
> create btrfs.
>
> Since the v0 root item is already deprecated for a long time, newly
> created btrfs should be already using the new root item, thus "btrfs
> subvolume list -u" should always report the correct uuid.
>
> The test case relies on external program "uuidparse" which should be
> provided by util-linux.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  .../056-subvolume-list-uuid/test.sh           | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100755 tests/misc-tests/056-subvolume-list-uuid/test.sh
>
> diff --git a/tests/misc-tests/056-subvolume-list-uuid/test.sh b/tests/misc-tests/056-subvolume-list-uuid/test.sh
> new file mode 100755
> index 000000000000..45f4f956c25f
> --- /dev/null
> +++ b/tests/misc-tests/056-subvolume-list-uuid/test.sh
> @@ -0,0 +1,28 @@
> +#!/bin/bash
> +#
> +# Make sure "btrfs subvolume list -u" shows uuid correctly
> +
> +source "$TEST_TOP/common"
> +
> +check_prereq mkfs.btrfs
> +check_prereq btrfs
> +check_global_prereq uuidparse
> +
> +setup_root_helper
> +prepare_test_dev
> +
> +tmp=$(_mktemp_dir list_uuid)
> +
> +run_check_mkfs_test_dev
> +run_check_mount_test_dev
> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1"
> +run_check_stdout $SUDO_HELPER "$TOP/btrfs" subvolume list -u "$TEST_MNT" |\
> +       cut -d\  -f9 > "$tmp/output"
> +
> +result=$(cat "$tmp/output" | uuidparse -o TYPE -n)
> +rm -rf -- "$tmp"
> +
> +if [ "$result" == "invalid" ]; then
> +       _fail "subvolume list failed to report uuid"
> +fi
> +run_check_umount_test_dev
> --
> 2.39.0
>

Reviewed-by: Neal Gompa <neal@gompa.dev>


-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH 0/2] btrfs-progs: fix the uuid report in "btrfs subvolume list -u"

[David Sterba]

On Tue, Dec 27, 2022 at 01:55:06PM +0800, Qu Wenruo wrote:
> There is a regression caused by commit d729048be6ef ("btrfs-progs: stop
> using btrfs_root_item_v0").
> 
> Just fix it and add a simple test case for it.
> 
> Qu Wenruo (2):
>   btrfs-progs: fix the wrong timestamp and UUID check for root items
>   btrfs-progs: misc-tests: add a test case to make sure uuid is
>     correctly  reported

Added to devel, thanks.