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

[Gerhard Heift <gerhard@heift.name>]

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