[PATCH v2 2/2] cifs: prevent readdir from changing file size due to stale directory metadata

[Frank Sorenson <sorenson@redhat.com>]

Windows Server's directory enumeration metadata (EndOfFile) lags behind
the actual file size immediately after a write and close.  If a concurrent
readdir() runs in the window between close() returning to userspace and
stat() being called, it receives a stale size from the server's directory
listing and overwrites the correct cached i_size.  A subsequent stat()
within the actimeo window then returns this incorrect value.

The actual race sequence:

  1. Thread A: write N bytes to file, then close()
  2. cifs_close() calls cifsFileInfo_put(), removing the handle from
     openFileList -- is_inode_writable() now returns false
  3. Thread B: readdir() runs, server returns stale EndOfFile=0 in the
     directory enumeration response
  4. is_size_safe_to_change() returns true (no writable handles, no RW
     lease), so cifs_fattr_to_inode() overwrites i_size with 0
  5. smb2_close_getattr() completes and stamps cifs_i->time = jiffies,
     marking the (now corrupt) cache as valid
  6. Thread A: stat() finds the cache valid and returns i_size=0

The existing is_size_safe_to_change() check blocks stale size updates
from readdir while an active RW lease is held, but does not cover the
window after the last writable handle is closed.  This is a
cross-syscall-boundary race: kernel locking alone cannot prevent it
because the stale data arrives from the server after close() has
returned.

Fix this by tracking the time of the last writable close or truncate in
a new cifsInodeInfo->time_last_write field.  When readdir attempts to
change i_size, is_size_safe_to_change() now first checks whether we are
still within acregmax jiffies of the last local write.  If so, the
update is blocked regardless of whether any file handles remain open.

When the size update is blocked and the server-reported size differs from
the locally cached value, cifs_i->time is set to zero.  This invalidates
the attribute cache so that the next stat() issues a fresh QUERY_INFO
to the server, which returns the authoritative size from the server's
open-file table rather than the stale directory enumeration metadata.
This is the same mechanism used by actimeo=0, which prevents the bug
entirely by bypassing the attribute cache on every stat().

Additionally, when the file has writable handles open or holds an active
RW lease, readdir is now unconditionally blocked from changing i_size
(previously it could grow i_size from readdir data in those cases).
With writable handles or an exclusive lease, the client is the
authoritative source for the file's size and readdir data is unreliable.

time_last_write is also set in the setattr truncation paths so that
truncate() followed by write()+close()+stat() is protected by the same
mechanism.

Reproducer: concurrent write+close+stat and readdir with 2 or more
threads against a Windows Server share.  The bug does not reproduce
against Samba (no directory metadata lag) or with actimeo=0 (attribute
cache bypassed).

Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/cifsfs.c   |  1 +
 fs/smb/client/cifsglob.h |  1 +
 fs/smb/client/file.c     | 24 +++++++++++++++++++++---
 fs/smb/client/inode.c    |  4 ++++
 4 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
index 4df6ca03a8de..505b9ef1a08a 100644
--- a/fs/smb/client/cifsfs.c
+++ b/fs/smb/client/cifsfs.c
@@ -440,6 +440,7 @@ cifs_alloc_inode(struct super_block *sb)
 		return NULL;
 	cifs_inode->cifsAttrs = ATTR_ARCHIVE;	/* default */
 	cifs_inode->time = 0;
+	cifs_inode->time_last_write = 0;
 	/*
 	 * Until the file is open and we have gotten oplock info back from the
 	 * server, can not assume caching of file data or metadata.
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 08e94633a9c1..bf8dc8d16fae 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1566,6 +1566,7 @@ struct cifsInodeInfo {
 	spinlock_t writers_lock;
 	unsigned int writers;		/* Number of writers on this inode */
 	unsigned long time;		/* jiffies of last update of inode */
+	unsigned long time_last_write;	/* jiffies of last local write/close */
 	u64  uniqueid;			/* server inode number */
 	u64  createtime;		/* creation time on server */
 	__u8 lease_key[SMB2_LEASE_KEY_SIZE];	/* lease key for this inode */
diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index 968740e7c9c3..84ecbca01d69 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -1452,6 +1452,9 @@ int cifs_close(struct inode *inode, struct file *file)
 	struct cifs_deferred_close *dclose;
 	struct cifs_tcon *tcon;

+	if (file->f_mode & FMODE_WRITE)
+		cinode->time_last_write = jiffies;
+
 	cifs_fscache_unuse_inode_cookie(inode, file->f_mode & FMODE_WRITE);

 	if (file->private_data != NULL) {
@@ -3225,13 +3228,21 @@ static int is_inode_writable(struct cifsInodeInfo *cifs_inode)
 bool is_size_safe_to_change(struct cifsInodeInfo *cifsInode, __u64 end_of_file,
 			    bool from_readdir)
 {
+	struct cifs_sb_info *cifs_sb;
+
 	if (!cifsInode)
 		return true;

+	cifs_sb = CIFS_SB(cifsInode);
+
+	if (from_readdir) {
+		if (time_before(jiffies, cifsInode->time_last_write + cifs_sb->ctx->acregmax))
+			return false;
+	}
+
 	if (is_inode_writable(cifsInode) ||
 		((cifsInode->oplock & CIFS_CACHE_RW_FLG) != 0 && from_readdir)) {
 		/* This inode is open for write at least once */
-		struct cifs_sb_info *cifs_sb = CIFS_SB(cifsInode);

 		if (cifs_sb_flags(cifs_sb) & CIFS_MOUNT_DIRECT_IO) {
 			/* since no page cache to corrupt on directio
@@ -3239,12 +3250,18 @@ bool is_size_safe_to_change(struct cifsInodeInfo *cifsInode, __u64 end_of_file,
 			return true;
 		}

+		/* Readdir data is unreliable when we have writable handles or
+		 * an exclusive lease -- never allow it to change i_size. */
+		if (from_readdir)
+			return false;
+
 		if (i_size_read(&cifsInode->netfs.inode) < end_of_file)
 			return true;

 		return false;
-	} else
-		return true;
+	}
+
+	return true;
 }

 void cifs_oplock_break(struct work_struct *work)
diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index e75138f5f6bc..026382b7d4fe 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -237,6 +237,8 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr,
 	if (is_size_safe_to_change(cifs_i, fattr->cf_eof, from_readdir)) {
 		i_size_write(inode, fattr->cf_eof);
 		inode->i_blocks = CIFS_INO_BLOCKS(fattr->cf_bytes);
+	} else if (from_readdir && i_size_read(inode) != fattr->cf_eof) {
+		cifs_i->time = 0;
 	}

 	if (S_ISLNK(fattr->cf_mode) && fattr->cf_symlink_target) {
@@ -3280,6 +3282,7 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
 		truncate_setsize(inode, attrs->ia_size);
 		netfs_resize_file(&cifsInode->netfs, attrs->ia_size, true);
 		fscache_resize_cookie(cifs_inode_cookie(inode), attrs->ia_size);
+		cifsInode->time_last_write = jiffies;
 	}

 	setattr_copy(&nop_mnt_idmap, inode, attrs);
@@ -3481,6 +3484,7 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
 		truncate_setsize(inode, attrs->ia_size);
 		netfs_resize_file(&cifsInode->netfs, attrs->ia_size, true);
 		fscache_resize_cookie(cifs_inode_cookie(inode), attrs->ia_size);
+		cifsInode->time_last_write = jiffies;
 	}

 	setattr_copy(&nop_mnt_idmap, inode, attrs);
--
2.55.0