> On Jul 11, 2022, at 10:46 AM, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>> Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute")
>>>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxx>
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
