| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e28297a2abe8253c0aa590831b3857432bef60f7.camel@kernel.org>
Date: Tue, 22 Jul 2025 18:19:51 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner
<brauner@...nel.org>, Jan Kara <jack@...e.cz>, Steven Rostedt
<rostedt@...dmis.org>, Masami Hiramatsu <mhiramat@...nel.org>, Mathieu
Desnoyers <mathieu.desnoyers@...icios.com>, Chuck Lever
<chuck.lever@...cle.com>, NeilBrown <neil@...wn.name>, Olga Kornievskaia
<okorniev@...hat.com>, Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey
<tom@...pey.com>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-trace-kernel@...r.kernel.org, linux-nfs@...r.kernel.org
Subject: Re: [PATCH 2/2] vfs: fix delegated timestamp handling in
setattr_copy()
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@...nel.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@...nel.org>
Powered by blists - more mailing lists