[PATCH 0/5] Subvolume ro/rw and received_uuid

[PATCH 0/5] Subvolume ro/rw and received_uuid

[David Sterba]

Here's implemented the userspace prevention of accidental ro->rw switch
of sent subvolumes. It's not finialized but the main idea is there so
here we go argue, as we haven't reached a consensus with Nikolay what is
the right usability approach.

There are two patches extending the printed information about root items
and subvolumes but they're only for convenience, that I also used for
debugging.

David Sterba (5):
  btrfs-progs: subvol show: print send and receive generation and
    timestamp
  btrfs-progs: dump-tree: print complete root_item
  btrfs-progs: props: add force parameter to set
  btrfs-progs: property: ro->rw and received_uuid
  btrfs-progs: tests: subvolume ro->rw switch and received_uuid

 cmds/property.c                               | 99 ++++++++++++++++---
 cmds/props.h                                  |  3 +-
 cmds/subvolume.c                              | 21 ++++
 kernel-shared/print-tree.c                    | 53 +++++-----
 .../050-receive-prop-ro-to-rw/test.sh         | 42 ++++++++
 5 files changed, 172 insertions(+), 46 deletions(-)
 create mode 100755 tests/misc-tests/050-receive-prop-ro-to-rw/test.sh

-- 
2.33.0

[PATCH 1/5] btrfs-progs: subvol show: print send and receive generation and timestamp

[David Sterba]

There are some send/receive related data not printed in subvol show,
while they're exported by the ioctls. Print them for convenience:

  $ btrfs subvol show test
  test
	  Name: 		test
	  UUID: 		dc16dd1b-825f-3245-94a8-557672d6cf85
	  Parent UUID: 		-
	  Received UUID: 	-
	  Creation time: 	2021-05-17 16:17:14 +0200
	  Subvolume ID: 	19112
	  Generation: 		7730702
	  Gen at creation:	7730701
	  Parent ID: 		5
	  Top level ID: 	5
	  Flags: 		-
	  Send transid: 	0
	  Send time: 		2021-05-17 16:17:14 +0200
	  Recv transid: 	0
	  Recv time: 		-
	  Snapshot(s):
				  test-snap

Signed-off-by: David Sterba <dsterba@suse.com>
---
 cmds/subvolume.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index 0b00f9601ebe..3d8db249d376 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -1411,6 +1411,27 @@ static int cmd_subvol_show(const struct cmd_struct *cmd, int argc, char **argv)
 	else
 		printf("\tFlags: \t\t\t-\n");
 
+	printf("\tSend transid: \t\t%" PRIu64 "\n", subvol.stransid);
+	printf("\tSend time: \t\t%s\n", tstr);
+	if (subvol.stime.tv_sec) {
+		struct tm tm;
+
+		localtime_r(&subvol.stime.tv_sec, &tm);
+		strftime(tstr, 256, "%Y-%m-%d %X %z", &tm);
+	} else {
+		strcpy(tstr, "-");
+	}
+	printf("\tRecv transid: \t\t%" PRIu64 "\n", subvol.rtransid);
+	if (subvol.rtime.tv_sec) {
+		struct tm tm;
+
+		localtime_r(&subvol.rtime.tv_sec, &tm);
+		strftime(tstr, 256, "%Y-%m-%d %X %z", &tm);
+	} else {
+		strcpy(tstr, "-");
+	}
+	printf("\tRecv time: \t\t%s\n", tstr);
+
 	/* print the snapshots of the given subvol if any*/
 	printf("\tSnapshot(s):\n");
 
-- 
2.33.0

[PATCH 2/5] btrfs-progs: dump-tree: print complete root_item

[David Sterba]

The output of root_item in the 'inspect dump-tree' command lacks some
items and some of them are printed conditionally. As the dump utility
is for debugging, it's better to print all the items, with names
matching the structure members and order.

Some values will inevitably be all zeros like uuids or various
timestamps, but that's a minor issue and affecting only a few trees.

Example:

  item 0 key (EXTENT_TREE ROOT_ITEM 0) itemoff 15844 itemsize 439
	  generation 5 root_dirid 0 bytenr 30523392 byte_limit 0 bytes_used 16384
	  last_snapshot 0 flags 0x0(none) refs 1
	  drop_progress key (0 UNKNOWN.0 0) drop_level 0
	  level 0 generation_v2 5
	  uuid 00000000-0000-0000-0000-000000000000
	  parent_uuid 00000000-0000-0000-0000-000000000000
	  received_uuid 00000000-0000-0000-0000-000000000000
	  ctransid 0 otransid 0 stransid 0 rtransid 0
	  ctime 0.0 (1970-01-01 01:00:00)
	  otime 0.0 (1970-01-01 01:00:00)
	  stime 0.0 (1970-01-01 01:00:00)
	  rtime 0.0 (1970-01-01 01:00:00)

  item 3 key (FS_TREE ROOT_ITEM 0) itemoff 14949 itemsize 439
	  generation 4 root_dirid 256 bytenr 30408704 byte_limit 0 bytes_used 16384
	  last_snapshot 0 flags 0x0(none) refs 1
	  drop_progress key (0 UNKNOWN.0 0) drop_level 0
	  level 0 generation_v2 4
	  uuid ec4669b6-6d21-46ab-857e-d60cafde45b3
	  parent_uuid 00000000-0000-0000-0000-000000000000
	  received_uuid 00000000-0000-0000-0000-000000000000
	  ctransid 0 otransid 0 stransid 0 rtransid 0
	  ctime 1633021823.0 (2021-09-30 19:10:23)
	  otime 1633021823.0 (2021-09-30 19:10:23)
	  stime 0.0 (1970-01-01 01:00:00)
	  rtime 0.0 (1970-01-01 01:00:00)

Signed-off-by: David Sterba <dsterba@suse.com>
---
 kernel-shared/print-tree.c | 53 ++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
index e5d4b453fc07..5668fd7873db 100644
--- a/kernel-shared/print-tree.c
+++ b/kernel-shared/print-tree.c
@@ -591,55 +591,46 @@ static void print_root_item(struct extent_buffer *leaf, int slot)
 	read_extent_buffer(leaf, &root_item, (unsigned long)ri, len);
 	root_flags_to_str(btrfs_root_flags(&root_item), flags_str);
 
-	printf("\t\tgeneration %llu root_dirid %llu bytenr %llu level %hhu refs %u\n",
+	printf("\t\tgeneration %llu root_dirid %llu bytenr %llu byte_limit %llu bytes_used %llu\n",
 		(unsigned long long)btrfs_root_generation(&root_item),
 		(unsigned long long)btrfs_root_dirid(&root_item),
 		(unsigned long long)btrfs_root_bytenr(&root_item),
-		btrfs_root_level(&root_item),
-		btrfs_root_refs(&root_item));
-	printf("\t\tlastsnap %llu byte_limit %llu bytes_used %llu flags 0x%llx(%s)\n",
-		(unsigned long long)btrfs_root_last_snapshot(&root_item),
 		(unsigned long long)btrfs_root_limit(&root_item),
-		(unsigned long long)btrfs_root_used(&root_item),
+		(unsigned long long)btrfs_root_used(&root_item));
+	printf("\t\tlast_snapshot %llu flags 0x%llx(%s) refs %u\n",
+		(unsigned long long)btrfs_root_last_snapshot(&root_item),
 		(unsigned long long)btrfs_root_flags(&root_item),
-		flags_str);
+		flags_str,
+		btrfs_root_refs(&root_item));
+	btrfs_disk_key_to_cpu(&drop_key, &root_item.drop_progress);
+	printf("\t\tdrop_progress ");
+	btrfs_print_key(&root_item.drop_progress);
+	printf(" drop_level %hhu\n", root_item.drop_level);
+
+	printf("\t\tlevel %hhu generation_v2 %llu\n",
+		btrfs_root_level(&root_item), root_item.generation_v2);
 
 	if (root_item.generation == root_item.generation_v2) {
 		uuid_unparse(root_item.uuid, uuid_str);
 		printf("\t\tuuid %s\n", uuid_str);
-		if (!empty_uuid(root_item.parent_uuid)) {
-			uuid_unparse(root_item.parent_uuid, uuid_str);
-			printf("\t\tparent_uuid %s\n", uuid_str);
-		}
-		if (!empty_uuid(root_item.received_uuid)) {
-			uuid_unparse(root_item.received_uuid, uuid_str);
-			printf("\t\treceived_uuid %s\n", uuid_str);
-		}
-		if (root_item.ctransid) {
-			printf("\t\tctransid %llu otransid %llu stransid %llu rtransid %llu\n",
+		uuid_unparse(root_item.parent_uuid, uuid_str);
+		printf("\t\tparent_uuid %s\n", uuid_str);
+		uuid_unparse(root_item.received_uuid, uuid_str);
+		printf("\t\treceived_uuid %s\n", uuid_str);
+		printf("\t\tctransid %llu otransid %llu stransid %llu rtransid %llu\n",
 				btrfs_root_ctransid(&root_item),
 				btrfs_root_otransid(&root_item),
 				btrfs_root_stransid(&root_item),
 				btrfs_root_rtransid(&root_item));
-		}
-		if (btrfs_timespec_sec(leaf, btrfs_root_ctime(ri)))
-			print_timespec(leaf, btrfs_root_ctime(ri),
+		print_timespec(leaf, btrfs_root_ctime(ri),
 					"\t\tctime ", "\n");
-		if (btrfs_timespec_sec(leaf, btrfs_root_otime(ri)))
-			print_timespec(leaf, btrfs_root_otime(ri),
+		print_timespec(leaf, btrfs_root_otime(ri),
 					"\t\totime ", "\n");
-		if (btrfs_timespec_sec(leaf, btrfs_root_stime(ri)))
-			print_timespec(leaf, btrfs_root_stime(ri),
+		print_timespec(leaf, btrfs_root_stime(ri),
 					"\t\tstime ", "\n");
-		if (btrfs_timespec_sec(leaf, btrfs_root_rtime(ri)))
-			print_timespec(leaf, btrfs_root_rtime(ri),
+		print_timespec(leaf, btrfs_root_rtime(ri),
 					"\t\trtime ", "\n");
 	}
-
-	btrfs_disk_key_to_cpu(&drop_key, &root_item.drop_progress);
-	printf("\t\tdrop ");
-	btrfs_print_key(&root_item.drop_progress);
-	printf(" level %hhu\n", root_item.drop_level);
 }
 
 static void print_free_space_header(struct extent_buffer *leaf, int slot)
-- 
2.33.0

[PATCH 3/5] btrfs-progs: props: add force parameter to set

[David Sterba]

Add option support to force the value change. This allows to do safety
checks by default and warn user that something might break. Using the
force will override that and changing the property should do change
itself and additionally any other changes that could break some
usecases.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 cmds/property.c | 42 ++++++++++++++++++++++++++++--------------
 cmds/props.h    |  3 ++-
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/cmds/property.c b/cmds/property.c
index 0971aee06a3f..647c3f07afb5 100644
--- a/cmds/property.c
+++ b/cmds/property.c
@@ -43,7 +43,8 @@
 static int prop_read_only(enum prop_object_type type,
 			  const char *object,
 			  const char *name,
-			  const char *value)
+			  const char *value,
+			  bool force)
 {
 	enum btrfs_util_error err;
 	bool read_only;
@@ -79,7 +80,8 @@ static int prop_read_only(enum prop_object_type type,
 static int prop_label(enum prop_object_type type,
 		      const char *object,
 		      const char *name,
-		      const char *value)
+		      const char *value,
+		      bool force)
 {
 	int ret;
 
@@ -99,7 +101,8 @@ static int prop_label(enum prop_object_type type,
 static int prop_compression(enum prop_object_type type,
 			    const char *object,
 			    const char *name,
-			    const char *value)
+			    const char *value,
+			    bool force)
 {
 	int ret;
 	ssize_t sret;
@@ -347,7 +350,7 @@ static int dump_prop(const struct prop_handler *prop,
 
 	if ((types & type) && (prop->types & type)) {
 		if (!name_and_help)
-			ret = prop->handler(type, object, prop->name, NULL);
+			ret = prop->handler(type, object, prop->name, NULL, false);
 		else
 			printf("%-20s%s\n", prop->name, prop->desc);
 	}
@@ -379,7 +382,7 @@ static int dump_props(int types, const char *object, int name_and_help)
 }
 
 static int setget_prop(int types, const char *object,
-		       const char *name, const char *value)
+		       const char *name, const char *value, bool force)
 {
 	int ret;
 	const struct prop_handler *prop = NULL;
@@ -407,7 +410,7 @@ static int setget_prop(int types, const char *object,
 		return 1;
 	}
 
-	ret = prop->handler(types, object, name, value);
+	ret = prop->handler(types, object, name, value, force);
 
 	if (ret < 0)
 		ret = 1;
@@ -420,19 +423,26 @@ static int setget_prop(int types, const char *object,
 
 static int parse_args(const struct cmd_struct *cmd, int argc, char **argv,
 		       int *types, char **object,
-		       char **name, char **value, int min_nonopt_args)
+		       char **name, char **value, int min_nonopt_args,
+		       bool *force)
 {
 	int ret;
 	char *type_str = NULL;
 	int max_nonopt_args = 1;
 
+	*force = false;
+
 	optind = 1;
 	while (1) {
-		int c = getopt(argc, argv, "t:");
+		int c = getopt(argc, argv, "ft:");
 		if (c < 0)
 			break;
 
 		switch (c) {
+		case 'f':
+			/* TODO: do not accept for get/list */
+			*force = true;
+			break;
 		case 't':
 			type_str = optarg;
 			break;
@@ -516,12 +526,13 @@ static int cmd_property_get(const struct cmd_struct *cmd,
 	char *object = NULL;
 	char *name = NULL;
 	int types = 0;
+	bool force;
 
-	if (parse_args(cmd, argc, argv, &types, &object, &name, NULL, 1))
+	if (parse_args(cmd, argc, argv, &types, &object, &name, NULL, 1, &force))
 		return 1;
 
 	if (name)
-		ret = setget_prop(types, object, name, NULL);
+		ret = setget_prop(types, object, name, NULL, force);
 	else
 		ret = dump_props(types, object, 0);
 
@@ -530,13 +541,14 @@ static int cmd_property_get(const struct cmd_struct *cmd,
 static DEFINE_SIMPLE_COMMAND(property_get, "get");
 
 static const char * const cmd_property_set_usage[] = {
-	"btrfs property set [-t <type>] <object> <name> <value>",
+	"btrfs property set [-f] [-t <type>] <object> <name> <value>",
 	"Set a property on a btrfs object",
 	"Set a property on a btrfs object where object is a path to file or",
 	"directory and can also represent the filesystem or device based on the type",
 	"",
 	"-t <TYPE>       list properties for the given object type (inode, subvol,",
 	"                filesystem, device)",
+	"-f              force the change, could potentially break something\n",
 	NULL
 };
 
@@ -548,11 +560,12 @@ static int cmd_property_set(const struct cmd_struct *cmd,
 	char *name = NULL;
 	char *value = NULL;
 	int types = 0;
+	bool force = false;
 
-	if (parse_args(cmd, argc, argv, &types, &object, &name, &value, 3))
+	if (parse_args(cmd, argc, argv, &types, &object, &name, &value, 3, &force))
 		return 1;
 
-	ret = setget_prop(types, object, name, value);
+	ret = setget_prop(types, object, name, value, force);
 
 	return ret;
 }
@@ -576,8 +589,9 @@ static int cmd_property_list(const struct cmd_struct *cmd,
 	int ret;
 	char *object = NULL;
 	int types = 0;
+	bool force;
 
-	if (parse_args(cmd, argc, argv, &types, &object, NULL, NULL, 1))
+	if (parse_args(cmd, argc, argv, &types, &object, NULL, NULL, 1, &force))
 		return 1;
 
 	ret = dump_props(types, object, 1);
diff --git a/cmds/props.h b/cmds/props.h
index a43cb2537f37..0f786b1866ee 100644
--- a/cmds/props.h
+++ b/cmds/props.h
@@ -28,7 +28,8 @@ enum prop_object_type {
 typedef int (*prop_handler_t)(enum prop_object_type type,
 			      const char *object,
 			      const char *name,
-			      const char *value);
+			      const char *value,
+			      bool force);
 
 struct prop_handler {
 	const char *name;
-- 
2.33.0

[PATCH 4/5] btrfs-progs: property: ro->rw and received_uuid

[David Sterba]

Implement safety check when a read-only subvolume is getting switched
to read-write and there's received_uuid set.

This prevents accidental breakage of incremental send usecase but allows
user to do the rw change anyway but resets the received_uuid in that
case.

As this is implemented entirely in userspace, it's racy and using the
raw ioctl won't prevent it nor reset the received_uuid.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 cmds/property.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/cmds/property.c b/cmds/property.c
index 647c3f07afb5..230bef04f6ce 100644
--- a/cmds/property.c
+++ b/cmds/property.c
@@ -22,6 +22,7 @@
 #include <sys/ioctl.h>
 #include <sys/stat.h>
 #include <sys/xattr.h>
+#include <uuid/uuid.h>
 #include <btrfsutil.h>
 #include "cmds/commands.h"
 #include "cmds/props.h"
@@ -40,6 +41,26 @@
 #define ENOATTR ENODATA
 #endif
 
+static int subvolume_clear_received_uuid(const char *path)
+{
+	struct btrfs_ioctl_received_subvol_args args = {};
+	int ret;
+	int fd;
+
+	fd = open(path, O_RDONLY | O_NOATIME);
+	if (fd == -1)
+		return -errno;
+
+	ret = ioctl(fd, BTRFS_IOC_SET_RECEIVED_SUBVOL, &args);
+	if (ret == -1) {
+		close(fd);
+		return -errno;
+	}
+	close(fd);
+
+	return 0;
+}
+
 static int prop_read_only(enum prop_object_type type,
 			  const char *object,
 			  const char *name,
@@ -50,6 +71,10 @@ static int prop_read_only(enum prop_object_type type,
 	bool read_only;
 
 	if (value) {
+		struct btrfs_util_subvolume_info info = {};
+		bool is_ro = false;
+		bool do_clear_received_uuid = false;
+
 		if (!strcmp(value, "true")) {
 			read_only = true;
 		} else if (!strcmp(value, "false")) {
@@ -58,12 +83,44 @@ static int prop_read_only(enum prop_object_type type,
 			error("invalid value for property: %s", value);
 			return -EINVAL;
 		}
+		err = btrfs_util_get_subvolume_read_only(object, &is_ro);
+		if (err) {
+			error_btrfs_util(err);
+			return -errno;
+		}
+		/* No change if already read-only */
+		if (is_ro && read_only)
+			return 0;
+
+		err = btrfs_util_subvolume_info(object, 0, &info);
+		if (err)
+			warning("cannot read subvolume info\n");
+		if (is_ro && !uuid_is_null(info.received_uuid)) {
+			printf("ro->rw switch but has set receive_uuid\n");
+
+			if (force) {
+				do_clear_received_uuid = true;
+			} else {
+				printf("cannot flip ro->rw with received_uuid set, use force\n");
+				return -EPERM;
+			}
+		}
+		if (!is_ro && !uuid_is_null(info.received_uuid))
+			warning("read-write subvolume with received_uuid, this is bad");
 
 		err = btrfs_util_set_subvolume_read_only(object, read_only);
 		if (err) {
 			error_btrfs_util(err);
 			return -errno;
 		}
+		if (do_clear_received_uuid) {
+			int ret;
+
+			printf("force used, clearing received_uuid\n");
+			ret = subvolume_clear_received_uuid(object);
+			if (ret < 0)
+				warning("failed to clear received_uuid: %m");
+		}
 	} else {
 		err = btrfs_util_get_subvolume_read_only(object, &read_only);
 		if (err) {
-- 
2.33.0

Re: [PATCH 4/5] btrfs-progs: property: ro->rw and received_uuid

[Andrei Borzenkov]

On 01.10.2021 18:29, David Sterba wrote:
> Implement safety check when a read-only subvolume is getting switched
> to read-write and there's received_uuid set.
> 
> This prevents accidental breakage of incremental send usecase but allows
> user to do the rw change anyway but resets the received_uuid in that
> case.
> 
> As this is implemented entirely in userspace, it's racy and using the
> raw ioctl won't prevent it nor reset the received_uuid.
> 

Is it feasible to add "force" flag to ioctl itself?

Re: [PATCH 4/5] btrfs-progs: property: ro->rw and received_uuid

[David Sterba]

On Sat, Oct 02, 2021 at 10:05:19AM +0300, Andrei Borzenkov wrote:
> On 01.10.2021 18:29, David Sterba wrote:
> > Implement safety check when a read-only subvolume is getting switched
> > to read-write and there's received_uuid set.
> > 
> > This prevents accidental breakage of incremental send usecase but allows
> > user to do the rw change anyway but resets the received_uuid in that
> > case.
> > 
> > As this is implemented entirely in userspace, it's racy and using the
> > raw ioctl won't prevent it nor reset the received_uuid.
> > 
> 
> Is it feasible to add "force" flag to ioctl itself?

Yeah that's possible but affects the ioctl semantics, so that's a bit
more tricky than the userspace side. It would solve the atomicity
problem in this patchset.

Re: [PATCH 4/5] btrfs-progs: property: ro->rw and received_uuid

[Nikolay Borisov]

On 1.10.21 г. 18:29, David Sterba wrote:
> Implement safety check when a read-only subvolume is getting switched
> to read-write and there's received_uuid set.
> 
> This prevents accidental breakage of incremental send usecase but allows
> user to do the rw change anyway but resets the received_uuid in that
> case.
> 
> As this is implemented entirely in userspace, it's racy and using the
> raw ioctl won't prevent it nor reset the received_uuid.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>


Thanks for sending this, I'm going to comment inline as well as compare
this to my proposal, hopefully that'll help you understand what I mean
and allow you to make an informed decision of which approach to take in
the end and merge.

> ---
>  cmds/property.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/cmds/property.c b/cmds/property.c
> index 647c3f07afb5..230bef04f6ce 100644
> --- a/cmds/property.c
> +++ b/cmds/property.c
> @@ -22,6 +22,7 @@
>  #include <sys/ioctl.h>
>  #include <sys/stat.h>
>  #include <sys/xattr.h>
> +#include <uuid/uuid.h>
>  #include <btrfsutil.h>
>  #include "cmds/commands.h"
>  #include "cmds/props.h"
> @@ -40,6 +41,26 @@
>  #define ENOATTR ENODATA
>  #endif
>  
> +static int subvolume_clear_received_uuid(const char *path)
> +{
> +	struct btrfs_ioctl_received_subvol_args args = {};
> +	int ret;
> +	int fd;
> +
> +	fd = open(path, O_RDONLY | O_NOATIME);
> +	if (fd == -1)
> +		return -errno;
> +
> +	ret = ioctl(fd, BTRFS_IOC_SET_RECEIVED_SUBVOL, &args);
> +	if (ret == -1) {
> +		close(fd);
> +		return -errno;
> +	}
> +	close(fd);
> +
> +	return 0;
> +}
> +
>  static int prop_read_only(enum prop_object_type type,
>  			  const char *object,
>  			  const char *name,
> @@ -50,6 +71,10 @@ static int prop_read_only(enum prop_object_type type,
>  	bool read_only;
>  
>  	if (value) {
> +		struct btrfs_util_subvolume_info info = {};
> +		bool is_ro = false;
> +		bool do_clear_received_uuid = false;
> +
>  		if (!strcmp(value, "true")) {
>  			read_only = true;
>  		} else if (!strcmp(value, "false")) {
> @@ -58,12 +83,44 @@ static int prop_read_only(enum prop_object_type type,
>  			error("invalid value for property: %s", value);
>  			return -EINVAL;
>  		}
> +		err = btrfs_util_get_subvolume_read_only(object, &is_ro);
> +		if (err) {
> +			error_btrfs_util(err);
> +			return -errno;
> +		}
> +		/* No change if already read-only */
> +		if (is_ro && read_only)
> +			return 0;
> +
> +		err = btrfs_util_subvolume_info(object, 0, &info);
> +		if (err)
> +			warning("cannot read subvolume info\n");
> +		if (is_ro && !uuid_is_null(info.received_uuid)) {
> +			printf("ro->rw switch but has set receive_uuid\n");
> +
> +			if (force) {
> +				do_clear_received_uuid = true;

So you'll require the user to explicitly pass -f in order to clear
received uuid when switching ro->rw but otherwise you are not actively
preventing the user to continue relying on invalid behavior.


> +			} else {
> +				printf("cannot flip ro->rw with received_uuid set, use force\n");
> +				return -EPERM;
> +			}
> +		}
> +		if (!is_ro && !uuid_is_null(info.received_uuid))
> +			warning("read-write subvolume with received_uuid, this is bad");

Do you intend to augment this error message or not, I know this is just
a prototype but I wonder what would a useful text here look like. Ok you
are going to warn the user what they are doing is wrong but you won't
stop them from doing it so how is this preventing possible future
streams of people coming to the mailing list and complaining that their
incremental send isn't working or that they are experiencing silent data
corruptions following in incremental send?


In contrast, my approach is to have btrfs progs check scan for such
cases and when it finds them to directly clear the received_uuid as part
of a --repair switch.


>  
>  		err = btrfs_util_set_subvolume_read_only(object, read_only);

So indeed, you are not preventing the users from continuing to exercise
the invalid behavior.

>  		if (err) {
>  			error_btrfs_util(err);
>  			return -errno;
>  		}
> +		if (do_clear_received_uuid) {
> +			int ret;
> +
> +			printf("force used, clearing received_uuid\n");
> +			ret = subvolume_clear_received_uuid(object);

The fact that this can't be made atomic w.r.t setting RO (from user
space) just demonstrates this is somewhat fragile. Even after someone
running btrfs property set -f and a crash occurs it's possible they end
up with a subvol.

My approach on the other hands solves this on the kernel side where we
can make the ro->rw switch + received_uuid clear atomic.

My opinion is that this approach is too fragile.

> +			if (ret < 0)
> +				warning("failed to clear received_uuid: %m");
> +		}
>  	} else {
>  		err = btrfs_util_get_subvolume_read_only(object, &read_only);
>  		if (err) {
> 

Re: [PATCH 4/5] btrfs-progs: property: ro->rw and received_uuid

[David Sterba]

On Mon, Oct 04, 2021 at 01:10:30PM +0300, Nikolay Borisov wrote:
> > +		err = btrfs_util_get_subvolume_read_only(object, &is_ro);
> > +		if (err) {
> > +			error_btrfs_util(err);
> > +			return -errno;
> > +		}
> > +		/* No change if already read-only */
> > +		if (is_ro && read_only)
> > +			return 0;
> > +
> > +		err = btrfs_util_subvolume_info(object, 0, &info);
> > +		if (err)
> > +			warning("cannot read subvolume info\n");
> > +		if (is_ro && !uuid_is_null(info.received_uuid)) {
> > +			printf("ro->rw switch but has set receive_uuid\n");
> > +
> > +			if (force) {
> > +				do_clear_received_uuid = true;
> 
> So you'll require the user to explicitly pass -f in order to clear
> received uuid when switching ro->rw but otherwise you are not actively
> preventing the user to continue relying on invalid behavior.

At first I'm solving the problem where it happens, ie. in most cases
switching the ro->rw by the command. As this is in user space and does
not rely on kernel changes it will affect users with old stable trees
after progs update. The kernel fixes go in parallel.

> > +			} else {
> > +				printf("cannot flip ro->rw with received_uuid set, use force\n");
> > +				return -EPERM;
> > +			}
> > +		}
> > +		if (!is_ro && !uuid_is_null(info.received_uuid))
> > +			warning("read-write subvolume with received_uuid, this is bad");
> 
> Do you intend to augment this error message or not, I know this is just
> a prototype but I wonder what would a useful text here look like. Ok you
> are going to warn the user what they are doing is wrong but you won't
> stop them from doing it so how is this preventing possible future
> streams of people coming to the mailing list and complaining that their
> incremental send isn't working or that they are experiencing silent data
> corruptions following in incremental send?

You mean that people will still come in the future even with these
changes in progs? The warning is a placeholder, the explanation will be
long and pointing to some specific steps what to do to resolve the
prolbem.

> In contrast, my approach is to have btrfs progs check

This is on a different level, 'check' works on an unmounted filesystem
and is not recommended to be run just because. Also I don't like much to
add yet another one-shot fix that would be probably more suitable for
command like 'rescue' group.

> scan for such
> cases and when it finds them to directly clear the received_uuid as part
> of a --repair switch.

With repair? No. It can be part of check to report but as it's not a
structural damage, it's not the best place and for a specific fix the
scope of --repair is too big.

> >  
> >  		err = btrfs_util_set_subvolume_read_only(object, read_only);
> 
> So indeed, you are not preventing the users from continuing to exercise
> the invalid behavior.

So I think that's the source of the disagreement or misunderstanding.
I'm looking for preventing the problem to happen in the first place, you
keep repeating 'continue the invalid behaviour'.  Yes we need to deal
with the existing rw+received_uuid too and will eventually.

> >  		if (err) {
> >  			error_btrfs_util(err);
> >  			return -errno;
> >  		}
> > +		if (do_clear_received_uuid) {
> > +			int ret;
> > +
> > +			printf("force used, clearing received_uuid\n");
> > +			ret = subvolume_clear_received_uuid(object);
> 
> The fact that this can't be made atomic w.r.t setting RO (from user
> space) just demonstrates this is somewhat fragile. Even after someone
> running btrfs property set -f and a crash occurs it's possible they end
> up with a subvol.

Yeah that's true, both ioctls call commit so there's an intermediate
state when RW is committed and uuid is not reset.

> My approach on the other hands solves this on the kernel side where we
> can make the ro->rw switch + received_uuid clear atomic.

That would be better but slightly changes the semantics of the setflags
ioctl, lacking the similar force flag like userspace does.

[PATCH 5/5] btrfs-progs: tests: subvolume ro->rw switch and received_uuid

[David Sterba]

Signed-off-by: David Sterba <dsterba@suse.com>
---
 .../050-receive-prop-ro-to-rw/test.sh         | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100755 tests/misc-tests/050-receive-prop-ro-to-rw/test.sh

diff --git a/tests/misc-tests/050-receive-prop-ro-to-rw/test.sh b/tests/misc-tests/050-receive-prop-ro-to-rw/test.sh
new file mode 100755
index 000000000000..fb1e0e64040d
--- /dev/null
+++ b/tests/misc-tests/050-receive-prop-ro-to-rw/test.sh
@@ -0,0 +1,42 @@
+#!/bin/bash
+# Prevent changing subvolume ro->rw status with received_uuid set, unless forced
+
+source "$TEST_TOP/common"
+
+setup_root_helper
+prepare_test_dev
+
+run_check_mkfs_test_dev
+run_check_mount_test_dev
+
+run_check $SUDO_HELPER mkdir "$TEST_MNT/src" "$TEST_MNT/dst"
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/src/subvol1"
+for i in `seq 10`; do
+	run_check $SUDO_HELPER dd if=/dev/zero of="$TEST_MNT/file$1" bs=10K count=1
+done
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r "$TEST_MNT/src/subvol1" "$TEST_MNT/src/snap1"
+
+for i in `seq 10`; do
+	run_check $SUDO_HELPER dd if=/dev/zero of="$TEST_MNT/file-new$1" bs=1K count=1
+done
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r "$TEST_MNT/src/subvol1" "$TEST_MNT/src/snap2"
+
+touch send1.stream send2.stream
+chmod a+w send1.stream send2.stream
+run_check $SUDO_HELPER "$TOP/btrfs" send -f send1.stream "$TEST_MNT/src/snap1"
+run_check $SUDO_HELPER "$TOP/btrfs" send -f send2.stream -p "$TEST_MNT/src/snap1" "$TEST_MNT/src/snap2"
+
+run_check $SUDO_HELPER "$TOP/btrfs" receive -f send1.stream -m "$TEST_MNT" "$TEST_MNT/dst"
+run_check $SUDO_HELPER "$TOP/btrfs" receive -f send2.stream -m "$TEST_MNT" "$TEST_MNT/dst"
+
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume show "$TEST_MNT/dst/snap2"
+run_check $SUDO_HELPER "$TOP/btrfs" property get "$TEST_MNT/dst/snap2" ro
+run_mustfail "ro->rw switch and received_uuid not reset" \
+	$SUDO_HELPER "$TOP/btrfs" property set "$TEST_MNT/dst/snap2" ro false
+
+run_check $SUDO_HELPER "$TOP/btrfs" property set -f "$TEST_MNT/dst/snap2" ro false
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume show "$TEST_MNT/dst/snap2"
+
+run_check_umount_test_dev
+
+rm -f send1.stream send2.stream
-- 
2.33.0