Linux-CIFS Archive on lore.kernel.org
 help / color / Atom feed
* [SMB3][PATCH] fix copy_file_range when copying beyond end of source file
@ 2019-06-20  1:44 Steve French
  2019-06-20  2:14 ` ronnie sahlberg
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Steve French @ 2019-06-20  1:44 UTC (permalink / raw)
  To: CIFS, samba-technical, Amir Goldstein, linux-fsdevel, Darrick J. Wong

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

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

[-- Attachment #2: 0001-SMB3-fix-copy-file-range-when-beyond-size-of-source-.patch --]
[-- Type: text/x-patch, Size: 1568 bytes --]

From a3d9033df7bb5206093f00eb037242336ff7ccfb Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
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 <stfrench@microsoft.com>
---
 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [SMB3][PATCH] fix copy_file_range when copying beyond end of source file
  2019-06-20  1:44 [SMB3][PATCH] fix copy_file_range when copying beyond end of source file Steve French
@ 2019-06-20  2:14 ` ronnie sahlberg
  2019-06-20  3:19   ` Steve French
  2019-06-20  5:28 ` Amir Goldstein
  2019-07-11 15:16 ` Steve French
  2 siblings, 1 reply; 11+ messages in thread
From: ronnie sahlberg @ 2019-06-20  2:14 UTC (permalink / raw)
  To: Steve French
  Cc: CIFS, samba-technical, Amir Goldstein, linux-fsdevel, Darrick J. Wong

On Thu, Jun 20, 2019 at 11:45 AM Steve French via samba-technical
<samba-technical@lists.samba.org> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [SMB3][PATCH] fix copy_file_range when copying beyond end of source file
  2019-06-20  2:14 ` ronnie sahlberg
@ 2019-06-20  3:19   ` Steve French
  2019-06-20  3:28     ` ronnie sahlberg
  2019-06-20  4:52     ` Dave Chinner
  0 siblings, 2 replies; 11+ messages in thread
From: Steve French @ 2019-06-20  3:19 UTC (permalink / raw)
  To: ronnie sahlberg
  Cc: CIFS, samba-technical, Amir Goldstein, linux-fsdevel, Darrick J. Wong

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
<ronniesahlberg@gmail.com> wrote:
>
> On Thu, Jun 20, 2019 at 11:45 AM Steve French via samba-technical
> <samba-technical@lists.samba.org> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [SMB3][PATCH] fix copy_file_range when copying beyond end of source file
  2019-06-20  3:19   ` Steve French
@ 2019-06-20  3:28     ` ronnie sahlberg
  2019-06-20  4:54       ` Amir Goldstein
  2019-06-20  4:52     ` Dave Chinner
  1 sibling, 1 reply; 11+ messages in thread
From: ronnie sahlberg @ 2019-06-20  3:28 UTC (permalink / raw)
  To: Steve French
  Cc: CIFS, samba-technical, Amir Goldstein, linux-fsdevel, Darrick J. Wong

Guess we need a fix to the man page.

Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>

On Thu, Jun 20, 2019 at 1:19 PM Steve French <smfrench@gmail.com> 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
> <ronniesahlberg@gmail.com> wrote:
> >
> > On Thu, Jun 20, 2019 at 11:45 AM Steve French via samba-technical
> > <samba-technical@lists.samba.org> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [SMB3][PATCH] fix copy_file_range when copying beyond end of source file
  2019-06-20  3:19   ` Steve French
  2019-06-20  3:28     ` ronnie sahlberg
@ 2019-06-20  4:52     ` Dave Chinner
  1 sibling, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2019-06-20  4:52 UTC (permalink / raw)
  To: Steve French
  Cc: ronnie sahlberg, CIFS, samba-technical, Amir Goldstein,
	linux-fsdevel, Darrick J. Wong

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.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [SMB3][PATCH] fix copy_file_range when copying beyond end of source file
  2019-06-20  3:28     ` ronnie sahlberg
@ 2019-06-20  4:54       ` Amir Goldstein
  0 siblings, 0 replies; 11+ messages in thread
From: Amir Goldstein @ 2019-06-20  4:54 UTC (permalink / raw)
  To: ronnie sahlberg
  Cc: Steve French, CIFS, samba-technical, linux-fsdevel, Darrick J. Wong

On Thu, Jun 20, 2019 at 6:28 AM ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
>
> Guess we need a fix to the man page.

You mean like this? ;-)
https://lore.kernel.org/linux-fsdevel/20190529174318.22424-15-amir73il@gmail.com/


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [SMB3][PATCH] fix copy_file_range when copying beyond end of source file
  2019-06-20  1:44 [SMB3][PATCH] fix copy_file_range when copying beyond end of source file Steve French
  2019-06-20  2:14 ` ronnie sahlberg
@ 2019-06-20  5:28 ` Amir Goldstein
  2019-06-20  5:46   ` Steve French
  2019-07-11 15:16 ` Steve French
  2 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2019-06-20  5:28 UTC (permalink / raw)
  To: Steve French; +Cc: CIFS, samba-technical, linux-fsdevel, Darrick J. Wong

On Thu, Jun 20, 2019 at 4:44 AM Steve French <smfrench@gmail.com> 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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [SMB3][PATCH] fix copy_file_range when copying beyond end of source file
  2019-06-20  5:28 ` Amir Goldstein
@ 2019-06-20  5:46   ` Steve French
  0 siblings, 0 replies; 11+ messages in thread
From: Steve French @ 2019-06-20  5:46 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: CIFS, samba-technical, linux-fsdevel, Darrick J. Wong

On Thu, Jun 20, 2019 at 12:28 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Jun 20, 2019 at 4:44 AM Steve French <smfrench@gmail.com> 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


-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [SMB3][PATCH] fix copy_file_range when copying beyond end of source file
  2019-06-20  1:44 [SMB3][PATCH] fix copy_file_range when copying beyond end of source file Steve French
  2019-06-20  2:14 ` ronnie sahlberg
  2019-06-20  5:28 ` Amir Goldstein
@ 2019-07-11 15:16 ` Steve French
  2019-07-11 15:19   ` Steve French
  2 siblings, 1 reply; 11+ messages in thread
From: Steve French @ 2019-07-11 15:16 UTC (permalink / raw)
  To: CIFS, samba-technical, Amir Goldstein, linux-fsdevel, Darrick J. Wong

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

On Wed, Jun 19, 2019 at 8:44 PM Steve French <smfrench@gmail.com> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [SMB3][PATCH] fix copy_file_range when copying beyond end of source file
  2019-07-11 15:16 ` Steve French
@ 2019-07-11 15:19   ` Steve French
  2019-07-15  7:50     ` Amir Goldstein
  0 siblings, 1 reply; 11+ messages in thread
From: Steve French @ 2019-07-11 15:19 UTC (permalink / raw)
  To: CIFS, samba-technical, Amir Goldstein, linux-fsdevel, Darrick J. Wong

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 <torvalds@linux-foundation.org>
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 <smfrench@gmail.com> 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 <smfrench@gmail.com> 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



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [SMB3][PATCH] fix copy_file_range when copying beyond end of source file
  2019-07-11 15:19   ` Steve French
@ 2019-07-15  7:50     ` Amir Goldstein
  0 siblings, 0 replies; 11+ messages in thread
From: Amir Goldstein @ 2019-07-15  7:50 UTC (permalink / raw)
  To: Steve French; +Cc: CIFS, samba-technical, linux-fsdevel, Darrick J. Wong

On Thu, Jul 11, 2019 at 6:19 PM Steve French <smfrench@gmail.com> 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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-20  1:44 [SMB3][PATCH] fix copy_file_range when copying beyond end of source file Steve French
2019-06-20  2:14 ` ronnie sahlberg
2019-06-20  3:19   ` Steve French
2019-06-20  3:28     ` ronnie sahlberg
2019-06-20  4:54       ` Amir Goldstein
2019-06-20  4:52     ` Dave Chinner
2019-06-20  5:28 ` Amir Goldstein
2019-06-20  5:46   ` Steve French
2019-07-11 15:16 ` Steve French
2019-07-11 15:19   ` Steve French
2019-07-15  7:50     ` Amir Goldstein

Linux-CIFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cifs/0 linux-cifs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cifs linux-cifs/ https://lore.kernel.org/linux-cifs \
		linux-cifs@vger.kernel.org linux-cifs@archiver.kernel.org
	public-inbox-index linux-cifs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cifs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox