[SMB3] fix copy_file_range when copying beyond end of source file

Commit Message

Steve French June 20, 2019, 1:44 a.m. UTC

Patch attached fixes the case where copy_file_range over an SMB3 mount
tries to go beyond the end of file of the source file.  This fixes
xfstests generic/430 and generic/431

Amir's patches had added a similar change in the VFS layer, but
presumably harmless to have the check in cifs.ko as well to ensure
that we don't try to copy beyond end of the source file (otherwise
SMB3 servers will return an error on copychunk rather than doing the
partial copy (up to end of the source file) that copy_file_range
expects).

Comments

ronnie sahlberg June 20, 2019, 2:14 a.m. UTC | #1

On Thu, Jun 20, 2019 at 11:45 AM Steve French via samba-technical
<[e-mail]> wrote:
>
> Patch attached fixes the case where copy_file_range over an SMB3 mount
> tries to go beyond the end of file of the source file.  This fixes
> xfstests generic/430 and generic/431
>
> Amir's patches had added a similar change in the VFS layer, but
> presumably harmless to have the check in cifs.ko as well to ensure
> that we don't try to copy beyond end of the source file (otherwise
> SMB3 servers will return an error on copychunk rather than doing the
> partial copy (up to end of the source file) that copy_file_range
> expects).

+ if (src_off >= inode->i_size) {
+ cifs_dbg(FYI, "nothing to do on copychunk\n");
+ goto cchunk_out; /* nothing to do */
+ } else if (src_off + len > inode->i_size) {
+ /* consider adding check to see if src oplocked */
+ len = inode->i_size - src_off;
+ cifs_dbg(FYI, "adjust copychunk len %lld less\n", len);
+ }

This makes us return rc == 0 when this triggers. Is that what we want?
Looking at "man copy_file_range" suggests we should return -EINVAL in
both these cases.



>
>
>
> --
> Thanks,
>
> Steve

Steve French June 20, 2019, 3:19 a.m. UTC | #2

Local file systems that I tried, seem to agree with xfstest and return
0 if you try to copy beyond end of src file but do create a zero
length target file

On Wed, Jun 19, 2019 at 9:14 PM ronnie sahlberg
<[e-mail]> wrote:
>
> On Thu, Jun 20, 2019 at 11:45 AM Steve French via samba-technical
> <[e-mail]> wrote:
> >
> > Patch attached fixes the case where copy_file_range over an SMB3 mount
> > tries to go beyond the end of file of the source file.  This fixes
> > xfstests generic/430 and generic/431
> >
> > Amir's patches had added a similar change in the VFS layer, but
> > presumably harmless to have the check in cifs.ko as well to ensure
> > that we don't try to copy beyond end of the source file (otherwise
> > SMB3 servers will return an error on copychunk rather than doing the
> > partial copy (up to end of the source file) that copy_file_range
> > expects).
>
> + if (src_off >= inode->i_size) {
> + cifs_dbg(FYI, "nothing to do on copychunk\n");
> + goto cchunk_out; /* nothing to do */
> + } else if (src_off + len > inode->i_size) {
> + /* consider adding check to see if src oplocked */
> + len = inode->i_size - src_off;
> + cifs_dbg(FYI, "adjust copychunk len %lld less\n", len);
> + }
>
> This makes us return rc == 0 when this triggers. Is that what we want?
> Looking at "man copy_file_range" suggests we should return -EINVAL in
> both these cases.
>
>
>
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve

ronnie sahlberg June 20, 2019, 3:28 a.m. UTC | #3

Guess we need a fix to the man page.

Reviewed-by: Ronnie Sahlberg <[e-mail]>

On Thu, Jun 20, 2019 at 1:19 PM Steve French <[e-mail]> wrote:
>
> Local file systems that I tried, seem to agree with xfstest and return
> 0 if you try to copy beyond end of src file but do create a zero
> length target file
>
> On Wed, Jun 19, 2019 at 9:14 PM ronnie sahlberg
> <[e-mail]> wrote:
> >
> > On Thu, Jun 20, 2019 at 11:45 AM Steve French via samba-technical
> > <[e-mail]> wrote:
> > >
> > > Patch attached fixes the case where copy_file_range over an SMB3 mount
> > > tries to go beyond the end of file of the source file.  This fixes
> > > xfstests generic/430 and generic/431
> > >
> > > Amir's patches had added a similar change in the VFS layer, but
> > > presumably harmless to have the check in cifs.ko as well to ensure
> > > that we don't try to copy beyond end of the source file (otherwise
> > > SMB3 servers will return an error on copychunk rather than doing the
> > > partial copy (up to end of the source file) that copy_file_range
> > > expects).
> >
> > + if (src_off >= inode->i_size) {
> > + cifs_dbg(FYI, "nothing to do on copychunk\n");
> > + goto cchunk_out; /* nothing to do */
> > + } else if (src_off + len > inode->i_size) {
> > + /* consider adding check to see if src oplocked */
> > + len = inode->i_size - src_off;
> > + cifs_dbg(FYI, "adjust copychunk len %lld less\n", len);
> > + }
> >
> > This makes us return rc == 0 when this triggers. Is that what we want?
> > Looking at "man copy_file_range" suggests we should return -EINVAL in
> > both these cases.
> >
> >
> >
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
>
>
>
> --
> Thanks,
>
> Steve

Dave Chinner June 20, 2019, 4:52 a.m. UTC | #4

On Wed, Jun 19, 2019 at 10:19:30PM -0500, Steve French wrote:
> Local file systems that I tried, seem to agree with xfstest and return
> 0 if you try to copy beyond end of src file but do create a zero
> length target file

The zero length target file that remains after cfr returns zero
because src > EOF was not created by cfr.  i.e. the destination file
has to be created by the application before cfr is called. Hence
what you see here is the result of running cfr to an existing zero
length file and copying zero bytes to it....

Cheers,

Dave.

Amir Goldstein June 20, 2019, 4:54 a.m. UTC | #5

On Thu, Jun 20, 2019 at 6:28 AM ronnie sahlberg
<[e-mail]> wrote:
>
> Guess we need a fix to the man page.

You mean like this? ;-)
https://lore.kernel.org/linux-fsdevel/[e-mail]/


@@ -1546,6 +1547,14 @@ smb2_copychunk_range(const unsigned int xid,
        tcon = tlink_tcon(trgtfile->tlink);

        while (len > 0) {
+               if (src_off >= inode->i_size) {
+                       cifs_dbg(FYI, "nothing to do on copychunk\n");
+                       goto cchunk_out; /* nothing to do */
+               } else if (src_off + len > inode->i_size) {
+                       /* consider adding check to see if src oplocked */
+                       len = inode->i_size - src_off;
+                       cifs_dbg(FYI, "adjust copychunk len %lld less\n", len);
+               }
                pcchunk->SourceOffset = cpu_to_le64(src_off);
                pcchunk->TargetOffset = cpu_to_le64(dest_off);
                pcchunk->Length =

You can do this shortening before entering the while loop,
then you won't need to SMB2_request_res_key() from the server.
and certainly no need to do that in every loop iteration...

Thanks,
Amir.

Amir Goldstein June 20, 2019, 5:28 a.m. UTC | #6

On Thu, Jun 20, 2019 at 4:44 AM Steve French <[e-mail]> wrote:
>
> Patch attached fixes the case where copy_file_range over an SMB3 mount
> tries to go beyond the end of file of the source file.  This fixes
> xfstests generic/430 and generic/431
>
> Amir's patches had added a similar change in the VFS layer...

BTW, Steve, do you intend to pull Darrick's copy-file-range-fixes
branch for 5.3 and add the extra cifs "file_modified" patch?

Thanks,
Amir.

Steve French June 20, 2019, 5:46 a.m. UTC | #7

On Thu, Jun 20, 2019 at 12:28 AM Amir Goldstein <[e-mail]> wrote:
>
> On Thu, Jun 20, 2019 at 4:44 AM Steve French <[e-mail]> wrote:
> >
> > Patch attached fixes the case where copy_file_range over an SMB3 mount
> > tries to go beyond the end of file of the source file.  This fixes
> > xfstests generic/430 and generic/431
> >
> > Amir's patches had added a similar change in the VFS layer...
>
> BTW, Steve, do you intend to pull Darrick's copy-file-range-fixes
> branch for 5.3 and add the extra cifs "file_modified" patch?
>
> Thanks,
> Amir.

Yes - that seems reasonable to me

Steve French July 11, 2019, 3:16 p.m. UTC | #8

I noticed that the copy_file_range patches were merged (which is good)

On Wed, Jun 19, 2019 at 8:44 PM Steve French <[e-mail]> wrote:
>
> Patch attached fixes the case where copy_file_range over an SMB3 mount
> tries to go beyond the end of file of the source file.  This fixes
> xfstests generic/430 and generic/431
>
> Amir's patches had added a similar change in the VFS layer, but
> presumably harmless to have the check in cifs.ko as well to ensure
> that we don't try to copy beyond end of the source file (otherwise
> SMB3 servers will return an error on copychunk rather than doing the
> partial copy (up to end of the source file) that copy_file_range
> expects).
>
>
>
> --
> Thanks,
>
> Steve

Steve French July 11, 2019, 3:19 p.m. UTC | #9

I noticed that the copy_file_range patches were merged (which is good)
- see below.

Anything else to merge for the changes to cifs.ko that we discussed
earlier.  I wasn't sure about the "SMB3: fix copy file when beyond
size of source" patch - it may be redundant.  I will need to check
with current mainline.  Anything else needed for the enabling of
copy_file_range cross mount etc.

commit 40f06c799539739a08a56be8a096f56aeed05731
Merge: a47f5c56b2eb fe0da9c09b2d
Author: Linus Torvalds <[e-mail]>
Date:   Wed Jul 10 20:32:37 2019 -0700

    Merge tag 'copy-file-range-fixes-1' of
git://git.kernel.org/pub/scm/fs/xfs/xfs-linux

    Pull copy_file_range updates from Darrick Wong:





On Thu, Jul 11, 2019 at 10:16 AM Steve French <[e-mail]> wrote:
>
> I noticed that the copy_file_range patches were merged (which is good)
>
> On Wed, Jun 19, 2019 at 8:44 PM Steve French <[e-mail]> wrote:
> >
> > Patch attached fixes the case where copy_file_range over an SMB3 mount
> > tries to go beyond the end of file of the source file.  This fixes
> > xfstests generic/430 and generic/431
> >
> > Amir's patches had added a similar change in the VFS layer, but
> > presumably harmless to have the check in cifs.ko as well to ensure
> > that we don't try to copy beyond end of the source file (otherwise
> > SMB3 servers will return an error on copychunk rather than doing the
> > partial copy (up to end of the source file) that copy_file_range
> > expects).
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve

Amir Goldstein July 15, 2019, 7:50 a.m. UTC | #10

On Thu, Jul 11, 2019 at 6:19 PM Steve French <[e-mail]> wrote:
>
> I noticed that the copy_file_range patches were merged (which is good)
> - see below.
>
> Anything else to merge for the changes to cifs.ko that we discussed
> earlier.

There was the cifs patch that I sent you:

cifs: copy_file_range needs to strip setuid bits and update timestamps

That was not included in Darrick's branch.

> I wasn't sure about the "SMB3: fix copy file when beyond
> size of source" patch - it may be redundant.  I will need to check

It is redundant, but if you plan to forward a patch to stable, it
will be easier for you to forward just the CIFS patch, so up to you.
I am not sure if and when I will get to testing and forwarding
copy_file_range patches to stable. Not sure it makes sense.

> with current mainline.  Anything else needed for the enabling of
> copy_file_range cross mount etc.

The mtime update patch is not *needed* for enabling of
copy_file_range cross mount. It is a correctness patch.
So copy_file_range cross mount should work in mainline
with cifs as long as other cifs related bugs have been fixed.

Thanks,
Amir.

Patch

From a3d9033df7bb5206093f00eb037242336ff7ccfb Mon Sep 17 00:00:00 2001
From: Steve French <[e-mail]>
Date: Wed, 19 Jun 2019 15:10:12 -0500
Subject: [PATCH] [SMB3] fix copy file range when beyond size of source file

When requesting a copy which would go beyond the end of the
source file, only copy to the end of the source file instead
of returning an error.  Fixes xfstests generic/430 and
generic/431

Signed-off-by: Steve French <[e-mail]>
---
 fs/cifs/smb2ops.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 376577cc4159..1cdbeec56453 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1522,6 +1522,7 @@  smb2_copychunk_range(const unsigned int xid,
 	int chunks_copied = 0;
 	bool chunk_sizes_updated = false;
 	ssize_t bytes_written, total_bytes_written = 0;
+	struct inode *inode = d_inode(srcfile->dentry);
 
 	pcchunk = kmalloc(sizeof(struct copychunk_ioctl), GFP_KERNEL);
 
@@ -1546,6 +1547,14 @@  smb2_copychunk_range(const unsigned int xid,
 	tcon = tlink_tcon(trgtfile->tlink);
 
 	while (len > 0) {
+		if (src_off >= inode->i_size) {
+			cifs_dbg(FYI, "nothing to do on copychunk\n");
+			goto cchunk_out; /* nothing to do */
+		} else if (src_off + len > inode->i_size) {
+			/* consider adding check to see if src oplocked */
+			len = inode->i_size - src_off;
+			cifs_dbg(FYI, "adjust copychunk len %lld less\n", len);
+		}
 		pcchunk->SourceOffset = cpu_to_le64(src_off);
 		pcchunk->TargetOffset = cpu_to_le64(dest_off);
 		pcchunk->Length =
-- 
2.20.1