[PATCH] utimes: Clamp the timestamps in notify_change()

[PATCH] utimes: Clamp the timestamps in notify_change()

[Amir Goldstein]

Push clamping timestamps down the call stack into notify_change(), so
in-kernel callers like nfsd and overlayfs will get similar timestamp
set behavior as utimes.

Suggested-by: Miklos Szeredi <mszeredi@redhat.com>
Fixes: 42e729b9ddbb ("utimes: Clamp the timestamps before update")
Cc: stable@vger.kernel.org # v5.4
Cc: Deepa Dinamani <deepa.kernel@gmail.com>
Cc: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Arnd,

This fixes xfstest generic/402 when run with -overlay setup.
Note that running the test requires latest xfstests with:
 acb2ba78 - overlay: support timestamp range check

I had previously posted a fix specific for overlayfs [1],
but Miklos suggested this more generic fix, which should also
serve nfsd and other in-kernel users.

I tested this change with test generic/402 on ext4/xfs/btrfs
and overlayfs, but not with nfsd.

Jeff, could you ack this change is good for nfsd as well?

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20191111073000.2957-1-amir73il@gmail.com/

 fs/attr.c   | 5 +++++
 fs/utimes.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index df28035aa23e..e8de5e636e66 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -268,8 +268,13 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
 	attr->ia_ctime = now;
 	if (!(ia_valid & ATTR_ATIME_SET))
 		attr->ia_atime = now;
+	else
+		attr->ia_atime = timestamp_truncate(attr->ia_atime, inode);
 	if (!(ia_valid & ATTR_MTIME_SET))
 		attr->ia_mtime = now;
+	else
+		attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
+
 	if (ia_valid & ATTR_KILL_PRIV) {
 		error = security_inode_need_killpriv(dentry);
 		if (error < 0)
diff --git a/fs/utimes.c b/fs/utimes.c
index 1ba3f7883870..090739322463 100644
--- a/fs/utimes.c
+++ b/fs/utimes.c
@@ -36,14 +36,14 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
 		if (times[0].tv_nsec == UTIME_OMIT)
 			newattrs.ia_valid &= ~ATTR_ATIME;
 		else if (times[0].tv_nsec != UTIME_NOW) {
-			newattrs.ia_atime = timestamp_truncate(times[0], inode);
+			newattrs.ia_atime = times[0];
 			newattrs.ia_valid |= ATTR_ATIME_SET;
 		}
 
 		if (times[1].tv_nsec == UTIME_OMIT)
 			newattrs.ia_valid &= ~ATTR_MTIME;
 		else if (times[1].tv_nsec != UTIME_NOW) {
-			newattrs.ia_mtime = timestamp_truncate(times[1], inode);
+			newattrs.ia_mtime = times[1];
 			newattrs.ia_valid |= ATTR_MTIME_SET;
 		}
 		/*
-- 
2.17.1

Re: [PATCH] utimes: Clamp the timestamps in notify_change()

[Al Viro]

On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> Push clamping timestamps down the call stack into notify_change(), so
> in-kernel callers like nfsd and overlayfs will get similar timestamp
> set behavior as utimes.
 
Makes sense; said that, shouldn't we go through ->setattr() instances and
get rid of that there, now that notify_change() is made to do it?

I mean,
        if (ia_valid & ATTR_ATIME)
                sd_iattr->ia_atime = timestamp_truncate(iattr->ia_atime,
                                                      inode);
in configfs_setattr() looks like it should be reverted to
        if (ia_valid & ATTR_ATIME)
                sd_iattr->ia_atime = iattr->ia_atime;
with that, etc.

Moreover, does that leave any valid callers of timestamp_truncate()
outside of notify_change() and current_time()?  IOW, is there any
point having it exported?  Look:
fs/attr.c:187:          inode->i_atime = timestamp_truncate(attr->ia_atime,
fs/attr.c:191:          inode->i_mtime = timestamp_truncate(attr->ia_mtime,
fs/attr.c:195:          inode->i_ctime = timestamp_truncate(attr->ia_ctime,
	setattr_copy(), called downstream of your changes.
fs/configfs/inode.c:79:         sd_iattr->ia_atime = timestamp_truncate(iattr->ia_atime,
fs/configfs/inode.c:82:         sd_iattr->ia_mtime = timestamp_truncate(iattr->ia_mtime,
fs/configfs/inode.c:85:         sd_iattr->ia_ctime = timestamp_truncate(iattr->ia_ctime,
	configfs_setattr(); ditto.
fs/f2fs/file.c:755:             inode->i_atime = timestamp_truncate(attr->ia_atime,
fs/f2fs/file.c:759:             inode->i_mtime = timestamp_truncate(attr->ia_mtime,
fs/f2fs/file.c:763:             inode->i_ctime = timestamp_truncate(attr->ia_ctime,
	__setattr_copy() from f2fs_setattr(); ditto.
fs/inode.c:2224:        return timestamp_truncate(now, inode);
	current_time()
fs/kernfs/inode.c:163:  inode->i_atime = timestamp_truncate(attrs->ia_atime, inode);
fs/kernfs/inode.c:164:  inode->i_mtime = timestamp_truncate(attrs->ia_mtime, inode);
fs/kernfs/inode.c:165:  inode->i_ctime = timestamp_truncate(attrs->ia_ctime, inode);
	->s_time_max and ->s_time_min are left TIME64_MAX and TIME64_MIN resp., so
timestamp_truncate() should be a no-op there.
fs/ntfs/inode.c:2903:           vi->i_atime = timestamp_truncate(attr->ia_atime,
fs/ntfs/inode.c:2907:           vi->i_mtime = timestamp_truncate(attr->ia_mtime,
fs/ntfs/inode.c:2911:           vi->i_ctime = timestamp_truncate(attr->ia_ctime,
	ntfs_setattr(); downstream from your changes
fs/ubifs/file.c:1082:           inode->i_atime = timestamp_truncate(attr->ia_atime,
fs/ubifs/file.c:1086:           inode->i_mtime = timestamp_truncate(attr->ia_mtime,
fs/ubifs/file.c:1090:           inode->i_ctime = timestamp_truncate(attr->ia_ctime,
	do_attr_changes(), from do_truncation() or do_setattr(), both from ubifs_setattr();
ditto.
fs/utimes.c:39:                 newattrs.ia_atime = timestamp_truncate(times[0], inode);
fs/utimes.c:46:                 newattrs.ia_mtime = timestamp_truncate(times[1], inode);
	disappears in your patch.

Re: [PATCH] utimes: Clamp the timestamps in notify_change()

[Amir Goldstein]

On Sun, Nov 24, 2019 at 9:49 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> > Push clamping timestamps down the call stack into notify_change(), so
> > in-kernel callers like nfsd and overlayfs will get similar timestamp
> > set behavior as utimes.
>
> Makes sense; said that, shouldn't we go through ->setattr() instances and
> get rid of that there, now that notify_change() is made to do it?
>

Sounds reasonable. But I'd rather leave this cleanup to Deepa,
who did all this work.

Thanks,
Amir.

Re: [PATCH] utimes: Clamp the timestamps in notify_change()

[Deepa Dinamani]

On Sun, Nov 24, 2019 at 12:50 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sun, Nov 24, 2019 at 9:49 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> > > Push clamping timestamps down the call stack into notify_change(), so
> > > in-kernel callers like nfsd and overlayfs will get similar timestamp
> > > set behavior as utimes.
> >
> > Makes sense; said that, shouldn't we go through ->setattr() instances and
> > get rid of that there, now that notify_change() is made to do it?
> >
>
> Sounds reasonable. But I'd rather leave this cleanup to Deepa,
> who did all this work.

Thanks, I can post a patch for this.

-Deepa

Re: [PATCH] utimes: Clamp the timestamps in notify_change()

[Deepa Dinamani]

On Sun, Nov 24, 2019 at 11:49 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> > Push clamping timestamps down the call stack into notify_change(), so
> > in-kernel callers like nfsd and overlayfs will get similar timestamp
> > set behavior as utimes.
>
> Makes sense; said that, shouldn't we go through ->setattr() instances and
> get rid of that there, now that notify_change() is made to do it?
>
> I mean,
>         if (ia_valid & ATTR_ATIME)
>                 sd_iattr->ia_atime = timestamp_truncate(iattr->ia_atime,
>                                                       inode);
> in configfs_setattr() looks like it should be reverted to
>         if (ia_valid & ATTR_ATIME)
>                 sd_iattr->ia_atime = iattr->ia_atime;
> with that, etc.
>
> Moreover, does that leave any valid callers of timestamp_truncate()
> outside of notify_change() and current_time()?  IOW, is there any
> point having it exported?  Look:
> fs/attr.c:187:          inode->i_atime = timestamp_truncate(attr->ia_atime,
> fs/attr.c:191:          inode->i_mtime = timestamp_truncate(attr->ia_mtime,
> fs/attr.c:195:          inode->i_ctime = timestamp_truncate(attr->ia_ctime,
>         setattr_copy(), called downstream of your changes.
> fs/configfs/inode.c:79:         sd_iattr->ia_atime = timestamp_truncate(iattr->ia_atime,
> fs/configfs/inode.c:82:         sd_iattr->ia_mtime = timestamp_truncate(iattr->ia_mtime,
> fs/configfs/inode.c:85:         sd_iattr->ia_ctime = timestamp_truncate(iattr->ia_ctime,
>         configfs_setattr(); ditto.
> fs/f2fs/file.c:755:             inode->i_atime = timestamp_truncate(attr->ia_atime,
> fs/f2fs/file.c:759:             inode->i_mtime = timestamp_truncate(attr->ia_mtime,
> fs/f2fs/file.c:763:             inode->i_ctime = timestamp_truncate(attr->ia_ctime,
>         __setattr_copy() from f2fs_setattr(); ditto.
> fs/inode.c:2224:        return timestamp_truncate(now, inode);
>         current_time()
> fs/kernfs/inode.c:163:  inode->i_atime = timestamp_truncate(attrs->ia_atime, inode);
> fs/kernfs/inode.c:164:  inode->i_mtime = timestamp_truncate(attrs->ia_mtime, inode);
> fs/kernfs/inode.c:165:  inode->i_ctime = timestamp_truncate(attrs->ia_ctime, inode);
>         ->s_time_max and ->s_time_min are left TIME64_MAX and TIME64_MIN resp., so
> timestamp_truncate() should be a no-op there.
> fs/ntfs/inode.c:2903:           vi->i_atime = timestamp_truncate(attr->ia_atime,
> fs/ntfs/inode.c:2907:           vi->i_mtime = timestamp_truncate(attr->ia_mtime,
> fs/ntfs/inode.c:2911:           vi->i_ctime = timestamp_truncate(attr->ia_ctime,
>         ntfs_setattr(); downstream from your changes
> fs/ubifs/file.c:1082:           inode->i_atime = timestamp_truncate(attr->ia_atime,
> fs/ubifs/file.c:1086:           inode->i_mtime = timestamp_truncate(attr->ia_mtime,
> fs/ubifs/file.c:1090:           inode->i_ctime = timestamp_truncate(attr->ia_ctime,
>         do_attr_changes(), from do_truncation() or do_setattr(), both from ubifs_setattr();
> ditto.
> fs/utimes.c:39:                 newattrs.ia_atime = timestamp_truncate(times[0], inode);
> fs/utimes.c:46:                 newattrs.ia_mtime = timestamp_truncate(times[1], inode);
>         disappears in your patch.

We also want to replace all uses of timespec64_trunc() with
timestamp_truncate() for all fs cases.

In that case we have a few more:

fs/ceph/mds_client.c:   req->r_stamp = timespec64_trunc(ts,
mdsc->fsc->sb->s_time_gran);
fs/cifs/inode.c:        fattr->cf_mtime =
timespec64_trunc(fattr->cf_mtime, sb->s_time_gran);
fs/cifs/inode.c:                fattr->cf_atime =
timespec64_trunc(fattr->cf_atime, sb->s_time_gran);
fs/fat/misc.c:static inline struct timespec64
fat_timespec64_trunc_2secs(struct timespec64 ts)
fs/fat/misc.c:                  inode->i_ctime =
timespec64_trunc(*now, 10000000);
fs/fat/misc.c:                  inode->i_ctime =
fat_timespec64_trunc_2secs(*now);
fs/fat/misc.c:          inode->i_mtime = fat_timespec64_trunc_2secs(*now);
fs/ubifs/sb.c:  ts = timespec64_trunc(ts, DEFAULT_TIME_GRAN);

These do not follow from notify_change(), so these might still need
timestamp_truncate() exported.
I will post a cleanup series for timespec64_trunc() also, then we can decide.

-Deepa

Re: [PATCH] utimes: Clamp the timestamps in notify_change()

[Al Viro]

On Sun, Nov 24, 2019 at 01:13:50PM -0800, Deepa Dinamani wrote:

> We also want to replace all uses of timespec64_trunc() with
> timestamp_truncate() for all fs cases.
> 
> In that case we have a few more:
> 
> fs/ceph/mds_client.c:   req->r_stamp = timespec64_trunc(ts,
> mdsc->fsc->sb->s_time_gran);

Umm... That comes from ktime_get_coarse_real_ts64(&ts);

> fs/cifs/inode.c:        fattr->cf_mtime =
> timespec64_trunc(fattr->cf_mtime, sb->s_time_gran);
ktime_get_real_ts64(&fattr->cf_mtime) here

> fs/cifs/inode.c:                fattr->cf_atime =
> timespec64_trunc(fattr->cf_atime, sb->s_time_gran);
ditto

> fs/fat/misc.c:                  inode->i_ctime =
> timespec64_trunc(*now, 10000000);

I wonder... some are from setattr, some (with NULL passed to fat_truncate_time())
from current_time()...  Wouldn't it make more sense to move the truncation into
the few callers that really need it (if any)?  Quite a few of those are *also*
getting the value from current_time(), after all.  fat_fill_inode() looks like
the only case that doesn't fall into these classes; does it need truncation?

BTW, could we *please* do something about fs/inode.c:update_time()?  I mean,
sure, local variable shadows file-scope function, so it's legitimate C, but
this is not IOCCC and having a function called 'update_time' end with
        return update_time(inode, time, flags);
is actively hostile towards casual readers...

> fs/fat/misc.c:                  inode->i_ctime =
> fat_timespec64_trunc_2secs(*now);
> fs/fat/misc.c:          inode->i_mtime = fat_timespec64_trunc_2secs(*now);
> fs/ubifs/sb.c:  ts = timespec64_trunc(ts, DEFAULT_TIME_GRAN);
> 
> These do not follow from notify_change(), so these might still need
> timestamp_truncate() exported.
> I will post a cleanup series for timespec64_trunc() also, then we can decide.

What I've got right now is

commit 6d13412e2b27970810037f7b1b418febcd7013aa
Author: Amir Goldstein <amir73il@gmail.com>
Date:   Sun Nov 24 21:31:45 2019 +0200

    utimes: Clamp the timestamps in notify_change()
    
    Push clamping timestamps into notify_change(), so in-kernel
    callers like nfsd and overlayfs will get similar timestamp
    set behavior as utimes.
    
    AV: get rid of clamping in ->setattr() instances; we don't need
    to bother with that there, with notify_change() doing normalization
    in all cases now (it already did for implicit case, since current_time()
    clamps).
    
    Suggested-by: Miklos Szeredi <mszeredi@redhat.com>
    Fixes: 42e729b9ddbb ("utimes: Clamp the timestamps before update")
    Cc: stable@vger.kernel.org # v5.4
    Cc: Deepa Dinamani <deepa.kernel@gmail.com>
    Cc: Jeff Layton <jlayton@kernel.org>
    Signed-off-by: Amir Goldstein <amir73il@gmail.com>
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/fs/attr.c b/fs/attr.c
index df28035aa23e..b4bbdbd4c8ca 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -183,18 +183,12 @@ void setattr_copy(struct inode *inode, const struct iattr *attr)
 		inode->i_uid = attr->ia_uid;
 	if (ia_valid & ATTR_GID)
 		inode->i_gid = attr->ia_gid;
-	if (ia_valid & ATTR_ATIME) {
-		inode->i_atime = timestamp_truncate(attr->ia_atime,
-						  inode);
-	}
-	if (ia_valid & ATTR_MTIME) {
-		inode->i_mtime = timestamp_truncate(attr->ia_mtime,
-						  inode);
-	}
-	if (ia_valid & ATTR_CTIME) {
-		inode->i_ctime = timestamp_truncate(attr->ia_ctime,
-						  inode);
-	}
+	if (ia_valid & ATTR_ATIME)
+		inode->i_atime = attr->ia_atime;
+	if (ia_valid & ATTR_MTIME)
+		inode->i_mtime = attr->ia_mtime;
+	if (ia_valid & ATTR_CTIME)
+		inode->i_ctime = attr->ia_ctime;
 	if (ia_valid & ATTR_MODE) {
 		umode_t mode = attr->ia_mode;
 
@@ -268,8 +262,13 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
 	attr->ia_ctime = now;
 	if (!(ia_valid & ATTR_ATIME_SET))
 		attr->ia_atime = now;
+	else
+		attr->ia_atime = timestamp_truncate(attr->ia_atime, inode);
 	if (!(ia_valid & ATTR_MTIME_SET))
 		attr->ia_mtime = now;
+	else
+		attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
+
 	if (ia_valid & ATTR_KILL_PRIV) {
 		error = security_inode_need_killpriv(dentry);
 		if (error < 0)
diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index 680aba9c00d5..fd0b5dd68f9e 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -76,14 +76,11 @@ int configfs_setattr(struct dentry * dentry, struct iattr * iattr)
 	if (ia_valid & ATTR_GID)
 		sd_iattr->ia_gid = iattr->ia_gid;
 	if (ia_valid & ATTR_ATIME)
-		sd_iattr->ia_atime = timestamp_truncate(iattr->ia_atime,
-						      inode);
+		sd_iattr->ia_atime = iattr->ia_atime;
 	if (ia_valid & ATTR_MTIME)
-		sd_iattr->ia_mtime = timestamp_truncate(iattr->ia_mtime,
-						      inode);
+		sd_iattr->ia_mtime = iattr->ia_mtime;
 	if (ia_valid & ATTR_CTIME)
-		sd_iattr->ia_ctime = timestamp_truncate(iattr->ia_ctime,
-						      inode);
+		sd_iattr->ia_ctime = iattr->ia_ctime;
 	if (ia_valid & ATTR_MODE) {
 		umode_t mode = iattr->ia_mode;
 
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 29bc0a542759..a286564ba2e1 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -751,18 +751,12 @@ static void __setattr_copy(struct inode *inode, const struct iattr *attr)
 		inode->i_uid = attr->ia_uid;
 	if (ia_valid & ATTR_GID)
 		inode->i_gid = attr->ia_gid;
-	if (ia_valid & ATTR_ATIME) {
-		inode->i_atime = timestamp_truncate(attr->ia_atime,
-						  inode);
-	}
-	if (ia_valid & ATTR_MTIME) {
-		inode->i_mtime = timestamp_truncate(attr->ia_mtime,
-						  inode);
-	}
-	if (ia_valid & ATTR_CTIME) {
-		inode->i_ctime = timestamp_truncate(attr->ia_ctime,
-						  inode);
-	}
+	if (ia_valid & ATTR_ATIME)
+		inode->i_atime = attr->ia_atime;
+	if (ia_valid & ATTR_MTIME)
+		inode->i_mtime = attr->ia_mtime;
+	if (ia_valid & ATTR_CTIME)
+		inode->i_ctime = attr->ia_ctime;
 	if (ia_valid & ATTR_MODE) {
 		umode_t mode = attr->ia_mode;
 
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 6c7388430ad3..d4359a1df3d5 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -2899,18 +2899,12 @@ int ntfs_setattr(struct dentry *dentry, struct iattr *attr)
 			ia_valid |= ATTR_MTIME | ATTR_CTIME;
 		}
 	}
-	if (ia_valid & ATTR_ATIME) {
-		vi->i_atime = timestamp_truncate(attr->ia_atime,
-					       vi);
-	}
-	if (ia_valid & ATTR_MTIME) {
-		vi->i_mtime = timestamp_truncate(attr->ia_mtime,
-					       vi);
-	}
-	if (ia_valid & ATTR_CTIME) {
-		vi->i_ctime = timestamp_truncate(attr->ia_ctime,
-					       vi);
-	}
+	if (ia_valid & ATTR_ATIME)
+		vi->i_atime = attr->ia_atime;
+	if (ia_valid & ATTR_MTIME)
+		vi->i_mtime = attr->ia_mtime;
+	if (ia_valid & ATTR_CTIME)
+		vi->i_ctime = attr->ia_ctime;
 	mark_inode_dirty(vi);
 out:
 	return err;
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index cd52585c8f4f..91362079f82a 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1078,18 +1078,12 @@ static void do_attr_changes(struct inode *inode, const struct iattr *attr)
 		inode->i_uid = attr->ia_uid;
 	if (attr->ia_valid & ATTR_GID)
 		inode->i_gid = attr->ia_gid;
-	if (attr->ia_valid & ATTR_ATIME) {
-		inode->i_atime = timestamp_truncate(attr->ia_atime,
-						  inode);
-	}
-	if (attr->ia_valid & ATTR_MTIME) {
-		inode->i_mtime = timestamp_truncate(attr->ia_mtime,
-						  inode);
-	}
-	if (attr->ia_valid & ATTR_CTIME) {
-		inode->i_ctime = timestamp_truncate(attr->ia_ctime,
-						  inode);
-	}
+	if (attr->ia_valid & ATTR_ATIME)
+		inode->i_atime = attr->ia_atime;
+	if (attr->ia_valid & ATTR_MTIME)
+		inode->i_mtime = attr->ia_mtime;
+	if (attr->ia_valid & ATTR_CTIME)
+		inode->i_ctime = attr->ia_ctime;
 	if (attr->ia_valid & ATTR_MODE) {
 		umode_t mode = attr->ia_mode;
 
diff --git a/fs/utimes.c b/fs/utimes.c
index 1ba3f7883870..090739322463 100644
--- a/fs/utimes.c
+++ b/fs/utimes.c
@@ -36,14 +36,14 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
 		if (times[0].tv_nsec == UTIME_OMIT)
 			newattrs.ia_valid &= ~ATTR_ATIME;
 		else if (times[0].tv_nsec != UTIME_NOW) {
-			newattrs.ia_atime = timestamp_truncate(times[0], inode);
+			newattrs.ia_atime = times[0];
 			newattrs.ia_valid |= ATTR_ATIME_SET;
 		}
 
 		if (times[1].tv_nsec == UTIME_OMIT)
 			newattrs.ia_valid &= ~ATTR_MTIME;
 		else if (times[1].tv_nsec != UTIME_NOW) {
-			newattrs.ia_mtime = timestamp_truncate(times[1], inode);
+			newattrs.ia_mtime = times[1];
 			newattrs.ia_valid |= ATTR_MTIME_SET;
 		}
 		/*

Re: [PATCH] utimes: Clamp the timestamps in notify_change()

[Deepa Dinamani]

On Sun, Nov 24, 2019 at 1:34 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sun, Nov 24, 2019 at 01:13:50PM -0800, Deepa Dinamani wrote:
>
> > We also want to replace all uses of timespec64_trunc() with
> > timestamp_truncate() for all fs cases.
> >
> > In that case we have a few more:
> >
> > fs/ceph/mds_client.c:   req->r_stamp = timespec64_trunc(ts,
> > mdsc->fsc->sb->s_time_gran);
>
> Umm... That comes from ktime_get_coarse_real_ts64(&ts);
>
> > fs/cifs/inode.c:        fattr->cf_mtime =
> > timespec64_trunc(fattr->cf_mtime, sb->s_time_gran);
> ktime_get_real_ts64(&fattr->cf_mtime) here
>
> > fs/cifs/inode.c:                fattr->cf_atime =
> > timespec64_trunc(fattr->cf_atime, sb->s_time_gran);
> ditto
>
> > fs/fat/misc.c:                  inode->i_ctime =
> > timespec64_trunc(*now, 10000000);
>
> I wonder... some are from setattr, some (with NULL passed to fat_truncate_time())
> from current_time()...  Wouldn't it make more sense to move the truncation into
> the few callers that really need it (if any)?  Quite a few of those are *also*
> getting the value from current_time(), after all.  fat_fill_inode() looks like
> the only case that doesn't fall into these classes; does it need truncation?

I've posted a series at
https://lore.kernel.org/lkml/20191130053030.7868-1-deepa.kernel@gmail.com/
I was able to get rid of all instances but it seemed like it would be
easier for cifs to use timestamp_truncate() directly.
If you really don't want it exported, I could find some other way of doing it.

> BTW, could we *please* do something about fs/inode.c:update_time()?  I mean,
> sure, local variable shadows file-scope function, so it's legitimate C, but
> this is not IOCCC and having a function called 'update_time' end with
>         return update_time(inode, time, flags);
> is actively hostile towards casual readers...

Added this to the series as well.

-Deepa

Re: [PATCH] utimes: Clamp the timestamps in notify_change()

[J . Bruce Fields]

On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> Push clamping timestamps down the call stack into notify_change(), so
> in-kernel callers like nfsd and overlayfs will get similar timestamp
> set behavior as utimes.

So, nfsd has always bypassed timestamp_truncate() and we've never
noticed till now?  What are the symptoms?  (Do timestamps go backwards
after cache eviction on filesystems with large time granularity?)

Looks like generic/402 has never run in my tests:

	generic/402     [not run] no kernel support for y2038 sysfs switch

--b.

> 
> Suggested-by: Miklos Szeredi <mszeredi@redhat.com>
> Fixes: 42e729b9ddbb ("utimes: Clamp the timestamps before update")
> Cc: stable@vger.kernel.org # v5.4
> Cc: Deepa Dinamani <deepa.kernel@gmail.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Arnd,
> 
> This fixes xfstest generic/402 when run with -overlay setup.
> Note that running the test requires latest xfstests with:
>  acb2ba78 - overlay: support timestamp range check
> 
> I had previously posted a fix specific for overlayfs [1],
> but Miklos suggested this more generic fix, which should also
> serve nfsd and other in-kernel users.
> 
> I tested this change with test generic/402 on ext4/xfs/btrfs
> and overlayfs, but not with nfsd.
> 
> Jeff, could you ack this change is good for nfsd as well?
> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-fsdevel/20191111073000.2957-1-amir73il@gmail.com/
> 
>  fs/attr.c   | 5 +++++
>  fs/utimes.c | 4 ++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/attr.c b/fs/attr.c
> index df28035aa23e..e8de5e636e66 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -268,8 +268,13 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
>  	attr->ia_ctime = now;
>  	if (!(ia_valid & ATTR_ATIME_SET))
>  		attr->ia_atime = now;
> +	else
> +		attr->ia_atime = timestamp_truncate(attr->ia_atime, inode);
>  	if (!(ia_valid & ATTR_MTIME_SET))
>  		attr->ia_mtime = now;
> +	else
> +		attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
> +
>  	if (ia_valid & ATTR_KILL_PRIV) {
>  		error = security_inode_need_killpriv(dentry);
>  		if (error < 0)
> diff --git a/fs/utimes.c b/fs/utimes.c
> index 1ba3f7883870..090739322463 100644
> --- a/fs/utimes.c
> +++ b/fs/utimes.c
> @@ -36,14 +36,14 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
>  		if (times[0].tv_nsec == UTIME_OMIT)
>  			newattrs.ia_valid &= ~ATTR_ATIME;
>  		else if (times[0].tv_nsec != UTIME_NOW) {
> -			newattrs.ia_atime = timestamp_truncate(times[0], inode);
> +			newattrs.ia_atime = times[0];
>  			newattrs.ia_valid |= ATTR_ATIME_SET;
>  		}
>  
>  		if (times[1].tv_nsec == UTIME_OMIT)
>  			newattrs.ia_valid &= ~ATTR_MTIME;
>  		else if (times[1].tv_nsec != UTIME_NOW) {
> -			newattrs.ia_mtime = timestamp_truncate(times[1], inode);
> +			newattrs.ia_mtime = times[1];
>  			newattrs.ia_valid |= ATTR_MTIME_SET;
>  		}
>  		/*
> -- 
> 2.17.1

Re: [PATCH] utimes: Clamp the timestamps in notify_change()

[Amir Goldstein]

On Mon, Nov 25, 2019 at 6:46 PM J . Bruce Fields <bfields@fieldses.org> wrote:
>
> On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> > Push clamping timestamps down the call stack into notify_change(), so
> > in-kernel callers like nfsd and overlayfs will get similar timestamp
> > set behavior as utimes.
>
> So, nfsd has always bypassed timestamp_truncate() and we've never
> noticed till now?  What are the symptoms?  (Do timestamps go backwards
> after cache eviction on filesystems with large time granularity?)

Clamping seems to be new behavior since v5.4-rc1.
Before that clamping was done implicitly when hitting the disk IIUC,
so it was observed mostly after cache eviction.

>
> Looks like generic/402 has never run in my tests:
>
>         generic/402     [not run] no kernel support for y2038 sysfs switch
>

The test in its current form is quite recent as well or at the _require
has changed recently.
See acb2ba78 - overlay: support timestamp range check

You'd probably need something similar for nfs (?)

Thanks,
Amir.

Re: [PATCH] utimes: Clamp the timestamps in notify_change()

[Deepa Dinamani]

On Mon, Nov 25, 2019 at 8:46 AM J . Bruce Fields <bfields@fieldses.org> wrote:
>
> On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> > Push clamping timestamps down the call stack into notify_change(), so
> > in-kernel callers like nfsd and overlayfs will get similar timestamp
> > set behavior as utimes.
>
> So, nfsd has always bypassed timestamp_truncate() and we've never
> noticed till now?  What are the symptoms?  (Do timestamps go backwards
> after cache eviction on filesystems with large time granularity?)
>
> Looks like generic/402 has never run in my tests:
>
>         generic/402     [not run] no kernel support for y2038 sysfs switch

You need this in your xfstest:
https://patchwork.kernel.org/patch/11049745/ . The test has been
updated recently.

And, you need a change like for overlayfs as Amir pointed out.

-Deepa