[RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"

[RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"

[Misono, Tomohiro]

changelog:

v2 -> v3
  - use get_euid() to check the caller's privilege (and remove 3rd patch)
  - improve error handling
v1 -> v2
  - add independent error handling patch (1st patch)
  - reimplement according to ioctl change
  - various cleanup
===

This RFC implements user version of "subvolume list/show" using three new ioctls.
The ioctl patch to the kernel can be found in the ML titled 
  "[PATCH v3 0/3] btrfs: Add three new unprivileged ioctls to allow normal users to call "sub list/show" etc.

1th patch is independent and improvements of error handling
2nd-4th are some prepartion works.
5th patch is the main part.
6th-7th adds the test for "subvolume list"

The main behavior differences between root and normal users are:

- "sub list" list the subvolumes which exist under the specified path 
(including the path itself). The specified path itself is not needed to be
 a subvolume. Also If the subvolume cannot be opend but the parent
directory can be, the information other than name or id would be zeroed out.

- snapshot filed of "subvolume show" just lists
the snapshots under the specified subvolume.


This is a part of RFC I sent last December[1] whose aim is to improve normal users' usability.
The remaining works of RFC are: 
  - Allow "sub delete" for empty subvolume
  - Allow "qgroup show" to check quota limit

[1] https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg70991.html


Tomohiro Misono (7):
  btrfs-progs: sub list: Call rb_free_nodes() in error path
  btrfs-progs: ioctl: Add 3 definitions of new unprivileged ioctl
  btrfs-progs: sub list: Pass specified path down to
    btrfs_list_subvols()
  btrfs-progs: fallback to open without O_NOATIME flag in
    find_mount_root()
  btrfs-progs: sub list: Allow normal user to call "subvolume list/show"
  btrfs-progs: test: Add helper function to check if test user exists
  btrfs-porgs: test: Add cli-test/009 to check subvolume list for both
    root and normal user

 btrfs-list.c                               | 376 +++++++++++++++++++++++++++--
 btrfs-list.h                               |   7 +-
 cmds-subvolume.c                           |  14 +-
 ioctl.h                                    |  86 +++++++
 tests/cli-tests/009-subvolume-list/test.sh | 136 +++++++++++
 tests/common                               |  10 +
 utils.c                                    |  13 +-
 7 files changed, 609 insertions(+), 33 deletions(-)
 create mode 100755 tests/cli-tests/009-subvolume-list/test.sh

-- 
2.14.3

[RFC PATCH v3 1/7] btrfs-progs: sub list: Call rb_free_nodes() in error path

[Misono, Tomohiro]

After btrfs_list_subvols() is called, root_lookup may hold some allocated
memory area even if the function fails.
Therefore rb_free_nodes() should be called.

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
---
 btrfs-list.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/btrfs-list.c b/btrfs-list.c
index e01c5899..50e5ce5f 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -1523,8 +1523,10 @@ int btrfs_list_subvols_print(int fd, struct btrfs_list_filter_set *filter_set,
 		return ret;
 
 	ret = btrfs_list_subvols(fd, &root_lookup);
-	if (ret)
+	if (ret) {
+		rb_free_nodes(&root_lookup.root, free_root_info);
 		return ret;
+	}
 	filter_and_sort_subvol(&root_lookup, &root_sort, filter_set,
 				 comp_set, top_id);
 
@@ -1554,14 +1556,18 @@ int btrfs_get_toplevel_subvol(int fd, struct root_info *the_ri)
 		return ret;
 
 	ret = btrfs_list_subvols(fd, &rl);
-	if (ret)
+	if (ret) {
+		rb_free_nodes(&rl.root, free_root_info);
 		return ret;
+	}
 
 	rbn = rb_first(&rl.root);
 	ri = rb_entry(rbn, struct root_info, rb_node);
 
-	if (ri->root_id != BTRFS_FS_TREE_OBJECTID)
+	if (ri->root_id != BTRFS_FS_TREE_OBJECTID) {
+		rb_free_nodes(&rl.root, free_root_info);
 		return -ENOENT;
+	}
 
 	memcpy(the_ri, ri, offsetof(struct root_info, path));
 	the_ri->path = strdup_or_null("/");
@@ -1585,8 +1591,10 @@ int btrfs_get_subvol(int fd, struct root_info *the_ri)
 		return ret;
 
 	ret = btrfs_list_subvols(fd, &rl);
-	if (ret)
+	if (ret) {
+		rb_free_nodes(&rl.root, free_root_info);
 		return ret;
+	}
 
 	rbn = rb_first(&rl.root);
 	while(rbn) {
-- 
2.14.3

[RFC PATCH v3 2/7] btrfs-progs: ioctl: Add 3 definitions of new unprivileged ioctl

[Misono, Tomohiro]

Add 3 definitions of new unprivileged ioctl (BTRFS_IOC_GET_SUBVOL_INFO,
BTRFS_IOC_GET_SUBVOL_ROOTREF and BTRFS_IOC_INO_LOOKUP_USER). They will
be used to implement user version of "btrfs subvolume list" etc.

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
---
 ioctl.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/ioctl.h b/ioctl.h
index 709e996f..c6624352 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -320,6 +320,22 @@ struct btrfs_ioctl_ino_lookup_args {
 };
 BUILD_ASSERT(sizeof(struct btrfs_ioctl_ino_lookup_args) == 4096);
 
+#define BTRFS_INO_LOOKUP_USER_PATH_MAX (4080-BTRFS_VOL_NAME_MAX-1)
+struct btrfs_ioctl_ino_lookup_user_args {
+	/* in, inode number containing the subvolume of 'subvolid' */
+	__u64 dirid;
+	/* in */
+	__u64 subvolid;
+	/* out, name of the subvolume of 'subvolid' */
+	char name[BTRFS_VOL_NAME_MAX + 1];
+	/*
+	 * out, constructed path from the directory with which
+	 * the ioctl is called to dirid
+	 */
+	char path[BTRFS_INO_LOOKUP_USER_PATH_MAX];
+};
+BUILD_ASSERT(sizeof(struct btrfs_ioctl_ino_lookup_user_args) == 4096);
+
 struct btrfs_ioctl_search_key {
 	/* which root are we searching.  0 is the tree of tree roots */
 	__u64 tree_id;
@@ -672,6 +688,70 @@ BUILD_ASSERT(sizeof(struct btrfs_ioctl_send_args_64) == 72);
 
 #define BTRFS_IOC_SEND_64_COMPAT_DEFINED 1
 
+struct btrfs_ioctl_get_subvol_info_args {
+	/* All filed is out */
+	/* Id of this subvolume */
+	__u64 id;
+	/* Name of this subvolume, used to get the real name at mount point */
+	char name[BTRFS_VOL_NAME_MAX + 1];
+	/*
+	 * Id of the subvolume which contains this subvolume.
+	 * Zero for top-level subvolume or deleted subvolume
+	 */
+	__u64 parent_id;
+	/*
+	 * Inode number of the directory which contains this subvolume.
+	 * Zero for top-level subvolume or deleted subvolume
+	 */
+	__u64 dirid;
+
+	/* Latest transaction id of this subvolume */
+	__u64 generation;
+	/* Flags of this subvolume */
+	__u64 flags;
+
+	/* uuid of this subvolume */
+	__u8 uuid[BTRFS_UUID_SIZE];
+	/*
+	 * uuid of the subvolume of which this subvolume is a snapshot.
+	 * All zero for non-snapshot subvolume
+	 */
+	__u8 parent_uuid[BTRFS_UUID_SIZE];
+	/*
+	 * uuid of the subvolume from which this subvolume is received.
+	 * All zero for non-received subvolume
+	 */
+	__u8 received_uuid[BTRFS_UUID_SIZE];
+
+	/* Transaction id indicates when change/create/send/receive happens */
+	__u64 ctransid;
+	__u64 otransid;
+	__u64 stransid;
+	__u64 rtransid;
+	/* Time corresponds to c/o/s/rtransid */
+	struct btrfs_ioctl_timespec ctime;
+	struct btrfs_ioctl_timespec otime;
+	struct btrfs_ioctl_timespec stime;
+	struct btrfs_ioctl_timespec rtime;
+
+	__u64 reserved[8];
+};
+
+#define BTRFS_MAX_ROOTREF_BUFFER_NUM 255
+struct btrfs_ioctl_get_subvol_rootref_args {
+		/* in/out, min id of rootref's subvolid to be searched */
+		__u64 min_id;
+		/* out */
+		struct {
+			__u64 subvolid;
+			__u64 dirid;
+		} rootref[BTRFS_MAX_ROOTREF_BUFFER_NUM];
+		/* out, number of found items */
+		__u8 num_items;
+		__u8 align[7];
+};
+BUILD_ASSERT(sizeof(struct btrfs_ioctl_get_subvol_rootref_args) == 4096);
+
 /* Error codes as returned by the kernel */
 enum btrfs_err_code {
 	notused,
@@ -828,6 +908,12 @@ static inline char *btrfs_err_str(enum btrfs_err_code err_code)
                                   struct btrfs_ioctl_feature_flags[3])
 #define BTRFS_IOC_RM_DEV_V2	_IOW(BTRFS_IOCTL_MAGIC, 58, \
 				   struct btrfs_ioctl_vol_args_v2)
+#define BTRFS_IOC_GET_SUBVOL_INFO _IOR(BTRFS_IOCTL_MAGIC, 60, \
+				struct btrfs_ioctl_get_subvol_info_args)
+#define BTRFS_IOC_GET_SUBVOL_ROOTREF _IOWR(BTRFS_IOCTL_MAGIC, 61, \
+				struct btrfs_ioctl_get_subvol_rootref_args)
+#define BTRFS_IOC_INO_LOOKUP_USER _IOWR(BTRFS_IOCTL_MAGIC, 62, \
+				struct btrfs_ioctl_ino_lookup_user_args)
 #ifdef __cplusplus
 }
 #endif
-- 
2.14.3

[RFC PATCH v3 3/7] btrfs-progs: sub list: Pass specified path down to btrfs_list_subvols()

[Misono, Tomohiro]

This is a preparetion work to allow normal user to call
"subvolume list/show".

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
---
 btrfs-list.c     | 16 +++++++++-------
 btrfs-list.h     |  7 ++++---
 cmds-subvolume.c |  6 +++---
 utils.c          | 10 +++++-----
 4 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/btrfs-list.c b/btrfs-list.c
index 50e5ce5f..833ff912 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -1489,7 +1489,8 @@ next:
 	}
 }
 
-static int btrfs_list_subvols(int fd, struct root_lookup *root_lookup)
+static int btrfs_list_subvols(int fd, struct root_lookup *root_lookup,
+			const char *path)
 {
 	int ret;
 
@@ -1510,7 +1511,7 @@ static int btrfs_list_subvols(int fd, struct root_lookup *root_lookup)
 int btrfs_list_subvols_print(int fd, struct btrfs_list_filter_set *filter_set,
 		       struct btrfs_list_comparer_set *comp_set,
 		       enum btrfs_list_layout layout, int full_path,
-		       const char *raw_prefix)
+		       const char *raw_prefix, const char *path)
 {
 	struct root_lookup root_lookup;
 	struct root_lookup root_sort;
@@ -1522,7 +1523,7 @@ int btrfs_list_subvols_print(int fd, struct btrfs_list_filter_set *filter_set,
 	if (ret)
 		return ret;
 
-	ret = btrfs_list_subvols(fd, &root_lookup);
+	ret = btrfs_list_subvols(fd, &root_lookup, path);
 	if (ret) {
 		rb_free_nodes(&root_lookup.root, free_root_info);
 		return ret;
@@ -1543,7 +1544,8 @@ static char *strdup_or_null(const char *s)
 	return strdup(s);
 }
 
-int btrfs_get_toplevel_subvol(int fd, struct root_info *the_ri)
+int btrfs_get_toplevel_subvol(int fd, struct root_info *the_ri,
+			const char *path)
 {
 	int ret;
 	struct root_lookup rl;
@@ -1555,7 +1557,7 @@ int btrfs_get_toplevel_subvol(int fd, struct root_info *the_ri)
 	if (ret)
 		return ret;
 
-	ret = btrfs_list_subvols(fd, &rl);
+	ret = btrfs_list_subvols(fd, &rl, path);
 	if (ret) {
 		rb_free_nodes(&rl.root, free_root_info);
 		return ret;
@@ -1578,7 +1580,7 @@ int btrfs_get_toplevel_subvol(int fd, struct root_info *the_ri)
 	return ret;
 }
 
-int btrfs_get_subvol(int fd, struct root_info *the_ri)
+int btrfs_get_subvol(int fd, struct root_info *the_ri, const char *path)
 {
 	int ret, rr;
 	struct root_lookup rl;
@@ -1590,7 +1592,7 @@ int btrfs_get_subvol(int fd, struct root_info *the_ri)
 	if (ret)
 		return ret;
 
-	ret = btrfs_list_subvols(fd, &rl);
+	ret = btrfs_list_subvols(fd, &rl, path);
 	if (ret) {
 		rb_free_nodes(&rl.root, free_root_info);
 		return ret;
diff --git a/btrfs-list.h b/btrfs-list.h
index 6e5fc778..9b6720e6 100644
--- a/btrfs-list.h
+++ b/btrfs-list.h
@@ -169,12 +169,13 @@ struct btrfs_list_comparer_set *btrfs_list_alloc_comparer_set(void);
 int btrfs_list_subvols_print(int fd, struct btrfs_list_filter_set *filter_set,
 		       struct btrfs_list_comparer_set *comp_set,
 		       enum btrfs_list_layout layot, int full_path,
-		       const char *raw_prefix);
+		       const char *raw_prefix, const char *path);
 int btrfs_list_find_updated_files(int fd, u64 root_id, u64 oldest_gen);
 int btrfs_list_get_default_subvolume(int fd, u64 *default_id);
 char *btrfs_list_path_for_root(int fd, u64 root);
 int btrfs_list_get_path_rootid(int fd, u64 *treeid);
-int btrfs_get_subvol(int fd, struct root_info *the_ri);
-int btrfs_get_toplevel_subvol(int fd, struct root_info *the_ri);
+int btrfs_get_subvol(int fd, struct root_info *the_ri, const char *path);
+int btrfs_get_toplevel_subvol(int fd, struct root_info *the_ri,
+			const char *path);
 
 #endif
diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 8a473f7a..faa10c5a 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -620,7 +620,7 @@ static int cmd_subvol_list(int argc, char **argv)
 	btrfs_list_setup_print_column(BTRFS_LIST_PATH);
 
 	ret = btrfs_list_subvols_print(fd, filter_set, comparer_set,
-			layout, !is_list_all && !is_only_in_path, NULL);
+			layout, !is_list_all && !is_only_in_path, NULL, subvol);
 
 out:
 	close_file_or_dir(fd, dirstream);
@@ -844,7 +844,7 @@ static int cmd_subvol_get_default(int argc, char **argv)
 	btrfs_list_setup_print_column(BTRFS_LIST_PATH);
 
 	ret = btrfs_list_subvols_print(fd, filter_set, NULL,
-		BTRFS_LIST_LAYOUT_DEFAULT, 1, NULL);
+		BTRFS_LIST_LAYOUT_DEFAULT, 1, NULL, subvol);
 
 	if (filter_set)
 		free(filter_set);
@@ -1110,7 +1110,7 @@ static int cmd_subvol_show(int argc, char **argv)
 		goto out;
 	}
 	btrfs_list_subvols_print(fd, filter_set, NULL, BTRFS_LIST_LAYOUT_RAW,
-			1, raw_prefix);
+			1, raw_prefix, fullpath);
 
 out:
 	/* clean up */
diff --git a/utils.c b/utils.c
index e9cb3a82..81c54faa 100644
--- a/utils.c
+++ b/utils.c
@@ -2542,9 +2542,9 @@ int get_subvol_info(const char *fullpath, struct root_info *get_ri)
 	get_ri->root_id = sv_id;
 
 	if (sv_id == BTRFS_FS_TREE_OBJECTID)
-		ret = btrfs_get_toplevel_subvol(mntfd, get_ri);
+		ret = btrfs_get_toplevel_subvol(mntfd, get_ri, mnt);
 	else
-		ret = btrfs_get_subvol(mntfd, get_ri);
+		ret = btrfs_get_subvol(mntfd, get_ri, mnt);
 	if (ret)
 		error("can't find '%s': %d", svpath, ret);
 
@@ -2570,9 +2570,9 @@ int get_subvol_info_by_rootid(const char *mnt, struct root_info *get_ri, u64 r_i
 	get_ri->root_id = r_id;
 
 	if (r_id == BTRFS_FS_TREE_OBJECTID)
-		ret = btrfs_get_toplevel_subvol(fd, get_ri);
+		ret = btrfs_get_toplevel_subvol(fd, get_ri, mnt);
 	else
-		ret = btrfs_get_subvol(fd, get_ri);
+		ret = btrfs_get_subvol(fd, get_ri, mnt);
 
 	if (ret)
 		error("can't find rootid '%llu' on '%s': %d", r_id, mnt, ret);
@@ -2595,7 +2595,7 @@ int get_subvol_info_by_uuid(const char *mnt, struct root_info *get_ri, u8 *uuid_
 	memset(get_ri, 0, sizeof(*get_ri));
 	uuid_copy(get_ri->uuid, uuid_arg);
 
-	ret = btrfs_get_subvol(fd, get_ri);
+	ret = btrfs_get_subvol(fd, get_ri, mnt);
 	if (ret) {
 		char uuid_parsed[BTRFS_UUID_UNPARSED_SIZE];
 		uuid_unparse(uuid_arg, uuid_parsed);
-- 
2.14.3

[RFC PATCH v3 4/7] btrfs-progs: fallback to open without O_NOATIME flag in find_mount_root()

[Misono, Tomohiro]

O_NOATIME flag requires effective UID of process matches file's owner
or has CAP_FOWNER capabilities. Fallback to open without O_NOATIME flag
so that normal user can also call find_mount_root().

This is a preparation work to allow normal user to call "subvolume show".

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
---
 utils.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/utils.c b/utils.c
index 81c54faa..acea70a5 100644
--- a/utils.c
+++ b/utils.c
@@ -2045,6 +2045,9 @@ int find_mount_root(const char *path, char **mount_root)
 	char *longest_match = NULL;
 
 	fd = open(path, O_RDONLY | O_NOATIME);
+	if (fd < 0 && errno == EPERM)
+		fd = open(path, O_RDONLY);
+
 	if (fd < 0)
 		return -errno;
 	close(fd);
-- 
2.14.3

[RFC PATCH v3 5/7] btrfs-progs: sub list: Allow normal user to call "subvolume list/show"

[Misono, Tomohiro]

Allow normal user to call "subvolume list/show" by using 3 new
unprivileged ioctls (BTRFS_IOC_GET_SUBVOL_INFO,
BTRFS_IOC_GET_SUBVOL_ROOTREF and BTRFS_IOC_INO_LOOKUP_USER).

Note that for root, "subvolume list" returns all the subvolume in the
filesystem by default, but for normal user, it returns subvolumes
which exist under the specified path (including the path itself).
The specified path itself is not needed to be a subvolume.
If the subvolume cannot be opend but the parent directory can be,
the information other than name or id would be zeroed out.

Also, for normal user, snapshot filed of "subvolume show" just lists
the snapshots under the specified subvolume.

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
---
 btrfs-list.c     | 344 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 cmds-subvolume.c |   8 ++
 2 files changed, 341 insertions(+), 11 deletions(-)

diff --git a/btrfs-list.c b/btrfs-list.c
index 833ff912..c819499f 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -33,6 +33,7 @@
 #include <uuid/uuid.h>
 #include "btrfs-list.h"
 #include "rbtree-utils.h"
+#include <sys/queue.h>
 
 #define BTRFS_LIST_NFILTERS_INCREASE	(2 * BTRFS_LIST_FILTER_MAX)
 #define BTRFS_LIST_NCOMPS_INCREASE	(2 * BTRFS_LIST_COMP_MAX)
@@ -549,6 +550,9 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
 	int len = 0;
 	struct root_info *found;
 
+	if (ri->full_path != NULL)
+		return 0;
+
 	/*
 	 * we go backwards from the root_info object and add pathnames
 	 * from parent directories as we go.
@@ -672,6 +676,50 @@ static int lookup_ino_path(int fd, struct root_info *ri)
 	return 0;
 }
 
+/* user version of lookup_ino_path which also cheks the access right */
+static int lookup_ino_path_user(int fd, struct root_info *ri)
+{
+	struct btrfs_ioctl_ino_lookup_user_args args;
+	int ret = 0;
+
+	if (ri->path)
+		return 0;
+	if (!ri->ref_tree)
+		return -ENOENT;
+
+	memset(&args, 0, sizeof(args));
+	args.dirid = ri->dir_id;
+	args.subvolid = ri->root_id;
+
+	ret = ioctl(fd, BTRFS_IOC_INO_LOOKUP_USER, &args);
+	if (ret < 0) {
+		if (errno == ENOENT) {
+			ri->ref_tree = 0;
+			return -ENOENT;
+		}
+		if (errno != EACCES) {
+			error("failed to lookup path for root %llu: %m",
+			(unsigned long long)ri->ref_tree);
+			return ret;
+		} else {
+			return -EACCES;
+		}
+	}
+
+	ri->path = malloc(strlen(args.path) + strlen(args.name) + 1);
+	if (!ri->path)
+		return -ENOMEM;
+	strcpy(ri->path, args.path);
+
+	ri->name = malloc(strlen(args.name) + 1);
+	if (!ri->name)
+		return -ENOMEM;
+	strcpy(ri->name, args.name);
+
+	strcat(ri->path, ri->name);
+	return ret;
+}
+
 /* finding the generation for a given path is a two step process.
  * First we use the inode lookup routine to find out the root id
  *
@@ -1310,6 +1358,266 @@ static int list_subvol_fill_paths(int fd, struct root_lookup *root_lookup)
 	return 0;
 }
 
+static int fill_subvol_info(struct root_lookup *root_lookup, int fd)
+{
+	struct btrfs_ioctl_get_subvol_info_args subvol_info;
+	u64 root_offset;
+	int ret;
+
+	ret = ioctl(fd, BTRFS_IOC_GET_SUBVOL_INFO, &subvol_info);
+	if (ret < 0)
+		return -errno;
+
+	if (!uuid_is_null(subvol_info.parent_uuid))
+		root_offset = subvol_info.otransid;
+	else
+		root_offset = 0;
+
+	add_root(root_lookup, subvol_info.id, 0,
+		 root_offset, subvol_info.flags, subvol_info.dirid,
+		 NULL, 0,
+		 subvol_info.otransid, subvol_info.generation,
+		 subvol_info.otime.sec, subvol_info.uuid,
+		 subvol_info.parent_uuid, subvol_info.received_uuid);
+
+	return 0;
+}
+
+static int fill_subvol_info_top(struct root_lookup *root_lookup, int fd)
+{
+	struct btrfs_ioctl_get_subvol_info_args subvol_info;
+	struct root_info *ri;
+	u64 root_offset;
+	int ret;
+
+	ret = ioctl(fd, BTRFS_IOC_GET_SUBVOL_INFO, &subvol_info);
+	if (ret < 0)
+		return -errno;
+
+	if (!uuid_is_null(subvol_info.parent_uuid))
+		root_offset = subvol_info.otransid;
+	else
+		root_offset = 0;
+
+	add_root(root_lookup, subvol_info.id, subvol_info.parent_id,
+		 root_offset, subvol_info.flags, subvol_info.dirid,
+		 subvol_info.name, strlen(subvol_info.name),
+		 subvol_info.otransid, subvol_info.generation,
+		 subvol_info.otime.sec, subvol_info.uuid,
+		 subvol_info.parent_uuid, subvol_info.received_uuid);
+
+	/*
+	 * fill the path since we won't lookup directory/subvolume
+	 * above this subvolume and cannot use root_lookup()
+	 */
+	ri = root_tree_search(root_lookup, subvol_info.id);
+	ri->top_id = subvol_info.parent_id;
+	if (subvol_info.id == BTRFS_FS_TREE_OBJECTID) {
+		ri->path = strdup("/");
+		ri->name = strdup("<FS_TREE>");
+		ri->full_path = strdup("/");
+		if (!ri->path || !ri->name || !ri->full_path)
+			return -ENOMEM;
+	} else {
+		ri->path = malloc(strlen(ri->name + 1));
+		ri->full_path = malloc(strlen(ri->name + 1));
+		if (!ri->path || !ri->full_path)
+			return -ENOMEM;
+
+		strcpy(ri->path, ri->name);
+		strcpy(ri->full_path, ri->name);
+	}
+
+	return 0;
+}
+
+static int fill_rootref(struct root_lookup *root_lookup, int fd, int parent_id)
+{
+	struct btrfs_ioctl_get_subvol_rootref_args rootrefs;
+	bool continue_search = true;
+	int i, ret;
+
+	memset(&rootrefs, 0, sizeof(rootrefs));
+	while (continue_search) {
+		ret = ioctl(fd, BTRFS_IOC_GET_SUBVOL_ROOTREF, &rootrefs);
+		if (ret < 0) {
+			if (errno != EOVERFLOW)
+				return -errno;
+			continue_search = true;
+		} else {
+			continue_search = false;
+		}
+
+		for (i = 0; i < rootrefs.num_items; i++) {
+			add_root(root_lookup, rootrefs.rootref[i].subvolid,
+			    parent_id, 0, 0, rootrefs.rootref[i].dirid,
+			    NULL, 0, 0, 0, 0, NULL, NULL, NULL);
+		}
+	}
+
+	return 0;
+}
+
+static int list_subvol_user(int top_fd, struct root_lookup *root_lookup,
+				const char *path)
+{
+	struct root_info *ri, *parent;
+	struct rb_node *n;
+	char *fullpath;
+	u64 top_id;
+	/* fifo queue entry which holds subvolume's id */
+	struct queue_entry {
+		u64 id;
+
+		STAILQ_ENTRY(queue_entry) entries;
+	} *e, *etemp;
+	int fd;
+	int ret = 0;
+
+	root_lookup->root.rb_node = NULL;
+
+	ret = btrfs_list_get_path_rootid(top_fd, &top_id);
+	if (ret)
+		return ret;
+
+	/* Add top_id to the queue */
+	STAILQ_HEAD(slistead, queue_entry) head = STAILQ_HEAD_INITIALIZER(head);
+	STAILQ_INIT(&head);
+	e = malloc(sizeof(struct queue_entry));
+	if (!e)
+		return -ENOMEM;
+	e->id = top_id;
+	STAILQ_INSERT_TAIL(&head, e, entries);
+
+	/*
+	 * Iterate until queue is empty:
+	 * 1. Pop the first entry
+	 * 2. Open the entry's path
+	 * 3  Get the subvolume information by fill_subvol_info/fill_rootref
+	 * 4. Iterate over rb_tree:
+	 * 4-1. Searth the rb_tree whose ref_tree is e->id
+	 *    (this means the subvolume exists under e->id's subvolume)
+	 * 4-2. Call ino_lookup_user ioctl
+	 * 4-3. If the call succeeds, add the subvolume id to the queue
+	 */
+	while (!STAILQ_EMPTY(&head)) {
+		e = STAILQ_FIRST(&head);
+		STAILQ_REMOVE_HEAD(&head, entries);
+
+		if (e->id == top_id) {
+			fd = top_fd;
+			ret = fill_subvol_info_top(root_lookup, fd);
+			if (ret)
+				goto err;
+			ret = fill_rootref(root_lookup, fd, e->id);
+			if (ret)
+				goto err;
+		} else {
+			parent = root_tree_search(root_lookup, e->id);
+			resolve_root(root_lookup, parent, top_id);
+			fd = openat(top_fd, parent->full_path, O_RDONLY);
+			if (fd == -1) {
+				if (errno == EACCES) {
+					/* skip this subvolume */
+					continue;
+				} else {
+					error("error at open %s: %m",
+							parent->full_path);
+					goto err;
+				}
+			}
+
+			ret = fill_subvol_info(root_lookup, fd);
+			if (ret)
+				goto err;
+			ret = fill_rootref(root_lookup, fd, e->id);
+			if (ret)
+				goto err;
+		}
+
+		n = rb_first(&root_lookup->root);
+		while (n) {
+			ri = rb_entry(n, struct root_info, rb_node);
+			if (ri->ref_tree == 0) {
+			/* BTRFS_FS_TREE_OBJECTID or deleted subvolume */
+				n = rb_next(n);
+				continue;
+			}
+
+			if (ri->ref_tree == e->id) {
+				ret = lookup_ino_path_user(fd, ri);
+				if (ret < 0 && ret != -ENOENT && ret != -EACCES)
+					goto err;
+
+				/* add ths subvol id to queue */
+				if (!ret) {
+					etemp = malloc(sizeof(struct queue_entry));
+
+					if (!etemp) {
+						ret = -ENOMEM;
+						goto err;
+					}
+					etemp->id = ri->root_id;
+					STAILQ_INSERT_TAIL(&head, etemp,
+					    entries);
+				}
+			}
+			n = rb_next(n);
+		}
+
+		if (fd != top_fd)
+			close(fd);
+		free(e);
+	}
+
+	/* If the specified path itself is not a subvolume, remove the entry */
+	fullpath = realpath(path, NULL);
+	if (!fullpath) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	ret = test_issubvolume(fullpath);
+	free(fullpath);
+	if (ret < 0)
+		goto err;
+
+	if (!ret) {
+		ri = root_tree_search(root_lookup, top_id);
+		rb_erase(&ri->rb_node, &root_lookup->root);
+		free_root_info(&ri->rb_node);
+	}
+
+	/* remove skipped entries */
+	n = rb_first(&root_lookup->root);
+	while (n) {
+		ri = rb_entry(n, struct root_info, rb_node);
+		if (!ri->path) {
+			struct rb_node *next = rb_next(n);
+
+			rb_erase(n, &root_lookup->root);
+			free_root_info(n);
+			n = next;
+		} else {
+			n = rb_next(n);
+		}
+	}
+
+	return 0;
+
+err:
+	if (fd != -1 && fd != top_fd)
+		close(fd);
+
+	/* free remaining queue entries */
+	while (!STAILQ_EMPTY(&head)) {
+		e = STAILQ_FIRST(&head);
+		STAILQ_REMOVE_HEAD(&head, entries);
+		free(e);
+	}
+
+	return ret;
+}
+
 static void print_subvolume_column(struct root_info *subv,
 				   enum btrfs_list_column_enum column)
 {
@@ -1492,19 +1800,33 @@ next:
 static int btrfs_list_subvols(int fd, struct root_lookup *root_lookup,
 			const char *path)
 {
+	uid_t uid;
 	int ret;
 
-	ret = list_subvol_search(fd, root_lookup);
-	if (ret) {
-		error("can't perform the search: %m");
-		return ret;
+	uid = geteuid();
+	if (!uid) {
+		ret = list_subvol_search(fd, root_lookup);
+		if (ret) {
+			error("can't perform the search: %s", strerror(-ret));
+			return ret;
+		}
+		/*
+		 * now we have an rbtree full of root_info objects, but we
+		 * need to fill in their path names within the subvol that
+		 * is referencing each one.
+		 */
+		ret = list_subvol_fill_paths(fd, root_lookup);
+	} else {
+		ret = list_subvol_user(fd, root_lookup, path);
+		if (ret) {
+			if (ret == -ENOTTY)
+error("can't perform the search: Operation by non-privileged user is not supported by this kernel.");
+			else
+				error("can't perform the search: %s",
+						    strerror(-ret));
+		}
 	}
 
-	/*
-	 * now we have an rbtree full of root_info objects, but we need to fill
-	 * in their path names within the subvol that is referencing each one.
-	 */
-	ret = list_subvol_fill_paths(fd, root_lookup);
 	return ret;
 }
 
@@ -1598,12 +1920,12 @@ int btrfs_get_subvol(int fd, struct root_info *the_ri, const char *path)
 		return ret;
 	}
 
+	ret = -ENOENT;
 	rbn = rb_first(&rl.root);
 	while(rbn) {
 		ri = rb_entry(rbn, struct root_info, rb_node);
 		rr = resolve_root(&rl, ri, root_id);
-		if (rr == -ENOENT) {
-			ret = -ENOENT;
+		if (rr == -ENOENT || uuid_is_null(ri->uuid)) {
 			rbn = rb_next(rbn);
 			continue;
 		}
diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index faa10c5a..68de7db7 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -488,6 +488,7 @@ static int cmd_subvol_list(int argc, char **argv)
 	int is_only_in_path = 0;
 	DIR *dirstream = NULL;
 	enum btrfs_list_layout layout = BTRFS_LIST_LAYOUT_DEFAULT;
+	uid_t uid;
 
 	filter_set = btrfs_list_alloc_filter_set();
 	comparer_set = btrfs_list_alloc_comparer_set();
@@ -596,6 +597,13 @@ static int cmd_subvol_list(int argc, char **argv)
 		goto out;
 	}
 
+	uid = geteuid();
+	if (uid != 0 && is_list_all) {
+		ret = -1;
+		error("only root can use -a option");
+		goto out;
+	}
+
 	if (flags)
 		btrfs_list_setup_filter(&filter_set, BTRFS_LIST_FILTER_FLAGS,
 					flags);
-- 
2.14.3

Re: [RFC PATCH v3 5/7] btrfs-progs: sub list: Allow normal user to call "subvolume list/show"

[Goffredo Baroncelli]

On 03/19/2018 08:32 AM, Misono, Tomohiro wrote:
> Allow normal user to call "subvolume list/show" by using 3 new
> unprivileged ioctls (BTRFS_IOC_GET_SUBVOL_INFO,
> BTRFS_IOC_GET_SUBVOL_ROOTREF and BTRFS_IOC_INO_LOOKUP_USER).
> 
> Note that for root, "subvolume list" returns all the subvolume in the
> filesystem by default, but for normal user, it returns subvolumes
> which exist under the specified path (including the path itself).
> The specified path itself is not needed to be a subvolume.
> If the subvolume cannot be opend but the parent directory can be,
> the information other than name or id would be zeroed out.
> 
> Also, for normal user, snapshot filed of "subvolume show" just lists
> the snapshots under the specified subvolume.
> 
> Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
> ---
>  btrfs-list.c     | 344 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  cmds-subvolume.c |   8 ++
>  2 files changed, 341 insertions(+), 11 deletions(-)
> 
> diff --git a/btrfs-list.c b/btrfs-list.c
> index 833ff912..c819499f 100644
> --- a/btrfs-list.c
> +++ b/btrfs-list.c
> @@ -33,6 +33,7 @@
>  #include <uuid/uuid.h>
>  #include "btrfs-list.h"
>  #include "rbtree-utils.h"
> +#include <sys/queue.h>
>  
>  #define BTRFS_LIST_NFILTERS_INCREASE	(2 * BTRFS_LIST_FILTER_MAX)
>  #define BTRFS_LIST_NCOMPS_INCREASE	(2 * BTRFS_LIST_COMP_MAX)
> @@ -549,6 +550,9 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
>  	int len = 0;
>  	struct root_info *found;
>  
> +	if (ri->full_path != NULL)
> +		return 0;
> +
>  	/*
>  	 * we go backwards from the root_info object and add pathnames
>  	 * from parent directories as we go.
> @@ -672,6 +676,50 @@ static int lookup_ino_path(int fd, struct root_info *ri)
>  	return 0;
>  }
>  
> +/* user version of lookup_ino_path which also cheks the access right */
> +static int lookup_ino_path_user(int fd, struct root_info *ri)
> +{
> +	struct btrfs_ioctl_ino_lookup_user_args args;
> +	int ret = 0;
> +
> +	if (ri->path)
> +		return 0;
> +	if (!ri->ref_tree)
> +		return -ENOENT;
> +
> +	memset(&args, 0, sizeof(args));
> +	args.dirid = ri->dir_id;
> +	args.subvolid = ri->root_id;
> +
> +	ret = ioctl(fd, BTRFS_IOC_INO_LOOKUP_USER, &args);
> +	if (ret < 0) {
> +		if (errno == ENOENT) {
> +			ri->ref_tree = 0;
> +			return -ENOENT;
> +		}
> +		if (errno != EACCES) {
> +			error("failed to lookup path for root %llu: %m",
> +			(unsigned long long)ri->ref_tree);
> +			return ret;
> +		} else {
> +			return -EACCES;
> +		}
> +	}
> +
> +	ri->path = malloc(strlen(args.path) + strlen(args.name) + 1);
> +	if (!ri->path)
> +		return -ENOMEM;
> +	strcpy(ri->path, args.path);
> +
> +	ri->name = malloc(strlen(args.name) + 1);
> +	if (!ri->name)
> +		return -ENOMEM;
> +	strcpy(ri->name, args.name);
> +
> +	strcat(ri->path, ri->name);
> +	return ret;
> +}
> +
>  /* finding the generation for a given path is a two step process.
>   * First we use the inode lookup routine to find out the root id
>   *
> @@ -1310,6 +1358,266 @@ static int list_subvol_fill_paths(int fd, struct root_lookup *root_lookup)
>  	return 0;
>  }
>  
> +static int fill_subvol_info(struct root_lookup *root_lookup, int fd)
> +{
> +	struct btrfs_ioctl_get_subvol_info_args subvol_info;
> +	u64 root_offset;
> +	int ret;
> +
> +	ret = ioctl(fd, BTRFS_IOC_GET_SUBVOL_INFO, &subvol_info);
> +	if (ret < 0)
> +		return -errno;
> +
> +	if (!uuid_is_null(subvol_info.parent_uuid))
> +		root_offset = subvol_info.otransid;
> +	else
> +		root_offset = 0;
> +
> +	add_root(root_lookup, subvol_info.id, 0,
> +		 root_offset, subvol_info.flags, subvol_info.dirid,
> +		 NULL, 0,
> +		 subvol_info.otransid, subvol_info.generation,
> +		 subvol_info.otime.sec, subvol_info.uuid,
> +		 subvol_info.parent_uuid, subvol_info.received_uuid);
> +
> +	return 0;
> +}
> +
> +static int fill_subvol_info_top(struct root_lookup *root_lookup, int fd)
> +{
> +	struct btrfs_ioctl_get_subvol_info_args subvol_info;
> +	struct root_info *ri;
> +	u64 root_offset;
> +	int ret;
> +
> +	ret = ioctl(fd, BTRFS_IOC_GET_SUBVOL_INFO, &subvol_info);
> +	if (ret < 0)
> +		return -errno;
> +
> +	if (!uuid_is_null(subvol_info.parent_uuid))
> +		root_offset = subvol_info.otransid;
> +	else
> +		root_offset = 0;
> +
> +	add_root(root_lookup, subvol_info.id, subvol_info.parent_id,
> +		 root_offset, subvol_info.flags, subvol_info.dirid,
> +		 subvol_info.name, strlen(subvol_info.name),
> +		 subvol_info.otransid, subvol_info.generation,
> +		 subvol_info.otime.sec, subvol_info.uuid,
> +		 subvol_info.parent_uuid, subvol_info.received_uuid);
> +
> +	/*
> +	 * fill the path since we won't lookup directory/subvolume
> +	 * above this subvolume and cannot use root_lookup()
> +	 */
> +	ri = root_tree_search(root_lookup, subvol_info.id);
> +	ri->top_id = subvol_info.parent_id;
> +	if (subvol_info.id == BTRFS_FS_TREE_OBJECTID) {
> +		ri->path = strdup("/");
> +		ri->name = strdup("<FS_TREE>");
> +		ri->full_path = strdup("/");
> +		if (!ri->path || !ri->name || !ri->full_path)
> +			return -ENOMEM;
> +	} else {
> +		ri->path = malloc(strlen(ri->name + 1));
> +		ri->full_path = malloc(strlen(ri->name + 1));
> +		if (!ri->path || !ri->full_path)
> +			return -ENOMEM;
> +
> +		strcpy(ri->path, ri->name);
> +		strcpy(ri->full_path, ri->name);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fill_rootref(struct root_lookup *root_lookup, int fd, int parent_id)
> +{
> +	struct btrfs_ioctl_get_subvol_rootref_args rootrefs;
> +	bool continue_search = true;
> +	int i, ret;
> +
> +	memset(&rootrefs, 0, sizeof(rootrefs));
> +	while (continue_search) {
> +		ret = ioctl(fd, BTRFS_IOC_GET_SUBVOL_ROOTREF, &rootrefs);
> +		if (ret < 0) {
> +			if (errno != EOVERFLOW)
> +				return -errno;
> +			continue_search = true;
> +		} else {
> +			continue_search = false;
> +		}
> +
> +		for (i = 0; i < rootrefs.num_items; i++) {
> +			add_root(root_lookup, rootrefs.rootref[i].subvolid,
> +			    parent_id, 0, 0, rootrefs.rootref[i].dirid,
> +			    NULL, 0, 0, 0, 0, NULL, NULL, NULL);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int list_subvol_user(int top_fd, struct root_lookup *root_lookup,
> +				const char *path)
> +{
> +	struct root_info *ri, *parent;
> +	struct rb_node *n;
> +	char *fullpath;
> +	u64 top_id;
> +	/* fifo queue entry which holds subvolume's id */
> +	struct queue_entry {
> +		u64 id;
> +
> +		STAILQ_ENTRY(queue_entry) entries;
> +	} *e, *etemp;
> +	int fd;
> +	int ret = 0;
> +
> +	root_lookup->root.rb_node = NULL;
> +
> +	ret = btrfs_list_get_path_rootid(top_fd, &top_id);
> +	if (ret)
> +		return ret;
> +
> +	/* Add top_id to the queue */
> +	STAILQ_HEAD(slistead, queue_entry) head = STAILQ_HEAD_INITIALIZER(head);
> +	STAILQ_INIT(&head);
> +	e = malloc(sizeof(struct queue_entry));
> +	if (!e)
> +		return -ENOMEM;
> +	e->id = top_id;
> +	STAILQ_INSERT_TAIL(&head, e, entries);
> +
> +	/*
> +	 * Iterate until queue is empty:
> +	 * 1. Pop the first entry
> +	 * 2. Open the entry's path
> +	 * 3  Get the subvolume information by fill_subvol_info/fill_rootref
> +	 * 4. Iterate over rb_tree:
> +	 * 4-1. Searth the rb_tree whose ref_tree is e->id
> +	 *    (this means the subvolume exists under e->id's subvolume)
> +	 * 4-2. Call ino_lookup_user ioctl
> +	 * 4-3. If the call succeeds, add the subvolume id to the queue
> +	 */
> +	while (!STAILQ_EMPTY(&head)) {
> +		e = STAILQ_FIRST(&head);
> +		STAILQ_REMOVE_HEAD(&head, entries);
> +
> +		if (e->id == top_id) {
> +			fd = top_fd;
> +			ret = fill_subvol_info_top(root_lookup, fd);
> +			if (ret)
> +				goto err;
> +			ret = fill_rootref(root_lookup, fd, e->id);
> +			if (ret)
> +				goto err;
> +		} else {
> +			parent = root_tree_search(root_lookup, e->id);
> +			resolve_root(root_lookup, parent, top_id);
> +			fd = openat(top_fd, parent->full_path, O_RDONLY);
> +			if (fd == -1) {
> +				if (errno == EACCES) {
> +					/* skip this subvolume */
> +					continue;
> +				} else {
> +					error("error at open %s: %m",
> +							parent->full_path);
> +					goto err;
> +				}
> +			}
> +
> +			ret = fill_subvol_info(root_lookup, fd);
> +			if (ret)
> +				goto err;
> +			ret = fill_rootref(root_lookup, fd, e->id);
> +			if (ret)
> +				goto err;
> +		}
> +
> +		n = rb_first(&root_lookup->root);
> +		while (n) {
> +			ri = rb_entry(n, struct root_info, rb_node);
> +			if (ri->ref_tree == 0) {
> +			/* BTRFS_FS_TREE_OBJECTID or deleted subvolume */
> +				n = rb_next(n);
> +				continue;
> +			}
> +
> +			if (ri->ref_tree == e->id) {
> +				ret = lookup_ino_path_user(fd, ri);
> +				if (ret < 0 && ret != -ENOENT && ret != -EACCES)
> +					goto err;
> +
> +				/* add ths subvol id to queue */
> +				if (!ret) {
> +					etemp = malloc(sizeof(struct queue_entry));
> +
> +					if (!etemp) {
> +						ret = -ENOMEM;
> +						goto err;
> +					}
> +					etemp->id = ri->root_id;
> +					STAILQ_INSERT_TAIL(&head, etemp,
> +					    entries);
> +				}
> +			}
> +			n = rb_next(n);
> +		}
> +
> +		if (fd != top_fd)
> +			close(fd);
> +		free(e);
> +	}
> +
> +	/* If the specified path itself is not a subvolume, remove the entry */
> +	fullpath = realpath(path, NULL);
> +	if (!fullpath) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +	ret = test_issubvolume(fullpath);
> +	free(fullpath);
> +	if (ret < 0)
> +		goto err;
> +
> +	if (!ret) {
> +		ri = root_tree_search(root_lookup, top_id);
> +		rb_erase(&ri->rb_node, &root_lookup->root);
> +		free_root_info(&ri->rb_node);
> +	}
> +
> +	/* remove skipped entries */
> +	n = rb_first(&root_lookup->root);
> +	while (n) {
> +		ri = rb_entry(n, struct root_info, rb_node);
> +		if (!ri->path) {
> +			struct rb_node *next = rb_next(n);
> +
> +			rb_erase(n, &root_lookup->root);
> +			free_root_info(n);
> +			n = next;
> +		} else {
> +			n = rb_next(n);
> +		}
> +	}
> +
> +	return 0;
> +
> +err:
> +	if (fd != -1 && fd != top_fd)
> +		close(fd);
> +
> +	/* free remaining queue entries */
> +	while (!STAILQ_EMPTY(&head)) {
> +		e = STAILQ_FIRST(&head);
> +		STAILQ_REMOVE_HEAD(&head, entries);
> +		free(e);
> +	}
> +
> +	return ret;
> +}
> +
>  static void print_subvolume_column(struct root_info *subv,
>  				   enum btrfs_list_column_enum column)
>  {
> @@ -1492,19 +1800,33 @@ next:
>  static int btrfs_list_subvols(int fd, struct root_lookup *root_lookup,
>  			const char *path)
>  {
> +	uid_t uid;
>  	int ret;
>  
> -	ret = list_subvol_search(fd, root_lookup);
> -	if (ret) {
> -		error("can't perform the search: %m");
> -		return ret;
> +	uid = geteuid();
> +	if (!uid) {

A minor tip: I think uid == 0 is better here, because we should check that uid is root

> +		ret = list_subvol_search(fd, root_lookup);
> +		if (ret) {
> +			error("can't perform the search: %s", strerror(-ret));
> +			return ret;
> +		}
> +		/*
> +		 * now we have an rbtree full of root_info objects, but we
> +		 * need to fill in their path names within the subvol that
> +		 * is referencing each one.
> +		 */
> +		ret = list_subvol_fill_paths(fd, root_lookup);
> +	} else {
> +		ret = list_subvol_user(fd, root_lookup, path);
> +		if (ret) {
> +			if (ret == -ENOTTY)
> +error("can't perform the search: Operation by non-privileged user is not supported by this kernel.");
> +			else
> +				error("can't perform the search: %s",
> +						    strerror(-ret));
> +		}

Sorry for noticing that only now: which is the reason to not using the new api also in "root" case? I know that the behavior is a bit different, so my question is: it is possible to extend the new ioctls to support also the old behavior in the root case? So we could get rid of the "tree search" ioctl, using it only for the old kernel

>  	}
>  
> -	/*
> -	 * now we have an rbtree full of root_info objects, but we need to fill
> -	 * in their path names within the subvol that is referencing each one.
> -	 */
> -	ret = list_subvol_fill_paths(fd, root_lookup);
>  	return ret;
>  }
>  
> @@ -1598,12 +1920,12 @@ int btrfs_get_subvol(int fd, struct root_info *the_ri, const char *path)
>  		return ret;
>  	}
>  
> +	ret = -ENOENT;
>  	rbn = rb_first(&rl.root);
>  	while(rbn) {
>  		ri = rb_entry(rbn, struct root_info, rb_node);
>  		rr = resolve_root(&rl, ri, root_id);
> -		if (rr == -ENOENT) {
> -			ret = -ENOENT;
> +		if (rr == -ENOENT || uuid_is_null(ri->uuid)) {
>  			rbn = rb_next(rbn);
>  			continue;
>  		}
> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> index faa10c5a..68de7db7 100644
> --- a/cmds-subvolume.c
> +++ b/cmds-subvolume.c
> @@ -488,6 +488,7 @@ static int cmd_subvol_list(int argc, char **argv)
>  	int is_only_in_path = 0;
>  	DIR *dirstream = NULL;
>  	enum btrfs_list_layout layout = BTRFS_LIST_LAYOUT_DEFAULT;
> +	uid_t uid;
>  
>  	filter_set = btrfs_list_alloc_filter_set();
>  	comparer_set = btrfs_list_alloc_comparer_set();
> @@ -596,6 +597,13 @@ static int cmd_subvol_list(int argc, char **argv)
>  		goto out;
>  	}
>  
> +	uid = geteuid();
> +	if (uid != 0 && is_list_all) {
> +		ret = -1;
> +		error("only root can use -a option");
> +		goto out;
> +	}
> +
>  	if (flags)
>  		btrfs_list_setup_filter(&filter_set, BTRFS_LIST_FILTER_FLAGS,
>  					flags);
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [RFC PATCH v3 5/7] btrfs-progs: sub list: Allow normal user to call "subvolume list/show"

[Misono, Tomohiro]

On 2018/03/20 2:09, Goffredo Baroncelli wrote:
> On 03/19/2018 08:32 AM, Misono, Tomohiro wrote
[snip]
>>  static void print_subvolume_column(struct root_info *subv,
>>  				   enum btrfs_list_column_enum column)
>>  {
>> @@ -1492,19 +1800,33 @@ next:
>>  static int btrfs_list_subvols(int fd, struct root_lookup *root_lookup,
>>  			const char *path)
>>  {
>> +	uid_t uid;
>>  	int ret;
>>  
>> -	ret = list_subvol_search(fd, root_lookup);
>> -	if (ret) {
>> -		error("can't perform the search: %m");
>> -		return ret;
>> +	uid = geteuid();
>> +	if (!uid) {
> 
> A minor tip: I think uid == 0 is better here, because we should check that uid is root

ok, thanks.

> 
>> +		ret = list_subvol_search(fd, root_lookup);
>> +		if (ret) {
>> +			error("can't perform the search: %s", strerror(-ret));
>> +			return ret;
>> +		}
>> +		/*
>> +		 * now we have an rbtree full of root_info objects, but we
>> +		 * need to fill in their path names within the subvol that
>> +		 * is referencing each one.
>> +		 */
>> +		ret = list_subvol_fill_paths(fd, root_lookup);
>> +	} else {
>> +		ret = list_subvol_user(fd, root_lookup, path);
>> +		if (ret) {
>> +			if (ret == -ENOTTY)
>> +error("can't perform the search: Operation by non-privileged user is not supported by this kernel.");
>> +			else
>> +				error("can't perform the search: %s",
>> +						    strerror(-ret));
>> +		}
> 
> Sorry for noticing that only now: which is the reason to not using the new api also in "root" case? I know that the behavior is a bit different, so my question is: it is possible to extend the new ioctls to support also the old behavior in the root case? So we could get rid of the "tree search" ioctl, using it only for the old kernel

It is not possible to provide the same functionality for root by current implementation of ioctl.

Currently all three ioctls returns the information of subvolume which contains the fd's inode.
This is because to prevent a user from getting arbitrary subvolume information in the filesystem.

So, in order to work "sub list" with these new ioctls , we need to do:
1. open the path
2. call GET_SUBVOL_INFO to get the subvolume info of opened path.
3. call GET_SUBVOL_ROOTREF to get the list of child subvolume id.
4. call INO_LOOKUP_USER to check if a user can really access the (parent directory of) child subvolume and construct the path.
5. for each accessible child subvolume, repeat 1-5. 

Therefore there is no way to search subvolumes outside of mount point if the default subvolume
has been changed and cannot be used as an alternative of tree search ioctl for root.

If we pass a subvolid to be searched as an argument (allowing for root only), it is possible to use
these ioctls to replace tree search ioctl. However, this means the behavior of ioctl is different from
root and normal user. Is it a good thing? Also, the number of ioctl call certainly increases since 
GET_SUBVOL_INFO/SUBVOL_ROOTREF needs to be called for each subvolume while tree search returns multiple
information at once. I'd like to know others' opinions.

[RFC PATCH v3 6/7] btrfs-progs: test: Add helper function to check if test user exists

[Misono, Tomohiro]

Test user 'progs-test' will be used to test the behavior of normal user.

In order to pass this check, add the user by "useradd -M progs-test".
Note that progs-test should not have root privileges.

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
---
 tests/common | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tests/common b/tests/common
index fae30f1d..e8f7c061 100644
--- a/tests/common
+++ b/tests/common
@@ -307,6 +307,16 @@ check_global_prereq()
 	fi
 }
 
+check_testuser()
+{
+	id -u progs-test > /dev/null 2>&1
+	if [ $? -ne 0 ]; then
+		_not_run "Need to add user \"progs-test\""
+	fi
+	# Note that progs-test should not have root privileges
+	# otherwise test may not run as expected
+}
+
 check_image()
 {
 	local image
-- 
2.14.3

[RFC PATCH v3 7/7] btrfs-porgs: test: Add cli-test/009 to check subvolume list for both root and normal user

[Misono, Tomohiro]

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
---
 tests/cli-tests/009-subvolume-list/test.sh | 136 +++++++++++++++++++++++++++++
 1 file changed, 136 insertions(+)
 create mode 100755 tests/cli-tests/009-subvolume-list/test.sh

diff --git a/tests/cli-tests/009-subvolume-list/test.sh b/tests/cli-tests/009-subvolume-list/test.sh
new file mode 100755
index 00000000..ac00a697
--- /dev/null
+++ b/tests/cli-tests/009-subvolume-list/test.sh
@@ -0,0 +1,136 @@
+#!/bin/bash
+# test for "subvolume list" both for root and normal user
+
+source "$TEST_TOP/common"
+
+check_testuser
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+setup_root_helper
+prepare_test_dev
+
+
+# test if the ids returned by "sub list" match expected ids
+# $1  ... indicate run as root or test user
+# $2  ... PATH to be specified by sub list command
+# $3~ ... expected return ids
+test_list()
+{
+	local SUDO
+	if [ $1 -eq 1 ]; then
+		SUDO=$SUDO_HELPER
+	else
+		SUDO="sudo -u progs-test"
+	fi
+
+	result=$(run_check_stdout $SUDO "$TOP/btrfs" subvolume list "$2" | \
+		awk '{print $2}' | xargs | sort -n)
+
+	shift
+	shift
+	expected=($(echo "$@" | tr " " "\n" | sort -n))
+	expected=$(IFS=" "; echo "${expected[*]}")
+
+	if [ "$result" != "$expected" ]; then
+		echo "result  : $result"
+		echo "expected: $expected"
+		_fail "ids returned by sub list does not match expected ids"
+	fi
+}
+
+run_check $SUDO_HELPER "$TOP/mkfs.btrfs" -f "$TEST_DEV"
+run_check_mount_test_dev
+cd "$TEST_MNT"
+
+# create subvolumes and directories and make some non-readable
+# by user 'progs-test'
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create sub1
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create sub1/subsub1
+run_check $SUDO_HELPER mkdir sub1/dir
+
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create sub2
+run_check $SUDO_HELPER mkdir -p sub2/dir/dirdir
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create sub2/dir/subsub2
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create sub2/dir/dirdir/subsubX
+
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create sub3
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create sub3/subsub3
+run_check $SUDO_HELPER mkdir sub3/dir
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create sub3/dir/subsubY
+run_check $SUDO_HELPER chmod o-r sub3
+
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create sub4
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create sub4/subsub4
+run_check $SUDO_HELPER mkdir sub4/dir
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create sub4/dir/subsubZ
+run_check $SUDO_HELPER setfacl -m u:progs-test:- sub4/dir
+
+run_check $SUDO_HELPER touch "file"
+
+# expected result for root:
+#
+# ID 257 gen 9 top level 5 path sub1
+# ID 258 gen 8 top level 257 path sub1/subsub1
+# ID 259 gen 11 top level 5 path sub2
+# ID 260 gen 10 top level 259 path sub2/dir/subsub2
+# ID 261 gen 11 top level 259 path sub2/dir/dirdir/subsubX
+# ID 262 gen 15 top level 5 path sub3
+# ID 263 gen 13 top level 262 path sub3/subsub3
+# ID 264 gen 14 top level 262 path sub3/dir/subsubY
+# ID 265 gen 17 top level 5 path sub4
+# ID 266 gen 16 top level 265 path sub4/subsub4
+# ID 267 gen 17 top level 265 path sub4/dir/subsubZ
+
+# check for root for both absolute/relative path
+# always returns all subvolumes
+all=$(seq 257 267)
+test_list 1 "$TEST_MNT" "${all[@]}"
+test_list 1 "$TEST_MNT/sub1" "${all[@]}"
+test_list 1 "$TEST_MNT/sub1/dir" "${all[@]}"
+test_list 1 "$TEST_MNT/sub2" "${all[@]}"
+test_list 1 "$TEST_MNT/sub2/dir" "${all[@]}"
+test_list 1 "$TEST_MNT/sub3" "${all[@]}"
+test_list 1 "$TEST_MNT/sub4" "${all[@]}"
+run_mustfail "should fail for file" \
+	$SUDO_HELPER "$TOP/btrfs" subvolume list "$TEST_MNT/file"
+
+test_list 1 "." "${all[@]}"
+test_list 1 "sub1" "${all[@]}"
+test_list 1 "sub1/dir" "${all[@]}"
+test_list 1 "sub2" "${all[@]}"
+test_list 1 "sub2/dir" "${all[@]}"
+test_list 1 "sub3" "${all[@]}"
+test_list 1 "sub4" "${all[@]}"
+run_mustfail "should fail for file" \
+	$SUDO_HELPER "$TOP/btrfs" subvolume list "file"
+
+# check for normal user for both absolute/relative path
+# only returns subvolumes under specified path
+test_list 0 "$TEST_MNT" "257 258 259 260 261 262 265 266"
+test_list 0 "$TEST_MNT/sub1" "257 258"
+test_list 0 "$TEST_MNT/sub1/dir" ""
+test_list 0 "$TEST_MNT/sub2" "259 260 261"
+test_list 0 "$TEST_MNT/sub2/dir" "260 261"
+run_mustfail "should raise permission error" \
+	sudo -u progs-test "$TOP/btrfs" subvolume list "$TEST_MNT/sub3"
+test_list 0 "$TEST_MNT/sub4" "265 266"
+run_mustfail "should raise permission error" \
+	sudo -u progs-test "$TOP/btrfs" subvolume list "$TEST_MNT/sub4/dir"
+run_mustfail "should fail for file" \
+	sudo -u progs-test "$TOP/btrfs" subvolume list "$TEST_MNT/file"
+
+test_list 0 "." "257 258 259 260 261 262 265 266"
+test_list 0 "sub1/dir" ""
+test_list 0 "sub2" "259 260 261"
+test_list 0 "sub2/dir" "260 261"
+run_mustfail "should raise permission error" \
+	sudo -u progs-test "$TOP/btrfs" subvolume list "sub3"
+test_list 0 "sub4" "265 266"
+run_mustfail "should raise permission error" \
+	sudo -u progs-test "$TOP/btrfs" subvolume list "sub4/dir"
+run_mustfail "should fail for file" \
+	sudo -u progs-test "$TOP/btrfs" subvolume list "file"
+
+cd ..
+run_check_umount_test_dev
-- 
2.14.3

Re: [RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"

[Zygo Blaxell]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Mon, Mar 19, 2018 at 04:30:17PM +0900, Misono, Tomohiro wrote:
> This is a part of RFC I sent last December[1] whose aim is to improve normal users' usability.
> The remaining works of RFC are: 
>   - Allow "sub delete" for empty subvolume

I don't mean to scope creep on you, but I have a couple of wishes related
to this topic:

  - allow "rmdir" to remove an empty subvolume, i.e. when a subvolume is
    detected in rmdir, try switching to subvol delete before returning
    an error.  This lets admin tools that are not btrfs-aware do 'rm
    -fr' on a user directory when it contains a subvolume.  Legacy admin
    tools (or legacy tools in general) can't remove a subvol, and there
    is no solution for environments where we can't just fire users who
    create them.

  - mount option to restrict "sub create" and "sub snapshot" to root only.
    If we get "rmdir" working then this is significantly less important.

>   - Allow "qgroup show" to check quota limit
> 
> [1] https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg70991.html


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"

[Zygo Blaxell]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

On Wed, Mar 28, 2018 at 09:45:02AM -0400, Zygo Blaxell wrote:
> On Mon, Mar 19, 2018 at 04:30:17PM +0900, Misono, Tomohiro wrote:
> > This is a part of RFC I sent last December[1] whose aim is to improve normal users' usability.
> > The remaining works of RFC are: 
> >   - Allow "sub delete" for empty subvolume
> 
> I don't mean to scope creep on you, but I have a couple of wishes related
> to this topic:
> 
>   - allow "rmdir" to remove an empty subvolume, i.e. when a subvolume is

Ignore me... I just noticed your patch to do exactly this.  ;)

>     detected in rmdir, try switching to subvol delete before returning
>     an error.  This lets admin tools that are not btrfs-aware do 'rm
>     -fr' on a user directory when it contains a subvolume.  Legacy admin
>     tools (or legacy tools in general) can't remove a subvol, and there
>     is no solution for environments where we can't just fire users who
>     create them.
> 
>   - mount option to restrict "sub create" and "sub snapshot" to root only.
>     If we get "rmdir" working then this is significantly less important.
> 
> >   - Allow "qgroup show" to check quota limit
> > 
> > [1] https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg70991.html
> 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"

[Goffredo Baroncelli]

Hi Misono,

I tested you patch, and it seems to work. I verified that the output is correct and the permission check is performed. However the output of "btrfs sub list" in the "root" mode and the "user" mode are a bit different.

As reported before, I find your output more consistent, and understandable than the original one, which is quite confusing about the sub-volume path relationship. 

The problem which I am talking is probably due to the fact that I mount a subvolume as root filesystem, and the root-subvolume (the one with ID=5) in a subdirectory (/var/btrfs for what matters).

I think that the "stock" "btrfs sub list", trying to scan the metadata of btrfs and merging these information with this filesystem layout (composed by different subvolumes mounted in different places) doesn't do a good job.

The constraints that your API has (the fact that the subvolume has to be accessed by the filesystem), create an output more friendly. Unfortunately this output became again confusing when the command is started by root.

[Un]fortunately, the stock "btrfs sub list" has the capability to show all the subvolumes, even the ones not mounted, so this can be entirely replaced by your API.

My conclusion, is that your work is not consistent with the current implementation; so I am suggesting to create a new subcommand ("btrfs sub tree" ?) where you can use, without constraint, your api.

Another small differences that I found is in the subcommand "btrfs sub show". This is not capable to show the snapshots of a subvolume; I think that, in "user mode", it should be reported that there is a "missing capabilities" problem than to show an empty list

Below an example of what I am say:

ghigo@venice:/tmp$ btrfs sub cre a
ghigo@venice:/tmp$ btrfs sub snap a c

ghigo@venice:/tmp$ # patched btrfs
ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
tmp/a
	Name: 			a
	UUID: 			0c19a7a4-a7f5-1849-9262-88ab3da504d0
	Parent UUID: 		-
	Received UUID: 		-
	Creation time: 		2018-03-28 22:48:09 +0200
	Subvolume ID: 		598
	Generation: 		696908
	Gen at creation: 	696907
	Parent ID: 		257
	Top level ID: 		257
	Flags: 			-
	Snapshot(s):
                            ^^^^^^^^^^^^^^^^^^^^

ghigo@venice:/tmp$ # stock btrfs
ghigo@venice:/tmp$ sudo /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
                  ^^^^^^
tmp/a
	Name: 			a
	UUID: 			0c19a7a4-a7f5-1849-9262-88ab3da504d0
	Parent UUID: 		-
	Received UUID: 		-
	Creation time: 		2018-03-28 22:48:09 +0200
	Subvolume ID: 		598
	Generation: 		696908
	Gen at creation: 	696907
	Parent ID: 		257
	Top level ID: 		257
	Flags: 			-
	Snapshot(s):
				debian/tmp/c
                                ^^^^^^^^^^^^^
                               

BR
G.Baroncelli


-----
Test performed:

ghigo@venice:/tmp$ sudo btrfs sub crea a
ghigo@venice:/tmp$ sudo btrfs sub crea a/a1
ghigo@venice:/tmp$ sudo btrfs sub crea a/a1/a2
ghigo@venice:/tmp$ sudo btrfs sub crea b
ghigo@venice:/tmp$ sudo btrfs sub crea b/b1
ghigo@venice:/tmp$ sudo chmod og-rwx b/.


# stock btrfs progs
ghigo@venice:/tmp$ sudo btrfs sub l .
ID 257 gen 696886 top level 5 path debian
ID 289 gen 587461 top level 257 path var/lib/machines
ID 299 gen 693561 top level 5 path boot
ID 582 gen 683965 top level 5 path i386
ID 592 gen 696884 top level 257 path tmp/a
ID 593 gen 696885 top level 592 path tmp/a/a1
ID 594 gen 696885 top level 593 path tmp/a/a1/a2
ID 595 gen 696887 top level 257 path tmp/b
ID 596 gen 696887 top level 595 path tmp/b/b1

# patched btrfs progs
ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub lis .
ID 592 gen 696884 top level 257 path a
ID 593 gen 696885 top level 592 path a/a1
ID 594 gen 696885 top level 593 path a/a1/a2
ID 595 gen 0 top level 257 path b


ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
tmp/a
	Name: 			a
	UUID: 			17520c11-ec8b-5b49-a07e-37ba58113261
	Parent UUID: 		-
	Received UUID: 		-
	Creation time: 		2018-03-28 22:19:48 +0200
	Subvolume ID: 		592
	Generation: 		696884
	Gen at creation: 	696883
	Parent ID: 		257
	Top level ID: 		257
	Flags: 			-
	Snapshot(s):


On 03/19/2018 08:30 AM, Misono, Tomohiro wrote:
> changelog:
> 
> v2 -> v3
>   - use get_euid() to check the caller's privilege (and remove 3rd patch)
>   - improve error handling
> v1 -> v2
>   - add independent error handling patch (1st patch)
>   - reimplement according to ioctl change
>   - various cleanup
> ===
> 
> This RFC implements user version of "subvolume list/show" using three new ioctls.
> The ioctl patch to the kernel can be found in the ML titled 
>   "[PATCH v3 0/3] btrfs: Add three new unprivileged ioctls to allow normal users to call "sub list/show" etc.
> 
> 1th patch is independent and improvements of error handling
> 2nd-4th are some prepartion works.
> 5th patch is the main part.
> 6th-7th adds the test for "subvolume list"
> 
> The main behavior differences between root and normal users are:
> 
> - "sub list" list the subvolumes which exist under the specified path 
> (including the path itself). The specified path itself is not needed to be
>  a subvolume. Also If the subvolume cannot be opend but the parent
> directory can be, the information other than name or id would be zeroed out.
> 
> - snapshot filed of "subvolume show" just lists
> the snapshots under the specified subvolume.
> 
> 
> This is a part of RFC I sent last December[1] whose aim is to improve normal users' usability.
> The remaining works of RFC are: 
>   - Allow "sub delete" for empty subvolume
>   - Allow "qgroup show" to check quota limit
> 
> [1] https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg70991.html
> 
> 
> Tomohiro Misono (7):
>   btrfs-progs: sub list: Call rb_free_nodes() in error path
>   btrfs-progs: ioctl: Add 3 definitions of new unprivileged ioctl
>   btrfs-progs: sub list: Pass specified path down to
>     btrfs_list_subvols()
>   btrfs-progs: fallback to open without O_NOATIME flag in
>     find_mount_root()
>   btrfs-progs: sub list: Allow normal user to call "subvolume list/show"
>   btrfs-progs: test: Add helper function to check if test user exists
>   btrfs-porgs: test: Add cli-test/009 to check subvolume list for both
>     root and normal user
> 
>  btrfs-list.c                               | 376 +++++++++++++++++++++++++++--
>  btrfs-list.h                               |   7 +-
>  cmds-subvolume.c                           |  14 +-
>  ioctl.h                                    |  86 +++++++
>  tests/cli-tests/009-subvolume-list/test.sh | 136 +++++++++++
>  tests/common                               |  10 +
>  utils.c                                    |  13 +-
>  7 files changed, 609 insertions(+), 33 deletions(-)
>  create mode 100755 tests/cli-tests/009-subvolume-list/test.sh
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"

[Misono Tomohiro]

On 2018/03/30 2:35, Goffredo Baroncelli wrote:
> Hi Misono,

Hello,

> 
> I tested you patch, and it seems to work. I verified that the output is correct and the permission check is performed. However the output of "btrfs sub list" in the "root" mode and the "user" mode are a bit different.
> 
> As reported before, I find your output more consistent, and understandable than the original one, which is quite confusing about the sub-volume path relationship. 
> 
> The problem which I am talking is probably due to the fact that I mount a subvolume as root filesystem, and the root-subvolume (the one with ID=5) in a subdirectory (/var/btrfs for what matters).

I didn't think this situation and thanks for pointing out.

Currently this RFC does not care about mounted subvolumes under the specified path.
However, it should be able to search those subvolumes too and list up all the subvolumes
including mounted ones (and below it), by changing progs code.

> 
> I think that the "stock" "btrfs sub list", trying to scan the metadata of btrfs and merging these information with this filesystem layout (composed by different subvolumes mounted in different places) doesn't do a good job.
> 
> The constraints that your API has (the fact that the subvolume has to be accessed by the filesystem), create an output more friendly. Unfortunately this output became again confusing when the command is started by root.
> 
> [Un]fortunately, the stock "btrfs sub list" has the capability to show all the subvolumes, even the ones not mounted, so this can be entirely replaced by your API.
> 
> My conclusion, is that your work is not consistent with the current implementation; so I am suggesting to create a new subcommand ("btrfs sub tree" ?) where you can use, without constraint, your api.

I agree that the current output of "sub list" is confusing first of all and
it is not easy to keep consistent behavior between user and root.

So, I think "sub list" (or new command) should be:
  - (default) list the subvolumes under the specified path, including subvolumes mounted below the specified path.
              Any user can do this (with appropriate permission checks).
              The path to be printed is the relative from the specified path.
  - (-a option) list all the subvolmumes in the filesystem. Only root can do this.
              The path to be printed is the absolute path from the toplevel subvolume.

This would need some changes in progs code, but I think we can use the same new ioctls I proposed[1] for the default behavior.

I'd like to update "sub list" command eventually rather than adding new command, even though this breaks the compatibility.
I want to know what other developer/user think.

[1] https://www.spinics.net/lists/linux-btrfs/msg76003.html

> 
> Another small differences that I found is in the subcommand "btrfs sub show". This is not capable to show the snapshots of a subvolume; I think that, in "user mode", it should be reported that there is a "missing capabilities" problem than to show an empty list

Yes, this is because that only the snapshots *under the specified path* are searched.
This can be easily changed to search under the mount point, but this also results in
slight change in print format of the path for root. I tried to keep as much consistency in this version.

Thanks,
Tomohiro Misono

> 
> Below an example of what I am say:
> 
> ghigo@venice:/tmp$ btrfs sub cre a
> ghigo@venice:/tmp$ btrfs sub snap a c
> 
> ghigo@venice:/tmp$ # patched btrfs
> ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
> tmp/a
> 	Name: 			a
> 	UUID: 			0c19a7a4-a7f5-1849-9262-88ab3da504d0
> 	Parent UUID: 		-
> 	Received UUID: 		-
> 	Creation time: 		2018-03-28 22:48:09 +0200
> 	Subvolume ID: 		598
> 	Generation: 		696908
> 	Gen at creation: 	696907
> 	Parent ID: 		257
> 	Top level ID: 		257
> 	Flags: 			-
> 	Snapshot(s):
>                             ^^^^^^^^^^^^^^^^^^^^
> 
> ghigo@venice:/tmp$ # stock btrfs
> ghigo@venice:/tmp$ sudo /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
>                   ^^^^^^
> tmp/a
> 	Name: 			a
> 	UUID: 			0c19a7a4-a7f5-1849-9262-88ab3da504d0
> 	Parent UUID: 		-
> 	Received UUID: 		-
> 	Creation time: 		2018-03-28 22:48:09 +0200
> 	Subvolume ID: 		598
> 	Generation: 		696908
> 	Gen at creation: 	696907
> 	Parent ID: 		257
> 	Top level ID: 		257
> 	Flags: 			-
> 	Snapshot(s):
> 				debian/tmp/c
>                                 ^^^^^^^^^^^^^
>                                
> 
> BR
> G.Baroncelli
> 
> 
> -----
> Test performed:
> 
> ghigo@venice:/tmp$ sudo btrfs sub crea a
> ghigo@venice:/tmp$ sudo btrfs sub crea a/a1
> ghigo@venice:/tmp$ sudo btrfs sub crea a/a1/a2
> ghigo@venice:/tmp$ sudo btrfs sub crea b
> ghigo@venice:/tmp$ sudo btrfs sub crea b/b1
> ghigo@venice:/tmp$ sudo chmod og-rwx b/.
> 
> 
> # stock btrfs progs
> ghigo@venice:/tmp$ sudo btrfs sub l .
> ID 257 gen 696886 top level 5 path debian
> ID 289 gen 587461 top level 257 path var/lib/machines
> ID 299 gen 693561 top level 5 path boot
> ID 582 gen 683965 top level 5 path i386
> ID 592 gen 696884 top level 257 path tmp/a
> ID 593 gen 696885 top level 592 path tmp/a/a1
> ID 594 gen 696885 top level 593 path tmp/a/a1/a2
> ID 595 gen 696887 top level 257 path tmp/b
> ID 596 gen 696887 top level 595 path tmp/b/b1
> 
> # patched btrfs progs
> ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub lis .
> ID 592 gen 696884 top level 257 path a
> ID 593 gen 696885 top level 592 path a/a1
> ID 594 gen 696885 top level 593 path a/a1/a2
> ID 595 gen 0 top level 257 path b
> 
> 
> ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
> tmp/a
> 	Name: 			a
> 	UUID: 			17520c11-ec8b-5b49-a07e-37ba58113261
> 	Parent UUID: 		-
> 	Received UUID: 		-
> 	Creation time: 		2018-03-28 22:19:48 +0200
> 	Subvolume ID: 		592
> 	Generation: 		696884
> 	Gen at creation: 	696883
> 	Parent ID: 		257
> 	Top level ID: 		257
> 	Flags: 			-
> 	Snapshot(s):
> 
> 
> On 03/19/2018 08:30 AM, Misono, Tomohiro wrote:
>> changelog:
>>
>> v2 -> v3
>>   - use get_euid() to check the caller's privilege (and remove 3rd patch)
>>   - improve error handling
>> v1 -> v2
>>   - add independent error handling patch (1st patch)
>>   - reimplement according to ioctl change
>>   - various cleanup
>> ===
>>
>> This RFC implements user version of "subvolume list/show" using three new ioctls.
>> The ioctl patch to the kernel can be found in the ML titled 
>>   "[PATCH v3 0/3] btrfs: Add three new unprivileged ioctls to allow normal users to call "sub list/show" etc.
>>
>> 1th patch is independent and improvements of error handling
>> 2nd-4th are some prepartion works.
>> 5th patch is the main part.
>> 6th-7th adds the test for "subvolume list"
>>
>> The main behavior differences between root and normal users are:
>>
>> - "sub list" list the subvolumes which exist under the specified path 
>> (including the path itself). The specified path itself is not needed to be
>>  a subvolume. Also If the subvolume cannot be opend but the parent
>> directory can be, the information other than name or id would be zeroed out.
>>
>> - snapshot filed of "subvolume show" just lists
>> the snapshots under the specified subvolume.
>>
>>
>> This is a part of RFC I sent last December[1] whose aim is to improve normal users' usability.
>> The remaining works of RFC are: 
>>   - Allow "sub delete" for empty subvolume
>>   - Allow "qgroup show" to check quota limit
>>
>> [1] https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg70991.html
>>
>>
>> Tomohiro Misono (7):
>>   btrfs-progs: sub list: Call rb_free_nodes() in error path
>>   btrfs-progs: ioctl: Add 3 definitions of new unprivileged ioctl
>>   btrfs-progs: sub list: Pass specified path down to
>>     btrfs_list_subvols()
>>   btrfs-progs: fallback to open without O_NOATIME flag in
>>     find_mount_root()
>>   btrfs-progs: sub list: Allow normal user to call "subvolume list/show"
>>   btrfs-progs: test: Add helper function to check if test user exists
>>   btrfs-porgs: test: Add cli-test/009 to check subvolume list for both
>>     root and normal user
>>
>>  btrfs-list.c                               | 376 +++++++++++++++++++++++++++--
>>  btrfs-list.h                               |   7 +-
>>  cmds-subvolume.c                           |  14 +-
>>  ioctl.h                                    |  86 +++++++
>>  tests/cli-tests/009-subvolume-list/test.sh | 136 +++++++++++
>>  tests/common                               |  10 +
>>  utils.c                                    |  13 +-
>>  7 files changed, 609 insertions(+), 33 deletions(-)
>>  create mode 100755 tests/cli-tests/009-subvolume-list/test.sh
>>
> 
> 

Re: [RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"

[Goffredo Baroncelli]

On 03/30/2018 06:46 AM, Misono Tomohiro wrote:
> [Un]fortunately, the stock "btrfs sub list" has the capability to show all the subvolumes, even the ones not mounted, so this can be entirely replaced by your API.

s/can be entirely/can't be entirely/

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"

[Goffredo Baroncelli]

On 03/30/2018 06:46 AM, Misono Tomohiro wrote:
> On 2018/03/30 2:35, Goffredo Baroncelli wrote:
>> Hi Misono,
> 
> Hello,
[...]
>> My conclusion, is that your work is not consistent with the current implementation; so I am suggesting to create a new subcommand ("btrfs sub tree" ?) where you can use, without constraint, your api.
> 
> I agree that the current output of "sub list" is confusing first of all and
> it is not easy to keep consistent behavior between user and root.
> 
> So, I think "sub list" (or new command) should be:
>   - (default) list the subvolumes under the specified path, including subvolumes mounted below the specified path.
>               Any user can do this (with appropriate permission checks).
>               The path to be printed is the relative from the specified path.
>   - (-a option) list all the subvolmumes in the filesystem. Only root can do this.
>               The path to be printed is the absolute path from the toplevel subvolume.
> 
> This would need some changes in progs code, but I think we can use the same new ioctls I proposed[1] for the default behavior.



> 
> I'd like to update "sub list" command eventually rather than adding new command, even though this breaks the compatibility.
> I want to know what other developer/user think.
> 
> [1] https://www.spinics.net/lists/linux-btrfs/msg76003.html
> 
>>
>> Another small differences that I found is in the subcommand "btrfs sub show". This is not capable to show the snapshots of a subvolume; I think that, in "user mode", it should be reported that there is a "missing capabilities" problem than to show an empty list
> 
> Yes, this is because that only the snapshots *under the specified path* are searched.
> This can be easily changed to search under the mount point, but this also results in
> slight change in print format of the path for root. I tried to keep as much consistency in this version.

When I perform a snapshot, I think the new snapshot more as a sister/brother than a child, so I put it at the same level instead of below the source subvolume. Moreover the path printed at the first line seems to be relative at the root of filesystem.

> Thanks,
> Tomohiro Misono
> 
>>
>> Below an example of what I am say:
>>
>> ghigo@venice:/tmp$ btrfs sub cre a
>> ghigo@venice:/tmp$ btrfs sub snap a c
>>
>> ghigo@venice:/tmp$ # patched btrfs
>> ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
>> tmp/a
>> 	Name: 			a
>> 	UUID: 			0c19a7a4-a7f5-1849-9262-88ab3da504d0
>> 	Parent UUID: 		-
>> 	Received UUID: 		-
>> 	Creation time: 		2018-03-28 22:48:09 +0200
>> 	Subvolume ID: 		598
>> 	Generation: 		696908
>> 	Gen at creation: 	696907
>> 	Parent ID: 		257
>> 	Top level ID: 		257
>> 	Flags: 			-
>> 	Snapshot(s):
>>                             ^^^^^^^^^^^^^^^^^^^^
>>
>> ghigo@venice:/tmp$ # stock btrfs
>> ghigo@venice:/tmp$ sudo /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
>>                   ^^^^^^
>> tmp/a
>> 	Name: 			a
>> 	UUID: 			0c19a7a4-a7f5-1849-9262-88ab3da504d0
>> 	Parent UUID: 		-
>> 	Received UUID: 		-
>> 	Creation time: 		2018-03-28 22:48:09 +0200
>> 	Subvolume ID: 		598
>> 	Generation: 		696908
>> 	Gen at creation: 	696907
>> 	Parent ID: 		257
>> 	Top level ID: 		257
>> 	Flags: 			-
>> 	Snapshot(s):
>> 				debian/tmp/c
>>                                 ^^^^^^^^^^^^^
>>                                
>>
>> BR
>> G.Baroncelli
>>
>>
>> -----
>> Test performed:
>>
>> ghigo@venice:/tmp$ sudo btrfs sub crea a
>> ghigo@venice:/tmp$ sudo btrfs sub crea a/a1
>> ghigo@venice:/tmp$ sudo btrfs sub crea a/a1/a2
>> ghigo@venice:/tmp$ sudo btrfs sub crea b
>> ghigo@venice:/tmp$ sudo btrfs sub crea b/b1
>> ghigo@venice:/tmp$ sudo chmod og-rwx b/.
>>
>>
>> # stock btrfs progs
>> ghigo@venice:/tmp$ sudo btrfs sub l .
>> ID 257 gen 696886 top level 5 path debian
>> ID 289 gen 587461 top level 257 path var/lib/machines
>> ID 299 gen 693561 top level 5 path boot
>> ID 582 gen 683965 top level 5 path i386
>> ID 592 gen 696884 top level 257 path tmp/a
>> ID 593 gen 696885 top level 592 path tmp/a/a1
>> ID 594 gen 696885 top level 593 path tmp/a/a1/a2
>> ID 595 gen 696887 top level 257 path tmp/b
>> ID 596 gen 696887 top level 595 path tmp/b/b1
>>
>> # patched btrfs progs
>> ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub lis .
>> ID 592 gen 696884 top level 257 path a
>> ID 593 gen 696885 top level 592 path a/a1
>> ID 594 gen 696885 top level 593 path a/a1/a2
>> ID 595 gen 0 top level 257 path b
>>
>>
>> ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
>> tmp/a
>> 	Name: 			a
>> 	UUID: 			17520c11-ec8b-5b49-a07e-37ba58113261
>> 	Parent UUID: 		-
>> 	Received UUID: 		-
>> 	Creation time: 		2018-03-28 22:19:48 +0200
>> 	Subvolume ID: 		592
>> 	Generation: 		696884
>> 	Gen at creation: 	696883
>> 	Parent ID: 		257
>> 	Top level ID: 		257
>> 	Flags: 			-
>> 	Snapshot(s):
>>
>>
>> On 03/19/2018 08:30 AM, Misono, Tomohiro wrote:
>>> changelog:
>>>
>>> v2 -> v3
>>>   - use get_euid() to check the caller's privilege (and remove 3rd patch)
>>>   - improve error handling
>>> v1 -> v2
>>>   - add independent error handling patch (1st patch)
>>>   - reimplement according to ioctl change
>>>   - various cleanup
>>> ===
>>>
>>> This RFC implements user version of "subvolume list/show" using three new ioctls.
>>> The ioctl patch to the kernel can be found in the ML titled 
>>>   "[PATCH v3 0/3] btrfs: Add three new unprivileged ioctls to allow normal users to call "sub list/show" etc.
>>>
>>> 1th patch is independent and improvements of error handling
>>> 2nd-4th are some prepartion works.
>>> 5th patch is the main part.
>>> 6th-7th adds the test for "subvolume list"
>>>
>>> The main behavior differences between root and normal users are:
>>>
>>> - "sub list" list the subvolumes which exist under the specified path 
>>> (including the path itself). The specified path itself is not needed to be
>>>  a subvolume. Also If the subvolume cannot be opend but the parent
>>> directory can be, the information other than name or id would be zeroed out.
>>>
>>> - snapshot filed of "subvolume show" just lists
>>> the snapshots under the specified subvolume.
>>>
>>>
>>> This is a part of RFC I sent last December[1] whose aim is to improve normal users' usability.
>>> The remaining works of RFC are: 
>>>   - Allow "sub delete" for empty subvolume
>>>   - Allow "qgroup show" to check quota limit
>>>
>>> [1] https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg70991.html
>>>
>>>
>>> Tomohiro Misono (7):
>>>   btrfs-progs: sub list: Call rb_free_nodes() in error path
>>>   btrfs-progs: ioctl: Add 3 definitions of new unprivileged ioctl
>>>   btrfs-progs: sub list: Pass specified path down to
>>>     btrfs_list_subvols()
>>>   btrfs-progs: fallback to open without O_NOATIME flag in
>>>     find_mount_root()
>>>   btrfs-progs: sub list: Allow normal user to call "subvolume list/show"
>>>   btrfs-progs: test: Add helper function to check if test user exists
>>>   btrfs-porgs: test: Add cli-test/009 to check subvolume list for both
>>>     root and normal user
>>>
>>>  btrfs-list.c                               | 376 +++++++++++++++++++++++++++--
>>>  btrfs-list.h                               |   7 +-
>>>  cmds-subvolume.c                           |  14 +-
>>>  ioctl.h                                    |  86 +++++++
>>>  tests/cli-tests/009-subvolume-list/test.sh | 136 +++++++++++
>>>  tests/common                               |  10 +
>>>  utils.c                                    |  13 +-
>>>  7 files changed, 609 insertions(+), 33 deletions(-)
>>>  create mode 100755 tests/cli-tests/009-subvolume-list/test.sh
>>>
>>
>>
> 
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5