[PATCH v3 0/3] fs/nfsd: accept a backdated timestamp from a delegation holder

[PATCH v3 0/3] fs/nfsd: accept a backdated timestamp from a delegation holder

[Jeff Layton]

This version fixes a couple of issues that Sashiko's review pointed out.

The main problem is that an attribute delegation has strict rules about
updates can be done, and that prevents applications that backdate
timestamps on files after writing them, (e.g. tar -x, cp -p, etc.) from
setting them properly.

We could fix this on the client by making it always issue an RPC for the
SETATTR, but that would harm performance in these common use-cases.
This set allows the server to accept some of these backdated mtime
updates, by changing how the ctime is handled.

It'd be nice to see this in v7.4.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v3:
- Compare after the clamp and truncate in inode_set_ctime_deleg(), not
  before, so a clamped update stamps too
- Compare a SETATTR report against the inode before clamping, so an
  untouched file with a future time keeps it
- Set dl_setattr only once nfsd_setattr() has applied the update, and
  hold the stateid reference until then
- Note the SETATTR/DELEGRETURN race on dl_setattr in a comment
- Link to v2: https://lore.kernel.org/r/20260902-delegts-v2-0-383cb289ce88@kernel.org

Changes in v2:
- Split the change into three patches
- Stamp now in inode_set_ctime_deleg() for a stale update, so nfsd need
  not compare
- Drop a SETATTR report that matches the inode, so an untouched file
  keeps its change attribute
- Set dl_setattr only on the branch that carries a c/mtime update
- Compare against the inode, not the grant-time snapshots (Chuck)
- Set the ctime whenever a CB_GETATTR moves the mtime
- Take i_rwsem before the CB_GETATTR comparison
- Drop the unused dl_atime, dl_mtime and dl_ctime, and make
  nfsd4_vet_deleg_time() static
- Link to v1: https://lore.kernel.org/r/20260901-delegts-v1-1-9937f7ee4370@kernel.org

---
Jeff Layton (3):
      fs: stamp the current time for a stale delegated ctime update
      nfsd: accept a backdated timestamp from a delegation holder
      nfsd: compare CB_GETATTR times against the inode

 fs/inode.c          | 25 +++++++++------
 fs/nfsd/nfs4proc.c  | 92 +++++++++++++++++++++++++++++++++++++++++------------
 fs/nfsd/nfs4state.c | 49 +++++++++++++++++-----------
 fs/nfsd/state.h     |  8 -----
 4 files changed, 119 insertions(+), 55 deletions(-)
---
base-commit: ac579868af0c900d8fd9c8cfe9e10db46ccc5a75
change-id: 20260901-delegts-e91a648f1427

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v3 1/3] fs: stamp the current time for a stale delegated ctime update

[Jeff Layton]

inode_set_ctime_deleg() drops an update that does not advance the ctime.
That is correct when a client reports a timestamp that it advanced on its
own. It is wrong when the client moves the timestamp backwards on purpose,
as utimensat() does.

Stamp the current time in that case. The ctime still never moves backwards.

Fixes: 3952f1cbcbc4 ("nfsd: fix SETATTR updates for delegated timestamps")
Assisted-by: LLM
Reviewed-by: Jan Kara <jack@suse.cz>
Acked-by: Chuck Lever <cel@kernel.org>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/inode.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index ba7da39be4a3..09eac2e0e172 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2959,11 +2959,17 @@ EXPORT_SYMBOL(inode_set_ctime_current);
  * inode attributes, including the mtime. When updating the mtime, update
  * the ctime to a value at least equal to that.
  *
- * This can race with concurrent updates to the inode, in which
- * case the update is skipped.
+ * The ctime never moves backwards. An @update that does not advance the ctime
+ * records the current time instead, so that the delegated change is still
+ * visible in the ctime.
+ *
+ * This can still race with a concurrent update to the inode. That stamp takes
+ * precedence, and is at least as recent as the one it displaces.
  *
  * Note that this works even when multigrain timestamps are not enabled,
  * so it is used in either case.
+ *
+ * Returns the resulting ctime.
  */
 struct timespec64 inode_set_ctime_deleg(struct inode *inode, struct timespec64 update)
 {
@@ -2975,10 +2981,6 @@ struct timespec64 inode_set_ctime_deleg(struct inode *inode, struct timespec64 u
 	cur_ts.tv_nsec = cur & ~I_CTIME_QUERIED;
 	cur_ts.tv_sec = inode_get_ctime_sec(inode);
 
-	/* If the update is older than the existing value, skip it. */
-	if (timespec64_compare(&update, &cur_ts) <= 0)
-		return cur_ts;
-
 	ktime_get_coarse_real_ts64_mg(&now);
 
 	/* Clamp the update to "now" if it's in the future */
@@ -2987,9 +2989,14 @@ struct timespec64 inode_set_ctime_deleg(struct inode *inode, struct timespec64 u
 
 	update = timestamp_truncate(update, inode);
 
-	/* No need to update if the values are already the same */
-	if (timespec64_equal(&update, &cur_ts))
-		return cur_ts;
+	/*
+	 * The update does not advance the ctime. Stamp the current time, so
+	 * that the delegated change is still visible in the ctime. Compare
+	 * after clamping and truncating, since either can pull an update that
+	 * was ahead of the ctime back onto it.
+	 */
+	if (timespec64_compare(&update, &cur_ts) <= 0)
+		return inode_set_ctime_current(inode);
 
 	/*
 	 * Try to swap the nsec value into place. If it fails, that means

-- 
2.55.0

[PATCH v3 2/3] nfsd: accept a backdated timestamp from a delegation holder

[Jeff Layton]

A client with an attribute delegation reports the file times in a
SETATTR at DELEGRETURN. nfsd ignores a time that moves backwards. If the
file was written, nfsd4_finalize_deleg_timestamps() then stamps the
c/mtime with the current time, so the file keeps the DELEGRETURN time.
cp -p, rsync -t and tar -x lose timestamps. The SETATTR returns NFS4_OK.

The client applies an explicit utimensat() to its own inode. It sends no
SETATTR while it holds the delegation, so the backdated value reaches
nfsd only as TIME_DELEG_MODIFY. The client can send an RPC for each time
change instead. That also works, but it loses the caching that the
delegation allows.

The delegation makes the client the authority for these times, so treat
its SETATTR as a statement of fact. The client can set the same value
with an ordinary SETATTR, which nfsd applies without a check.

- nfsd accepts a backwards atime or mtime.
- An mtime that moves backwards sets the ctime to the current time.
  inode_set_ctime_deleg() does that, so nfsd does not compare against the
  ctime here. The ctime never moves backwards.
- The client reports the times at every DELEGRETURN, changed or not. Drop
  a report that already matches the inode, and leave the ctime alone when
  neither the mtime nor the data moved. Otherwise an untouched file gets a
  new change attribute every time a delegation comes back and every other
  client drops its cache, notify_change() runs for nothing, and a holder
  that does not own the file gets -EPERM out of setattr_prepare() where it
  used to get NFS4_OK.
- Compare against the inode before clamping. A file can carry a time in the
  future, set by an ordinary SETATTR, which nfsd does not clamp. Clamping
  first makes an unchanged report differ from the inode and drags the time
  back to "now".
- dl_setattr stops nfsd4_finalize_deleg_timestamps() from stamping over the
  reported times, so only the branch that carries a c/mtime update may set
  it, and only once nfsd_setattr() has applied it. A write that follows a
  no-op SETATTR would otherwise lose its timestamps, and so would one that
  precedes a failed SETATTR. Hold the stateid reference until then.
- nfsd still clamps a future time.
- The CB_GETATTR path keeps the old rule. A backwards time there shows
  a stale report.

RFC 9754 says that the server ignores a time before the original time.
This patch does not follow that sentence. The same section also says
that the server MUST accept the change or MUST reject it with
NFS4ERR_DELAY. A silent discard does neither. A retry after
NFS4ERR_DELAY carries the same backdated value, so that option cannot
succeed.

There is still one gap: nfsd cannot tell an explicit utimensat() from a
report of a write. An mtime after the ctime and before the current time
therefore sets the ctime to that mtime, instead of to "now". RFC 9754
requires this. Fixing that would require the client to issue an RPC for
the mtime.

Fixes: 3952f1cbcbc4 ("nfsd: fix SETATTR updates for delegated timestamps")
Assisted-by: LLM
Acked-by: Chuck Lever <cel@kernel.org>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4proc.c | 92 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 72 insertions(+), 20 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index bb74eef43938..edab1144016a 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1292,32 +1292,69 @@ nfsd4_secinfo_no_name_release(union nfsd4_op_u *u)
 }
 
 /*
- * Validate that the requested timestamps are within the acceptable range. If
- * timestamp appears to be in the future, then it will be clamped to
- * current_time().
+ * A client holding a delegation with delegated timestamps is the authority for
+ * the file's timestamps, so a SETATTR from it asserts what they are rather than
+ * reporting that they have advanced. Honor a value that moves a timestamp
+ * backwards: the client could set the same value with an ordinary SETATTR, so
+ * refusing it here only loses data. Clamp a value in the future to the current
+ * time, as RFC 9754 permits.
  */
 static void
+clamp_deleg_time(struct timespec64 *req, const struct timespec64 *now)
+{
+	if (timespec64_compare(req, now) > 0)
+		*req = *now;
+}
+
+/*
+ * Apply the timestamps that a delegation holder supplied in a SETATTR.
+ *
+ * Returns true if the request carries a c/mtime update, so that the caller can
+ * set dl_setattr once the update has been applied.
+ */
+static bool
 vet_deleg_attrs(struct nfsd4_setattr *setattr, struct nfs4_delegation *dp)
 {
-	struct timespec64 now = current_time(dp->dl_stid.sc_file->fi_inode);
+	struct inode *inode = dp->dl_stid.sc_file->fi_inode;
+	struct timespec64 now = current_time(inode);
 	struct iattr *iattr = &setattr->sa_iattr;
 
-	if ((setattr->sa_bmval[2] & FATTR4_WORD2_TIME_DELEG_ACCESS) &&
-	    !nfsd4_vet_deleg_time(&iattr->ia_atime, &dp->dl_atime, &now))
-		iattr->ia_valid &= ~(ATTR_ATIME | ATTR_ATIME_SET);
+	/*
+	 * The client reports the times at every DELEGRETURN, changed or not.
+	 * Drop a report that matches the inode. An untouched file then keeps its
+	 * change attribute, and nfsd_setattr() skips the call into the
+	 * filesystem. Compare before clamping: a file can carry a time in the
+	 * future, and clamping first would make the report differ from the inode
+	 * and drag the time back to "now".
+	 *
+	 * The times are read without i_rwsem. A conflicting writer must break
+	 * the delegation first, and FMODE_NOCMTIME stops the holder's own writes
+	 * from stamping the c/mtime. touch_atime() and a CB_GETATTR can still
+	 * move them here. A stale read then costs at most one extra update.
+	 */
+	if (setattr->sa_bmval[2] & FATTR4_WORD2_TIME_DELEG_ACCESS) {
+		struct timespec64 atime = inode_get_atime(inode);
+
+		if (timespec64_equal(&iattr->ia_atime, &atime))
+			iattr->ia_valid &= ~(ATTR_ATIME | ATTR_ATIME_SET);
+		else
+			clamp_deleg_time(&iattr->ia_atime, &now);
+	}
 
 	if (setattr->sa_bmval[2] & FATTR4_WORD2_TIME_DELEG_MODIFY) {
-		if (nfsd4_vet_deleg_time(&iattr->ia_mtime, &dp->dl_mtime, &now)) {
+		struct timespec64 mtime = inode_get_mtime(inode);
+
+		if (dp->dl_written ||
+		    !timespec64_equal(&iattr->ia_mtime, &mtime)) {
+			clamp_deleg_time(&iattr->ia_mtime, &now);
 			iattr->ia_ctime = iattr->ia_mtime;
-			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 |
-					     ATTR_MTIME | ATTR_MTIME_SET);
+			return true;
 		}
+
+		iattr->ia_valid &= ~(ATTR_CTIME | ATTR_CTIME_SET |
+				     ATTR_MTIME | ATTR_MTIME_SET);
 	}
+	return false;
 }
 
 static __be32
@@ -1331,7 +1368,8 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		.na_pacl	= posix_acl_dup(setattr->sa_pacl),
 		.na_dpacl	= posix_acl_dup(setattr->sa_dpacl),
 	};
-	bool save_no_wcc, deleg_attrs;
+	bool save_no_wcc, deleg_attrs, deleg_cmtime = false;
+	struct nfs4_delegation *dp = NULL;
 	struct nfs4_stid *st = NULL;
 	struct inode *inode;
 	__be32 status = nfs_ok;
@@ -1356,17 +1394,15 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (deleg_attrs) {
 		status = nfserr_bad_stateid;
 		if (st && (st->sc_type & SC_TYPE_DELEG)) {
-			struct nfs4_delegation *dp = delegstateid(st);
+			dp = delegstateid(st);
 
 			/* Only for *_ATTRS_DELEG flavors */
 			if (deleg_attrs_deleg(dp->dl_type)) {
-				vet_deleg_attrs(setattr, dp);
+				deleg_cmtime = vet_deleg_attrs(setattr, dp);
 				status = nfs_ok;
 			}
 		}
 	}
-	if (st)
-		nfs4_put_stid(st);
 	if (status)
 		goto out_err;
 
@@ -1396,6 +1432,20 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	cstate->current_fh.fh_no_wcc = true;
 	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs, NULL);
 	cstate->current_fh.fh_no_wcc = save_no_wcc;
+
+	/*
+	 * The times are in place, so keep nfsd4_finalize_deleg_timestamps() from
+	 * stamping over them. Set this only once the update has been applied: a
+	 * failed SETATTR that set it would suppress the fallback stamp at
+	 * DELEGRETURN and lose the timestamps of an earlier write for good.
+	 *
+	 * A DELEGRETURN that races this SETATTR can read dl_setattr before it is
+	 * set and stamp "now" over the times just applied. Only a client that
+	 * pipelines the two can hit that, and it lands on the old behavior.
+	 */
+	if (!status && deleg_cmtime)
+		dp->dl_setattr = true;
+
 	if (!status)
 		status = nfserrno(attrs.na_labelerr);
 	if (!status)
@@ -1405,6 +1455,8 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 out:
 	fh_drop_write(&cstate->current_fh);
 out_err:
+	if (st)
+		nfs4_put_stid(st);
 	nfsd_attrs_free(&attrs);
 	return status;
 }

-- 
2.55.0

[PATCH v3 3/3] nfsd: compare CB_GETATTR times against the inode

[Jeff Layton]

cb_getattr_update_times() vets the times that a client reports in a
CB_GETATTR against dl_atime, dl_mtime and dl_ctime. nfsd samples those
three when it grants the delegation, and nothing refreshes them. An
earlier CB_GETATTR or SETATTR can move the inode past them.

The vetting then compares against a stale value and accepts a report
that it must reject. setattr_copy() applies the atime and the mtime with
no check of the current value, so both move backwards. That is the
result the vetting exists to prevent.

Compare against the inode instead. Take the inode lock before the
comparison, so that a setattr from a concurrent delegation break cannot
land before notify_change() runs.

With this change, there is no longer a need to keep dl_atime/mtime/ctime
in the delegation. Drop those fields as well. That also drops a read of
an uninitialized struct kstat: the read-delegation arm assigned
stat.atime to dl_atime even when nfs4_delegation_stat() had not run.
Nothing consumed the value, since dl_atime is only read under
deleg_attrs_deleg().

Fixes: 3952f1cbcbc4 ("nfsd: fix SETATTR updates for delegated timestamps")
Assisted-by: LLM
Acked-by: Chuck Lever <cel@kernel.org>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 49 +++++++++++++++++++++++++++++++------------------
 fs/nfsd/state.h     |  8 --------
 2 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ae0af9490fb1..b2b8d3a8030b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7307,9 +7307,6 @@ nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
 		open->op_delegate_type = deleg_ts ? OPEN_DELEGATE_WRITE_ATTRS_DELEG :
 						    OPEN_DELEGATE_WRITE;
 		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
-		dp->dl_atime = stat.atime;
-		dp->dl_ctime = stat.ctime;
-		dp->dl_mtime = stat.mtime;
 		spin_lock(&f->f_lock);
 		if (deleg_ts)
 			f->f_mode |= FMODE_NOCMTIME;
@@ -7318,7 +7315,6 @@ nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
 	} else {
 		open->op_delegate_type = deleg_ts && nfs4_delegation_stat(dp, currentfh, &stat) ?
 					 OPEN_DELEGATE_READ_ATTRS_DELEG : OPEN_DELEGATE_READ;
-		dp->dl_atime = stat.atime;
 		trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
 	}
 	nfs4_put_stid(&dp->dl_stid);
@@ -10447,7 +10443,7 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
 /**
  * nfsd4_vet_deleg_time - vet and set the timespec for a delegated timestamp update
  * @req: timestamp from the client
- * @orig: original timestamp in the inode
+ * @cur: current timestamp in the inode
  * @now: current time
  *
  * Given a timestamp from the client response, check it against the
@@ -10455,15 +10451,17 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
  * if the inode's timestamp needs to be updated, and false otherwise.
  * @req may also be changed if the timestamp needs to be clamped.
  */
-bool nfsd4_vet_deleg_time(struct timespec64 *req, const struct timespec64 *orig,
-			  const struct timespec64 *now)
+static bool nfsd4_vet_deleg_time(struct timespec64 *req,
+				 const struct timespec64 *cur,
+				 const struct timespec64 *now)
 {
-
 	/*
-	 * "When the time presented is before the original time, then the
-	 *  update is ignored." Also no need to update if there is no change.
+	 * RFC 9754 has the server ignore a time that is before the original
+	 * one. Compare against the value in the inode rather than the original:
+	 * an earlier CB_GETATTR or SETATTR may have moved it, and a report that
+	 * does not advance it is stale.
 	 */
-	if (timespec64_compare(req, orig) <= 0)
+	if (timespec64_compare(req, cur) <= 0)
 		return false;
 
 	/*
@@ -10484,30 +10482,45 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
 	struct iattr attrs = { };
 	int ret;
 
+	/*
+	 * Take the inode lock first, so that a setattr from a delegation break
+	 * cannot land between the comparison and notify_change(). The atime can
+	 * still move under us: touch_atime() takes no lock, and FMODE_NOCMTIME
+	 * does not cover it.
+	 */
+	inode_lock(inode);
+
 	if (deleg_attrs_deleg(dp->dl_type)) {
 		struct timespec64 now = current_time(inode);
+		struct timespec64 atime = inode_get_atime(inode);
+		struct timespec64 mtime = inode_get_mtime(inode);
 
 		attrs.ia_atime = ncf->ncf_cb_atime;
 		attrs.ia_mtime = ncf->ncf_cb_mtime;
 
-		if (nfsd4_vet_deleg_time(&attrs.ia_atime, &dp->dl_atime, &now))
+		if (nfsd4_vet_deleg_time(&attrs.ia_atime, &atime, &now))
 			attrs.ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
 
-		if (nfsd4_vet_deleg_time(&attrs.ia_mtime, &dp->dl_mtime, &now)) {
-			attrs.ia_valid |= ATTR_MTIME | ATTR_MTIME_SET;
+		/*
+		 * A change to the mtime must show in the ctime.
+		 * inode_set_ctime_deleg() takes the mtime when it advances the
+		 * ctime, and stamps the current time otherwise.
+		 */
+		if (nfsd4_vet_deleg_time(&attrs.ia_mtime, &mtime, &now)) {
+			attrs.ia_valid |= ATTR_MTIME | ATTR_MTIME_SET |
+					  ATTR_CTIME | ATTR_CTIME_SET;
 			attrs.ia_ctime = attrs.ia_mtime;
-			if (nfsd4_vet_deleg_time(&attrs.ia_ctime, &dp->dl_ctime, &now))
-				attrs.ia_valid |= ATTR_CTIME | ATTR_CTIME_SET;
 		}
 	} else {
 		attrs.ia_valid |= ATTR_MTIME | ATTR_CTIME;
 	}
 
-	if (!attrs.ia_valid)
+	if (!attrs.ia_valid) {
+		inode_unlock(inode);
 		return 0;
+	}
 
 	attrs.ia_valid |= ATTR_DELEG;
-	inode_lock(inode);
 	ret = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
 	inode_unlock(inode);
 	return ret;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index cd9294f024bb..e7fe2af45f53 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -330,11 +330,6 @@ struct nfs4_delegation {
 		struct nfsd4_cb_notify	dl_cb_notify;
 	};
 
-	/* For delegated timestamps */
-	struct timespec64	dl_atime;
-	struct timespec64	dl_mtime;
-	struct timespec64	dl_ctime;
-
 	/* For dir delegations */
 	u32			dl_notify_mask;
 	u32			dl_child_attrs[2];
@@ -357,9 +352,6 @@ static inline bool deleg_attrs_deleg(u32 dl_type)
 	       dl_type == OPEN_DELEGATE_WRITE_ATTRS_DELEG;
 }
 
-bool nfsd4_vet_deleg_time(struct timespec64 *cb, const struct timespec64 *orig,
-			  const struct timespec64 *now);
-
 #define cb_to_delegation(cb) \
 	container_of(cb, struct nfs4_delegation, dl_recall)
 

-- 
2.55.0