btrfs-progs: fix clone from wrong subvolume

From: Omar Sandoval <[email protected]>

When we process a clone request, we look up the source subvolume by UUID, even if the source is the subvolume that we're currently receiving. Usually, this is fine. However, if for some reason we previously received the same subvolume, then this will use paths relative to the previously received subvolume instead of the current one. This is incorrect, since the send stream may use temporary names for the clone source. This can be reproduced as follows:

btrfs subvolume create subvol
dd if=/dev/urandom of=subvol/foo bs=1M count=1
cp --reflink subvol/foo subvol/bar
mkdir subvol/dir
mv subvol/foo subvol/dir/
btrfs property set subvol ro true
btrfs send -f send.data subvol
mkdir first second
btrfs receive -f send.data first
btrfs receive -f send.data second

The second receive results in this error:

ERROR: cannot open first/subvol/o259-7-0/foo: No such file or directory

Fix it by always cloning from the current subvolume if its UUID matches. This has the nice side effect of avoiding unnecessary UUID tree lookups in that case.

Fixes f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")

Signed-off-by Omar Sandoval <[email protected]>

---
 cmds/receive.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

cmds/receive.c +8 -11

index dba05982..1e6a29dd 100644
@@ -744,15 +744,14 @@ static int process_clone(const char *path, u64 offset, u64 len,
        if (ret < 0)
                goto out;
 
-       si = subvol_uuid_search(&rctx->sus, 0, clone_uuid, clone_ctransid,
-                               NULL,
-                               subvol_search_by_received_uuid);
-       if (IS_ERR_OR_NULL(si)) {
-               if (memcmp(clone_uuid, rctx->cur_subvol.received_uuid,
-                               BTRFS_UUID_SIZE) == 0) {
-                       /* TODO check generation of extent */
-                       subvol_path = rctx->cur_subvol_path;
-               } else {
+       if (memcmp(clone_uuid, rctx->cur_subvol.received_uuid,
+                  BTRFS_UUID_SIZE) == 0) {
+               subvol_path = rctx->cur_subvol_path;
+       } else {
+               si = subvol_uuid_search(&rctx->sus, 0, clone_uuid, clone_ctransid,
+                                       NULL,
+                                       subvol_search_by_received_uuid);
+               if (IS_ERR_OR_NULL(si)) {
                        if (!si)
                                ret = -ENOENT;
                        else

@@ -760,7 +759,6 @@ static int process_clone(const char *path, u64 offset, u64 len,
                        error("clone: did not find source subvol");
                        goto out;
                }
-       } else {
                /* strip the subvolume that we are receiving to from the start of subvol_path */
                if (rctx->full_root_path) {
                        size_t root_len = strlen(rctx->full_root_path);
--

2.22.0

From: Omar Sandoval <[email protected]>

This test case is the reproducer for the previous fix.

Signed-off-by Omar Sandoval <[email protected]>

---
 .../test.sh                                   | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100755 tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh

tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh new file +34 -1

new file mode 100755
index 00000000..be648605
@@ -0,0 +1,34 @@
+#!/bin/bash
+# Test that when receiving a subvolume whose received UUID already exists in
+# the filesystem, we clone from the correct source (the subvolume that we are
+# receiving, not the existing subvolume). This is a regression test for
+# "btrfs-progs: receive: don't lookup clone root for received subvolume".
+
+source "$TEST_TOP/common"
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+
+setup_root_helper
+
+rm -f disk
+run_check truncate -s 1G disk
+chmod a+w disk
+run_check $SUDO_HELPER "$TOP/mkfs.btrfs" -f disk
+run_check $SUDO_HELPER mount -o loop disk "$TEST_MNT"
+
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subvol"
+run_check $SUDO_HELPER dd if=/dev/urandom of="$TEST_MNT/subvol/foo" \
+       bs=1M count=1 status=none
+run_check $SUDO_HELPER cp --reflink "$TEST_MNT/subvol/foo" "$TEST_MNT/subvol/bar"
+run_check $SUDO_HELPER mkdir "$TEST_MNT/subvol/dir"
+run_check $SUDO_HELPER mv "$TEST_MNT/subvol/foo" "$TEST_MNT/subvol/dir"
+run_check $SUDO_HELPER "$TOP/btrfs" property set "$TEST_MNT/subvol" ro true
+run_check $SUDO_HELPER "$TOP/btrfs" send -f send.data "$TEST_MNT/subvol"
+
+run_check $SUDO_HELPER mkdir "$TEST_MNT/first" "$TEST_MNT/second"
+run_check $SUDO_HELPER "$TOP/btrfs" receive -f send.data "$TEST_MNT/first"
+run_check $SUDO_HELPER "$TOP/btrfs" receive -f send.data "$TEST_MNT/second"
+
+run_check $SUDO_HELPER umount "$TEST_MNT"
+rm -f disk send.data
--

2.22.0

From: Omar Sandoval <[email protected]>

The checks for a subvolume being modified after it was received have been commented out since they were added back in commit f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands"). Let's just get rid of the noise.

Signed-off-by Omar Sandoval <[email protected]>

---
 cmds/receive.c | 25 -------------------------
 1 file changed, 25 deletions(-)

cmds/receive.c +0 -0

index b97850a7..830ed082 100644

Quoted 12 lines · Omar Sandoval

From: Omar Sandoval <[email protected]>

The checks for a subvolume being modified after it was received have been commented out since they were added back in commit f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands"). Let's just get rid of the noise.

Signed-off-by Omar Sandoval <[email protected]>

---
 cmds/receive.c | 25 -------------------------
 1 file changed, 25 deletions(-)

cmds/receive.c static int process_snapshot(const char *path, const u8 *uuid, u64 ctransid, · line 347

Quoted 9 lines

cmds/receive.c +0 -1

index b97850a7..830ed082 100644
@@ -344,15 +344,6 @@ static int process_snapshot(const char *path, const u8 *uuid, u64 ctransid,
                      parent_subvol->path[sub_len - root_len - 1] = '\0';
              }
      }
-     /*if (rs_args.ctransid > rs_args.rtransid) {

This looks like a (missing) sanity check, don't we want it?

From: Omar Sandoval <[email protected]>

This is v2 of [1]. Changes from v1:

- Split out removing commented-out code to new patch 1 and removed a related comment block.
- Made subvol_path const char * in patch 2.
- Added test case as patch 4.
- Fixed wrong signed-off-by.

Thanks,
Omar

1: https://lore.kernel.org/linux-btrfs/[email protected]/T/#u

Omar Sandoval (4):
btrfs-progs: receive: remove commented out transid checks
btrfs-progs: receive: get rid of unnecessary strdup()
btrfs-progs: receive: don't lookup clone root for received subvolume
btrfs-progs: tests: add test for receiving clone from duplicate
subvolume

 cmds/receive.c                                | 50 ++++---------------
 .../test.sh                                   | 34 +++++++++++++
 2 files changed, 45 insertions(+), 39 deletions(-)
 create mode 100755 tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh

Quoted 22 lines · Omar Sandoval

From: Omar Sandoval <[email protected]>

This is v2 of [1]. Changes from v1:

- Split out removing commented-out code to new patch 1 and removed a related comment block.
- Made subvol_path const char * in patch 2.
- Added test case as patch 4.
- Fixed wrong signed-off-by.

Thanks,
Omar

1: https://lore.kernel.org/linux-btrfs/[email protected]/T/#u

Omar Sandoval (4):
btrfs-progs: receive: remove commented out transid checks
btrfs-progs: receive: get rid of unnecessary strdup()
btrfs-progs: receive: don't lookup clone root for received subvolume
btrfs-progs: tests: add test for receiving clone from duplicate
subvolume

Quoted 4 lines

 cmds/receive.c                                | 50 ++++---------------
 .../test.sh                                   | 34 +++++++++++++
 2 files changed, 45 insertions(+), 39 deletions(-)
 create mode 100755 tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh

Ping. Looks like only patch 2 is in the devel branch.

Quoted 26 lines · Omar Sandoval

From: Omar Sandoval <[email protected]>

When we process a clone request, we look up the source subvolume by UUID, even if the source is the subvolume that we're currently receiving. Usually, this is fine. However, if for some reason we previously received the same subvolume, then this will use paths relative to the previously received subvolume instead of the current one. This is incorrect, since the send stream may use temporary names for the clone source. This can be reproduced as follows:

btrfs subvolume create subvol
dd if=/dev/urandom of=subvol/foo bs=1M count=1
cp --reflink subvol/foo subvol/bar
mkdir subvol/dir
mv subvol/foo subvol/dir/
btrfs property set subvol ro true
btrfs send -f send.data subvol
mkdir first second
btrfs receive -f send.data first
btrfs receive -f send.data second

The second receive results in this error:

ERROR: cannot open first/subvol/o259-7-0/foo: No such file or directory

Quoted 6 lines

Fix it by always cloning from the current subvolume if its UUID matches. This has the nice side effect of avoiding unnecessary UUID tree lookups in that case.

Fixes f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")

Signed-off-by Omar Sandoval <[email protected]>

Reviewed-by Filipe Manana <[email protected]>

Thanks!

Quoted 41 lines

---
 cmds/receive.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

cmds/receive.c +8 -11

index dba05982..1e6a29dd 100644
@@ -744,15 +744,14 @@ static int process_clone(const char *path, u64 offset, u64 len,
        if (ret < 0)
                goto out;
 
-       si = subvol_uuid_search(&rctx->sus, 0, clone_uuid, clone_ctransid,
-                               NULL,
-                               subvol_search_by_received_uuid);
-       if (IS_ERR_OR_NULL(si)) {
-               if (memcmp(clone_uuid, rctx->cur_subvol.received_uuid,
-                               BTRFS_UUID_SIZE) == 0) {
-                       /* TODO check generation of extent */
-                       subvol_path = rctx->cur_subvol_path;
-               } else {
+       if (memcmp(clone_uuid, rctx->cur_subvol.received_uuid,
+                  BTRFS_UUID_SIZE) == 0) {
+               subvol_path = rctx->cur_subvol_path;
+       } else {
+               si = subvol_uuid_search(&rctx->sus, 0, clone_uuid, clone_ctransid,
+                                       NULL,
+                                       subvol_search_by_received_uuid);
+               if (IS_ERR_OR_NULL(si)) {
                        if (!si)
                                ret = -ENOENT;
                        else

@@ -760,7 +759,6 @@ static int process_clone(const char *path, u64 offset, u64 len,
                        error("clone: did not find source subvol");
                        goto out;
                }
-       } else {
                /* strip the subvolume that we are receiving to from the start of subvol_path */
                if (rctx->full_root_path) {
                        size_t root_len = strlen(rctx->full_root_path);
--

Quoted 2 lines

2.22.0

--
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Quoted 32 lines · Filipe Manana

Quoted 30 lines · Omar Sandoval

From: Omar Sandoval <[email protected]>

When we process a clone request, we look up the source subvolume by UUID, even if the source is the subvolume that we're currently receiving. Usually, this is fine. However, if for some reason we previously received the same subvolume, then this will use paths relative to the previously received subvolume instead of the current one. This is incorrect, since the send stream may use temporary names for the clone source. This can be reproduced as follows:

btrfs subvolume create subvol
dd if=/dev/urandom of=subvol/foo bs=1M count=1
cp --reflink subvol/foo subvol/bar
mkdir subvol/dir
mv subvol/foo subvol/dir/
btrfs property set subvol ro true
btrfs send -f send.data subvol
mkdir first second
btrfs receive -f send.data first
btrfs receive -f send.data second

The second receive results in this error:

ERROR: cannot open first/subvol/o259-7-0/foo: No such file or directory

Fix it by always cloning from the current subvolume if its UUID matches. This has the nice side effect of avoiding unnecessary UUID tree lookups in that case.

Quoted 4 lines

Quoted 2 lines

Fixes f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")

Signed-off-by Omar Sandoval <[email protected]>

Reviewed-by Filipe Manana <[email protected]>

I can't find this patch in btrfs-progs. Any reason why it was never applied?

thanks