On Tue, 2025-07-22 at 14:52 -0400, Jeff Layton wrote:
> There are a couple of problems with delegated timestamp updates via
> setattr:
>
> 1/ the ia_ctime is always clobbered by notify_change(), so setting the
> ia_ctime to the same value as the ia_mtime in nfsd4_decode_fattr4()
> doesn't work.
>
> 2/ while it does test the ctime's validity vs. the existing ctime and
> current_time(), the same is not done for the atime or mtime. The spec
> requires this.
>
> Add a new setattr_copy_delegts() function that handles updating the
> timestamps whenever ATTR_DELEG is set. For both atime and mtime,
> validate and clamp the value to current_time(), and then set it. If the
> mtime gets updated, also update the ctime.
>
> Fixes: 7f2c86cba3c5 ("fs: handle delegated timestamps in setattr_copy_mgtime")
> Signed-off-by: Jeff Layton <jlay...@kernel.org>
> ---
> fs/attr.c | 52 ++++++++++++++++++++++++++++++++++++++--------------
> fs/nfsd/nfs4xdr.c | 4 +---
> 2 files changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index
> 9caf63d20d03e86c535e9c8c91d49c2a34d34b7a..3e636943d26a36aeeed0ff8b428b6dd3e63f8dde
> 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -287,14 +287,7 @@ static void setattr_copy_mgtime(struct inode *inode,
> const struct iattr *attr)
> struct timespec64 now;
>
> if (ia_valid & ATTR_CTIME) {
> - /*
> - * In the case of an update for a write delegation, we must
> respect
> - * the value in ia_ctime and not use the current time.
> - */
> - if (ia_valid & ATTR_DELEG)
> - now = inode_set_ctime_deleg(inode, attr->ia_ctime);
> - else
> - now = inode_set_ctime_current(inode);
> + now = inode_set_ctime_current(inode);
> } else {
> /* If ATTR_CTIME isn't set, then ATTR_MTIME shouldn't be
> either. */
> WARN_ON_ONCE(ia_valid & ATTR_MTIME);
> @@ -312,6 +305,39 @@ static void setattr_copy_mgtime(struct inode *inode,
> const struct iattr *attr)
> inode_set_mtime_to_ts(inode, now);
> }
>
> +/*
> + * Skip update if new value is older than existing time. Clamp
> + * to current_time() if it's in the future.
> + */
> +static void setattr_copy_delegts(struct inode *inode, const struct iattr
> *attr)
> +{
> + struct timespec64 now = current_time(inode);
> + unsigned int ia_valid = attr->ia_valid;
> +
> + if (ia_valid & ATTR_MTIME) {
> + struct timespec64 cur = inode_get_mtime(inode);
> +
> + if (timespec64_compare(&attr->ia_mtime, &cur) > 0) {
> + if (timespec64_compare(&attr->ia_mtime, &now) > 0)
> + inode_set_mtime_to_ts(inode, now);
> + else
> + inode_set_mtime_to_ts(inode, attr->ia_mtime);
> + inode_set_ctime_deleg(inode, attr->ia_mtime);
> + }
> + }
> +
> + if (ia_valid & ATTR_ATIME) {
> + struct timespec64 cur = inode_get_atime(inode);
> +
> + if (timespec64_compare(&attr->ia_atime, &cur) > 0) {
> + if (timespec64_compare(&attr->ia_atime, &now) > 0)
> + inode_set_atime_to_ts(inode, now);
> + else
> + inode_set_atime_to_ts(inode, attr->ia_atime);
> + }
> + }
> +}
> +
> /**
> * setattr_copy - copy simple metadata updates into the generic inode
> * @idmap: idmap of the mount the inode was found from
> @@ -352,6 +378,8 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode
> *inode,
> inode->i_mode = mode;
> }
>
> + if (ia_valid & ATTR_DELEG)
> + return setattr_copy_delegts(inode, attr);
> if (is_mgtime(inode))
> return setattr_copy_mgtime(inode, attr);
>
> @@ -359,12 +387,8 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode
> *inode,
> inode_set_atime_to_ts(inode, attr->ia_atime);
> if (ia_valid & ATTR_MTIME)
> inode_set_mtime_to_ts(inode, attr->ia_mtime);
> - if (ia_valid & ATTR_CTIME) {
> - if (ia_valid & ATTR_DELEG)
> - inode_set_ctime_deleg(inode, attr->ia_ctime);
> - else
> - inode_set_ctime_to_ts(inode, attr->ia_ctime);
> - }
> + if (ia_valid & ATTR_CTIME)
> + inode_set_ctime_to_ts(inode, attr->ia_ctime);
> }
> EXPORT_SYMBOL(setattr_copy);
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index
> 8b68f74a8cf08c6aa1305a2a3093467656085e4a..e6899a3502332d686138abee2284c87fc7fbc0ae
> 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -537,9 +537,7 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32
> *bmval, u32 bmlen,
> return nfserr_bad_xdr;
> iattr->ia_mtime.tv_sec = modify.seconds;
> iattr->ia_mtime.tv_nsec = modify.nseconds;
> - iattr->ia_ctime.tv_sec = modify.seconds;
> - iattr->ia_ctime.tv_nsec = modify.seconds;
> - iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET |
> ATTR_DELEG;
> + iattr->ia_valid |= ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
> }
>
> /* request sanity: did attrlist4 contain the expected number of words?
> */
Based on Trond's comments in this thread, I think I'm going to have to
respin this:
https://lore.kernel.org/linux-nfs/bfa20f4a81e0c2d5df8525476fb29af156f4f5f1.ca...@kernel.org/
I'll post a v2 in the near future.
Cheers,
--
Jeff Layton <jlay...@kernel.org>
