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

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

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

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: stable@vger.kernel.org
Signed-off-by: Xu Rao <raoxu@uniontech.com>
---
 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));
 }

--
2.50.1

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

[Steve French]

merged into cifs-2.6.git for-next

On Tue, Jul 7, 2026 at 8:43 AM raoxu <raoxu@uniontech.com> wrote:
>
> From: Xu Rao <raoxu@uniontech.com>
>
> 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: stable@vger.kernel.org
> Signed-off-by: Xu Rao <raoxu@uniontech.com>
> ---
>  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));
>  }
>
> --
> 2.50.1
>
>


-- 
Thanks,

Steve

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

[David Laight]

On Tue,  7 Jul 2026 21:30:17 +0800
raoxu <raoxu@uniontech.com> wrote:

> From: Xu Rao <raoxu@uniontech.com>
> 
> 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: stable@vger.kernel.org
> Signed-off-by: Xu Rao <raoxu@uniontech.com>
> ---
>  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.

	David

>  }
> 
> --
> 2.50.1
> 
> 

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

[Steve French]

On Thu, Jul 9, 2026 at 11:40 AM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Tue,  7 Jul 2026 21:30:17 +0800
> raoxu <raoxu@uniontech.com> wrote:
>
> > From: Xu Rao <raoxu@uniontech.com>
> >
> > 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: stable@vger.kernel.org
> > Signed-off-by: Xu Rao <raoxu@uniontech.com>
> > ---
> >  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