[PATCH 0/3] Bake-a-thon fixes

[PATCH 0/3] Bake-a-thon fixes

[Chuck Lever]

Hi Trond, Bruce-

I know it is late, but please consider these three bug fixes for 2.6.28.

The first patch is another result of Olaf's recent exploration of the
Linux rpcbind port.  It belongs with the other patches for 2.6.28 that
provide proper support for IPv6 and rpcbind v4 when the kernel registers
its RPC services with the local rpcbind daemon.

The second patch is a simple clean up that reduces the verbosity of
rpcbind debugging messages.

The third is important for NFSv4 over IPv6.  Without it, the NFSv4.0
callback channel does not work properly over IPv6.  There may be more
problems there, so this does not yet guarantee that we have working
callback support over IPv6.

---

Chuck Lever (3):
      NFS: SETCLIENTID truncates client ID and netid
      SUNRPC: Clean up debug messages in rpcb_clnt.c
      SUNRPC: Fix up svc_unregister()


 include/linux/nfs_xdr.h |    8 +++---
 net/sunrpc/rpcb_clnt.c  |   14 ++++-------
 net/sunrpc/svc.c        |   58 +++++++++++++++++++++++++++++++----------------
 3 files changed, 47 insertions(+), 33 deletions(-)

-- 
Chuck Lever

[PATCH 1/3] SUNRPC: Fix up svc_unregister()

[Chuck Lever]

With the new rpcbind code, a PMAP_UNSET will not have any effect on
services registered via rpcbind v3 or v4.

Implement a version of svc_unregister() that uses an RPCB_UNSET with
an empty netid string to make sure we have cleared *all* entries for
a kernel RPC service when shutting down, or before starting a fresh
instance of the service.

Use the new version only when CONFIG_SUNRPC_REGISTER_V4 is enabled;
otherwise, the legacy PMAP version is used to ensure complete
backwards-compatibility with the Linux portmapper daemon.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 net/sunrpc/svc.c |   58 +++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 5089014..e626f01 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -897,31 +897,51 @@ int svc_register(const struct svc_serv *serv, const unsigned short proto,
 	return error;
 }
 
+#ifdef CONFIG_SUNRPC_REGISTER_V4
+
+static void __svc_unregister(const u32 program, const u32 version,
+			     const char *progname)
+{
+	struct sockaddr_in6 sin6 = {
+		.sin6_family		= AF_INET6,
+		.sin6_addr		= IN6ADDR_ANY_INIT,
+		.sin6_port		= 0,
+	};
+	int error;
+
+	error = rpcb_v4_register(program, version,
+				(struct sockaddr *)&sin6, "");
+	dprintk("svc: %s(%sv%u), error %d\n",
+			__func__, progname, version, error);
+}
+
+#else	/* CONFIG_SUNRPC_REGISTER_V4 */
+
+static void __svc_unregister(const u32 program, const u32 version,
+			     const char *progname)
+{
+	int error;
+
+	error = rpcb_register(program, version, 0, 0);
+	dprintk("svc: %s(%sv%u), error %d\n",
+			__func__, progname, version, error);
+}
+
+#endif	/* CONFIG_SUNRPC_REGISTER_V4 */
+
 /*
- * All transport protocols and ports for this service are removed
- * from the local rpcbind database if the service is not hidden.
- *
- * The result of unregistration is reported via dprintk for those
- * who want verification of the result, but is otherwise not
- * important.
+ * All netids, bind addresses and ports registered for [program, version]
+ * are removed from the local rpcbind database (if the service is not
+ * hidden) to make way for a new instance of the service.
  *
- * The local rpcbind daemon listens on either only IPv6 or only
- * IPv4.  The kernel can't tell how it's configured.  However,
- * AF_INET addresses are mapped to AF_INET6 in IPv6-only config-
- * urations, so even an unregistration request on AF_INET will
- * get to a local rpcbind daemon listening only on AF_INET6.  So
- * we always unregister via AF_INET.
- *
- * At this point we don't need rpcbind version 4 for unregis-
- * tration:  A v2 UNSET request will clear all transports (netids),
- * addresses, and address families for [program, version].
+ * The result of unregistration is reported via dprintk for those who want
+ * verification of the result, but is otherwise not important.
  */
 static void svc_unregister(const struct svc_serv *serv)
 {
 	struct svc_program *progp;
 	unsigned long flags;
 	unsigned int i;
-	int error;
 
 	clear_thread_flag(TIF_SIGPENDING);
 
@@ -932,9 +952,7 @@ static void svc_unregister(const struct svc_serv *serv)
 			if (progp->pg_vers[i]->vs_hidden)
 				continue;
 
-			error = rpcb_register(progp->pg_prog, i, 0, 0);
-			dprintk("svc: svc_unregister(%sv%u), error %d\n",
-					progp->pg_name, i, error);
+			__svc_unregister(progp->pg_prog, i, progp->pg_name);
 		}
 	}
 

[PATCH 2/3] SUNRPC: Clean up debug messages in rpcb_clnt.c

[Chuck Lever]

The RPCB XDR functions are used for multiple procedures.  For instance,
rpcb_encode_getaddr() is used for RPCB_GETADDR, RPCB_SET, and
RPCB_UNSET.  Make the XDR debug messages more generic so they are less
confusing.

And, unlike in other RPC consumers in the kernel, a single debug flag
enables all levels of debug messages in the RPC bind client, including
XDR debug messages.  Since the XDR decoders already report success or
failure in this case, remove redundant debug messages in the mid-level
rpcb_register_call() function.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 net/sunrpc/rpcb_clnt.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 02164d4..0b93205 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -197,12 +197,8 @@ static int rpcb_register_call(struct sockaddr *addr, size_t addrlen,
 		return error;
 	}
 
-	if (!result) {
-		dprintk("RPC:       registration failed\n");
+	if (!result)
 		return -EACCES;
-	}
-
-	dprintk("RPC:       registration succeeded\n");
 	return 0;
 }
 
@@ -628,7 +624,7 @@ static void rpcb_getport_done(struct rpc_task *child, void *data)
 static int rpcb_encode_mapping(struct rpc_rqst *req, __be32 *p,
 			       struct rpcbind_args *rpcb)
 {
-	dprintk("RPC:       rpcb_encode_mapping(%u, %u, %d, %u)\n",
+	dprintk("RPC:       encoding rpcb request (%u, %u, %d, %u)\n",
 			rpcb->r_prog, rpcb->r_vers, rpcb->r_prot, rpcb->r_port);
 	*p++ = htonl(rpcb->r_prog);
 	*p++ = htonl(rpcb->r_vers);
@@ -643,7 +639,7 @@ static int rpcb_decode_getport(struct rpc_rqst *req, __be32 *p,
 			       unsigned short *portp)
 {
 	*portp = (unsigned short) ntohl(*p++);
-	dprintk("RPC:       rpcb_decode_getport result %u\n",
+	dprintk("RPC:       rpcb getport result: %u\n",
 			*portp);
 	return 0;
 }
@@ -652,7 +648,7 @@ static int rpcb_decode_set(struct rpc_rqst *req, __be32 *p,
 			   unsigned int *boolp)
 {
 	*boolp = (unsigned int) ntohl(*p++);
-	dprintk("RPC:       rpcb_decode_set: call %s\n",
+	dprintk("RPC:       rpcb set/unset call %s\n",
 			(*boolp ? "succeeded" : "failed"));
 	return 0;
 }
@@ -660,7 +656,7 @@ static int rpcb_decode_set(struct rpc_rqst *req, __be32 *p,
 static int rpcb_encode_getaddr(struct rpc_rqst *req, __be32 *p,
 			       struct rpcbind_args *rpcb)
 {
-	dprintk("RPC:       rpcb_encode_getaddr(%u, %u, %s)\n",
+	dprintk("RPC:       encoding rpcb request (%u, %u, %s)\n",
 			rpcb->r_prog, rpcb->r_vers, rpcb->r_addr);
 	*p++ = htonl(rpcb->r_prog);
 	*p++ = htonl(rpcb->r_vers);

[PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

[Chuck Lever]

The sc_name field is currently 56 bytes long.  This is not large enough
to hold a pair of IPv6 addresses, the authentication type, the protocol
name, and a uniquifier number.  The maximum possible size of the name
string using IPv6 addresses is just under 110 bytes, so I increased the
size of the sc_name field to accomodate this maximum.

In addition, the strings in the nfs4_setclientid structure are
constructed with scnprintf(), which wants to terminate its output with
'\0'.  The sc_netid field was large enough only for a three byte netid
string and a '\0' so inet6 netids were being truncated.  Perhaps we
don't need the overhead of scnprintf() to do a simple string copy, but
I fixed this by increasing the size of the buffer by one byte.

Since all three of the string buffers in nfs4_setclientid are
constructed with scnprintf(), I increased the size of all three by one
byte to document the requirement, although I don't think either the
universal address field or the name field will be so small that these
strings get truncated in this way.

The size of the Linux client's client ID on the wire will be larger
than before.  RFC 3530 suggests the size limit for client IDs is 1024,
and we are still well below that.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 include/linux/nfs_xdr.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 8c77c11..dc34977 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -672,16 +672,16 @@ struct nfs4_rename_res {
 	struct nfs_fattr *		new_fattr;
 };
 
-#define NFS4_SETCLIENTID_NAMELEN	(56)
+#define NFS4_SETCLIENTID_NAMELEN	(128)
 struct nfs4_setclientid {
 	const nfs4_verifier *		sc_verifier;
 	unsigned int			sc_name_len;
-	char				sc_name[NFS4_SETCLIENTID_NAMELEN];
+	char				sc_name[NFS4_SETCLIENTID_NAMELEN + 1];
 	u32				sc_prog;
 	unsigned int			sc_netid_len;
-	char				sc_netid[RPCBIND_MAXNETIDLEN];
+	char				sc_netid[RPCBIND_MAXNETIDLEN + 1];
 	unsigned int			sc_uaddr_len;
-	char				sc_uaddr[RPCBIND_MAXUADDRLEN];
+	char				sc_uaddr[RPCBIND_MAXUADDRLEN + 1];
 	u32				sc_cb_ident;
 };
 

Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

[Trond Myklebust]

On Thu, 2008-09-25 at 11:57 -0400, Chuck Lever wrote:
> The sc_name field is currently 56 bytes long.  This is not large enough
> to hold a pair of IPv6 addresses, the authentication type, the protocol
> name, and a uniquifier number.  The maximum possible size of the name
> string using IPv6 addresses is just under 110 bytes, so I increased the
> size of the sc_name field to accomodate this maximum.
> 
> In addition, the strings in the nfs4_setclientid structure are
> constructed with scnprintf(), which wants to terminate its output with
> '\0'.  The sc_netid field was large enough only for a three byte netid
> string and a '\0' so inet6 netids were being truncated.  Perhaps we
> don't need the overhead of scnprintf() to do a simple string copy, but
> I fixed this by increasing the size of the buffer by one byte.
> 
> Since all three of the string buffers in nfs4_setclientid are
> constructed with scnprintf(), I increased the size of all three by one
> byte to document the requirement, although I don't think either the
> universal address field or the name field will be so small that these
> strings get truncated in this way.
> 
> The size of the Linux client's client ID on the wire will be larger
> than before.  RFC 3530 suggests the size limit for client IDs is 1024,
> and we are still well below that.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

ACK. I'll take this patch, but should Bruce take the other 2? I believe
he should already have other changes to rpcb_clnt.c in his tree...

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

[J. Bruce Fields]

On Thu, Sep 25, 2008 at 12:10:26PM -0400, Trond Myklebust wrote:
> ACK. I'll take this patch, but should Bruce take the other 2? I believe
> he should already have other changes to rpcb_clnt.c in his tree...

Yes I do; I'll take a look.  (My goal is to get through my backlog from
Trond this afternoon....)

I recall one remaining uncertainty about the patches already in my
for-2.6.28: they allow building either a kernel that supports nfs/ipv6,
or a kernel that works with older nfs-utils, but not both.

I'd prefer a stricter level of backwards compatibility.  The current
approach may be confusing to distributors, early adopters, and testers.
But I'm willing to settle for it and let it be a lesson to us if it
turns out to cause more problems than expected.

Talking to Trond the other day he asked why we couldn't use
PROG_MISMATCH (unsupported program version) error to fall back.  Chuck
says in the changelog comment:

	"I tried adding some automatic logic to fall back if registering
	with a v4 protocol request failed, but there are too many corner
	cases."

Which I can believe, though I haven't looked at it myself.

In any case I'd like Trond's ACK or NACK.

--b.

Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

[J. Bruce Fields]

On Thu, Sep 25, 2008 at 12:58:21PM -0400, bfields wrote:
> On Thu, Sep 25, 2008 at 12:10:26PM -0400, Trond Myklebust wrote:
> > ACK. I'll take this patch, but should Bruce take the other 2? I believe
> > he should already have other changes to rpcb_clnt.c in his tree...
> 
> Yes I do; I'll take a look.  (My goal is to get through my backlog from
> Trond this afternoon....)

(err, I meant "Chuck" not "Trond" there.  You guys are so hard to tell
apart, ya know.)

--b.

> 
> I recall one remaining uncertainty about the patches already in my
> for-2.6.28: they allow building either a kernel that supports nfs/ipv6,
> or a kernel that works with older nfs-utils, but not both.
> 
> I'd prefer a stricter level of backwards compatibility.  The current
> approach may be confusing to distributors, early adopters, and testers.
> But I'm willing to settle for it and let it be a lesson to us if it
> turns out to cause more problems than expected.
> 
> Talking to Trond the other day he asked why we couldn't use
> PROG_MISMATCH (unsupported program version) error to fall back.  Chuck
> says in the changelog comment:
> 
> 	"I tried adding some automatic logic to fall back if registering
> 	with a v4 protocol request failed, but there are too many corner
> 	cases."
> 
> Which I can believe, though I haven't looked at it myself.
> 
> In any case I'd like Trond's ACK or NACK.
> 
> --b.

Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

[Chuck Lever]

On Sep 25, 2008, at 12:58 PM, J. Bruce Fields wrote:
> On Thu, Sep 25, 2008 at 12:10:26PM -0400, Trond Myklebust wrote:
>> ACK. I'll take this patch, but should Bruce take the other 2? I  
>> believe
>> he should already have other changes to rpcb_clnt.c in his tree...
>
> Yes I do; I'll take a look.  (My goal is to get through my backlog  
> from
> Trond this afternoon....)
>
>
> I recall one remaining uncertainty about the patches already in my
> for-2.6.28: they allow building either a kernel that supports nfs/ 
> ipv6,
> or a kernel that works with older nfs-utils, but not both.

That's not quite accurate.

The build option enables support for using rpcbindv4 upcalls.   
Otherwise, the kernel will support NFS on IPv6 if CONFIG_IPV6 or  
CONFIG_IPV6_MODULE are enabled, but NFSD and lockd won't start if the  
rpcbind upcall fails.

> I'd prefer a stricter level of backwards compatibility.  The current
> approach may be confusing to distributors, early adopters, and  
> testers.

The issues are spelled out in the help text of  
CONFIG_SUNRPC_REGISTER_V4, and it defaults to legacy behavior.

> But I'm willing to settle for it and let it be a lesson to us if it
> turns out to cause more problems than expected.

I will be here to fix it if there is a problem.  In this case, this  
whole NFS/IPv6 thing is so complicated that I'm implementing just what  
is needed as we go along.  We can fill in the niceties at a later point.

/me is taking his cue from "lazy evaluation."

> Talking to Trond the other day he asked why we couldn't use
> PROG_MISMATCH (unsupported program version) error to fall back.  Chuck
> says in the changelog comment:
>
> 	"I tried adding some automatic logic to fall back if registering
> 	with a v4 protocol request failed, but there are too many corner
> 	cases."
>
> Which I can believe, though I haven't looked at it myself.

Here's what I can remember right off the top of my head:

1.  The kernel has to detect whether the local rpcbind has an IPv6  
listener or not.

2.  The kernel has to detect whether user space has configured an IPv6  
loopback address.

3.  The kernel has to detect whether the local rpcbind speaks rpcbind  
v4.

Today, if CONFIG_SUNRPC_REGISTER_V4 is enabled and svc_register()  
can't contact rpcbind's IPv6 listener and issue a v4 SET request, it  
fails and the RPC service is shut down.

The only area that might be trouble is when a sysadmin shuts off ALL  
IPv6 in her network configuration, even if the kernel is build with  
IPv6 support.  The network layer should do the right thing and map the  
IPv6 loopback address to the IPv4 loopback address automatically, but  
I haven't tested this.

I'm also not convinced that people will try to install a 2.6 kernel on  
a distribution that was built for 2.4 or earlier kernels.  There are  
too many missing pieces in the old distributions (like kernel module  
utilities) to make it easy.  So I'm not trying to make this compatible  
with every distribution since the beginning.

> In any case I'd like Trond's ACK or NACK.
>
> --b.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

[J. Bruce Fields]

On Thu, Sep 25, 2008 at 02:13:11PM -0400, Chuck Lever wrote:
> On Sep 25, 2008, at 12:58 PM, J. Bruce Fields wrote:
>> But I'm willing to settle for it and let it be a lesson to us if it
>> turns out to cause more problems than expected.
>
> I will be here to fix it if there is a problem.  In this case, this  
> whole NFS/IPv6 thing is so complicated that I'm implementing just what  
> is needed as we go along.  We can fill in the niceties at a later point.
>
> /me is taking his cue from "lazy evaluation."

OK, fair enough, barring any other objections.

> Today, if CONFIG_SUNRPC_REGISTER_V4 is enabled and svc_register() can't 
> contact rpcbind's IPv6 listener and issue a v4 SET request, it fails and 
> the RPC service is shut down.
>
> The only area that might be trouble is when a sysadmin shuts off ALL  
> IPv6 in her network configuration, even if the kernel is build with IPv6 
> support.  The network layer should do the right thing and map the IPv6 
> loopback address to the IPv4 loopback address automatically, but I 
> haven't tested this.
>
> I'm also not convinced that people will try to install a 2.6 kernel on a 
> distribution that was built for 2.4 or earlier kernels.  There are too 
> many missing pieces in the old distributions (like kernel module  
> utilities) to make it easy.  So I'm not trying to make this compatible  
> with every distribution since the beginning.

Is it 2.4-era distributions that's really the issue?

I thought the userspace rpcbind stuff was still a bit experimental--so
that when the switchover is made to a kernel that supports rpcbind v4,
the userspace that's required will be more recent than that.

Just curious.

--b.

Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

[Chuck Lever]

On Sep 26, 2008, at Sep 26, 2008, 8:03 PM, J. Bruce Fields wrote:
> On Thu, Sep 25, 2008 at 02:13:11PM -0400, Chuck Lever wrote:
>> On Sep 25, 2008, at 12:58 PM, J. Bruce Fields wrote:
>>> But I'm willing to settle for it and let it be a lesson to us if it
>>> turns out to cause more problems than expected.
>>
>> I will be here to fix it if there is a problem.  In this case, this
>> whole NFS/IPv6 thing is so complicated that I'm implementing just  
>> what
>> is needed as we go along.  We can fill in the niceties at a later  
>> point.
>>
>> /me is taking his cue from "lazy evaluation."
>
> OK, fair enough, barring any other objections.
>
>> Today, if CONFIG_SUNRPC_REGISTER_V4 is enabled and svc_register()  
>> can't
>> contact rpcbind's IPv6 listener and issue a v4 SET request, it  
>> fails and
>> the RPC service is shut down.
>>
>> The only area that might be trouble is when a sysadmin shuts off ALL
>> IPv6 in her network configuration, even if the kernel is build with  
>> IPv6
>> support.  The network layer should do the right thing and map the  
>> IPv6
>> loopback address to the IPv4 loopback address automatically, but I
>> haven't tested this.
>>
>> I'm also not convinced that people will try to install a 2.6 kernel  
>> on a
>> distribution that was built for 2.4 or earlier kernels.  There are  
>> too
>> many missing pieces in the old distributions (like kernel module
>> utilities) to make it easy.  So I'm not trying to make this  
>> compatible
>> with every distribution since the beginning.
>
> Is it 2.4-era distributions that's really the issue?

I'm merely trying to bound the amount of backwards-compatibility that  
we realistically have to worry about.  It could be more recent than  
that, even.

> I thought the userspace rpcbind stuff was still a bit experimental--so
> that when the switchover is made to a kernel that supports rpcbind v4,
> the userspace that's required will be more recent than that.

You can drop libtirpc, rpcbind, and the latest nfs-utils on older  
distributions without much difficulty.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

[Peter Staubach]

Chuck Lever wrote:
> The sc_name field is currently 56 bytes long.  This is not large enough
> to hold a pair of IPv6 addresses, the authentication type, the protocol
> name, and a uniquifier number.  The maximum possible size of the name
> string using IPv6 addresses is just under 110 bytes, so I increased the
> size of the sc_name field to accomodate this maximum.
>
> In addition, the strings in the nfs4_setclientid structure are
> constructed with scnprintf(), which wants to terminate its output with
> '\0'.  The sc_netid field was large enough only for a three byte netid
> string and a '\0' so inet6 netids were being truncated.  Perhaps we
> don't need the overhead of scnprintf() to do a simple string copy, but
> I fixed this by increasing the size of the buffer by one byte.
>
> Since all three of the string buffers in nfs4_setclientid are
> constructed with scnprintf(), I increased the size of all three by one
> byte to document the requirement, although I don't think either the
> universal address field or the name field will be so small that these
> strings get truncated in this way.
>
> The size of the Linux client's client ID on the wire will be larger
> than before.  RFC 3530 suggests the size limit for client IDs is 1024,
> and we are still well below that.
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>
>  include/linux/nfs_xdr.h |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 8c77c11..dc34977 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -672,16 +672,16 @@ struct nfs4_rename_res {
>  	struct nfs_fattr *		new_fattr;
>  };
>  
> -#define NFS4_SETCLIENTID_NAMELEN	(56)
> +#define NFS4_SETCLIENTID_NAMELEN	(128)
>   

Perhaps (127) might have been a better choice here?  In the
struct below, the arrays end up being allocated to (128) + 1
plus whatever alignment bytes are valid for the platform.
This wastes a fair amount of space.

       ps

>  struct nfs4_setclientid {
>  	const nfs4_verifier *		sc_verifier;
>  	unsigned int			sc_name_len;
> -	char				sc_name[NFS4_SETCLIENTID_NAMELEN];
> +	char				sc_name[NFS4_SETCLIENTID_NAMELEN + 1];
>  	u32				sc_prog;
>  	unsigned int			sc_netid_len;
> -	char				sc_netid[RPCBIND_MAXNETIDLEN];
> +	char				sc_netid[RPCBIND_MAXNETIDLEN + 1];
>  	unsigned int			sc_uaddr_len;
> -	char				sc_uaddr[RPCBIND_MAXUADDRLEN];
> +	char				sc_uaddr[RPCBIND_MAXUADDRLEN + 1];
>  	u32				sc_cb_ident;
>  };
>  
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

[Chuck Lever]

On Sep 25, 2008, at 1:35 PM, Peter Staubach wrote:

> Chuck Lever wrote:
>> The sc_name field is currently 56 bytes long.  This is not large  
>> enough
>> to hold a pair of IPv6 addresses, the authentication type, the  
>> protocol
>> name, and a uniquifier number.  The maximum possible size of the name
>> string using IPv6 addresses is just under 110 bytes, so I increased  
>> the
>> size of the sc_name field to accomodate this maximum.
>>
>> In addition, the strings in the nfs4_setclientid structure are
>> constructed with scnprintf(), which wants to terminate its output  
>> with
>> '\0'.  The sc_netid field was large enough only for a three byte  
>> netid
>> string and a '\0' so inet6 netids were being truncated.  Perhaps we
>> don't need the overhead of scnprintf() to do a simple string copy,  
>> but
>> I fixed this by increasing the size of the buffer by one byte.
>>
>> Since all three of the string buffers in nfs4_setclientid are
>> constructed with scnprintf(), I increased the size of all three by  
>> one
>> byte to document the requirement, although I don't think either the
>> universal address field or the name field will be so small that these
>> strings get truncated in this way.
>>
>> The size of the Linux client's client ID on the wire will be larger
>> than before.  RFC 3530 suggests the size limit for client IDs is  
>> 1024,
>> and we are still well below that.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> include/linux/nfs_xdr.h |    8 ++++----
>> 1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
>> index 8c77c11..dc34977 100644
>> --- a/include/linux/nfs_xdr.h
>> +++ b/include/linux/nfs_xdr.h
>> @@ -672,16 +672,16 @@ struct nfs4_rename_res {
>> 	struct nfs_fattr *		new_fattr;
>> };
>> -#define NFS4_SETCLIENTID_NAMELEN	(56)
>> +#define NFS4_SETCLIENTID_NAMELEN	(128)
>>
>
> Perhaps (127) might have been a better choice here?  In the
> struct below, the arrays end up being allocated to (128) + 1
> plus whatever alignment bytes are valid for the platform.
> This wastes a fair amount of space.

Probably the worst alignment padding would be 7 bytes.  The struct is  
already large, and is only used once on the stack just after mounting,  
so I'm not that concerned.  I would rather have a little extra here  
than not enough.

However, we can trim the length of the name a little.  I counted about  
105 characters maximum in my simple calculations.  We could probably  
make it (111) to round out the field to 112 bytes (divisible by 8).

>       ps
>
>> struct nfs4_setclientid {
>> 	const nfs4_verifier *		sc_verifier;
>> 	unsigned int			sc_name_len;
>> -	char				sc_name[NFS4_SETCLIENTID_NAMELEN];
>> +	char				sc_name[NFS4_SETCLIENTID_NAMELEN + 1];
>> 	u32				sc_prog;
>> 	unsigned int			sc_netid_len;
>> -	char				sc_netid[RPCBIND_MAXNETIDLEN];
>> +	char				sc_netid[RPCBIND_MAXNETIDLEN + 1];
>> 	unsigned int			sc_uaddr_len;
>> -	char				sc_uaddr[RPCBIND_MAXUADDRLEN];
>> +	char				sc_uaddr[RPCBIND_MAXUADDRLEN + 1];
>> 	u32				sc_cb_ident;
>> };
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux- 
>> nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 0/3] Bake-a-thon fixes

[J. Bruce Fields]

On Thu, Sep 25, 2008 at 11:56:47AM -0400, Chuck Lever wrote:
> Hi Trond, Bruce-
> 
> I know it is late, but please consider these three bug fixes for 2.6.28.

Thanks, I've applied 1 and 2.

--b.

> 
> The first patch is another result of Olaf's recent exploration of the
> Linux rpcbind port.  It belongs with the other patches for 2.6.28 that
> provide proper support for IPv6 and rpcbind v4 when the kernel registers
> its RPC services with the local rpcbind daemon.
> 
> The second patch is a simple clean up that reduces the verbosity of
> rpcbind debugging messages.
> 
> The third is important for NFSv4 over IPv6.  Without it, the NFSv4.0
> callback channel does not work properly over IPv6.  There may be more
> problems there, so this does not yet guarantee that we have working
> callback support over IPv6.
> 
> ---
> 
> Chuck Lever (3):
>       NFS: SETCLIENTID truncates client ID and netid
>       SUNRPC: Clean up debug messages in rpcb_clnt.c
>       SUNRPC: Fix up svc_unregister()
> 
> 
>  include/linux/nfs_xdr.h |    8 +++---
>  net/sunrpc/rpcb_clnt.c  |   14 ++++-------
>  net/sunrpc/svc.c        |   58 +++++++++++++++++++++++++++++++----------------
>  3 files changed, 47 insertions(+), 33 deletions(-)
> 
> -- 
> Chuck Lever
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html