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

Commit Message

David Howells Aug. 23, 2022, 1:07 p.m. UTC

From: Steve French <[e-mail]>

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: [e-mail]
Fixes: 5476b5dd82c8b ("cifs: add support for FALLOC_FL_COLLAPSE_RANGE")
Reported-by: David Howells <[e-mail]>
Tested-by: David Howells <[e-mail]>
Reviewed-by: David Howells <[e-mail]>
Signed-off-by: Steve French <[e-mail]>
cc: Ronnie Sahlberg <[e-mail]>
---

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

Comments

Matthew Wilcox Aug. 23, 2022, 2:07 p.m. UTC | #1

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);

David Howells Aug. 23, 2022, 2:14 p.m. UTC | #2

Matthew Wilcox <[e-mail]> wrote:

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

truncate_pagecache_range() is void.

David

David Howells Aug. 23, 2022, 2:17 p.m. UTC | #3

Matthew Wilcox <[e-mail]> 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

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;
 }