Re: [PATCH 2/2] vfs: fix delegated timestamp handling in setattr_copy()

[Jeff Layton <jlayton@kernel.org>]

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 <jlayton@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.camel@kernel.org/

I'll post a v2 in the near future.

Cheers,
-- 
Jeff Layton <jlayton@kernel.org>