linux-kernel - Re: [PATCH] new flag COPY_FILE_RANGE_FILESIZE for copy_file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 13 Apr 2019 14:48:41 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     Shawn Landden <shawn@....icu>
Cc:     linux-api@...r.kernel, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-api@...r.kernel.org
Subject: Re: [PATCH] new flag COPY_FILE_RANGE_FILESIZE for copy_file_range()



> On Apr 13, 2019, at 1:49 PM, Shawn Landden <shawn@....icu> wrote:
> 
> If flags includes COPY_FILE_RANGE_FILESIZE then the length
> copied is the length of the file. off_in and off_out are
> ignored. len must be 0 or the file size.
> 
> This implementation saves a call to stat() in the common case
> of copying files. It does not fix any race conditions, but that
> is possible in the future with this interface.
> 
> EAGAIN: If COPY_FILE_RANGE_FILESIZE was passed and len is not 0
> or the file size.

I think you’re asking for trouble here. I assume you have some kind of race prevention in mind here.  The trouble is that passing zero means copy the whole thing, but the size-checking behavior is only available for nonzero sizes. This means that anyone who passes their idea of the size needs to account for inconsistent behavior if the size is zero.

Also, what happens if the file size changes mid copy?  I assume the result is more or less arbitrary, but you should document what behavior is allowed.  The docs should cover the case where you race against an O_APPEND write.

> 
> Signed-off-by: Shawn Landden <shawn@....icu>
> CC: <linux-api@...r.kernel.org>
> ---
> fs/read_write.c           | 14 +++++++++++++-
> include/uapi/linux/stat.h |  4 ++++
> 2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 61b43ad7608e..6d06361f0856 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1557,7 +1557,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>    struct inode *inode_out = file_inode(file_out);
>    ssize_t ret;
> 
> -    if (flags != 0)
> +    if ((flags & ~COPY_FILE_RANGE_FILESIZE) != 0)
>        return -EINVAL;
> 
>    if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
> @@ -1565,6 +1565,18 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>    if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
>        return -EINVAL;
> 
> +    if (flags & COPY_FILE_RANGE_FILESIZE) {
> +        struct kstat stat;
> +        int error;
> +        error = vfs_getattr(&file_in->f_path, &stat,
> +                    STATX_SIZE, 0);
> +        if (error < 0)
> +            return error;
> +        if (!(len == 0 || len == stat.size))
> +            return -EAGAIN;
> +        len = stat.size;
> +    }
> +
>    ret = rw_verify_area(READ, file_in, &pos_in, len);
>    if (unlikely(ret))
>        return ret;
> diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> index 7b35e98d3c58..1075aa4666ef 100644
> --- a/include/uapi/linux/stat.h
> +++ b/include/uapi/linux/stat.h
> @@ -170,5 +170,9 @@ struct statx {
> 
> #define STATX_ATTR_AUTOMOUNT        0x00001000 /* Dir: Automount trigger */
> 
> +/*
> + * Flags for copy_file_range()
> + */
> +#define COPY_FILE_RANGE_FILESIZE    0x00000001 /* Copy the full length of the input file */
> 
> #endif /* _UAPI_LINUX_STAT_H */
> -- 
> 2.20.1
>