Re: [PATCH v1] NFSD: Decode NFSv4 birth time attribute

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2022-07-11 at 14:29 +0000, Chuck Lever III wrote:
> 
> > On Jul 11, 2022, at 7:36 AM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Sun, 2022-07-10 at 14:46 -0400, Chuck Lever wrote:
> > > NFSD has advertised support for the NFSv4 time_create attribute
> > > since commit e377a3e698fb ("nfsd: Add support for the birth time
> > > attribute").
> > > 
> > > Igor Mammedov reports that Mac OS clients attempt to set the NFSv4
> > > birth time attribute via OPEN(CREATE) and SETATTR if the server
> > > indicates that it supports it, but since the above commit was
> > > merged, those attempts now fail.
> > > 
> > > Table 5 in RFC 8881 lists the time_create attribute as one that can
> > > be both set and retrieved, but the above commit did not add server
> > > support for clients to provide a time_create attribute. IMO that's
> > > a bug in our implementation of the NFSv4 protocol, which this commit
> > > addresses.
> > > 
> > > Whether NFSD silently ignores the new birth time or actually sets it
> > > is another matter. I haven't found another filesystem service in the
> > > Linux kernel that enables users or clients to modify a file's birth
> > > time attribute.
> > > 
> > > This commit reflects my (perhaps incorrect) understanding of whether
> > > Linux users can set a file's birth time. NFSD will now recognize a
> > > time_create attribute but it ignores its value. It clears the
> > > time_create bit in the returned attribute bitmask to indicate that
> > > the value was not used.
> > > 
> > > Reported-by: Igor Mammedov <imammedo@redhat.com>
> > > Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute")
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > ---
> > > fs/nfsd/nfs4xdr.c |    9 +++++++++
> > > fs/nfsd/nfsd.h    |    3 ++-
> > > 2 files changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > > index 61b2aae81abb..2acea7792bb2 100644
> > > --- a/fs/nfsd/nfs4xdr.c
> > > +++ b/fs/nfsd/nfs4xdr.c
> > > @@ -470,6 +470,15 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
> > > 			return nfserr_bad_xdr;
> > > 		}
> > > 	}
> > > +	if (bmval[1] & FATTR4_WORD1_TIME_CREATE) {
> > > +		struct timespec64 ts;
> > > +
> > > +		/* No Linux filesystem supports setting this attribute. */
> > > +		bmval[1] &= ~FATTR4_WORD1_TIME_CREATE;
> > > +		status = nfsd4_decode_nfstime4(argp, &ts);
> > > +		if (status)
> > > +			return status;
> > > +	}
> > > 	if (bmval[1] & FATTR4_WORD1_TIME_MODIFY_SET) {
> > > 		u32 set_it;
> > > 
> > > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > > index 847b482155ae..9a8b09afc173 100644
> > > --- a/fs/nfsd/nfsd.h
> > > +++ b/fs/nfsd/nfsd.h
> > > @@ -465,7 +465,8 @@ static inline bool nfsd_attrs_supported(u32 minorversion, const u32 *bmval)
> > > 	(FATTR4_WORD0_SIZE | FATTR4_WORD0_ACL)
> > > #define NFSD_WRITEABLE_ATTRS_WORD1 \
> > > 	(FATTR4_WORD1_MODE | FATTR4_WORD1_OWNER | FATTR4_WORD1_OWNER_GROUP \
> > > -	| FATTR4_WORD1_TIME_ACCESS_SET | FATTR4_WORD1_TIME_MODIFY_SET)
> > > +	| FATTR4_WORD1_TIME_ACCESS_SET | FATTR4_WORD1_TIME_CREATE \
> > > +	| FATTR4_WORD1_TIME_MODIFY_SET)
> > > #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> > > #define MAYBE_FATTR4_WORD2_SECURITY_LABEL \
> > > 	FATTR4_WORD2_SECURITY_LABEL
> > > 
> > > 
> > 
> > RFC5661 lists time_create as being writeable, so silently ignoring it
> > seems wrong.
> 
> Open for debate. The protocol does allow a SETATTR. But the
> specification doesn't have much else to say about the semantics
> of time_create; contrast that with mtime or ctime.
> 
> 
> > It seems like we ought to have nfsd attempt to set the
> > btime and then just return an error if it doesn't work...
> 
> The usual way the NFSv4 protocol handles this is that the
> attribute's bit in the returned bitmask is cleared, so that
> the rest of the COMPOUND is able to succeed. There's no
> NFS4ERR status code in this case.
> 
> 
> > but, I don't
> > see a mechanism in the kernel for setting it. ATTR_BTIME doesn't exist,
> > for instance.
> 
> That's what I observed: there doesn't seem to be a mechanism in
> Linux for setting it. Perhaps I should have copied fsdevel.
> 
> 
> > Still, since we can't set it, returning an error there seems more
> > correct. NFS4ERR_INVAL is probably the wrong one -- maybe
> > NFS4ERR_NOTSUPP ? It's a bit weird since we do support querying it, but
> > not setting it. Maybe we need to propose a new NFS4ERR_ATTR_RO ?
> 
> As I said above, the protocol's way of dealing with it is to
> clear the attribute's bit in the returned attribute bitmask.
> "You asked me to set this attribute, but I didn't". Clients,
> IMO, will be more prepared to deal with that than having
> all of their OPENs fail with NFS4ERR_NOTSUPP.
> 
> IMO explicitly setting a file's birth time doesn't seem quite
> kosher, and it's not a POSIX attribute anyway, so we don't
> have a standard to cleave to here (at least one that I'm aware
> of). I'm fine with the patch as it stands, but I'm open to
> hear more opinions about this.
> 
> 

Ok, now that I looked over the SETATTR part of the spec, I agree. Just
clearing the bit in the "attrsset" mask should do the right thing, and
that's probably better than returning an error.

Reviewed-by: Jeff Layton <jlayton@kernel.org>