Message-ID: <ZuRX/QfG+OLm9fTR@tissot.1015granger.net>
Date: Fri, 13 Sep 2024 11:19:25 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: Pali Rohár <pali@...nel.org>
Cc: Jeff Layton <jlayton@...nel.org>, Neil Brown <neilb@...e.de>,
        Olga Kornievskaia <okorniev@...hat.com>, Dai Ngo <Dai.Ngo@...cle.com>,
        Tom Talpey <tom@...pey.com>, linux-nfs@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nfsd: Fill NFSv4.1 server implementation fields in
 OP_EXCHANGE_ID response

On Fri, Sep 13, 2024 at 12:09:19AM +0200, Pali Rohár wrote:
> NFSv4.1 OP_EXCHANGE_ID response from server may contain server
> implementation details (domain, name and build time) in optional
> nfs_impl_id4 field. Currently nfsd does not fill this field.
> 
> NFSv4.1 OP_EXCHANGE_ID call request from client may contain client
> implementation details and Linux NFSv4.1 client is already filling these
> information based on runtime module param "nfs.send_implementation_id" and
> build time Kconfig option "NFS_V4_1_IMPLEMENTATION_ID_DOMAIN". Module param
> send_implementation_id specify whether to fill implementation fields and
> Kconfig option "NFS_V4_1_IMPLEMENTATION_ID_DOMAIN" specify the domain
> string.
> 
> Do same in nfsd, introduce new runtime param "nfsd.send_implementation_id"
> and build time Kconfig option "NFSD_V4_1_IMPLEMENTATION_ID_DOMAIN" and
> based on them fill NFSv4.1 server implementation details in OP_EXCHANGE_ID
> response. Logic in nfsd is exactly same as in nfs.
> 
> This aligns Linux NFSv4.1 server logic with Linux NFSv4.1 client logic.
> 
> NFSv4.1 client and server implementation fields are useful for statistic
> purposes or for identifying type of clients and servers.

NFSD has gotten along for more than a decade without returning this
information. The patch description should explain the use case in a
little more detail, IMO.

As a general comment, I recognize that you copied the client code
for EXCHANGE_ID to construct this patch. The client and server code
bases are somewhat different and have different coding conventions.
Most of the comments below have to do with those differences.


> Signed-off-by: Pali Rohár <pali@...nel.org>
> ---
>  fs/nfsd/Kconfig   | 12 +++++++++++
>  fs/nfsd/nfs4xdr.c | 55 +++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> index ec2ab6429e00..70067c29316e 100644
> --- a/fs/nfsd/Kconfig
> +++ b/fs/nfsd/Kconfig
> @@ -136,6 +136,18 @@ config NFSD_FLEXFILELAYOUT
>  
>  	  If unsure, say N.
>  
> +config NFSD_V4_1_IMPLEMENTATION_ID_DOMAIN
> +	string "NFSv4.1 Implementation ID Domain"
> +	depends on NFSD_V4
> +	default "kernel.org"
> +	help
> +	  This option defines the domain portion of the implementation ID that
> +	  may be sent in the NFS exchange_id operation.  The value must be in

Nit: "that the server returns in its NFSv4 EXCHANGE_ID response."


> +	  the format of a DNS domain name and should be set to the DNS domain
> +	  name of the distribution.

Perhaps add: "See the description of the nii_domain field in Section
3.3.21 of RFC 8881 for details."

But honestly, I'm not sure why nii_domain is parametrized at all, on
the client. Why not /always/ return "kernel.org" ?

What checking should be done to ensure that the value of this
setting is a valid DNS label?


> +	  If the NFS server is unchanged from the upstream kernel, this
> +	  option should be set to the default "kernel.org".
> +
>  config NFSD_V4_2_INTER_SSC
>  	bool "NFSv4.2 inter server to server COPY"
>  	depends on NFSD_V4 && NFS_V4_2
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index b45ea5757652..5e89f999d4c7 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -62,6 +62,9 @@
>  #include <linux/security.h>
>  #endif
>  
> +static bool send_implementation_id = true;
> +module_param(send_implementation_id, bool, 0644);
> +MODULE_PARM_DESC(send_implementation_id, "Send implementation ID with NFSv4.1 exchange_id");

I'd rather not add a module parameter if we don't have to. Can you
explain why this new parameter is necessary? For instance, is there
a reason why an administrator who runs NFSD on a stock distro kernel
would want to change this setting to "false" ?

If it turns out that the parameter is valuable, is there admin
documentation to go with it?


>  #define NFSDDBG_FACILITY		NFSDDBG_XDR
>  
> @@ -4833,6 +4836,53 @@ nfsd4_encode_server_owner4(struct xdr_stream *xdr, struct svc_rqst *rqstp)
>  	return nfsd4_encode_opaque(xdr, nn->nfsd_name, strlen(nn->nfsd_name));
>  }
>  
> +#define IMPL_NAME_LIMIT (sizeof(utsname()->sysname) + sizeof(utsname()->release) + \
> +			 sizeof(utsname()->version) + sizeof(utsname()->machine) + 8)
> +
> +static __be32
> +nfsd4_encode_server_impl_id(struct xdr_stream *xdr)
> +{

The matching XDR decoder in fs/nfsd/nfs4xdr.c is:

   static __be32 nfsd4_decode_nfs_impl_id4( ... )

The function name matches the name of the XDR type in the spec. So
let's call this function nfsd4_encode_nfs_impl_id4().


> +	char impl_name[IMPL_NAME_LIMIT];
> +	int impl_name_len;
> +	__be32 *p;
> +
> +	impl_name_len = 0;
> +	if (send_implementation_id &&
> +	    sizeof(CONFIG_NFSD_V4_1_IMPLEMENTATION_ID_DOMAIN) > 1 &&
> +	    sizeof(CONFIG_NFSD_V4_1_IMPLEMENTATION_ID_DOMAIN) <= NFS4_OPAQUE_LIMIT)
> +		impl_name_len = snprintf(impl_name, sizeof(impl_name), "%s %s %s %s",
> +			       utsname()->sysname, utsname()->release,
> +			       utsname()->version, utsname()->machine);
> +
> +	if (impl_name_len <= 0) {
> +		if (xdr_stream_encode_u32(xdr, 0) != XDR_UNIT)
> +			return nfserr_resource;
> +		return nfs_ok;
> +	}

IMPL_NAME_LIMIT looks like it could be hundreds of bytes. Probably
not good to put a character array that size on the stack.

I prefer that the construction and checking is done in
nfsd4_exchange_id() instead, and the content of these fields passed
to this encoder via struct nfsd4_exchange_id.

As a guideline, the XDR layer should be concerned solely with
marshaling and unmarshalling data types. The content of the
marshaled data fields should be handled by NFSD's proc layer
(fs/nfsd/nfs4proc.c).


> +
> +	if (xdr_stream_encode_u32(xdr, 1) != XDR_UNIT)
> +		return nfserr_resource;

A brief comment would help remind readers that what is encoded here
is an array item count, and not a string length or a "value follows"
boolean.

Nit: In fact, this value isn't really a part of the base
nfs_impl_id4 data type. Maybe better to do this bit of logic in the
caller nfsd4_encode_exchange_id().


> +
> +	p = xdr_reserve_space(xdr,
> +		4 /* nii_domain.len */ +
> +		(XDR_QUADLEN(sizeof(CONFIG_NFSD_V4_1_IMPLEMENTATION_ID_DOMAIN) - 1) * 4) +
> +		4 /* nii_name.len */ +
> +		(XDR_QUADLEN(impl_name_len) * 4) +
> +		8 /* nii_time.tv_sec */ +
> +		4 /* nii_time.tv_nsec */);
> +	if (!p)
> +		return nfserr_resource;
> +
> +	p = xdr_encode_opaque(p, CONFIG_NFSD_V4_1_IMPLEMENTATION_ID_DOMAIN,
> +				sizeof(CONFIG_NFSD_V4_1_IMPLEMENTATION_ID_DOMAIN) - 1);
> +	p = xdr_encode_opaque(p, impl_name, impl_name_len);
> +	/* just send zeros for nii_date - the date is in nii_name */
> +	p = xdr_encode_hyper(p, 0); /* tv_sec */
> +	*p++ = cpu_to_be32(0); /* tv_nsec */

We no longer write raw encoders like this in NFSD code. This is
especially unnecessary because EXCHANGE_ID is not a hot path.

Instead, use the XDR utility functions to spell out the field names
and data types, for easier auditing. For instance, something like
this:

	status = nfsd4_encode_opaque(xdr, exid->nii_domain.data,
				     exid->nii_domain.len);        
	if (status != nfs_ok)
		return status;
	status = nfsd4_encode_opaque(xdr, exid->nii_name.data,
				     exid->nii_name.len);        
	return nfsd4_encode_nfstime4(xdr, &exid->nii_date);

Regarding the content of these fields: I don't mind filling in
nii_date, duplicating what might appear in the nii_name field, if
that is not a bother.


> +
> +	return nfs_ok;
> +}
> +
>  static __be32
>  nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
>  			 union nfsd4_op_u *u)
> @@ -4867,8 +4917,9 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
>  	if (nfserr != nfs_ok)
>  		return nfserr;
>  	/* eir_server_impl_id<1> */
> -	if (xdr_stream_encode_u32(xdr, 0) != XDR_UNIT)
> -		return nfserr_resource;
> +	nfserr = nfsd4_encode_server_impl_id(xdr);
> +	if (nfserr != nfs_ok)
> +		return nfserr;
>  
>  	return nfs_ok;
>  }
> -- 
> 2.20.1
> 

-- 
Chuck Lever

Powered by blists - more mailing lists