fs/nfsd/Kconfig | 12 +++++++++++ fs/nfsd/nfs4xdr.c | 55 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 65 insertions(+), 2 deletions(-)
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.
Signed-off-by: Pali Rohár <pali@kernel.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
+ the format of a DNS domain name and should be set to the DNS domain
+ name of the distribution.
+ 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");
#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)
+{
+ 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;
+ }
+
+ if (xdr_stream_encode_u32(xdr, 1) != XDR_UNIT)
+ return nfserr_resource;
+
+ 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 */
+
+ 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
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@kernel.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
On Friday 13 September 2024 11:19:25 Chuck Lever wrote:
> 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.
Ok, this can be adjusted/aligned.
>
> > Signed-off-by: Pali Rohár <pali@kernel.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."
Ok.
> But honestly, I'm not sure why nii_domain is parametrized at all, on
> the client. Why not /always/ return "kernel.org" ?
I do not know. I just followed logic of client. In my opinion, it does
not make sense to have different logic in client and server. If it is
not needed, maybe remove it from client too?
> What checking should be done to ensure that the value of this
> setting is a valid DNS label?
Checking for valid DNS label is not easy. Client does not do it, so is
it needed?
>
> > + 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" ?
I really do not know. Client has this parameter, so I though it is a
good idea to have it.
> If it turns out that the parameter is valuable, is there admin
> documentation to go with it?
I'm not sure if client have documentation for 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().
Ok.
>
> > + 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).
Ok, I could try to look at it.
>
> > +
> > + 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().
Ok, it is truth that array item could is not part of impl_id4.
>
> > +
> > + 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);
Ok.
> 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.
I looked at this, and getting timestamp in numeric form is not possible.
Kernel utsname() and UTS functions provides date only in `date` format
which is unsuitable for parsing in kernel and converting into seconds
since epoch. Moreover uts structures are exported to userspace, so
changing and providing numeric value would be harder.
>
> > +
> > + 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
On Fri, Sep 13, 2024 at 05:36:31PM +0200, Pali Rohár wrote: > On Friday 13 September 2024 11:19:25 Chuck Lever wrote: > > 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. > > Ok, this can be adjusted/aligned. > > > > > > Signed-off-by: Pali Rohár <pali@kernel.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." > > Ok. > > > But honestly, I'm not sure why nii_domain is parametrized at all, on > > the client. Why not /always/ return "kernel.org" ? > > I do not know. I just followed logic of client. In my opinion, it does > not make sense to have different logic in client and server. If it is > not needed, maybe remove it from client too? > > What checking should be done to ensure that the value of this > > setting is a valid DNS label? > > Checking for valid DNS label is not easy. Client does not do it, so is > it needed? Input checking is always a good thing to do. But I haven't found a compliance mandate in RFC 8881 for the content of nii_domain, so maybe it doesn't matter. One possibility would be to not add the parametrization of this string on the server unless it is found to be needed. So, this patch could simply always set "kernel.org", and then a Kconfig option can be added by a subsequent patch if/when a use case ever turns up. Or... NFSD could simply re-use the client's setting. I can't think of a reason why the NFS client and NFS server in the same kernel should report different nii_domain strings. > > > + 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" ? > > I really do not know. Client has this parameter, so I though it is a > good idea to have it. > > > If it turns out that the parameter is valuable, is there admin > > documentation to go with it? > > I'm not sure if client have documentation for it. Again, if we don't have a clear use case in front of us, it is sensible to postpone the addition of this parameter. [ ... snip ... ] > > 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. > > I looked at this, and getting timestamp in numeric form is not possible. > Kernel utsname() and UTS functions provides date only in `date` format > which is unsuitable for parsing in kernel and converting into seconds > since epoch. Moreover uts structures are exported to userspace, so > changing and providing numeric value would be harder. Not a big deal. And, it's something that can be changed later if someone finds a clean way to extract a numeric build time. -- Chuck Lever
On Friday 13 September 2024 12:07:36 Chuck Lever wrote: > On Fri, Sep 13, 2024 at 05:36:31PM +0200, Pali Rohár wrote: > > On Friday 13 September 2024 11:19:25 Chuck Lever wrote: > > > 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. > > > > Ok, this can be adjusted/aligned. > > > > > > > > > Signed-off-by: Pali Rohár <pali@kernel.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." > > > > Ok. > > > > > But honestly, I'm not sure why nii_domain is parametrized at all, on > > > the client. Why not /always/ return "kernel.org" ? > > > > I do not know. I just followed logic of client. In my opinion, it does > > not make sense to have different logic in client and server. If it is > > not needed, maybe remove it from client too? > > > > What checking should be done to ensure that the value of this > > > setting is a valid DNS label? > > > > Checking for valid DNS label is not easy. Client does not do it, so is > > it needed? > > Input checking is always a good thing to do. But I haven't found a > compliance mandate in RFC 8881 for the content of nii_domain, so > maybe it doesn't matter. > > One possibility would be to not add the parametrization of this > string on the server unless it is found to be needed. So, this > patch could simply always set "kernel.org", and then a Kconfig > option can be added by a subsequent patch if/when a use case ever > turns up. No problem, I can drop it. > Or... NFSD could simply re-use the client's setting. I can't think > of a reason why the NFS client and NFS server in the same kernel > should report different nii_domain strings. > > > > > > + 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" ? > > > > I really do not know. Client has this parameter, so I though it is a > > good idea to have it. > > > > > If it turns out that the parameter is valuable, is there admin > > > documentation to go with it? > > > > I'm not sure if client have documentation for it. > > Again, if we don't have a clear use case in front of us, it is > sensible to postpone the addition of this parameter. > > > [ ... snip ... ] > > > > 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. > > > > I looked at this, and getting timestamp in numeric form is not possible. > > Kernel utsname() and UTS functions provides date only in `date` format > > which is unsuitable for parsing in kernel and converting into seconds > > since epoch. Moreover uts structures are exported to userspace, so > > changing and providing numeric value would be harder. > > Not a big deal. And, it's something that can be changed later if > someone finds a clean way to extract a numeric build time. Ok.
On Friday 13 September 2024 18:10:00 Pali Rohár wrote: > On Friday 13 September 2024 12:07:36 Chuck Lever wrote: > > On Fri, Sep 13, 2024 at 05:36:31PM +0200, Pali Rohár wrote: > > > On Friday 13 September 2024 11:19:25 Chuck Lever wrote: > > > > 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. > > > > > > Ok, this can be adjusted/aligned. > > > > > > > > > > > > Signed-off-by: Pali Rohár <pali@kernel.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." > > > > > > Ok. > > > > > > > But honestly, I'm not sure why nii_domain is parametrized at all, on > > > > the client. Why not /always/ return "kernel.org" ? > > > > > > I do not know. I just followed logic of client. In my opinion, it does > > > not make sense to have different logic in client and server. If it is > > > not needed, maybe remove it from client too? > > > > > > What checking should be done to ensure that the value of this > > > > setting is a valid DNS label? > > > > > > Checking for valid DNS label is not easy. Client does not do it, so is > > > it needed? > > > > Input checking is always a good thing to do. But I haven't found a > > compliance mandate in RFC 8881 for the content of nii_domain, so > > maybe it doesn't matter. > > > > One possibility would be to not add the parametrization of this > > string on the server unless it is found to be needed. So, this > > patch could simply always set "kernel.org", and then a Kconfig > > option can be added by a subsequent patch if/when a use case ever > > turns up. > > No problem, I can drop it. > > > Or... NFSD could simply re-use the client's setting. I can't think > > of a reason why the NFS client and NFS server in the same kernel > > should report different nii_domain strings. > > > > > > > > > + 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" ? > > > > > > I really do not know. Client has this parameter, so I though it is a > > > good idea to have it. > > > > > > > If it turns out that the parameter is valuable, is there admin > > > > documentation to go with it? > > > > > > I'm not sure if client have documentation for it. > > > > Again, if we don't have a clear use case in front of us, it is > > sensible to postpone the addition of this parameter. > > > > > > [ ... snip ... ] > > > > > > 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. > > > > > > I looked at this, and getting timestamp in numeric form is not possible. > > > Kernel utsname() and UTS functions provides date only in `date` format > > > which is unsuitable for parsing in kernel and converting into seconds > > > since epoch. Moreover uts structures are exported to userspace, so > > > changing and providing numeric value would be harder. > > > > Not a big deal. And, it's something that can be changed later if > > someone finds a clean way to extract a numeric build time. > > Ok. I sent V2 version. I hope that I addressed all points from this discussion.
On Fri, 13 Sep 2024, 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.
>
> Signed-off-by: Pali Rohár <pali@kernel.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
> + the format of a DNS domain name and should be set to the DNS domain
> + name of the distribution.
> + 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");
>
> #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)
This "+8" seems strange. In the xdr_reserve_space() call below you are
very thorough about explaining the magic numbers - which is great. Here
that is this unexplained 8.
I don't think you need +8 at all. sizeof(string) will give room to
print the string plus a trailing space or nul, and that is all you need.
Otherwise the patch looks OK.
Thanks,
NeilBrown
> +
> +static __be32
> +nfsd4_encode_server_impl_id(struct xdr_stream *xdr)
> +{
> + 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;
> + }
> +
> + if (xdr_stream_encode_u32(xdr, 1) != XDR_UNIT)
> + return nfserr_resource;
> +
> + 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 */
> +
> + 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
>
>
On Friday 13 September 2024 08:31:55 NeilBrown wrote:
> On Fri, 13 Sep 2024, 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.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.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
> > + the format of a DNS domain name and should be set to the DNS domain
> > + name of the distribution.
> > + 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");
> >
> > #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)
>
> This "+8" seems strange. In the xdr_reserve_space() call below you are
> very thorough about explaining the magic numbers - which is great. Here
> that is this unexplained 8.
I copied this code from nfs/nfs4xdr.c file. And verified in wireshark
that packets were correctly constructed.
> I don't think you need +8 at all. sizeof(string) will give room to
> print the string plus a trailing space or nul, and that is all you need.
I'm looking at it, and I think you are right, +8 should not be needed.
Thanks for review.
> Otherwise the patch looks OK.
>
> Thanks,
> NeilBrown
>
>
> > +
> > +static __be32
> > +nfsd4_encode_server_impl_id(struct xdr_stream *xdr)
> > +{
> > + 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;
> > + }
> > +
> > + if (xdr_stream_encode_u32(xdr, 1) != XDR_UNIT)
> > + return nfserr_resource;
> > +
> > + 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 */
> > +
> > + 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
> >
> >
>
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.
Send these information in NFSv4.1 OP_EXCHANGE_ID response. Fill them with
the same values as what is Linux NFSv4.1 client doing. Domain is hardcoded
to "kernel.org", name is composed in the same way as "uname -srvm" output
and build time is hardcoded to zeros.
NFSv4.1 client and server implementation fields are useful for statistic
purposes or for identifying type of clients and servers.
Signed-off-by: Pali Rohár <pali@kernel.org>
---
Changes in v2:
* Prepare string arguments in nfsd4_exchange_id() instead of in nfsd4_encode_server_impl_id()
* Use nfsd4_encode_*() functions for encoding instead of raw encoders
* Rename nfsd4_encode_nfs_impl_id4() to nfsd4_encode_server_impl_id()
* Remove Kconfig build option for nii_domain
* Remove runtime module option send_implementation_id
---
fs/nfsd/nfs4proc.c | 1 +
fs/nfsd/nfs4state.c | 28 ++++++++++++++++++++++++++++
fs/nfsd/nfs4xdr.c | 27 +++++++++++++++++++++++++--
fs/nfsd/xdr4.h | 2 ++
4 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 0f67f4a7b8b2..19ffb4f966d0 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -3447,6 +3447,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
/* NFSv4.1 operations */
[OP_EXCHANGE_ID] = {
.op_func = nfsd4_exchange_id,
+ .op_release = nfsd4_exchange_id_release,
.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
| OP_MODIFIES_SOMETHING,
.op_name = "OP_EXCHANGE_ID",
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index a366fb1c1b9b..68b38bc41828 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3475,6 +3475,13 @@ static __be32 copy_impl_id(struct nfs4_client *clp,
return 0;
}
+void
+nfsd4_exchange_id_release(union nfsd4_op_u *u)
+{
+ struct nfsd4_exchange_id *exid = &u->exchange_id;
+ kfree(exid->server_impl_name);
+}
+
__be32
nfsd4_exchange_id(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
union nfsd4_op_u *u)
@@ -3495,6 +3502,12 @@ nfsd4_exchange_id(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
__func__, rqstp, exid, exid->clname.len, exid->clname.data,
addr_str, exid->flags, exid->spa_how);
+ exid->server_impl_name = kasprintf(GFP_KERNEL, "%s %s %s %s",
+ utsname()->sysname, utsname()->release,
+ utsname()->version, utsname()->machine);
+ if (!exid->server_impl_name)
+ return nfserr_jukebox;
+
if (exid->flags & ~EXCHGID4_FLAG_MASK_A)
return nfserr_inval;
@@ -3632,6 +3645,21 @@ nfsd4_exchange_id(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
exid->seqid = conf->cl_cs_slot.sl_seqid + 1;
nfsd4_set_ex_flags(conf, exid);
+ exid->nii_domain.len = sizeof("kernel.org")-1;
+ exid->nii_domain.data = "kernel.org";
+
+ /* Note that RFC 8881 places no length limit on
+ * nii_name, but this implementation permits no
+ * more than NFS4_OPAQUE_LIMIT bytes */
+ exid->nii_name.len = strlen(exid->server_impl_name);
+ if (exid->nii_name.len > NFS4_OPAQUE_LIMIT)
+ exid->nii_name.len = NFS4_OPAQUE_LIMIT;
+ exid->nii_name.data = exid->server_impl_name;
+
+ /* just send zeros - the date is in nii_name */
+ exid->nii_time.tv_sec = 0;
+ exid->nii_time.tv_nsec = 0;
+
dprintk("nfsd4_exchange_id seqid %d flags %x\n",
conf->cl_cs_slot.sl_seqid, conf->cl_exchange_flags);
status = nfs_ok;
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index b45ea5757652..85334f4483eb 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4833,6 +4833,28 @@ nfsd4_encode_server_owner4(struct xdr_stream *xdr, struct svc_rqst *rqstp)
return nfsd4_encode_opaque(xdr, nn->nfsd_name, strlen(nn->nfsd_name));
}
+static __be32
+nfsd4_encode_nfs_impl_id4(struct xdr_stream *xdr, struct nfsd4_exchange_id *exid)
+{
+ __be32 status;
+
+ /* array count = 1 */
+ if (xdr_stream_encode_u32(xdr, 1) != XDR_UNIT)
+ return nfserr_resource;
+ /* nii_domain */
+ status = nfsd4_encode_opaque(xdr, exid->nii_domain.data,
+ exid->nii_domain.len);
+ if (status != nfs_ok)
+ return status;
+ /* nii_name */
+ status = nfsd4_encode_opaque(xdr, exid->nii_name.data,
+ exid->nii_name.len);
+ if (status != nfs_ok)
+ return status;
+ /* nii_time */
+ return nfsd4_encode_nfstime4(xdr, &exid->nii_time);
+}
+
static __be32
nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
union nfsd4_op_u *u)
@@ -4867,8 +4889,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_nfs_impl_id4(xdr, exid);
+ if (nfserr != nfs_ok)
+ return nfserr;
return nfs_ok;
}
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index fbdd42cde1fa..891861e8bb0c 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -567,6 +567,7 @@ struct nfsd4_exchange_id {
struct xdr_netobj nii_domain;
struct xdr_netobj nii_name;
struct timespec64 nii_time;
+ char * server_impl_name;
};
struct nfsd4_sequence {
@@ -929,6 +930,7 @@ extern __be32 nfsd4_setclientid(struct svc_rqst *rqstp,
struct nfsd4_compound_state *, union nfsd4_op_u *u);
extern __be32 nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
struct nfsd4_compound_state *, union nfsd4_op_u *u);
+void nfsd4_exchange_id_release(union nfsd4_op_u *u);
extern __be32 nfsd4_exchange_id(struct svc_rqst *rqstp,
struct nfsd4_compound_state *, union nfsd4_op_u *u);
extern __be32 nfsd4_backchannel_ctl(struct svc_rqst *,
--
2.20.1
From: Chuck Lever <chuck.lever@oracle.com>
On Sat, 05 Oct 2024 20:33:49 +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.
>
> Send these information in NFSv4.1 OP_EXCHANGE_ID response. Fill them with
> the same values as what is Linux NFSv4.1 client doing. Domain is hardcoded
> to "kernel.org", name is composed in the same way as "uname -srvm" output
> and build time is hardcoded to zeros.
>
> [...]
Applied to nfsd-next for v6.13, thanks!
[1/1] nfsd: Fill NFSv4.1 server implementation fields in OP_EXCHANGE_ID response
commit: 5d063672154b3d2d3f9bc3426e0ee4e42ab0e811
--
Chuck Lever
© 2016 - 2024 Red Hat, Inc.