Re: [PATCH RFCv2 4/6] btrfs: new ioctl TREE_SEARCH_V2

[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.

> +	}
> +
> +	/* 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;
> +}