| Message ID | 1390829312-814-5-git-send-email-Gerhard@Heift.Name (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Jan 27, 2014 at 02:28:30PM +0100, Gerhard Heift wrote: > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -2032,6 +2032,52 @@ static noinline int btrfs_ioctl_tree_search(struct file *file, > return ret; > } > > +static noinline int btrfs_ioctl_tree_search_v2(struct file *file, > + void __user *argp) > +{ > + struct btrfs_ioctl_search_args_v2 *args; > + struct inode *inode; > + int ret; > + char *buf; > + size_t buf_size; > + > + if (!capable(CAP_SYS_ADMIN)) > + return -EPERM; > + > + /* copy search header and buffer size */ > + args = memdup_user(argp, sizeof(*args)); > + if (IS_ERR(args)) > + return PTR_ERR(args); > + > + buf_size = args->buf_size; > + > + if (buf_size < sizeof(struct btrfs_ioctl_search_header)) { > + kfree(args); > + return -ENOMEM; ENOMEM does not seem correct here, it's not a memory allocation failure but rather an underflow of the buffer size, possibly EINVAL or EOVERFLOW as the other checks return. > + } > + > + /* limit memory */ > + if (buf_size > PAGE_SIZE * 32) > + buf_size = PAGE_SIZE * 32; > + > + buf = memdup_user(argp->buf, buf_size); Memory allocations are not that easy, getting a contiguous 32 * 4k = 128k buffer may often fail. And the point of the V2 ioctl was to avoid allocating the buffer, and do copy_to_user directly. Also, you remove this code in the next patch, I don't think this level of patch granularity is necessary. > + if (IS_ERR(buf)) { > + kfree(args); > + return PTR_ERR(buf); > + } > + > + inode = file_inode(file); > + ret = search_ioctl(inode, &args->key, buf_size, buf); > + if (ret == 0 && ( > + copy_to_user(argp, args, sizeof(*args)) || > + copy_to_user(argp->buf, buf, buf_size) > + )) > + ret = -EFAULT; > + kfree(buf); > + kfree(args); > + return ret; > +} -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-01-27 David Sterba <dsterba@suse.cz>: > On Mon, Jan 27, 2014 at 02:28:30PM +0100, Gerhard Heift wrote: >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -2032,6 +2032,52 @@ static noinline int btrfs_ioctl_tree_search(struct file *file, >> return ret; >> } >> >> +static noinline int btrfs_ioctl_tree_search_v2(struct file *file, >> + void __user *argp) >> +{ >> + struct btrfs_ioctl_search_args_v2 *args; >> + struct inode *inode; >> + int ret; >> + char *buf; >> + size_t buf_size; >> + >> + if (!capable(CAP_SYS_ADMIN)) >> + return -EPERM; >> + >> + /* copy search header and buffer size */ >> + args = memdup_user(argp, sizeof(*args)); >> + if (IS_ERR(args)) >> + return PTR_ERR(args); >> + >> + buf_size = args->buf_size; >> + >> + if (buf_size < sizeof(struct btrfs_ioctl_search_header)) { >> + kfree(args); >> + return -ENOMEM; > > ENOMEM does not seem correct here, it's not a memory allocation failure > but rather an underflow of the buffer size, possibly EINVAL or EOVERFLOW > as the other checks return. Saw this to during the rewrite, fixed it for the new version. >> + } >> + >> + /* limit memory */ >> + if (buf_size > PAGE_SIZE * 32) >> + buf_size = PAGE_SIZE * 32; >> + >> + buf = memdup_user(argp->buf, buf_size); > > Memory allocations are not that easy, getting a contiguous 32 * 4k = 128k > buffer may often fail. And the point of the V2 ioctl was to avoid > allocating the buffer, and do copy_to_user directly. > > Also, you remove this code in the next patch, I don't think this level > of patch granularity is necessary. In the new version I do it the other way around, first direct copy, then the new ioctl, so this is not necessary any more. >> + if (IS_ERR(buf)) { >> + kfree(args); >> + return PTR_ERR(buf); >> + } >> + >> + inode = file_inode(file); >> + ret = search_ioctl(inode, &args->key, buf_size, buf); >> + if (ret == 0 && ( >> + copy_to_user(argp, args, sizeof(*args)) || >> + copy_to_user(argp->buf, buf, buf_size) >> + )) >> + ret = -EFAULT; >> + kfree(buf); >> + kfree(args); >> + return ret; >> +} -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 9fa222d..c44fcdd 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2032,6 +2032,52 @@ static noinline int btrfs_ioctl_tree_search(struct file *file, return ret; } +static noinline int btrfs_ioctl_tree_search_v2(struct file *file, + void __user *argp) +{ + struct btrfs_ioctl_search_args_v2 *args; + struct inode *inode; + int ret; + char *buf; + size_t buf_size; + + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + + /* copy search header and buffer size */ + args = memdup_user(argp, sizeof(*args)); + if (IS_ERR(args)) + return PTR_ERR(args); + + buf_size = args->buf_size; + + if (buf_size < sizeof(struct btrfs_ioctl_search_header)) { + kfree(args); + return -ENOMEM; + } + + /* limit memory */ + if (buf_size > PAGE_SIZE * 32) + buf_size = PAGE_SIZE * 32; + + buf = memdup_user(argp->buf, buf_size); + if (IS_ERR(buf)) { + kfree(args); + return PTR_ERR(buf); + } + + inode = file_inode(file); + ret = search_ioctl(inode, &args->key, buf_size, buf); + if (ret == 0 && ( + copy_to_user(argp, args, sizeof(*args)) || + copy_to_user(argp->buf, buf, buf_size) + )) + ret = -EFAULT; + kfree(buf); + kfree(args); + return ret; +} + /* * Search INODE_REFs to identify path name of 'dirid' directory * in a 'tree_id' tree. and sets path name to 'name'. @@ -4777,6 +4823,8 @@ long btrfs_ioctl(struct file *file, unsigned int return btrfs_ioctl_trans_end(file); case BTRFS_IOC_TREE_SEARCH: return btrfs_ioctl_tree_search(file, argp); + case BTRFS_IOC_TREE_SEARCH_V2: + return btrfs_ioctl_tree_search_v2(file, argp); case BTRFS_IOC_INO_LOOKUP: return btrfs_ioctl_ino_lookup(file, argp); case BTRFS_IOC_INO_PATHS: diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h index 1b8a0f4..6ba0d0f 100644 --- a/include/uapi/linux/btrfs.h +++ b/include/uapi/linux/btrfs.h @@ -301,6 +301,12 @@ struct btrfs_ioctl_search_args { char buf[BTRFS_SEARCH_ARGS_BUFSIZE]; }; +struct btrfs_ioctl_search_args_v2 { + struct btrfs_ioctl_search_key key; + size_t buf_size; + char buf[0]; +}; + struct btrfs_ioctl_clone_range_args { __s64 src_fd; __u64 src_offset, src_length; @@ -553,6 +559,8 @@ static inline char *btrfs_err_str(enum btrfs_err_code err_code) struct btrfs_ioctl_defrag_range_args) #define BTRFS_IOC_TREE_SEARCH _IOWR(BTRFS_IOCTL_MAGIC, 17, \ struct btrfs_ioctl_search_args) +#define BTRFS_IOC_TREE_SEARCH_V2 _IOWR(BTRFS_IOCTL_MAGIC, 17, \ + struct btrfs_ioctl_search_args_v2) #define BTRFS_IOC_INO_LOOKUP _IOWR(BTRFS_IOCTL_MAGIC, 18, \ struct btrfs_ioctl_ino_lookup_args) #define BTRFS_IOC_DEFAULT_SUBVOL _IOW(BTRFS_IOCTL_MAGIC, 19, __u64)
This new ioctl call allows the user to supply a buffer of varying size in which a tree search can store its results. This is much more flexible if you want to receive items which are larger than the current fixed buffer of 3992 bytes or if you want to fetch mor item at once. Currently the buffer is limited to 32 pages. Signed-off-by: Gerhard Heift <Gerhard@Heift.Name> --- fs/btrfs/ioctl.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/btrfs.h | 8 ++++++++ 2 files changed, 56 insertions(+)