| Message ID | 20240912220919.23449-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: Fill NFSv4.1 server implementation fields in OP_EXCHANGE_ID response | expand |
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 > > > > >
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 >
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.
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.
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; }
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(-)