[PATCH 00/23] Shorter series for 2.6.30

[PATCH 00/23] Shorter series for 2.6.30

[Chuck Lever]

Hi Trond, Bruce-

Since Trond has sent some late-breaking patches for 2.6.29, I had to
fix some merge problems with my latest patch set.  Jeff also found a
minor bug with rpcbind version 4 replies that I'd like to include.

I've stripped this patch set down so it is just 23 patches.
Unfortunately, it touches server-side RPC stuff and client-side NFSv4
callback and lockd support.  I don't see a clean way to split these up.

Can we discuss how to get these into 2.6.30?  Perhaps, since these have
the greatest impact on client behavior, Trond should take all of these?

---

Chuck Lever (23):
      NFS: Simplify logic to compare socket addresses in client.c
      NFS: Start PF_INET6 callback listener only if IPv6 support is available
      lockd: Start PF_INET6 listener only if IPv6 support is available
      SUNRPC: Remove CONFIG_SUNRPC_REGISTER_V4
      SUNRPC: rpcb_register() should handle errors silently
      SUNRPC: Simplify kernel RPC service registration
      SUNRPC: Simplify svc_unregister()
      SUNRPC: Allow callers to pass rpcb_v4_register a NULL address
      SUNRPC: rpcbind actually interprets r_owner string
      SUNRPC: Clean up address type casts in rpcb_v4_register()
      SUNRPC: Don't return EPROTONOSUPPORT in svc_register()'s helpers
      SUNRPC: Use IPv4 loopback for registering AF_INET6 kernel RPC services
      SUNRPC: Set IPV6ONLY flag on PF_INET6 RPC listener sockets
      NFS: Revert creation of IPv6 listeners for lockd and NFSv4 callbacks
      SUNRPC: Remove @family argument from svc_create() and svc_create_pooled()
      SUNRPC: Change svc_create_xprt() to take a @family argument
      SUNRPC: svc_setup_socket() gets protocol family from socket
      SUNRPC: Pass a family argument to svc_register()
      SUNRPC: Clean up svc_find_xprt() calling sequence
      NFSD: If port value written to /proc/fs/nfsd/portlist is invalid, return EINVAL
      SUNRPC: Clean up static inline functions in svc_xprt.h
      SUNRPC: Fix return type of svc_addr_len()
      SUNRPC: Don't flag empty RPCB_GETADDR reply as bogus


 fs/lockd/clntlock.c                      |   51 ----------
 fs/lockd/svc.c                           |   42 ++++----
 fs/nfs/callback.c                        |   31 +++---
 fs/nfs/callback.h                        |    1 
 fs/nfs/client.c                          |  116 ++++++++++------------
 fs/nfs/nfs4state.c                       |   10 ++
 fs/nfsd/nfsctl.c                         |    6 +
 fs/nfsd/nfssvc.c                         |    5 -
 include/linux/sunrpc/svc.h               |    9 +-
 include/linux/sunrpc/svc_xprt.h          |   55 ++++++----
 net/sunrpc/Kconfig                       |   22 ----
 net/sunrpc/rpcb_clnt.c                   |  103 +++++++++++---------
 net/sunrpc/svc.c                         |  158 ++++++++++++++----------------
 net/sunrpc/svc_xprt.c                    |   31 ++++--
 net/sunrpc/svcsock.c                     |   14 +--
 net/sunrpc/xprtrdma/svc_rdma_transport.c |   16 +++
 16 files changed, 314 insertions(+), 356 deletions(-)

-- 
Chuck Lever

[PATCH 01/23] SUNRPC: Don't flag empty RPCB_GETADDR reply as bogus

[Chuck Lever]

In 2007, commit e65fe3976f594603ed7b1b4a99d3e9b867f573ea added
additional sanity checking to rpcb_decode_getaddr() to make sure we
were getting a reply that was long enough to be an actual universal
address.  If the uaddr string isn't long enough, the XDR decoder
returns EIO.

However, an empty string is a valid RPCB_GETADDR response if the
requested service isn't registered.  Moreover, "::.n.m" is also a
valid RPCB_GETADDR response for IPv6 addresses that is shorter
than rpcb_decode_getaddr()'s lower limit of 11.  So this sanity
check introduced a regression for rpcbind requests against IPv6
remotes.

So revert the lower bound check added by commit
e65fe3976f594603ed7b1b4a99d3e9b867f573ea, and add an explicit check
for an empty uaddr string, similar to libtirpc's rpcb_getaddr(3).

Pointed-out-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 net/sunrpc/rpcb_clnt.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 03ae007..2caa7ed 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -703,11 +703,16 @@ static int rpcb_decode_getaddr(struct rpc_rqst *req, __be32 *p,
 	*portp = 0;
 	addr_len = ntohl(*p++);
 
+	if (addr_len == 0) {
+		dprintk("RPC:       rpcb_decode_getaddr: "
+					"service is not registered\n");
+		return 0;
+	}
+
 	/*
-	 * Simple sanity check.  The smallest possible universal
-	 * address is an IPv4 address string containing 11 bytes.
+	 * Simple sanity check.
 	 */
-	if (addr_len < 11 || addr_len > RPCBIND_MAXUADDRLEN)
+	if (addr_len > RPCBIND_MAXUADDRLEN)
 		goto out_err;
 
 	/*

[PATCH 02/23] SUNRPC: Fix return type of svc_addr_len()

[Chuck Lever]

The svc_addr_len() helper function can return a negative errno value,
but its return type is size_t, which is unsigned.

The RDMA transport code passes this return value directly to memset(),
without checking first if it's negative.  This could cause memset() to
clobber a large piece of memory if svc_addr_len() has returned an
error.

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

 include/linux/sunrpc/svc_xprt.h          |    3 ++-
 net/sunrpc/xprtrdma/svc_rdma_transport.c |   16 ++++++++++++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index 0127dac..c2aa8cd 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -113,7 +113,7 @@ static inline unsigned short svc_addr_port(struct sockaddr *sa)
 	return ret;
 }
 
-static inline size_t svc_addr_len(struct sockaddr *sa)
+static inline int svc_addr_len(const struct sockaddr *sa)
 {
 	switch (sa->sa_family) {
 	case AF_INET:
@@ -121,6 +121,7 @@ static inline size_t svc_addr_len(struct sockaddr *sa)
 	case AF_INET6:
 		return sizeof(struct sockaddr_in6);
 	}
+
 	return -EAFNOSUPPORT;
 }
 
diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index 3d810e7..d1ec6f9 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -546,6 +546,7 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
 	struct svcxprt_rdma *listen_xprt = new_cma_id->context;
 	struct svcxprt_rdma *newxprt;
 	struct sockaddr *sa;
+	int len;
 
 	/* Create a new transport */
 	newxprt = rdma_create_xprt(listen_xprt->sc_xprt.xpt_server, 0);
@@ -563,9 +564,20 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
 
 	/* Set the local and remote addresses in the transport */
 	sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
-	svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
+	len = svc_addr_len(sa);
+	if (len < 0) {
+		dprintk("svcrdma: dst_addr has a bad address family\n");
+		return;
+	}
+	svc_xprt_set_remote(&newxprt->sc_xprt, sa, len);
+
 	sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
-	svc_xprt_set_local(&newxprt->sc_xprt, sa, svc_addr_len(sa));
+	len = svc_addr_len(sa);
+	if (len < 0) {
+		dprintk("svcrdma: src_addr has a bad address family\n");
+		return;
+	}
+	svc_xprt_set_local(&newxprt->sc_xprt, sa, len);
 
 	/*
 	 * Enqueue the new transport on the accept queue of the listening

[PATCH 03/23] SUNRPC: Clean up static inline functions in svc_xprt.h

[Chuck Lever]

Clean up:  Enable the use of const arguments in higher level svc_ APIs
by adding const to the arguments of the helper functions in svc_xprt.h

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

 include/linux/sunrpc/svc_xprt.h |   46 ++++++++++++++++++++++-----------------
 1 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index c2aa8cd..ee21e8a 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -88,29 +88,32 @@ static inline void svc_xprt_get(struct svc_xprt *xprt)
 	kref_get(&xprt->xpt_ref);
 }
 static inline void svc_xprt_set_local(struct svc_xprt *xprt,
-				      struct sockaddr *sa, int salen)
+				      const struct sockaddr *sa,
+				      const size_t salen)
 {
 	memcpy(&xprt->xpt_local, sa, salen);
 	xprt->xpt_locallen = salen;
 }
 static inline void svc_xprt_set_remote(struct svc_xprt *xprt,
-				       struct sockaddr *sa, int salen)
+				       const struct sockaddr *sa,
+				       const size_t salen)
 {
 	memcpy(&xprt->xpt_remote, sa, salen);
 	xprt->xpt_remotelen = salen;
 }
-static inline unsigned short svc_addr_port(struct sockaddr *sa)
+static inline unsigned short svc_addr_port(const struct sockaddr *sa)
 {
-	unsigned short ret = 0;
+	const struct sockaddr_in *sin = (const struct sockaddr_in *)sa;
+	const struct sockaddr_in6 *sin6 = (const struct sockaddr_in6 *)sa;
+
 	switch (sa->sa_family) {
 	case AF_INET:
-		ret = ntohs(((struct sockaddr_in *)sa)->sin_port);
-		break;
+		return ntohs(sin->sin_port);
 	case AF_INET6:
-		ret = ntohs(((struct sockaddr_in6 *)sa)->sin6_port);
-		break;
+		return ntohs(sin6->sin6_port);
 	}
-	return ret;
+
+	return 0;
 }
 
 static inline int svc_addr_len(const struct sockaddr *sa)
@@ -125,36 +128,39 @@ static inline int svc_addr_len(const struct sockaddr *sa)
 	return -EAFNOSUPPORT;
 }
 
-static inline unsigned short svc_xprt_local_port(struct svc_xprt *xprt)
+static inline unsigned short svc_xprt_local_port(const struct svc_xprt *xprt)
 {
-	return svc_addr_port((struct sockaddr *)&xprt->xpt_local);
+	return svc_addr_port((const struct sockaddr *)&xprt->xpt_local);
 }
 
-static inline unsigned short svc_xprt_remote_port(struct svc_xprt *xprt)
+static inline unsigned short svc_xprt_remote_port(const struct svc_xprt *xprt)
 {
-	return svc_addr_port((struct sockaddr *)&xprt->xpt_remote);
+	return svc_addr_port((const struct sockaddr *)&xprt->xpt_remote);
 }
 
-static inline char *__svc_print_addr(struct sockaddr *addr,
-				     char *buf, size_t len)
+static inline char *__svc_print_addr(const struct sockaddr *addr,
+				     char *buf, const size_t len)
 {
+	const struct sockaddr_in *sin = (const struct sockaddr_in *)addr;
+	const struct sockaddr_in6 *sin6 = (const struct sockaddr_in6 *)addr;
+
 	switch (addr->sa_family) {
 	case AF_INET:
-		snprintf(buf, len, "%pI4, port=%u",
-			&((struct sockaddr_in *)addr)->sin_addr,
-			ntohs(((struct sockaddr_in *) addr)->sin_port));
+		snprintf(buf, len, "%pI4, port=%u", &sin->sin_addr,
+			ntohs(sin->sin_port));
 		break;
 
 	case AF_INET6:
 		snprintf(buf, len, "%pI6, port=%u",
-			 &((struct sockaddr_in6 *)addr)->sin6_addr,
-			ntohs(((struct sockaddr_in6 *) addr)->sin6_port));
+			 &sin6->sin6_addr,
+			ntohs(sin6->sin6_port));
 		break;
 
 	default:
 		snprintf(buf, len, "unknown address type: %d", addr->sa_family);
 		break;
 	}
+
 	return buf;
 }
 #endif /* SUNRPC_SVC_XPRT_H */

[PATCH 04/23] NFSD: If port value written to /proc/fs/nfsd/portlist is invalid, return EINVAL

[Chuck Lever]

Make sure port value read from user space by write_ports is valid before
passing it to svc_find_xprt().  If it wasn't, the writer would get ENOENT
instead of EINVAL.

Noticed-by: J. Bruce Fields <bfields@fieldses.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfsd/nfsctl.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 3d93b20..5a936c1 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -938,6 +938,8 @@ static ssize_t __write_ports(struct file *file, char *buf, size_t size)
 		char transport[16];
 		int port;
 		if (sscanf(buf, "%15s %4d", transport, &port) == 2) {
+			if (port < 1 || port > 65535)
+				return -EINVAL;
 			err = nfsd_create_serv();
 			if (!err) {
 				err = svc_create_xprt(nfsd_serv,
@@ -960,7 +962,7 @@ static ssize_t __write_ports(struct file *file, char *buf, size_t size)
 		char transport[16];
 		int port;
 		if (sscanf(&buf[1], "%15s %4d", transport, &port) == 2) {
-			if (port == 0)
+			if (port < 1 || port > 65535)
 				return -EINVAL;
 			if (nfsd_serv) {
 				xprt = svc_find_xprt(nfsd_serv, transport,

[PATCH 05/23] SUNRPC: Clean up svc_find_xprt() calling sequence

[Chuck Lever]

Clean up: add documentating comment and use appropriate data types for
svc_find_xprt()'s arguments.

This also eliminates a mixed sign comparison: @port was an int, while
the return value of svc_xprt_local_port() is an unsigned short.

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

 include/linux/sunrpc/svc_xprt.h |    3 ++-
 net/sunrpc/svc_xprt.c           |   16 +++++++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index ee21e8a..167e4b5 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -80,7 +80,8 @@ void	svc_close_xprt(struct svc_xprt *xprt);
 void	svc_delete_xprt(struct svc_xprt *xprt);
 int	svc_port_is_privileged(struct sockaddr *sin);
 int	svc_print_xprts(char *buf, int maxlen);
-struct	svc_xprt *svc_find_xprt(struct svc_serv *, char *, int, int);
+struct	svc_xprt *svc_find_xprt(struct svc_serv *serv, const char *xcl_name,
+			const sa_family_t af, const unsigned short port);
 int	svc_xprt_names(struct svc_serv *serv, char *buf, int buflen);
 
 static inline void svc_xprt_get(struct svc_xprt *xprt)
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index e588df5..c947c93 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -1033,7 +1033,13 @@ static struct svc_deferred_req *svc_deferred_dequeue(struct svc_xprt *xprt)
 	return dr;
 }
 
-/*
+/**
+ * svc_find_xprt - find an RPC transport instance
+ * @serv: pointer to svc_serv to search
+ * @xcl_name: C string containing transport's class name
+ * @af: Address family of transport's local address
+ * @port: transport's IP port number
+ *
  * Return the transport instance pointer for the endpoint accepting
  * connections/peer traffic from the specified transport class,
  * address family and port.
@@ -1042,14 +1048,14 @@ static struct svc_deferred_req *svc_deferred_dequeue(struct svc_xprt *xprt)
  * wild-card, and will result in matching the first transport in the
  * service's list that has a matching class name.
  */
-struct svc_xprt *svc_find_xprt(struct svc_serv *serv, char *xcl_name,
-			       int af, int port)
+struct svc_xprt *svc_find_xprt(struct svc_serv *serv, const char *xcl_name,
+			       const sa_family_t af, const unsigned short port)
 {
 	struct svc_xprt *xprt;
 	struct svc_xprt *found = NULL;
 
 	/* Sanity check the args */
-	if (!serv || !xcl_name)
+	if (serv == NULL || xcl_name == NULL)
 		return found;
 
 	spin_lock_bh(&serv->sv_lock);
@@ -1058,7 +1064,7 @@ struct svc_xprt *svc_find_xprt(struct svc_serv *serv, char *xcl_name,
 			continue;
 		if (af != AF_UNSPEC && af != xprt->xpt_local.ss_family)
 			continue;
-		if (port && port != svc_xprt_local_port(xprt))
+		if (port != 0 && port != svc_xprt_local_port(xprt))
 			continue;
 		found = xprt;
 		svc_xprt_get(xprt);

[PATCH 06/23] SUNRPC: Pass a family argument to svc_register()

[Chuck Lever]

The sv_family field is going away.  Instead of using sv_family, have
the svc_register() function take a protocol family argument.

Since this argument represents a protocol family, and not an address
family, this argument takes an int, as this is what is passed to
sock_create_kern().  Also make sure svc_register's helpers are
checking for PF_FOO instead of AF_FOO.  The value of [AP]F_FOO are
equivalent; this is simply a symbolic change to reflect the semantics
of the value stored in that variable.

sock_create_kern() should return EPFNOSUPPORT if the passed-in
protocol family isn't supported, but it uses EAFNOSUPPORT for this
case.  We will stick with that tradition here, as svc_register()
is called by the RPC server in the same path as sock_create_kern().

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

 include/linux/sunrpc/svc.h |    4 ++--
 net/sunrpc/svc.c           |   21 +++++++++++----------
 net/sunrpc/svcsock.c       |    2 +-
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 3435d24..1f18fc7 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -396,8 +396,8 @@ struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
 int		   svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
 void		   svc_destroy(struct svc_serv *);
 int		   svc_process(struct svc_rqst *);
-int		   svc_register(const struct svc_serv *, const unsigned short,
-				const unsigned short);
+int		   svc_register(const struct svc_serv *, const int,
+				const unsigned short, const unsigned short);
 
 void		   svc_wake_up(struct svc_serv *);
 void		   svc_reserve(struct svc_rqst *rqstp, int space);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index c51fed4..41bc36e 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -800,17 +800,17 @@ static int __svc_rpcb_register6(const u32 program, const u32 version,
  * if any error occurs.
  */
 static int __svc_register(const u32 program, const u32 version,
-			  const sa_family_t family,
+			  const int family,
 			  const unsigned short protocol,
 			  const unsigned short port)
 {
 	int error;
 
 	switch (family) {
-	case AF_INET:
+	case PF_INET:
 		return __svc_rpcb_register4(program, version,
 						protocol, port);
-	case AF_INET6:
+	case PF_INET6:
 		error = __svc_rpcb_register6(program, version,
 						protocol, port);
 		if (error < 0)
@@ -840,11 +840,11 @@ static int __svc_register(const u32 program, const u32 version,
  * if any error occurs.
  */
 static int __svc_register(const u32 program, const u32 version,
-			  sa_family_t family,
+			  const int family,
 			  const unsigned short protocol,
 			  const unsigned short port)
 {
-	if (family != AF_INET)
+	if (family != PF_INET)
 		return -EAFNOSUPPORT;
 
 	return rpcb_register(program, version, protocol, port);
@@ -855,13 +855,14 @@ static int __svc_register(const u32 program, const u32 version,
 /**
  * svc_register - register an RPC service with the local portmapper
  * @serv: svc_serv struct for the service to register
+ * @family: protocol family of service's listener socket
  * @proto: transport protocol number to advertise
  * @port: port to advertise
  *
- * Service is registered for any address in serv's address family
+ * Service is registered for any address in the passed-in protocol family
  */
-int svc_register(const struct svc_serv *serv, const unsigned short proto,
-		 const unsigned short port)
+int svc_register(const struct svc_serv *serv, const int family,
+		 const unsigned short proto, const unsigned short port)
 {
 	struct svc_program	*progp;
 	unsigned int		i;
@@ -879,7 +880,7 @@ int svc_register(const struct svc_serv *serv, const unsigned short proto,
 					i,
 					proto == IPPROTO_UDP?  "udp" : "tcp",
 					port,
-					serv->sv_family,
+					family,
 					progp->pg_vers[i]->vs_hidden?
 						" (but not telling portmap)" : "");
 
@@ -887,7 +888,7 @@ int svc_register(const struct svc_serv *serv, const unsigned short proto,
 				continue;
 
 			error = __svc_register(progp->pg_prog, i,
-						serv->sv_family, proto, port);
+						family, proto, port);
 			if (error < 0)
 				break;
 		}
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 5763e64..d00583c 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1122,7 +1122,7 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
 
 	/* Register socket with portmapper */
 	if (*errp >= 0 && pmap_register)
-		*errp = svc_register(serv, inet->sk_protocol,
+		*errp = svc_register(serv, serv->sv_family, inet->sk_protocol,
 				     ntohs(inet_sk(inet)->sport));
 
 	if (*errp < 0) {

[PATCH 07/23] SUNRPC: svc_setup_socket() gets protocol family from socket

[Chuck Lever]

Since the sv_family field is going away, modify svc_setup_socket() to
extract the protocol family from the passed-in socket instead of from
the passed-in svc_serv struct.

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

 net/sunrpc/svcsock.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index d00583c..d00bc33 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1122,7 +1122,7 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
 
 	/* Register socket with portmapper */
 	if (*errp >= 0 && pmap_register)
-		*errp = svc_register(serv, serv->sv_family, inet->sk_protocol,
+		*errp = svc_register(serv, inet->sk_family, inet->sk_protocol,
 				     ntohs(inet_sk(inet)->sport));
 
 	if (*errp < 0) {
@@ -1145,13 +1145,13 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
 
 	/*
 	 * We start one listener per sv_serv.  We want AF_INET
-	 * requests to be automatically shunted to our AF_INET6
+	 * requests to be automatically shunted to our PF_INET6
 	 * listener using a mapped IPv4 address.  Make sure
 	 * no-one starts an equivalent IPv4 listener, which
 	 * would steal our incoming connections.
 	 */
 	val = 0;
-	if (serv->sv_family == AF_INET6)
+	if (inet->sk_family == PF_INET6)
 		kernel_setsockopt(sock, SOL_IPV6, IPV6_V6ONLY,
 					(char *)&val, sizeof(val));
 

[PATCH 08/23] SUNRPC: Change svc_create_xprt() to take a @family argument

[Chuck Lever]

The sv_family field is going away.  Pass a protocol family argument to
svc_create_xprt() instead of extracting the family from the passed-in
svc_serv struct.

Again, as this is a listener socket and not an address, we make this
new argument an "int" protocol family, instead of an "sa_family_t."

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

 fs/lockd/svc.c                  |    3 ++-
 fs/nfs/callback.c               |    4 ++--
 fs/nfsd/nfsctl.c                |    2 +-
 fs/nfsd/nfssvc.c                |    4 ++--
 include/linux/sunrpc/svc_xprt.h |    3 ++-
 net/sunrpc/svc_xprt.c           |   15 +++++++++------
 6 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 64f1c31..390c559 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -211,7 +211,8 @@ static int create_lockd_listener(struct svc_serv *serv, char *name,
 
 	xprt = svc_find_xprt(serv, name, 0, 0);
 	if (xprt == NULL)
-		return svc_create_xprt(serv, name, port, SVC_SOCK_DEFAULTS);
+		return svc_create_xprt(serv, name, nlmsvc_family,
+					port, SVC_SOCK_DEFAULTS);
 
 	svc_xprt_put(xprt);
 	return 0;
diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 3e634f2..fb35cab 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -122,8 +122,8 @@ int nfs_callback_up(void)
 	if (!serv)
 		goto out_err;
 
-	ret = svc_create_xprt(serv, "tcp", nfs_callback_set_tcpport,
-			      SVC_SOCK_ANONYMOUS);
+	ret = svc_create_xprt(serv, "tcp", nfs_callback_family,
+				nfs_callback_set_tcpport, SVC_SOCK_ANONYMOUS);
 	if (ret <= 0)
 		goto out_err;
 	nfs_callback_tcpport = ret;
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 5a936c1..a4ed864 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -943,7 +943,7 @@ static ssize_t __write_ports(struct file *file, char *buf, size_t size)
 			err = nfsd_create_serv();
 			if (!err) {
 				err = svc_create_xprt(nfsd_serv,
-						      transport, port,
+						      transport, PF_INET, port,
 						      SVC_SOCK_ANONYMOUS);
 				if (err == -ENOENT)
 					/* Give a reasonable perror msg for
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 07e4f5d..ab7f249 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -244,7 +244,7 @@ static int nfsd_init_socks(int port)
 	if (!list_empty(&nfsd_serv->sv_permsocks))
 		return 0;
 
-	error = svc_create_xprt(nfsd_serv, "udp", port,
+	error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
 					SVC_SOCK_DEFAULTS);
 	if (error < 0)
 		return error;
@@ -253,7 +253,7 @@ static int nfsd_init_socks(int port)
 	if (error < 0)
 		return error;
 
-	error = svc_create_xprt(nfsd_serv, "tcp", port,
+	error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
 					SVC_SOCK_DEFAULTS);
 	if (error < 0)
 		return error;
diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index 167e4b5..72c5e9c 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -71,7 +71,8 @@ int	svc_reg_xprt_class(struct svc_xprt_class *);
 void	svc_unreg_xprt_class(struct svc_xprt_class *);
 void	svc_xprt_init(struct svc_xprt_class *, struct svc_xprt *,
 		      struct svc_serv *);
-int	svc_create_xprt(struct svc_serv *, char *, unsigned short, int);
+int	svc_create_xprt(struct svc_serv *, const char *, const int,
+			const unsigned short, int);
 void	svc_xprt_enqueue(struct svc_xprt *xprt);
 void	svc_xprt_received(struct svc_xprt *);
 void	svc_xprt_put(struct svc_xprt *xprt);
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index c947c93..2819ee0 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -161,7 +161,9 @@ EXPORT_SYMBOL_GPL(svc_xprt_init);
 
 static struct svc_xprt *__svc_xpo_create(struct svc_xprt_class *xcl,
 					 struct svc_serv *serv,
-					 unsigned short port, int flags)
+					 const int family,
+					 const unsigned short port,
+					 int flags)
 {
 	struct sockaddr_in sin = {
 		.sin_family		= AF_INET,
@@ -176,12 +178,12 @@ static struct svc_xprt *__svc_xpo_create(struct svc_xprt_class *xcl,
 	struct sockaddr *sap;
 	size_t len;
 
-	switch (serv->sv_family) {
-	case AF_INET:
+	switch (family) {
+	case PF_INET:
 		sap = (struct sockaddr *)&sin;
 		len = sizeof(sin);
 		break;
-	case AF_INET6:
+	case PF_INET6:
 		sap = (struct sockaddr *)&sin6;
 		len = sizeof(sin6);
 		break;
@@ -192,7 +194,8 @@ static struct svc_xprt *__svc_xpo_create(struct svc_xprt_class *xcl,
 	return xcl->xcl_ops->xpo_create(serv, sap, len, flags);
 }
 
-int svc_create_xprt(struct svc_serv *serv, char *xprt_name, unsigned short port,
+int svc_create_xprt(struct svc_serv *serv, const char *xprt_name,
+		    const int family, const unsigned short port,
 		    int flags)
 {
 	struct svc_xprt_class *xcl;
@@ -209,7 +212,7 @@ int svc_create_xprt(struct svc_serv *serv, char *xprt_name, unsigned short port,
 			goto err;
 
 		spin_unlock(&svc_xprt_class_lock);
-		newxprt = __svc_xpo_create(xcl, serv, port, flags);
+		newxprt = __svc_xpo_create(xcl, serv, family, port, flags);
 		if (IS_ERR(newxprt)) {
 			module_put(xcl->xcl_owner);
 			return PTR_ERR(newxprt);

[PATCH 09/23] SUNRPC: Remove @family argument from svc_create() and svc_create_pooled()

[Chuck Lever]

Since an RPC service listener's protocol family is specified now via
svc_create_xprt(), it no longer needs to be passed to svc_create() or
svc_create_pooled().  Remove that argument from the synopsis of those
functions, and remove the sv_family field from the svc_serv struct.

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

 fs/lockd/svc.c             |    2 +-
 fs/nfs/callback.c          |    3 +--
 fs/nfsd/nfssvc.c           |    1 -
 include/linux/sunrpc/svc.h |    5 ++---
 net/sunrpc/svc.c           |   11 +++++------
 5 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 390c559..d309200 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -275,7 +275,7 @@ int lockd_up(void)
 			"lockd_up: no pid, %d users??\n", nlmsvc_users);
 
 	error = -ENOMEM;
-	serv = svc_create(&nlmsvc_program, LOCKD_BUFSIZE, nlmsvc_family, NULL);
+	serv = svc_create(&nlmsvc_program, LOCKD_BUFSIZE, NULL);
 	if (!serv) {
 		printk(KERN_WARNING "lockd_up: create service failed\n");
 		goto out;
diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index fb35cab..ddf4b4a 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -116,8 +116,7 @@ int nfs_callback_up(void)
 	mutex_lock(&nfs_callback_mutex);
 	if (nfs_callback_info.users++ || nfs_callback_info.task != NULL)
 		goto out;
-	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE,
-				nfs_callback_family, NULL);
+	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE, NULL);
 	ret = -ENOMEM;
 	if (!serv)
 		goto out_err;
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index ab7f249..bc3567b 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -229,7 +229,6 @@ int nfsd_create_serv(void)
 
 	atomic_set(&nfsd_busy, 0);
 	nfsd_serv = svc_create_pooled(&nfsd_program, nfsd_max_blksize,
-				      AF_INET,
 				      nfsd_last_thread, nfsd, THIS_MODULE);
 	if (nfsd_serv == NULL)
 		err = -ENOMEM;
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 1f18fc7..d3a4c02 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -69,7 +69,6 @@ struct svc_serv {
 	struct list_head	sv_tempsocks;	/* all temporary sockets */
 	int			sv_tmpcnt;	/* count of temporary sockets */
 	struct timer_list	sv_temptimer;	/* timer for aging temporary sockets */
-	sa_family_t		sv_family;	/* listener's address family */
 
 	char *			sv_name;	/* service name */
 
@@ -385,13 +384,13 @@ struct svc_procedure {
 /*
  * Function prototypes.
  */
-struct svc_serv *svc_create(struct svc_program *, unsigned int, sa_family_t,
+struct svc_serv *svc_create(struct svc_program *, unsigned int,
 			    void (*shutdown)(struct svc_serv *));
 struct svc_rqst *svc_prepare_thread(struct svc_serv *serv,
 					struct svc_pool *pool);
 void		   svc_exit_thread(struct svc_rqst *);
 struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
-			sa_family_t, void (*shutdown)(struct svc_serv *),
+			void (*shutdown)(struct svc_serv *),
 			svc_thread_fn, struct module *);
 int		   svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
 void		   svc_destroy(struct svc_serv *);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 41bc36e..d72ff44 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -359,7 +359,7 @@ svc_pool_for_cpu(struct svc_serv *serv, int cpu)
  */
 static struct svc_serv *
 __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
-	   sa_family_t family, void (*shutdown)(struct svc_serv *serv))
+	     void (*shutdown)(struct svc_serv *serv))
 {
 	struct svc_serv	*serv;
 	unsigned int vers;
@@ -368,7 +368,6 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
 
 	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
 		return NULL;
-	serv->sv_family    = family;
 	serv->sv_name      = prog->pg_name;
 	serv->sv_program   = prog;
 	serv->sv_nrthreads = 1;
@@ -427,21 +426,21 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
 
 struct svc_serv *
 svc_create(struct svc_program *prog, unsigned int bufsize,
-		sa_family_t family, void (*shutdown)(struct svc_serv *serv))
+	   void (*shutdown)(struct svc_serv *serv))
 {
-	return __svc_create(prog, bufsize, /*npools*/1, family, shutdown);
+	return __svc_create(prog, bufsize, /*npools*/1, shutdown);
 }
 EXPORT_SYMBOL_GPL(svc_create);
 
 struct svc_serv *
 svc_create_pooled(struct svc_program *prog, unsigned int bufsize,
-		  sa_family_t family, void (*shutdown)(struct svc_serv *serv),
+		  void (*shutdown)(struct svc_serv *serv),
 		  svc_thread_fn func, struct module *mod)
 {
 	struct svc_serv *serv;
 	unsigned int npools = svc_pool_map_get();
 
-	serv = __svc_create(prog, bufsize, npools, family, shutdown);
+	serv = __svc_create(prog, bufsize, npools, shutdown);
 
 	if (serv != NULL) {
 		serv->sv_function = func;

[PATCH 10/23] NFS: Revert creation of IPv6 listeners for lockd and NFSv4 callbacks

[Chuck Lever]

We're about to convert over to using separate PF_INET and PF_INET6
listeners, instead of a single PF_INET6 listener that also receives
AF_INET requests and maps them to AF_INET6.

Clear the way by removing the logic in lockd and the NFSv4 callback
server that creates an AF_INET6 service listener.

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

 fs/lockd/svc.c    |   13 +------------
 fs/nfs/callback.c |   14 ++------------
 2 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index d309200..566932b 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -53,17 +53,6 @@ static struct svc_rqst		*nlmsvc_rqst;
 unsigned long			nlmsvc_timeout;
 
 /*
- * If the kernel has IPv6 support available, always listen for
- * both AF_INET and AF_INET6 requests.
- */
-#if (defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)) && \
-	defined(CONFIG_SUNRPC_REGISTER_V4)
-static const sa_family_t	nlmsvc_family = AF_INET6;
-#else	/* (CONFIG_IPV6 || CONFIG_IPV6_MODULE) && CONFIG_SUNRPC_REGISTER_V4 */
-static const sa_family_t	nlmsvc_family = AF_INET;
-#endif	/* (CONFIG_IPV6 || CONFIG_IPV6_MODULE) && CONFIG_SUNRPC_REGISTER_V4 */
-
-/*
  * These can be set at insmod time (useful for NFS as root filesystem),
  * and also changed through the sysctl interface.  -- Jamie Lokier, Aug 2003
  */
@@ -211,7 +200,7 @@ static int create_lockd_listener(struct svc_serv *serv, char *name,
 
 	xprt = svc_find_xprt(serv, name, 0, 0);
 	if (xprt == NULL)
-		return svc_create_xprt(serv, name, nlmsvc_family,
+		return svc_create_xprt(serv, name, PF_INET,
 					port, SVC_SOCK_DEFAULTS);
 
 	svc_xprt_put(xprt);
diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index ddf4b4a..0ef47df 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -41,16 +41,6 @@ unsigned short nfs_callback_tcpport;
 static const int nfs_set_port_min = 0;
 static const int nfs_set_port_max = 65535;
 
-/*
- * If the kernel has IPv6 support available, always listen for
- * both AF_INET and AF_INET6 requests.
- */
-#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
-static const sa_family_t	nfs_callback_family = AF_INET6;
-#else
-static const sa_family_t	nfs_callback_family = AF_INET;
-#endif
-
 static int param_set_port(const char *val, struct kernel_param *kp)
 {
 	char *endp;
@@ -121,13 +111,13 @@ int nfs_callback_up(void)
 	if (!serv)
 		goto out_err;
 
-	ret = svc_create_xprt(serv, "tcp", nfs_callback_family,
+	ret = svc_create_xprt(serv, "tcp", PF_INET,
 				nfs_callback_set_tcpport, SVC_SOCK_ANONYMOUS);
 	if (ret <= 0)
 		goto out_err;
 	nfs_callback_tcpport = ret;
 	dprintk("NFS: Callback listener port = %u (af %u)\n",
-			nfs_callback_tcpport, nfs_callback_family);
+			nfs_callback_tcpport, PF_INET);
 
 	nfs_callback_info.rqst = svc_prepare_thread(serv, &serv->sv_pools[0]);
 	if (IS_ERR(nfs_callback_info.rqst)) {

[PATCH 11/23] SUNRPC: Set IPV6ONLY flag on PF_INET6 RPC listener sockets

[Chuck Lever]

We are about to convert to using separate RPC listener sockets for
PF_INET and PF_INET6.  This echoes the way IPv6 is handled in user
space by TI-RPC, and eliminates the need for ULPs to worry about
mapped IPv4 AF_INET6 addresses when doing address comparisons.

Start by setting the IPV6ONLY flag on PF_INET6 RPC listener sockets.

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

 net/sunrpc/svcsock.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index d00bc33..ac6cd65 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1144,13 +1144,11 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
 		svc_tcp_init(svsk, serv);
 
 	/*
-	 * We start one listener per sv_serv.  We want AF_INET
-	 * requests to be automatically shunted to our PF_INET6
-	 * listener using a mapped IPv4 address.  Make sure
-	 * no-one starts an equivalent IPv4 listener, which
-	 * would steal our incoming connections.
+	 * If this is a PF_INET6 listener, we want to avoid
+	 * getting requests from IPv4 remotes.  Those should
+	 * be shunted to a PF_INET listener via rpcbind.
 	 */
-	val = 0;
+	val = 1;
 	if (inet->sk_family == PF_INET6)
 		kernel_setsockopt(sock, SOL_IPV6, IPV6_V6ONLY,
 					(char *)&val, sizeof(val));

[PATCH 12/23] SUNRPC: Use IPv4 loopback for registering AF_INET6 kernel RPC services

[Chuck Lever]

The kernel uses an IPv6 loopback address when registering its AF_INET6
RPC services so that it can tell whether the local portmapper is
actually IPv6-enabled.

Since the legacy portmapper doesn't listen on IPv6, however, this
causes a long timeout on older systems if the kernel happens to try
creating and registering an AF_INET6 RPC service.  Originally I wanted
to use a connected transport (either TCP or connected UDP) so that the
upcall would fail immediately if the portmapper wasn't listening on
IPv6, but we never agreed on what transport to use.

In the end, it's of little consequence to the kernel whether the local
portmapper is listening on IPv6.  It's only important whether the
portmapper supports rpcbind v4.  And the kernel can't tell that at all
if it is sending requests via IPv6 -- the portmapper will just ignore
them.

So, send both rpcbind v2 and v4 SET/UNSET requests via IPv4 loopback
to maintain better backwards compatibility between new kernels and
legacy user space, and prevent multi-second hangs in some cases when
the kernel attempts to register RPC services.

This patch is part of a series that addresses

   http://bugzilla.kernel.org/show_bug.cgi?id=12256

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

 net/sunrpc/rpcb_clnt.c |   23 ++++++-----------------
 1 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 2caa7ed..ebce7a5 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -124,12 +124,6 @@ static const struct sockaddr_in rpcb_inaddr_loopback = {
 	.sin_port		= htons(RPCBIND_PORT),
 };
 
-static const struct sockaddr_in6 rpcb_in6addr_loopback = {
-	.sin6_family		= AF_INET6,
-	.sin6_addr		= IN6ADDR_LOOPBACK_INIT,
-	.sin6_port		= htons(RPCBIND_PORT),
-};
-
 static struct rpc_clnt *rpcb_create_local(struct sockaddr *addr,
 					  size_t addrlen, u32 version)
 {
@@ -176,9 +170,10 @@ static struct rpc_clnt *rpcb_create(char *hostname, struct sockaddr *srvaddr,
 	return rpc_create(&args);
 }
 
-static int rpcb_register_call(struct sockaddr *addr, size_t addrlen,
-			      u32 version, struct rpc_message *msg)
+static int rpcb_register_call(u32 version, struct rpc_message *msg)
 {
+	struct sockaddr *addr = (struct sockaddr *)&rpcb_inaddr_loopback;
+	size_t addrlen = sizeof(rpcb_inaddr_loopback);
 	struct rpc_clnt *rpcb_clnt;
 	int result, error = 0;
 
@@ -254,9 +249,7 @@ int rpcb_register(u32 prog, u32 vers, int prot, unsigned short port)
 	if (port)
 		msg.rpc_proc = &rpcb_procedures2[RPCBPROC_SET];
 
-	return rpcb_register_call((struct sockaddr *)&rpcb_inaddr_loopback,
-					sizeof(rpcb_inaddr_loopback),
-					RPCBVERS_2, &msg);
+	return rpcb_register_call(RPCBVERS_2, &msg);
 }
 
 /*
@@ -284,9 +277,7 @@ static int rpcb_register_netid4(struct sockaddr_in *address_to_register,
 	if (port)
 		msg->rpc_proc = &rpcb_procedures4[RPCBPROC_SET];
 
-	return rpcb_register_call((struct sockaddr *)&rpcb_inaddr_loopback,
-					sizeof(rpcb_inaddr_loopback),
-					RPCBVERS_4, msg);
+	return rpcb_register_call(RPCBVERS_4, msg);
 }
 
 /*
@@ -318,9 +309,7 @@ static int rpcb_register_netid6(struct sockaddr_in6 *address_to_register,
 	if (port)
 		msg->rpc_proc = &rpcb_procedures4[RPCBPROC_SET];
 
-	return rpcb_register_call((struct sockaddr *)&rpcb_in6addr_loopback,
-					sizeof(rpcb_in6addr_loopback),
-					RPCBVERS_4, msg);
+	return rpcb_register_call(RPCBVERS_4, msg);
 }
 
 /**

[PATCH 13/23] SUNRPC: Don't return EPROTONOSUPPORT in svc_register()'s helpers

[Chuck Lever]

The RPC client returns -EPROTONOSUPPORT if there is a protocol version
mismatch (ie the remote RPC server doesn't support the RPC protocol
version sent by the client).

Helpers for the svc_register() function return -EPROTONOSUPPORT if they
don't recognize the passed-in IPPROTO_ value.

These are two entirely different failure modes.

Have the helpers return -ENOPROTOOPT instead of -EPROTONOSUPPORT.  This
will allow callers to determine more precisely what the underlying
problem is, and decide to report or recover appropriately.

This patch is part of a series that addresses
   http://bugzilla.kernel.org/show_bug.cgi?id=12256

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

 net/sunrpc/svc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index d72ff44..17e0d72 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -749,7 +749,7 @@ static int __svc_rpcb_register4(const u32 program, const u32 version,
 		netid = RPCBIND_NETID_TCP;
 		break;
 	default:
-		return -EPROTONOSUPPORT;
+		return -ENOPROTOOPT;
 	}
 
 	return rpcb_v4_register(program, version,
@@ -785,7 +785,7 @@ static int __svc_rpcb_register6(const u32 program, const u32 version,
 		netid = RPCBIND_NETID_TCP6;
 		break;
 	default:
-		return -EPROTONOSUPPORT;
+		return -ENOPROTOOPT;
 	}
 
 	return rpcb_v4_register(program, version,

[PATCH 14/23] SUNRPC: Clean up address type casts in rpcb_v4_register()

[Chuck Lever]

Clean up: Simplify rpcb_v4_register() and its helpers by moving the
details of sockaddr type casting to rpcb_v4_register()'s helper
functions.

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

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

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index ebce7a5..44d0732 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -170,7 +170,7 @@ static struct rpc_clnt *rpcb_create(char *hostname, struct sockaddr *srvaddr,
 	return rpc_create(&args);
 }
 
-static int rpcb_register_call(u32 version, struct rpc_message *msg)
+static int rpcb_register_call(const u32 version, struct rpc_message *msg)
 {
 	struct sockaddr *addr = (struct sockaddr *)&rpcb_inaddr_loopback;
 	size_t addrlen = sizeof(rpcb_inaddr_loopback);
@@ -255,17 +255,17 @@ int rpcb_register(u32 prog, u32 vers, int prot, unsigned short port)
 /*
  * Fill in AF_INET family-specific arguments to register
  */
-static int rpcb_register_netid4(struct sockaddr_in *address_to_register,
+static int rpcb_register_netid4(const struct sockaddr *sap,
 				struct rpc_message *msg)
 {
+	const struct sockaddr_in *sin = (const struct sockaddr_in *)sap;
 	struct rpcbind_args *map = msg->rpc_argp;
-	unsigned short port = ntohs(address_to_register->sin_port);
+	unsigned short port = ntohs(sin->sin_port);
 	char buf[32];
 
 	/* Construct AF_INET universal address */
 	snprintf(buf, sizeof(buf), "%pI4.%u.%u",
-		 &address_to_register->sin_addr.s_addr,
-		 port >> 8, port & 0xff);
+		 &sin->sin_addr.s_addr, port >> 8, port & 0xff);
 	map->r_addr = buf;
 
 	dprintk("RPC:       %sregistering [%u, %u, %s, '%s'] with "
@@ -283,21 +283,21 @@ static int rpcb_register_netid4(struct sockaddr_in *address_to_register,
 /*
  * Fill in AF_INET6 family-specific arguments to register
  */
-static int rpcb_register_netid6(struct sockaddr_in6 *address_to_register,
+static int rpcb_register_netid6(const struct sockaddr *sap,
 				struct rpc_message *msg)
 {
+	const struct sockaddr_in6 *sin6 = (const struct sockaddr_in6 *)sap;
 	struct rpcbind_args *map = msg->rpc_argp;
-	unsigned short port = ntohs(address_to_register->sin6_port);
+	unsigned short port = ntohs(sin6->sin6_port);
 	char buf[64];
 
 	/* Construct AF_INET6 universal address */
-	if (ipv6_addr_any(&address_to_register->sin6_addr))
+	if (ipv6_addr_any(&sin6->sin6_addr))
 		snprintf(buf, sizeof(buf), "::.%u.%u",
 				port >> 8, port & 0xff);
 	else
 		snprintf(buf, sizeof(buf), "%pI6.%u.%u",
-			 &address_to_register->sin6_addr,
-			 port >> 8, port & 0xff);
+			 &sin6->sin6_addr, port >> 8, port & 0xff);
 	map->r_addr = buf;
 
 	dprintk("RPC:       %sregistering [%u, %u, %s, '%s'] with "
@@ -369,11 +369,9 @@ int rpcb_v4_register(const u32 program, const u32 version,
 
 	switch (address->sa_family) {
 	case AF_INET:
-		return rpcb_register_netid4((struct sockaddr_in *)address,
-					    &msg);
+		return rpcb_register_netid4(address, &msg);
 	case AF_INET6:
-		return rpcb_register_netid6((struct sockaddr_in6 *)address,
-					    &msg);
+		return rpcb_register_netid6(address, &msg);
 	}
 
 	return -EAFNOSUPPORT;

[PATCH 15/23] SUNRPC: rpcbind actually interprets r_owner string

[Chuck Lever]

RFC 1833 has little to say about the contents of r_owner; it only
specifies that it is a string, and states that it is used to control
who can UNSET an entry.

Our port of rpcbind (from Sun) assumes this string contains a numeric
UID value, not alphabetical or symbolic characters, but checks this
value only for AF_LOCAL RPCB_SET or RPCB_UNSET requests.  In all other
cases, rpcbind ignores the contents of the r_owner string.

The reference user space implementation of rpcb_set(3) uses a numeric
UID for all SET/UNSET requests (even via the network) and an empty
string for all other requests.  We emulate that behavior here to
maintain bug-for-bug compatibility.

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

 net/sunrpc/rpcb_clnt.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 44d0732..d550d0b 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -63,9 +63,16 @@ enum {
  * r_owner
  *
  * The "owner" is allowed to unset a service in the rpcbind database.
- * We always use the following (arbitrary) fixed string.
+ *
+ * For AF_LOCAL SET/UNSET requests, rpcbind treats this string as a
+ * UID which it maps to a local user name via a password lookup.
+ * In all other cases it is ignored.
+ *
+ * For SET/UNSET requests, user space provides a value, even for
+ * network requests, and GETADDR uses an empty string.  We follow
+ * those precedents here.
  */
-#define RPCB_OWNER_STRING	"rpcb"
+#define RPCB_OWNER_STRING	"0"
 #define RPCB_MAXOWNERLEN	sizeof(RPCB_OWNER_STRING)
 
 static void			rpcb_getport_done(struct rpc_task *, void *);
@@ -566,7 +573,7 @@ void rpcb_getport_async(struct rpc_task *task)
 	map->r_xprt = xprt_get(xprt);
 	map->r_netid = rpc_peeraddr2str(clnt, RPC_DISPLAY_NETID);
 	map->r_addr = rpc_peeraddr2str(rpcb_clnt, RPC_DISPLAY_UNIVERSAL_ADDR);
-	map->r_owner = RPCB_OWNER_STRING;	/* ignored for GETADDR */
+	map->r_owner = "";
 	map->r_status = -EIO;
 
 	child = rpcb_call_async(rpcb_clnt, map, proc);

[PATCH 16/23] SUNRPC: Allow callers to pass rpcb_v4_register a NULL address

[Chuck Lever]

The user space TI-RPC library uses an empty string for the universal
address when unregistering all target addresses for [program, version].
The kernel's rpcb client should behave the same way.

Here, we are switching between several registration methods based on
the protocol family of the incoming address.  Rename the other rpcbind
v4 registration functions to make it clear that they, as well, are
switched on protocol family.  In /etc/netconfig, this is either "inet"
or "inet6".

NB: The loopback protocol families are not supported in the kernel.

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

 net/sunrpc/rpcb_clnt.c |   38 ++++++++++++++++++++++++++++----------
 1 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index d550d0b..8ea8907 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -262,8 +262,8 @@ int rpcb_register(u32 prog, u32 vers, int prot, unsigned short port)
 /*
  * Fill in AF_INET family-specific arguments to register
  */
-static int rpcb_register_netid4(const struct sockaddr *sap,
-				struct rpc_message *msg)
+static int rpcb_register_inet4(const struct sockaddr *sap,
+			       struct rpc_message *msg)
 {
 	const struct sockaddr_in *sin = (const struct sockaddr_in *)sap;
 	struct rpcbind_args *map = msg->rpc_argp;
@@ -290,8 +290,8 @@ static int rpcb_register_netid4(const struct sockaddr *sap,
 /*
  * Fill in AF_INET6 family-specific arguments to register
  */
-static int rpcb_register_netid6(const struct sockaddr *sap,
-				struct rpc_message *msg)
+static int rpcb_register_inet6(const struct sockaddr *sap,
+			       struct rpc_message *msg)
 {
 	const struct sockaddr_in6 *sin6 = (const struct sockaddr_in6 *)sap;
 	struct rpcbind_args *map = msg->rpc_argp;
@@ -319,6 +319,20 @@ static int rpcb_register_netid6(const struct sockaddr *sap,
 	return rpcb_register_call(RPCBVERS_4, msg);
 }
 
+static int rpcb_unregister_all_protofamilies(struct rpc_message *msg)
+{
+	struct rpcbind_args *map = msg->rpc_argp;
+
+	dprintk("RPC:       unregistering [%u, %u, '%s'] with "
+		"local rpcbind\n",
+			map->r_prog, map->r_vers, map->r_netid);
+
+	map->r_addr = "";
+	msg->rpc_proc = &rpcb_procedures4[RPCBPROC_UNSET];
+
+	return rpcb_register_call(RPCBVERS_4, msg);
+}
+
 /**
  * rpcb_v4_register - set or unset a port registration with the local rpcbind
  * @program: RPC program number of service to (un)register
@@ -336,10 +350,11 @@ static int rpcb_register_netid6(const struct sockaddr *sap,
  * invoke this function once for each [program, version, address,
  * netid] tuple they wish to advertise.
  *
- * Callers may also unregister RPC services that are no longer
- * available by setting the port number in the passed-in address
- * to zero.  Callers pass a netid of "" to unregister all
- * transport netids associated with [program, version, address].
+ * Callers may also unregister RPC services that are registered at a
+ * specific address by setting the port number in @address to zero.
+ * They may unregister all registered protocol families at once for
+ * a service by passing a NULL @address argument.  If @netid is ""
+ * then all netids for [program, version, address] are unregistered.
  *
  * This function uses rpcbind protocol version 4 to contact the
  * local rpcbind daemon.  The local rpcbind daemon must support
@@ -374,11 +389,14 @@ int rpcb_v4_register(const u32 program, const u32 version,
 		.rpc_argp	= &map,
 	};
 
+	if (address == NULL)
+		return rpcb_unregister_all_protofamilies(&msg);
+
 	switch (address->sa_family) {
 	case AF_INET:
-		return rpcb_register_netid4(address, &msg);
+		return rpcb_register_inet4(address, &msg);
 	case AF_INET6:
-		return rpcb_register_netid6(address, &msg);
+		return rpcb_register_inet6(address, &msg);
 	}
 
 	return -EAFNOSUPPORT;

[PATCH 17/23] SUNRPC: Simplify svc_unregister()

[Chuck Lever]

Our initial implementation of svc_unregister() assumed that PMAP_UNSET
cleared all rpcbind registrations for a [program, version] tuple.
However, we now have evidence that PMAP_UNSET clears only "inet"
entries, and not "inet6" entries, in the rpcbind database.

For backwards compatibility with the legacy portmapper, the
svc_unregister() function also must work if user space doesn't support
rpcbind version 4 at all.

Thus we'll send an rpcbind v4 UNSET, and if that fails, we'll send a
PMAP_UNSET.

This simplifies the code in svc_unregister() and provides better
backwards compatibility with legacy user space that does not support
rpcbind version 4.  We can get rid of the conditional compilation in
here as well.

This patch is part of a series that addresses
   http://bugzilla.kernel.org/show_bug.cgi?id=12256

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

 net/sunrpc/svc.c |   35 ++++++++++++++---------------------
 1 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 17e0d72..bd0ee31 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -896,38 +896,31 @@ int svc_register(const struct svc_serv *serv, const int family,
 	return error;
 }
 
-#ifdef CONFIG_SUNRPC_REGISTER_V4
-
+/*
+ * If user space is running rpcbind, it should take the v4 UNSET
+ * and clear everything for this [program, version].  If user space
+ * is running portmap, it will reject the v4 UNSET, but won't have
+ * any "inet6" entries anyway.  So a PMAP_UNSET should be sufficient
+ * in this case to clear all existing entries for [program, version].
+ */
 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 */
+	error = rpcb_v4_register(program, version, NULL, "");
 
-static void __svc_unregister(const u32 program, const u32 version,
-			     const char *progname)
-{
-	int error;
+	/*
+	 * User space didn't support rpcbind v4, so retry this
+	 * request with the legacy rpcbind v2 protocol.
+	 */
+	if (error == -EPROTONOSUPPORT)
+		error = rpcb_register(program, version, 0, 0);
 
-	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 netids, bind addresses and ports registered for [program, version]
  * are removed from the local rpcbind database (if the service is not

[PATCH 18/23] SUNRPC: Simplify kernel RPC service registration

[Chuck Lever]

The kernel registers RPC services with the local portmapper with an
rpcbind SET upcall to the local portmapper.  Traditionally, this used
rpcbind v2 (PMAP), but registering RPC services that support IPv6
requires rpcbind v3 or v4.

Since we now want separate PF_INET and PF_INET6 listeners for each
kernel RPC service, svc_register() will do only one of those
registrations at a time.

For PF_INET, it tries an rpcb v4 SET upcall first; if that fails, it
does a legacy portmap SET.  This makes it entirely backwards
compatible with legacy user space, but allows a proper v4 SET to be
used if rpcbind is available.

For PF_INET6, it does an rpcb v4 SET upcall.  If that fails, it fails
the registration, and thus the transport creation.  This let's the
kernel detect if user space is able to support IPv6 RPC services, and
thus whether it should maintain a PF_INET6 listener for each service
at all.

This provides complete backwards compatibilty with legacy user space
that only supports rpcbind v2.  The only down-side is that registering
a new kernel RPC service may take an extra exchange with the local
portmapper on legacy systems, but this is an infrequent operation and
is done over UDP (no lingering sockets in TIMEWAIT), so it shouldn't
be consequential.

This patch is part of a series that addresses
   http://bugzilla.kernel.org/show_bug.cgi?id=12256

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

 net/sunrpc/svc.c |   79 +++++++++++++++++++++++-------------------------------
 1 files changed, 34 insertions(+), 45 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index bd0ee31..142f647 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -718,8 +718,6 @@ svc_exit_thread(struct svc_rqst *rqstp)
 }
 EXPORT_SYMBOL_GPL(svc_exit_thread);
 
-#ifdef CONFIG_SUNRPC_REGISTER_V4
-
 /*
  * Register an "inet" protocol family netid with the local
  * rpcbind daemon via an rpcbind v4 SET request.
@@ -734,12 +732,13 @@ static int __svc_rpcb_register4(const u32 program, const u32 version,
 				const unsigned short protocol,
 				const unsigned short port)
 {
-	struct sockaddr_in sin = {
+	const struct sockaddr_in sin = {
 		.sin_family		= AF_INET,
 		.sin_addr.s_addr	= htonl(INADDR_ANY),
 		.sin_port		= htons(port),
 	};
-	char *netid;
+	const char *netid;
+	int error;
 
 	switch (protocol) {
 	case IPPROTO_UDP:
@@ -752,10 +751,20 @@ static int __svc_rpcb_register4(const u32 program, const u32 version,
 		return -ENOPROTOOPT;
 	}
 
-	return rpcb_v4_register(program, version,
-				(struct sockaddr *)&sin, netid);
+	error = rpcb_v4_register(program, version,
+					(const struct sockaddr *)&sin, netid);
+
+	/*
+	 * User space didn't support rpcbind v4, so retry this
+	 * registration request with the legacy rpcbind v2 protocol.
+	 */
+	if (error == -EPROTONOSUPPORT)
+		error = rpcb_register(program, version, protocol, port);
+
+	return error;
 }
 
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 /*
  * Register an "inet6" protocol family netid with the local
  * rpcbind daemon via an rpcbind v4 SET request.
@@ -770,12 +779,13 @@ static int __svc_rpcb_register6(const u32 program, const u32 version,
 				const unsigned short protocol,
 				const unsigned short port)
 {
-	struct sockaddr_in6 sin6 = {
+	const struct sockaddr_in6 sin6 = {
 		.sin6_family		= AF_INET6,
 		.sin6_addr		= IN6ADDR_ANY_INIT,
 		.sin6_port		= htons(port),
 	};
-	char *netid;
+	const char *netid;
+	int error;
 
 	switch (protocol) {
 	case IPPROTO_UDP:
@@ -788,9 +798,19 @@ static int __svc_rpcb_register6(const u32 program, const u32 version,
 		return -ENOPROTOOPT;
 	}
 
-	return rpcb_v4_register(program, version,
-				(struct sockaddr *)&sin6, netid);
+	error = rpcb_v4_register(program, version,
+					(const struct sockaddr *)&sin6, netid);
+
+	/*
+	 * User space didn't support rpcbind version 4, so we won't
+	 * use a PF_INET6 listener.
+	 */
+	if (error == -EPROTONOSUPPORT)
+		error = -EAFNOSUPPORT;
+
+	return error;
 }
+#endif	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
 
 /*
  * Register a kernel RPC service via rpcbind version 4.
@@ -809,48 +829,17 @@ static int __svc_register(const u32 program, const u32 version,
 	case PF_INET:
 		return __svc_rpcb_register4(program, version,
 						protocol, port);
+		break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case PF_INET6:
-		error = __svc_rpcb_register6(program, version,
+		return__svc_rpcb_register6(program, version,
 						protocol, port);
-		if (error < 0)
-			return error;
-
-		/*
-		 * Work around bug in some versions of Linux rpcbind
-		 * which don't allow registration of both inet and
-		 * inet6 netids.
-		 *
-		 * Error return ignored for now.
-		 */
-		__svc_rpcb_register4(program, version,
-						protocol, port);
-		return 0;
+#endif	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
 	}
 
 	return -EAFNOSUPPORT;
 }
 
-#else	/* CONFIG_SUNRPC_REGISTER_V4 */
-
-/*
- * Register a kernel RPC service via rpcbind version 2.
- *
- * Returns zero on success; a negative errno value is returned
- * if any error occurs.
- */
-static int __svc_register(const u32 program, const u32 version,
-			  const int family,
-			  const unsigned short protocol,
-			  const unsigned short port)
-{
-	if (family != PF_INET)
-		return -EAFNOSUPPORT;
-
-	return rpcb_register(program, version, protocol, port);
-}
-
-#endif /* CONFIG_SUNRPC_REGISTER_V4 */
-
 /**
  * svc_register - register an RPC service with the local portmapper
  * @serv: svc_serv struct for the service to register

[PATCH 19/23] SUNRPC: rpcb_register() should handle errors silently

[Chuck Lever]

Move error reporting for RPC registration to rpcb_register's caller.

This way the caller can choose to recover silently from certain
errors, but report errors it does not recognize.  Error reporting
for kernel RPC service registration is now handled in one place.

This patch is part of a series that addresses
   http://bugzilla.kernel.org/show_bug.cgi?id=12256

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

 net/sunrpc/rpcb_clnt.c |    2 +-
 net/sunrpc/svc.c       |   18 +++++++++++-------
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 8ea8907..beee6da 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -194,7 +194,7 @@ static int rpcb_register_call(const u32 version, struct rpc_message *msg)
 		error = PTR_ERR(rpcb_clnt);
 
 	if (error < 0) {
-		printk(KERN_WARNING "RPC: failed to contact local rpcbind "
+		dprintk("RPC:       failed to contact local rpcbind "
 				"server (errno %d).\n", -error);
 		return error;
 	}
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 142f647..8ba654b 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -818,26 +818,30 @@ static int __svc_rpcb_register6(const u32 program, const u32 version,
  * Returns zero on success; a negative errno value is returned
  * if any error occurs.
  */
-static int __svc_register(const u32 program, const u32 version,
+static int __svc_register(const char *progname,
+			  const u32 program, const u32 version,
 			  const int family,
 			  const unsigned short protocol,
 			  const unsigned short port)
 {
-	int error;
+	int error = -EAFNOSUPPORT;
 
 	switch (family) {
 	case PF_INET:
-		return __svc_rpcb_register4(program, version,
+		error = __svc_rpcb_register4(program, version,
 						protocol, port);
 		break;
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case PF_INET6:
-		return__svc_rpcb_register6(program, version,
+		error = __svc_rpcb_register6(program, version,
 						protocol, port);
 #endif	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
 	}
 
-	return -EAFNOSUPPORT;
+	if (error < 0)
+		printk(KERN_WARNING "svc: failed to register %sv%u RPC "
+			"service (errno %d).\n", progname, version, -error);
+	return error;
 }
 
 /**
@@ -875,8 +879,8 @@ int svc_register(const struct svc_serv *serv, const int family,
 			if (progp->pg_vers[i]->vs_hidden)
 				continue;
 
-			error = __svc_register(progp->pg_prog, i,
-						family, proto, port);
+			error = __svc_register(progp->pg_name, progp->pg_prog,
+						i, family, proto, port);
 			if (error < 0)
 				break;
 		}

[PATCH 20/23] SUNRPC: Remove CONFIG_SUNRPC_REGISTER_V4

[Chuck Lever]

We just augmented the kernel's RPC service registration code so that
it automatically adjusts to what is supported in user space.  Thus we
no longer need the kernel configuration option to enable registering
RPC services with v4 -- it's all done automatically.

This patch is part of a series that addresses
   http://bugzilla.kernel.org/show_bug.cgi?id=12256

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

 net/sunrpc/Kconfig |   22 ----------------------
 1 files changed, 0 insertions(+), 22 deletions(-)

diff --git a/net/sunrpc/Kconfig b/net/sunrpc/Kconfig
index 5592883..afd91c7 100644
--- a/net/sunrpc/Kconfig
+++ b/net/sunrpc/Kconfig
@@ -17,28 +17,6 @@ config SUNRPC_XPRT_RDMA
 
 	  If unsure, say N.
 
-config SUNRPC_REGISTER_V4
-	bool "Register local RPC services via rpcbind v4 (EXPERIMENTAL)"
-	depends on SUNRPC && EXPERIMENTAL
-	default n
-	help
-	  Sun added support for registering RPC services at an IPv6
-	  address by creating two new versions of the rpcbind protocol
-	  (RFC 1833).
-
-	  This option enables support in the kernel RPC server for
-	  registering kernel RPC services via version 4 of the rpcbind
-	  protocol.  If you enable this option, you must run a portmapper
-	  daemon that supports rpcbind protocol version 4.
-
-	  Serving NFS over IPv6 from knfsd (the kernel's NFS server)
-	  requires that you enable this option and use a portmapper that
-	  supports rpcbind version 4.
-
-	  If unsure, say N to get traditional behavior (register kernel
-	  RPC services using only rpcbind version 2).  Distributions
-	  using the legacy Linux portmapper daemon must say N here.
-
 config RPCSEC_GSS_KRB5
 	tristate "Secure RPC: Kerberos V mechanism (EXPERIMENTAL)"
 	depends on SUNRPC && EXPERIMENTAL

[PATCH 21/23] lockd: Start PF_INET6 listener only if IPv6 support is available

[Chuck Lever]

Apparently a lot of people need to disable IPv6 completely on their
distributor-built systems, which have CONFIG_IPV6_MODULE enabled at
build time.

They do this by blacklisting the ipv6.ko module.  This causes the
creation of the lockd service listener to fail if CONFIG_IPV6_MODULE
is set, but the module cannot be loaded.

Now that the kernel's PF_INET6 RPC listeners are completely separate
from PF_INET listeners, we can always start PF_INET.  Then lockd can
try to start PF_INET6, but it isn't required to be available.

Note this has the added benefit that NLM callbacks from AF_INET6
servers will never come from AF_INET remotes.  We no longer have to
worry about matching mapped IPv4 addresses to AF_INET when comparing
addresses.

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

 fs/lockd/clntlock.c |   51 +--------------------------------------------------
 fs/lockd/svc.c      |   30 +++++++++++++++++++++---------
 2 files changed, 22 insertions(+), 59 deletions(-)

diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index aedc47a..1f3b0fc 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -139,55 +139,6 @@ int nlmclnt_block(struct nlm_wait *block, struct nlm_rqst *req, long timeout)
 	return 0;
 }
 
-#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
-static const struct in6_addr *nlmclnt_map_v4addr(const struct sockaddr *sap,
-						 struct in6_addr *addr_mapped)
-{
-	const struct sockaddr_in *sin = (const struct sockaddr_in *)sap;
-
-	switch (sap->sa_family) {
-	case AF_INET6:
-		return &((const struct sockaddr_in6 *)sap)->sin6_addr;
-	case AF_INET:
-		ipv6_addr_set_v4mapped(sin->sin_addr.s_addr, addr_mapped);
-		return addr_mapped;
-	}
-
-	return NULL;
-}
-
-/*
- * If lockd is using a PF_INET6 listener, all incoming requests appear
- * to come from AF_INET6 remotes.  The address of AF_INET remotes are
- * mapped to AF_INET6 automatically by the network layer.  In case the
- * user passed an AF_INET server address at mount time, ensure both
- * addresses are AF_INET6 before comparing them.
- */
-static int nlmclnt_cmp_addr(const struct nlm_host *host,
-			    const struct sockaddr *sap)
-{
-	const struct in6_addr *addr1;
-	const struct in6_addr *addr2;
-	struct in6_addr addr1_mapped;
-	struct in6_addr addr2_mapped;
-
-	addr1 = nlmclnt_map_v4addr(nlm_addr(host), &addr1_mapped);
-	if (likely(addr1 != NULL)) {
-		addr2 = nlmclnt_map_v4addr(sap, &addr2_mapped);
-		if (likely(addr2 != NULL))
-			return ipv6_addr_equal(addr1, addr2);
-	}
-
-	return 0;
-}
-#else	/* !(CONFIG_IPV6 || CONFIG_IPV6_MODULE) */
-static int nlmclnt_cmp_addr(const struct nlm_host *host,
-			    const struct sockaddr *sap)
-{
-	return nlm_cmp_addr(nlm_addr(host), sap);
-}
-#endif	/* !(CONFIG_IPV6 || CONFIG_IPV6_MODULE) */
-
 /*
  * The server lockd has called us back to tell us the lock was granted
  */
@@ -215,7 +166,7 @@ __be32 nlmclnt_grant(const struct sockaddr *addr, const struct nlm_lock *lock)
 		 */
 		if (fl_blocked->fl_u.nfs_fl.owner->pid != lock->svid)
 			continue;
-		if (!nlmclnt_cmp_addr(block->b_host, addr))
+		if (!nlm_cmp_addr(nlm_addr(block->b_host), addr))
 			continue;
 		if (nfs_compare_fh(NFS_FH(fl_blocked->fl_file->f_path.dentry->d_inode) ,fh) != 0)
 			continue;
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 566932b..abf8388 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -193,20 +193,30 @@ lockd(void *vrqstp)
 	return 0;
 }
 
-static int create_lockd_listener(struct svc_serv *serv, char *name,
-				 unsigned short port)
+static int create_lockd_listener(struct svc_serv *serv, const char *name,
+				 const int family, const unsigned short port)
 {
 	struct svc_xprt *xprt;
 
-	xprt = svc_find_xprt(serv, name, 0, 0);
+	xprt = svc_find_xprt(serv, name, family, 0);
 	if (xprt == NULL)
-		return svc_create_xprt(serv, name, PF_INET,
-					port, SVC_SOCK_DEFAULTS);
-
+		return svc_create_xprt(serv, name, family, port,
+						SVC_SOCK_DEFAULTS);
 	svc_xprt_put(xprt);
 	return 0;
 }
 
+static int create_lockd_family(struct svc_serv *serv, const int family)
+{
+	int err;
+
+	err = create_lockd_listener(serv, "udp", family, nlm_udpport);
+	if (err < 0)
+		return err;
+
+	return create_lockd_listener(serv, "tcp", family, nlm_tcpport);
+}
+
 /*
  * Ensure there are active UDP and TCP listeners for lockd.
  *
@@ -222,13 +232,15 @@ static int make_socks(struct svc_serv *serv)
 	static int warned;
 	int err;
 
-	err = create_lockd_listener(serv, "udp", nlm_udpport);
+	err = create_lockd_family(serv, PF_INET);
 	if (err < 0)
 		goto out_err;
 
-	err = create_lockd_listener(serv, "tcp", nlm_tcpport);
-	if (err < 0)
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+	err = create_lockd_family(serv, PF_INET6);
+	if (err < 0 && err != -EAFNOSUPPORT)
 		goto out_err;
+#endif	/* CONFIG_IPV6 || CONFIG_IPV6_MODULE */
 
 	warned = 0;
 	return 0;

[PATCH 22/23] NFS: Start PF_INET6 callback listener only if IPv6 support is available

[Chuck Lever]

Apparently a lot of people need to disable IPv6 completely on their
distributor-built systems, which have CONFIG_IPV6_MODULE enabled at
build time.

They do this by blacklisting the ipv6.ko module.  This causes the
creation of the NFSv4 callback service listener to fail if
CONFIG_IPV6_MODULE is set, but the module cannot be loaded.

Now that the kernel's PF_INET6 RPC listeners are completely separate
from PF_INET listeners, we can always start PF_INET.  Then the NFS
client can try to start a PF_INET6 listener, but it isn't required
to be available.

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

 fs/nfs/callback.c  |   12 ++++++++++++
 fs/nfs/callback.h  |    1 +
 fs/nfs/nfs4state.c |   10 ++++++++--
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 0ef47df..a886e69 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -38,6 +38,7 @@ static struct svc_program nfs4_callback_program;
 
 unsigned int nfs_callback_set_tcpport;
 unsigned short nfs_callback_tcpport;
+unsigned short nfs_callback_tcpport6;
 static const int nfs_set_port_min = 0;
 static const int nfs_set_port_max = 65535;
 
@@ -119,6 +120,17 @@ int nfs_callback_up(void)
 	dprintk("NFS: Callback listener port = %u (af %u)\n",
 			nfs_callback_tcpport, PF_INET);
 
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+	ret = svc_create_xprt(serv, "tcp", PF_INET6,
+				nfs_callback_set_tcpport, SVC_SOCK_ANONYMOUS);
+	if (ret > 0) {
+		nfs_callback_tcpport6 = ret;
+		dprintk("NFS: Callback listener port = %u (af %u)\n",
+				nfs_callback_tcpport6, PF_INET6);
+	} else if (ret != -EAFNOSUPPORT)
+		goto out_err;
+#endif	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
+
 	nfs_callback_info.rqst = svc_prepare_thread(serv, &serv->sv_pools[0]);
 	if (IS_ERR(nfs_callback_info.rqst)) {
 		ret = PTR_ERR(nfs_callback_info.rqst);
diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
index bb25d21..e110e28 100644
--- a/fs/nfs/callback.h
+++ b/fs/nfs/callback.h
@@ -72,5 +72,6 @@ extern void nfs_callback_down(void);
 
 extern unsigned int nfs_callback_set_tcpport;
 extern unsigned short nfs_callback_tcpport;
+extern unsigned short nfs_callback_tcpport6;
 
 #endif /* __LINUX_FS_NFS_CALLBACK_H */
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 2022fe4..0298e90 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -62,8 +62,14 @@ static LIST_HEAD(nfs4_clientid_list);
 
 static int nfs4_init_client(struct nfs_client *clp, struct rpc_cred *cred)
 {
-	int status = nfs4_proc_setclientid(clp, NFS4_CALLBACK,
-			nfs_callback_tcpport, cred);
+	unsigned short port;
+	int status;
+
+	port = nfs_callback_tcpport;
+	if (clp->cl_addr.ss_family == AF_INET6)
+		port = nfs_callback_tcpport6;
+
+	status = nfs4_proc_setclientid(clp, NFS4_CALLBACK, port, cred);
 	if (status == 0)
 		status = nfs4_proc_setclientid_confirm(clp, cred);
 	if (status == 0)

[PATCH 23/23] NFS: Simplify logic to compare socket addresses in client.c

[Chuck Lever]

Callback requests from IPv4 servers are now always guaranteed to be
AF_INET, and never mapped IPv4 AF_INET6 addresses.  Both
nfs_match_client() and nfs_find_client() can now share the same
address comparison logic, so fold them together.

We can also dispense with of most of the conditional compilation
in here.

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

 fs/nfs/client.c |  116 +++++++++++++++++++++++++------------------------------
 1 files changed, 52 insertions(+), 64 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 574158a..855daac 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -224,38 +224,6 @@ void nfs_put_client(struct nfs_client *clp)
 }
 
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
-static const struct in6_addr *nfs_map_ipv4_addr(const struct sockaddr *sa, struct in6_addr *addr_mapped)
-{
-	switch (sa->sa_family) {
-		default:
-			return NULL;
-		case AF_INET6:
-			return &((const struct sockaddr_in6 *)sa)->sin6_addr;
-			break;
-		case AF_INET:
-			ipv6_addr_set_v4mapped(((const struct sockaddr_in *)sa)->sin_addr.s_addr,
-					addr_mapped);
-			return addr_mapped;
-	}
-}
-
-static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
-		const struct sockaddr *sa2)
-{
-	const struct in6_addr *addr1;
-	const struct in6_addr *addr2;
-	struct in6_addr addr1_mapped;
-	struct in6_addr addr2_mapped;
-
-	addr1 = nfs_map_ipv4_addr(sa1, &addr1_mapped);
-	if (likely(addr1 != NULL)) {
-		addr2 = nfs_map_ipv4_addr(sa2, &addr2_mapped);
-		if (likely(addr2 != NULL))
-			return ipv6_addr_equal(addr1, addr2);
-	}
-	return 0;
-}
-
 /*
  * Test if two ip6 socket addresses refer to the same socket by
  * comparing relevant fields. The padding bytes specifically, are not
@@ -267,38 +235,21 @@ static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
  *
  * The caller should ensure both socket addresses are AF_INET6.
  */
-static int nfs_sockaddr_cmp_ip6(const struct sockaddr *sa1,
-				const struct sockaddr *sa2)
+static int nfs_sockaddr_match_ipaddr6(const struct sockaddr *sa1,
+				      const struct sockaddr *sa2)
 {
-	const struct sockaddr_in6 *saddr1 = (const struct sockaddr_in6 *)sa1;
-	const struct sockaddr_in6 *saddr2 = (const struct sockaddr_in6 *)sa2;
+	const struct sockaddr_in6 *sin1 = (const struct sockaddr_in6 *)sa1;
+	const struct sockaddr_in6 *sin2 = (const struct sockaddr_in6 *)sa2;
 
-	if (!ipv6_addr_equal(&saddr1->sin6_addr,
-			     &saddr1->sin6_addr))
+	if (ipv6_addr_scope(&sin1->sin6_addr) == IPV6_ADDR_SCOPE_LINKLOCAL &&
+	    sin1->sin6_scope_id != sin2->sin6_scope_id)
 		return 0;
-	if (ipv6_addr_scope(&saddr1->sin6_addr) == IPV6_ADDR_SCOPE_LINKLOCAL &&
-	    saddr1->sin6_scope_id != saddr2->sin6_scope_id)
-		return 0;
-	return saddr1->sin6_port == saddr2->sin6_port;
-}
-#else
-static int nfs_sockaddr_match_ipaddr4(const struct sockaddr_in *sa1,
-				 const struct sockaddr_in *sa2)
-{
-	return sa1->sin_addr.s_addr == sa2->sin_addr.s_addr;
-}
 
-static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
-				 const struct sockaddr *sa2)
-{
-	if (unlikely(sa1->sa_family != AF_INET || sa2->sa_family != AF_INET))
-		return 0;
-	return nfs_sockaddr_match_ipaddr4((const struct sockaddr_in *)sa1,
-			(const struct sockaddr_in *)sa2);
+	return ipv6_addr_equal(&sin1->sin6_addr, &sin1->sin6_addr);
 }
-
-static int nfs_sockaddr_cmp_ip6(const struct sockaddr * sa1,
-				const struct sockaddr * sa2)
+#else	/* !defined(CONFIG_IPV6) && !defined(CONFIG_IPV6_MODULE) */
+static int nfs_sockaddr_match_ipaddr6(const struct sockaddr *sa1,
+				      const struct sockaddr *sa2)
 {
 	return 0;
 }
@@ -311,20 +262,57 @@ static int nfs_sockaddr_cmp_ip6(const struct sockaddr * sa1,
  *
  * The caller should ensure both socket addresses are AF_INET.
  */
+static int nfs_sockaddr_match_ipaddr4(const struct sockaddr *sa1,
+				      const struct sockaddr *sa2)
+{
+	const struct sockaddr_in *sin1 = (const struct sockaddr_in *)sa1;
+	const struct sockaddr_in *sin2 = (const struct sockaddr_in *)sa2;
+
+	return sin1->sin_addr.s_addr == sin2->sin_addr.s_addr;
+}
+
+static int nfs_sockaddr_cmp_ip6(const struct sockaddr *sa1,
+				const struct sockaddr *sa2)
+{
+	const struct sockaddr_in6 *sin1 = (const struct sockaddr_in6 *)sa1;
+	const struct sockaddr_in6 *sin2 = (const struct sockaddr_in6 *)sa2;
+
+	return nfs_sockaddr_match_ipaddr6(sa1, sa2) &&
+		(sin1->sin6_port == sin2->sin6_port);
+}
+
 static int nfs_sockaddr_cmp_ip4(const struct sockaddr *sa1,
 				const struct sockaddr *sa2)
 {
-	const struct sockaddr_in *saddr1 = (const struct sockaddr_in *)sa1;
-	const struct sockaddr_in *saddr2 = (const struct sockaddr_in *)sa2;
+	const struct sockaddr_in *sin1 = (const struct sockaddr_in *)sa1;
+	const struct sockaddr_in *sin2 = (const struct sockaddr_in *)sa2;
 
-	if (saddr1->sin_addr.s_addr != saddr2->sin_addr.s_addr)
+	return nfs_sockaddr_match_ipaddr4(sa1, sa2) &&
+		(sin1->sin_port == sin2->sin_port);
+}
+
+/*
+ * Test if two socket addresses represent the same actual socket,
+ * by comparing (only) relevant fields, excluding the port number.
+ */
+static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
+				     const struct sockaddr *sa2)
+{
+	if (sa1->sa_family != sa2->sa_family)
 		return 0;
-	return saddr1->sin_port == saddr2->sin_port;
+
+	switch (sa1->sa_family) {
+	case AF_INET:
+		return nfs_sockaddr_match_ipaddr4(sa1, sa2);
+	case AF_INET6:
+		return nfs_sockaddr_match_ipaddr6(sa1, sa2);
+	}
+	return 0;
 }
 
 /*
  * Test if two socket addresses represent the same actual socket,
- * by comparing (only) relevant fields.
+ * by comparing (only) relevant fields, including the port number.
  */
 static int nfs_sockaddr_cmp(const struct sockaddr *sa1,
 			    const struct sockaddr *sa2)

Re: [PATCH 02/23] SUNRPC: Fix return type of svc_addr_len()

[J. Bruce Fields]

On Wed, Mar 18, 2009 at 08:45:36PM -0400, Chuck Lever wrote:
> The svc_addr_len() helper function can return a negative errno value,
> but its return type is size_t, which is unsigned.
> 
> The RDMA transport code passes this return value directly to memset(),
> without checking first if it's negative.  This could cause memset() to
> clobber a large piece of memory if svc_addr_len() has returned an
> error.

I'd still like to understand when this can happen, to better understand
how the error should be handled.

Also:

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  include/linux/sunrpc/svc_xprt.h          |    3 ++-
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |   16 ++++++++++++++--
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> index 0127dac..c2aa8cd 100644
> --- a/include/linux/sunrpc/svc_xprt.h
> +++ b/include/linux/sunrpc/svc_xprt.h
> @@ -113,7 +113,7 @@ static inline unsigned short svc_addr_port(struct sockaddr *sa)
>  	return ret;
>  }
>  
> -static inline size_t svc_addr_len(struct sockaddr *sa)
> +static inline int svc_addr_len(const struct sockaddr *sa)
>  {
>  	switch (sa->sa_family) {
>  	case AF_INET:
> @@ -121,6 +121,7 @@ static inline size_t svc_addr_len(struct sockaddr *sa)
>  	case AF_INET6:
>  		return sizeof(struct sockaddr_in6);
>  	}
> +
>  	return -EAFNOSUPPORT;
>  }
>  
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 3d810e7..d1ec6f9 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -546,6 +546,7 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
>  	struct svcxprt_rdma *listen_xprt = new_cma_id->context;
>  	struct svcxprt_rdma *newxprt;
>  	struct sockaddr *sa;
> +	int len;
>  
>  	/* Create a new transport */
>  	newxprt = rdma_create_xprt(listen_xprt->sc_xprt.xpt_server, 0);
> @@ -563,9 +564,20 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
>  
>  	/* Set the local and remote addresses in the transport */
>  	sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
> -	svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
> +	len = svc_addr_len(sa);
> +	if (len < 0) {
> +		dprintk("svcrdma: dst_addr has a bad address family\n");
> +		return;

we're probably leaking something here.

I don't want to fix this until it's understood well enough to fix it
correctly.

--b.

> +	}
> +	svc_xprt_set_remote(&newxprt->sc_xprt, sa, len);
> +
>  	sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
> -	svc_xprt_set_local(&newxprt->sc_xprt, sa, svc_addr_len(sa));
> +	len = svc_addr_len(sa);
> +	if (len < 0) {
> +		dprintk("svcrdma: src_addr has a bad address family\n");
> +		return;
> +	}
> +	svc_xprt_set_local(&newxprt->sc_xprt, sa, len);
>  
>  	/*
>  	 * Enqueue the new transport on the accept queue of the listening
> 

Re: [PATCH 02/23] SUNRPC: Fix return type of svc_addr_len()

[Chuck Lever]

On Mar 24, 2009, at 4:32 PM, J. Bruce Fields wrote:
> On Wed, Mar 18, 2009 at 08:45:36PM -0400, Chuck Lever wrote:
>> The svc_addr_len() helper function can return a negative errno value,
>> but its return type is size_t, which is unsigned.
>>
>> The RDMA transport code passes this return value directly to  
>> memset(),
>> without checking first if it's negative.  This could cause memset()  
>> to
>> clobber a large piece of memory if svc_addr_len() has returned an
>> error.
>
> I'd still like to understand when this can happen, to better  
> understand
> how the error should be handled.
>
> Also:
>
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> include/linux/sunrpc/svc_xprt.h          |    3 ++-
>> net/sunrpc/xprtrdma/svc_rdma_transport.c |   16 ++++++++++++++--
>> 2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/ 
>> svc_xprt.h
>> index 0127dac..c2aa8cd 100644
>> --- a/include/linux/sunrpc/svc_xprt.h
>> +++ b/include/linux/sunrpc/svc_xprt.h
>> @@ -113,7 +113,7 @@ static inline unsigned short  
>> svc_addr_port(struct sockaddr *sa)
>> 	return ret;
>> }
>>
>> -static inline size_t svc_addr_len(struct sockaddr *sa)
>> +static inline int svc_addr_len(const struct sockaddr *sa)
>> {
>> 	switch (sa->sa_family) {
>> 	case AF_INET:
>> @@ -121,6 +121,7 @@ static inline size_t svc_addr_len(struct  
>> sockaddr *sa)
>> 	case AF_INET6:
>> 		return sizeof(struct sockaddr_in6);
>> 	}
>> +
>> 	return -EAFNOSUPPORT;
>> }

It's clearly a bug to return -EAFNOSUPPORT in a size_t.

>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/ 
>> xprtrdma/svc_rdma_transport.c
>> index 3d810e7..d1ec6f9 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -546,6 +546,7 @@ static void handle_connect_req(struct  
>> rdma_cm_id *new_cma_id, size_t client_ird)
>> 	struct svcxprt_rdma *listen_xprt = new_cma_id->context;
>> 	struct svcxprt_rdma *newxprt;
>> 	struct sockaddr *sa;
>> +	int len;
>>
>> 	/* Create a new transport */
>> 	newxprt = rdma_create_xprt(listen_xprt->sc_xprt.xpt_server, 0);
>> @@ -563,9 +564,20 @@ static void handle_connect_req(struct  
>> rdma_cm_id *new_cma_id, size_t client_ird)
>>
>> 	/* Set the local and remote addresses in the transport */
>> 	sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
>> -	svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>> +	len = svc_addr_len(sa);
>> +	if (len < 0) {
>> +		dprintk("svcrdma: dst_addr has a bad address family\n");
>> +		return;
>
> we're probably leaking something here.
>
> I don't want to fix this until it's understood well enough to fix it
> correctly.

Tom needs respond to this, but I think it would be safe to simply  
BUG() here.

> --b.
>
>> +	}
>> +	svc_xprt_set_remote(&newxprt->sc_xprt, sa, len);
>> +
>> 	sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
>> -	svc_xprt_set_local(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>> +	len = svc_addr_len(sa);
>> +	if (len < 0) {
>> +		dprintk("svcrdma: src_addr has a bad address family\n");
>> +		return;
>> +	}
>> +	svc_xprt_set_local(&newxprt->sc_xprt, sa, len);
>>
>> 	/*
>> 	 * Enqueue the new transport on the accept queue of the listening

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

Re: [PATCH 02/23] SUNRPC: Fix return type of svc_addr_len()

[J. Bruce Fields]

On Tue, Mar 24, 2009 at 04:37:49PM -0400, Chuck Lever wrote:
> On Mar 24, 2009, at 4:32 PM, J. Bruce Fields wrote:
>> On Wed, Mar 18, 2009 at 08:45:36PM -0400, Chuck Lever wrote:
>>> The svc_addr_len() helper function can return a negative errno value,
>>> but its return type is size_t, which is unsigned.
>>>
>>> The RDMA transport code passes this return value directly to  
>>> memset(),
>>> without checking first if it's negative.  This could cause memset()  
>>> to
>>> clobber a large piece of memory if svc_addr_len() has returned an
>>> error.
>>
>> I'd still like to understand when this can happen, to better  
>> understand
>> how the error should be handled.
>>
>> Also:
>>
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> include/linux/sunrpc/svc_xprt.h          |    3 ++-
>>> net/sunrpc/xprtrdma/svc_rdma_transport.c |   16 ++++++++++++++--
>>> 2 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/ 
>>> svc_xprt.h
>>> index 0127dac..c2aa8cd 100644
>>> --- a/include/linux/sunrpc/svc_xprt.h
>>> +++ b/include/linux/sunrpc/svc_xprt.h
>>> @@ -113,7 +113,7 @@ static inline unsigned short  
>>> svc_addr_port(struct sockaddr *sa)
>>> 	return ret;
>>> }
>>>
>>> -static inline size_t svc_addr_len(struct sockaddr *sa)
>>> +static inline int svc_addr_len(const struct sockaddr *sa)
>>> {
>>> 	switch (sa->sa_family) {
>>> 	case AF_INET:
>>> @@ -121,6 +121,7 @@ static inline size_t svc_addr_len(struct  
>>> sockaddr *sa)
>>> 	case AF_INET6:
>>> 		return sizeof(struct sockaddr_in6);
>>> 	}
>>> +
>>> 	return -EAFNOSUPPORT;
>>> }
>
> It's clearly a bug to return -EAFNOSUPPORT in a size_t.

I agree.  My comment is just that this patch doesn't necessarily fix the
real bug; so as long as there's a bug here, I'd prefer it to be an
obvious one.

>>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/ 
>>> xprtrdma/svc_rdma_transport.c
>>> index 3d810e7..d1ec6f9 100644
>>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> @@ -546,6 +546,7 @@ static void handle_connect_req(struct rdma_cm_id 
>>> *new_cma_id, size_t client_ird)
>>> 	struct svcxprt_rdma *listen_xprt = new_cma_id->context;
>>> 	struct svcxprt_rdma *newxprt;
>>> 	struct sockaddr *sa;
>>> +	int len;
>>>
>>> 	/* Create a new transport */
>>> 	newxprt = rdma_create_xprt(listen_xprt->sc_xprt.xpt_server, 0);
>>> @@ -563,9 +564,20 @@ static void handle_connect_req(struct  
>>> rdma_cm_id *new_cma_id, size_t client_ird)
>>>
>>> 	/* Set the local and remote addresses in the transport */
>>> 	sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
>>> -	svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>>> +	len = svc_addr_len(sa);
>>> +	if (len < 0) {
>>> +		dprintk("svcrdma: dst_addr has a bad address family\n");
>>> +		return;
>>
>> we're probably leaking something here.
>>
>> I don't want to fix this until it's understood well enough to fix it
>> correctly.
>
> Tom needs respond to this, but I think it would be safe to simply BUG() 
> here.

Could be.--b.

Re: [PATCH 00/23] Shorter series for 2.6.30

[J. Bruce Fields]

On Wed, Mar 18, 2009 at 08:45:17PM -0400, Chuck Lever wrote:
> Hi Trond, Bruce-
> 
> Since Trond has sent some late-breaking patches for 2.6.29, I had to
> fix some merge problems with my latest patch set.  Jeff also found a
> minor bug with rpcbind version 4 replies that I'd like to include.
> 
> I've stripped this patch set down so it is just 23 patches.
> Unfortunately, it touches server-side RPC stuff and client-side NFSv4
> callback and lockd support.  I don't see a clean way to split these up.
> 
> Can we discuss how to get these into 2.6.30?  Perhaps, since these have
> the greatest impact on client behavior, Trond should take all of these?

Except as noted (#2), I'm fine with them.  Happy to apply them to my
tree, or if Trond takes them, he can add an

	Acked-by: J. Bruce Fields <bfields@citi.umich.edu>

as desired to spread the blame around.

--b.

Re: [PATCH 02/23] SUNRPC: Fix return type of svc_addr_len()

[Greg Banks]

Chuck Lever wrote:
> On Mar 24, 2009, at 4:32 PM, J. Bruce Fields wrote:
>
>>>
>>> @@ -563,9 +564,20 @@ static void handle_connect_req(struct
>>> rdma_cm_id *new_cma_id, size_t client_ird)
>>>
>>> /* Set the local and remote addresses in the transport */
>>> sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
>>> - svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>>> + len = svc_addr_len(sa);
>>> + if (len < 0) {
>>> + dprintk("svcrdma: dst_addr has a bad address family\n");
>>> + return;
>>
>> we're probably leaking something here.
Just a bit. The svcxprt_rdma, the rdma_cm_id, a bunch of other IB state...
>>
>> I don't want to fix this until it's understood well enough to fix it
>> correctly.
>
> Tom needs respond to this, but I think it would be safe to simply
> BUG() here.

My 2c. If I understand the RDMA CM behaviour correctly, it should not be
possible at this point in the code for
newxprt->sc_cm_id->route.addr.{src,dst}_addr to be anything but a valid
sockaddr_storages holding the IPv4 or (theoretically) the IPv6 address
of the server and client. So I would be happy with a BUG_ON(len < 0).
That would also render the leakage moot.

-- 
Greg Banks, P.Engineer, SGI Australian Software Group.
the brightly coloured sporks of revolution.
I don't speak for SGI.

Re: [PATCH 15/23] SUNRPC: rpcbind actually interprets r_owner string

[Greg Banks]

Chuck Lever wrote:
>
> Our port of rpcbind (from Sun) assumes this string contains a numeric
> UID value, not alphabetical or symbolic characters, but checks this
> value only for AF_LOCAL RPCB_SET or RPCB_UNSET requests.  In all other
> cases, rpcbind ignores the contents of the r_owner string.
>   

Not that this makes the slightest difference to the usefulness of the
patch, but it sounds like pretty strange behaviour for an rpcbind server
to be using an incoming r_owner value off the wire under any circumstances.

If I read the Irix and Solaris rpcbind source correctly, they will
(sensibly, IMO) ignore incoming r_owner values in all cases.  When an
owner string is needed (during SET for storing in the internal data
structure, and during UNSET for checking against the internal data
structure) it is calculated from SVCXPRT itself and not from anything
the client sends.  The calculation returns one of:

a) if the transport is over TLI loopback, a numerical representation of
the peer process UID as reported by the special magical TLI kernel hack
which reliably returns that, or

b) if the transport is over IP and the peer port is privileged, the
string "superuser", or

c) otherwise, the string "unknown".

AFAICS the only sensible use for the r_owner field is reporting the
internally-calculated value back to the client, e.g. for the "rpcinfo
-s" display.

> The reference user space implementation of rpcb_set(3) uses a numeric
> UID for all SET/UNSET requests (even via the network) and an empty
> string for all other requests.  We emulate that behavior here to
> maintain bug-for-bug compatibility.
>   

Fair enough.

Reviewed-by: Greg Banks <gnb@sgi.com>

-- 
Greg Banks, P.Engineer, SGI Australian Software Group.
the brightly coloured sporks of revolution.
I don't speak for SGI.

Re: [PATCH 15/23] SUNRPC: rpcbind actually interprets r_owner string

[Chuck Lever]

On Mar 26, 2009, at 4:58 AM, Greg Banks wrote:
> Chuck Lever wrote:
>>
>> Our port of rpcbind (from Sun) assumes this string contains a numeric
>> UID value, not alphabetical or symbolic characters, but checks this
>> value only for AF_LOCAL RPCB_SET or RPCB_UNSET requests.  In all  
>> other
>> cases, rpcbind ignores the contents of the r_owner string.
>
> Not that this makes the slightest difference to the usefulness of the
> patch, but it sounds like pretty strange behaviour for an rpcbind  
> server
> to be using an incoming r_owner value off the wire under any  
> circumstances.

It's ignored for wire requests.  r_owner is used only for AF_LOCAL  
requests (ie a local file socket) where the kernel can guarantee the  
owner.

>> The reference user space implementation of rpcb_set(3) uses a numeric
>> UID for all SET/UNSET requests (even via the network) and an empty
>> string for all other requests.  We emulate that behavior here to
>> maintain bug-for-bug compatibility.
>
> Fair enough.
>
> Reviewed-by: Greg Banks <gnb@sgi.com>

Thanks.

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

Re: [PATCH 02/23] SUNRPC: Fix return type of svc_addr_len()

[Chuck Lever]

On Mar 26, 2009, at 3:43 AM, Greg Banks wrote:
> Chuck Lever wrote:
>> On Mar 24, 2009, at 4:32 PM, J. Bruce Fields wrote:
>>
>>>>
>>>> @@ -563,9 +564,20 @@ static void handle_connect_req(struct
>>>> rdma_cm_id *new_cma_id, size_t client_ird)
>>>>
>>>> /* Set the local and remote addresses in the transport */
>>>> sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
>>>> - svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>>>> + len = svc_addr_len(sa);
>>>> + if (len < 0) {
>>>> + dprintk("svcrdma: dst_addr has a bad address family\n");
>>>> + return;
>>>
>>> we're probably leaking something here.
> Just a bit. The svcxprt_rdma, the rdma_cm_id, a bunch of other IB  
> state...
>>>
>>> I don't want to fix this until it's understood well enough to fix it
>>> correctly.
>>
>> Tom needs respond to this, but I think it would be safe to simply
>> BUG() here.
>
> My 2c. If I understand the RDMA CM behaviour correctly, it should  
> not be
> possible at this point in the code for
> newxprt->sc_cm_id->route.addr.{src,dst}_addr to be anything but a  
> valid
> sockaddr_storages holding the IPv4 or (theoretically) the IPv6 address
> of the server and client. So I would be happy with a BUG_ON(len < 0).
> That would also render the leakage moot.

My primary objection is the negative return value from  
svc_addr_len().  Perhaps it would be safer all around if  
svc_addr_len() returned zero if it didn't recognize the address family  
in the passed in sockaddr.

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

Re: [PATCH 15/23] SUNRPC: rpcbind actually interprets r_owner string

[Greg Banks]

Chuck Lever wrote:
> On Mar 26, 2009, at 4:58 AM, Greg Banks wrote:
>> Chuck Lever wrote:
>>>
>>> Our port of rpcbind (from Sun) assumes this string contains a numeric
>>> UID value, not alphabetical or symbolic characters, but checks this
>>> value only for AF_LOCAL RPCB_SET or RPCB_UNSET requests.  In all other
>>> cases, rpcbind ignores the contents of the r_owner string.
>>
>> Not that this makes the slightest difference to the usefulness of the
>> patch, but it sounds like pretty strange behaviour for an rpcbind server
>> to be using an incoming r_owner value off the wire under any
>> circumstances.
>
> It's ignored for wire requests.  r_owner is used only for AF_LOCAL
> requests (ie a local file socket) where the kernel can guarantee the
> owner.
>
>
Sorry, I'm confused now.  Consider the r_owner field in the rpcb
structure whose XDR representation flows through the AF_LOCAL socket; is
that used by rpcbind at all?  I don't understand how the kernel would
guarantee that?

-- 
Greg Banks, P.Engineer, SGI Australian Software Group.
the brightly coloured sporks of revolution.
I don't speak for SGI.

Re: [PATCH 15/23] SUNRPC: rpcbind actually interprets r_owner string

[Chuck Lever]

On Mar 26, 2009, at 7:18 PM, Greg Banks wrote:
> Chuck Lever wrote:
>> On Mar 26, 2009, at 4:58 AM, Greg Banks wrote:
>>> Chuck Lever wrote:
>>>>
>>>> Our port of rpcbind (from Sun) assumes this string contains a  
>>>> numeric
>>>> UID value, not alphabetical or symbolic characters, but checks this
>>>> value only for AF_LOCAL RPCB_SET or RPCB_UNSET requests.  In all  
>>>> other
>>>> cases, rpcbind ignores the contents of the r_owner string.
>>>
>>> Not that this makes the slightest difference to the usefulness of  
>>> the
>>> patch, but it sounds like pretty strange behaviour for an rpcbind  
>>> server
>>> to be using an incoming r_owner value off the wire under any
>>> circumstances.
>>
>> It's ignored for wire requests.  r_owner is used only for AF_LOCAL
>> requests (ie a local file socket) where the kernel can guarantee the
>> owner.
>>
>>
> Sorry, I'm confused now.  Consider the r_owner field in the rpcb
> structure whose XDR representation flows through the AF_LOCAL  
> socket; is
> that used by rpcbind at all?  I don't understand how the kernel would
> guarantee that?

The library uses geteuid(2) to generate the r_owner string during  
RPCB_SET, as I mentioned before.  This is likely legacy behavior.

rpcbind then throws this away and uses getsockopt(SO_PEERCRED).  On  
network sockets I think that's always going to return a uid of -1, but  
for AF_LOCAL it will have a meaningful value.

I was loose about the use of the term "r_owner" : the r_owner  
parameter of RPCB_SET is always ignored, but _owner_ _information_ is  
used for requests coming in via AF_LOCAL.  The rpcb_clnt patch for the  
kernel is nothing more than a documentation change.

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

Re: [PATCH 15/23] SUNRPC: rpcbind actually interprets r_owner string

[Greg Banks]

Chuck Lever wrote:
> On Mar 26, 2009, at 7:18 PM, Greg Banks wrote:
>
> rpcbind then throws this away and uses getsockopt(SO_PEERCRED).  On
> network sockets I think that's always going to return a uid of -1, but
> for AF_LOCAL it will have a meaningful value.
Aha.  Unconfused now,  thanks.

-- 
Greg Banks, P.Engineer, SGI Australian Software Group.
the brightly coloured sporks of revolution.
I don't speak for SGI.

Re: [PATCH 02/23] SUNRPC: Fix return type of svc_addr_len()

[Tom Tucker]

J. Bruce Fields wrote:
> On Wed, Mar 18, 2009 at 08:45:36PM -0400, Chuck Lever wrote:
>> The svc_addr_len() helper function can return a negative errno value,
>> but its return type is size_t, which is unsigned.
>>
>> The RDMA transport code passes this return value directly to memset(),
>> without checking first if it's negative.  This could cause memset() to
>> clobber a large piece of memory if svc_addr_len() has returned an
>> error.
> 
> I'd still like to understand when this can happen, to better understand
> how the error should be handled.


I don't think that the current code base can cause this to occur.

My recollection is that this code was added at the time we were 
in-flight with the IPv6 integration and I was somewhat uncomfortable bug 
checking on an unknown address type, however, this may in fact be the 
right thing to do.

I think changing the return type to int is fine.


> 
> Also:
> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>>  include/linux/sunrpc/svc_xprt.h          |    3 ++-
>>  net/sunrpc/xprtrdma/svc_rdma_transport.c |   16 ++++++++++++++--
>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
>> index 0127dac..c2aa8cd 100644
>> --- a/include/linux/sunrpc/svc_xprt.h
>> +++ b/include/linux/sunrpc/svc_xprt.h
>> @@ -113,7 +113,7 @@ static inline unsigned short svc_addr_port(struct sockaddr *sa)
>>  	return ret;
>>  }
>>  
>> -static inline size_t svc_addr_len(struct sockaddr *sa)
>> +static inline int svc_addr_len(const struct sockaddr *sa)
>>  {
>>  	switch (sa->sa_family) {
>>  	case AF_INET:
>> @@ -121,6 +121,7 @@ static inline size_t svc_addr_len(struct sockaddr *sa)
>>  	case AF_INET6:
>>  		return sizeof(struct sockaddr_in6);
>>  	}
>> +
>>  	return -EAFNOSUPPORT;
>>  }
>>  
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> index 3d810e7..d1ec6f9 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -546,6 +546,7 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
>>  	struct svcxprt_rdma *listen_xprt = new_cma_id->context;
>>  	struct svcxprt_rdma *newxprt;
>>  	struct sockaddr *sa;
>> +	int len;
>>  
>>  	/* Create a new transport */
>>  	newxprt = rdma_create_xprt(listen_xprt->sc_xprt.xpt_server, 0);
>> @@ -563,9 +564,20 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
>>  
>>  	/* Set the local and remote addresses in the transport */
>>  	sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
>> -	svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>> +	len = svc_addr_len(sa);
>> +	if (len < 0) {
>> +		dprintk("svcrdma: dst_addr has a bad address family\n");
>> +		return;
> 
> we're probably leaking something here.
> 
> I don't want to fix this until it's understood well enough to fix it
> correctly.
> 
> --b.
> 
>> +	}
>> +	svc_xprt_set_remote(&newxprt->sc_xprt, sa, len);
>> +
>>  	sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
>> -	svc_xprt_set_local(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>> +	len = svc_addr_len(sa);
>> +	if (len < 0) {
>> +		dprintk("svcrdma: src_addr has a bad address family\n");
>> +		return;
>> +	}
>> +	svc_xprt_set_local(&newxprt->sc_xprt, sa, len);
>>  
>>  	/*
>>  	 * Enqueue the new transport on the accept queue of the listening
>>
> --
> 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 02/23] SUNRPC: Fix return type of svc_addr_len()

[J. Bruce Fields]

On Wed, Apr 01, 2009 at 08:43:11PM -0500, Tom Tucker wrote:
> J. Bruce Fields wrote:
>> On Wed, Mar 18, 2009 at 08:45:36PM -0400, Chuck Lever wrote:
>>> The svc_addr_len() helper function can return a negative errno value,
>>> but its return type is size_t, which is unsigned.
>>>
>>> The RDMA transport code passes this return value directly to memset(),
>>> without checking first if it's negative.  This could cause memset() to
>>> clobber a large piece of memory if svc_addr_len() has returned an
>>> error.
>>
>> I'd still like to understand when this can happen, to better understand
>> how the error should be handled.
>
>
> I don't think that the current code base can cause this to occur.
>
> My recollection is that this code was added at the time we were  
> in-flight with the IPv6 integration and I was somewhat uncomfortable bug  
> checking on an unknown address type, however, this may in fact be the  
> right thing to do.
>
> I think changing the return type to int is fine.

OK, thanks.  So sounds like the consensus is that the return value
should either trigger a BUG() or just be ignored.--b.

Re: [PATCH 02/23] SUNRPC: Fix return type of svc_addr_len()

[Chuck Lever]

On Apr 2, 2009, at 6:39 PM, J. Bruce Fields wrote:
> On Wed, Apr 01, 2009 at 08:43:11PM -0500, Tom Tucker wrote:
>> J. Bruce Fields wrote:
>>> On Wed, Mar 18, 2009 at 08:45:36PM -0400, Chuck Lever wrote:
>>>> The svc_addr_len() helper function can return a negative errno  
>>>> value,
>>>> but its return type is size_t, which is unsigned.
>>>>
>>>> The RDMA transport code passes this return value directly to  
>>>> memset(),
>>>> without checking first if it's negative.  This could cause  
>>>> memset() to
>>>> clobber a large piece of memory if svc_addr_len() has returned an
>>>> error.
>>>
>>> I'd still like to understand when this can happen, to better  
>>> understand
>>> how the error should be handled.
>>
>>
>> I don't think that the current code base can cause this to occur.
>>
>> My recollection is that this code was added at the time we were
>> in-flight with the IPv6 integration and I was somewhat  
>> uncomfortable bug
>> checking on an unknown address type, however, this may in fact be the
>> right thing to do.
>>
>> I think changing the return type to int is fine.
>
> OK, thanks.  So sounds like the consensus is that the return value
> should either trigger a BUG() or just be ignored.--b.

I crafted a new version of this patch where svc_addr_len() simply  
returns zero if it doesn't recognize the incoming address family, with  
no changes to the RDMA transport code.  I figure this is safer overall.

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