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;
Message-ID: <531053e36e291fc5d99bb766e76d52b0333ecc94.camel@kernel.org>
Subject: Re: [PATCH v1] NFSD: Decode NFSv4 birth time attribute
From:   Jeff Layton <jlayton@kernel.org>
To:     Chuck Lever <chuck.lever@oracle.com>, imammedo@redhat.com
Cc:     linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Mon, 11 Jul 2022 07:36:52 -0400
In-Reply-To: <165747876458.1259.8334435718280903102.stgit@bazille.1015granger.net>
References: <165747876458.1259.8334435718280903102.stgit@bazille.1015granger.net>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.3 (3.44.3-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

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 *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] &=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 minorvers=
ion, 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

RFC5661 lists time_create as being writeable, so silently ignoring it
seems wrong. It seems like we ought to have nfsd attempt to set the
btime and then just return an error if it doesn't work...but, I don't
see a mechanism in the kernel for setting it. ATTR_BTIME doesn't exist,
for instance.

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
Jeff Layton <jlayton@kernel.org>