[PATCH 0/5] smb3: Fix missing locks and invalidation in fallocate

[PATCH 0/5] smb3: Fix missing locks and invalidation in fallocate

[David Howells]

Here are some patches to fix locking and invalidation in the smb3/cifs
fallocate, in particular in zero_range, punch_hole, collapse_range and
insert_range.

Those four operations were, for the most part, missing calls to inode_lock(),
filemap_invalidate_lock() and truncate_pagecache_range(), the last of which
was causing generic/031 to show data corruption.

David
---
David Howells (4):
      smb3: Move the flush out of smb2_copychunk_range() into its callers
      smb3: missing inode locks in zero range
      smb3: missing inode locks in punch hole
      smb3: fix temporary data corruption in insert range

Steve French (1):
      smb3: fix temporary data corruption in collapse range


 fs/cifs/cifsfs.c  |   2 +
 fs/cifs/smb2ops.c | 131 ++++++++++++++++++++++++++--------------------
 2 files changed, 75 insertions(+), 58 deletions(-)

[PATCH 1/5] smb3: Move the flush out of smb2_copychunk_range() into its callers

[David Howells]

Move the flush out of smb2_copychunk_range() into its callers.  This will
allow the pagecache to be invalidated between the flush and the operation
in smb3_collapse_range() and smb3_insert_range().

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Steve French <stfrench@microsoft.com>
cc: Ronnie Sahlberg <lsahlber@redhat.com>
---

 fs/cifs/cifsfs.c  |    2 ++
 fs/cifs/smb2ops.c |   20 ++++++++------------
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index f54d8bf2732a..e9fb338b8e7e 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1219,6 +1219,8 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 
 	cifs_dbg(FYI, "copychunk range\n");
 
+	filemap_write_and_wait(src_inode->i_mapping);
+
 	if (!src_file->private_data || !dst_file->private_data) {
 		rc = -EBADF;
 		cifs_dbg(VFS, "missing cifsFileInfo on copy range src file\n");
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 96f3b0573606..7e3de6a0e1dc 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1600,17 +1600,8 @@ 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;
 
 	pcchunk = kmalloc(sizeof(struct copychunk_ioctl), GFP_KERNEL);
-
-	/*
-	 * We need to flush all unwritten data before we can send the
-	 * copychunk ioctl to the server.
-	 */
-	inode = d_inode(trgtfile->dentry);
-	filemap_write_and_wait(inode->i_mapping);
-
 	if (pcchunk == NULL)
 		return -ENOMEM;
 
@@ -3689,6 +3680,8 @@ static long smb3_collapse_range(struct file *file, struct cifs_tcon *tcon,
 		goto out;
 	}
 
+	filemap_write_and_wait(inode->i_mapping);
+
 	rc = smb2_copychunk_range(xid, cfile, cfile, off + len,
 				  i_size_read(inode) - off - len, off);
 	if (rc < 0)
@@ -3716,18 +3709,21 @@ static long smb3_insert_range(struct file *file, struct cifs_tcon *tcon,
 	int rc;
 	unsigned int xid;
 	struct cifsFileInfo *cfile = file->private_data;
+	struct inode *inode = file_inode(file);
 	__le64 eof;
 	__u64  count;
 
 	xid = get_xid();
 
-	if (off >= i_size_read(file->f_inode)) {
+	if (off >= i_size_read(inode)) {
 		rc = -EINVAL;
 		goto out;
 	}
 
-	count = i_size_read(file->f_inode) - off;
-	eof = cpu_to_le64(i_size_read(file->f_inode) + len);
+	count = i_size_read(inode) - off;
+	eof = cpu_to_le64(i_size_read(inode) + len);
+
+	filemap_write_and_wait(inode->i_mapping);
 
 	rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
 			  cfile->fid.volatile_fid, cfile->pid, &eof);

Re: [PATCH 1/5] smb3: Move the flush out of smb2_copychunk_range() into its callers

[Steve French]

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

Shouldn't this be using filemap_write_and_wait_range() not
filemap_write_and_wait as we see in similar code in ext4 (and
shouldn't it check rc in some of these cases)?   For example for the
copychunk_range example shouldn't the patch be modified similar to the
following:

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index e9fb338b8e7e..51963e83daf7 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1219,8 +1219,6 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,

        cifs_dbg(FYI, "copychunk range\n");

-       filemap_write_and_wait(src_inode->i_mapping);
-
        if (!src_file->private_data || !dst_file->private_data) {
                rc = -EBADF;
                cifs_dbg(VFS, "missing cifsFileInfo on copy range src file\n");
@@ -1250,6 +1248,12 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
        lock_two_nondirectories(target_inode, src_inode);

        cifs_dbg(FYI, "about to flush pages\n");
+
+       rc = filemap_write_and_wait_range(src_inode->i_mapping, off,
+                                         off + len - 1);
+        if (rc)
+               goto out;
+
        /* should we flush first and last page first */
        truncate_inode_pages(&target_inode->i_data, 0);

On Tue, Aug 23, 2022 at 8:09 AM David Howells via samba-technical
<samba-technical@lists.samba.org> wrote:
>
> Move the flush out of smb2_copychunk_range() into its callers.  This will
> allow the pagecache to be invalidated between the flush and the operation
> in smb3_collapse_range() and smb3_insert_range().
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Steve French <stfrench@microsoft.com>
> cc: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>
>  fs/cifs/cifsfs.c  |    2 ++
>  fs/cifs/smb2ops.c |   20 ++++++++------------
>  2 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index f54d8bf2732a..e9fb338b8e7e 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -1219,6 +1219,8 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
>
>         cifs_dbg(FYI, "copychunk range\n");
>
> +       filemap_write_and_wait(src_inode->i_mapping);
> +
>         if (!src_file->private_data || !dst_file->private_data) {
>                 rc = -EBADF;
>                 cifs_dbg(VFS, "missing cifsFileInfo on copy range src file\n");
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 96f3b0573606..7e3de6a0e1dc 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1600,17 +1600,8 @@ 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;
>
>         pcchunk = kmalloc(sizeof(struct copychunk_ioctl), GFP_KERNEL);
> -
> -       /*
> -        * We need to flush all unwritten data before we can send the
> -        * copychunk ioctl to the server.
> -        */
> -       inode = d_inode(trgtfile->dentry);
> -       filemap_write_and_wait(inode->i_mapping);
> -
>         if (pcchunk == NULL)
>                 return -ENOMEM;
>
> @@ -3689,6 +3680,8 @@ static long smb3_collapse_range(struct file *file, struct cifs_tcon *tcon,
>                 goto out;
>         }
>
> +       filemap_write_and_wait(inode->i_mapping);
> +
>         rc = smb2_copychunk_range(xid, cfile, cfile, off + len,
>                                   i_size_read(inode) - off - len, off);
>         if (rc < 0)
> @@ -3716,18 +3709,21 @@ static long smb3_insert_range(struct file *file, struct cifs_tcon *tcon,
>         int rc;
>         unsigned int xid;
>         struct cifsFileInfo *cfile = file->private_data;
> +       struct inode *inode = file_inode(file);
>         __le64 eof;
>         __u64  count;
>
>         xid = get_xid();
>
> -       if (off >= i_size_read(file->f_inode)) {
> +       if (off >= i_size_read(inode)) {
>                 rc = -EINVAL;
>                 goto out;
>         }
>
> -       count = i_size_read(file->f_inode) - off;
> -       eof = cpu_to_le64(i_size_read(file->f_inode) + len);
> +       count = i_size_read(inode) - off;
> +       eof = cpu_to_le64(i_size_read(inode) + len);
> +
> +       filemap_write_and_wait(inode->i_mapping);
>
>         rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
>                           cfile->fid.volatile_fid, cfile->pid, &eof);
>
>
>


-- 
Thanks,

Steve

[-- Attachment #2: use-filemap_write_and_wait_range.patch --]
[-- Type: text/x-patch, Size: 839 bytes --]

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index e9fb338b8e7e..51963e83daf7 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1219,8 +1219,6 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 
 	cifs_dbg(FYI, "copychunk range\n");
 
-	filemap_write_and_wait(src_inode->i_mapping);
-
 	if (!src_file->private_data || !dst_file->private_data) {
 		rc = -EBADF;
 		cifs_dbg(VFS, "missing cifsFileInfo on copy range src file\n");
@@ -1250,6 +1248,12 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 	lock_two_nondirectories(target_inode, src_inode);
 
 	cifs_dbg(FYI, "about to flush pages\n");
+
+	rc = filemap_write_and_wait_range(src_inode->i_mapping, off,
+					  off + len - 1);
+        if (rc)
+		goto out;
+
 	/* should we flush first and last page first */
 	truncate_inode_pages(&target_inode->i_data, 0);
 

Re: [PATCH 1/5] smb3: Move the flush out of smb2_copychunk_range() into its callers

[Steve French]

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

e.g. something like the following


On Mon, Aug 29, 2022 at 12:06 AM Steve French <smfrench@gmail.com> wrote:
>
> Shouldn't this be using filemap_write_and_wait_range() not
> filemap_write_and_wait as we see in similar code in ext4 (and
> shouldn't it check rc in some of these cases)?   For example for the
> copychunk_range example shouldn't the patch be modified similar to the
> following:
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index e9fb338b8e7e..51963e83daf7 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -1219,8 +1219,6 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
>
>         cifs_dbg(FYI, "copychunk range\n");
>
> -       filemap_write_and_wait(src_inode->i_mapping);
> -
>         if (!src_file->private_data || !dst_file->private_data) {
>                 rc = -EBADF;
>                 cifs_dbg(VFS, "missing cifsFileInfo on copy range src file\n");
> @@ -1250,6 +1248,12 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
>         lock_two_nondirectories(target_inode, src_inode);
>
>         cifs_dbg(FYI, "about to flush pages\n");
> +
> +       rc = filemap_write_and_wait_range(src_inode->i_mapping, off,
> +                                         off + len - 1);
> +        if (rc)
> +               goto out;
> +
>         /* should we flush first and last page first */
>         truncate_inode_pages(&target_inode->i_data, 0);
>
> On Tue, Aug 23, 2022 at 8:09 AM David Howells via samba-technical
> <samba-technical@lists.samba.org> wrote:
> >
> > Move the flush out of smb2_copychunk_range() into its callers.  This will
> > allow the pagecache to be invalidated between the flush and the operation
> > in smb3_collapse_range() and smb3_insert_range().
> >
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > cc: Steve French <stfrench@microsoft.com>
> > cc: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >
> >  fs/cifs/cifsfs.c  |    2 ++
> >  fs/cifs/smb2ops.c |   20 ++++++++------------
> >  2 files changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index f54d8bf2732a..e9fb338b8e7e 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -1219,6 +1219,8 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
> >
> >         cifs_dbg(FYI, "copychunk range\n");
> >
> > +       filemap_write_and_wait(src_inode->i_mapping);
> > +
> >         if (!src_file->private_data || !dst_file->private_data) {
> >                 rc = -EBADF;
> >                 cifs_dbg(VFS, "missing cifsFileInfo on copy range src file\n");
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index 96f3b0573606..7e3de6a0e1dc 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -1600,17 +1600,8 @@ 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;
> >
> >         pcchunk = kmalloc(sizeof(struct copychunk_ioctl), GFP_KERNEL);
> > -
> > -       /*
> > -        * We need to flush all unwritten data before we can send the
> > -        * copychunk ioctl to the server.
> > -        */
> > -       inode = d_inode(trgtfile->dentry);
> > -       filemap_write_and_wait(inode->i_mapping);
> > -
> >         if (pcchunk == NULL)
> >                 return -ENOMEM;
> >
> > @@ -3689,6 +3680,8 @@ static long smb3_collapse_range(struct file *file, struct cifs_tcon *tcon,
> >                 goto out;
> >         }
> >
> > +       filemap_write_and_wait(inode->i_mapping);
> > +
> >         rc = smb2_copychunk_range(xid, cfile, cfile, off + len,
> >                                   i_size_read(inode) - off - len, off);
> >         if (rc < 0)
> > @@ -3716,18 +3709,21 @@ static long smb3_insert_range(struct file *file, struct cifs_tcon *tcon,
> >         int rc;
> >         unsigned int xid;
> >         struct cifsFileInfo *cfile = file->private_data;
> > +       struct inode *inode = file_inode(file);
> >         __le64 eof;
> >         __u64  count;
> >
> >         xid = get_xid();
> >
> > -       if (off >= i_size_read(file->f_inode)) {
> > +       if (off >= i_size_read(inode)) {
> >                 rc = -EINVAL;
> >                 goto out;
> >         }
> >
> > -       count = i_size_read(file->f_inode) - off;
> > -       eof = cpu_to_le64(i_size_read(file->f_inode) + len);
> > +       count = i_size_read(inode) - off;
> > +       eof = cpu_to_le64(i_size_read(inode) + len);
> > +
> > +       filemap_write_and_wait(inode->i_mapping);
> >
> >         rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
> >                           cfile->fid.volatile_fid, cfile->pid, &eof);
> >
> >
> >
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb3-use-filemap_write_and_wait_range-instead-of-fil.patch --]
[-- Type: text/x-patch, Size: 2262 bytes --]

From 5b12e7e9dcefe683c18ccfa064d216c8f83672d0 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Mon, 29 Aug 2022 11:53:41 -0500
Subject: [PATCH] smb3: use filemap_write_and_wait_range instead of
 filemap_write_and_wait

When doing insert range and collapse range we should be
writing out the cached pages for the ranges affected but not
the whole file.

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsfs.c  | 8 ++++++--
 fs/cifs/smb2ops.c | 2 ++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index e9fb338b8e7e..51963e83daf7 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1219,8 +1219,6 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 
 	cifs_dbg(FYI, "copychunk range\n");
 
-	filemap_write_and_wait(src_inode->i_mapping);
-
 	if (!src_file->private_data || !dst_file->private_data) {
 		rc = -EBADF;
 		cifs_dbg(VFS, "missing cifsFileInfo on copy range src file\n");
@@ -1250,6 +1248,12 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 	lock_two_nondirectories(target_inode, src_inode);
 
 	cifs_dbg(FYI, "about to flush pages\n");
+
+	rc = filemap_write_and_wait_range(src_inode->i_mapping, off,
+					  off + len - 1);
+	if (rc)
+		goto out;
+
 	/* should we flush first and last page first */
 	truncate_inode_pages(&target_inode->i_data, 0);
 
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 7c941ce1e7a9..12319cef42a7 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -3688,6 +3688,7 @@ static long smb3_collapse_range(struct file *file, struct cifs_tcon *tcon,
 
 	filemap_invalidate_lock(inode->i_mapping);
 	filemap_write_and_wait(inode->i_mapping);
+	rc = filemap_write_and_wait_range(inode->i_mapping, off, old_eof - 1);
 	truncate_pagecache_range(inode, off, old_eof);
 
 	rc = smb2_copychunk_range(xid, cfile, cfile, off + len,
@@ -3739,6 +3740,7 @@ static long smb3_insert_range(struct file *file, struct cifs_tcon *tcon,
 
 	filemap_invalidate_lock(inode->i_mapping);
 	filemap_write_and_wait(inode->i_mapping);
+	rc = filemap_write_and_wait_range(inode->i_mapping, off, eof - 1);
 	truncate_pagecache_range(inode, off, old_eof);
 
 	rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
-- 
2.34.1

[PATCH 2/5] smb3: missing inode locks in zero range

[David Howells]

smb3 fallocate zero range was not grabbing the inode or filemap_invalidate
locks so could have race with pagemap reinstantiating the page.

Cc: stable@vger.kernel.org
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---

 fs/cifs/smb2ops.c |   55 +++++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 7e3de6a0e1dc..1c5a93ced946 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -3298,26 +3298,43 @@ get_smb2_acl(struct cifs_sb_info *cifs_sb,
 	return pntsd;
 }
 
+static long smb3_zero_data(struct file *file, struct cifs_tcon *tcon,
+			     loff_t offset, loff_t len, unsigned int xid)
+{
+	struct cifsFileInfo *cfile = file->private_data;
+	struct file_zero_data_information fsctl_buf;
+
+	cifs_dbg(FYI, "Offset %lld len %lld\n", offset, len);
+
+	fsctl_buf.FileOffset = cpu_to_le64(offset);
+	fsctl_buf.BeyondFinalZero = cpu_to_le64(offset + len);
+
+	return SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
+			  cfile->fid.volatile_fid, FSCTL_SET_ZERO_DATA,
+			  (char *)&fsctl_buf,
+			  sizeof(struct file_zero_data_information),
+			  0, NULL, NULL);
+}
+
 static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
 			    loff_t offset, loff_t len, bool keep_size)
 {
 	struct cifs_ses *ses = tcon->ses;
-	struct inode *inode;
-	struct cifsInodeInfo *cifsi;
+	struct inode *inode = file_inode(file);
+	struct cifsInodeInfo *cifsi = CIFS_I(inode);
 	struct cifsFileInfo *cfile = file->private_data;
-	struct file_zero_data_information fsctl_buf;
 	long rc;
 	unsigned int xid;
 	__le64 eof;
 
 	xid = get_xid();
 
-	inode = d_inode(cfile->dentry);
-	cifsi = CIFS_I(inode);
-
 	trace_smb3_zero_enter(xid, cfile->fid.persistent_fid, tcon->tid,
 			      ses->Suid, offset, len);
 
+	inode_lock(inode);
+	filemap_invalidate_lock(inode->i_mapping);
+
 	/*
 	 * We zero the range through ioctl, so we need remove the page caches
 	 * first, otherwise the data may be inconsistent with the server.
@@ -3325,26 +3342,12 @@ static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
 	truncate_pagecache_range(inode, offset, offset + len - 1);
 
 	/* if file not oplocked can't be sure whether asking to extend size */
-	if (!CIFS_CACHE_READ(cifsi))
-		if (keep_size == false) {
-			rc = -EOPNOTSUPP;
-			trace_smb3_zero_err(xid, cfile->fid.persistent_fid,
-				tcon->tid, ses->Suid, offset, len, rc);
-			free_xid(xid);
-			return rc;
-		}
-
-	cifs_dbg(FYI, "Offset %lld len %lld\n", offset, len);
-
-	fsctl_buf.FileOffset = cpu_to_le64(offset);
-	fsctl_buf.BeyondFinalZero = cpu_to_le64(offset + len);
+	rc = -EOPNOTSUPP;
+	if (keep_size == false && !CIFS_CACHE_READ(cifsi))
+		goto zero_range_exit;
 
-	rc = SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
-			cfile->fid.volatile_fid, FSCTL_SET_ZERO_DATA,
-			(char *)&fsctl_buf,
-			sizeof(struct file_zero_data_information),
-			0, NULL, NULL);
-	if (rc)
+	rc = smb3_zero_data(file, tcon, offset, len, xid);
+	if (rc < 0)
 		goto zero_range_exit;
 
 	/*
@@ -3357,6 +3360,8 @@ static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
 	}
 
  zero_range_exit:
+	filemap_invalidate_unlock(inode->i_mapping);
+	inode_unlock(inode);
 	free_xid(xid);
 	if (rc)
 		trace_smb3_zero_err(xid, cfile->fid.persistent_fid, tcon->tid,

[PATCH 3/5] smb3: fix temporary data corruption in collapse range

[David Howells]

From: Steve French <stfrench@microsoft.com>

collapse range doesn't discard the affected cached region
so can risk temporarily corrupting the file data. This
fixes xfstest generic/031

I also decided to merge a minor cleanup to this into the same patch
(avoiding rereading inode size repeatedly unnecessarily) to make it
clearer.

Cc: stable@vger.kernel.org
Fixes: 5476b5dd82c8b ("cifs: add support for FALLOC_FL_COLLAPSE_RANGE")
Reported-by: David Howells <dhowells@redhat.com>
Tested-by: David Howells <dhowells@redhat.com>
Reviewed-by: David Howells <dhowells@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
cc: Ronnie Sahlberg <lsahlber@redhat.com>
---

 fs/cifs/smb2ops.c |   26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 1c5a93ced946..75fcf6a0df56 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -3669,41 +3669,47 @@ static long smb3_collapse_range(struct file *file, struct cifs_tcon *tcon,
 {
 	int rc;
 	unsigned int xid;
-	struct inode *inode;
+	struct inode *inode = file_inode(file);
 	struct cifsFileInfo *cfile = file->private_data;
-	struct cifsInodeInfo *cifsi;
+	struct cifsInodeInfo *cifsi = CIFS_I(inode);
 	__le64 eof;
+	loff_t old_eof;
 
 	xid = get_xid();
 
-	inode = d_inode(cfile->dentry);
-	cifsi = CIFS_I(inode);
+	inode_lock(inode);
 
-	if (off >= i_size_read(inode) ||
-	    off + len >= i_size_read(inode)) {
+	old_eof = i_size_read(inode);
+	if ((off >= old_eof) ||
+	    off + len >= old_eof) {
 		rc = -EINVAL;
 		goto out;
 	}
 
+	filemap_invalidate_lock(inode->i_mapping);
 	filemap_write_and_wait(inode->i_mapping);
+	truncate_pagecache_range(inode, off, old_eof);
 
 	rc = smb2_copychunk_range(xid, cfile, cfile, off + len,
-				  i_size_read(inode) - off - len, off);
+				  old_eof - off - len, off);
 	if (rc < 0)
-		goto out;
+		goto out_2;
 
-	eof = cpu_to_le64(i_size_read(inode) - len);
+	eof = cpu_to_le64(old_eof - len);
 	rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
 			  cfile->fid.volatile_fid, cfile->pid, &eof);
 	if (rc < 0)
-		goto out;
+		goto out_2;
 
 	rc = 0;
 
 	cifsi->server_eof = i_size_read(inode) - len;
 	truncate_setsize(inode, cifsi->server_eof);
 	fscache_resize_cookie(cifs_inode_cookie(inode), cifsi->server_eof);
+out_2:
+	filemap_invalidate_unlock(inode->i_mapping);
  out:
+	inode_unlock(inode);
 	free_xid(xid);
 	return rc;
 }

Re: [PATCH 3/5] smb3: fix temporary data corruption in collapse range

[Matthew Wilcox]

On Tue, Aug 23, 2022 at 02:07:41PM +0100, David Howells wrote:
>  
> +	filemap_invalidate_lock(inode->i_mapping);
>  	filemap_write_and_wait(inode->i_mapping);
> +	truncate_pagecache_range(inode, off, old_eof);

It's a bit odd to writeback the entire file but then truncate only part
of it.  XFS does the same part:

        error = filemap_write_and_wait_range(inode->i_mapping, start, end);
        if (error)
                return error;
        truncate_pagecache_range(inode, start, end);

... and presumably, you'd also want the error check?

>  	rc = smb2_copychunk_range(xid, cfile, cfile, off + len,
> -				  i_size_read(inode) - off - len, off);
> +				  old_eof - off - len, off);

Re: [PATCH 3/5] smb3: fix temporary data corruption in collapse range

[David Howells]

Matthew Wilcox <willy@infradead.org> wrote:

>         truncate_pagecache_range(inode, start, end);
> 
> ... and presumably, you'd also want the error check?

truncate_pagecache_range() is void.

David

Re: [PATCH 3/5] smb3: fix temporary data corruption in collapse range

[David Howells]

Matthew Wilcox <willy@infradead.org> wrote:

> >  	filemap_write_and_wait(inode->i_mapping);
> > +	truncate_pagecache_range(inode, off, old_eof);
> 
> It's a bit odd to writeback the entire file but then truncate only part
> of it.  XFS does the same part:

Actually, filemap_write_and_wait() should check for error, yes.

Is there something that combines these that we should use?
invalidate_inode_pages2_range() for example.

David

[PATCH 4/5] smb3: missing inode locks in punch hole

[David Howells]

smb3 fallocate punch hole was not grabbing the inode or filemap_invalidate
locks so could have race with pagemap reinstantiating the page.

Cc: stable@vger.kernel.org
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---

 fs/cifs/smb2ops.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 75fcf6a0df56..5b5ddc1b4638 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -3375,7 +3375,7 @@ static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
 static long smb3_punch_hole(struct file *file, struct cifs_tcon *tcon,
 			    loff_t offset, loff_t len)
 {
-	struct inode *inode;
+	struct inode *inode = file_inode(file);
 	struct cifsFileInfo *cfile = file->private_data;
 	struct file_zero_data_information fsctl_buf;
 	long rc;
@@ -3384,14 +3384,12 @@ static long smb3_punch_hole(struct file *file, struct cifs_tcon *tcon,
 
 	xid = get_xid();
 
-	inode = d_inode(cfile->dentry);
-
+	inode_lock(inode);
 	/* Need to make file sparse, if not already, before freeing range. */
 	/* Consider adding equivalent for compressed since it could also work */
 	if (!smb2_set_sparse(xid, tcon, cfile, inode, set_sparse)) {
 		rc = -EOPNOTSUPP;
-		free_xid(xid);
-		return rc;
+		goto out;
 	}
 
 	filemap_invalidate_lock(inode->i_mapping);
@@ -3411,8 +3409,10 @@ static long smb3_punch_hole(struct file *file, struct cifs_tcon *tcon,
 			(char *)&fsctl_buf,
 			sizeof(struct file_zero_data_information),
 			CIFSMaxBufSize, NULL, NULL);
-	free_xid(xid);
 	filemap_invalidate_unlock(inode->i_mapping);
+out:
+	inode_unlock(inode);
+	free_xid(xid);
 	return rc;
 }
 

[PATCH 5/5] smb3: fix temporary data corruption in insert range

[David Howells]

insert range doesn't discard the affected cached region
so can risk temporarily corrupting file data.

Also includes some minor cleanup (avoiding rereading
inode size repeatedly unnecessarily) to make it clearer.

Cc: stable@vger.kernel.org
Fixes: 7fe6fe95b9360 ("cifs: FALLOC_FL_INSERT_RANGE support")
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
cc: Ronnie Sahlberg <lsahlber@redhat.com>
---

 fs/cifs/smb2ops.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 5b5ddc1b4638..00c8d6a715c7 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -3722,35 +3722,43 @@ static long smb3_insert_range(struct file *file, struct cifs_tcon *tcon,
 	struct cifsFileInfo *cfile = file->private_data;
 	struct inode *inode = file_inode(file);
 	__le64 eof;
-	__u64  count;
+	__u64  count, old_eof;
+
+	inode_lock(inode);
 
 	xid = get_xid();
 
-	if (off >= i_size_read(inode)) {
+	old_eof = i_size_read(inode);
+	if (off >= old_eof) {
 		rc = -EINVAL;
 		goto out;
 	}
 
-	count = i_size_read(inode) - off;
-	eof = cpu_to_le64(i_size_read(inode) + len);
+	count = old_eof - off;
+	eof = cpu_to_le64(old_eof + len);
 
+	filemap_invalidate_lock(inode->i_mapping);
 	filemap_write_and_wait(inode->i_mapping);
+	truncate_pagecache_range(inode, off, old_eof);
 
 	rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
 			  cfile->fid.volatile_fid, cfile->pid, &eof);
 	if (rc < 0)
-		goto out;
+		goto out_2;
 
 	rc = smb2_copychunk_range(xid, cfile, cfile, off, count, off + len);
 	if (rc < 0)
-		goto out;
+		goto out_2;
 
-	rc = smb3_zero_range(file, tcon, off, len, 1);
+	rc = smb3_zero_data(file, tcon, off, len, xid);
 	if (rc < 0)
-		goto out;
+		goto out_2;
 
 	rc = 0;
+out_2:
+	filemap_invalidate_unlock(inode->i_mapping);
  out:
+	inode_unlock(inode);
 	free_xid(xid);
 	return rc;
 }

Re: [PATCH 5/5] smb3: fix temporary data corruption in insert range

[Steve French]

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

lightly updated to move inode lock down one line and fix signed off

On Tue, Aug 23, 2022 at 8:24 AM David Howells via samba-technical
<samba-technical@lists.samba.org> wrote:
>
> insert range doesn't discard the affected cached region
> so can risk temporarily corrupting file data.
>
> Also includes some minor cleanup (avoiding rereading
> inode size repeatedly unnecessarily) to make it clearer.
>
> Cc: stable@vger.kernel.org
> Fixes: 7fe6fe95b9360 ("cifs: FALLOC_FL_INSERT_RANGE support")
> Signed-off-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> cc: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>
>  fs/cifs/smb2ops.c |   24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 5b5ddc1b4638..00c8d6a715c7 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -3722,35 +3722,43 @@ static long smb3_insert_range(struct file *file, struct cifs_tcon *tcon,
>         struct cifsFileInfo *cfile = file->private_data;
>         struct inode *inode = file_inode(file);
>         __le64 eof;
> -       __u64  count;
> +       __u64  count, old_eof;
> +
> +       inode_lock(inode);
>
>         xid = get_xid();
>
> -       if (off >= i_size_read(inode)) {
> +       old_eof = i_size_read(inode);
> +       if (off >= old_eof) {
>                 rc = -EINVAL;
>                 goto out;
>         }
>
> -       count = i_size_read(inode) - off;
> -       eof = cpu_to_le64(i_size_read(inode) + len);
> +       count = old_eof - off;
> +       eof = cpu_to_le64(old_eof + len);
>
> +       filemap_invalidate_lock(inode->i_mapping);
>         filemap_write_and_wait(inode->i_mapping);
> +       truncate_pagecache_range(inode, off, old_eof);
>
>         rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
>                           cfile->fid.volatile_fid, cfile->pid, &eof);
>         if (rc < 0)
> -               goto out;
> +               goto out_2;
>
>         rc = smb2_copychunk_range(xid, cfile, cfile, off, count, off + len);
>         if (rc < 0)
> -               goto out;
> +               goto out_2;
>
> -       rc = smb3_zero_range(file, tcon, off, len, 1);
> +       rc = smb3_zero_data(file, tcon, off, len, xid);
>         if (rc < 0)
> -               goto out;
> +               goto out_2;
>
>         rc = 0;
> +out_2:
> +       filemap_invalidate_unlock(inode->i_mapping);
>   out:
> +       inode_unlock(inode);
>         free_xid(xid);
>         return rc;
>  }
>
>
>


-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb3-fix-temporary-data-corruption-in-insert-range.patch --]
[-- Type: text/x-patch, Size: 2199 bytes --]

From b044b4dd604818efa3d7036d14b9750e3deb9bf3 Mon Sep 17 00:00:00 2001
From: David Howells via samba-technical <samba-technical@lists.samba.org>
Date: Tue, 23 Aug 2022 14:07:55 +0100
Subject: [PATCH] smb3: fix temporary data corruption in insert range

insert range doesn't discard the affected cached region
so can risk temporarily corrupting file data.

Also includes some minor cleanup (avoiding rereading
inode size repeatedly unnecessarily) to make it clearer.

Cc: stable@vger.kernel.org
Fixes: 7fe6fe95b9360 ("cifs: FALLOC_FL_INSERT_RANGE support")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/smb2ops.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 5b5ddc1b4638..7c941ce1e7a9 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -3722,35 +3722,43 @@ static long smb3_insert_range(struct file *file, struct cifs_tcon *tcon,
 	struct cifsFileInfo *cfile = file->private_data;
 	struct inode *inode = file_inode(file);
 	__le64 eof;
-	__u64  count;
+	__u64  count, old_eof;
 
 	xid = get_xid();
 
-	if (off >= i_size_read(inode)) {
+	inode_lock(inode);
+
+	old_eof = i_size_read(inode);
+	if (off >= old_eof) {
 		rc = -EINVAL;
 		goto out;
 	}
 
-	count = i_size_read(inode) - off;
-	eof = cpu_to_le64(i_size_read(inode) + len);
+	count = old_eof - off;
+	eof = cpu_to_le64(old_eof + len);
 
+	filemap_invalidate_lock(inode->i_mapping);
 	filemap_write_and_wait(inode->i_mapping);
+	truncate_pagecache_range(inode, off, old_eof);
 
 	rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
 			  cfile->fid.volatile_fid, cfile->pid, &eof);
 	if (rc < 0)
-		goto out;
+		goto out_2;
 
 	rc = smb2_copychunk_range(xid, cfile, cfile, off, count, off + len);
 	if (rc < 0)
-		goto out;
+		goto out_2;
 
-	rc = smb3_zero_range(file, tcon, off, len, 1);
+	rc = smb3_zero_data(file, tcon, off, len, xid);
 	if (rc < 0)
-		goto out;
+		goto out_2;
 
 	rc = 0;
+out_2:
+	filemap_invalidate_unlock(inode->i_mapping);
  out:
+	inode_unlock(inode);
 	free_xid(xid);
 	return rc;
 }
-- 
2.34.1