[PATCH 1/2] btrfs-progs: receive: better error reporting for snapshots

[PATCH 1/2] btrfs-progs: receive: better error reporting for snapshots

[Benedikt Morbach]

Two fixes:

1)

Check that the parent subvol actually is reachable via our root path.
The previous code wouldn't catch

    parent subvol: foo/bar
    root path:     bar   (i.e. mounted with -o subvol=bar)

where the parent isn't reachable from the root path.
(but the original "strstr(parent, root_path) == NULL" check still doesn't hold)

Also check for the slash after "root_path", i.e. throw an error on

    parent subvol: foobar
    root path:     foo

2)

If the parent subvol is the one that is mounted we obviously can't
receive into it, as it has to be read-only by definition.

We'd get a rather cryptic:

    At subvol /tmp/test/dest.snap
    At snapshot dest.snap
    ERROR: creating snapshot / -> dest.snap failed: Invalid cross-device link

(not sure what it says if "/" isn't even a btrfs)

But with this we get

    At subvol /tmp/test/dest.snap
    At snapshot dest.snap
    ERROR: creating snapshot . -> dest.snap failed: Read-only file system

which is both more helpful and more correct.

Signed-off-by: Benedikt Morbach <benedikt.morbach@googlemail.com>
---
 cmds-receive.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cmds-receive.c b/cmds-receive.c
index 166d37d..790218c 100644
--- a/cmds-receive.c
+++ b/cmds-receive.c
@@ -314,8 +314,8 @@ static int process_snapshot(const char *path, const u8 *uuid, u64 ctransid,
 		sub_len = strlen(parent_subvol->path);
 
 		/* First make sure the parent subvol is actually in our path */
-		if (sub_len < root_len ||
-		    strstr(parent_subvol->path, rctx->full_root_path) == NULL) {
+		if (strstr(parent_subvol->path, rctx->full_root_path) != parent_subvol->path ||
+		    sub_len > root_len && parent_subvol->path[root_len] != '/') {
 			error(
 		"parent subvol is not reachable from inside the root subvol");
 			ret = -ENOENT;
@@ -323,7 +323,7 @@ static int process_snapshot(const char *path, const u8 *uuid, u64 ctransid,
 		}
 
 		if (sub_len == root_len) {
-			parent_subvol->path[0] = '/';
+			parent_subvol->path[0] = '.';
 			parent_subvol->path[1] = '\0';
 		} else {
 			/*
-- 
2.11.1

[PATCH 2/2] btrfs-progs: receive: handle root subvol path in clone

[Benedikt Morbach]

testcase:
    # ro subvol /src/parent
    # rw subvol /src/foo
    clone /src/parent/file /src/foo/file
    subvol snapshot -r /src/foo /src/foo.snap

    # generates a "clone parent/file -> foo.snap/file" send command
    send -p /src/parent /src/foo.snap

    # target fs:
    #    dest/
    #        |--- parent/...
    # mounted with -o subvol=dest, such that "parent" is at <target>/parent
    receive <target>

result:
    ERROR: cannot open dest/parent/file: No such file or directory

expected:
    "dest/" get's stripped from the clone source path to get the actual
    path in the target fs, if reachable from the mount point/chroot.

    This is exactly what process_snapshot does, which gets called on
    _every_ incremental receive and I'm quite certain is correct in
    doing so

Signed-off-by: Benedikt Morbach <benedikt.morbach@googlemail.com>
---

Hi,

I first tried fixing this ages ago with [1], which was met with some scepticism.
While that patch wasn't 100% correct I believe this is, and as mentioned it does
exacly the same thing as process_snapshot because that has the exact same problem.

An fstest to reproduce this will be following shortly

[1] https://patchwork.kernel.org/patch/9177155/

 cmds-receive.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/cmds-receive.c b/cmds-receive.c
index 790218c..01345a4 100644
--- a/cmds-receive.c
+++ b/cmds-receive.c
@@ -782,7 +782,24 @@ static int process_clone(const char *path, u64 offset, u64 len,
 						r->subvol_parent_name);
 			}
 		}*/
-		subvol_path = strdup(si->path);
+
+		/* 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);
+			size_t sub_len = strlen(si->path);
+
+			if (sub_len > root_len &&
+			    strstr(si->path, rctx->full_root_path) == si->path &&
+			    si->path[root_len] == '/') {
+				subvol_path = strdup(si->path + root_len + 1);
+			} else {
+				error("clone: source subvol path %s unreachable from %s",
+					si->path, rctx->full_root_path);
+				goto out;
+			}
+		} else {
+			subvol_path = strdup(si->path);
+		}
 	}
 
 	ret = path_cat_out(full_clone_path, subvol_path, clone_path);
-- 
2.11.1

Re: [PATCH 2/2] btrfs-progs: receive: handle root subvol path in clone

[David Sterba]

On Wed, Feb 22, 2017 at 11:56:37PM +0100, Benedikt Morbach wrote:
> testcase:
>     # ro subvol /src/parent
>     # rw subvol /src/foo
>     clone /src/parent/file /src/foo/file
>     subvol snapshot -r /src/foo /src/foo.snap
> 
>     # generates a "clone parent/file -> foo.snap/file" send command
>     send -p /src/parent /src/foo.snap
> 
>     # target fs:
>     #    dest/
>     #        |--- parent/...
>     # mounted with -o subvol=dest, such that "parent" is at <target>/parent
>     receive <target>
> 
> result:
>     ERROR: cannot open dest/parent/file: No such file or directory
> 
> expected:
>     "dest/" get's stripped from the clone source path to get the actual
>     path in the target fs, if reachable from the mount point/chroot.
> 
>     This is exactly what process_snapshot does, which gets called on
>     _every_ incremental receive and I'm quite certain is correct in
>     doing so
> 
> Signed-off-by: Benedikt Morbach <benedikt.morbach@googlemail.com>

1-2 applied, thanks.

> ---
> 
> Hi,
> 
> I first tried fixing this ages ago with [1], which was met with some scepticism.
> While that patch wasn't 100% correct I believe this is, and as mentioned it does
> exacly the same thing as process_snapshot because that has the exact same problem.
> 
> An fstest to reproduce this will be following shortly

I'll add an adapted version of the test script to progs as I'd like to
be able to test any changes to receive from there.