subject:"\[PATCH 1\/2\] btrfs\: Add unprivileged subvolume search ioctl"

Re: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

2018-03-08 Thread Misono, Tomohiro


On 2018/03/08 17:29, Nikolay Borisov wrote:
> 
> 
> On  6.03.2018 10:30, Misono, Tomohiro wrote:
>> Add new unprivileged ioctl (BTRFS_IOC_GET_SUBVOL_INFO) which searches
>> and returns only subvolume related item (ROOT_ITEM/ROOT_BACKREF/ROOT_REF)
>> from root tree. The arguments of this ioctl are the same as treesearch
>> ioctl and can be used like treesearch ioctl.
>>
>> Since treesearch ioctl requires root privilege, this ioctl is needed
>> to allow normal users to call "btrfs subvolume list/show" etc.
>>
>> Note that the subvolume name in ROOT_BACKREF/ROOT_REF will not be copied to
>> prevent potential name leak. The name can be obtained by calling
>> user version's ino_lookup ioctl (BTRFS_IOC_INO_LOOKUP_USER).
>>
>> Signed-off-by: Tomohiro Misono 
>> ---
>>  fs/btrfs/ioctl.c   | 254 
>> +
>>  include/uapi/linux/btrfs.h |   2 +
>>  2 files changed, 256 insertions(+)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 111ee282b777..1dba309dce31 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1921,6 +1921,28 @@ static noinline int key_in_sk(struct btrfs_key *key,
>>  return 1;
>>  }
>>  
>> +/*
>> + * check if key is in sk and subvolume related range
>> + */
>> +static noinline int key_in_sk_and_subvol(struct btrfs_key *key,
>> +  struct btrfs_ioctl_search_key *sk)
>> +{
>> +int ret;
>> +
>> +ret = key_in_sk(key, sk);
>> +if (!ret)
>> +return 0;
>> +
>> +if ((key->objectid == BTRFS_FS_TREE_OBJECTID ||
>> +(key->objectid >= BTRFS_FIRST_FREE_OBJECTID &&
>> + key->objectid <= BTRFS_LAST_FREE_OBJECTID)) &&
> 
> Why do you need the FIRST_FREE/LAST_FREE object id checks? The range
> described by those are ordinary files. Since you are only interested in
> subvolume data then you should omit those, no?

There are ROOT_ITEM for other trees (EXTETN_TREE, DEV_TREE etc.) and they 
should be skipped.

> 
>> +key->type >= BTRFS_ROOT_ITEM_KEY &&
>> +key->type <= BTRFS_ROOT_BACKREF_KEY)
> I think this check should really be:
> 
> if (key->objectid == BTRFS_FS_TREE_OBJECTID || key->objectid ==
> BTRFS_ROOT_ITEM_KEY || key->type == BTRFS_ROOT_BACKREF_KEY
>>> +   return 1;
>> +
>> +return 0;
>> +}
>> +
>>  static noinline int copy_to_sk(struct btrfs_path *path,
>> struct btrfs_key *key,
>> struct btrfs_ioctl_search_key *sk,
>> @@ -2045,6 +2067,142 @@ static noinline int copy_to_sk(struct btrfs_path 
>> *path,
>>  return ret;
>>  }
>>  
>> +/*
>> + * Basically the same as copy_to_sk() but restricts the copied item
>> + * within subvolume related one using key_in_sk_and_subvol().
>> + * Also, the name of subvolume will be omitted from
>> + * ROOT_BACKREF/ROOT_REF item.
>> + */
>> +static noinline int copy_subvol_item(struct btrfs_path *path,
>> +   struct btrfs_key *key,
>> +   struct btrfs_ioctl_search_key *sk,
>> +   size_t *buf_size,
>> +   char __user *ubuf,
>> +   unsigned long *sk_offset,
>> +   int *num_found)
>> +{
>> +u64 found_transid;
>> +struct extent_buffer *leaf;
>> +struct btrfs_ioctl_search_header sh;
>> +struct btrfs_key test;
>> +unsigned long item_off;
>> +unsigned long item_len;
>> +int nritems;
>> +int i;
>> +int slot;
>> +int ret = 0;
>> +
>> +leaf = path->nodes[0];
>> +slot = path->slots[0];
>> +nritems = btrfs_header_nritems(leaf);
>> +
>> +if (btrfs_header_generation(leaf) > sk->max_transid) {
>> +i = nritems;
>> +goto advance_key;
> 
> I don't see why you need a goto label at all. Just put the necessary
> code inside the if and return directly.

Most of this code is just copied from copy_to_sk().
I will rewrite code in more appropriate way in future version.

> 
>> +}
>> +found_transid = btrfs_header_generation(leaf);
>> +
>> +for (i = slot; i < nritems; i++) {
>> +item_off = btrfs_item_ptr_offset(leaf, i);
>> +item_len = btrfs_item_size_nr(leaf, i);
>> +
>> +btrfs_item_key_to_cpu(leaf, key, i);
>> +if (!key_in_sk_and_subvol(key, sk))
>> +continue;
>> +
>> +/* will not copy the name of subvolume */
>> +if (key->type == BTRFS_ROOT_BACKREF_KEY ||
>> +key->type == BTRFS_ROOT_REF_KEY)
>> +item_len = sizeof(struct btrfs_root_ref);
>> +
>> +if (sizeof(sh) + item_len > *buf_size) {
>> +if (*num_found) {
>> +ret = 1;
>> +goto out;
>  This can be a simple return 1;
> 
> 
>> +}
>> +
>> +/*
>> + * return one empty item back for

Re: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

2018-03-08 Thread Misono, Tomohiro



On 2018/03/08 4:00, Goffredo Baroncelli wrote:
> On 03/07/2018 01:40 AM, Misono, Tomohiro wrote:
>> On 2018/03/07 5:29, Goffredo Baroncelli wrote:
>>> On 03/06/2018 09:30 AM, Misono, Tomohiro wrote:
 Add new unprivileged ioctl (BTRFS_IOC_GET_SUBVOL_INFO) which searches
 and returns only subvolume related item (ROOT_ITEM/ROOT_BACKREF/ROOT_REF)
 from root tree. The arguments of this ioctl are the same as treesearch
 ioctl and can be used like treesearch ioctl.
>>>
>>> Is it a pro ? The treesearch ioctl is tightly coupled to the btrfs internal 
>>> structure, this means that if we would change the btrfs internal structure, 
>>> we have to update all the clients of this api. For the treesearch it is an 
>>> acceptable compromise between flexibility and speed of developing. But for 
>>> a more specialized API, I think that it would make sense to provide a less 
>>> coupled api to the internal structure.
>>
>> Thanks for the comments.
>>
>> The reason I choose the same api is just to minimize the code change in 
>> btrfs-progs.
>> For tree search part, it works just switching the ioctl number from 
>> BTRFS_IOC_TREE_SEARCH
>> to BTRFS_IOC_GET_SUBVOL_INFO in list_subvol_search()[1].
>>
>> [1] https://marc.info/?l=linux-btrfs=152032537911218=2
> 
> I suggest to avoid this approach. An API is forever; we already have a 
> "root-only" one which is quite unfriendly and error prone; I think that we 
> should put all the energies to make a better one. 
> 
> I think that the major weaknesses of this api are:
> - it exports the the data in "le" format  (see struct btrfs_root_item as 
> example); 
> - it requires the user to increase the key for the next ioctl call. This 
> could be doable in the kernel space before returning
> - this ioctl exports both the ROOT_BACKREF and ROOT_ITEM info. Why not make 
> two separate (simplers) ioctl(s) ?

I agree with you and will split this ioctl into two pats (one is for getting 
ROOT_ITEM and the other for ROOT_REF/BACKREF)
and make them to have user friendly apis.

> 
>>>
>>> Below some comments
> [...]
> 
 +  if ((key->objectid == BTRFS_FS_TREE_OBJECTID ||
 +  (key->objectid >= BTRFS_FIRST_FREE_OBJECTID &&
 +   key->objectid <= BTRFS_LAST_FREE_OBJECTID)) &&
 +  key->type >= BTRFS_ROOT_ITEM_KEY &&
 +  key->type <= BTRFS_ROOT_BACKREF_KEY)
>>>
>>> Why returning all the range [BTRFS_ROOT_ITEM_KEY...BTRFS_ROOT_BACKREF_KEY]. 
>>> It would be sufficient to return only
>>>
>>>   + (key->type == BTRFS_ROOT_ITEM_KEY ||
>>>   +  key->type == BTRFS_ROOT_BACKREF_KEY))
>>
>> Sorry, it is a mistake, I mean "key->type <= BTRFS_ROOTREF_KEY".
>> Although btrfs-progs only uses BTRFS_ROOT_BACKREF_KEY, I notice libbtrfs 
>> uses BTRFS_ROOT_REF_KEY instead. So, I think it is better to return both
>> ROOT_BACKREF_KEY and ROOT_REF_KEY. I will fix this in v2.
>>
> 
> I was referring to the '>=' and '<=' instead of '=='. If another type is 
> added in the middle, it would be returned. I find it a bit error prone.

ok, I will change this. thanks.
Tomohiro Misono

> 
> BR
> G.Baroncelli
> 

--
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: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

2018-03-08 Thread Nikolay Borisov



On  6.03.2018 10:30, Misono, Tomohiro wrote:
> Add new unprivileged ioctl (BTRFS_IOC_GET_SUBVOL_INFO) which searches
> and returns only subvolume related item (ROOT_ITEM/ROOT_BACKREF/ROOT_REF)
> from root tree. The arguments of this ioctl are the same as treesearch
> ioctl and can be used like treesearch ioctl.
> 
> Since treesearch ioctl requires root privilege, this ioctl is needed
> to allow normal users to call "btrfs subvolume list/show" etc.
> 
> Note that the subvolume name in ROOT_BACKREF/ROOT_REF will not be copied to
> prevent potential name leak. The name can be obtained by calling
> user version's ino_lookup ioctl (BTRFS_IOC_INO_LOOKUP_USER).
> 
> Signed-off-by: Tomohiro Misono 
> ---
>  fs/btrfs/ioctl.c   | 254 
> +
>  include/uapi/linux/btrfs.h |   2 +
>  2 files changed, 256 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 111ee282b777..1dba309dce31 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1921,6 +1921,28 @@ static noinline int key_in_sk(struct btrfs_key *key,
>   return 1;
>  }
>  
> +/*
> + * check if key is in sk and subvolume related range
> + */
> +static noinline int key_in_sk_and_subvol(struct btrfs_key *key,
> +   struct btrfs_ioctl_search_key *sk)
> +{
> + int ret;
> +
> + ret = key_in_sk(key, sk);
> + if (!ret)
> + return 0;
> +
> + if ((key->objectid == BTRFS_FS_TREE_OBJECTID ||
> + (key->objectid >= BTRFS_FIRST_FREE_OBJECTID &&
> +  key->objectid <= BTRFS_LAST_FREE_OBJECTID)) &&

Why do you need the FIRST_FREE/LAST_FREE object id checks? The range
described by those are ordinary files. Since you are only interested in
subvolume data then you should omit those, no?

> + key->type >= BTRFS_ROOT_ITEM_KEY &&
> + key->type <= BTRFS_ROOT_BACKREF_KEY)
I think this check should really be:

if (key->objectid == BTRFS_FS_TREE_OBJECTID || key->objectid ==
BTRFS_ROOT_ITEM_KEY || key->type == BTRFS_ROOT_BACKREF_KEY

> + return 1;
> +
> + return 0;
> +}
> +
>  static noinline int copy_to_sk(struct btrfs_path *path,
>  struct btrfs_key *key,
>  struct btrfs_ioctl_search_key *sk,
> @@ -2045,6 +2067,142 @@ static noinline int copy_to_sk(struct btrfs_path 
> *path,
>   return ret;
>  }
>  
> +/*
> + * Basically the same as copy_to_sk() but restricts the copied item
> + * within subvolume related one using key_in_sk_and_subvol().
> + * Also, the name of subvolume will be omitted from
> + * ROOT_BACKREF/ROOT_REF item.
> + */
> +static noinline int copy_subvol_item(struct btrfs_path *path,
> +struct btrfs_key *key,
> +struct btrfs_ioctl_search_key *sk,
> +size_t *buf_size,
> +char __user *ubuf,
> +unsigned long *sk_offset,
> +int *num_found)
> +{
> + u64 found_transid;
> + struct extent_buffer *leaf;
> + struct btrfs_ioctl_search_header sh;
> + struct btrfs_key test;
> + unsigned long item_off;
> + unsigned long item_len;
> + int nritems;
> + int i;
> + int slot;
> + int ret = 0;
> +
> + leaf = path->nodes[0];
> + slot = path->slots[0];
> + nritems = btrfs_header_nritems(leaf);
> +
> + if (btrfs_header_generation(leaf) > sk->max_transid) {
> + i = nritems;
> + goto advance_key;

I don't see why you need a goto label at all. Just put the necessary
code inside the if and return directly.

> + }
> + found_transid = btrfs_header_generation(leaf);
> +
> + for (i = slot; i < nritems; i++) {
> + item_off = btrfs_item_ptr_offset(leaf, i);
> + item_len = btrfs_item_size_nr(leaf, i);
> +
> + btrfs_item_key_to_cpu(leaf, key, i);
> + if (!key_in_sk_and_subvol(key, sk))
> + continue;
> +
> + /* will not copy the name of subvolume */
> + if (key->type == BTRFS_ROOT_BACKREF_KEY ||
> + key->type == BTRFS_ROOT_REF_KEY)
> + item_len = sizeof(struct btrfs_root_ref);
> +
> + if (sizeof(sh) + item_len > *buf_size) {
> + if (*num_found) {
> + ret = 1;
> + goto out;
 This can be a simple return 1;


> + }
> +
> + /*
> +  * return one empty item back for v1, which does not
> +  * handle -EOVERFLOW
> +  */
> +
> + *buf_size = sizeof(sh) + item_len;
> + item_len = 0;
> + ret = -EOVERFLOW;
> + }
> +
> + if (sizeof(sh) + item_len + *sk_offset > *buf_size) {
> +

Re: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

2018-03-07 Thread Goffredo Baroncelli

On 03/07/2018 01:40 AM, Misono, Tomohiro wrote:
> On 2018/03/07 5:29, Goffredo Baroncelli wrote:
>> On 03/06/2018 09:30 AM, Misono, Tomohiro wrote:
>>> Add new unprivileged ioctl (BTRFS_IOC_GET_SUBVOL_INFO) which searches
>>> and returns only subvolume related item (ROOT_ITEM/ROOT_BACKREF/ROOT_REF)
>>> from root tree. The arguments of this ioctl are the same as treesearch
>>> ioctl and can be used like treesearch ioctl.
>>
>> Is it a pro ? The treesearch ioctl is tightly coupled to the btrfs internal 
>> structure, this means that if we would change the btrfs internal structure, 
>> we have to update all the clients of this api. For the treesearch it is an 
>> acceptable compromise between flexibility and speed of developing. But for a 
>> more specialized API, I think that it would make sense to provide a less 
>> coupled api to the internal structure.
> 
> Thanks for the comments.
> 
> The reason I choose the same api is just to minimize the code change in 
> btrfs-progs.
> For tree search part, it works just switching the ioctl number from 
> BTRFS_IOC_TREE_SEARCH
> to BTRFS_IOC_GET_SUBVOL_INFO in list_subvol_search()[1].
> 
> [1] https://marc.info/?l=linux-btrfs=152032537911218=2

I suggest to avoid this approach. An API is forever; we already have a 
"root-only" one which is quite unfriendly and error prone; I think that we 
should put all the energies to make a better one. 

I think that the major weaknesses of this api are:
- it exports the the data in "le" format  (see struct btrfs_root_item as 
example); 
- it requires the user to increase the key for the next ioctl call. This could 
be doable in the kernel space before returning
- this ioctl exports both the ROOT_BACKREF and ROOT_ITEM info. Why not make two 
separate (simplers) ioctl(s) ?

>>
>> Below some comments
[...]

>>> +   if ((key->objectid == BTRFS_FS_TREE_OBJECTID ||
>>> +   (key->objectid >= BTRFS_FIRST_FREE_OBJECTID &&
>>> +key->objectid <= BTRFS_LAST_FREE_OBJECTID)) &&
>>> +   key->type >= BTRFS_ROOT_ITEM_KEY &&
>>> +   key->type <= BTRFS_ROOT_BACKREF_KEY)
>>
>> Why returning all the range [BTRFS_ROOT_ITEM_KEY...BTRFS_ROOT_BACKREF_KEY]. 
>> It would be sufficient to return only
>>
>>   +  (key->type == BTRFS_ROOT_ITEM_KEY ||
>>   +   key->type == BTRFS_ROOT_BACKREF_KEY))
> 
> Sorry, it is a mistake, I mean "key->type <= BTRFS_ROOTREF_KEY".
> Although btrfs-progs only uses BTRFS_ROOT_BACKREF_KEY, I notice libbtrfs 
> uses BTRFS_ROOT_REF_KEY instead. So, I think it is better to return both
> ROOT_BACKREF_KEY and ROOT_REF_KEY. I will fix this in v2.
> 

I was referring to the '>=' and '<=' instead of '=='. If another type is added 
in the middle, it would be returned. I find it a bit error prone.

BR
G.Baroncelli
-- 
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

Re: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

2018-03-07 Thread David Sterba

On Wed, Mar 07, 2018 at 09:40:18AM +0900, Misono, Tomohiro wrote:
> On 2018/03/07 5:29, Goffredo Baroncelli wrote:
> > On 03/06/2018 09:30 AM, Misono, Tomohiro wrote:
> >> Add new unprivileged ioctl (BTRFS_IOC_GET_SUBVOL_INFO) which searches
> >> and returns only subvolume related item (ROOT_ITEM/ROOT_BACKREF/ROOT_REF)
> >> from root tree. The arguments of this ioctl are the same as treesearch
> >> ioctl and can be used like treesearch ioctl.
> > 
> > Is it a pro ? The treesearch ioctl is tightly coupled to the btrfs internal 
> > structure, this means that if we would change the btrfs internal structure, 
> > we have to update all the clients of this api. For the treesearch it is an 
> > acceptable compromise between flexibility and speed of developing. But for 
> > a more specialized API, I think that it would make sense to provide a less 
> > coupled api to the internal structure.
> 
> Thanks for the comments.
> 
> The reason I choose the same api is just to minimize the code change in 
> btrfs-progs.

That's not IMO a good reason. We can cahnge the code in btrfs-progs and
that's not going to be the only user of the ioctl so the interfact (ie.
the structures) should be adapted for the needs of the ioctl.
--
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: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

2018-03-06 Thread Misono, Tomohiro

On 2018/03/07 5:29, Goffredo Baroncelli wrote:
> On 03/06/2018 09:30 AM, Misono, Tomohiro wrote:
>> Add new unprivileged ioctl (BTRFS_IOC_GET_SUBVOL_INFO) which searches
>> and returns only subvolume related item (ROOT_ITEM/ROOT_BACKREF/ROOT_REF)
>> from root tree. The arguments of this ioctl are the same as treesearch
>> ioctl and can be used like treesearch ioctl.
> 
> Is it a pro ? The treesearch ioctl is tightly coupled to the btrfs internal 
> structure, this means that if we would change the btrfs internal structure, 
> we have to update all the clients of this api. For the treesearch it is an 
> acceptable compromise between flexibility and speed of developing. But for a 
> more specialized API, I think that it would make sense to provide a less 
> coupled api to the internal structure.

Thanks for the comments.

The reason I choose the same api is just to minimize the code change in 
btrfs-progs.
For tree search part, it works just switching the ioctl number from 
BTRFS_IOC_TREE_SEARCH
to BTRFS_IOC_GET_SUBVOL_INFO in list_subvol_search()[1].

[1] https://marc.info/?l=linux-btrfs=152032537911218=2

> 
> Below some comments
> 
> 
>>
>> Since treesearch ioctl requires root privilege, this ioctl is needed
>> to allow normal users to call "btrfs subvolume list/show" etc.
>>
>> Note that the subvolume name in ROOT_BACKREF/ROOT_REF will not be copied to
>> prevent potential name leak. The name can be obtained by calling
>> user version's ino_lookup ioctl (BTRFS_IOC_INO_LOOKUP_USER).
>>
>> Signed-off-by: Tomohiro Misono 
>> ---
>>  fs/btrfs/ioctl.c   | 254 
>> +
>>  include/uapi/linux/btrfs.h |   2 +
>>  2 files changed, 256 insertions(+)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 111ee282b777..1dba309dce31 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1921,6 +1921,28 @@ static noinline int key_in_sk(struct btrfs_key *key,
>>  return 1;
>>  }
>>  
>> +/*
>> + * check if key is in sk and subvolume related range
>> + */
>> +static noinline int key_in_sk_and_subvol(struct btrfs_key *key,
>> +  struct btrfs_ioctl_search_key *sk)
>> +{
>> +int ret;
>> +
>> +ret = key_in_sk(key, sk);
>> +if (!ret)
>> +return 0;
>> +
>> +if ((key->objectid == BTRFS_FS_TREE_OBJECTID ||
>> +(key->objectid >= BTRFS_FIRST_FREE_OBJECTID &&
>> + key->objectid <= BTRFS_LAST_FREE_OBJECTID)) &&
>> +key->type >= BTRFS_ROOT_ITEM_KEY &&
>> +key->type <= BTRFS_ROOT_BACKREF_KEY)
> 
> Why returning all the range [BTRFS_ROOT_ITEM_KEY...BTRFS_ROOT_BACKREF_KEY]. 
> It would be sufficient to return only
> 
>   +   (key->type == BTRFS_ROOT_ITEM_KEY ||
>   +key->type == BTRFS_ROOT_BACKREF_KEY))

Sorry, it is a mistake, I mean "key->type <= BTRFS_ROOTREF_KEY".
Although btrfs-progs only uses BTRFS_ROOT_BACKREF_KEY, I notice libbtrfs 
uses BTRFS_ROOT_REF_KEY instead. So, I think it is better to return both
ROOT_BACKREF_KEY and ROOT_REF_KEY. I will fix this in v2.

> 
> 
>> +return 1;
>> +
>> +return 0;
>> +}
>> +
>>  static noinline int copy_to_sk(struct btrfs_path *path,
>> struct btrfs_key *key,
>> struct btrfs_ioctl_search_key *sk,
>> @@ -2045,6 +2067,142 @@ static noinline int copy_to_sk(struct btrfs_path 
>> *path,
>>  return ret;
>>  }
>>  
>> +/*
>> + * Basically the same as copy_to_sk() but restricts the copied item
>> + * within subvolume related one using key_in_sk_and_subvol().
>> + * Also, the name of subvolume will be omitted from
>> + * ROOT_BACKREF/ROOT_REF item.
>> + */
>> +static noinline int copy_subvol_item(struct btrfs_path *path,
>> +   struct btrfs_key *key,
>> +   struct btrfs_ioctl_search_key *sk,
>> +   size_t *buf_size,
>> +   char __user *ubuf,
>> +   unsigned long *sk_offset,
>> +   int *num_found)
>> +{
>> +u64 found_transid;
>> +struct extent_buffer *leaf;
>> +struct btrfs_ioctl_search_header sh;
>> +struct btrfs_key test;
>> +unsigned long item_off;
>> +unsigned long item_len;
>> +int nritems;
>> +int i;
>> +int slot;
>> +int ret = 0;
>> +
>> +leaf = path->nodes[0];
>> +slot = path->slots[0];
>> +nritems = btrfs_header_nritems(leaf);
>> +
>> +if (btrfs_header_generation(leaf) > sk->max_transid) {
>> +i = nritems;
>> +goto advance_key;
>> +}
>> +found_transid = btrfs_header_generation(leaf);
>> +
>> +for (i = slot; i < nritems; i++) {
>> +item_off = btrfs_item_ptr_offset(leaf, i);
>> +item_len = btrfs_item_size_nr(leaf, i);
>> +
>> +btrfs_item_key_to_cpu(leaf, key, i);
>> +if (!key_in_sk_and_subvol(key, sk))
>> +continue;

Re: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

2018-03-06 Thread Goffredo Baroncelli

On 03/06/2018 09:30 AM, Misono, Tomohiro wrote:
> Add new unprivileged ioctl (BTRFS_IOC_GET_SUBVOL_INFO) which searches
> and returns only subvolume related item (ROOT_ITEM/ROOT_BACKREF/ROOT_REF)
> from root tree. The arguments of this ioctl are the same as treesearch
> ioctl and can be used like treesearch ioctl.

Is it a pro ? The treesearch ioctl is tightly coupled to the btrfs internal 
structure, this means that if we would change the btrfs internal structure, we 
have to update all the clients of this api. For the treesearch it is an 
acceptable compromise between flexibility and speed of developing. But for a 
more specialized API, I think that it would make sense to provide a less 
coupled api to the internal structure.

Below some comments


> 
> Since treesearch ioctl requires root privilege, this ioctl is needed
> to allow normal users to call "btrfs subvolume list/show" etc.
> 
> Note that the subvolume name in ROOT_BACKREF/ROOT_REF will not be copied to
> prevent potential name leak. The name can be obtained by calling
> user version's ino_lookup ioctl (BTRFS_IOC_INO_LOOKUP_USER).
> 
> Signed-off-by: Tomohiro Misono 
> ---
>  fs/btrfs/ioctl.c   | 254 
> +
>  include/uapi/linux/btrfs.h |   2 +
>  2 files changed, 256 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 111ee282b777..1dba309dce31 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1921,6 +1921,28 @@ static noinline int key_in_sk(struct btrfs_key *key,
>   return 1;
>  }
>  
> +/*
> + * check if key is in sk and subvolume related range
> + */
> +static noinline int key_in_sk_and_subvol(struct btrfs_key *key,
> +   struct btrfs_ioctl_search_key *sk)
> +{
> + int ret;
> +
> + ret = key_in_sk(key, sk);
> + if (!ret)
> + return 0;
> +
> + if ((key->objectid == BTRFS_FS_TREE_OBJECTID ||
> + (key->objectid >= BTRFS_FIRST_FREE_OBJECTID &&
> +  key->objectid <= BTRFS_LAST_FREE_OBJECTID)) &&
> + key->type >= BTRFS_ROOT_ITEM_KEY &&
> + key->type <= BTRFS_ROOT_BACKREF_KEY)

Why returning all the range [BTRFS_ROOT_ITEM_KEY...BTRFS_ROOT_BACKREF_KEY]. It 
would be sufficient to return only

  + (key->type == BTRFS_ROOT_ITEM_KEY ||
  +  key->type == BTRFS_ROOT_BACKREF_KEY))


> + return 1;
> +
> + return 0;
> +}
> +
>  static noinline int copy_to_sk(struct btrfs_path *path,
>  struct btrfs_key *key,
>  struct btrfs_ioctl_search_key *sk,
> @@ -2045,6 +2067,142 @@ static noinline int copy_to_sk(struct btrfs_path 
> *path,
>   return ret;
>  }
>  
> +/*
> + * Basically the same as copy_to_sk() but restricts the copied item
> + * within subvolume related one using key_in_sk_and_subvol().
> + * Also, the name of subvolume will be omitted from
> + * ROOT_BACKREF/ROOT_REF item.
> + */
> +static noinline int copy_subvol_item(struct btrfs_path *path,
> +struct btrfs_key *key,
> +struct btrfs_ioctl_search_key *sk,
> +size_t *buf_size,
> +char __user *ubuf,
> +unsigned long *sk_offset,
> +int *num_found)
> +{
> + u64 found_transid;
> + struct extent_buffer *leaf;
> + struct btrfs_ioctl_search_header sh;
> + struct btrfs_key test;
> + unsigned long item_off;
> + unsigned long item_len;
> + int nritems;
> + int i;
> + int slot;
> + int ret = 0;
> +
> + leaf = path->nodes[0];
> + slot = path->slots[0];
> + nritems = btrfs_header_nritems(leaf);
> +
> + if (btrfs_header_generation(leaf) > sk->max_transid) {
> + i = nritems;
> + goto advance_key;
> + }
> + found_transid = btrfs_header_generation(leaf);
> +
> + for (i = slot; i < nritems; i++) {
> + item_off = btrfs_item_ptr_offset(leaf, i);
> + item_len = btrfs_item_size_nr(leaf, i);
> +
> + btrfs_item_key_to_cpu(leaf, key, i);
> + if (!key_in_sk_and_subvol(key, sk))
> + continue;
> +
> + /* will not copy the name of subvolume */
> + if (key->type == BTRFS_ROOT_BACKREF_KEY ||
> + key->type == BTRFS_ROOT_REF_KEY)
> + item_len = sizeof(struct btrfs_root_ref);
> +
> + if (sizeof(sh) + item_len > *buf_size) {
> + if (*num_found) {
> + ret = 1;
> + goto out;
> + }
> +
> + /*
> +  * return one empty item back for v1, which does not
> +  * handle -EOVERFLOW
> +  */
It is still applicable ?
> +
> + *buf_size = sizeof(sh) + item_len;

[PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

2018-03-06 Thread Misono, Tomohiro

Add new unprivileged ioctl (BTRFS_IOC_GET_SUBVOL_INFO) which searches
and returns only subvolume related item (ROOT_ITEM/ROOT_BACKREF/ROOT_REF)
from root tree. The arguments of this ioctl are the same as treesearch
ioctl and can be used like treesearch ioctl.

Since treesearch ioctl requires root privilege, this ioctl is needed
to allow normal users to call "btrfs subvolume list/show" etc.

Note that the subvolume name in ROOT_BACKREF/ROOT_REF will not be copied to
prevent potential name leak. The name can be obtained by calling
user version's ino_lookup ioctl (BTRFS_IOC_INO_LOOKUP_USER).

Signed-off-by: Tomohiro Misono 
---
 fs/btrfs/ioctl.c   | 254 +
 include/uapi/linux/btrfs.h |   2 +
 2 files changed, 256 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 111ee282b777..1dba309dce31 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1921,6 +1921,28 @@ static noinline int key_in_sk(struct btrfs_key *key,
return 1;
 }
 
+/*
+ * check if key is in sk and subvolume related range
+ */
+static noinline int key_in_sk_and_subvol(struct btrfs_key *key,
+ struct btrfs_ioctl_search_key *sk)
+{
+   int ret;
+
+   ret = key_in_sk(key, sk);
+   if (!ret)
+   return 0;
+
+   if ((key->objectid == BTRFS_FS_TREE_OBJECTID ||
+   (key->objectid >= BTRFS_FIRST_FREE_OBJECTID &&
+key->objectid <= BTRFS_LAST_FREE_OBJECTID)) &&
+   key->type >= BTRFS_ROOT_ITEM_KEY &&
+   key->type <= BTRFS_ROOT_BACKREF_KEY)
+   return 1;
+
+   return 0;
+}
+
 static noinline int copy_to_sk(struct btrfs_path *path,
   struct btrfs_key *key,
   struct btrfs_ioctl_search_key *sk,
@@ -2045,6 +2067,142 @@ static noinline int copy_to_sk(struct btrfs_path *path,
return ret;
 }
 
+/*
+ * Basically the same as copy_to_sk() but restricts the copied item
+ * within subvolume related one using key_in_sk_and_subvol().
+ * Also, the name of subvolume will be omitted from
+ * ROOT_BACKREF/ROOT_REF item.
+ */
+static noinline int copy_subvol_item(struct btrfs_path *path,
+  struct btrfs_key *key,
+  struct btrfs_ioctl_search_key *sk,
+  size_t *buf_size,
+  char __user *ubuf,
+  unsigned long *sk_offset,
+  int *num_found)
+{
+   u64 found_transid;
+   struct extent_buffer *leaf;
+   struct btrfs_ioctl_search_header sh;
+   struct btrfs_key test;
+   unsigned long item_off;
+   unsigned long item_len;
+   int nritems;
+   int i;
+   int slot;
+   int ret = 0;
+
+   leaf = path->nodes[0];
+   slot = path->slots[0];
+   nritems = btrfs_header_nritems(leaf);
+
+   if (btrfs_header_generation(leaf) > sk->max_transid) {
+   i = nritems;
+   goto advance_key;
+   }
+   found_transid = btrfs_header_generation(leaf);
+
+   for (i = slot; i < nritems; i++) {
+   item_off = btrfs_item_ptr_offset(leaf, i);
+   item_len = btrfs_item_size_nr(leaf, i);
+
+   btrfs_item_key_to_cpu(leaf, key, i);
+   if (!key_in_sk_and_subvol(key, sk))
+   continue;
+
+   /* will not copy the name of subvolume */
+   if (key->type == BTRFS_ROOT_BACKREF_KEY ||
+   key->type == BTRFS_ROOT_REF_KEY)
+   item_len = sizeof(struct btrfs_root_ref);
+
+   if (sizeof(sh) + item_len > *buf_size) {
+   if (*num_found) {
+   ret = 1;
+   goto out;
+   }
+
+   /*
+* return one empty item back for v1, which does not
+* handle -EOVERFLOW
+*/
+
+   *buf_size = sizeof(sh) + item_len;
+   item_len = 0;
+   ret = -EOVERFLOW;
+   }
+
+   if (sizeof(sh) + item_len + *sk_offset > *buf_size) {
+   ret = 1;
+   goto out;
+   }
+
+   sh.objectid = key->objectid;
+   sh.offset = key->offset;
+   sh.type = key->type;
+   sh.len = item_len;
+   sh.transid = found_transid;
+
+   /* copy search result header */
+   if (copy_to_user(ubuf + *sk_offset, , sizeof(sh))) {
+   ret = -EFAULT;
+   goto out;
+   }
+
+   *sk_offset += sizeof(sh);
+
+   if (item_len) {
+   char __user *up = ubuf + *sk_offset;
+
+   /* copy the item */

Re: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

Re: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

Re: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

Re: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

Re: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

Re: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

Re: [PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

[PATCH 1/2] btrfs: Add unprivileged subvolume search ioctl

8 matches

Site Navigation

Mail list logo

Footer information