Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
CC:     Igor Mammedov <imammedo@redhat.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1] NFSD: Decode NFSv4 birth time attribute
Thread-Topic: [PATCH v1] NFSD: Decode NFSv4 birth time attribute
Thread-Index: AQHYlI1Yu1IMkeKgwEG1/w/Jb2AUD615C+wAgAAwWQCAAASvAIAAAsyA
Date:   Mon, 11 Jul 2022 14:56:42 +0000
Message-ID: <17D3289B-4148-4EBB-9371-E10FCD796339@oracle.com>
References: <165747876458.1259.8334435718280903102.stgit@bazille.1015granger.net>
 <531053e36e291fc5d99bb766e76d52b0333ecc94.camel@kernel.org>
 <26FF2E45-1463-4923-8B17-CEB4441D774A@oracle.com>
 <cc71fab34310e40df01022bfce78e5ac501fb53d.camel@kernel.org>
In-Reply-To: <cc71fab34310e40df01022bfce78e5ac501fb53d.camel@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?rNgQwaDrDWhRQb6lOWcTYy0NCl55H0JTBta8/oqRQx+tEBRwfybP+jkLrS+L?=
 =?us-ascii?Q?+MVoxdJr/wwzyAJ86cwEboDfWemrgBvdHMHdXAqLWOrymWTBXsk7Rb5uY5Pj?=
 =?us-ascii?Q?RG65TYpEGFNV0mZH51KouLD8FlhaLGazBL9cvOQaXhiyPjrrxTm79I8q99DT?=
 =?us-ascii?Q?WX0azW3/S8SfOKPIhFXXDZeXqfNUxUmpd3iaORaGwjqPY0mGGcZWTxOJtJh6?=
 =?us-ascii?Q?45gvrqJxpdZqsA1XfqCH/H1h+4CNVCJ5E0egRjHF5hTDP0vaJsplFSO/iy1D?=
 =?us-ascii?Q?dCv0R2CCMDDArd0oNm8C1vnfa1xh2N1/rkfwpngaEeqTFlLxEa16rP55wIsZ?=
 =?us-ascii?Q?Zc609tH9W2XjrKR8GUdPN5tHMF8EpcBF4uoShHFt0xnha6GK36+mQtp652qt?=
 =?us-ascii?Q?xmCWBydG8P29WUTrSUcUgPu1m3l8/ATHPTD3wwjQxaQL+/HQZCjbhScv4pB3?=
 =?us-ascii?Q?wqvWe8HDR1QuBCrLoUzqtxHznOyeW4uDm//4duEblyoyHiaNfstjG0pPegTQ?=
 =?us-ascii?Q?qttRUJYjY4awsGmu0PZlm1m9TPoJasp/Xa7cN8EVTO5SIZ5Pifm0L7iMwxFj?=
 =?us-ascii?Q?rECnJePCDmDvpuaTmp1rtyt/2L8j7Bxj/FM2nwE3/8xzARF7B6SzQmN30vz+?=
 =?us-ascii?Q?kAMsKU0x6WRjpJ9kE7tjU0UYB2Y+m7pycHxwtDiUgYeWBqBWEG+8pQRll7pj?=
 =?us-ascii?Q?vukNe8ZH6cqf0SeaRbLzsWaYneqRrZGZczJ4qqFWHhgi4QG7rv0T5iVyfUv5?=
 =?us-ascii?Q?JNd7J1EKJhZZp4ofIxnh62rvQuSaOBL1sF22YRLJijMvh9oOwZzSf0kh9fwE?=
 =?us-ascii?Q?09LN2da5HU6yTkRVY7dKb5GH0C+4yPsCtpSHoIj8AWbYMy3/mRZ5LpEnd/V1?=
 =?us-ascii?Q?m00N/BXOUzcCqBO0Pl5SxHd3kS/WE7EbmNQMD5wjRY7lfI+INSPc7aZz8xkd?=
 =?us-ascii?Q?538mJIWfZl87fr5wwUZMZ0j1JlfXYjekRB59aOHFz8Fr7y093xhuTYLWuukV?=
 =?us-ascii?Q?WBvC/BAInRgB1gobiuwQoLCqIbq/UQ0M833Tt16I3lJ4FlGvuKdjX3QKUGxD?=
 =?us-ascii?Q?4Ta9kEnIjo4Ecrn/OG5w8OHVHzuM35OhqPVjrKaCLeTE+4FiXKNcs5LkegQ5?=
 =?us-ascii?Q?M9FMNNiLEwoZPqHIweMSHB3MFdQfAUnxzd1lyuG3K5HhhOuWvNgxQdN5Q9hh?=
 =?us-ascii?Q?Xk9gO9GLEofboRYr9MAFSeieU6P+SDR2gZoBqGYX348+O5ZCUpV2mQJMrdKM?=
 =?us-ascii?Q?xtvNIJQEYJ3Jbi1Ah/WH7bL3NNt6V8ePH3yWJe0gm/tHmVX4abO22vwlJ0C+?=
 =?us-ascii?Q?XWptK+kL1pU1k6FT0mMbLRv2gC06kpjyG2jDt9rVh28teDqiMM2eY2wFCNiq?=
 =?us-ascii?Q?bqTgJ4Cj+uYuqoFsQDpl8cpRXZzlCkKuDc85g9/whDkDWmT5HaUYpihVjus+?=
 =?us-ascii?Q?zIZ9+qagegkSnfOlIXHc4+VZ2tv2eE1BJk2XWbRWUBSUMYXWrHiUhGPJ/FA6?=
 =?us-ascii?Q?nnCvnEbj/Ic5Y4EzFuFoGpzO5fup6pkurU4IpBfWC6Nf66S0zyG00BZNnp/y?=
 =?us-ascii?Q?p4LwR5WvpkHeSWtAlbJ/0ogrfXvq4bRfDHzTF/3L7mjN3S4jVQ+hPILCeF5X?=
 =?us-ascii?Q?xQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <0587375D7EDF0C46AC67476A25B3E10B@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 744d3859-efda-4059-590e-08da634d9110
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Jul 2022 14:56:42.3433
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: dmt1KMZUcHhJdNYBL2S1RfyDJC2fUWPCx7KkhjTLikEfcaKOgShYmqJ6Cxfh3lNnyX2VhRO/pn4nfbhpK91iow==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3719
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.883
 definitions=2022-07-11_19:2022-07-08,2022-07-11 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 phishscore=0 mlxscore=0
 suspectscore=0 adultscore=0 spamscore=0 mlxlogscore=999 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2207110063
X-Proofpoint-GUID: k27r0OC5_8DwaYjQEY5o6i2j5XEhG6xe
X-Proofpoint-ORIG-GUID: k27r0OC5_8DwaYjQEY5o6i2j5XEhG6xe
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jul 11, 2022, at 10:46 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Mon, 2022-07-11 at 14:29 +0000, Chuck Lever III wrote:
>>=20
>>> On Jul 11, 2022, at 7:36 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>>=20
>>> 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").
>>>>=20
>>>> 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.
>>>>=20
>>>> 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.
>>>>=20
>>>> 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.
>>>>=20
>>>> 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.
>>>>=20
>>>> 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(-)
>>>>=20
>>>> 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 *ar=
gp, 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] &=3D ~FATTR4_WORD1_TIME_CREATE;
>>>> +		status =3D nfsd4_decode_nfstime4(argp, &ts);
>>>> +		if (status)
>>>> +			return status;
>>>> +	}
>>>> 	if (bmval[1] & FATTR4_WORD1_TIME_MODIFY_SET) {
>>>> 		u32 set_it;
>>>>=20
>>>> 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 minorv=
ersion, 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
>>>>=20
>>>>=20
>>>=20
>>> RFC5661 lists time_create as being writeable, so silently ignoring it
>>> seems wrong.
>>=20
>> 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.
>>=20
>>=20
>>> It seems like we ought to have nfsd attempt to set the
>>> btime and then just return an error if it doesn't work...
>>=20
>> 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.
>>=20
>>=20
>>> but, I don't
>>> see a mechanism in the kernel for setting it. ATTR_BTIME doesn't exist,
>>> for instance.
>>=20
>> That's what I observed: there doesn't seem to be a mechanism in
>> Linux for setting it. Perhaps I should have copied fsdevel.
>>=20
>>=20
>>> 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 ?
>>=20
>> 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.
>>=20
>> 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.
>>=20
>>=20
>=20
> 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>

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