Message-ID: <17D3289B-4148-4EBB-9371-E10FCD796339@oracle.com>
Date:   Mon, 11 Jul 2022 14:56:42 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     Jeff Layton <jlayton@...nel.org>
CC:     Igor Mammedov <imammedo@...hat.com>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] NFSD: Decode NFSv4 birth time attribute



> On Jul 11, 2022, at 10:46 AM, Jeff Layton <jlayton@...nel.org> wrote:
> 
> On Mon, 2022-07-11 at 14:29 +0000, Chuck Lever III wrote:
>> 
>>> On Jul 11, 2022, at 7:36 AM, Jeff Layton <jlayton@...nel.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@...hat.com>
>>>> Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute")
>>>> Signed-off-by: Chuck Lever <chuck.lever@...cle.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@...nel.org>

Thanks for your review!

Just because I'm not 100% certain of this choice, I will hang
onto it for a few more days. Also, Igor, please feel free to try
this out and reply with Tested-by. I do have Monterey here to
do some simple testing, but the more the merrier.

--
Chuck Lever

Powered by blists - more mailing lists