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.c...
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; } /*
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.
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.
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
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
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; } /*
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
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.c...
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);- 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
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.
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
-
Al Viro -
Amir Goldstein -
Deepa Dinamani -
J . Bruce Fields