Re: [PATCH] smb: client: fix atime clamp check in read completion

On Thu, Jul 9, 2026 at 11:40 AM David Laight
<[email protected]> wrote:
>
> On Tue,  7 Jul 2026 21:30:17 +0800
> raoxu <[email protected]> wrote:
>
> > From: Xu Rao <[email protected]>
> >
> > cifs_rreq_done() updates the inode atime to current_time(inode) after a
> > netfs read.  It then preserves the CIFS rule that atime should not be
> > older than mtime, because some applications break if atime is less than
> > mtime.  That rule only requires clamping when atime < mtime.
> >
> > The current check uses the raw non-zero result of timespec64_compare().
> > It therefore takes the clamp path for both atime < mtime and
> > atime > mtime.  The latter is the normal case when reading an older file:
> > the newly recorded atime is newer than the file mtime.  The completion
> > handler then immediately moves atime back to mtime, losing the access
> > time that was just recorded.  Userspace tools that rely on atime, such as
> > stat, find -atime, backup tools or cold-data classifiers, can therefore
> > see a recently read CIFS file as not recently accessed.
> >
> > This is easy to miss because the bug is silent: read I/O still succeeds,
> > no error is reported, and many systems either do not check atime after
> > reads or mount with policies such as relatime/noatime.  It becomes
> > visible when a CIFS file has an mtime older than the current time, the
> > file is read, and the local inode atime is inspected before a later
> > revalidation replaces the cached timestamps.
> >
> > Clamp only when atime is actually older than mtime.  This matches the
> > same atime/mtime rule used when applying CIFS inode attributes.
> >
> > Fixes: 69c3c023af25 ("cifs: Implement netfslib hooks")
> > Cc: [email protected]
> > Signed-off-by: Xu Rao <[email protected]>
> > ---
> >  fs/smb/client/file.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> > index 58430ba51b10..62605928d2b8 100644
> > --- a/fs/smb/client/file.c
> > +++ b/fs/smb/client/file.c
> > @@ -301,7 +301,7 @@ static void cifs_rreq_done(struct netfs_io_request *rreq)
> >       /* we do not want atime to be less than mtime, it broke some apps */
> >       atime = inode_set_atime_to_ts(inode, current_time(inode));
> >       mtime = inode_get_mtime(inode);
> > -     if (timespec64_compare(&atime, &mtime))
> > +     if (timespec64_compare(&atime, &mtime) < 0)
> >               inode_set_atime_to_ts(inode, inode_get_mtime(inode));
>
> Should that be calling inode_get_mtime() again?
> It seems to have the value cached.

Would that be a performance hit?



-- 
Thanks,

Steve