[PATCH 0/2] btrfs-progs: fix clone from wrong subvolume

[PATCH 0/2] btrfs-progs: fix clone from wrong subvolume

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Hi,

This series is a couple of minor fixes to btrfs receive. Patch 1 fixes a
missing strdup() return value check by getting rid of the strdup().
Patch 2 (the more important one) fixes a receive error when the same
subvolume is received multiple times.

Thanks,
Omar

Omar Sandoval (2):
  btrfs-progs: receive: get rid of unnecessary strdup()
  btrfs-progs: receive: don't lookup clone root for received subvolume

 cmds/receive.c | 41 +++++++++++------------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

-- 
2.22.0

[PATCH 1/2] btrfs-progs: receive: get rid of unnecessary strdup()

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

In process_clone(), we're not checking the return value of strdup().
But, there's no reason to strdup() in the first place: we just pass the
path into path_cat_out(). Get rid of the strdup().

Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")
Signed-off-by: Omar Sandoval <osandov@osandov.com>
---
 cmds/receive.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/cmds/receive.c b/cmds/receive.c
index b97850a7..a3e62985 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -739,7 +739,7 @@ static int process_clone(const char *path, u64 offset, u64 len,
 	struct btrfs_ioctl_clone_range_args clone_args;
 	struct subvol_info *si = NULL;
 	char full_path[PATH_MAX];
-	char *subvol_path = NULL;
+	char *subvol_path;
 	char full_clone_path[PATH_MAX];
 	int clone_fd = -1;
 
@@ -760,7 +760,7 @@ static int process_clone(const char *path, u64 offset, u64 len,
 		if (memcmp(clone_uuid, rctx->cur_subvol.received_uuid,
 				BTRFS_UUID_SIZE) == 0) {
 			/* TODO check generation of extent */
-			subvol_path = strdup(rctx->cur_subvol_path);
+			subvol_path = rctx->cur_subvol_path;
 		} else {
 			if (!si)
 				ret = -ENOENT;
@@ -794,14 +794,14 @@ static int process_clone(const char *path, u64 offset, u64 len,
 			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);
+				subvol_path = 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);
+			subvol_path = si->path;
 		}
 	}
 
@@ -839,7 +839,6 @@ out:
 		free(si->path);
 		free(si);
 	}
-	free(subvol_path);
 	if (clone_fd != -1)
 		close(clone_fd);
 	return ret;
-- 
2.22.0

Re: [PATCH 1/2] btrfs-progs: receive: get rid of unnecessary strdup()

[Omar Sandoval]

On Fri, Jul 19, 2019 at 10:40:00PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> In process_clone(), we're not checking the return value of strdup().
> But, there's no reason to strdup() in the first place: we just pass the
> path into path_cat_out(). Get rid of the strdup().
> 
> Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")
> Signed-off-by: Omar Sandoval <osandov@osandov.com>

Wrong SOB on this one, should be the usual

Signed-off-by: Omar Sandoval <osandov@fb.com>

Re: [PATCH 1/2] btrfs-progs: receive: get rid of unnecessary strdup()

[Mike Fleetwood]

On Sat, 20 Jul 2019 at 06:43, Omar Sandoval <osandov@osandov.com> wrote:
>
> From: Omar Sandoval <osandov@fb.com>
>
> In process_clone(), we're not checking the return value of strdup().
> But, there's no reason to strdup() in the first place: we just pass the
> path into path_cat_out(). Get rid of the strdup().
>
> Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")
> Signed-off-by: Omar Sandoval <osandov@osandov.com>
> ---
>  cmds/receive.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/cmds/receive.c b/cmds/receive.c
> index b97850a7..a3e62985 100644
> --- a/cmds/receive.c
> +++ b/cmds/receive.c
> @@ -739,7 +739,7 @@ static int process_clone(const char *path, u64 offset, u64 len,
>         struct btrfs_ioctl_clone_range_args clone_args;
>         struct subvol_info *si = NULL;
>         char full_path[PATH_MAX];
> -       char *subvol_path = NULL;
> +       char *subvol_path;
I think that should become const char *.

>         char full_clone_path[PATH_MAX];
>         int clone_fd = -1;
>
> @@ -760,7 +760,7 @@ static int process_clone(const char *path, u64 offset, u64 len,
>                 if (memcmp(clone_uuid, rctx->cur_subvol.received_uuid,
>                                 BTRFS_UUID_SIZE) == 0) {
>                         /* TODO check generation of extent */
> -                       subvol_path = strdup(rctx->cur_subvol_path);
> +                       subvol_path = rctx->cur_subvol_path;
>                 } else {
>                         if (!si)
>                                 ret = -ENOENT;
> @@ -794,14 +794,14 @@ static int process_clone(const char *path, u64 offset, u64 len,
>                         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);
> +                               subvol_path = 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);
> +                       subvol_path = si->path;
>                 }
>         }
>
> @@ -839,7 +839,6 @@ out:
>                 free(si->path);
>                 free(si);
>         }
> -       free(subvol_path);
>         if (clone_fd != -1)
>                 close(clone_fd);
>         return ret;
> --
> 2.22.0
>

Mike

Re: [PATCH 1/2] btrfs-progs: receive: get rid of unnecessary strdup()

[Omar Sandoval]

On Sat, Jul 20, 2019 at 09:34:24AM +0100, Mike Fleetwood wrote:
> On Sat, 20 Jul 2019 at 06:43, Omar Sandoval <osandov@osandov.com> wrote:
> >
> > From: Omar Sandoval <osandov@fb.com>
> >
> > In process_clone(), we're not checking the return value of strdup().
> > But, there's no reason to strdup() in the first place: we just pass the
> > path into path_cat_out(). Get rid of the strdup().
> >
> > Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")
> > Signed-off-by: Omar Sandoval <osandov@osandov.com>
> > ---
> >  cmds/receive.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/cmds/receive.c b/cmds/receive.c
> > index b97850a7..a3e62985 100644
> > --- a/cmds/receive.c
> > +++ b/cmds/receive.c
> > @@ -739,7 +739,7 @@ static int process_clone(const char *path, u64 offset, u64 len,
> >         struct btrfs_ioctl_clone_range_args clone_args;
> >         struct subvol_info *si = NULL;
> >         char full_path[PATH_MAX];
> > -       char *subvol_path = NULL;
> > +       char *subvol_path;
> I think that should become const char *.

Yeah, that wouldn't hurt. Dave, can you add that when you apply this or
should I resend?

[PATCH 2/2] btrfs-progs: receive: don't lookup clone root for received subvolume

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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 subvol 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 stream subvol
mkdir first second
btrfs receive -f stream first
btrfs receive -f stream 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. Also, while we're here, get rid of some code that has been
commented out since it was added.

Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 cmds/receive.c | 34 ++++++++--------------------------
 1 file changed, 8 insertions(+), 26 deletions(-)

diff --git a/cmds/receive.c b/cmds/receive.c
index a3e62985..ed089af2 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -753,15 +753,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
@@ -769,23 +768,6 @@ static int process_clone(const char *path, u64 offset, u64 len,
 			error("clone: did not find source subvol");
 			goto out;
 		}
-	} else {
-		/*if (rs_args.ctransid > rs_args.rtransid) {
-			if (!r->force) {
-				ret = -EINVAL;
-				fprintf(stderr, "ERROR: subvolume %s was "
-						"modified after it was "
-						"received.\n",
-						r->subvol_parent_name);
-				goto out;
-			} else {
-				fprintf(stderr, "WARNING: subvolume %s was "
-						"modified after it was "
-						"received.\n",
-						r->subvol_parent_name);
-			}
-		}*/
-
 		/* 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

Re: [PATCH 2/2] btrfs-progs: receive: don't lookup clone root for received subvolume

[Filipe Manana]

On Sat, Jul 20, 2019 at 3:34 PM Omar Sandoval <osandov@osandov.com> wrote:
>
> From: Omar Sandoval <osandov@fb.com>
>
> 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 subvol 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 stream subvol
> mkdir first second
> btrfs receive -f stream first
> btrfs receive -f stream 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. Also, while we're here, get rid of some code that has been
> commented out since it was added.
>
> Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  cmds/receive.c | 34 ++++++++--------------------------
>  1 file changed, 8 insertions(+), 26 deletions(-)
>
> diff --git a/cmds/receive.c b/cmds/receive.c
> index a3e62985..ed089af2 100644
> --- a/cmds/receive.c
> +++ b/cmds/receive.c
> @@ -753,15 +753,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)) {

Hi Omar,

This, and the change log, look good.

>                         if (!si)
>                                 ret = -ENOENT;
>                         else
> @@ -769,23 +768,6 @@ static int process_clone(const char *path, u64 offset, u64 len,
>                         error("clone: did not find source subvol");
>                         goto out;
>                 }
> -       } else {
> -               /*if (rs_args.ctransid > rs_args.rtransid) {
> -                       if (!r->force) {
> -                               ret = -EINVAL;
> -                               fprintf(stderr, "ERROR: subvolume %s was "
> -                                               "modified after it was "
> -                                               "received.\n",
> -                                               r->subvol_parent_name);
> -                               goto out;
> -                       } else {
> -                               fprintf(stderr, "WARNING: subvolume %s was "
> -                                               "modified after it was "
> -                                               "received.\n",
> -                                               r->subvol_parent_name);
> -                       }
> -               }*/
> -

Isn't this unrelated? Shouldn't go to a separate patch?

Also, would you please create a test case as well?

Thanks.

>                 /* 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
>


-- 
Filipe David Manana,

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

Re: [PATCH 2/2] btrfs-progs: receive: don't lookup clone root for received subvolume

[Omar Sandoval]

On Mon, Jul 22, 2019 at 01:16:35PM +0100, Filipe Manana wrote:
> On Sat, Jul 20, 2019 at 3:34 PM Omar Sandoval <osandov@osandov.com> wrote:
> >
> > From: Omar Sandoval <osandov@fb.com>
> >
> > 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 subvol 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 stream subvol
> > mkdir first second
> > btrfs receive -f stream first
> > btrfs receive -f stream 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. Also, while we're here, get rid of some code that has been
> > commented out since it was added.
> >
> > Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> >  cmds/receive.c | 34 ++++++++--------------------------
> >  1 file changed, 8 insertions(+), 26 deletions(-)
> >
> > diff --git a/cmds/receive.c b/cmds/receive.c
> > index a3e62985..ed089af2 100644
> > --- a/cmds/receive.c
> > +++ b/cmds/receive.c
> > @@ -753,15 +753,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)) {
> 
> Hi Omar,
> 
> This, and the change log, look good.
> 
> >                         if (!si)
> >                                 ret = -ENOENT;
> >                         else
> > @@ -769,23 +768,6 @@ static int process_clone(const char *path, u64 offset, u64 len,
> >                         error("clone: did not find source subvol");
> >                         goto out;
> >                 }
> > -       } else {
> > -               /*if (rs_args.ctransid > rs_args.rtransid) {
> > -                       if (!r->force) {
> > -                               ret = -EINVAL;
> > -                               fprintf(stderr, "ERROR: subvolume %s was "
> > -                                               "modified after it was "
> > -                                               "received.\n",
> > -                                               r->subvol_parent_name);
> > -                               goto out;
> > -                       } else {
> > -                               fprintf(stderr, "WARNING: subvolume %s was "
> > -                                               "modified after it was "
> > -                                               "received.\n",
> > -                                               r->subvol_parent_name);
> > -                       }
> > -               }*/
> > -
> 
> Isn't this unrelated? Shouldn't go to a separate patch?

It didn't seem worth it to make a separate patch when I'm moving this
code around anyways, but I noticed that there's a similar check in
another location, so I'll just make it a separate patch.

> Also, would you please create a test case as well?

Will do.

Thanks!