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

2018-03-19 Thread 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 
>> ---
>>  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 = 
>> +int ret;
>> +
>> +memset(, 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, );
>> +
>> +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
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2018-03-17 Thread 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 
> ---
>  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 = 
> + int ret;
> +
> + memset(, 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, );
> +
> + 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 
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2018-03-15 Thread Misono, Tomohiro 
This is a preparetion work to allow normal user to call
"subvolume list/show".

Signed-off-by: Tomohiro Misono 
---
 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 = 
+   int ret;
+
+   memset(, 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, );
+
+   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

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html