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(-)
smb3 fallocate zero range was not grabbing the inode or filemap_invalidate
locks so could have race with pagemap reinstantiating the page.
Cc: [email protected]
Signed-off-by: David Howells <[email protected]>
Signed-off-by: Steve French <[email protected]>
---
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,
From: Steve French <[email protected]>
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: [email protected]
Fixes: 5476b5dd82c8b ("cifs: add support for FALLOC_FL_COLLAPSE_RANGE")
Reported-by: David Howells <[email protected]>
Tested-by: David Howells <[email protected]>
Reviewed-by: David Howells <[email protected]>
Signed-off-by: Steve French <[email protected]>
cc: Ronnie Sahlberg <[email protected]>
---
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;
}
smb3 fallocate punch hole was not grabbing the inode or filemap_invalidate
locks so could have race with pagemap reinstantiating the page.
Cc: [email protected]
Signed-off-by: David Howells <[email protected]>
Signed-off-by: Steve French <[email protected]>
---
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;
}
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: [email protected]
Fixes: 7fe6fe95b9360 ("cifs: FALLOC_FL_INSERT_RANGE support")
Signed-off-by: David Howells <[email protected]>
Signed-off-by: Steve French <[email protected]>
cc: Ronnie Sahlberg <[email protected]>
---
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;
}
Matthew Wilcox <[email protected]> wrote:
> truncate_pagecache_range(inode, start, end);
>
> ... and presumably, you'd also want the error check?
truncate_pagecache_range() is void.
David
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);
Matthew Wilcox <[email protected]> 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
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
<[email protected]> 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: [email protected]
> Fixes: 7fe6fe95b9360 ("cifs: FALLOC_FL_INSERT_RANGE support")
> Signed-off-by: David Howells <[email protected]>
> Signed-off-by: Steve French <[email protected]>
> cc: Ronnie Sahlberg <[email protected]>
> ---
>
> 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