DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 026AA20869
Message-ID: <fc9619bee3e27d6756ecff9ae143c646d2567b62.camel@kernel.org>
Subject: Re: [PATCH v1 01/11] fs: Don't copy beyond the end of the file
From:   Jeff Layton <jlayton@kernel.org>
To:     Olga Kornievskaia <olga.kornievskaia@gmail.com>,
        trondmy@hammerspace.com, anna.schumaker@netapp.com
Cc:     linux-nfs@vger.kernel.org
Date:   Sun, 21 Oct 2018 10:29:10 -0400
In-Reply-To: <20181019152932.32462-2-olga.kornievskaia@gmail.com>
References: <20181019152932.32462-1-olga.kornievskaia@gmail.com>
         <20181019152932.32462-2-olga.kornievskaia@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Fri, 2018-10-19 at 11:29 -0400, Olga Kornievskaia wrote:
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> 
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
>  fs/read_write.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 39b4a21..c60790f 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1570,6 +1570,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>  	if (unlikely(ret))
>  		return ret;
>  
> +	if (pos_in >= i_size_read(inode_in))
> +		return -EINVAL;
> +
>  	if (!(file_in->f_mode & FMODE_READ) ||
>  	    !(file_out->f_mode & FMODE_WRITE) ||
>  	    (file_out->f_flags & O_APPEND))

The patch description could use a bit more fleshing-out. The
copy_file_range manpage says:

       EINVAL Requested range extends beyond the end of the source file; or the flags argument is not 0.

So I guess this is intended to satisfy that requirement? If so,
i_size_read is just going to return whatever is in inode->isize. 

Could a copy_file_range call end up getting issued to copy from a file
that is already open on a range that it doesn't know about yet? i.e.
where the inode cache has not yet been updated.

It seems like that could on network filesystems (like NFS). Would this
be better handled in ->copy_file_range instead, where the driver could
make a better determination of the file size?
-- 
Jeff Layton <jlayton@kernel.org>