From: Luis Henriques <lhenriques@suse.com> To: Olga Kornievskaia <olga.kornievskaia@gmail.com> Cc: willy@infradead.org, david@fromorbit.com, "Darrick J. Wong" <darrick.wong@oracle.com>, linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-nfs <linux-nfs@vger.kernel.org>, linux-unionfs@vger.kernel.org, ceph-devel@vger.kernel.org, linux-cifs@vger.kernel.org Subject: Re: [PATCH 04/11] vfs: add missing checks to copy_file_range Date: Thu, 13 Dec 2018 10:29:52 +0000 [thread overview] Message-ID: <87bm5pra73.fsf@suse.com> (raw) In-Reply-To: <CAN-5tyGmdrbbiej_mMosYKrCzOEuKMKJALfOJxvbBqHwZFiUOw@mail.gmail.com> (Olga Kornievskaia's message of "Wed, 12 Dec 2018 15:22:23 -0500") Olga Kornievskaia <olga.kornievskaia@gmail.com> writes: > On Wed, Dec 12, 2018 at 2:43 PM Matthew Wilcox <willy@infradead.org> wrote: >> >> On Wed, Dec 12, 2018 at 01:55:28PM -0500, Olga Kornievskaia wrote: >> > On Wed, Dec 12, 2018 at 6:31 AM Luis Henriques <lhenriques@suse.com> wrote: >> > > I was wondering if, with the above check, it would make sense to also >> > > have an extra patch changing some filesystems (ceph, nfs and cifs) to >> > > simply return -EOPNOTSUPP (instead of -EINVAL) when inode_in == >> > > inode_out. Something like the diff below (not tested!). >> >> > > +++ b/fs/nfs/nfs4file.c >> > > @@ -136,7 +136,7 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in, >> > > ssize_t ret; >> > > >> > > if (file_inode(file_in) == file_inode(file_out)) >> > > - return -EINVAL; >> > > + return -EOPNOTSUPP; >> > >> > Please don't change the NFS bits. This is against the NFS >> > specifications. RFC 7862 15.2.3 >> > >> > (snippet) >> > SAVED_FH and CURRENT_FH must be different files. If SAVED_FH and >> > CURRENT_FH refer to the same file, the operation MUST fail with >> > NFS4ERR_INVAL. >> >> I don't see how that applies. That refers to a requirement _in the >> protocol_ that determines what the server MUST do if the client sends >> it two FHs which refer to the same file. >> >> What we're talking about here is how a Linux filesystem behaves when >> receiving a copy_file_range() referring to the same file. As long as >> the Linux filesystem doesn't react by sending out one of these invalid >> protocol messages, I don't see the problem. > > Ok then this should be changed to call generic_copy_file_range() not > returning the EOPNOTSUPP since there is no longer fallback in vfs to > call the generic_copy_file_range() and in turn responsibility of each > file system. Ah, I didn't look close enough and didn't realised the nfs code was doing something slightly different from the other 2 FSs. In that case simply deleting that check seems to be enough to fallback to the vfs generic_copy_file_range. Anyway, please find below an updated patch (with proper changelog). Cheers, -- Luis >From f66a07e22dc93827bdafc1666d4980edc986bce4 Mon Sep 17 00:00:00 2001 From: Luis Henriques <lhenriques@suse.com> Date: Thu, 13 Dec 2018 10:19:54 +0000 Subject: [PATCH] vfs: fallback to generic_copy_file_range if copying within the same file If source and destination inode are the same simply fallback to the VFS generic_copy_file_range, as we've already checked overlapping areas in generic_copy_file_checks. Signed-off-by: Luis Henriques <lhenriques@suse.com> --- fs/ceph/file.c | 2 +- fs/cifs/cifsfs.c | 2 +- fs/nfs/nfs4file.c | 3 --- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/fs/ceph/file.c b/fs/ceph/file.c index eb876e19c1dc..ff48dc52c30e 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -1904,7 +1904,7 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, bool do_final_copy = false; if (src_inode == dst_inode) - return -EINVAL; + return -EOPNOTSUPP; if (src_inode->i_sb != dst_inode->i_sb) return -EXDEV; if (ceph_snap(dst_inode) != CEPH_NOSNAP) diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index 03e4b9eacbd1..3c66454c59b6 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -1068,7 +1068,7 @@ ssize_t cifs_file_copychunk_range(unsigned int xid, cifs_dbg(FYI, "copychunk range\n"); if (src_inode == target_inode) { - rc = -EINVAL; + rc = -EOPNOTSUPP; goto out; } diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c index 4783c0c1c49e..dc7f344849e9 100644 --- a/fs/nfs/nfs4file.c +++ b/fs/nfs/nfs4file.c @@ -135,9 +135,6 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in, { ssize_t ret = -EXDEV; - if (file_inode(file_in) == file_inode(file_out)) - return -EINVAL; - /* only offload copy if superblock is the same */ if (file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) { do {
WARNING: multiple messages have this Message-ID (diff)
From: Luis Henriques <lhenriques@suse.com> To: Olga Kornievskaia <olga.kornievskaia@gmail.com> Cc: willy@infradead.org, david@fromorbit.com, "Darrick J. Wong" <darrick.wong@oracle.com>, linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-nfs <linux-nfs@vger.kernel.org>, linux-unionfs@vger.kernel.org, ceph-devel@vger.kernel.org, linux-cifs@vger.kernel.org Subject: Re: [PATCH 04/11] vfs: add missing checks to copy_file_range Date: Thu, 13 Dec 2018 10:29:52 +0000 [thread overview] Message-ID: <87bm5pra73.fsf@suse.com> (raw) In-Reply-To: <CAN-5tyGmdrbbiej_mMosYKrCzOEuKMKJALfOJxvbBqHwZFiUOw@mail.gmail.com> (Olga Kornievskaia's message of "Wed, 12 Dec 2018 15:22:23 -0500") Olga Kornievskaia <olga.kornievskaia@gmail.com> writes: > On Wed, Dec 12, 2018 at 2:43 PM Matthew Wilcox <willy@infradead.org> wrote: >> >> On Wed, Dec 12, 2018 at 01:55:28PM -0500, Olga Kornievskaia wrote: >> > On Wed, Dec 12, 2018 at 6:31 AM Luis Henriques <lhenriques@suse.com> wrote: >> > > I was wondering if, with the above check, it would make sense to also >> > > have an extra patch changing some filesystems (ceph, nfs and cifs) to >> > > simply return -EOPNOTSUPP (instead of -EINVAL) when inode_in == >> > > inode_out. Something like the diff below (not tested!). >> >> > > +++ b/fs/nfs/nfs4file.c >> > > @@ -136,7 +136,7 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in, >> > > ssize_t ret; >> > > >> > > if (file_inode(file_in) == file_inode(file_out)) >> > > - return -EINVAL; >> > > + return -EOPNOTSUPP; >> > >> > Please don't change the NFS bits. This is against the NFS >> > specifications. RFC 7862 15.2.3 >> > >> > (snippet) >> > SAVED_FH and CURRENT_FH must be different files. If SAVED_FH and >> > CURRENT_FH refer to the same file, the operation MUST fail with >> > NFS4ERR_INVAL. >> >> I don't see how that applies. That refers to a requirement _in the >> protocol_ that determines what the server MUST do if the client sends >> it two FHs which refer to the same file. >> >> What we're talking about here is how a Linux filesystem behaves when >> receiving a copy_file_range() referring to the same file. As long as >> the Linux filesystem doesn't react by sending out one of these invalid >> protocol messages, I don't see the problem. > > Ok then this should be changed to call generic_copy_file_range() not > returning the EOPNOTSUPP since there is no longer fallback in vfs to > call the generic_copy_file_range() and in turn responsibility of each > file system. Ah, I didn't look close enough and didn't realised the nfs code was doing something slightly different from the other 2 FSs. In that case simply deleting that check seems to be enough to fallback to the vfs generic_copy_file_range. Anyway, please find below an updated patch (with proper changelog). Cheers, -- Luis From f66a07e22dc93827bdafc1666d4980edc986bce4 Mon Sep 17 00:00:00 2001 From: Luis Henriques <lhenriques@suse.com> Date: Thu, 13 Dec 2018 10:19:54 +0000 Subject: [PATCH] vfs: fallback to generic_copy_file_range if copying within the same file If source and destination inode are the same simply fallback to the VFS generic_copy_file_range, as we've already checked overlapping areas in generic_copy_file_checks. Signed-off-by: Luis Henriques <lhenriques@suse.com> --- fs/ceph/file.c | 2 +- fs/cifs/cifsfs.c | 2 +- fs/nfs/nfs4file.c | 3 --- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/fs/ceph/file.c b/fs/ceph/file.c index eb876e19c1dc..ff48dc52c30e 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -1904,7 +1904,7 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, bool do_final_copy = false; if (src_inode == dst_inode) - return -EINVAL; + return -EOPNOTSUPP; if (src_inode->i_sb != dst_inode->i_sb) return -EXDEV; if (ceph_snap(dst_inode) != CEPH_NOSNAP) diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index 03e4b9eacbd1..3c66454c59b6 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -1068,7 +1068,7 @@ ssize_t cifs_file_copychunk_range(unsigned int xid, cifs_dbg(FYI, "copychunk range\n"); if (src_inode == target_inode) { - rc = -EINVAL; + rc = -EOPNOTSUPP; goto out; } diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c index 4783c0c1c49e..dc7f344849e9 100644 --- a/fs/nfs/nfs4file.c +++ b/fs/nfs/nfs4file.c @@ -135,9 +135,6 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in, { ssize_t ret = -EXDEV; - if (file_inode(file_in) == file_inode(file_out)) - return -EINVAL; - /* only offload copy if superblock is the same */ if (file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) { do {
next prev parent reply other threads:[~2018-12-13 10:29 UTC|newest] Thread overview: 85+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-03 8:34 [PATCH 0/11] fs: fixes for major copy_file_range() issues Dave Chinner 2018-12-03 8:34 ` [PATCH 01/11] vfs: copy_file_range source range over EOF should fail Dave Chinner 2018-12-03 12:46 ` Amir Goldstein 2018-12-04 15:13 ` Christoph Hellwig 2018-12-04 21:29 ` Dave Chinner 2018-12-04 21:47 ` Olga Kornievskaia 2018-12-04 22:31 ` Dave Chinner 2018-12-05 16:51 ` J. Bruce Fields 2019-05-20 9:10 ` Amir Goldstein 2019-05-20 13:12 ` Olga Kornievskaia 2019-05-20 13:36 ` Amir Goldstein 2019-05-20 13:58 ` Olga Kornievskaia 2019-05-20 14:02 ` Amir Goldstein 2018-12-05 14:12 ` Christoph Hellwig 2018-12-05 21:08 ` Dave Chinner 2018-12-05 21:30 ` Christoph Hellwig 2018-12-03 8:34 ` [PATCH 02/11] vfs: introduce generic_copy_file_range() Dave Chinner 2018-12-03 10:03 ` Amir Goldstein 2018-12-03 23:00 ` Dave Chinner 2018-12-04 15:14 ` Christoph Hellwig 2018-12-03 8:34 ` [PATCH 03/11] vfs: no fallback for ->copy_file_range Dave Chinner 2018-12-03 10:22 ` Amir Goldstein 2018-12-03 23:02 ` Dave Chinner 2018-12-06 4:16 ` Amir Goldstein 2018-12-06 21:30 ` Dave Chinner 2018-12-07 5:38 ` Amir Goldstein 2018-12-03 18:23 ` Anna Schumaker 2018-12-04 15:16 ` Christoph Hellwig 2018-12-03 8:34 ` [PATCH 04/11] vfs: add missing checks to copy_file_range Dave Chinner 2018-12-03 12:42 ` Amir Goldstein 2018-12-03 19:04 ` Darrick J. Wong 2018-12-03 21:33 ` Olga Kornievskaia 2018-12-03 23:04 ` Dave Chinner 2018-12-04 15:18 ` Christoph Hellwig 2018-12-12 11:31 ` Luis Henriques 2018-12-12 16:42 ` Darrick J. Wong 2018-12-12 18:55 ` Olga Kornievskaia 2018-12-12 19:42 ` Matthew Wilcox 2018-12-12 20:22 ` Olga Kornievskaia 2018-12-13 10:29 ` Luis Henriques [this message] 2018-12-13 10:29 ` Luis Henriques 2018-12-03 8:34 ` [PATCH 05/11] vfs: use inode_permission in copy_file_range() Dave Chinner 2018-12-03 12:47 ` Amir Goldstein 2018-12-03 18:18 ` Darrick J. Wong 2018-12-03 23:55 ` Dave Chinner 2018-12-05 17:28 ` J. Bruce Fields 2018-12-03 18:53 ` Eric Biggers 2018-12-04 15:19 ` Christoph Hellwig 2018-12-03 8:34 ` [PATCH 06/11] vfs: copy_file_range needs to strip setuid bits Dave Chinner 2018-12-03 12:51 ` Amir Goldstein 2018-12-04 15:21 ` Christoph Hellwig 2018-12-03 8:34 ` [PATCH 07/11] vfs: copy_file_range should update file timestamps Dave Chinner 2018-12-03 10:47 ` Amir Goldstein 2018-12-03 17:33 ` Olga Kornievskaia 2018-12-03 18:22 ` Darrick J. Wong 2018-12-03 23:19 ` Dave Chinner 2018-12-04 15:24 ` Christoph Hellwig 2018-12-03 8:34 ` [PATCH 08/11] vfs: push EXDEV check down into ->remap_file_range Dave Chinner 2018-12-03 11:04 ` Amir Goldstein 2018-12-03 19:11 ` Darrick J. Wong 2018-12-03 23:37 ` Dave Chinner 2018-12-03 23:58 ` Darrick J. Wong 2018-12-04 9:17 ` Amir Goldstein 2018-12-04 9:17 ` Amir Goldstein 2018-12-03 23:34 ` Dave Chinner 2018-12-03 18:24 ` Darrick J. Wong 2018-12-04 8:18 ` Olga Kornievskaia 2018-12-03 8:34 ` [PATCH 09/11] vfs: push copy_file_ranges -EXDEV checks down Dave Chinner 2018-12-03 12:36 ` Amir Goldstein 2018-12-03 17:58 ` Olga Kornievskaia 2018-12-03 18:53 ` Anna Schumaker 2018-12-03 19:27 ` Olga Kornievskaia 2018-12-03 23:40 ` Dave Chinner 2018-12-04 15:43 ` Christoph Hellwig 2018-12-04 22:18 ` Dave Chinner 2018-12-04 23:33 ` Olga Kornievskaia 2018-12-05 14:09 ` Christoph Hellwig 2018-12-05 17:01 ` Olga Kornievskaia 2018-12-03 8:34 ` [PATCH 10/11] vfs: allow generic_copy_file_range to copy across devices Dave Chinner 2018-12-03 12:54 ` Amir Goldstein 2018-12-03 8:34 ` [PATCH 11/11] ovl: allow cross-device copy_file_range calls Dave Chinner 2018-12-03 12:55 ` Amir Goldstein 2018-12-03 8:39 ` [PATCH 12/11] man-pages: copy_file_range updates Dave Chinner 2018-12-03 13:05 ` Amir Goldstein 2019-05-21 5:52 ` Amir Goldstein
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87bm5pra73.fsf@suse.com \ --to=lhenriques@suse.com \ --cc=ceph-devel@vger.kernel.org \ --cc=darrick.wong@oracle.com \ --cc=david@fromorbit.com \ --cc=linux-cifs@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-nfs@vger.kernel.org \ --cc=linux-unionfs@vger.kernel.org \ --cc=linux-xfs@vger.kernel.org \ --cc=olga.kornievskaia@gmail.com \ --cc=willy@infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.