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

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

[Misono, Tomohiro]

changelog:
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 v2 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-5th are some preparation works.
6th patch is the main part.
7th-8th adds the test for "subvolume list"

The main behavior differences between root and normal users are:

1. "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 opened but the parent
directory can be, the information other than name or id would be zeroed out.

2. 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 (8):
  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: Add helper function which checks the permission
    for tree search 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                               | 391 +++++++++++++++++++++++++++--
 btrfs-list.h                               |   8 +-
 cmds-subvolume.c                           |  19 +-
 ioctl.h                                    |  86 +++++++
 tests/cli-tests/009-subvolume-list/test.sh | 136 ++++++++++
 tests/common                               |  10 +
 utils.c                                    |  13 +-
 7 files changed, 634 insertions(+), 29 deletions(-)
 create mode 100755 tests/cli-tests/009-subvolume-list/test.sh

-- 
2.14.3

[RFC PATCH v2 1/8] 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 v2 2/8] 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 v2 3/8] btrfs-progs: sub list: Add helper function which checks the permission for tree search ioctl

[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 | 33 +++++++++++++++++++++++++++++++++
 btrfs-list.h |  1 +
 2 files changed, 34 insertions(+)

diff --git a/btrfs-list.c b/btrfs-list.c
index 50e5ce5f..88689a9d 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -958,6 +958,39 @@ out:
 	return 0;
 }
 
+/*
+ * Check the permission for tree search ioctl by searching a key
+ * which alwasys exists
+ */
+int check_perm_for_tree_search(int fd)
+{
+	struct btrfs_ioctl_search_args args;
+	struct btrfs_ioctl_search_key *sk = &args.key;
+	int ret;
+
+	memset(&args, 0, sizeof(args));
+	sk->tree_id = BTRFS_ROOT_TREE_OBJECTID;
+	sk->min_objectid = BTRFS_EXTENT_TREE_OBJECTID;
+	sk->max_objectid = BTRFS_EXTENT_TREE_OBJECTID;
+	sk->min_type = BTRFS_ROOT_ITEM_KEY;
+	sk->max_type = BTRFS_ROOT_ITEM_KEY;
+	sk->min_offset = 0;
+	sk->max_offset = (u64)-1;
+	sk->min_transid = 0;
+	sk->max_transid = (u64)-1;
+	sk->nr_items = 1;
+	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
+
+	if (!ret)
+		ret = 1;
+	else if (ret < 0 && errno == EPERM)
+		ret = 0;
+	else
+		ret = -errno;
+
+	return ret;
+}
+
 static int list_subvol_search(int fd, struct root_lookup *root_lookup)
 {
 	int ret;
diff --git a/btrfs-list.h b/btrfs-list.h
index 6e5fc778..6225311d 100644
--- a/btrfs-list.h
+++ b/btrfs-list.h
@@ -176,5 +176,6 @@ 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 check_perm_for_tree_search(int fd);
 
 #endif
-- 
2.14.3

Re: [RFC PATCH v2 3/8] btrfs-progs: sub list: Add helper function which checks the permission for tree search ioctl

[Goffredo Baroncelli]

On 03/15/2018 09:13 AM, Misono, Tomohiro wrote:
> 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 | 33 +++++++++++++++++++++++++++++++++
>  btrfs-list.h |  1 +
>  2 files changed, 34 insertions(+)
> 
> diff --git a/btrfs-list.c b/btrfs-list.c
> index 50e5ce5f..88689a9d 100644
> --- a/btrfs-list.c
> +++ b/btrfs-list.c
> @@ -958,6 +958,39 @@ out:
>  	return 0;
>  }
>  
> +/*
> + * Check the permission for tree search ioctl by searching a key
> + * which alwasys exists
> + */
> +int check_perm_for_tree_search(int fd)
> +{

In what this is different from '(getuid() == 0)' ? 
Which would be the cases where an error different from EPERM would be returned (other than the filesystem is not a BTRFS one )?

I am not against to this kind of function. However with this implementation is not clear its behavior:
- does it check if the user has enough privileges to perform a BTRFS_IOC_TREE_SEARCH
or 
- does it check if BTRFS_IOC_TREE_SEARCH might return some error ?

If the former, I would put this function into utils.[hc] checking only the [e]uid of the user. If the latter the name is confusing...

> +	struct btrfs_ioctl_search_args args;
> +	struct btrfs_ioctl_search_key *sk = &args.key;
> +	int ret;
> +
> +	memset(&args, 0, sizeof(args));
> +	sk->tree_id = BTRFS_ROOT_TREE_OBJECTID;
> +	sk->min_objectid = BTRFS_EXTENT_TREE_OBJECTID;
> +	sk->max_objectid = BTRFS_EXTENT_TREE_OBJECTID;
> +	sk->min_type = BTRFS_ROOT_ITEM_KEY;
> +	sk->max_type = BTRFS_ROOT_ITEM_KEY;
> +	sk->min_offset = 0;
> +	sk->max_offset = (u64)-1;
> +	sk->min_transid = 0;
> +	sk->max_transid = (u64)-1;
> +	sk->nr_items = 1;
> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
> +
> +	if (!ret)
> +		ret = 1;
> +	else if (ret < 0 && errno == EPERM)
> +		ret = 0;
> +	else
> +		ret = -errno;
> +
> +	return ret;
> +}
> +
>  static int list_subvol_search(int fd, struct root_lookup *root_lookup)
>  {
>  	int ret;
> diff --git a/btrfs-list.h b/btrfs-list.h
> index 6e5fc778..6225311d 100644
> --- a/btrfs-list.h
> +++ b/btrfs-list.h
> @@ -176,5 +176,6 @@ 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 check_perm_for_tree_search(int fd);
>  
>  #endif
> 


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

Re: [RFC PATCH v2 3/8] btrfs-progs: sub list: Add helper function which checks the permission for tree search ioctl

[Misono, Tomohiro]

On 2018/03/17 22:23, Goffredo Baroncelli wrote:
> On 03/15/2018 09:13 AM, Misono, Tomohiro wrote:
>> 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 | 33 +++++++++++++++++++++++++++++++++
>>  btrfs-list.h |  1 +
>>  2 files changed, 34 insertions(+)
>>
>> diff --git a/btrfs-list.c b/btrfs-list.c
>> index 50e5ce5f..88689a9d 100644
>> --- a/btrfs-list.c
>> +++ b/btrfs-list.c
>> @@ -958,6 +958,39 @@ out:
>>  	return 0;
>>  }
>>  
>> +/*
>> + * Check the permission for tree search ioctl by searching a key
>> + * which alwasys exists
>> + */
>> +int check_perm_for_tree_search(int fd)
>> +{
> 
> In what this is different from '(getuid() == 0)' ? 
> Which would be the cases where an error different from EPERM would be returned (other than the filesystem is not a BTRFS one )?
> 
> I am not against to this kind of function. However with this implementation is not clear its behavior:
> - does it check if the user has enough privileges to perform a BTRFS_IOC_TREE_SEARCH
> or 
> - does it check if BTRFS_IOC_TREE_SEARCH might return some error ?
> 
> If the former, I would put this function into utils.[hc] checking only the [e]uid of the user. If the latter the name is confusing...

The reason I wrote this function is that the permission for BTRFS_IOC_TREE_SEARCH is
CAP_SYS_ADMIN and might not equal uid == 0.

However, it seems that get_euid() is simple and enough for btrfs-progs, so I will
use it instead of this function in next version.

Thanks,
Tomohiro Misono

> 
>> +	struct btrfs_ioctl_search_args args;
>> +	struct btrfs_ioctl_search_key *sk = &args.key;
>> +	int ret;
>> +
>> +	memset(&args, 0, sizeof(args));
>> +	sk->tree_id = BTRFS_ROOT_TREE_OBJECTID;
>> +	sk->min_objectid = BTRFS_EXTENT_TREE_OBJECTID;
>> +	sk->max_objectid = BTRFS_EXTENT_TREE_OBJECTID;
>> +	sk->min_type = BTRFS_ROOT_ITEM_KEY;
>> +	sk->max_type = BTRFS_ROOT_ITEM_KEY;
>> +	sk->min_offset = 0;
>> +	sk->max_offset = (u64)-1;
>> +	sk->min_transid = 0;
>> +	sk->max_transid = (u64)-1;
>> +	sk->nr_items = 1;
>> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
>> +
>> +	if (!ret)
>> +		ret = 1;
>> +	else if (ret < 0 && errno == EPERM)
>> +		ret = 0;
>> +	else
>> +		ret = -errno;
>> +
>> +	return ret;
>> +}
>> +
>>  static int list_subvol_search(int fd, struct root_lookup *root_lookup)
>>  {
>>  	int ret;
>> diff --git a/btrfs-list.h b/btrfs-list.h
>> index 6e5fc778..6225311d 100644
>> --- a/btrfs-list.h
>> +++ b/btrfs-list.h
>> @@ -176,5 +176,6 @@ 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 check_perm_for_tree_search(int fd);
>>  
>>  #endif
>>
> 
> 

[RFC PATCH v2 4/8] 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 88689a9d..2e82dc5c 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -1522,7 +1522,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;
 
@@ -1543,7 +1544,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;
@@ -1555,7 +1556,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;
@@ -1576,7 +1577,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;
@@ -1588,7 +1590,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;
@@ -1611,7 +1613,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;
@@ -1623,7 +1625,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 6225311d..eb21bebe 100644
--- a/btrfs-list.h
+++ b/btrfs-list.h
@@ -169,13 +169,14 @@ 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);
 int check_perm_for_tree_search(int fd);
 
 #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 v2 5/8] 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 v2 6/8] 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 opened 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     | 326 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 cmds-subvolume.c |  13 +++
 2 files changed, 332 insertions(+), 7 deletions(-)

diff --git a/btrfs-list.c b/btrfs-list.c
index 2e82dc5c..ef97d2db 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
  *
@@ -1303,6 +1351,20 @@ static void filter_and_sort_subvol(struct root_lookup *all_subvols,
 	while (n) {
 		entry = rb_entry(n, struct root_info, rb_node);
 
+		/*
+		 * If list_subvol_user() is used, there may be
+		 * entries which have been skipped for search.
+		 * Just remove these entries here.
+		 */
+		if (!entry->path) {
+			struct rb_node *prev = rb_prev(n);
+
+			rb_erase(n, &all_subvols->root);
+			free_root_info(n);
+			n = prev;
+			continue;
+		}
+
 		ret = resolve_root(all_subvols, entry, top_id);
 		if (ret == -ENOENT) {
 			if (entry->root_id != BTRFS_FS_TREE_OBJECTID) {
@@ -1343,6 +1405,243 @@ 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;
+			fill_subvol_info_top(root_lookup, fd);
+			fill_rootref(root_lookup, fd, e->id);
+		} 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;
+				}
+			}
+
+			fill_subvol_info(root_lookup, fd);
+			fill_rootref(root_lookup, fd, e->id);
+		}
+
+		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 */
+				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);
+	}
+
+	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)
 {
@@ -1527,17 +1826,28 @@ static int btrfs_list_subvols(int fd, struct root_lookup *root_lookup,
 {
 	int ret;
 
-	ret = list_subvol_search(fd, root_lookup);
-	if (ret) {
-		error("can't perform the search: %m");
-		return ret;
+	ret = check_perm_for_tree_search(fd);
+	if (ret < 0) {
+		error("can't check the permission for tree search: %s",
+				strerror(-ret));
+		return -1;
 	}
 
 	/*
 	 * 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);
+	if (ret) {
+		ret = list_subvol_search(fd, root_lookup);
+		if (ret) {
+			error("can't perform the search: %s", strerror(-ret));
+			return ret;
+		}
+		ret = list_subvol_fill_paths(fd, root_lookup);
+	} else {
+		ret = list_subvol_user(fd, root_lookup, path);
+	}
+
 	return ret;
 }
 
@@ -1631,12 +1941,14 @@ 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 ||
+		    ri->root_id == BTRFS_FS_TREE_OBJECTID ||
+		    uuid_is_null(ri->uuid)) {
 			rbn = rb_next(rbn);
 			continue;
 		}
diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index faa10c5a..7a7c6f3b 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -596,6 +596,19 @@ static int cmd_subvol_list(int argc, char **argv)
 		goto out;
 	}
 
+	ret = check_perm_for_tree_search(fd);
+	if (ret < 0) {
+		ret = -1;
+		error("can't check the permission for tree search: %s",
+				strerror(-ret));
+		goto out;
+	}
+	if (!ret && 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 v2 6/8] btrfs-progs: sub list: Allow normal user to call "subvolume list/show"

[Goffredo Baroncelli]

On 03/15/2018 09:15 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).

I found the original "btrfs sub list" behavior quite confusing. I think that the problem is that the output is too technical. And the '-a' switch increase this confusion. May be that I am no smart enough :(

The "normal user behavior" seems to me more coherent. However I am not sure that this differences should be acceptable. In any case it should be tracked in the man page.

Time to add another command (something like "btrfs sub ls") with a more "human friendly" output ?

> The specified path itself is not needed to be a subvolume.
> If the subvolume cannot be opened 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     | 326 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  cmds-subvolume.c |  13 +++
>  2 files changed, 332 insertions(+), 7 deletions(-)
> 

[....]

>  static void print_subvolume_column(struct root_info *subv,
>  				   enum btrfs_list_column_enum column)
>  {
> @@ -1527,17 +1826,28 @@ static int btrfs_list_subvols(int fd, struct root_lookup *root_lookup,
>  {
>  	int ret;
>  
> -	ret = list_subvol_search(fd, root_lookup);
> -	if (ret) {
> -		error("can't perform the search: %m");
> -		return ret;
> +	ret = check_perm_for_tree_search(fd);
> +	if (ret < 0) {
> +		error("can't check the permission for tree search: %s",
> +				strerror(-ret));
> +		return -1;
>  	}
>  
>  	/*
>  	 * 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);
> +	if (ret) {
> +		ret = list_subvol_search(fd, root_lookup);
> +		if (ret) {
> +			error("can't perform the search: %s", strerror(-ret));
> +			return ret;
> +		}
> +		ret = list_subvol_fill_paths(fd, root_lookup);
> +	} else {
> +		ret = list_subvol_user(fd, root_lookup, path);
> +	}
> +
>  	return ret;

I think that the check above should be refined: if I run "btrfs sub list" patched in a kernel without patch I don't get any error and/or warning:

ghigo@venice:~/btrfs/btrfs-progs$ # kernel w/o patch; btrfs-progs w/patch
ghigo@venice:~/btrfs/btrfs-progs$ ./btrfs sub list /
ghigo@venice:~/btrfs/btrfs-progs$ # kernel w/o patch; btrfs-progs w/o patch
ghigo@venice:~/btrfs/btrfs-progs$ btrfs sub list /
ERROR: can't perform the search: Operation not permitted

I think that in both case an error should be raised


>  }
>  
> @@ -1631,12 +1941,14 @@ 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 ||
> +		    ri->root_id == BTRFS_FS_TREE_OBJECTID ||
> +		    uuid_is_null(ri->uuid)) {
>  			rbn = rb_next(rbn);
>  			continue;
>  		}
> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> index faa10c5a..7a7c6f3b 100644
> --- a/cmds-subvolume.c
> +++ b/cmds-subvolume.c
> @@ -596,6 +596,19 @@ static int cmd_subvol_list(int argc, char **argv)
>  		goto out;
>  	}
>  
> +	ret = check_perm_for_tree_search(fd);
> +	if (ret < 0) {
> +		ret = -1;
> +		error("can't check the permission for tree search: %s",
> +				strerror(-ret));
> +		goto out;
> +	}
> +	if (!ret && 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 v2 6/8] btrfs-progs: sub list: Allow normal user to call "subvolume list/show"

[Misono, Tomohiro]

On 2018/03/17 22:23, Goffredo Baroncelli wrote:
> On 03/15/2018 09:15 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).
> 
> I found the original "btrfs sub list" behavior quite confusing. I think that the problem is that the output is too technical. And the '-a' switch increase this confusion. May be that I am no smart enough :(
> 
> The "normal user behavior" seems to me more coherent. However I am not sure that this differences should be acceptable. In any case it should be tracked in the man page.
> 
> Time to add another command (something like "btrfs sub ls") with a more "human friendly" output ?

Yes, I'm also for to fix the output of "sub list" more user friendly (and the behavior of -a or -o).

However, there are other ongoing works to btrfs-progs which changes code base largely
(i.e libbtrfs and json output, and that is the part of reason this series is RFC as the
changes conflict especially for libbtrfs). So, I'd like to see these changes will be merged first. 

> 
>> The specified path itself is not needed to be a subvolume.
>> If the subvolume cannot be opened 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     | 326 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  cmds-subvolume.c |  13 +++
>>  2 files changed, 332 insertions(+), 7 deletions(-)
>>
> 
> [....]
> 
>>  static void print_subvolume_column(struct root_info *subv,
>>  				   enum btrfs_list_column_enum column)
>>  {
>> @@ -1527,17 +1826,28 @@ static int btrfs_list_subvols(int fd, struct root_lookup *root_lookup,
>>  {
>>  	int ret;
>>  
>> -	ret = list_subvol_search(fd, root_lookup);
>> -	if (ret) {
>> -		error("can't perform the search: %m");
>> -		return ret;
>> +	ret = check_perm_for_tree_search(fd);
>> +	if (ret < 0) {
>> +		error("can't check the permission for tree search: %s",
>> +				strerror(-ret));
>> +		return -1;
>>  	}
>>  
>>  	/*
>>  	 * 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);
>> +	if (ret) {
>> +		ret = list_subvol_search(fd, root_lookup);
>> +		if (ret) {
>> +			error("can't perform the search: %s", strerror(-ret));
>> +			return ret;
>> +		}
>> +		ret = list_subvol_fill_paths(fd, root_lookup);
>> +	} else {
>> +		ret = list_subvol_user(fd, root_lookup, path);
>> +	}
>> +
>>  	return ret;
> 
> I think that the check above should be refined: if I run "btrfs sub list" patched in a kernel without patch I don't get any error and/or warning:
> 
> ghigo@venice:~/btrfs/btrfs-progs$ # kernel w/o patch; btrfs-progs w/patch
> ghigo@venice:~/btrfs/btrfs-progs$ ./btrfs sub list /
> ghigo@venice:~/btrfs/btrfs-progs$ # kernel w/o patch; btrfs-progs w/o patch
> ghigo@venice:~/btrfs/btrfs-progs$ btrfs sub list /
> ERROR: can't perform the search: Operation not permitted
> 
> I think that in both case an error should be raised

I will fix this in next version.

Thanks,
Tomohiro Misono

> 
> 
>>  }
>>  
>> @@ -1631,12 +1941,14 @@ 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 ||
>> +		    ri->root_id == BTRFS_FS_TREE_OBJECTID ||
>> +		    uuid_is_null(ri->uuid)) {
>>  			rbn = rb_next(rbn);
>>  			continue;
>>  		}
>> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
>> index faa10c5a..7a7c6f3b 100644
>> --- a/cmds-subvolume.c
>> +++ b/cmds-subvolume.c
>> @@ -596,6 +596,19 @@ static int cmd_subvol_list(int argc, char **argv)
>>  		goto out;
>>  	}
>>  
>> +	ret = check_perm_for_tree_search(fd);
>> +	if (ret < 0) {
>> +		ret = -1;
>> +		error("can't check the permission for tree search: %s",
>> +				strerror(-ret));
>> +		goto out;
>> +	}
>> +	if (!ret && 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);
>>
> 
> 

[RFC PATCH v2 7/8] 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 v2 8/8] 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