On 2018/03/08 18:47, Nikolay Borisov wrote:
>
>
> On 6.03.2018 10:31, Misono, Tomohiro wrote:
>> Add unprivileged version of ino_lookup ioctl (BTRFS_IOC_INO_LOOKUP_USER)
>> to allow normal users to call "btrfs subvololume list/show" etc. in
>> combination with BTRFS_IOC_GET_SUBVOL_INFO.
>>
>> This can be used like BTRFS_IOC_INO_LOOKUP but the argument is
>> different because it also returns the name of bottom subvolume,
>> which is ommited by BTRFS_IOC_GET_SUBVOL_INFO ioctl.
>> Also, this must be called with fd which exists under the tree of
>> args->treeid to prevent for user to search any tree.
>>
>> The main differences from original ino_lookup ioctl are:
>> 1. Read permission will be checked using inode_permission()
>> during path construction. -EACCES will be returned in case
>> of failure.
>> 2. Path construction will be stopped at the inode number which
>> corresponds to the fd with which this ioctl is called. If
>> constructed path does not exist under fd's inode, -EACCES
>> will be returned.
>> 3. The name of bottom subvolume is also searched and filled.
>>
>> Note that the maximum length of path is shorter 272 bytes than
>
> Did you mean 255? Since BTRFS_VOL_NAME_MAX is defined as 255?
My mistake, actually 255 + 1 (for null) + 8 (for subvolume id) = 264 bytes in this version.
>
>> ino_lookup ioctl because of space of subvolume's id and name.
>>
>> Signed-off-by: Tomohiro Misono <misono.tomohiro@xxxxxxxxxxxxxx>
>> ---
>> fs/btrfs/ioctl.c | 218 +++++++++++++++++++++++++++++++++++++++++++++
>> include/uapi/linux/btrfs.h | 16 ++++
>> 2 files changed, 234 insertions(+)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 1dba309dce31..ac23da98b7e7 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -2425,6 +2425,179 @@ static noinline int btrfs_search_path_in_tree(struct btrfs_fs_info *info,
>> return ret;
>> }
>>
>> +static noinline int btrfs_search_path_in_tree_user(struct btrfs_fs_info *info,
>> + struct super_block *sb,
>> + struct btrfs_key upper_limit,
>> + struct btrfs_ioctl_ino_lookup_user_args *args)
>> +{
>> + struct btrfs_root *root;
>> + struct btrfs_key key, key2;
>> + char *ptr;
>> + int ret = -1;
>> + int slot;
>> + int len;
>> + int total_len = 0;
>> + u64 dirid = args->dirid;
>> + struct btrfs_inode_ref *iref;
>> + struct btrfs_root_ref rref;
>> +
>> + unsigned long item_off;
>> + unsigned long item_len;
>> +
>> + struct extent_buffer *l;
>> + struct btrfs_path *path = NULL;
>> +
>> + struct inode *inode;
>> + int *new = 0;
>> +
>> + path = btrfs_alloc_path();
>> + if (!path)
>> + return -ENOMEM;
>> +
>> + if (dirid == upper_limit.objectid)
>> + /*
>> + * If the bottom subvolume exists directly under upper limit,
>> + * there is no need to construct the path and just get the
>> + * subvolume's name
>> + */
>> + goto get_subvol_name;
>
> Eliminate the label and factor out the code in a new function or just
> put in the body of the if.
ok.
>
>> + if (dirid == BTRFS_FIRST_FREE_OBJECTID)
>> + /* The subvolume does not exist under upper_limit */
>> + goto access_err;
>
> just set ret to -EACCESS and goto out. And eliminate access_err label
> altogether. Furthermore, shouldn't this check really ether:
>
> a) Be the first one in this function so that we return ASAP (i.e. even
> before calling btrfs_alloc_path)
>
> or
>
> b) Put in the ioctl function itself. Currently for the
I'd like to adopt b) approach.
> btrfs_ioctl_ino_lookup case it's duplicated in both that function and
> btrfs_search_path_in_tree
> >> +
>> + ptr = &args->path[BTRFS_INO_LOOKUP_PATH_MAX - 1];
>> +
>> + key.objectid = args->treeid;
>> + key.type = BTRFS_ROOT_ITEM_KEY;
>> + key.offset = (u64)-1;
>> + root = btrfs_read_fs_root_no_name(info, &key);
>> + if (IS_ERR(root)) {
>> + btrfs_err(info, "could not find root %llu", args->treeid);
>> + ret = -ENOENT;
>> + goto out;
>> + }
>> +
>> + key.objectid = dirid;
>> + key.type = BTRFS_INODE_REF_KEY;
>> + key.offset = (u64)-1;
>> +
>> + while (1) {
>> + ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
>> + if (ret < 0)
>> + goto out;
>> + else if (ret > 0) {
>> + ret = btrfs_previous_item(root, path, dirid,
>> + BTRFS_INODE_REF_KEY);
>> + if (ret < 0)
>> + goto out;
>> + else if (ret > 0) {
>> + ret = -ENOENT;
>> + goto out;
>> + }
>> + }
>> +
>> + l = path->nodes[0];
>> + slot = path->slots[0];
>> + btrfs_item_key_to_cpu(l, &key, slot);
>> +
>> + iref = btrfs_item_ptr(l, slot, struct btrfs_inode_ref);
>> + len = btrfs_inode_ref_name_len(l, iref);
>> + ptr -= len + 1;
>> + total_len += len + 1;
>> + if (ptr < args->path) {
>> + ret = -ENAMETOOLONG;
>> + goto out;
>> + }
>> +
>> + *(ptr + len) = '/';
>> + read_extent_buffer(l, ptr, (unsigned long)(iref + 1), len);
>> +
>> + /* Check the read permission */
>> + ret = btrfs_previous_item(root, path, dirid,
>> + BTRFS_INODE_ITEM_KEY);
>> + if (ret < 0) {
>> + goto out;
>> + } else if (ret > 0) {
>> + ret = -ENOENT;
>> + goto out;
>> + }
>> + l = path->nodes[0];
>> + slot = path->slots[0];
>> +
>> + btrfs_item_key_to_cpu(l, &key2, slot);
>> + inode = btrfs_iget(sb, &key2, root, new);
>> + ret = inode_permission(inode, MAY_READ);
>> + iput(inode);
>> + if (ret)
>> + goto access_err;
>> +
>> + if (key.offset == BTRFS_FIRST_FREE_OBJECTID ||
>> + key.offset == upper_limit.objectid)
>> + break;
>> +
>> + btrfs_release_path(path);
>> + key.objectid = key.offset;
>> + key.offset = (u64)-1;
>> + dirid = key.objectid;
>> + }
>> + /* Check if the searched path exists under the upper_limit */
>> + if (key.offset == BTRFS_FIRST_FREE_OBJECTID &&
>> + upper_limit.objectid != BTRFS_FIRST_FREE_OBJECTID)
>> + goto access_err;
>> +
>> + memmove(args->path, ptr, total_len);
>> + args->path[total_len] = '\0';
>> + btrfs_release_path(path);
>> + ret = 0;
>> +
>> +get_subvol_name:
>> + /* Get subolume's name from ROOT_REF */
>> + key.objectid = BTRFS_ROOT_TREE_OBJECTID;
>> + key.type = BTRFS_ROOT_ITEM_KEY;
>> + key.offset = (u64)-1;
>> + root = btrfs_read_fs_root_no_name(info, &key);
>> + if (IS_ERR(root)) {
>> + ret = -ENOENT;
>> + goto out;
>> + }
>> +
>> + key.objectid = args->treeid;
>> + key.type = BTRFS_ROOT_REF_KEY;
>> + key.offset = args->subid;
>> + ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
>> + if (ret != 0) {
>> + ret = -ENOENT;
>> + goto out;
>> + }
>> + l = path->nodes[0];
>> + slot = path->slots[0];
>> + btrfs_item_key_to_cpu(l, &key, slot);
>> +
>> + item_off = btrfs_item_ptr_offset(l, slot);
>> + item_len = btrfs_item_size_nr(l, slot);
>> + /* Check if dirid in ROOT_REF corresponds to passed arg */
>> + read_extent_buffer(l, &rref, item_off, sizeof(struct btrfs_root_ref));
>> + if (rref.dirid != args->dirid) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + /* Copy subvolume's name */
>> + item_off += sizeof(struct btrfs_root_ref);
>> + item_len -= sizeof(struct btrfs_root_ref);
>> + read_extent_buffer(l, args->name, item_off, item_len);
>> + args->name[item_len] = '\0';
>> +
>> +out:
>> + btrfs_free_path(path);
>> + return ret;
>> +
>> +access_err:
>> + btrfs_free_path(path);
>> + ret = -EACCES;
>> + return ret;
>> +}
>> +
>> static noinline int btrfs_ioctl_ino_lookup(struct file *file,
>> void __user *argp)
>> {
>> @@ -2467,6 +2640,49 @@ static noinline int btrfs_ioctl_ino_lookup(struct file *file,
>> return ret;
>> }
>>
>> +/*
>> + * User version of ino_lookup ioctl (unprivileged)
>> + *
>> + * The main differences from original ino_lookup ioctl are:
>> + * 1. Read permission will be checked using inode_permission()
>> + * during path construction. -EACCES will be returned in case
>> + * of failure.
>> + * 2. Path construction will be stopped at the inode number which
>> + * corresponds to the fd with which this ioctl is called. If
>> + * constructed path does not exist under fd's inode, -EACCES
>> + * will be returned.
>> + * 3. The name of bottom subvolume is also searched and filled.
>> + */
>> +static noinline int btrfs_ioctl_ino_lookup_user(struct file *file,
>> + void __user *argp)
>> +{
>> + struct btrfs_ioctl_ino_lookup_user_args *args;
>> + struct inode *inode;
>> + int ret = 0;
>> +
>> + args = memdup_user(argp, sizeof(*args));
>> + if (IS_ERR(args))
>> + return PTR_ERR(args);
>> +
>> + inode = file_inode(file);
>> + /*
>> + * args->treeid must be matched with this inode's objectid
>> + * to previent for user to search any fs/file tree.
>> + */
>> + if (args->treeid != BTRFS_I(inode)->root->root_key.objectid)
>> + return -EINVAL;
>
> Do we want something like :
> if (args->treeid == 0)
> args->treeid = BTRFS_I(inode)->root->root_key.objectid;
>
> or does the ioctl impose a hard requirement for treeid to be set?
My first plan is yes, but this seems unreasonable.
so I will remove the variable 'treeid' from struct ino_lookup_args_user.
>> +
>> + ret = btrfs_search_path_in_tree_user(BTRFS_I(inode)->root->fs_info,
>> + inode->i_sb, BTRFS_I(inode)->location,
>> + args);
>
> Just pass in the the inode and reference all the parameters inside the
> function. btrfs_search_path_in_tree_user really requires only 2 args:
> inode and 'args'
>
ok.
>> +
>> + if (ret == 0 && copy_to_user(argp, args, sizeof(*args)))
>> + ret = -EFAULT;
>> +
>> + kfree(args);
>> + return ret;
>> +}
>> +
>> static noinline int search_subvol(struct inode *inode,
>> struct btrfs_ioctl_search_key *sk,
>> size_t *buf_size,
>> @@ -5917,6 +6133,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>> return btrfs_ioctl_set_features(file, argp);
>> case BTRFS_IOC_GET_SUBVOL_INFO:
>> return btrfs_ioctl_get_subvol_info(file, argp);
>> + case BTRFS_IOC_INO_LOOKUP_USER:
>> + return btrfs_ioctl_ino_lookup_user(file, argp);
>> }
>>
>> return -ENOTTY;
>> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
>> index 1e9361cf66d5..22326d631417 100644
>> --- a/include/uapi/linux/btrfs.h
>> +++ b/include/uapi/linux/btrfs.h
>> @@ -422,6 +422,20 @@ struct btrfs_ioctl_ino_lookup_args {
>> char name[BTRFS_INO_LOOKUP_PATH_MAX];
>> };
>>
>> +#define BTRFS_INO_LOOKUP_USER_PATH_MAX (4072-BTRFS_VOL_NAME_MAX)
> Where does 4072 come from? Is it related to
> #define BTRFS_INO_LOOKUP_PATH_MAX 4080
>
> in any way ?
>> +struct btrfs_ioctl_ino_lookup_user_args {
>> + /* in */
>> + __u64 treeid;
>> + /* in, 'dirid' is the same as objectid in btrfs_ioctl_ino_lookup_args */
>
> Given there is no description of the objectid parameter of
> ino_lookup_args. Your reference to it doesn't really make its purpose
> any more clear.
The variable names in struct ino_lookup_args are rather confusing and I will add a comment too.
Thanks,
Tomohiro Misono
>
>> + __u64 dirid;
>> + /* in, subvolume id which exists under the directory of 'dirid' */
>> + __u64 subid;
>> + /* out, name of the subvolume of 'subid' */
>> + char name[BTRFS_VOL_NAME_MAX];
>> + /* out, 'path' is the same as 'name' in btrfs_ioctl_ino_lookup_args */
>> + char path[BTRFS_INO_LOOKUP_USER_PATH_MAX];
>> +};
>> +
>> /* Search criteria for the btrfs SEARCH ioctl family. */
>> struct btrfs_ioctl_search_key {
>> /*
>> @@ -845,5 +859,7 @@ enum btrfs_err_code {
>> struct btrfs_ioctl_logical_ino_args)
>> #define BTRFS_IOC_GET_SUBVOL_INFO _IOWR(BTRFS_IOCTL_MAGIC, 60, \
>> struct btrfs_ioctl_search_args)
>> +#define BTRFS_IOC_INO_LOOKUP_USER _IOWR(BTRFS_IOCTL_MAGIC, 61, \
>> + struct btrfs_ioctl_ino_lookup_args)
>>
>> #endif /* _UAPI_LINUX_BTRFS_H */
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
