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

Frank Sorenson <[email protected]> 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) <[email protected]>