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] [day] [month] [year] [list]
Message-id: <175401119116.2234665.1029066014854377072@noble.neil.brown.name>
Date: Fri, 01 Aug 2025 11:19:51 +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 v4 8/8] nfsd: freeze c/mtime updates with outstanding
 WRITE_ATTRS delegation

On Wed, 30 Jul 2025, Jeff Layton wrote:
> Instead of allowing the ctime to roll backward with a WRITE_ATTRS
> delegation, set FMODE_NOCMTIME on the file and have it skip mtime and
> ctime updates.
> 
> It is possible that the client will never send a SETATTR to set the
> times before returning the delegation. Add two new bools to struct
> nfs4_delegation:
> 
> dl_written: tracks whether the file has been written since the
> delegation was granted. This is set in the WRITE and LAYOUTCOMMIT
> handlers.
> 
> dl_setattr: tracks whether the client has sent at least one valid
> mtime that can also update the ctime in a SETATTR.

Do we really need both of these?
Could we set dl_written on a write and clear it on a setattr that sets
mtime/ctime? 
Then on close, if it is still set we force an mtime update.

I would be inclined to put this bit in ->f_mode setting it precisely when
FMODE_NOCMTIME is uses to skip the update.  (There are 4 spare fmode bits).

But none of that need delay the current patchset which looks good to me
(though I haven't dug quite enough to give a Reviewed-by).

Thanks,
NeilBRown

> 
> When unlocking the lease for the delegation, clear FMODE_NOCMTIME. If
> the file has been written, but no setattr for the delegated mtime and
> ctime has been done, update the timestamps to current_time().
> 
> Suggested-by: NeilBrown <neil@...wn.name>
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
>  fs/nfsd/nfs4proc.c  | 26 ++++++++++++++++++++++++--
>  fs/nfsd/nfs4state.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  fs/nfsd/state.h     |  4 +++-
>  3 files changed, 69 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index aacd912a5fbe29ba5ccac206d13243308f36b7fa..bfebe6e25638a76d3607bb79a239bdc92e42e7b5 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1151,7 +1151,9 @@ vet_deleg_attrs(struct nfsd4_setattr *setattr, struct nfs4_delegation *dp)
>  	if (setattr->sa_bmval[2] & FATTR4_WORD2_TIME_DELEG_MODIFY) {
>  		if (nfsd4_vet_deleg_time(&iattr->ia_mtime, &dp->dl_mtime, &now)) {
>  			iattr->ia_ctime = iattr->ia_mtime;
> -			if (!nfsd4_vet_deleg_time(&iattr->ia_ctime, &dp->dl_ctime, &now))
> +			if (nfsd4_vet_deleg_time(&iattr->ia_ctime, &dp->dl_ctime, &now))
> +				dp->dl_setattr = true;
> +			else
>  				iattr->ia_valid &= ~(ATTR_CTIME | ATTR_CTIME_SET);
>  		} else {
>  			iattr->ia_valid &= ~(ATTR_CTIME | ATTR_CTIME_SET |
> @@ -1238,12 +1240,26 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	return status;
>  }
>  
> +static void nfsd4_file_mark_deleg_written(struct nfs4_file *fi)
> +{
> +	spin_lock(&fi->fi_lock);
> +	if (!list_empty(&fi->fi_delegations)) {
> +		struct nfs4_delegation *dp = list_first_entry(&fi->fi_delegations,
> +							      struct nfs4_delegation, dl_perfile);
> +
> +		if (dp->dl_type == OPEN_DELEGATE_WRITE_ATTRS_DELEG)
> +			dp->dl_written = true;
> +	}
> +	spin_unlock(&fi->fi_lock);
> +}
> +
>  static __be32
>  nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	    union nfsd4_op_u *u)
>  {
>  	struct nfsd4_write *write = &u->write;
>  	stateid_t *stateid = &write->wr_stateid;
> +	struct nfs4_stid *stid = NULL;
>  	struct nfsd_file *nf = NULL;
>  	__be32 status = nfs_ok;
>  	unsigned long cnt;
> @@ -1256,10 +1272,15 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	trace_nfsd_write_start(rqstp, &cstate->current_fh,
>  			       write->wr_offset, cnt);
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> -						stateid, WR_STATE, &nf, NULL);
> +						stateid, WR_STATE, &nf, &stid);
>  	if (status)
>  		return status;
>  
> +	if (stid) {
> +		nfsd4_file_mark_deleg_written(stid->sc_file);
> +		nfs4_put_stid(stid);
> +	}
> +
>  	write->wr_how_written = write->wr_stable_how;
>  	status = nfsd_vfs_write(rqstp, &cstate->current_fh, nf,
>  				write->wr_offset, &write->wr_payload,
> @@ -2550,6 +2571,7 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
>  	mutex_unlock(&ls->ls_mutex);
>  
>  	nfserr = ops->proc_layoutcommit(inode, rqstp, lcp);
> +	nfsd4_file_mark_deleg_written(ls->ls_stid.sc_file);
>  	nfs4_put_stid(&ls->ls_stid);
>  out:
>  	return nfserr;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 205ee8cc6fa2b9f74d08f7938b323d03bdf8286c..81fa7cc6c77b3cdc5ff22bc60ab0654f95dc258d 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1222,6 +1222,42 @@ static void put_deleg_file(struct nfs4_file *fp)
>  		nfs4_file_put_access(fp, NFS4_SHARE_ACCESS_READ);
>  }
>  
> +static void nfsd4_finalize_deleg_timestamps(struct nfs4_delegation *dp, struct file *f)
> +{
> +	struct iattr ia = { .ia_valid = ATTR_ATIME | ATTR_CTIME | ATTR_MTIME };
> +	struct inode *inode = file_inode(f);
> +	int ret;
> +
> +	/* don't do anything if FMODE_NOCMTIME isn't set */
> +	if ((READ_ONCE(f->f_mode) & FMODE_NOCMTIME) == 0)
> +		return;
> +
> +	spin_lock(&f->f_lock);
> +	f->f_mode &= ~FMODE_NOCMTIME;
> +	spin_unlock(&f->f_lock);
> +
> +	/* was it never written? */
> +	if (!dp->dl_written)
> +		return;
> +
> +	/* did it get a setattr for the timestamps at some point? */
> +	if (dp->dl_setattr)
> +		return;
> +
> +	/* Stamp everything to "now" */
> +	inode_lock(inode);
> +	ret = notify_change(&nop_mnt_idmap, f->f_path.dentry, &ia, NULL);
> +	inode_unlock(inode);
> +	if (ret) {
> +		struct inode *inode = file_inode(f);
> +
> +		pr_notice_ratelimited("Unable to update timestamps on inode %02x:%02x:%lu: %d\n",
> +					MAJOR(inode->i_sb->s_dev),
> +					MINOR(inode->i_sb->s_dev),
> +					inode->i_ino, ret);
> +	}
> +}
> +
>  static void nfs4_unlock_deleg_lease(struct nfs4_delegation *dp)
>  {
>  	struct nfs4_file *fp = dp->dl_stid.sc_file;
> @@ -1229,6 +1265,7 @@ static void nfs4_unlock_deleg_lease(struct nfs4_delegation *dp)
>  
>  	WARN_ON_ONCE(!fp->fi_delegees);
>  
> +	nfsd4_finalize_deleg_timestamps(dp, nf->nf_file);
>  	kernel_setlease(nf->nf_file, F_UNLCK, NULL, (void **)&dp);
>  	put_deleg_file(fp);
>  }
> @@ -6265,6 +6302,8 @@ nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
>  	memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
>  
>  	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> +		struct file *f = dp->dl_stid.sc_file->fi_deleg_file->nf_file;
> +
>  		if (!nfsd4_add_rdaccess_to_wrdeleg(rqstp, open, fh, stp) ||
>  				!nfs4_delegation_stat(dp, currentfh, &stat)) {
>  			nfs4_put_stid(&dp->dl_stid);
> @@ -6278,6 +6317,9 @@ nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
>  		dp->dl_atime = stat.atime;
>  		dp->dl_ctime = stat.ctime;
>  		dp->dl_mtime = stat.mtime;
> +		spin_lock(&f->f_lock);
> +		f->f_mode |= FMODE_NOCMTIME;
> +		spin_unlock(&f->f_lock);
>  		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
>  	} else {
>  		open->op_delegate_type = deleg_ts && nfs4_delegation_stat(dp, currentfh, &stat) ?
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index bf9436cdb93c5dd5502ecf83433ea311e3678711..b6ac0f37e9cdfcfddde5861c8c0c51bad60101b7 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -217,10 +217,12 @@ struct nfs4_delegation {
>  	struct nfs4_clnt_odstate *dl_clnt_odstate;
>  	time64_t		dl_time;
>  	u32			dl_type;
> -/* For recall: */
> +	/* For recall: */
>  	int			dl_retries;
>  	struct nfsd4_callback	dl_recall;
>  	bool			dl_recalled;
> +	bool			dl_written;
> +	bool			dl_setattr;
>  
>  	/* for CB_GETATTR */
>  	struct nfs4_cb_fattr    dl_cb_fattr;
> 
> -- 
> 2.50.1
> 
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ