Skip to contentSkip to conversation

netfs, cifs: data corruption fixes

PATCH Superseded Paulo Alcantara 2 participants 17 messages Sep 28 – Sep 29

Subsystems (5)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Paulo Alcantara

author maintainer
#0 lore raw

cover netfs, cifs: data corruption fixes

This series fixes a number of data corruption and spurious I/O error bugs found by running generic/363 (fsx) in a loop against Windows Server 2022 and Samba 4.24 servers.

* Post-EOF pagecache poisoning on extend (1-5)

Data dirtied past EOF through an mmapped region was never discarded, so it reappeared as file content once the file was extended by an ordinary write, a truncate, a zero range, a copy range or a clone range. pagecache_isize_extended() doesn't help here: it's a no-op on cifs because i_blkbits is 14. Only the one folio straddling the old EOF can hold such data, since pages wholly beyond EOF can't be faulted in, so each extending path now zeroes or discards that folio.

* Missing flush or drain before trusting server or pagecache state (6-9)

FSCTL_QUERY_ALLOCATED_RANGES can report just-written data as a hole unless it has been committed to disk first, and the O_TRUNC open, interior zero range and server-side copy/clone paths did not flush dirty data or drain in-flight I/O before an operation that assumes the pagecache and the server agree on the file's contents.

* fallocate refused without a read lease (10, 12)

smb3_zero_range() and smb3_simple_falloc() returned -EOPNOTSUPP for any size-extending request whenever the inode wasn't read caching, since the cached i_size couldn't be trusted. Query the server's EOF in that case instead of refusing the request outright.

* Short reads leaving stale data behind (11, 13, 14)

The read-gaps path and the DIO/unbuffered read collector left the untransferred tail of a short read untouched, and cifs could not tell a genuine EOF from a stale cached remote_i_size after a lease downgrade. A read racing an extending write could come back short and, in the read-gaps case, have that stale folio content written back to the server.

* Unstable pages during a signed write (15)

cifs signs the pagecache folios in place before handing them to the socket. A buffered write could modify a folio while a write subrequest built from it was still in flight, so the signature no longer matched the data that followed it; the server answered STATUS_ACCESS_DENIED, which surfaced later as -EIO.

Paulo Alcantara (15):
netfs: clear post-EOF pagecache when extending a file via write
smb: client: clear post-EOF pagecache when extending a file via
truncate
smb: client: discard post-EOF pagecache when extending a file via zero
range
smb: client: discard post-EOF pagecache when extending a file via copy
range
smb: client: discard post-EOF pagecache when extending a file via
clone range
smb: client: flush and commit data before querying allocated ranges
smb: client: drain outstanding I/O before truncating on O_TRUNC open
smb: client: flush dirty data before zeroing a range
smb: client: drain and invalidate before server-side copy/clone
smb: client: only require read lease for size-extending zero range
netfs: zero gaps in read-gaps folio to avoid writing back stale data
smb: client: only require read lease for size-extending preallocate
netfs: zero the tail of a short DIO/unbuffered read
smb: client: distinguish real EOF from a stale remote_i_size on read
smb: client: require stable pages for signed connections

 Documentation/filesystems/netfs_library.rst | 25 ++++++
 fs/netfs/buffered_read.c                    |  7 ++
 fs/netfs/buffered_write.c                   |  9 ++
 fs/netfs/direct_write.c                     |  9 ++
 fs/netfs/misc.c                             | 71 +++++++++++++++
 fs/netfs/read_collect.c                     | 24 +++++
 fs/smb/client/cifsfs.c                      | 25 +++++-
 fs/smb/client/file.c                        |  2 +
 fs/smb/client/inode.c                       | 19 ++--
 fs/smb/client/smb2ops.c                     | 97 ++++++++++++++++-----
 fs/smb/client/smb2pdu.c                     | 10 ++-
 include/linux/netfs.h                       |  2 +
 12 files changed, 267 insertions(+), 33 deletions(-)
Signature · 1 line
2.55.0
author maintainer
#1 lore raw

PATCH 1/15 netfs: clear post-EOF pagecache when extending a file via write

Fix netfs to erase the contents of a hole created after the EOF by an ordinary write if dirty data has been previously left there by writes through an mmapped region. Neither the buffered nor the unbuffered/DIO write path clears that stale pagecache.

Zero the tail of the folio straddling the EOF before an extending write. That is the only folio that can hold data written past the EOF through an mmap, as pages wholly beyond the EOF can't be faulted in. The folio is zeroed rather than dropped so a concurrent extending write can't lose data.

Both write paths downgrade the i_rwsem to shared, so extending writes can run concurrently and the i_size read by the caller may be stale by the time the folio is locked. Re-read i_size under the folio lock and clamp the zeroed range up to it, so a racing write that already put data into the folio isn't clobbered.

Wait for any writeback on the folio to finish before zeroing it so that the pagecache isn't modified while it may still be read by the transport during transmission. Honour IOCB_NOWAIT by returning -EAGAIN rather than blocking on the folio lock, on writeback, or in folio_mkclean()'s rmap walk when the folio is mapped.

truncate_pagecache() can't be used here: it must be called with the i_rwsem held exclusively, but these write paths only hold it shared, and it would block unconditionally, breaking IOCB_NOWAIT.

Export the helper so filesystems can clear the same stale data from their own truncate paths.

This helper is required by the CIFS client to fix generic/363.

Fixes 938e13a73b24 ("netfs: Implement buffered write API")
Fixes 153a9961b551 ("netfs: Implement unbuffered/DIO write support")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 Documentation/filesystems/netfs_library.rst | 25 ++++++++
 fs/netfs/buffered_write.c                   |  9 +++
 fs/netfs/direct_write.c                     |  9 +++
 fs/netfs/misc.c                             | 71 +++++++++++++++++++++
 include/linux/netfs.h                       |  2 +
 5 files changed, 116 insertions(+)
Documentation/filesystems/netfs_library.rst +25 -0
index ddd799df6ce3..f2f22076168c 100644
@@ -451,6 +451,31 @@ one.
 The inode should be marked ``NETFS_ICTX_SINGLE_NO_UPLOAD`` if this API is to be
 used.  The writeback function requires the buffer to be of ITER_FOLIOQ type.
 
+Clearing Stale Post-EOF Pagecache
+---------------------------------
+
+When a file is extended, data left in the pagecache past the old EOF by a write
+through an mmap must not be exposed as file content.  Netfslib clears this on
+its own write paths, and exports a helper so a filesystem can do the same from,
+for instance, its truncate or fallocate paths::
+
+	int netfs_clear_stale_post_eof(struct inode *inode, uoff_t from,
+				       uoff_t to, bool nowait);
+
+This zeroes any such data within the ``[from, to)`` hole to be made, where @from
+is the current EOF.  Only the folio straddling @from can hold data written past
+the EOF through an mmap, as pages wholly beyond the EOF can't be faulted in, so
+the zeroing is limited to that folio and clamped to the top of the hole.  The
+folio is zeroed rather than dropped so that a concurrent extending write can't
+lose data.  If @nowait is set, it returns ``-EAGAIN`` rather than blocking.
+
+This overlaps with ``pagecache_isize_extended()`` but can't reuse it: that
+helper is keyed on a sub-page block size and is a no-op when the block size is
+``>= PAGE_SIZE`` (as on network filesystems), it runs after ``i_size`` has been
+updated, it can't honour a non-blocking caller, and it doesn't wait for
+writeback.  As both address the same problem, a change to one should probably be
+reflected in the other to keep them in sync.
+
 High-Level VM API
 ==================
 
fs/netfs/buffered_write.c +9 -0
index 2cdb68e6b16f..cc0561f77bd0 100644
@@ -469,6 +469,7 @@ ssize_t netfs_buffered_write_iter_locked(struct kiocb *iocb, struct iov_iter *fr
 					 struct netfs_group *netfs_group)
 {
 	struct file *file = iocb->ki_filp;
+	struct inode *inode = file_inode(file);
 	ssize_t ret;
 
 	trace_netfs_write_iter(iocb, from);
@@ -481,6 +482,14 @@ ssize_t netfs_buffered_write_iter_locked(struct kiocb *iocb, struct iov_iter *fr
 	if (ret)
 		return ret;
 
+	if (iocb->ki_pos > i_size_read(inode)) {
+		ret = netfs_clear_stale_post_eof(inode, i_size_read(inode),
+						 iocb->ki_pos,
+						 iocb->ki_flags & IOCB_NOWAIT);
+		if (ret)
+			return ret;
+	}
+
 	return netfs_perform_write(iocb, from, netfs_group);
 }
 EXPORT_SYMBOL(netfs_buffered_write_iter_locked);
fs/netfs/direct_write.c +9 -0
index 2361277416c7..36c36bc6c9d2 100644
@@ -359,6 +359,15 @@ ssize_t netfs_unbuffered_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	ret = file_update_time(file);
 	if (ret < 0)
 		goto out;
+
+	if (iocb->ki_pos > i_size_read(inode)) {
+		ret = netfs_clear_stale_post_eof(inode, i_size_read(inode),
+						 iocb->ki_pos,
+						 iocb->ki_flags & IOCB_NOWAIT);
+		if (ret < 0)
+			goto out;
+	}
+
 	if (iocb->ki_flags & IOCB_NOWAIT) {
 		/* We could block if there are any pages in the range. */
 		ret = -EAGAIN;
fs/netfs/misc.c +71 -0

80 lines not on this page · raw message

include/linux/netfs.h +2 -0
index b4dd32863dd4..c7b2ac404853 100644
@@ -397,6 +397,8 @@ ssize_t netfs_unbuffered_write_iter(struct kiocb *iocb, struct iov_iter *from);
 ssize_t netfs_unbuffered_write_iter_locked(struct kiocb *iocb, struct iov_iter *iter,
 					   struct netfs_group *netfs_group);
 ssize_t netfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from);
+int netfs_clear_stale_post_eof(struct inode *inode, uoff_t from,
+			       uoff_t to, bool nowait);
 
 /* Single, monolithic object read/write API. */
 void netfs_single_mark_inode_dirty(struct inode *inode);
Signature · 1 line
2.55.0
author maintainer
#2 lore raw

PATCH 2/15 smb: client: clear post-EOF pagecache when extending a file via truncate

In reply to Paulo Alcantara

cifs_setsize() relied on pagecache_isize_extended() to zero the tail of the folio straddling the old EOF, but that helper is a no-op on CIFS (i_blkbits is 14), so data dirtied past EOF through an mmap survived and became visible once the file was extended.

When extending, zero the tail of the folio straddling the old EOF with netfs_clear_stale_post_eof() instead, mirroring what pagecache_isize_extended() does for filesystems with a sub-page block size. Do this before updating i_size so that the helper, which clamps its zeroing to the current EOF, zeroes the whole [old_size, offset) hole. truncate_pagecache() is then called as in truncate_setsize(): it is essentially a no-op on extend and drops the pagecache beyond the new EOF on shrink.

Fixes c510edb9734a ("cifs: call pagecache_isize_extended() in cifs_setsize() when extending")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/smb/client/inode.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
fs/smb/client/inode.c +11 -5
index 1fe0ef0a95db..ebc620c166a9 100644
@@ -3055,13 +3055,20 @@ int cifs_fiemap(struct inode *inode, struct fiemap_extent_info *fei, u64 start,
 
 void cifs_setsize(struct inode *inode, loff_t offset)
 {
-	loff_t old_size;
+	loff_t old_size = i_size_read(inode);
 	u64 blocks = CIFS_INO_BLOCKS(offset);
 
+	/*
+	 * When extending, zero the tail of the folio straddling the old EOF
+	 * (before updating i_size) so data dirtied past EOF through an mmap
+	 * isn't exposed.  truncate_pagecache() then drops any pagecache beyond
+	 * the new EOF, as in truncate_setsize().
+	 */
+	if (offset > old_size)
+		netfs_clear_stale_post_eof(inode, old_size, offset, false);
+
 	spin_lock(&inode->i_lock);
-	old_size = i_size_read(inode);
 	i_size_write(inode, offset);
-
 	/*
 	 * Extending EOF does not allocate the intervening range. Only clamp
 	 * i_blocks on shrink; allocation growth comes from writes or from the
@@ -3071,8 +3078,7 @@ void cifs_setsize(struct inode *inode, loff_t offset)
 		inode->i_blocks = blocks;
 	spin_unlock(&inode->i_lock);
 	inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
-	if (offset > old_size)
-		pagecache_isize_extended(inode, old_size, offset);
+
 	truncate_pagecache(inode, offset);
 	netfs_wait_for_outstanding_io(inode);
 }
Signature · 1 line
2.55.0
author maintainer
#3 lore raw

PATCH 3/15 smb: client: discard post-EOF pagecache when extending a file via zero range

In reply to Paulo Alcantara

smb3_zero_range() removed the pagecache only for the range being zeroed, so when it extends the file the folio straddling the old EOF - which may hold data dirtied past EOF through an mmap - was never dropped and became visible as file content once the file was extended.

Lower the discard boundary to the smaller of the zero-range offset and the old EOF.

Fixes 72c419d9b073 ("cifs: fix smb3_zero_range so it can expand the file-size when required")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/smb/client/smb2ops.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/smb/client/smb2ops.c +4 -1
index aa142420dae2..e55e081e47b9 100644
@@ -3558,8 +3558,11 @@ static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
 	/*
 	 * We zero the range through ioctl, so we need remove the page caches
 	 * first, otherwise the data may be inconsistent with the server.
+	 *
+	 * Start at the old EOF when extending so the folio straddling it, which
+	 * may hold data written past EOF through an mmap, is dropped too.
 	 */
-	truncate_pagecache_range(inode, offset, offset + len - 1);
+	truncate_pagecache_range(inode, min(offset, i_size), offset + len - 1);
 	netfs_wait_for_outstanding_io(inode);
 
 	/* if file not oplocked can't be sure whether asking to extend size */
Signature · 1 line
2.55.0
author maintainer
#4 lore raw

PATCH 4/15 smb: client: discard post-EOF pagecache when extending a file via copy range

In reply to Paulo Alcantara

cifs_file_copychunk_range() invalidated the pagecache only for the destination region, so when a copy extends the file the folio straddling the old EOF - which may hold data dirtied past EOF through an mmap - was never dropped and became visible as file content once the file was extended.

Start the invalidation at the smaller of the destination offset and the old EOF. filemap_invalidate_inode() writes back before invalidating while i_size is still old, so nothing past the old EOF is flushed to the server.

Fixes 8101d6e112e2 ("cifs: Fix copy offload to flush destination region")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/smb/client/cifsfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/smb/client/cifsfs.c +6 -1
index 7ecd70efdfea..f1fb4b7b7b7c 100644
@@ -1581,8 +1581,13 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 	/* Flush and invalidate all the folios in the destination region.  If
 	 * the copy was successful, then some of the flush is extra overhead,
 	 * but we need to allow for the copy failing in some way (eg. ENOSPC).
+	 *
+	 * Start at the old EOF when extending so the folio straddling it, which
+	 * may hold data written past EOF through an mmap, is dropped too.
 	 */
-	rc = filemap_invalidate_inode(target_inode, true, destoff, destoff + len - 1);
+	rc = filemap_invalidate_inode(target_inode, true,
+				      min(destoff, i_size_read(target_inode)),
+				      destoff + len - 1);
 	if (rc)
 		goto unlock;
 
Signature · 1 line
2.55.0
author maintainer
#5 lore raw

PATCH 5/15 smb: client: discard post-EOF pagecache when extending a file via clone range

In reply to Paulo Alcantara

cifs_remap_file_range() discarded the pagecache only for the folios overlapping the destination region, so when a clone extends the file the folio straddling the old EOF - which may hold data dirtied past EOF through an mmap - was never dropped and became visible as file content once the file was extended.

Start the discard at the smaller of fstart and the old EOF. i_size is still old here, so nothing past the old EOF is flushed to the server.

Fixes c54fc3a4f375 ("cifs: Fix flushing, invalidation and file size with FICLONE")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/smb/client/cifsfs.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
fs/smb/client/cifsfs.c +7 -2
index f1fb4b7b7b7c..6410249f529a 100644
@@ -1467,9 +1467,14 @@ static loff_t cifs_remap_file_range(struct file *src_file, loff_t off,
 	i_size = target_inode->i_size;
 	spin_unlock(&target_inode->i_lock);
 
-	/* Discard all the folios that overlap the destination region. */
+	/*
+	 * Discard all the folios that overlap the destination region.  Start at
+	 * the old EOF when extending so the folio straddling it, which may hold
+	 * data written past EOF through an mmap, is dropped too.
+	 */
 	cifs_dbg(FYI, "about to discard pages %llx-%llx\n", fstart, fend);
-	truncate_inode_pages_range(&target_inode->i_data, fstart, fend);
+	truncate_inode_pages_range(&target_inode->i_data,
+				   min(fstart, i_size), fend);
 
 	fscache_invalidate(cifs_inode_cookie(target_inode), NULL, i_size, 0);
 
Signature · 1 line
2.55.0
author maintainer
#6 lore raw

PATCH 6/15 smb: client: flush and commit data before querying allocated ranges

In reply to Paulo Alcantara

The mode 0 / FALLOC_FL_KEEP_SIZE fallocate emulation for small interior ranges in smb3_simple_fallocate_range() issues FSCTL_QUERY_ALLOCATED_RANGES and zero-fills any sub-range the server reports as an unallocated hole, to force block allocation without changing file contents.

This is unsafe against Windows servers. Per the Win32 documentation, FSCTL_QUERY_ALLOCATED_RANGES only reports ranges that *may* contain nonzero data and is explicitly not coherent with recently written data:

"a call to FSCTL_QUERY_ALLOCATED_RANGES [after writing to a network
file] would not necessarily return a correct list of allocated regions. To ensure coherency [...] flush the data to the file"

fsx (generic/363) reproduces the resulting corruption against Windows Server 2022: a range is written, an uncached read returns the data, yet the next FSCTL_QUERY_ALLOCATED_RANGES on that range reports it as a whole hole, so the emulation overwrites live data with zeroes. A network trace confirmed the WRITE, the READ returning data and the query returning an empty range list within microseconds. Samba does not exhibit this; its query is coherent with the written data.

Write back the dirty pages, drain any outstanding I/O and issue an SMB2 FLUSH to force the server to commit the data before querying the allocated ranges, so the query reflects the data actually present.

With this, generic/363 passes against both Windows Server 2022 and Samba over 100000 fsx operations.

Fixes 966a3cb7c7db ("cifs: improve fallocate emulation")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/smb/client/smb2ops.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)
fs/smb/client/smb2ops.c +26 -6
index e55e081e47b9..dbab69d1b28a 100644
@@ -3748,6 +3748,24 @@ static int smb3_simple_fallocate_range(unsigned int xid,
 		goto out;
 	}
 
+	filemap_invalidate_lock(inode->i_mapping);
+
+	/*
+	 * Flush and commit the data to the server, otherwise
+	 * FSCTL_QUERY_ALLOCATED_RANGES might report recently written data as
+	 * unallocated holes on Windows Servers, and the loop below would
+	 * then zero-fill them and corrupt the file.
+	 */
+	rc = filemap_write_and_wait_range(inode->i_mapping, off,
+					  off + len - 1);
+	if (rc)
+		goto out_unlock;
+	netfs_wait_for_outstanding_io(inode);
+	rc = SMB2_flush(xid, tcon, cfile->fid.persistent_fid,
+			cfile->fid.volatile_fid);
+	if (rc)
+		goto out_unlock;
+
 	in_data.file_offset = cpu_to_le64(off);
 	in_data.length = cpu_to_le64(len);
 	rc = SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
@@ -3757,7 +3775,7 @@ static int smb3_simple_fallocate_range(unsigned int xid,
 			1024 * sizeof(struct file_allocated_range_buffer),
 			(char **)&out_data, &out_data_len);
 	if (rc)
-		goto out;
+		goto out_unlock;
 
 	tmp_data = out_data;
 	while (len) {
@@ -3767,12 +3785,12 @@ static int smb3_simple_fallocate_range(unsigned int xid,
 		if (out_data_len == 0) {
 			rc = smb3_simple_fallocate_write_range(xid, tcon,
 					       cfile, off, len, buf);
-			goto out;
+			goto out_unlock;
 		}
 
 		if (out_data_len < sizeof(struct file_allocated_range_buffer)) {
 			rc = -EINVAL;
-			goto out;
+			goto out_unlock;
 		}
 
 		range_start = le64_to_cpu(tmp_data->file_offset);
@@ -3780,7 +3798,7 @@ static int smb3_simple_fallocate_range(unsigned int xid,
 		if (check_add_overflow(range_start, range_len, &range_end) ||
 		    range_end > S64_MAX) {
 			rc = -EINVAL;
-			goto out;
+			goto out_unlock;
 		}
 
 		if (off < range_start) {
@@ -3795,11 +3813,11 @@ static int smb3_simple_fallocate_range(unsigned int xid,
 			rc = smb3_simple_fallocate_write_range(xid, tcon,
 					       cfile, off, l, buf);
 			if (rc)
-				goto out;
+				goto out_unlock;
 			off = off + l;
 			len = len - l;
 			if (len == 0)
-				goto out;
+				goto out_unlock;
 		}
 		/*
 		 * We are at a section of allocated data, just skip forward
@@ -3818,6 +3836,8 @@ static int smb3_simple_fallocate_range(unsigned int xid,
 		out_data_len -= sizeof(struct file_allocated_range_buffer);
 	}
 
+ out_unlock:
+	filemap_invalidate_unlock(inode->i_mapping);
  out:
 	kfree(out_data);
 	kvfree(buf);
Signature · 1 line
2.55.0
author maintainer
#7 lore raw

PATCH 7/15 smb: client: drain outstanding I/O before truncating on O_TRUNC open

In reply to Paulo Alcantara

cifs_do_truncate() flushes the pagecache with filemap_write_and_wait() before resetting the file size to zero, but that does not wait for direct/async I/O that was already in flight, which can complete after the truncate and reinstate stale data past the new EOF.

Drain outstanding netfs I/O with netfs_wait_for_outstanding_io() before resizing the file.

Fixes a8603b52b39f ("smb: client: fix data corruption with concurrent writes and O_TRUNC")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/smb/client/file.c | 2 ++
 1 file changed, 2 insertions(+)
fs/smb/client/file.c +2 -0
index 0d428517f454..d3114cb7fbc5 100644
@@ -1012,6 +1012,8 @@ static int cifs_do_truncate(const unsigned int xid, struct dentry *dentry)
 	}
 	mapping_set_error(inode->i_mapping, rc);
 
+	netfs_wait_for_outstanding_io(inode);
+
 	cfile = find_writable_file(cinode, FIND_FSUID_ONLY);
 	rc = cifs_file_flush(xid, inode, cfile);
 	if (!rc) {
Signature · 1 line
2.55.0
author maintainer
#8 lore raw

PATCH 8/15 smb: client: flush dirty data before zeroing a range

In reply to Paulo Alcantara

smb3_zero_range() emulates FALLOC_FL_ZERO_RANGE by invalidating the page cache over the target range with truncate_pagecache_range() and then issuing FSCTL_SET_ZERO_DATA to the server.

Dirty data was only flushed conditionally, when the range reached or extended EOF. For a purely interior zero range that does not reach EOF, no flush happened, so a dirty folio overlapping the range could be written back to the server after the FSCTL and refill the range that was just zeroed with stale data.

Fix this by unconditionally flushing and waiting for dirty data in the range before invalidating the page cache and issuing FSCTL_SET_ZERO_DATA, exactly as was done for smb3_punch_hole() in commit d7d2adcd022b ("smb/client: flush dirty data before punching a hole"); the two paths are structurally identical here.

This is observed as generic/363 randomly reading stale data where a zeroed range is expected against Windows Server.

Fixes 91d1dfae4649 ("cifs: Fix FALLOC_FL_ZERO_RANGE to preflush buffered part of target region")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/smb/client/smb2ops.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
fs/smb/client/smb2ops.c +4 -6
index dbab69d1b28a..6e1682dedb87 100644
@@ -3547,13 +3547,11 @@ static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
 	filemap_invalidate_lock(inode->i_mapping);
 
 	netfs_read_sizes(inode, &i_size, &remote_i_size, &zero_point);
-	if (offset + len >= remote_i_size && offset < i_size) {
-		unsigned long long top = umin(offset + len, i_size);
 
-		rc = filemap_write_and_wait_range(inode->i_mapping, offset, top - 1);
-		if (rc < 0)
-			goto zero_range_exit;
-	}
+	rc = filemap_write_and_wait_range(inode->i_mapping, offset,
+					  offset + len - 1);
+	if (rc < 0)
+		goto zero_range_exit;
 
 	/*
 	 * We zero the range through ioctl, so we need remove the page caches
Signature · 1 line
2.55.0
author maintainer
#9 lore raw

PATCH 9/15 smb: client: drain and invalidate before server-side copy/clone

In reply to Paulo Alcantara

cifs_file_copychunk_range() and the clone (FICLONE) path of cifs_remap_file_range() do not serialise the destination page cache against the server-side copy the way the other server-side range operations (smb3_zero_range(), smb3_punch_hole(), smb3_collapse_range() and smb3_insert_range()) do.

cifs_file_copychunk_range() invalidates the destination range with filemap_invalidate_inode(), which takes and drops the mapping's invalidate_lock internally, so the lock is no longer held when the copychunk ioctl is issued. It also never drains in-flight netfs I/O on the target. The clone path never takes the invalidate_lock at all (only i_rwsem), uses a bare truncate_inode_pages_range() and likewise does not drain outstanding I/O.

As a result an asynchronous destination writeback can complete after the server-side copy/clone has run and reinstate stale data over the region just written by the server, corrupting the file. This is the same class of corruption as commit d7d2adcd022b ("smb/client: flush dirty data before punching a hole") and has been seen randomly in generic/363 against Windows Server.

Fix both paths to follow the established ordering: hold the target mapping's invalidate_lock across the flush and invalidation of the destination and the server ioctl, and call netfs_wait_for_outstanding_io() on the target to drain in-flight writes before the ioctl is issued. Only the target inode's invalidate_lock is required, as the source is merely flushed and not invalidated; i_rwsem (already held via lock_two_nondirectories()) is acquired before the invalidate_lock, matching the VFS lock ordering.

Fixes 8101d6e112e2 ("cifs: Fix copy offload to flush destination region")
Fixes c54fc3a4f375 ("cifs: Fix flushing, invalidation and file size with FICLONE")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/smb/client/cifsfs.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
fs/smb/client/cifsfs.c +12 -3
index 6410249f529a..a650c8022d0f 100644
@@ -1412,6 +1412,7 @@ static loff_t cifs_remap_file_range(struct file *src_file, loff_t off,
 	 * server could even support copy of range where source = target
 	 */
 	lock_two_nondirectories(target_inode, src_inode);
+	filemap_invalidate_lock(target_inode->i_mapping);
 
 	if (len == 0) {
 		loff_t src_size = i_size_read(src_inode);
@@ -1475,6 +1476,7 @@ static loff_t cifs_remap_file_range(struct file *src_file, loff_t off,
 	cifs_dbg(FYI, "about to discard pages %llx-%llx\n", fstart, fend);
 	truncate_inode_pages_range(&target_inode->i_data,
 				   min(fstart, i_size), fend);
+	netfs_wait_for_outstanding_io(target_inode);
 
 	fscache_invalidate(cifs_inode_cookie(target_inode), NULL, i_size, 0);
 
@@ -1513,6 +1515,7 @@ static loff_t cifs_remap_file_range(struct file *src_file, loff_t off,
 	if (rc)
 		CIFS_I(target_inode)->time = 0;
 unlock:
+	filemap_invalidate_unlock(target_inode->i_mapping);
 	/* although unlocking in the reverse order from locking is not
 	   strictly necessary here it is a little cleaner to be consistent */
 	unlock_two_nondirectories(src_inode, target_inode);
@@ -1565,6 +1568,7 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 	 * server could even support copy of range where source = target
 	 */
 	lock_two_nondirectories(target_inode, src_inode);
+	filemap_invalidate_lock(target_inode->i_mapping);
 
 	cifs_dbg(FYI, "about to flush pages\n");
 
@@ -1590,11 +1594,15 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 	 * Start at the old EOF when extending so the folio straddling it, which
 	 * may hold data written past EOF through an mmap, is dropped too.
 	 */
-	rc = filemap_invalidate_inode(target_inode, true,
-				      min(destoff, i_size_read(target_inode)),
-				      destoff + len - 1);
+	rc = filemap_write_and_wait_range(target_inode->i_mapping,
+					  min(destoff, i_size_read(target_inode)),
+					  destoff + len - 1);
 	if (rc)
 		goto unlock;
+	invalidate_inode_pages2_range(target_inode->i_mapping,
+				      min(destoff, i_size_read(target_inode)) >> PAGE_SHIFT,
+				      (destoff + len - 1) >> PAGE_SHIFT);
+	netfs_wait_for_outstanding_io(target_inode);
 
 	fscache_invalidate(cifs_inode_cookie(target_inode), NULL,
 			   i_size_read(target_inode), 0);
@@ -1626,6 +1634,7 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 	CIFS_I(target_inode)->time = 0;
 
 unlock:
+	filemap_invalidate_unlock(target_inode->i_mapping);
 	/* although unlocking in the reverse order from locking is not
 	 * strictly necessary here it is a little cleaner to be consistent
 	 */
Signature · 1 line
2.55.0
author maintainer
#10 lore raw

PATCH 10/15 smb: client: only require read lease for size-extending zero range

In reply to Paulo Alcantara

smb3_zero_range() refuses any FALLOC_FL_ZERO_RANGE that clears FALLOC_FL_KEEP_SIZE with -EOPNOTSUPP whenever the inode is not read caching:

	/* if file not oplocked can't be sure whether asking to extend size */
	rc = -EOPNOTSUPP;
	if (keep_size == false && !CIFS_CACHE_READ(cifsi))
		goto zero_range_exit;

The read lease is only needed to trust the cached i_size when deciding whether the range extends the file. When it is not held, the size can instead be fetched from the server, which is authoritative, rather than refusing the request outright: query the server's end of file and take the larger of it and the cached size for the interior-vs-extend decision. The larger of the two is used because the server's end of file reflects another client's growth while the cached size reflects this client's own writes that may not have reached the server yet; using the server size alone would wrongly shrink the file when the range flush above left an extending write unwritten.

Rejecting interior ranges is observed as generic/363 randomly failing against Windows Server with

do_zero_range: fallocate: Operation not supported

fsx issues an interior, non-KEEP_SIZE zero range while the inode is transiently not read caching: the server had just downgraded the file's lease from RWH to RH after breaking the write caching, and the ensuing handle reopen/revalidation left CIFS_CACHE_READ momentarily clear. The range sat well within the server's end of file, so no extend was needed, yet the range was refused and fsx aborted. This keeps the emulation correct even when a genuine lease break from another client leaves the inode without read caching -- the case the -EOPNOTSUPP guard turned into a hard failure. The extra round trip only happens on the no-lease path; the common cached case is unchanged.

Fixes 30175628bf7f ("[SMB3] Enable fallocate -z support for SMB3 mounts")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/smb/client/smb2ops.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)
fs/smb/client/smb2ops.c +21 -5
index 6e1682dedb87..45d5c66f1ad9 100644
@@ -3520,6 +3520,20 @@ static long smb3_zero_data(struct file *file, struct cifs_tcon *tcon,
 			  0, NULL, NULL);
 }
 
+static long query_server_eof(const unsigned int xid, struct cifs_tcon *tcon,
+			     struct cifsFileInfo *cfile,
+			     unsigned long long *eof)
+{
+	struct smb2_file_all_info file_inf = {};
+	long rc;
+
+	rc = SMB2_query_info(xid, tcon, cfile->fid.persistent_fid,
+			     cfile->fid.volatile_fid, &file_inf);
+	if (!rc)
+		*eof = le64_to_cpu(file_inf.EndOfFile);
+	return rc;
+}
+
 static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
 			    unsigned long long offset, unsigned long long len,
 			    bool keep_size)
@@ -3563,10 +3577,12 @@ static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
 	truncate_pagecache_range(inode, min(offset, i_size), offset + len - 1);
 	netfs_wait_for_outstanding_io(inode);
 
-	/* if file not oplocked can't be sure whether asking to extend size */
-	rc = -EOPNOTSUPP;
-	if (keep_size == false && !CIFS_CACHE_READ(cifsi))
-		goto zero_range_exit;
+	if (!keep_size && !CIFS_CACHE_READ(cifsi)) {
+		rc = query_server_eof(xid, tcon, cfile, &remote_i_size);
+		if (rc)
+			goto zero_range_exit;
+		i_size = max(i_size, remote_i_size);
+	}
 
 	fscache_invalidate(cifs_inode_cookie(inode), NULL,
 			   i_size_read(inode), 0);
@@ -3578,7 +3594,7 @@ static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
 	/*
 	 * do we also need to change the size of the file?
 	 */
-	if (keep_size == false && (unsigned long long)i_size_read(inode) < new_size) {
+	if (!keep_size && i_size < new_size) {
 		rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
 				  cfile->fid.volatile_fid, cfile->pid, new_size);
 		if (rc >= 0) {
Signature · 1 line
2.55.0
author maintainer
#11 lore raw

PATCH 11/15 netfs: zero gaps in read-gaps folio to avoid writing back stale data

In reply to Paulo Alcantara

netfs_read_gaps() leaves gaps around a streaming write's dirty region unread on the server side, so a short/EOF response there left stale folio content that later got written back.

Zero it after the read completes, not before: netfs_wait_for_read() returns rreq->transferred once ret >= 0, so build an iterator over the same bvec array the read used, advance past ret, and zero the rest. The dirty region already maps to sink pages in that array rather than the real folio, so this can't touch it, and only the actual shortfall gets zeroed instead of the whole gap upfront.

Fixes ee4cdf7ba857 ("netfs: Speed up buffered reading")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/netfs/buffered_read.c | 7 +++++++
 1 file changed, 7 insertions(+)
fs/netfs/buffered_read.c +7 -0
index 105194de6e13..e6506942aeda 100644
@@ -541,6 +541,13 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 
 	ret = netfs_wait_for_read(rreq);
 	if (ret >= 0) {
+		if (ret < flen) {
+			struct iov_iter iter;
+
+			iov_iter_bvec(&iter, ITER_DEST, bvec, i, flen);
+			iov_iter_advance(&iter, ret);
+			iov_iter_zero(flen - ret, &iter);
+		}
 		if (group)
 			folio_change_private(folio, group);
 		else
Signature · 1 line
2.55.0
author maintainer
#12 lore raw

PATCH 12/15 smb: client: only require read lease for size-extending preallocate

In reply to Paulo Alcantara

smb3_simple_falloc() refuses any fallocate that clears FALLOC_FL_KEEP_SIZE with -EOPNOTSUPP whenever the inode is not read caching:

	/* if file not oplocked can't be sure whether asking to extend size */
	if (!CIFS_CACHE_READ(cifsi))
		if (!keep_size) {
			...
			return rc;
		}

As with smb3_zero_range(), the read lease is only needed to trust the cached size when deciding whether the request extends the file. When it is not held, the size can instead be fetched from the server, which is authoritative, rather than refusing the request outright: after flushing, query the server's end of file and take the larger of it and the cached size for the interior-vs-extend decision. The larger of the two is used because the server's end of file reflects another client's growth while the cached size reflects this client's own writes that may not have reached the server yet; using the server size alone would wrongly treat an interior request as extending when a range flush left an extending write unwritten.

Rejecting interior requests is observed as generic/363 randomly failing against Windows Server with

do_preallocate: fallocate: Operation not supported

fsx issues an interior, non-KEEP_SIZE preallocate while the inode is transiently not read caching: the server had just downgraded the file's lease from RWH to RH after breaking the write caching, and the ensuing handle reopen/revalidation left CIFS_CACHE_READ momentarily clear. The range sat within the server's end of file, so no extend was needed, yet it was refused and fsx aborted. This keeps the emulation correct even when a genuine lease break from another client leaves the inode without read caching -- the case the -EOPNOTSUPP guard turned into a hard failure. The extra flush and round trip only happen when both keep_size is false and no read lease is held; every other case is unchanged.

Fixes 9ccf3216238c ("Add support for original fallocate")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/smb/client/smb2ops.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)
fs/smb/client/smb2ops.c +19 -5
index 45d5c66f1ad9..c97d5a9db575 100644
@@ -3880,14 +3880,28 @@ static long smb3_simple_falloc(struct file *file, struct cifs_tcon *tcon,
 
 	trace_smb3_falloc_enter(xid, cfile->fid.persistent_fid, tcon->tid,
 				tcon->ses->Suid, off, len);
-	/* if file not oplocked can't be sure whether asking to extend size */
-	if (!CIFS_CACHE_READ(cifsi))
-		if (!keep_size) {
+
+	if (!keep_size && !CIFS_CACHE_READ(cifsi)) {
+		unsigned long long server_eof;
+
+		rc = filemap_write_and_wait(inode->i_mapping);
+		if (rc) {
 			trace_smb3_falloc_err(xid, cfile->fid.persistent_fid,
 				tcon->tid, tcon->ses->Suid, off, len, rc);
 			free_xid(xid);
 			return rc;
 		}
+		netfs_wait_for_outstanding_io(inode);
+
+		rc = query_server_eof(xid, tcon, cfile, &server_eof);
+		if (rc) {
+			trace_smb3_falloc_err(xid, cfile->fid.persistent_fid,
+				tcon->tid, tcon->ses->Suid, off, len, rc);
+			free_xid(xid);
+			return rc;
+		}
+		old_eof = max_t(loff_t, old_eof, server_eof);
+	}
 
 	/*
 	 * Extending the file
@@ -4013,7 +4027,7 @@ static long smb3_simple_falloc(struct file *file, struct cifs_tcon *tcon,
 		}
 	}
 
-	if ((keep_size == true) || (i_size_read(inode) >= off + len)) {
+	if (keep_size || old_eof >= off + len) {
 		/*
 		 * At this point, we are trying to fallocate an internal
 		 * regions of a sparse file. Since smb2 does not have a
@@ -4042,7 +4056,7 @@ static long smb3_simple_falloc(struct file *file, struct cifs_tcon *tcon,
 		 * ie potentially making a few extra pages at the beginning
 		 * or end of the file non-sparse via set_sparse is harmless.
 		 */
-		if ((off > 8192) || (off + len + 8192 < i_size_read(inode))) {
+		if (off > 8192 || off + len + 8192 < old_eof) {
 			rc = -EOPNOTSUPP;
 			goto out;
 		}
Signature · 1 line
2.55.0
author maintainer
#13 lore raw

PATCH 13/15 netfs: zero the tail of a short DIO/unbuffered read

In reply to Paulo Alcantara

The buffered read collector zero-fills the tail of a short read that stops below the inode's i_size (netfs_clear_unread()), so a read that races an extending write still returns the expected number of bytes. The non-buffered collector path does no such thing: it just records how much was transferred.

Add the same zero-fill for the non-buffered case, gated on
NETFS_SREQ_CLEAR_TAIL: a subreq's source sets that flag when a short result from it is known to be safe to treat as a hole, as opposed to NETFS_SREQ_HIT_EOF, which means the read genuinely ran off the end of the file and should be reported short as-is. Only CLEAR_TAIL should zero-fill here; a real EOF must stay a real short read.

No source currently sets CLEAR_TAIL on an unbuffered/DIO subrequest, so this is inert on its own -- a following change teaches cifs to set it in the one case that needs it.

Fixes e2d46f2ec332 ("netfs: Change the read result collector to only use one work item")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/netfs/read_collect.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
fs/netfs/read_collect.c +24 -0
index a94197ef0181..c5bfaf2c6f25 100644
@@ -33,6 +33,22 @@ static void netfs_clear_unread(struct netfs_io_subrequest *subreq)
 		__set_bit(NETFS_SREQ_HIT_EOF, &subreq->flags);
 }
 
+static void netfs_clear_unread_dio(struct netfs_io_subrequest *subreq)
+{
+	uoff_t pos = subreq->start + subreq->transferred;
+	struct netfs_io_request *rreq = subreq->rreq;
+	size_t fill;
+
+	if (pos >= rreq->i_size)
+		return;
+
+	fill = min_t(uoff_t, rreq->i_size - pos,
+		     subreq->len - subreq->transferred);
+
+	netfs_reset_iter(subreq);
+	subreq->transferred += iov_iter_zero(fill, &subreq->io_iter);
+}
+
 /*
  * Cancel the copy-to-cache mark on a folio.
  */
@@ -311,6 +327,14 @@ static void netfs_collect_read_results(struct netfs_io_request *rreq)
 			    test_bit(NETFS_SREQ_HIT_EOF, &front->flags))
 				netfs_read_unlock_folios(rreq, &notes);
 		} else {
+			if (!(notes & HIT_PENDING) &&
+			    front->error == 0 &&
+			    transferred < front->len &&
+			    test_bit(NETFS_SREQ_CLEAR_TAIL, &front->flags)) {
+				netfs_clear_unread_dio(front);
+				transferred = front->transferred;
+				trace_netfs_sreq(front, netfs_sreq_trace_clear);
+			}
 			stream->collected_to = front->start + transferred;
 			rreq->collected_to = stream->collected_to;
 		}
Signature · 1 line
2.55.0
author maintainer
#14 lore raw

PATCH 14/15 smb: client: distinguish real EOF from a stale remote_i_size on read

In reply to Paulo Alcantara

smb2_readv_callback() sets NETFS_SREQ_HIT_EOF whenever a short read lines up with netfs_read_remote_i_size(inode), the server's EOF as the client currently believes it. That belief can be stale: after a lease downgrade and handle reopen, the tracked remote_i_size can sit below the client's own i_size while an extending write hasn't reached the server yet. A read in that gap comes back short for a reason that has nothing to do with the file's real size, but was still marked HIT_EOF, and netfs reports a short read for it as-is.

Only treat it as real EOF when the position is also at or past the client's own i_size; otherwise mark it NETFS_SREQ_CLEAR_TAIL instead, which tells netfs the shortfall is safe to zero-fill rather than report as a short read.

This is what fsx (generic/363) sees as "short read: 0x0 bytes instead of 0x<n>" against a Windows server.

Fixes 1da29f2c39b6 ("netfs, cifs: Fix handling of short DIO read")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/smb/client/smb2pdu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
fs/smb/client/smb2pdu.c +8 -2
index 3d7ead36d1a0..cfe3c4b4abe7 100644
@@ -4796,14 +4796,20 @@ smb2_readv_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 				     rdata->got_bytes);
 
 	if (rdata->result == -ENODATA) {
-		__set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
 		rdata->result = 0;
+		if (rdata->subreq.start + rdata->subreq.transferred >= i_size_read(inode))
+			__set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
+		else
+			__set_bit(NETFS_SREQ_CLEAR_TAIL, &rdata->subreq.flags);
 	} else {
 		size_t trans = rdata->subreq.transferred + rdata->got_bytes;
 		if (trans < rdata->subreq.len &&
 		    rdata->subreq.start + trans >= netfs_read_remote_i_size(inode)) {
-			__set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
 			rdata->result = 0;
+			if (rdata->subreq.start + trans >= i_size_read(inode))
+				__set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
+			else
+				__set_bit(NETFS_SREQ_CLEAR_TAIL, &rdata->subreq.flags);
 		}
 		if (rdata->got_bytes)
 			__set_bit(NETFS_SREQ_MADE_PROGRESS, &rdata->subreq.flags);
Signature · 1 line
2.55.0
author maintainer
#15 lore raw

PATCH 15/15 smb: client: require stable pages for signed connections

In reply to Paulo Alcantara

When signing, cifs computes the SMB signature over the pagecache folios in place and then hands those same folios to the socket. If a buffered write mutates a folio while a write subrequest is still in flight, the signature no longer matches the data that follows it, the server rejects the write with STATUS_ACCESS_DENIED (-EACCES), and the error is latched in the mapping, so the next fsync()/fallocate() returns -EIO.

Mark the mapping for stable writes so netfs_perform_write() waits for writeback to complete before modifying an in-flight folio. This is only needed for in-place signing: encryption copies the data into a transform buffer before sending, and plaintext transfers neither sign nor checksum the data, so gate it on the same condition cifs uses to decide whether to sign a request.

Fixes 3ee1a1fc3981 ("cifs: Cut over to using netfslib")
Cc Christian Brauner <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Namjae Jeon <[email protected]>
Cc Ronnie Sahlberg <[email protected]>
Cc Shyam Prasad N <[email protected]>
Cc Tom Talpey <[email protected]>
Cc Bharath SM <[email protected]>
---
 fs/smb/client/inode.c | 3 +++
 1 file changed, 3 insertions(+)
fs/smb/client/inode.c +3 -0
index ebc620c166a9..30cd7c95dd3d 100644
@@ -88,6 +88,9 @@ static void cifs_set_ops(struct inode *inode)
 		else
 			inode->i_data.a_ops = &cifs_addr_ops;
 		mapping_set_large_folios(inode->i_mapping);
+		if (tcon->ses->server->sign &&
+		    !smb3_encryption_required(tcon))
+			mapping_set_stable_writes(inode->i_mapping);
 		break;
 	case S_IFDIR:
 		if (IS_AUTOMOUNT(inode)) {
Signature · 1 line
2.55.0
maintainer Reviewed-by on the v1 series
#16 lore raw
In reply to Paulo Alcantara
Quoted 59 lines · Paulo Alcantara

This series fixes a number of data corruption and spurious I/O error bugs found by running generic/363 (fsx) in a loop against Windows Server 2022 and Samba 4.24 servers.

* Post-EOF pagecache poisoning on extend (1-5)

Data dirtied past EOF through an mmapped region was never discarded, so it reappeared as file content once the file was extended by an ordinary write, a truncate, a zero range, a copy range or a clone range. pagecache_isize_extended() doesn't help here: it's a no-op on cifs because i_blkbits is 14. Only the one folio straddling the old EOF can hold such data, since pages wholly beyond EOF can't be faulted in, so each extending path now zeroes or discards that folio.

* Missing flush or drain before trusting server or pagecache state (6-9)

FSCTL_QUERY_ALLOCATED_RANGES can report just-written data as a hole unless it has been committed to disk first, and the O_TRUNC open, interior zero range and server-side copy/clone paths did not flush dirty data or drain in-flight I/O before an operation that assumes the pagecache and the server agree on the file's contents.

* fallocate refused without a read lease (10, 12)

smb3_zero_range() and smb3_simple_falloc() returned -EOPNOTSUPP for any size-extending request whenever the inode wasn't read caching, since the cached i_size couldn't be trusted. Query the server's EOF in that case instead of refusing the request outright.

* Short reads leaving stale data behind (11, 13, 14)

The read-gaps path and the DIO/unbuffered read collector left the untransferred tail of a short read untouched, and cifs could not tell a genuine EOF from a stale cached remote_i_size after a lease downgrade. A read racing an extending write could come back short and, in the read-gaps case, have that stale folio content written back to the server.

* Unstable pages during a signed write (15)

cifs signs the pagecache folios in place before handing them to the socket. A buffered write could modify a folio while a write subrequest built from it was still in flight, so the signature no longer matched the data that followed it; the server answered STATUS_ACCESS_DENIED, which surfaced later as -EIO.

Paulo Alcantara (15):
netfs: clear post-EOF pagecache when extending a file via write
smb: client: clear post-EOF pagecache when extending a file via
truncate
smb: client: discard post-EOF pagecache when extending a file via zero
range
smb: client: discard post-EOF pagecache when extending a file via copy
range
smb: client: discard post-EOF pagecache when extending a file via
clone range
smb: client: flush and commit data before querying allocated ranges
smb: client: drain outstanding I/O before truncating on O_TRUNC open

Quoted 8 lines

smb: client: flush dirty data before zeroing a range
smb: client: drain and invalidate before server-side copy/clone
smb: client: only require read lease for size-extending zero range
netfs: zero gaps in read-gaps folio to avoid writing back stale data
smb: client: only require read lease for size-extending preallocate
netfs: zero the tail of a short DIO/unbuffered read
smb: client: distinguish real EOF from a stale remote_i_size on read
smb: client: require stable pages for signed connections

Thanks!