Re: [PATCH] nfsd: accept a backdated timestamp from a delegation holder

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2026-09-01 at 11:19 -0400, Chuck Lever wrote:
> On Tue, Sep 1, 2026, at 9:09 AM, Jeff Layton wrote:
> > A client with an attribute delegation reports the file times in a
> > SETATTR at DELEGRETURN. nfsd ignores a time that moves backwards. It
> > 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 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.
> >   The ctime never moves backwards.
> > - 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 delegation 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")
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > Assisted-by: Claude:claude-opus-5
> 
> 
> Hi Jeff, LLM-generated review suggested the below finding is a blocker.
> 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index bb74eef43938..fa3a43c20e95 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> 
> [ ... ]
> 
> > @@ -1302,21 +1315,23 @@ vet_deleg_attrs(struct nfsd4_setattr *setattr, struct nfs4_delegation *dp)
> >  	struct timespec64 now = current_time(dp->dl_stid.sc_file->fi_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);
> > +	if (setattr->sa_bmval[2] & FATTR4_WORD2_TIME_DELEG_ACCESS)
> > +		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)) {
> > -			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);
> > -		}
> > +		clamp_deleg_time(&iattr->ia_mtime, &now);
> > +
> > +		/*
> > +		 * The ctime must not move backwards. Carry the mtime into it
> > +		 * only when that advances it; otherwise clear ATTR_CTIME_SET so
> > +		 * that notify_change() stamps the ctime with the current time,
> > +		 * which is what a local utimensat() would do.
> > +		 */
> > +		iattr->ia_ctime = iattr->ia_mtime;
> > +		if (!nfsd4_vet_deleg_time(&iattr->ia_ctime, &dp->dl_ctime, &now))
>                                                              ^^^^^^^^^^^^
> 
> Is dp->dl_ctime the right value to compare against here?  It is sampled
> once, when the delegation is granted, and nothing refreshes it after
> that:
> 
> fs/nfsd/nfs4state.c:nfs4_open_delegation() {
>     ...
> 	dp->dl_atime = stat.atime;
> 	dp->dl_ctime = stat.ctime;
> 	dp->dl_mtime = stat.mtime;
>     ...
> }
> 
> Is there anything that stops a client from sending more than one SETATTR
> with TIME_DELEG_MODIFY while it holds the delegation?
> 
> Say the first one carries an mtime T5 that is later than dl_ctime.
> nfsd4_vet_deleg_time() returns true, ATTR_CTIME_SET stays set, and the
> inode ctime becomes T5.
> 
> If a second SETATTR then backdates the mtime to T3, where dl_ctime < T3 <
> T5, the comparison is still against the grant-time dl_ctime.  It returns
> true again, so ATTR_CTIME_SET stays set and T3 is carried into ia_ctime.
> 
> notify_change() -> setattr_copy() -> inode_set_ctime_deleg() then drops
> that update, because T3 is older than the T5 already in the inode:
> 
> fs/inode.c:inode_set_ctime_deleg() {
>     ...
> 	/* If the update is older than the existing value, skip it. */
> 	if (timespec64_compare(&update, &cur_ts) <= 0)
> 		return cur_ts;
>     ...
> }
> 
> The mtime moves back to T3 and the ctime stays at T5, so the ctime is
> neither carried from the mtime nor stamped with the current time.  That
> looks different from the comment just above it, and from the commit
> message: "An mtime that moves backwards sets the ctime to the current
> time."
> 
> Would comparing against the inode's current ctime work better here?  The
> inode is already in hand for the current_time() call at the top of the
> function.
> 
> 

Yes, I think we do want to rework it to do that. Additionally, I
noticed another race condition that we ought to fix while we're in
here.

I'll be sending a v2, but I think we have to push some of this handling
into the VFS layer so that it's done under more consistent locking and
properly uses the mgtime infrastructure.

-- 
Jeff Layton <jlayton@kernel.org>