Re: [PATCH v1 01/11] fs: Don't copy beyond the end of the file

From: Olga Kornievskaia <olga.kornievskaia@gmail.com>
To: jlayton@kernel.org
Cc: Trond Myklebust <trondmy@hammerspace.com>,
	Anna Schumaker <anna.schumaker@netapp.com>,
	linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v1 01/11] fs: Don't copy beyond the end of the file
Date: Mon, 22 Oct 2018 14:32:58 -0400	[thread overview]
Message-ID: <CAN-5tyEbGmujqD5Q1FBH9uBWR0YYbW_ojBhdbcD_7TYV9dqMag@mail.gmail.com> (raw)
In-Reply-To: <fc9619bee3e27d6756ecff9ae143c646d2567b62.camel@kernel.org>

On Sun, Oct 21, 2018 at 10:29 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> 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?

I agree the description of the patch is poor. It sort of falls under
the the man page's description of range beyond the end of the source
file. But in NFSv4.2, there is an explicit wording for the validity of
the input parameters and having an input source offset that's beyond
the end of the file is what this patch is attempting to check.

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

I thought that with NFSv4 cache consistency, the inode->isize is
accurate. If previous open had a read delegation, any modification on
a server would trigger a CB_RECALL and the open for the copy offload
would retrieve the latest size. In case of no delegations, the open
retrieves the latest size and the call to copy_file_range() would have
an update size.

> 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?

I'm not opposed to moving the size check into the NFS's copy_file_size
(again in my opinion NFS attribute cache has the same file size as the
inode's size). I think the thought was that such check should be done
at the VFS layer as oppose to doing it by each of the file systems.

> --
> Jeff Layton <jlayton@kernel.org>
>