linux-kernel - Re: [PATCH v3 3/8] vfs: add ATTR_CTIME

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <175366106815.2234665.13768447223879357240@noble.neil.brown.name>
Date: Mon, 28 Jul 2025 10:04:28 +1000
From: "NeilBrown" <neil@...wn.name>
To: "Jeff Layton" <jlayton@...nel.org>
Cc: "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>,
 "Olga Kornievskaia" <okorniev@...hat.com>, "Dai Ngo" <Dai.Ngo@...cle.com>,
 "Tom Talpey" <tom@...pey.com>, "Trond Myklebust" <trondmy@...merspace.com>,
 "Anna Schumaker" <anna@...nel.org>, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 linux-nfs@...r.kernel.org, "Jeff Layton" <jlayton@...nel.org>
Subject: Re: [PATCH v3 3/8] vfs: add ATTR_CTIME_SET flag

On Mon, 28 Jul 2025, Jeff Layton wrote:
> When ATTR_ATIME_SET and ATTR_MTIME_SET are set in the ia_valid mask, the
> notify_change() logic takes that to mean that the request should set
> those values explicitly, and not override them with "now".
> 
> With the advent of delegated timestamps, similar functionality is needed
> for the ctime. Add a ATTR_CTIME_SET flag, and use that to indicate that
> the ctime should be accepted as-is. Also, clean up the if statements to
> eliminate the extra negatives.

I don't feel entirely comfortable with this.  ctime is a fallback for
"has anything changed" - mtime can be changed but ctime is always
reliable, controlled by VFS and FS.

Until now.

I know you aren't exposing this to user-space, but then not doing so
blocks user-space file servers from using this functionality.

I see that you also move vetting of the value out of vfs code and into
nfsd code.  I don't really understand why you did that.  Maybe nfsd has
more information about previous timestamps than the vfs has?

Anyway I would much prefer that ATTR_CTIME_SET could only change the
ctime value to something between the old ctime value and the current
time (inclusive).

Certainly nfsd might impose extra restrictions, but I think that basic
restriction should by in the VFS close to what ATTR_CTIME_SET is
honoured.  What way if someone else finds another use for it some day
they will have to work within the same restriction (or change it
explicitly and try to justify that change).

Lustre has the equivalent of ATTR_CTIME_SET (MFS_ATTR_CTIME_SET and
LA_CTIME) and would want to use it if the server-side code ever landed
upstream.  It appears to just assume the client sent a valid timestamp.
I would rather it were vetted by the VFS.

Thanks,
NeilBrown


> 
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
>  fs/attr.c          | 15 +++++++++------
>  include/linux/fs.h |  1 +
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/attr.c b/fs/attr.c
> index 9caf63d20d03e86c535e9c8c91d49c2a34d34b7a..f0dabd2985989d283a931536a5fc53eda366b373 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -463,15 +463,18 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
>  
>  	now = current_time(inode);
>  
> -	attr->ia_ctime = now;
> -	if (!(ia_valid & ATTR_ATIME_SET))
> -		attr->ia_atime = now;
> -	else
> +	if (ia_valid & ATTR_ATIME_SET)
>  		attr->ia_atime = timestamp_truncate(attr->ia_atime, inode);
> -	if (!(ia_valid & ATTR_MTIME_SET))
> -		attr->ia_mtime = now;
>  	else
> +		attr->ia_atime = now;
> +	if (ia_valid & ATTR_CTIME_SET)
> +		attr->ia_ctime = timestamp_truncate(attr->ia_ctime, inode);
> +	else
> +		attr->ia_ctime = now;
> +	if (ia_valid & ATTR_MTIME_SET)
>  		attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
> +	else
> +		attr->ia_mtime = now;
>  
>  	if (ia_valid & ATTR_KILL_PRIV) {
>  		error = security_inode_need_killpriv(dentry);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 040c0036320fdf87a2379d494ab408a7991875bd..f18f45e88545c39716b917b1378fb7248367b41d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -237,6 +237,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
>  #define ATTR_ATIME_SET	(1 << 7)
>  #define ATTR_MTIME_SET	(1 << 8)
>  #define ATTR_FORCE	(1 << 9) /* Not a change, but a change it */
> +#define ATTR_CTIME_SET	(1 << 10)
>  #define ATTR_KILL_SUID	(1 << 11)
>  #define ATTR_KILL_SGID	(1 << 12)
>  #define ATTR_FILE	(1 << 13)
> 
> -- 
> 2.50.1
> 
>