[PATCH 0/2] cifs: follow-on fixes to time_last_write mechanism

[PATCH 0/2] cifs: follow-on fixes to time_last_write mechanism

[Frank Sorenson]

This series addresses two correctness issues in commit e8a8d54c2d50
("cifs: prevent readdir from changing file size due to stale directory
metadata"), which introduced the time_last_write field to protect cached
file sizes from stale Windows Server directory enumeration metadata.

Patch 1 consolidates the time_last_write stamp into _cifsFileInfo_put().
The original commit scattered the stamp across cifs_close(),
smb2_deferred_work_close(), and three deferred-close drain functions in
misc.c.  This missed the case where background I/O holds the final file
reference after userspace close() returns -- the stamp was anchored to
the earlier userspace-close time rather than the actual server close,
allowing the protection window to expire before the handle was removed
from openFileList.  Moving the stamp into _cifsFileInfo_put() under
open_file_lock provides a single canonical location that covers all close
paths and uses the spinlock's store-release/load-acquire pairing with
is_inode_writable() for correct memory ordering.

Patch 2 fixes the stamp placement in the setattr/truncate paths.
cifs_file_set_size() calls cifs_setsize() on success, which calls
i_size_write() and updates i_size.  The subsequent check
attrs->ia_size != i_size_read() therefore always evaluates false after a
successful cifs_file_set_size(), making the stamp dead code -- the
truncate path was completely unprotected.  The fix moves the stamp to
before the RPC call, guarded by attrs->ia_size != i_size_read() to
exclude no-op same-size ftruncate(2) calls.  On failure the stamp is
left in place rather than restored: restoring a stale snapshot (prev_tlw)
could silently erase a concurrent _cifsFileInfo_put() close stamp if that
close arrived between the READ_ONCE and the smp_store_release.  readdir
is suppressed until the stamp expires; stat() is unaffected because
cifs_revalidate_dentry_attr() uses from_readdir=false, bypassing the
time_last_write check entirely and always returning an authoritative
QUERY_INFO result.

Frank Sorenson (2):
  cifs: consolidate time_last_write stamp into _cifsFileInfo_put()
  cifs: fix time_last_write stamp placement in setattr/truncate paths

 fs/smb/client/file.c  | 37 ++++++++++++++++---------------------
 fs/smb/client/inode.c | 26 ++++++++++++++++++++++----
 fs/smb/client/misc.c  | 51 ++++++---------------------------------------------
 3 files changed, 44 insertions(+), 70 deletions(-)

--
2.55.0

[PATCH 1/2] cifs: consolidate time_last_write stamp into _cifsFileInfo_put()

[Frank Sorenson]

The time_last_write stamp was scattered across cifs_close(),
smb2_deferred_work_close(), and the three drain functions in misc.c.
This missed the case where background I/O holds the final reference
after userspace close() returns, and required explicit maintenance at
each close-path site.

Move the smp_store_release() into _cifsFileInfo_put(), immediately
before releasing open_file_lock.  This single location covers all
close paths unconditionally: normal close, background I/O dropping the
final reference, deferred close via timer or external drain.  The
spinlock's store-release/load-acquire pairing with is_inode_writable()
already provides the ordering guarantee documented in
is_size_safe_to_change().

Remove the now-redundant stamps from cifs_close(),
smb2_deferred_work_close(), and all six stamp sites in the misc.c
deferred-close drain functions.

Fixes: e8a8d54c2d50 ("cifs: prevent readdir from changing file size due to stale directory metadata")
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/file.c | 37 ++++++++++++++------------------
 fs/smb/client/misc.c | 51 ++++++--------------------------------------
 2 files changed, 22 insertions(+), 66 deletions(-)

diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index b279a44be729..ac89c1ba56b1 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -915,6 +915,14 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file,
 		cifs_set_oplock_level(cifsi, 0);
 	}
 
+	if (OPEN_FMODE(cifs_file->f_flags) & FMODE_WRITE) {
+		/* Stamp while open_file_lock is held; covers all close paths
+		 * including background I/O. Pairs with smp_load_acquire() in
+		 * is_size_safe_to_change().
+		 */
+		smp_store_release(&cifsi->time_last_write, jiffies);
+	}
+
 	spin_unlock(&cifsi->open_file_lock);
 	spin_unlock(&tcon->open_file_lock);
 
@@ -1429,15 +1437,6 @@ void smb2_deferred_work_close(struct work_struct *work)
 	cifs_del_deferred_close(cfile);
 	cfile->deferred_close_scheduled = false;
 	spin_unlock(&cinode->deferred_lock);
-	/*
-	 * Refresh time_last_write immediately before the actual server close
-	 * so the protection window is anchored to the real close time, not
-	 * the earlier userspace close time stored by cifs_close().
-	 */
-	if (OPEN_FMODE(cfile->f_flags) & FMODE_WRITE) {
-		/* Pairs with smp_load_acquire() in is_size_safe_to_change(). */
-		smp_store_release(&cinode->time_last_write, jiffies);
-	}
 	_cifsFileInfo_put(cfile, true, false);
 }
 
@@ -1467,10 +1466,6 @@ int cifs_close(struct inode *inode, struct file *file)
 	if (file->private_data != NULL) {
 		cfile = file->private_data;
 		file->private_data = NULL;
-		if (file->f_mode & FMODE_WRITE) {
-			/* Pairs with smp_load_acquire() in is_size_safe_to_change(). */
-			smp_store_release(&cinode->time_last_write, jiffies);
-		}
 		dclose = kmalloc_obj(struct cifs_deferred_close);
 		if ((cfile->status_file_deleted == false) &&
 		    (smb2_can_defer_close(inode, dclose))) {
@@ -3276,13 +3271,13 @@ bool is_size_safe_to_change(struct cifsInodeInfo *cifsInode, __u64 end_of_file,
 	 * No writable handles open. Check whether we are within the attribute
 	 * cache validity window of a recent local modification.
 	 *
-	 * For the close() path: cifs_close() calls smp_store_release() on
-	 * time_last_write before _cifsFileInfo_put() removes the handle under
-	 * open_file_lock. That spin_unlock() is a store-release that pairs
-	 * with the spin_lock() (load-acquire) in is_inode_writable() above,
-	 * so if is_inode_writable() returned false the smp_load_acquire()
-	 * below is guaranteed to observe any time_last_write update from a
-	 * concurrent close().
+	 * For the close() path: _cifsFileInfo_put() stamps time_last_write
+	 * (via smp_store_release()) before releasing open_file_lock. That
+	 * spin_unlock() is a store-release that pairs with the spin_lock()
+	 * (load-acquire) in is_inode_writable() above, so if
+	 * is_inode_writable() returned false the smp_load_acquire() below is
+	 * guaranteed to observe any time_last_write update from a concurrent
+	 * close(), covering all close paths including background I/O.
 	 *
 	 * For the setattr/truncate paths: those callers use smp_store_release()
 	 * directly; the smp_load_acquire() below pairs with that store. There
@@ -3297,7 +3292,7 @@ bool is_size_safe_to_change(struct cifsInodeInfo *cifsInode, __u64 end_of_file,
 	 * jiffies is still close to INITIAL_JIFFIES on 32-bit systems.
 	 */
 	if (from_readdir) {
-		/* Pairs with smp_store_release() at close and truncate sites. */
+		/* Pairs with smp_store_release() in _cifsFileInfo_put() and setattr. */
 		tlw = smp_load_acquire(&cifsInode->time_last_write);
 		if (tlw && time_before(jiffies, tlw + cifs_sb->ctx->acregmax))
 			return false;
diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index 6edebc0807ea..46e1382e8e04 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -525,24 +525,11 @@ cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode)
 	}
 	spin_unlock(&cifs_inode->open_file_lock);
 
-	if (failed_cfile) {
-		if (OPEN_FMODE(failed_cfile->f_flags) & FMODE_WRITE) {
-			/* Pairs with smp_load_acquire() in is_size_safe_to_change(). */
-			smp_store_release(&CIFS_I(d_inode(failed_cfile->dentry))->time_last_write,
-					  jiffies);
-		}
+	if (failed_cfile)
 		_cifsFileInfo_put(failed_cfile, false, false);
-	}
 
 	list_for_each_entry_safe(tmp_list, tmp_next_list, &file_head, list) {
-		struct cifsFileInfo *cfile = tmp_list->cfile;
-
-		if (OPEN_FMODE(cfile->f_flags) & FMODE_WRITE) {
-			/* Pairs with smp_load_acquire() in is_size_safe_to_change(). */
-			smp_store_release(&CIFS_I(d_inode(cfile->dentry))->time_last_write,
-					  jiffies);
-		}
-		_cifsFileInfo_put(cfile, false, false);
+		_cifsFileInfo_put(tmp_list->cfile, false, false);
 		list_del(&tmp_list->list);
 		kfree(tmp_list);
 	}
@@ -576,24 +563,11 @@ cifs_close_all_deferred_files(struct cifs_tcon *tcon)
 	}
 	spin_unlock(&tcon->open_file_lock);
 
-	if (failed_cfile) {
-		if (OPEN_FMODE(failed_cfile->f_flags) & FMODE_WRITE) {
-			/* Pairs with smp_load_acquire() in is_size_safe_to_change(). */
-			smp_store_release(&CIFS_I(d_inode(failed_cfile->dentry))->time_last_write,
-					  jiffies);
-		}
+	if (failed_cfile)
 		_cifsFileInfo_put(failed_cfile, true, false);
-	}
 
 	list_for_each_entry_safe(tmp_list, tmp_next_list, &file_head, list) {
-		struct cifsFileInfo *cfile = tmp_list->cfile;
-
-		if (OPEN_FMODE(cfile->f_flags) & FMODE_WRITE) {
-			/* Pairs with smp_load_acquire() in is_size_safe_to_change(). */
-			smp_store_release(&CIFS_I(d_inode(cfile->dentry))->time_last_write,
-					  jiffies);
-		}
-		_cifsFileInfo_put(cfile, true, false);
+		_cifsFileInfo_put(tmp_list->cfile, true, false);
 		list_del(&tmp_list->list);
 		kfree(tmp_list);
 	}
@@ -663,24 +637,11 @@ void cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon,
 	}
 	spin_unlock(&tcon->open_file_lock);
 
-	if (failed_cfile) {
-		if (OPEN_FMODE(failed_cfile->f_flags) & FMODE_WRITE) {
-			/* Pairs with smp_load_acquire() in is_size_safe_to_change(). */
-			smp_store_release(&CIFS_I(d_inode(failed_cfile->dentry))->time_last_write,
-					  jiffies);
-		}
+	if (failed_cfile)
 		_cifsFileInfo_put(failed_cfile, true, false);
-	}
 
 	list_for_each_entry_safe(tmp_list, tmp_next_list, &file_head, list) {
-		struct cifsFileInfo *cfile = tmp_list->cfile;
-
-		if (OPEN_FMODE(cfile->f_flags) & FMODE_WRITE) {
-			/* Pairs with smp_load_acquire() in is_size_safe_to_change(). */
-			smp_store_release(&CIFS_I(d_inode(cfile->dentry))->time_last_write,
-					  jiffies);
-		}
-		_cifsFileInfo_put(cfile, true, false);
+		_cifsFileInfo_put(tmp_list->cfile, true, false);
 		list_del(&tmp_list->list);
 		kfree(tmp_list);
 	}
-- 
2.55.0

[PATCH 2/2] cifs: fix time_last_write stamp placement in setattr/truncate paths

[Frank Sorenson]

cifs_file_set_size() calls cifs_setsize() on success, which calls
i_size_write(), updating i_size to the new value.  The subsequent
check attrs->ia_size != i_size_read() in both cifs_setattr_unix()
and cifs_setattr_nounix() therefore always evaluates false after a
successful cifs_file_set_size(), making the smp_store_release() of
time_last_write dead code.  The truncate path was unprotected against
stale readdir size updates.

Move the stamp to before the cifs_file_set_size() RPC call, guarded
by attrs->ia_size != i_size_read() to exclude no-op same-size
ftruncate(2) calls from stamping time_last_write unnecessarily.

On the error path the stamp remains rather than being restored:
restoring a stale snapshot (prev_tlw) could silently erase a
concurrent _cifsFileInfo_put() close stamp if that close arrived
between the READ_ONCE and the smp_store_release.  readdir is
suppressed until the stamp expires, which extends beyond one acregmax
if the caller retries failed truncations.  stat() is unaffected: the
cifs_revalidate_dentry_attr() path calls cifs_fattr_to_inode() with
from_readdir=false, which bypasses the time_last_write check in
is_size_safe_to_change() entirely and always writes the authoritative
QUERY_INFO result to i_size.

Remove the now-unreachable stamp from the dead block in both functions.

Fixes: e8a8d54c2d50 ("cifs: prevent readdir from changing file size due to stale directory metadata")
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/inode.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index b2806371bfde..808085eb0cdc 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -3190,6 +3190,17 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
 	rc = 0;
 
 	if (attrs->ia_valid & ATTR_SIZE) {
+		if (attrs->ia_size != i_size_read(inode)) {
+			/* Stamp before RPC. On failure the stamp remains: restoring a
+			 * stale snapshot could silently erase a concurrent
+			 * _cifsFileInfo_put() close stamp.  readdir is suppressed
+			 * until the stamp expires; stat() bypasses this via the
+			 * from_readdir=false path in is_size_safe_to_change() and
+			 * always returns an authoritative QUERY_INFO result.
+			 * Pairs with smp_load_acquire() in is_size_safe_to_change().
+			 */
+			smp_store_release(&cifsInode->time_last_write, jiffies);
+		}
 		rc = cifs_file_set_size(xid, direntry, full_path,
 					open_file, attrs->ia_size);
 		if (rc != 0)
@@ -3279,8 +3290,6 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
 
 	if ((attrs->ia_valid & ATTR_SIZE) &&
 	    attrs->ia_size != i_size_read(inode)) {
-		/* Pairs with smp_load_acquire() in is_size_safe_to_change(). */
-		smp_store_release(&cifsInode->time_last_write, jiffies);
 		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);
@@ -3370,6 +3379,17 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
 	}
 
 	if (attrs->ia_valid & ATTR_SIZE) {
+		if (attrs->ia_size != i_size_read(inode)) {
+			/* Stamp before RPC. On failure the stamp remains: restoring a
+			 * stale snapshot could silently erase a concurrent
+			 * _cifsFileInfo_put() close stamp.  readdir is suppressed
+			 * until the stamp expires; stat() bypasses this via the
+			 * from_readdir=false path in is_size_safe_to_change() and
+			 * always returns an authoritative QUERY_INFO result.
+			 * Pairs with smp_load_acquire() in is_size_safe_to_change().
+			 */
+			smp_store_release(&cifsInode->time_last_write, jiffies);
+		}
 		rc = cifs_file_set_size(xid, direntry, full_path,
 					cfile, attrs->ia_size);
 		if (rc != 0)
@@ -3482,8 +3502,6 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
 
 	if ((attrs->ia_valid & ATTR_SIZE) &&
 	    attrs->ia_size != i_size_read(inode)) {
-		/* Pairs with smp_load_acquire() in is_size_safe_to_change(). */
-		smp_store_release(&cifsInode->time_last_write, jiffies);
 		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);
-- 
2.55.0

Re: [PATCH 0/2] cifs: follow-on fixes to time_last_write mechanism

[Paulo Alcantara]

Frank Sorenson <sorenson@redhat.com> writes:

> This series addresses two correctness issues in commit e8a8d54c2d50
> ("cifs: prevent readdir from changing file size due to stale directory
> metadata"), which introduced the time_last_write field to protect cached
> file sizes from stale Windows Server directory enumeration metadata.
>
> Patch 1 consolidates the time_last_write stamp into _cifsFileInfo_put().
> The original commit scattered the stamp across cifs_close(),
> smb2_deferred_work_close(), and three deferred-close drain functions in
> misc.c.  This missed the case where background I/O holds the final file
> reference after userspace close() returns -- the stamp was anchored to
> the earlier userspace-close time rather than the actual server close,
> allowing the protection window to expire before the handle was removed
> from openFileList.  Moving the stamp into _cifsFileInfo_put() under
> open_file_lock provides a single canonical location that covers all close
> paths and uses the spinlock's store-release/load-acquire pairing with
> is_inode_writable() for correct memory ordering.
>
> Patch 2 fixes the stamp placement in the setattr/truncate paths.
> cifs_file_set_size() calls cifs_setsize() on success, which calls
> i_size_write() and updates i_size.  The subsequent check
> attrs->ia_size != i_size_read() therefore always evaluates false after a
> successful cifs_file_set_size(), making the stamp dead code -- the
> truncate path was completely unprotected.  The fix moves the stamp to
> before the RPC call, guarded by attrs->ia_size != i_size_read() to
> exclude no-op same-size ftruncate(2) calls.  On failure the stamp is
> left in place rather than restored: restoring a stale snapshot (prev_tlw)
> could silently erase a concurrent _cifsFileInfo_put() close stamp if that
> close arrived between the READ_ONCE and the smp_store_release.  readdir
> is suppressed until the stamp expires; stat() is unaffected because
> cifs_revalidate_dentry_attr() uses from_readdir=false, bypassing the
> time_last_write check entirely and always returning an authoritative
> QUERY_INFO result.
>
> Frank Sorenson (2):
>   cifs: consolidate time_last_write stamp into _cifsFileInfo_put()
>   cifs: fix time_last_write stamp placement in setattr/truncate paths

Reviewed-by: Paulo Alcantara (Red Hat) <pc@manguebit.org>

Re: [PATCH 0/2] cifs: follow-on fixes to time_last_write mechanism

[Steve French]

Tentatively merged into cifs-2.6.git for-next pending testing

On Mon, Jul 27, 2026 at 1:22 PM Paulo Alcantara <pc@manguebit.org> wrote:
>
> Frank Sorenson <sorenson@redhat.com> writes:
>
> > This series addresses two correctness issues in commit e8a8d54c2d50
> > ("cifs: prevent readdir from changing file size due to stale directory
> > metadata"), which introduced the time_last_write field to protect cached
> > file sizes from stale Windows Server directory enumeration metadata.
> >
> > Patch 1 consolidates the time_last_write stamp into _cifsFileInfo_put().
> > The original commit scattered the stamp across cifs_close(),
> > smb2_deferred_work_close(), and three deferred-close drain functions in
> > misc.c.  This missed the case where background I/O holds the final file
> > reference after userspace close() returns -- the stamp was anchored to
> > the earlier userspace-close time rather than the actual server close,
> > allowing the protection window to expire before the handle was removed
> > from openFileList.  Moving the stamp into _cifsFileInfo_put() under
> > open_file_lock provides a single canonical location that covers all close
> > paths and uses the spinlock's store-release/load-acquire pairing with
> > is_inode_writable() for correct memory ordering.
> >
> > Patch 2 fixes the stamp placement in the setattr/truncate paths.
> > cifs_file_set_size() calls cifs_setsize() on success, which calls
> > i_size_write() and updates i_size.  The subsequent check
> > attrs->ia_size != i_size_read() therefore always evaluates false after a
> > successful cifs_file_set_size(), making the stamp dead code -- the
> > truncate path was completely unprotected.  The fix moves the stamp to
> > before the RPC call, guarded by attrs->ia_size != i_size_read() to
> > exclude no-op same-size ftruncate(2) calls.  On failure the stamp is
> > left in place rather than restored: restoring a stale snapshot (prev_tlw)
> > could silently erase a concurrent _cifsFileInfo_put() close stamp if that
> > close arrived between the READ_ONCE and the smp_store_release.  readdir
> > is suppressed until the stamp expires; stat() is unaffected because
> > cifs_revalidate_dentry_attr() uses from_readdir=false, bypassing the
> > time_last_write check entirely and always returning an authoritative
> > QUERY_INFO result.
> >
> > Frank Sorenson (2):
> >   cifs: consolidate time_last_write stamp into _cifsFileInfo_put()
> >   cifs: fix time_last_write stamp placement in setattr/truncate paths
>
> Reviewed-by: Paulo Alcantara (Red Hat) <pc@manguebit.org>
>


-- 
Thanks,

Steve