[PATCH 0/13] sunrpc: Compact the xprtsock code a bit

[PATCH 0/13] sunrpc: Compact the xprtsock code a bit

[Pavel Emelyanov]

Hi!

This set removes some unused parts of code and merges together
routines that are related to the sockets creation.

After all the changes the result is

 net/sunrpc/xprtsock.c: 77 insertions(+), 206 deletions(-)

from the code-lines point of view and

  add/remove: 2/11 grow/shrink: 4/1 up/down: 879/-1626 (-747)

from the bloat-o-meter's point of view.

Thanks,
Pavel

[PATCH 1/13] sunrpc: Remove unused sock arg from xs_get_srcport

[Pavel Emelyanov]

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index f9964ef..304e2de 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1515,7 +1515,7 @@ static void xs_set_port(struct rpc_xprt *xprt, unsigned short port)
 	xs_update_peer_port(xprt);
 }
 
-static unsigned short xs_get_srcport(struct sock_xprt *transport, struct socket *sock)
+static unsigned short xs_get_srcport(struct sock_xprt *transport)
 {
 	unsigned short port = transport->srcport;
 
@@ -1542,7 +1542,7 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
 	};
 	struct sockaddr_in *sa;
 	int err, nloop = 0;
-	unsigned short port = xs_get_srcport(transport, sock);
+	unsigned short port = xs_get_srcport(transport);
 	unsigned short last;
 
 	sa = (struct sockaddr_in *)&transport->srcaddr;
@@ -1575,7 +1575,7 @@ static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
 	};
 	struct sockaddr_in6 *sa;
 	int err, nloop = 0;
-	unsigned short port = xs_get_srcport(transport, sock);
+	unsigned short port = xs_get_srcport(transport);
 	unsigned short last;
 
 	sa = (struct sockaddr_in6 *)&transport->srcaddr;
-- 
1.5.5.6

[PATCH 2/13] sunrpc: Remove unused sock arg from xs_next_srcport

[Pavel Emelyanov]

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 304e2de..024a644 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1524,7 +1524,7 @@ static unsigned short xs_get_srcport(struct sock_xprt *transport)
 	return port;
 }
 
-static unsigned short xs_next_srcport(struct sock_xprt *transport, struct socket *sock, unsigned short port)
+static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned short port)
 {
 	if (transport->srcport != 0)
 		transport->srcport = 0;
@@ -1558,7 +1558,7 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
 			break;
 		}
 		last = port;
-		port = xs_next_srcport(transport, sock, port);
+		port = xs_next_srcport(transport, port);
 		if (port > last)
 			nloop++;
 	} while (err == -EADDRINUSE && nloop != 2);
@@ -1591,7 +1591,7 @@ static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
 			break;
 		}
 		last = port;
-		port = xs_next_srcport(transport, sock, port);
+		port = xs_next_srcport(transport, port);
 		if (port > last)
 			nloop++;
 	} while (err == -EADDRINUSE && nloop != 2);
-- 
1.5.5.6

[PATCH 3/13] sunrpc: Get xprt pointer once in xs_tcp_setup_socket

[Pavel Emelyanov]

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 024a644..a76446a 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1851,12 +1851,12 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
  *
  * Invoked by a work queue tasklet.
  */
-static void xs_tcp_setup_socket(struct rpc_xprt *xprt,
-		struct sock_xprt *transport,
+static void xs_tcp_setup_socket(struct sock_xprt *transport,
 		struct socket *(*create_sock)(struct rpc_xprt *,
 			struct sock_xprt *))
 {
 	struct socket *sock = transport->sock;
+	struct rpc_xprt *xprt = &transport->xprt;
 	int status = -EIO;
 
 	if (xprt->shutdown)
@@ -1958,9 +1958,8 @@ static void xs_tcp_connect_worker4(struct work_struct *work)
 {
 	struct sock_xprt *transport =
 		container_of(work, struct sock_xprt, connect_worker.work);
-	struct rpc_xprt *xprt = &transport->xprt;
 
-	xs_tcp_setup_socket(xprt, transport, xs_create_tcp_sock4);
+	xs_tcp_setup_socket(transport, xs_create_tcp_sock4);
 }
 
 static struct socket *xs_create_tcp_sock6(struct rpc_xprt *xprt,
@@ -1997,9 +1996,8 @@ static void xs_tcp_connect_worker6(struct work_struct *work)
 {
 	struct sock_xprt *transport =
 		container_of(work, struct sock_xprt, connect_worker.work);
-	struct rpc_xprt *xprt = &transport->xprt;
 
-	xs_tcp_setup_socket(xprt, transport, xs_create_tcp_sock6);
+	xs_tcp_setup_socket(transport, xs_create_tcp_sock6);
 }
 
 /**
-- 
1.5.5.6

[PATCH 4/13] sunrpc: Remove duplicate xprt/transport arguments from calls

[Pavel Emelyanov]

The xs_tcp_reuse_connection takes the xprt only to pass it down
to the xs_abort_connection. The later one can get it from the given
transport itself.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index a76446a..8ff57c5 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1754,12 +1754,12 @@ out:
  * We need to preserve the port number so the reply cache on the server can
  * find our cached RPC replies when we get around to reconnecting.
  */
-static void xs_abort_connection(struct rpc_xprt *xprt, struct sock_xprt *transport)
+static void xs_abort_connection(struct sock_xprt *transport)
 {
 	int result;
 	struct sockaddr any;
 
-	dprintk("RPC:       disconnecting xprt %p to reuse port\n", xprt);
+	dprintk("RPC:       disconnecting xprt %p to reuse port\n", transport);
 
 	/*
 	 * Disconnect the transport socket by doing a connect operation
@@ -1769,13 +1769,13 @@ static void xs_abort_connection(struct rpc_xprt *xprt, struct sock_xprt *transpo
 	any.sa_family = AF_UNSPEC;
 	result = kernel_connect(transport->sock, &any, sizeof(any), 0);
 	if (!result)
-		xs_sock_mark_closed(xprt);
+		xs_sock_mark_closed(&transport->xprt);
 	else
 		dprintk("RPC:       AF_UNSPEC connect return code %d\n",
 				result);
 }
 
-static void xs_tcp_reuse_connection(struct rpc_xprt *xprt, struct sock_xprt *transport)
+static void xs_tcp_reuse_connection(struct sock_xprt *transport)
 {
 	unsigned int state = transport->inet->sk_state;
 
@@ -1798,7 +1798,7 @@ static void xs_tcp_reuse_connection(struct rpc_xprt *xprt, struct sock_xprt *tra
 				"sk_shutdown set to %d\n",
 				__func__, transport->inet->sk_shutdown);
 	}
-	xs_abort_connection(xprt, transport);
+	xs_abort_connection(transport);
 }
 
 static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
@@ -1875,7 +1875,7 @@ static void xs_tcp_setup_socket(struct sock_xprt *transport,
 		abort_and_exit = test_and_clear_bit(XPRT_CONNECTION_ABORT,
 				&xprt->state);
 		/* "close" the socket, preserving the local port */
-		xs_tcp_reuse_connection(xprt, transport);
+		xs_tcp_reuse_connection(transport);
 
 		if (abort_and_exit)
 			goto out_eagain;
-- 
1.5.5.6

[PATCH 5/13] sunrpc: Factor out udp sockets creation

[Pavel Emelyanov]

Make it look like the TCP sockets creation.
Unfortunately the git diff made the patch look messy :(

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |   96 ++++++++++++++++++++++++++++--------------------
 1 files changed, 56 insertions(+), 40 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 8ff57c5..df53dc5 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1660,37 +1660,22 @@ static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 	xs_udp_do_set_buffer_size(xprt);
 }
 
-/**
- * xs_udp_connect_worker4 - set up a UDP socket
- * @work: RPC transport to connect
- *
- * Invoked by a work queue tasklet.
- */
-static void xs_udp_connect_worker4(struct work_struct *work)
+static void xs_udp_setup_socket(struct sock_xprt *transport,
+		struct socket *(*create_sock)(struct rpc_xprt *,
+			struct sock_xprt *))
 {
-	struct sock_xprt *transport =
-		container_of(work, struct sock_xprt, connect_worker.work);
 	struct rpc_xprt *xprt = &transport->xprt;
 	struct socket *sock = transport->sock;
-	int err, status = -EIO;
+	int status = -EIO;
 
 	if (xprt->shutdown)
 		goto out;
 
 	/* Start by resetting any existing state */
 	xs_reset_transport(transport);
-
-	err = __sock_create(xprt->xprt_net, PF_INET, SOCK_DGRAM, IPPROTO_UDP, &sock, 1);
-	if (err < 0) {
-		dprintk("RPC:       can't create UDP transport socket (%d).\n", -err);
+	sock = create_sock(xprt, transport);
+	if (IS_ERR(sock))
 		goto out;
-	}
-	xs_reclassify_socket4(sock);
-
-	if (xs_bind4(transport, sock)) {
-		sock_release(sock);
-		goto out;
-	}
 
 	dprintk("RPC:       worker connecting xprt %p via %s to "
 				"%s (port %s)\n", xprt,
@@ -1706,24 +1691,55 @@ out:
 }
 
 /**
- * xs_udp_connect_worker6 - set up a UDP socket
+ * xs_udp_connect_worker4 - set up a UDP socket
  * @work: RPC transport to connect
  *
  * Invoked by a work queue tasklet.
  */
-static void xs_udp_connect_worker6(struct work_struct *work)
+
+static struct socket *xs_create_udp_sock4(struct rpc_xprt *xprt,
+		struct sock_xprt *transport)
+{
+	struct socket *sock;
+	int err;
+
+	err = __sock_create(xprt->xprt_net, PF_INET, SOCK_DGRAM, IPPROTO_UDP, &sock, 1);
+	if (err < 0) {
+		dprintk("RPC:       can't create UDP transport socket (%d).\n", -err);
+		goto out;
+	}
+	xs_reclassify_socket4(sock);
+
+	if (xs_bind4(transport, sock)) {
+		sock_release(sock);
+		goto out;
+	}
+
+	return sock;
+out:
+	return ERR_PTR(err);
+}
+
+static void xs_udp_connect_worker4(struct work_struct *work)
 {
 	struct sock_xprt *transport =
 		container_of(work, struct sock_xprt, connect_worker.work);
-	struct rpc_xprt *xprt = &transport->xprt;
-	struct socket *sock = transport->sock;
-	int err, status = -EIO;
 
-	if (xprt->shutdown)
-		goto out;
+	xs_udp_setup_socket(transport, xs_create_udp_sock4);
+}
 
-	/* Start by resetting any existing state */
-	xs_reset_transport(transport);
+/**
+ * xs_udp_connect_worker6 - set up a UDP socket
+ * @work: RPC transport to connect
+ *
+ * Invoked by a work queue tasklet.
+ */
+
+static struct socket *xs_create_udp_sock6(struct rpc_xprt *xprt,
+		struct sock_xprt *transport)
+{
+	struct socket *sock;
+	int err;
 
 	err = __sock_create(xprt->xprt_net, PF_INET6, SOCK_DGRAM, IPPROTO_UDP, &sock, 1);
 	if (err < 0) {
@@ -1737,17 +1753,17 @@ static void xs_udp_connect_worker6(struct work_struct *work)
 		goto out;
 	}
 
-	dprintk("RPC:       worker connecting xprt %p via %s to "
-				"%s (port %s)\n", xprt,
-			xprt->address_strings[RPC_DISPLAY_PROTO],
-			xprt->address_strings[RPC_DISPLAY_ADDR],
-			xprt->address_strings[RPC_DISPLAY_PORT]);
-
-	xs_udp_finish_connecting(xprt, sock);
-	status = 0;
+	return sock;
 out:
-	xprt_clear_connecting(xprt);
-	xprt_wake_pending_tasks(xprt, status);
+	return ERR_PTR(err);
+}
+
+static void xs_udp_connect_worker6(struct work_struct *work)
+{
+	struct sock_xprt *transport =
+		container_of(work, struct sock_xprt, connect_worker.work);
+
+	xs_udp_setup_socket(transport, xs_create_udp_sock6);
 }
 
 /*
-- 
1.5.5.6

[PATCH 6/13] sunrpc: Factor out v4 sockets creation

[Pavel Emelyanov]

The UDPv4 and TCPv4 socket creation callbacks now look very similar.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |   63 ++++++++++++++++++++----------------------------
 1 files changed, 26 insertions(+), 37 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index df53dc5..b73a605 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1631,6 +1631,30 @@ static inline void xs_reclassify_socket6(struct socket *sock)
 }
 #endif
 
+static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
+		struct sock_xprt *transport, int type, int protocol)
+{
+	struct socket *sock;
+	int err;
+
+	err = __sock_create(xprt->xprt_net, PF_INET, type, protocol, &sock, 1);
+	if (err < 0) {
+		dprintk("RPC:       can't create %d transport socket (%d).\n",
+				protocol, -err);
+		goto out;
+	}
+	xs_reclassify_socket4(sock);
+
+	if (xs_bind4(transport, sock)) {
+		sock_release(sock);
+		goto out;
+	}
+
+	return sock;
+out:
+	return ERR_PTR(err);
+}
+
 static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 {
 	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
@@ -1700,24 +1724,7 @@ out:
 static struct socket *xs_create_udp_sock4(struct rpc_xprt *xprt,
 		struct sock_xprt *transport)
 {
-	struct socket *sock;
-	int err;
-
-	err = __sock_create(xprt->xprt_net, PF_INET, SOCK_DGRAM, IPPROTO_UDP, &sock, 1);
-	if (err < 0) {
-		dprintk("RPC:       can't create UDP transport socket (%d).\n", -err);
-		goto out;
-	}
-	xs_reclassify_socket4(sock);
-
-	if (xs_bind4(transport, sock)) {
-		sock_release(sock);
-		goto out;
-	}
-
-	return sock;
-out:
-	return ERR_PTR(err);
+	return xs_create_sock4(xprt, transport, SOCK_DGRAM, IPPROTO_UDP);
 }
 
 static void xs_udp_connect_worker4(struct work_struct *work)
@@ -1943,25 +1950,7 @@ out:
 static struct socket *xs_create_tcp_sock4(struct rpc_xprt *xprt,
 		struct sock_xprt *transport)
 {
-	struct socket *sock;
-	int err;
-
-	/* start from scratch */
-	err = __sock_create(xprt->xprt_net, PF_INET, SOCK_STREAM, IPPROTO_TCP, &sock, 1);
-	if (err < 0) {
-		dprintk("RPC:       can't create TCP transport socket (%d).\n",
-				-err);
-		goto out_err;
-	}
-	xs_reclassify_socket4(sock);
-
-	if (xs_bind4(transport, sock) < 0) {
-		sock_release(sock);
-		goto out_err;
-	}
-	return sock;
-out_err:
-	return ERR_PTR(-EIO);
+	return xs_create_sock4(xprt, transport, SOCK_STREAM, IPPROTO_TCP);
 }
 
 /**
-- 
1.5.5.6

[PATCH 7/13] sunrpc: Factor out v6 sockets creation

[Pavel Emelyanov]

Same patch for v6 protocols.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |   63 ++++++++++++++++++++----------------------------
 1 files changed, 26 insertions(+), 37 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index b73a605..96128d0 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1655,6 +1655,30 @@ out:
 	return ERR_PTR(err);
 }
 
+static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
+		struct sock_xprt *transport, int type, int protocol)
+{
+	struct socket *sock;
+	int err;
+
+	err = __sock_create(xprt->xprt_net, PF_INET6, type, protocol, &sock, 1);
+	if (err < 0) {
+		dprintk("RPC:       can't create %d transport socket (%d).\n",
+				protocol, -err);
+		goto out;
+	}
+	xs_reclassify_socket6(sock);
+
+	if (xs_bind6(transport, sock)) {
+		sock_release(sock);
+		goto out;
+	}
+
+	return sock;
+out:
+	return ERR_PTR(err);
+}
+
 static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 {
 	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
@@ -1745,24 +1769,7 @@ static void xs_udp_connect_worker4(struct work_struct *work)
 static struct socket *xs_create_udp_sock6(struct rpc_xprt *xprt,
 		struct sock_xprt *transport)
 {
-	struct socket *sock;
-	int err;
-
-	err = __sock_create(xprt->xprt_net, PF_INET6, SOCK_DGRAM, IPPROTO_UDP, &sock, 1);
-	if (err < 0) {
-		dprintk("RPC:       can't create UDP transport socket (%d).\n", -err);
-		goto out;
-	}
-	xs_reclassify_socket6(sock);
-
-	if (xs_bind6(transport, sock) < 0) {
-		sock_release(sock);
-		goto out;
-	}
-
-	return sock;
-out:
-	return ERR_PTR(err);
+	return xs_create_sock6(xprt, transport, SOCK_DGRAM, IPPROTO_UDP);
 }
 
 static void xs_udp_connect_worker6(struct work_struct *work)
@@ -1970,25 +1977,7 @@ static void xs_tcp_connect_worker4(struct work_struct *work)
 static struct socket *xs_create_tcp_sock6(struct rpc_xprt *xprt,
 		struct sock_xprt *transport)
 {
-	struct socket *sock;
-	int err;
-
-	/* start from scratch */
-	err = __sock_create(xprt->xprt_net, PF_INET6, SOCK_STREAM, IPPROTO_TCP, &sock, 1);
-	if (err < 0) {
-		dprintk("RPC:       can't create TCP transport socket (%d).\n",
-				-err);
-		goto out_err;
-	}
-	xs_reclassify_socket6(sock);
-
-	if (xs_bind6(transport, sock) < 0) {
-		sock_release(sock);
-		goto out_err;
-	}
-	return sock;
-out_err:
-	return ERR_PTR(-EIO);
+	return xs_create_sock6(xprt, transport, SOCK_STREAM, IPPROTO_TCP);
 }
 
 /**
-- 
1.5.5.6

[PATCH 8/13] sunrpc: Call xs_create_sockX directly from setup_socket

[Pavel Emelyanov]

Remove now unneeded wrappers that just add type and protocol
to socket creation callback.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |   40 ++++++++--------------------------------
 1 files changed, 8 insertions(+), 32 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 96128d0..7fdf2bb 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1710,7 +1710,7 @@ static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 
 static void xs_udp_setup_socket(struct sock_xprt *transport,
 		struct socket *(*create_sock)(struct rpc_xprt *,
-			struct sock_xprt *))
+			struct sock_xprt *, int type, int protocol))
 {
 	struct rpc_xprt *xprt = &transport->xprt;
 	struct socket *sock = transport->sock;
@@ -1721,7 +1721,7 @@ static void xs_udp_setup_socket(struct sock_xprt *transport,
 
 	/* Start by resetting any existing state */
 	xs_reset_transport(transport);
-	sock = create_sock(xprt, transport);
+	sock = create_sock(xprt, transport, SOCK_DGRAM, IPPROTO_UDP);
 	if (IS_ERR(sock))
 		goto out;
 
@@ -1745,18 +1745,12 @@ out:
  * Invoked by a work queue tasklet.
  */
 
-static struct socket *xs_create_udp_sock4(struct rpc_xprt *xprt,
-		struct sock_xprt *transport)
-{
-	return xs_create_sock4(xprt, transport, SOCK_DGRAM, IPPROTO_UDP);
-}
-
 static void xs_udp_connect_worker4(struct work_struct *work)
 {
 	struct sock_xprt *transport =
 		container_of(work, struct sock_xprt, connect_worker.work);
 
-	xs_udp_setup_socket(transport, xs_create_udp_sock4);
+	xs_udp_setup_socket(transport, xs_create_sock4);
 }
 
 /**
@@ -1766,18 +1760,12 @@ static void xs_udp_connect_worker4(struct work_struct *work)
  * Invoked by a work queue tasklet.
  */
 
-static struct socket *xs_create_udp_sock6(struct rpc_xprt *xprt,
-		struct sock_xprt *transport)
-{
-	return xs_create_sock6(xprt, transport, SOCK_DGRAM, IPPROTO_UDP);
-}
-
 static void xs_udp_connect_worker6(struct work_struct *work)
 {
 	struct sock_xprt *transport =
 		container_of(work, struct sock_xprt, connect_worker.work);
 
-	xs_udp_setup_socket(transport, xs_create_udp_sock6);
+	xs_udp_setup_socket(transport, xs_create_sock6);
 }
 
 /*
@@ -1883,7 +1871,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
  */
 static void xs_tcp_setup_socket(struct sock_xprt *transport,
 		struct socket *(*create_sock)(struct rpc_xprt *,
-			struct sock_xprt *))
+			struct sock_xprt *, int type, int protocol))
 {
 	struct socket *sock = transport->sock;
 	struct rpc_xprt *xprt = &transport->xprt;
@@ -1894,7 +1882,7 @@ static void xs_tcp_setup_socket(struct sock_xprt *transport,
 
 	if (!sock) {
 		clear_bit(XPRT_CONNECTION_ABORT, &xprt->state);
-		sock = create_sock(xprt, transport);
+		sock = create_sock(xprt, transport, SOCK_STREAM, IPPROTO_TCP);
 		if (IS_ERR(sock)) {
 			status = PTR_ERR(sock);
 			goto out;
@@ -1954,12 +1942,6 @@ out:
 	xprt_wake_pending_tasks(xprt, status);
 }
 
-static struct socket *xs_create_tcp_sock4(struct rpc_xprt *xprt,
-		struct sock_xprt *transport)
-{
-	return xs_create_sock4(xprt, transport, SOCK_STREAM, IPPROTO_TCP);
-}
-
 /**
  * xs_tcp_connect_worker4 - connect a TCP socket to a remote endpoint
  * @work: RPC transport to connect
@@ -1971,13 +1953,7 @@ static void xs_tcp_connect_worker4(struct work_struct *work)
 	struct sock_xprt *transport =
 		container_of(work, struct sock_xprt, connect_worker.work);
 
-	xs_tcp_setup_socket(transport, xs_create_tcp_sock4);
-}
-
-static struct socket *xs_create_tcp_sock6(struct rpc_xprt *xprt,
-		struct sock_xprt *transport)
-{
-	return xs_create_sock6(xprt, transport, SOCK_STREAM, IPPROTO_TCP);
+	xs_tcp_setup_socket(transport, xs_create_sock4);
 }
 
 /**
@@ -1991,7 +1967,7 @@ static void xs_tcp_connect_worker6(struct work_struct *work)
 	struct sock_xprt *transport =
 		container_of(work, struct sock_xprt, connect_worker.work);
 
-	xs_tcp_setup_socket(transport, xs_create_tcp_sock6);
+	xs_tcp_setup_socket(transport, xs_create_sock6);
 }
 
 /**
-- 
1.5.5.6

[PATCH 9/13] sunrpc: Merge the xs_bind code

[Pavel Emelyanov]

There's the only difference betseen the xs_bind4 and the
xs_bind6 - the size of sockaddr structure they use.

Fortunately its size can be indirectly get from transport and
the port in question resides on the same offset within the
v4 and the v6 sockaddrs.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |   65 +++++++++++++-----------------------------------
 1 files changed, 18 insertions(+), 47 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 7fdf2bb..3618cfd 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1534,23 +1534,19 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
 		return xprt_max_resvport;
 	return --port;
 }
-
-static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
+static int xs_bind(struct sock_xprt *transport, struct socket *sock)
 {
-	struct sockaddr_in myaddr = {
-		.sin_family = AF_INET,
-	};
-	struct sockaddr_in *sa;
+	struct sockaddr myaddr;
 	int err, nloop = 0;
 	unsigned short port = xs_get_srcport(transport);
 	unsigned short last;
 
-	sa = (struct sockaddr_in *)&transport->srcaddr;
-	myaddr.sin_addr = sa->sin_addr;
+	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
 	do {
-		myaddr.sin_port = htons(port);
-		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
-						sizeof(myaddr));
+		BUILD_BUG_ON(offsetof(struct sockaddr_in, sin_port) !=
+				offsetof(struct sockaddr_in6, sin6_port));
+		((struct sockaddr_in *)&myaddr)->sin_port = htons(port);
+		err = kernel_bind(sock, &myaddr, transport->xprt.addrlen);
 		if (port == 0)
 			break;
 		if (err == 0) {
@@ -1562,44 +1558,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
 		if (port > last)
 			nloop++;
 	} while (err == -EADDRINUSE && nloop != 2);
-	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
-			__func__, &myaddr.sin_addr,
-			port, err ? "failed" : "ok", err);
-	return err;
-}
-
-static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
-{
-	struct sockaddr_in6 myaddr = {
-		.sin6_family = AF_INET6,
-	};
-	struct sockaddr_in6 *sa;
-	int err, nloop = 0;
-	unsigned short port = xs_get_srcport(transport);
-	unsigned short last;
 
-	sa = (struct sockaddr_in6 *)&transport->srcaddr;
-	myaddr.sin6_addr = sa->sin6_addr;
-	do {
-		myaddr.sin6_port = htons(port);
-		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
-						sizeof(myaddr));
-		if (port == 0)
-			break;
-		if (err == 0) {
-			transport->srcport = port;
-			break;
-		}
-		last = port;
-		port = xs_next_srcport(transport, port);
-		if (port > last)
-			nloop++;
-	} while (err == -EADDRINUSE && nloop != 2);
-	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
-		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
+	if (myaddr.sa_family == PF_INET)
+		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
+				&((struct sockaddr_in *)&myaddr)->sin_addr,
+				port, err ? "failed" : "ok", err);
+	else
+		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
+				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
+				port, err ? "failed" : "ok", err);
 	return err;
 }
 
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 static struct lock_class_key xs_key[2];
 static struct lock_class_key xs_slock_key[2];
@@ -1645,7 +1616,7 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
 	}
 	xs_reclassify_socket4(sock);
 
-	if (xs_bind4(transport, sock)) {
+	if (xs_bind(transport, sock)) {
 		sock_release(sock);
 		goto out;
 	}
@@ -1669,7 +1640,7 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
 	}
 	xs_reclassify_socket6(sock);
 
-	if (xs_bind6(transport, sock)) {
+	if (xs_bind(transport, sock)) {
 		sock_release(sock);
 		goto out;
 	}
-- 
1.5.5.6

Re: [PATCH 9/13] sunrpc: Merge the xs_bind code

[Chuck Lever]

On Oct 4, 2010, at 8:56 AM, Pavel Emelyanov wrote:

> There's the only difference betseen the xs_bind4 and the
> xs_bind6 - the size of sockaddr structure they use.
> 
> Fortunately its size can be indirectly get from transport and
> the port in question resides on the same offset within the
> v4 and the v6 sockaddrs.
> 
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> ---
> net/sunrpc/xprtsock.c |   65 +++++++++++++-----------------------------------
> 1 files changed, 18 insertions(+), 47 deletions(-)
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 7fdf2bb..3618cfd 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1534,23 +1534,19 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
> 		return xprt_max_resvport;
> 	return --port;
> }
> -
> -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
> {
> -	struct sockaddr_in myaddr = {
> -		.sin_family = AF_INET,
> -	};
> -	struct sockaddr_in *sa;
> +	struct sockaddr myaddr;

You want "struct sockaddr_storage" here.  A "struct sockaddr" is required by standard (iirc) to be only as large as a "struct sockaddr_in".  So this can't possibly hold an IPv6 socket address without overflowing.

> 	int err, nloop = 0;
> 	unsigned short port = xs_get_srcport(transport);
> 	unsigned short last;
> 
> -	sa = (struct sockaddr_in *)&transport->srcaddr;
> -	myaddr.sin_addr = sa->sin_addr;
> +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
> 	do {
> -		myaddr.sin_port = htons(port);
> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> -						sizeof(myaddr));
> +		BUILD_BUG_ON(offsetof(struct sockaddr_in, sin_port) !=
> +				offsetof(struct sockaddr_in6, sin6_port));
> +		((struct sockaddr_in *)&myaddr)->sin_port = htons(port);

This series of patches looks mostly OK at first blush, but this little change is a bit offensive.  :-)

There should be some static inline helper functions that can get and set the port number in a socket address without a lot of magic, and I would prefer you use those instead.  (I would point you directly to them, but I can't easily access kernel source at the moment -- hotel internet sucks).

There should be one or two code examples in the RPC module or in headers that set and get ports in an automatic struct sockaddr_storage variable to give you an idea of what this needs to look like to avoid stack overruns and bad pointer aliasing.

> +		err = kernel_bind(sock, &myaddr, transport->xprt.addrlen);
> 		if (port == 0)
> 			break;
> 		if (err == 0) {
> @@ -1562,44 +1558,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> 		if (port > last)
> 			nloop++;
> 	} while (err == -EADDRINUSE && nloop != 2);
> -	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
> -			__func__, &myaddr.sin_addr,
> -			port, err ? "failed" : "ok", err);
> -	return err;
> -}
> -
> -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
> -{
> -	struct sockaddr_in6 myaddr = {
> -		.sin6_family = AF_INET6,
> -	};
> -	struct sockaddr_in6 *sa;
> -	int err, nloop = 0;
> -	unsigned short port = xs_get_srcport(transport);
> -	unsigned short last;
> 
> -	sa = (struct sockaddr_in6 *)&transport->srcaddr;
> -	myaddr.sin6_addr = sa->sin6_addr;
> -	do {
> -		myaddr.sin6_port = htons(port);
> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> -						sizeof(myaddr));
> -		if (port == 0)
> -			break;
> -		if (err == 0) {
> -			transport->srcport = port;
> -			break;
> -		}
> -		last = port;
> -		port = xs_next_srcport(transport, port);
> -		if (port > last)
> -			nloop++;
> -	} while (err == -EADDRINUSE && nloop != 2);
> -	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
> -		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
> +	if (myaddr.sa_family == PF_INET)
> +		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
> +				&((struct sockaddr_in *)&myaddr)->sin_addr,
> +				port, err ? "failed" : "ok", err);
> +	else
> +		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
> +				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
> +				port, err ? "failed" : "ok", err);
> 	return err;
> }
> 
> +
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> static struct lock_class_key xs_key[2];
> static struct lock_class_key xs_slock_key[2];
> @@ -1645,7 +1616,7 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
> 	}
> 	xs_reclassify_socket4(sock);
> 
> -	if (xs_bind4(transport, sock)) {
> +	if (xs_bind(transport, sock)) {
> 		sock_release(sock);
> 		goto out;
> 	}
> @@ -1669,7 +1640,7 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
> 	}
> 	xs_reclassify_socket6(sock);
> 
> -	if (xs_bind6(transport, sock)) {
> +	if (xs_bind(transport, sock)) {
> 		sock_release(sock);
> 		goto out;
> 	}
> -- 
> 1.5.5.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 9/13] sunrpc: Merge the xs_bind code

[Pavel Emelyanov]

>> -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
>> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
>> {
>> -	struct sockaddr_in myaddr = {
>> -		.sin_family = AF_INET,
>> -	};
>> -	struct sockaddr_in *sa;
>> +	struct sockaddr myaddr;
> 
> You want "struct sockaddr_storage" here.  A "struct sockaddr" is required by
> standard (iirc) to be only as large as a "struct sockaddr_in".  So this can't
> possibly hold an IPv6 socket address without overflowing.

OK

>> 	int err, nloop = 0;
>> 	unsigned short port = xs_get_srcport(transport);
>> 	unsigned short last;
>>
>> -	sa = (struct sockaddr_in *)&transport->srcaddr;
>> -	myaddr.sin_addr = sa->sin_addr;
>> +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
>> 	do {
>> -		myaddr.sin_port = htons(port);
>> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
>> -						sizeof(myaddr));
>> +		BUILD_BUG_ON(offsetof(struct sockaddr_in, sin_port) !=
>> +				offsetof(struct sockaddr_in6, sin6_port));
>> +		((struct sockaddr_in *)&myaddr)->sin_port = htons(port);
> 
> This series of patches looks mostly OK at first blush, but this little 
> change is a bit offensive.  :-)
> 
> There should be some static inline helper functions that can get and set the
> port number in a socket address without a lot of magic, and I would prefer you
> use those instead.  (I would point you directly to them, but I can't easily
> access kernel source at the moment -- hotel internet sucks).
> 
> There should be one or two code examples in the RPC module or in headers that
> set and get ports in an automatic struct sockaddr_storage variable to give you
> an idea of what this needs to look like to avoid stack overruns and bad 
> pointer aliasing.

I believe you mean the rpc_set_port() one. Will do!

Thanks,
Pavel

[PATCH v2 9/13] sunrpc: Merge the xs_bind code

[Pavel Emelyanov]

There's the only difference betseen the xs_bind4 and the
xs_bind6 - the size of sockaddr structure they use.

Fortunatelly its size can be indirectly get from the transport.

Change since v1:
* use sockaddr_storage instead of sockaddr
* use rpc_set_port instead of manual port assigning

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |   64 +++++++++++++------------------------------------
 1 files changed, 17 insertions(+), 47 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 7fdf2bb..bab808f 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
 		return xprt_max_resvport;
 	return --port;
 }
-
-static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
+static int xs_bind(struct sock_xprt *transport, struct socket *sock)
 {
-	struct sockaddr_in myaddr = {
-		.sin_family = AF_INET,
-	};
-	struct sockaddr_in *sa;
+	struct sockaddr_storage myaddr;
 	int err, nloop = 0;
 	unsigned short port = xs_get_srcport(transport);
 	unsigned short last;
 
-	sa = (struct sockaddr_in *)&transport->srcaddr;
-	myaddr.sin_addr = sa->sin_addr;
+	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
 	do {
-		myaddr.sin_port = htons(port);
-		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
-						sizeof(myaddr));
+		rpc_set_port((struct sockaddr *)&myaddr, port);
+		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
+				transport->xprt.addrlen);
 		if (port == 0)
 			break;
 		if (err == 0) {
@@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
 		if (port > last)
 			nloop++;
 	} while (err == -EADDRINUSE && nloop != 2);
-	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
-			__func__, &myaddr.sin_addr,
-			port, err ? "failed" : "ok", err);
-	return err;
-}
-
-static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
-{
-	struct sockaddr_in6 myaddr = {
-		.sin6_family = AF_INET6,
-	};
-	struct sockaddr_in6 *sa;
-	int err, nloop = 0;
-	unsigned short port = xs_get_srcport(transport);
-	unsigned short last;
 
-	sa = (struct sockaddr_in6 *)&transport->srcaddr;
-	myaddr.sin6_addr = sa->sin6_addr;
-	do {
-		myaddr.sin6_port = htons(port);
-		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
-						sizeof(myaddr));
-		if (port == 0)
-			break;
-		if (err == 0) {
-			transport->srcport = port;
-			break;
-		}
-		last = port;
-		port = xs_next_srcport(transport, port);
-		if (port > last)
-			nloop++;
-	} while (err == -EADDRINUSE && nloop != 2);
-	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
-		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
+	if (myaddr.ss_family == PF_INET)
+		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
+				&((struct sockaddr_in *)&myaddr)->sin_addr,
+				port, err ? "failed" : "ok", err);
+	else
+		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
+				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
+				port, err ? "failed" : "ok", err);
 	return err;
 }
 
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 static struct lock_class_key xs_key[2];
 static struct lock_class_key xs_slock_key[2];
@@ -1645,7 +1615,7 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
 	}
 	xs_reclassify_socket4(sock);
 
-	if (xs_bind4(transport, sock)) {
+	if (xs_bind(transport, sock)) {
 		sock_release(sock);
 		goto out;
 	}
@@ -1669,7 +1639,7 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
 	}
 	xs_reclassify_socket6(sock);
 
-	if (xs_bind6(transport, sock)) {
+	if (xs_bind(transport, sock)) {
 		sock_release(sock);
 		goto out;
 	}
-- 
1.5.5.6

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[Chuck Lever]

On Oct 5, 2010, at 7:53 AM, Pavel Emelyanov wrote:

> There's the only difference betseen the xs_bind4 and the
> xs_bind6 - the size of sockaddr structure they use.
> 
> Fortunatelly its size can be indirectly get from the transport.
> 
> Change since v1:
> * use sockaddr_storage instead of sockaddr
> * use rpc_set_port instead of manual port assigning
> 
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> ---
> net/sunrpc/xprtsock.c |   64 +++++++++++++------------------------------------
> 1 files changed, 17 insertions(+), 47 deletions(-)
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 7fdf2bb..bab808f 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
> 		return xprt_max_resvport;
> 	return --port;
> }
> -
> -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
> {
> -	struct sockaddr_in myaddr = {
> -		.sin_family = AF_INET,
> -	};
> -	struct sockaddr_in *sa;
> +	struct sockaddr_storage myaddr;
> 	int err, nloop = 0;
> 	unsigned short port = xs_get_srcport(transport);
> 	unsigned short last;
> 
> -	sa = (struct sockaddr_in *)&transport->srcaddr;
> -	myaddr.sin_addr = sa->sin_addr;
> +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
> 	do {
> -		myaddr.sin_port = htons(port);
> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> -						sizeof(myaddr));
> +		rpc_set_port((struct sockaddr *)&myaddr, port);
> +		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
> +				transport->xprt.addrlen);

That looks reasonable, and probably even improves your ratio of deletions to additions.

For the series,

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

> 		if (port == 0)
> 			break;
> 		if (err == 0) {
> @@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> 		if (port > last)
> 			nloop++;
> 	} while (err == -EADDRINUSE && nloop != 2);
> -	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
> -			__func__, &myaddr.sin_addr,
> -			port, err ? "failed" : "ok", err);
> -	return err;
> -}
> -
> -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
> -{
> -	struct sockaddr_in6 myaddr = {
> -		.sin6_family = AF_INET6,
> -	};
> -	struct sockaddr_in6 *sa;
> -	int err, nloop = 0;
> -	unsigned short port = xs_get_srcport(transport);
> -	unsigned short last;
> 
> -	sa = (struct sockaddr_in6 *)&transport->srcaddr;
> -	myaddr.sin6_addr = sa->sin6_addr;
> -	do {
> -		myaddr.sin6_port = htons(port);
> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> -						sizeof(myaddr));
> -		if (port == 0)
> -			break;
> -		if (err == 0) {
> -			transport->srcport = port;
> -			break;
> -		}
> -		last = port;
> -		port = xs_next_srcport(transport, port);
> -		if (port > last)
> -			nloop++;
> -	} while (err == -EADDRINUSE && nloop != 2);
> -	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
> -		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
> +	if (myaddr.ss_family == PF_INET)
> +		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
> +				&((struct sockaddr_in *)&myaddr)->sin_addr,
> +				port, err ? "failed" : "ok", err);
> +	else
> +		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
> +				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
> +				port, err ? "failed" : "ok", err);
> 	return err;
> }
> 
> +
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> static struct lock_class_key xs_key[2];
> static struct lock_class_key xs_slock_key[2];
> @@ -1645,7 +1615,7 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
> 	}
> 	xs_reclassify_socket4(sock);
> 
> -	if (xs_bind4(transport, sock)) {
> +	if (xs_bind(transport, sock)) {
> 		sock_release(sock);
> 		goto out;
> 	}
> @@ -1669,7 +1639,7 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
> 	}
> 	xs_reclassify_socket6(sock);
> 
> -	if (xs_bind6(transport, sock)) {
> +	if (xs_bind(transport, sock)) {
> 		sock_release(sock);
> 		goto out;
> 	}
> -- 
> 1.5.5.6
> 

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

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[J. Bruce Fields]

On Tue, Oct 05, 2010 at 03:53:08PM +0400, Pavel Emelyanov wrote:
> There's the only difference betseen the xs_bind4 and the
> xs_bind6 - the size of sockaddr structure they use.
> 
> Fortunatelly its size can be indirectly get from the transport.
> 
> Change since v1:
> * use sockaddr_storage instead of sockaddr
> * use rpc_set_port instead of manual port assigning

Whoops, dropping this; it breaks nfsd startup.  I haven't figured out
why yet, but I get

RPC: server localhost requires stronger authentication.
svc: failed to register nfsdv2 RPC service (errno 13).

--b.

> 
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> ---
>  net/sunrpc/xprtsock.c |   64 +++++++++++++------------------------------------
>  1 files changed, 17 insertions(+), 47 deletions(-)
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 7fdf2bb..bab808f 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
>  		return xprt_max_resvport;
>  	return --port;
>  }
> -
> -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
>  {
> -	struct sockaddr_in myaddr = {
> -		.sin_family = AF_INET,
> -	};
> -	struct sockaddr_in *sa;
> +	struct sockaddr_storage myaddr;
>  	int err, nloop = 0;
>  	unsigned short port = xs_get_srcport(transport);
>  	unsigned short last;
>  
> -	sa = (struct sockaddr_in *)&transport->srcaddr;
> -	myaddr.sin_addr = sa->sin_addr;
> +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
>  	do {
> -		myaddr.sin_port = htons(port);
> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> -						sizeof(myaddr));
> +		rpc_set_port((struct sockaddr *)&myaddr, port);
> +		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
> +				transport->xprt.addrlen);
>  		if (port == 0)
>  			break;
>  		if (err == 0) {
> @@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
>  		if (port > last)
>  			nloop++;
>  	} while (err == -EADDRINUSE && nloop != 2);
> -	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
> -			__func__, &myaddr.sin_addr,
> -			port, err ? "failed" : "ok", err);
> -	return err;
> -}
> -
> -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
> -{
> -	struct sockaddr_in6 myaddr = {
> -		.sin6_family = AF_INET6,
> -	};
> -	struct sockaddr_in6 *sa;
> -	int err, nloop = 0;
> -	unsigned short port = xs_get_srcport(transport);
> -	unsigned short last;
>  
> -	sa = (struct sockaddr_in6 *)&transport->srcaddr;
> -	myaddr.sin6_addr = sa->sin6_addr;
> -	do {
> -		myaddr.sin6_port = htons(port);
> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> -						sizeof(myaddr));
> -		if (port == 0)
> -			break;
> -		if (err == 0) {
> -			transport->srcport = port;
> -			break;
> -		}
> -		last = port;
> -		port = xs_next_srcport(transport, port);
> -		if (port > last)
> -			nloop++;
> -	} while (err == -EADDRINUSE && nloop != 2);
> -	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
> -		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
> +	if (myaddr.ss_family == PF_INET)
> +		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
> +				&((struct sockaddr_in *)&myaddr)->sin_addr,
> +				port, err ? "failed" : "ok", err);
> +	else
> +		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
> +				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
> +				port, err ? "failed" : "ok", err);
>  	return err;
>  }
>  
> +
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>  static struct lock_class_key xs_key[2];
>  static struct lock_class_key xs_slock_key[2];
> @@ -1645,7 +1615,7 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
>  	}
>  	xs_reclassify_socket4(sock);
>  
> -	if (xs_bind4(transport, sock)) {
> +	if (xs_bind(transport, sock)) {
>  		sock_release(sock);
>  		goto out;
>  	}
> @@ -1669,7 +1639,7 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
>  	}
>  	xs_reclassify_socket6(sock);
>  
> -	if (xs_bind6(transport, sock)) {
> +	if (xs_bind(transport, sock)) {
>  		sock_release(sock);
>  		goto out;
>  	}
> -- 
> 1.5.5.6
> 

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[Pavel Emelyanov]

On 10/15/2010 08:05 PM, J. Bruce Fields wrote:
> On Tue, Oct 05, 2010 at 03:53:08PM +0400, Pavel Emelyanov wrote:
>> There's the only difference betseen the xs_bind4 and the
>> xs_bind6 - the size of sockaddr structure they use.
>>
>> Fortunatelly its size can be indirectly get from the transport.
>>
>> Change since v1:
>> * use sockaddr_storage instead of sockaddr
>> * use rpc_set_port instead of manual port assigning
> 
> Whoops, dropping this; it breaks nfsd startup.  I haven't figured out
> why yet, but I get
> 
> RPC: server localhost requires stronger authentication.
> svc: failed to register nfsdv2 RPC service (errno 13).

Hm... Will take a look, I haven't seen that at my tests :(

> --b.
> 
>>
>> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>> ---
>>  net/sunrpc/xprtsock.c |   64 +++++++++++++------------------------------------
>>  1 files changed, 17 insertions(+), 47 deletions(-)
>>
>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>> index 7fdf2bb..bab808f 100644
>> --- a/net/sunrpc/xprtsock.c
>> +++ b/net/sunrpc/xprtsock.c
>> @@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
>>  		return xprt_max_resvport;
>>  	return --port;
>>  }
>> -
>> -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
>> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
>>  {
>> -	struct sockaddr_in myaddr = {
>> -		.sin_family = AF_INET,
>> -	};
>> -	struct sockaddr_in *sa;
>> +	struct sockaddr_storage myaddr;
>>  	int err, nloop = 0;
>>  	unsigned short port = xs_get_srcport(transport);
>>  	unsigned short last;
>>  
>> -	sa = (struct sockaddr_in *)&transport->srcaddr;
>> -	myaddr.sin_addr = sa->sin_addr;
>> +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
>>  	do {
>> -		myaddr.sin_port = htons(port);
>> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
>> -						sizeof(myaddr));
>> +		rpc_set_port((struct sockaddr *)&myaddr, port);
>> +		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
>> +				transport->xprt.addrlen);
>>  		if (port == 0)
>>  			break;
>>  		if (err == 0) {
>> @@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
>>  		if (port > last)
>>  			nloop++;
>>  	} while (err == -EADDRINUSE && nloop != 2);
>> -	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
>> -			__func__, &myaddr.sin_addr,
>> -			port, err ? "failed" : "ok", err);
>> -	return err;
>> -}
>> -
>> -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
>> -{
>> -	struct sockaddr_in6 myaddr = {
>> -		.sin6_family = AF_INET6,
>> -	};
>> -	struct sockaddr_in6 *sa;
>> -	int err, nloop = 0;
>> -	unsigned short port = xs_get_srcport(transport);
>> -	unsigned short last;
>>  
>> -	sa = (struct sockaddr_in6 *)&transport->srcaddr;
>> -	myaddr.sin6_addr = sa->sin6_addr;
>> -	do {
>> -		myaddr.sin6_port = htons(port);
>> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
>> -						sizeof(myaddr));
>> -		if (port == 0)
>> -			break;
>> -		if (err == 0) {
>> -			transport->srcport = port;
>> -			break;
>> -		}
>> -		last = port;
>> -		port = xs_next_srcport(transport, port);
>> -		if (port > last)
>> -			nloop++;
>> -	} while (err == -EADDRINUSE && nloop != 2);
>> -	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
>> -		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
>> +	if (myaddr.ss_family == PF_INET)
>> +		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
>> +				&((struct sockaddr_in *)&myaddr)->sin_addr,
>> +				port, err ? "failed" : "ok", err);
>> +	else
>> +		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
>> +				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
>> +				port, err ? "failed" : "ok", err);
>>  	return err;
>>  }
>>  
>> +
>>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>  static struct lock_class_key xs_key[2];
>>  static struct lock_class_key xs_slock_key[2];
>> @@ -1645,7 +1615,7 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
>>  	}
>>  	xs_reclassify_socket4(sock);
>>  
>> -	if (xs_bind4(transport, sock)) {
>> +	if (xs_bind(transport, sock)) {
>>  		sock_release(sock);
>>  		goto out;
>>  	}
>> @@ -1669,7 +1639,7 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
>>  	}
>>  	xs_reclassify_socket6(sock);
>>  
>> -	if (xs_bind6(transport, sock)) {
>> +	if (xs_bind(transport, sock)) {
>>  		sock_release(sock);
>>  		goto out;
>>  	}
>> -- 
>> 1.5.5.6
>>
> .
> 

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[Chuck Lever]

On Oct 15, 2010, at 12:05 PM, J. Bruce Fields wrote:

> On Tue, Oct 05, 2010 at 03:53:08PM +0400, Pavel Emelyanov wrote:
>> There's the only difference betseen the xs_bind4 and the
>> xs_bind6 - the size of sockaddr structure they use.
>> 
>> Fortunatelly its size can be indirectly get from the transport.
>> 
>> Change since v1:
>> * use sockaddr_storage instead of sockaddr
>> * use rpc_set_port instead of manual port assigning
> 
> Whoops, dropping this; it breaks nfsd startup.  I haven't figured out
> why yet, but I get
> 
> RPC: server localhost requires stronger authentication.
> svc: failed to register nfsdv2 RPC service (errno 13).

Capturing a network trace of lo during server initialization should reveal all.  Compare a trace from a working run and a non-working one.

> 
> --b.
> 
>> 
>> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>> ---
>> net/sunrpc/xprtsock.c |   64 +++++++++++++------------------------------------
>> 1 files changed, 17 insertions(+), 47 deletions(-)
>> 
>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>> index 7fdf2bb..bab808f 100644
>> --- a/net/sunrpc/xprtsock.c
>> +++ b/net/sunrpc/xprtsock.c
>> @@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
>> 		return xprt_max_resvport;
>> 	return --port;
>> }
>> -
>> -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
>> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
>> {
>> -	struct sockaddr_in myaddr = {
>> -		.sin_family = AF_INET,
>> -	};
>> -	struct sockaddr_in *sa;
>> +	struct sockaddr_storage myaddr;
>> 	int err, nloop = 0;
>> 	unsigned short port = xs_get_srcport(transport);
>> 	unsigned short last;
>> 
>> -	sa = (struct sockaddr_in *)&transport->srcaddr;
>> -	myaddr.sin_addr = sa->sin_addr;
>> +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
>> 	do {
>> -		myaddr.sin_port = htons(port);
>> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
>> -						sizeof(myaddr));
>> +		rpc_set_port((struct sockaddr *)&myaddr, port);
>> +		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
>> +				transport->xprt.addrlen);
>> 		if (port == 0)
>> 			break;
>> 		if (err == 0) {
>> @@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
>> 		if (port > last)
>> 			nloop++;
>> 	} while (err == -EADDRINUSE && nloop != 2);
>> -	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
>> -			__func__, &myaddr.sin_addr,
>> -			port, err ? "failed" : "ok", err);
>> -	return err;
>> -}
>> -
>> -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
>> -{
>> -	struct sockaddr_in6 myaddr = {
>> -		.sin6_family = AF_INET6,
>> -	};
>> -	struct sockaddr_in6 *sa;
>> -	int err, nloop = 0;
>> -	unsigned short port = xs_get_srcport(transport);
>> -	unsigned short last;
>> 
>> -	sa = (struct sockaddr_in6 *)&transport->srcaddr;
>> -	myaddr.sin6_addr = sa->sin6_addr;
>> -	do {
>> -		myaddr.sin6_port = htons(port);
>> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
>> -						sizeof(myaddr));
>> -		if (port == 0)
>> -			break;
>> -		if (err == 0) {
>> -			transport->srcport = port;
>> -			break;
>> -		}
>> -		last = port;
>> -		port = xs_next_srcport(transport, port);
>> -		if (port > last)
>> -			nloop++;
>> -	} while (err == -EADDRINUSE && nloop != 2);
>> -	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
>> -		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
>> +	if (myaddr.ss_family == PF_INET)
>> +		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
>> +				&((struct sockaddr_in *)&myaddr)->sin_addr,
>> +				port, err ? "failed" : "ok", err);
>> +	else
>> +		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
>> +				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
>> +				port, err ? "failed" : "ok", err);
>> 	return err;
>> }
>> 
>> +
>> #ifdef CONFIG_DEBUG_LOCK_ALLOC
>> static struct lock_class_key xs_key[2];
>> static struct lock_class_key xs_slock_key[2];
>> @@ -1645,7 +1615,7 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
>> 	}
>> 	xs_reclassify_socket4(sock);
>> 
>> -	if (xs_bind4(transport, sock)) {
>> +	if (xs_bind(transport, sock)) {
>> 		sock_release(sock);
>> 		goto out;
>> 	}
>> @@ -1669,7 +1639,7 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
>> 	}
>> 	xs_reclassify_socket6(sock);
>> 
>> -	if (xs_bind6(transport, sock)) {
>> +	if (xs_bind(transport, sock)) {
>> 		sock_release(sock);
>> 		goto out;
>> 	}
>> -- 
>> 1.5.5.6
>> 

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

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[J. Bruce Fields]

On Fri, Oct 15, 2010 at 12:39:36PM -0400, Chuck Lever wrote:
> 
> On Oct 15, 2010, at 12:05 PM, J. Bruce Fields wrote:
> 
> > On Tue, Oct 05, 2010 at 03:53:08PM +0400, Pavel Emelyanov wrote:
> >> There's the only difference betseen the xs_bind4 and the
> >> xs_bind6 - the size of sockaddr structure they use.
> >> 
> >> Fortunatelly its size can be indirectly get from the transport.
> >> 
> >> Change since v1:
> >> * use sockaddr_storage instead of sockaddr
> >> * use rpc_set_port instead of manual port assigning
> > 
> > Whoops, dropping this; it breaks nfsd startup.  I haven't figured out
> > why yet, but I get
> > 
> > RPC: server localhost requires stronger authentication.
> > svc: failed to register nfsdv2 RPC service (errno 13).
> 
> Capturing a network trace of lo during server initialization should reveal all.  Compare a trace from a working run and a non-working one.

Hm.  One difference is the source port of the portmap calls: 33471 in
the bad case, 1016 in the bad.--b.

> 
> > 
> > --b.
> > 
> >> 
> >> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> >> ---
> >> net/sunrpc/xprtsock.c |   64 +++++++++++++------------------------------------
> >> 1 files changed, 17 insertions(+), 47 deletions(-)
> >> 
> >> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> >> index 7fdf2bb..bab808f 100644
> >> --- a/net/sunrpc/xprtsock.c
> >> +++ b/net/sunrpc/xprtsock.c
> >> @@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
> >> 		return xprt_max_resvport;
> >> 	return --port;
> >> }
> >> -
> >> -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> >> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
> >> {
> >> -	struct sockaddr_in myaddr = {
> >> -		.sin_family = AF_INET,
> >> -	};
> >> -	struct sockaddr_in *sa;
> >> +	struct sockaddr_storage myaddr;
> >> 	int err, nloop = 0;
> >> 	unsigned short port = xs_get_srcport(transport);
> >> 	unsigned short last;
> >> 
> >> -	sa = (struct sockaddr_in *)&transport->srcaddr;
> >> -	myaddr.sin_addr = sa->sin_addr;
> >> +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
> >> 	do {
> >> -		myaddr.sin_port = htons(port);
> >> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> >> -						sizeof(myaddr));
> >> +		rpc_set_port((struct sockaddr *)&myaddr, port);
> >> +		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
> >> +				transport->xprt.addrlen);
> >> 		if (port == 0)
> >> 			break;
> >> 		if (err == 0) {
> >> @@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> >> 		if (port > last)
> >> 			nloop++;
> >> 	} while (err == -EADDRINUSE && nloop != 2);
> >> -	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
> >> -			__func__, &myaddr.sin_addr,
> >> -			port, err ? "failed" : "ok", err);
> >> -	return err;
> >> -}
> >> -
> >> -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
> >> -{
> >> -	struct sockaddr_in6 myaddr = {
> >> -		.sin6_family = AF_INET6,
> >> -	};
> >> -	struct sockaddr_in6 *sa;
> >> -	int err, nloop = 0;
> >> -	unsigned short port = xs_get_srcport(transport);
> >> -	unsigned short last;
> >> 
> >> -	sa = (struct sockaddr_in6 *)&transport->srcaddr;
> >> -	myaddr.sin6_addr = sa->sin6_addr;
> >> -	do {
> >> -		myaddr.sin6_port = htons(port);
> >> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> >> -						sizeof(myaddr));
> >> -		if (port == 0)
> >> -			break;
> >> -		if (err == 0) {
> >> -			transport->srcport = port;
> >> -			break;
> >> -		}
> >> -		last = port;
> >> -		port = xs_next_srcport(transport, port);
> >> -		if (port > last)
> >> -			nloop++;
> >> -	} while (err == -EADDRINUSE && nloop != 2);
> >> -	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
> >> -		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
> >> +	if (myaddr.ss_family == PF_INET)
> >> +		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
> >> +				&((struct sockaddr_in *)&myaddr)->sin_addr,
> >> +				port, err ? "failed" : "ok", err);
> >> +	else
> >> +		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
> >> +				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
> >> +				port, err ? "failed" : "ok", err);
> >> 	return err;
> >> }
> >> 
> >> +
> >> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >> static struct lock_class_key xs_key[2];
> >> static struct lock_class_key xs_slock_key[2];
> >> @@ -1645,7 +1615,7 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
> >> 	}
> >> 	xs_reclassify_socket4(sock);
> >> 
> >> -	if (xs_bind4(transport, sock)) {
> >> +	if (xs_bind(transport, sock)) {
> >> 		sock_release(sock);
> >> 		goto out;
> >> 	}
> >> @@ -1669,7 +1639,7 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
> >> 	}
> >> 	xs_reclassify_socket6(sock);
> >> 
> >> -	if (xs_bind6(transport, sock)) {
> >> +	if (xs_bind(transport, sock)) {
> >> 		sock_release(sock);
> >> 		goto out;
> >> 	}
> >> -- 
> >> 1.5.5.6
> >> 
> 
> -- 
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
> 

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[J. Bruce Fields]

On Fri, Oct 15, 2010 at 02:08:52PM -0400, J. Bruce Fields wrote:
> On Fri, Oct 15, 2010 at 12:39:36PM -0400, Chuck Lever wrote:
> > 
> > On Oct 15, 2010, at 12:05 PM, J. Bruce Fields wrote:
> > 
> > > On Tue, Oct 05, 2010 at 03:53:08PM +0400, Pavel Emelyanov wrote:
> > >> There's the only difference betseen the xs_bind4 and the
> > >> xs_bind6 - the size of sockaddr structure they use.
> > >> 
> > >> Fortunatelly its size can be indirectly get from the transport.
> > >> 
> > >> Change since v1:
> > >> * use sockaddr_storage instead of sockaddr
> > >> * use rpc_set_port instead of manual port assigning
> > > 
> > > Whoops, dropping this; it breaks nfsd startup.  I haven't figured out
> > > why yet, but I get
> > > 
> > > RPC: server localhost requires stronger authentication.
> > > svc: failed to register nfsdv2 RPC service (errno 13).
> > 
> > Capturing a network trace of lo during server initialization should reveal all.  Compare a trace from a working run and a non-working one.
> 
> Hm.  One difference is the source port of the portmap calls: 33471 in
> the bad case, 1016 in the bad.--b.
			    ^^^
			    (I meant "good", obviously!)
			    --b.


> 
> > 
> > > 
> > > --b.
> > > 
> > >> 
> > >> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> > >> ---
> > >> net/sunrpc/xprtsock.c |   64 +++++++++++++------------------------------------
> > >> 1 files changed, 17 insertions(+), 47 deletions(-)
> > >> 
> > >> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > >> index 7fdf2bb..bab808f 100644
> > >> --- a/net/sunrpc/xprtsock.c
> > >> +++ b/net/sunrpc/xprtsock.c
> > >> @@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
> > >> 		return xprt_max_resvport;
> > >> 	return --port;
> > >> }
> > >> -
> > >> -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> > >> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
> > >> {
> > >> -	struct sockaddr_in myaddr = {
> > >> -		.sin_family = AF_INET,
> > >> -	};
> > >> -	struct sockaddr_in *sa;
> > >> +	struct sockaddr_storage myaddr;
> > >> 	int err, nloop = 0;
> > >> 	unsigned short port = xs_get_srcport(transport);
> > >> 	unsigned short last;
> > >> 
> > >> -	sa = (struct sockaddr_in *)&transport->srcaddr;
> > >> -	myaddr.sin_addr = sa->sin_addr;
> > >> +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
> > >> 	do {
> > >> -		myaddr.sin_port = htons(port);
> > >> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> > >> -						sizeof(myaddr));
> > >> +		rpc_set_port((struct sockaddr *)&myaddr, port);
> > >> +		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
> > >> +				transport->xprt.addrlen);
> > >> 		if (port == 0)
> > >> 			break;
> > >> 		if (err == 0) {
> > >> @@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> > >> 		if (port > last)
> > >> 			nloop++;
> > >> 	} while (err == -EADDRINUSE && nloop != 2);
> > >> -	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
> > >> -			__func__, &myaddr.sin_addr,
> > >> -			port, err ? "failed" : "ok", err);
> > >> -	return err;
> > >> -}
> > >> -
> > >> -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
> > >> -{
> > >> -	struct sockaddr_in6 myaddr = {
> > >> -		.sin6_family = AF_INET6,
> > >> -	};
> > >> -	struct sockaddr_in6 *sa;
> > >> -	int err, nloop = 0;
> > >> -	unsigned short port = xs_get_srcport(transport);
> > >> -	unsigned short last;
> > >> 
> > >> -	sa = (struct sockaddr_in6 *)&transport->srcaddr;
> > >> -	myaddr.sin6_addr = sa->sin6_addr;
> > >> -	do {
> > >> -		myaddr.sin6_port = htons(port);
> > >> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> > >> -						sizeof(myaddr));
> > >> -		if (port == 0)
> > >> -			break;
> > >> -		if (err == 0) {
> > >> -			transport->srcport = port;
> > >> -			break;
> > >> -		}
> > >> -		last = port;
> > >> -		port = xs_next_srcport(transport, port);
> > >> -		if (port > last)
> > >> -			nloop++;
> > >> -	} while (err == -EADDRINUSE && nloop != 2);
> > >> -	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
> > >> -		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
> > >> +	if (myaddr.ss_family == PF_INET)
> > >> +		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
> > >> +				&((struct sockaddr_in *)&myaddr)->sin_addr,
> > >> +				port, err ? "failed" : "ok", err);
> > >> +	else
> > >> +		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
> > >> +				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
> > >> +				port, err ? "failed" : "ok", err);
> > >> 	return err;
> > >> }
> > >> 
> > >> +
> > >> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > >> static struct lock_class_key xs_key[2];
> > >> static struct lock_class_key xs_slock_key[2];
> > >> @@ -1645,7 +1615,7 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
> > >> 	}
> > >> 	xs_reclassify_socket4(sock);
> > >> 
> > >> -	if (xs_bind4(transport, sock)) {
> > >> +	if (xs_bind(transport, sock)) {
> > >> 		sock_release(sock);
> > >> 		goto out;
> > >> 	}
> > >> @@ -1669,7 +1639,7 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
> > >> 	}
> > >> 	xs_reclassify_socket6(sock);
> > >> 
> > >> -	if (xs_bind6(transport, sock)) {
> > >> +	if (xs_bind(transport, sock)) {
> > >> 		sock_release(sock);
> > >> 		goto out;
> > >> 	}
> > >> -- 
> > >> 1.5.5.6
> > >> 
> > 
> > -- 
> > chuck[dot]lever[at]oracle[dot]com
> > 
> > 
> > 
> > 

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[Chuck Lever]

On Oct 15, 2010, at 2:08 PM, J. Bruce Fields wrote:

> On Fri, Oct 15, 2010 at 12:39:36PM -0400, Chuck Lever wrote:
>> 
>> On Oct 15, 2010, at 12:05 PM, J. Bruce Fields wrote:
>> 
>>> On Tue, Oct 05, 2010 at 03:53:08PM +0400, Pavel Emelyanov wrote:
>>>> There's the only difference betseen the xs_bind4 and the
>>>> xs_bind6 - the size of sockaddr structure they use.
>>>> 
>>>> Fortunatelly its size can be indirectly get from the transport.
>>>> 
>>>> Change since v1:
>>>> * use sockaddr_storage instead of sockaddr
>>>> * use rpc_set_port instead of manual port assigning
>>> 
>>> Whoops, dropping this; it breaks nfsd startup.  I haven't figured out
>>> why yet, but I get
>>> 
>>> RPC: server localhost requires stronger authentication.
>>> svc: failed to register nfsdv2 RPC service (errno 13).
>> 
>> Capturing a network trace of lo during server initialization should reveal all.  Compare a trace from a working run and a non-working one.
> 
> Hm.  One difference is the source port of the portmap calls: 33471 in
> the bad case, 1016 in the bad.--b.

1016 in the good... yes, that's because the server's registration upcall needs a privileged port, and xs_bind is not obliging here.  That certainly would result in a "requires stronger authentication" error from rpcbind.

> 
>> 
>>> 
>>> --b.
>>> 
>>>> 
>>>> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>>>> ---
>>>> net/sunrpc/xprtsock.c |   64 +++++++++++++------------------------------------
>>>> 1 files changed, 17 insertions(+), 47 deletions(-)
>>>> 
>>>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>>>> index 7fdf2bb..bab808f 100644
>>>> --- a/net/sunrpc/xprtsock.c
>>>> +++ b/net/sunrpc/xprtsock.c
>>>> @@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
>>>> 		return xprt_max_resvport;
>>>> 	return --port;
>>>> }
>>>> -
>>>> -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
>>>> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
>>>> {
>>>> -	struct sockaddr_in myaddr = {
>>>> -		.sin_family = AF_INET,
>>>> -	};
>>>> -	struct sockaddr_in *sa;
>>>> +	struct sockaddr_storage myaddr;
>>>> 	int err, nloop = 0;
>>>> 	unsigned short port = xs_get_srcport(transport);
>>>> 	unsigned short last;
>>>> 
>>>> -	sa = (struct sockaddr_in *)&transport->srcaddr;
>>>> -	myaddr.sin_addr = sa->sin_addr;
>>>> +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
>>>> 	do {
>>>> -		myaddr.sin_port = htons(port);
>>>> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
>>>> -						sizeof(myaddr));
>>>> +		rpc_set_port((struct sockaddr *)&myaddr, port);
>>>> +		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
>>>> +				transport->xprt.addrlen);
>>>> 		if (port == 0)
>>>> 			break;
>>>> 		if (err == 0) {
>>>> @@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
>>>> 		if (port > last)
>>>> 			nloop++;
>>>> 	} while (err == -EADDRINUSE && nloop != 2);
>>>> -	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
>>>> -			__func__, &myaddr.sin_addr,
>>>> -			port, err ? "failed" : "ok", err);
>>>> -	return err;
>>>> -}
>>>> -
>>>> -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
>>>> -{
>>>> -	struct sockaddr_in6 myaddr = {
>>>> -		.sin6_family = AF_INET6,
>>>> -	};
>>>> -	struct sockaddr_in6 *sa;
>>>> -	int err, nloop = 0;
>>>> -	unsigned short port = xs_get_srcport(transport);
>>>> -	unsigned short last;
>>>> 
>>>> -	sa = (struct sockaddr_in6 *)&transport->srcaddr;
>>>> -	myaddr.sin6_addr = sa->sin6_addr;
>>>> -	do {
>>>> -		myaddr.sin6_port = htons(port);
>>>> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
>>>> -						sizeof(myaddr));
>>>> -		if (port == 0)
>>>> -			break;
>>>> -		if (err == 0) {
>>>> -			transport->srcport = port;
>>>> -			break;
>>>> -		}
>>>> -		last = port;
>>>> -		port = xs_next_srcport(transport, port);
>>>> -		if (port > last)
>>>> -			nloop++;
>>>> -	} while (err == -EADDRINUSE && nloop != 2);
>>>> -	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
>>>> -		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
>>>> +	if (myaddr.ss_family == PF_INET)
>>>> +		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
>>>> +				&((struct sockaddr_in *)&myaddr)->sin_addr,
>>>> +				port, err ? "failed" : "ok", err);
>>>> +	else
>>>> +		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
>>>> +				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
>>>> +				port, err ? "failed" : "ok", err);
>>>> 	return err;
>>>> }
>>>> 
>>>> +
>>>> #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>>> static struct lock_class_key xs_key[2];
>>>> static struct lock_class_key xs_slock_key[2];
>>>> @@ -1645,7 +1615,7 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
>>>> 	}
>>>> 	xs_reclassify_socket4(sock);
>>>> 
>>>> -	if (xs_bind4(transport, sock)) {
>>>> +	if (xs_bind(transport, sock)) {
>>>> 		sock_release(sock);
>>>> 		goto out;
>>>> 	}
>>>> @@ -1669,7 +1639,7 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
>>>> 	}
>>>> 	xs_reclassify_socket6(sock);
>>>> 
>>>> -	if (xs_bind6(transport, sock)) {
>>>> +	if (xs_bind(transport, sock)) {
>>>> 		sock_release(sock);
>>>> 		goto out;
>>>> 	}
>>>> -- 
>>>> 1.5.5.6
>>>> 
>> 
>> -- 
>> chuck[dot]lever[at]oracle[dot]com
>> 
>> 
>> 
>> 

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

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[J. Bruce Fields]

On Fri, Oct 15, 2010 at 02:11:29PM -0400, Chuck Lever wrote:
> 
> On Oct 15, 2010, at 2:08 PM, J. Bruce Fields wrote:
> 
> > On Fri, Oct 15, 2010 at 12:39:36PM -0400, Chuck Lever wrote:
> >> 
> >> On Oct 15, 2010, at 12:05 PM, J. Bruce Fields wrote:
> >> 
> >>> On Tue, Oct 05, 2010 at 03:53:08PM +0400, Pavel Emelyanov wrote:
> >>>> There's the only difference betseen the xs_bind4 and the
> >>>> xs_bind6 - the size of sockaddr structure they use.
> >>>> 
> >>>> Fortunatelly its size can be indirectly get from the transport.
> >>>> 
> >>>> Change since v1:
> >>>> * use sockaddr_storage instead of sockaddr
> >>>> * use rpc_set_port instead of manual port assigning
> >>> 
> >>> Whoops, dropping this; it breaks nfsd startup.  I haven't figured out
> >>> why yet, but I get
> >>> 
> >>> RPC: server localhost requires stronger authentication.
> >>> svc: failed to register nfsdv2 RPC service (errno 13).
> >> 
> >> Capturing a network trace of lo during server initialization should reveal all.  Compare a trace from a working run and a non-working one.
> > 
> > Hm.  One difference is the source port of the portmap calls: 33471 in
> > the bad case, 1016 in the bad.--b.
> 
> 1016 in the good... yes, that's because the server's registration upcall needs a privileged port, and xs_bind is not obliging here.  That certainly would result in a "requires stronger authentication" error from rpcbind.

So, adding some printk's: the problem is that the patch assumes that the
trasnport->xprt.addrlen and transport->srcaddr.ss_family have been
initialized at this point.  But they haven't been.  I don't know where
that's supposed to happen.

--b.

> 
> > 
> >> 
> >>> 
> >>> --b.
> >>> 
> >>>> 
> >>>> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> >>>> ---
> >>>> net/sunrpc/xprtsock.c |   64 +++++++++++++------------------------------------
> >>>> 1 files changed, 17 insertions(+), 47 deletions(-)
> >>>> 
> >>>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> >>>> index 7fdf2bb..bab808f 100644
> >>>> --- a/net/sunrpc/xprtsock.c
> >>>> +++ b/net/sunrpc/xprtsock.c
> >>>> @@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
> >>>> 		return xprt_max_resvport;
> >>>> 	return --port;
> >>>> }
> >>>> -
> >>>> -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> >>>> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
> >>>> {
> >>>> -	struct sockaddr_in myaddr = {
> >>>> -		.sin_family = AF_INET,
> >>>> -	};
> >>>> -	struct sockaddr_in *sa;
> >>>> +	struct sockaddr_storage myaddr;
> >>>> 	int err, nloop = 0;
> >>>> 	unsigned short port = xs_get_srcport(transport);
> >>>> 	unsigned short last;
> >>>> 
> >>>> -	sa = (struct sockaddr_in *)&transport->srcaddr;
> >>>> -	myaddr.sin_addr = sa->sin_addr;
> >>>> +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
> >>>> 	do {
> >>>> -		myaddr.sin_port = htons(port);
> >>>> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> >>>> -						sizeof(myaddr));
> >>>> +		rpc_set_port((struct sockaddr *)&myaddr, port);
> >>>> +		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
> >>>> +				transport->xprt.addrlen);
> >>>> 		if (port == 0)
> >>>> 			break;
> >>>> 		if (err == 0) {
> >>>> @@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> >>>> 		if (port > last)
> >>>> 			nloop++;
> >>>> 	} while (err == -EADDRINUSE && nloop != 2);
> >>>> -	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
> >>>> -			__func__, &myaddr.sin_addr,
> >>>> -			port, err ? "failed" : "ok", err);
> >>>> -	return err;
> >>>> -}
> >>>> -
> >>>> -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
> >>>> -{
> >>>> -	struct sockaddr_in6 myaddr = {
> >>>> -		.sin6_family = AF_INET6,
> >>>> -	};
> >>>> -	struct sockaddr_in6 *sa;
> >>>> -	int err, nloop = 0;
> >>>> -	unsigned short port = xs_get_srcport(transport);
> >>>> -	unsigned short last;
> >>>> 
> >>>> -	sa = (struct sockaddr_in6 *)&transport->srcaddr;
> >>>> -	myaddr.sin6_addr = sa->sin6_addr;
> >>>> -	do {
> >>>> -		myaddr.sin6_port = htons(port);
> >>>> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> >>>> -						sizeof(myaddr));
> >>>> -		if (port == 0)
> >>>> -			break;
> >>>> -		if (err == 0) {
> >>>> -			transport->srcport = port;
> >>>> -			break;
> >>>> -		}
> >>>> -		last = port;
> >>>> -		port = xs_next_srcport(transport, port);
> >>>> -		if (port > last)
> >>>> -			nloop++;
> >>>> -	} while (err == -EADDRINUSE && nloop != 2);
> >>>> -	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
> >>>> -		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
> >>>> +	if (myaddr.ss_family == PF_INET)
> >>>> +		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
> >>>> +				&((struct sockaddr_in *)&myaddr)->sin_addr,
> >>>> +				port, err ? "failed" : "ok", err);
> >>>> +	else
> >>>> +		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
> >>>> +				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
> >>>> +				port, err ? "failed" : "ok", err);
> >>>> 	return err;
> >>>> }
> >>>> 
> >>>> +
> >>>> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >>>> static struct lock_class_key xs_key[2];
> >>>> static struct lock_class_key xs_slock_key[2];
> >>>> @@ -1645,7 +1615,7 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
> >>>> 	}
> >>>> 	xs_reclassify_socket4(sock);
> >>>> 
> >>>> -	if (xs_bind4(transport, sock)) {
> >>>> +	if (xs_bind(transport, sock)) {
> >>>> 		sock_release(sock);
> >>>> 		goto out;
> >>>> 	}
> >>>> @@ -1669,7 +1639,7 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
> >>>> 	}
> >>>> 	xs_reclassify_socket6(sock);
> >>>> 
> >>>> -	if (xs_bind6(transport, sock)) {
> >>>> +	if (xs_bind(transport, sock)) {
> >>>> 		sock_release(sock);
> >>>> 		goto out;
> >>>> 	}
> >>>> -- 
> >>>> 1.5.5.6
> >>>> 
> >> 
> >> -- 
> >> chuck[dot]lever[at]oracle[dot]com
> >> 
> >> 
> >> 
> >> 
> 
> -- 
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
> 

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[J. Bruce Fields]

On Mon, Oct 18, 2010 at 05:15:37PM -0400, J. Bruce Fields wrote:
> On Fri, Oct 15, 2010 at 02:11:29PM -0400, Chuck Lever wrote:
> > 
> > On Oct 15, 2010, at 2:08 PM, J. Bruce Fields wrote:
> > 
> > > On Fri, Oct 15, 2010 at 12:39:36PM -0400, Chuck Lever wrote:
> > >> 
> > >> On Oct 15, 2010, at 12:05 PM, J. Bruce Fields wrote:
> > >> 
> > >>> On Tue, Oct 05, 2010 at 03:53:08PM +0400, Pavel Emelyanov wrote:
> > >>>> There's the only difference betseen the xs_bind4 and the
> > >>>> xs_bind6 - the size of sockaddr structure they use.
> > >>>> 
> > >>>> Fortunatelly its size can be indirectly get from the transport.
> > >>>> 
> > >>>> Change since v1:
> > >>>> * use sockaddr_storage instead of sockaddr
> > >>>> * use rpc_set_port instead of manual port assigning
> > >>> 
> > >>> Whoops, dropping this; it breaks nfsd startup.  I haven't figured out
> > >>> why yet, but I get
> > >>> 
> > >>> RPC: server localhost requires stronger authentication.
> > >>> svc: failed to register nfsdv2 RPC service (errno 13).
> > >> 
> > >> Capturing a network trace of lo during server initialization should reveal all.  Compare a trace from a working run and a non-working one.
> > > 
> > > Hm.  One difference is the source port of the portmap calls: 33471 in
> > > the bad case, 1016 in the bad.--b.
> > 
> > 1016 in the good... yes, that's because the server's registration upcall needs a privileged port, and xs_bind is not obliging here.  That certainly would result in a "requires stronger authentication" error from rpcbind.
> 
> So, adding some printk's: the problem is that the patch assumes that the
> trasnport->xprt.addrlen and transport->srcaddr.ss_family have been
> initialized at this point.  But they haven't been.  I don't know where
> that's supposed to happen.

Sorry, I meant just the family, the length seems fine.

I'll just fold this into the relevant patches and push out the result if
nobody objects.

--b.

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 8dc287f..324d97a 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1625,6 +1625,7 @@ static struct socket *xs_create_sock(struct rpc_xprt *xprt,
 				protocol, -err);
 		goto out;
 	}
+	transport->srcaddr.ss_family = family;
 	xs_reclassify_socket(family, sock);
 
 	if (xs_bind(transport, sock)) {

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[Chuck Lever]

On Oct 18, 2010, at 5:43 PM, J. Bruce Fields wrote:

> On Mon, Oct 18, 2010 at 05:15:37PM -0400, J. Bruce Fields wrote:
>> On Fri, Oct 15, 2010 at 02:11:29PM -0400, Chuck Lever wrote:
>>> 
>>> On Oct 15, 2010, at 2:08 PM, J. Bruce Fields wrote:
>>> 
>>>> On Fri, Oct 15, 2010 at 12:39:36PM -0400, Chuck Lever wrote:
>>>>> 
>>>>> On Oct 15, 2010, at 12:05 PM, J. Bruce Fields wrote:
>>>>> 
>>>>>> On Tue, Oct 05, 2010 at 03:53:08PM +0400, Pavel Emelyanov wrote:
>>>>>>> There's the only difference betseen the xs_bind4 and the
>>>>>>> xs_bind6 - the size of sockaddr structure they use.
>>>>>>> 
>>>>>>> Fortunatelly its size can be indirectly get from the transport.
>>>>>>> 
>>>>>>> Change since v1:
>>>>>>> * use sockaddr_storage instead of sockaddr
>>>>>>> * use rpc_set_port instead of manual port assigning
>>>>>> 
>>>>>> Whoops, dropping this; it breaks nfsd startup.  I haven't figured out
>>>>>> why yet, but I get
>>>>>> 
>>>>>> RPC: server localhost requires stronger authentication.
>>>>>> svc: failed to register nfsdv2 RPC service (errno 13).
>>>>> 
>>>>> Capturing a network trace of lo during server initialization should reveal all.  Compare a trace from a working run and a non-working one.
>>>> 
>>>> Hm.  One difference is the source port of the portmap calls: 33471 in
>>>> the bad case, 1016 in the bad.--b.
>>> 
>>> 1016 in the good... yes, that's because the server's registration upcall needs a privileged port, and xs_bind is not obliging here.  That certainly would result in a "requires stronger authentication" error from rpcbind.
>> 
>> So, adding some printk's: the problem is that the patch assumes that the
>> trasnport->xprt.addrlen and transport->srcaddr.ss_family have been
>> initialized at this point.  But they haven't been.  I don't know where
>> that's supposed to happen.
> 
> Sorry, I meant just the family, the length seems fine.
> 
> I'll just fold this into the relevant patches and push out the result if
> nobody objects.
> 
> --b.
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 8dc287f..324d97a 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1625,6 +1625,7 @@ static struct socket *xs_create_sock(struct rpc_xprt *xprt,
> 				protocol, -err);
> 		goto out;
> 	}
> +	transport->srcaddr.ss_family = family;
> 	xs_reclassify_socket(family, sock);
> 
> 	if (xs_bind(transport, sock)) {

Can you post the full revised patch?  I'm wondering if it's OK to init the sockaddr's family that way, but I can't tell from just the snippet.

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

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[J. Bruce Fields]

On Mon, Oct 18, 2010 at 05:53:45PM -0400, Chuck Lever wrote:
> Can you post the full revised patch?  I'm wondering if it's OK to init the sockaddr's family that way, but I can't tell from just the snippet.

I end up modifying these two patches (then the change propagates through
the others without conflicts).

--b.

commit c923f8c579bd65e4d4096cdcc1ca2b17780143ce
Author: Pavel Emelyanov <xemul@parallels.com>
Date:   Tue Oct 5 15:53:08 2010 +0400

    sunrpc: Merge the xs_bind code
    
    There's the only difference betseen the xs_bind4 and the
    xs_bind6 - the size of sockaddr structure they use.
    
    Fortunatelly its size can be indirectly get from the transport.
    
    Change since v1:
    * use sockaddr_storage instead of sockaddr
    * use rpc_set_port instead of manual port assigning
    
    Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
    [bfields@redhat.com: fix address family initialization]
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 7fdf2bb..fc1e767 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
 		return xprt_max_resvport;
 	return --port;
 }
-
-static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
+static int xs_bind(struct sock_xprt *transport, struct socket *sock)
 {
-	struct sockaddr_in myaddr = {
-		.sin_family = AF_INET,
-	};
-	struct sockaddr_in *sa;
+	struct sockaddr_storage myaddr;
 	int err, nloop = 0;
 	unsigned short port = xs_get_srcport(transport);
 	unsigned short last;
 
-	sa = (struct sockaddr_in *)&transport->srcaddr;
-	myaddr.sin_addr = sa->sin_addr;
+	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
 	do {
-		myaddr.sin_port = htons(port);
-		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
-						sizeof(myaddr));
+		rpc_set_port((struct sockaddr *)&myaddr, port);
+		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
+				transport->xprt.addrlen);
 		if (port == 0)
 			break;
 		if (err == 0) {
@@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
 		if (port > last)
 			nloop++;
 	} while (err == -EADDRINUSE && nloop != 2);
-	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
-			__func__, &myaddr.sin_addr,
-			port, err ? "failed" : "ok", err);
-	return err;
-}
-
-static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
-{
-	struct sockaddr_in6 myaddr = {
-		.sin6_family = AF_INET6,
-	};
-	struct sockaddr_in6 *sa;
-	int err, nloop = 0;
-	unsigned short port = xs_get_srcport(transport);
-	unsigned short last;
 
-	sa = (struct sockaddr_in6 *)&transport->srcaddr;
-	myaddr.sin6_addr = sa->sin6_addr;
-	do {
-		myaddr.sin6_port = htons(port);
-		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
-						sizeof(myaddr));
-		if (port == 0)
-			break;
-		if (err == 0) {
-			transport->srcport = port;
-			break;
-		}
-		last = port;
-		port = xs_next_srcport(transport, port);
-		if (port > last)
-			nloop++;
-	} while (err == -EADDRINUSE && nloop != 2);
-	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
-		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
+	if (myaddr.ss_family == PF_INET)
+		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
+				&((struct sockaddr_in *)&myaddr)->sin_addr,
+				port, err ? "failed" : "ok", err);
+	else
+		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
+				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
+				port, err ? "failed" : "ok", err);
 	return err;
 }
 
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 static struct lock_class_key xs_key[2];
 static struct lock_class_key xs_slock_key[2];
@@ -1643,9 +1613,10 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
 				protocol, -err);
 		goto out;
 	}
+	transport->srcaddr.ss_family = AF_INET;
 	xs_reclassify_socket4(sock);
 
-	if (xs_bind4(transport, sock)) {
+	if (xs_bind(transport, sock)) {
 		sock_release(sock);
 		goto out;
 	}
@@ -1667,9 +1638,10 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
 				protocol, -err);
 		goto out;
 	}
+	transport->srcaddr.ss_family = AF_INET6;
 	xs_reclassify_socket6(sock);
 
-	if (xs_bind6(transport, sock)) {
+	if (xs_bind(transport, sock)) {
 		sock_release(sock);
 		goto out;
 	}
commit fa8045a09755f64f8a74c7a8f5046da9d632d4c5
Author: Pavel Emelyanov <xemul@parallels.com>
Date:   Mon Oct 4 16:56:38 2010 +0400

    sunrpc: Merge xs_create_sock code
    
    After xs_bind is merged it's easy to merge its callers.
    
    Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
    [bfields@redhat.com: fix address family initialization]
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index fc1e767..324d97a 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1591,6 +1591,14 @@ static inline void xs_reclassify_socket6(struct socket *sock)
 	sock_lock_init_class_and_name(sk, "slock-AF_INET6-RPC",
 		&xs_slock_key[1], "sk_lock-AF_INET6-RPC", &xs_key[1]);
 }
+
+static inline void xs_reclassify_socket(int family, struct socket *sock)
+{
+	if (family == PF_INET)
+		xs_reclassify_socket4(sock);
+	else
+		xs_reclassify_socket6(sock);
+}
 #else
 static inline void xs_reclassify_socket4(struct socket *sock)
 {
@@ -1599,22 +1607,26 @@ static inline void xs_reclassify_socket4(struct socket *sock)
 static inline void xs_reclassify_socket6(struct socket *sock)
 {
 }
+
+static inline void xs_reclassify_socket(int family, struct socket *sock)
+{
+}
 #endif
 
-static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
-		struct sock_xprt *transport, int type, int protocol)
+static struct socket *xs_create_sock(struct rpc_xprt *xprt,
+		struct sock_xprt *transport, int family, int type, int protocol)
 {
 	struct socket *sock;
 	int err;
 
-	err = __sock_create(xprt->xprt_net, PF_INET, type, protocol, &sock, 1);
+	err = __sock_create(xprt->xprt_net, family, type, protocol, &sock, 1);
 	if (err < 0) {
 		dprintk("RPC:       can't create %d transport socket (%d).\n",
 				protocol, -err);
 		goto out;
 	}
-	transport->srcaddr.ss_family = AF_INET;
-	xs_reclassify_socket4(sock);
+	transport->srcaddr.ss_family = family;
+	xs_reclassify_socket(family, sock);
 
 	if (xs_bind(transport, sock)) {
 		sock_release(sock);
@@ -1626,29 +1638,16 @@ out:
 	return ERR_PTR(err);
 }
 
-static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
+static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
 		struct sock_xprt *transport, int type, int protocol)
 {
-	struct socket *sock;
-	int err;
-
-	err = __sock_create(xprt->xprt_net, PF_INET6, type, protocol, &sock, 1);
-	if (err < 0) {
-		dprintk("RPC:       can't create %d transport socket (%d).\n",
-				protocol, -err);
-		goto out;
-	}
-	transport->srcaddr.ss_family = AF_INET6;
-	xs_reclassify_socket6(sock);
-
-	if (xs_bind(transport, sock)) {
-		sock_release(sock);
-		goto out;
-	}
+	return xs_create_sock(xprt, transport, PF_INET, type, protocol);
+}
 
-	return sock;
-out:
-	return ERR_PTR(err);
+static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
+		struct sock_xprt *transport, int type, int protocol)
+{
+	return xs_create_sock(xprt, transport, PF_INET6, type, protocol);
 }
 
 static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[Chuck Lever]

On Oct 18, 2010, at 5:57 PM, J. Bruce Fields wrote:

> On Mon, Oct 18, 2010 at 05:53:45PM -0400, Chuck Lever wrote:
>> Can you post the full revised patch?  I'm wondering if it's OK to init the sockaddr's family that way, but I can't tell from just the snippet.
> 
> I end up modifying these two patches (then the change propagates through
> the others without conflicts).
> 
> --b.
> 
> commit c923f8c579bd65e4d4096cdcc1ca2b17780143ce
> Author: Pavel Emelyanov <xemul@parallels.com>
> Date:   Tue Oct 5 15:53:08 2010 +0400
> 
>    sunrpc: Merge the xs_bind code
> 
>    There's the only difference betseen the xs_bind4 and the
>    xs_bind6 - the size of sockaddr structure they use.
> 
>    Fortunatelly its size can be indirectly get from the transport.
> 
>    Change since v1:
>    * use sockaddr_storage instead of sockaddr
>    * use rpc_set_port instead of manual port assigning
> 
>    Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>    [bfields@redhat.com: fix address family initialization]
>    Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 7fdf2bb..fc1e767 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
> 		return xprt_max_resvport;
> 	return --port;
> }
> -
> -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
> {
> -	struct sockaddr_in myaddr = {
> -		.sin_family = AF_INET,
> -	};
> -	struct sockaddr_in *sa;
> +	struct sockaddr_storage myaddr;
> 	int err, nloop = 0;
> 	unsigned short port = xs_get_srcport(transport);
> 	unsigned short last;
> 
> -	sa = (struct sockaddr_in *)&transport->srcaddr;
> -	myaddr.sin_addr = sa->sin_addr;
> +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);

This routine used to construct my_addr by hand in automatic storage by copying just the address part from srcaddr.  Since there was a separate xs_bind for IPv4 and one for IPv6, we already had the source address family planted in my_addr.

Now we're copying the full source address, including sa_family, from the passed in srcaddr.  But most ULPs don't actually set up srcaddr.  In fact, I think only lockd does.

The calling convention for rpc_create() allows this.  Notice in xs_setup_xprt() the source address is copied only if the args->srcaddr pointer is not NULL.  If it _is_ NULL, then the sock_xprt's srcaddr field should be filled with all zeros (which is the ANYADDR in both IPv4 and IPv6) because we allocate the new sock_xprt structure via kzalloc().  But the sa_family field is left zero as well in this case.

This is where the actual bug is, I think.  When srcaddr is NULL, instead of leaving that srcaddr field uninitialized in xs_setup_xprt(), it should plant an appropriate ANYADDR address there.  The family of that ANYADDR address is determined by the family of dstaddr, which is always initialized before this function is called.  I can send you a simple example patch that might apply before Pavel's work, if you like.

In the current code, the fact that ANYADDR just happens to be all zeroes saved our ass on this one.

> 	do {
> -		myaddr.sin_port = htons(port);
> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> -						sizeof(myaddr));
> +		rpc_set_port((struct sockaddr *)&myaddr, port);
> +		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
> +				transport->xprt.addrlen);
> 		if (port == 0)
> 			break;
> 		if (err == 0) {
> @@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> 		if (port > last)
> 			nloop++;
> 	} while (err == -EADDRINUSE && nloop != 2);
> -	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
> -			__func__, &myaddr.sin_addr,
> -			port, err ? "failed" : "ok", err);
> -	return err;
> -}
> -
> -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
> -{
> -	struct sockaddr_in6 myaddr = {
> -		.sin6_family = AF_INET6,
> -	};
> -	struct sockaddr_in6 *sa;
> -	int err, nloop = 0;
> -	unsigned short port = xs_get_srcport(transport);
> -	unsigned short last;
> 
> -	sa = (struct sockaddr_in6 *)&transport->srcaddr;
> -	myaddr.sin6_addr = sa->sin6_addr;
> -	do {
> -		myaddr.sin6_port = htons(port);
> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> -						sizeof(myaddr));
> -		if (port == 0)
> -			break;
> -		if (err == 0) {
> -			transport->srcport = port;
> -			break;
> -		}
> -		last = port;
> -		port = xs_next_srcport(transport, port);
> -		if (port > last)
> -			nloop++;
> -	} while (err == -EADDRINUSE && nloop != 2);
> -	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
> -		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
> +	if (myaddr.ss_family == PF_INET)
> +		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
> +				&((struct sockaddr_in *)&myaddr)->sin_addr,
> +				port, err ? "failed" : "ok", err);
> +	else
> +		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
> +				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
> +				port, err ? "failed" : "ok", err);
> 	return err;
> }
> 
> +
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> static struct lock_class_key xs_key[2];
> static struct lock_class_key xs_slock_key[2];
> @@ -1643,9 +1613,10 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
> 				protocol, -err);
> 		goto out;
> 	}
> +	transport->srcaddr.ss_family = AF_INET;
> 	xs_reclassify_socket4(sock);
> 
> -	if (xs_bind4(transport, sock)) {
> +	if (xs_bind(transport, sock)) {
> 		sock_release(sock);
> 		goto out;
> 	}
> @@ -1667,9 +1638,10 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
> 				protocol, -err);
> 		goto out;
> 	}
> +	transport->srcaddr.ss_family = AF_INET6;
> 	xs_reclassify_socket6(sock);
> 
> -	if (xs_bind6(transport, sock)) {
> +	if (xs_bind(transport, sock)) {
> 		sock_release(sock);
> 		goto out;
> 	}
> commit fa8045a09755f64f8a74c7a8f5046da9d632d4c5
> Author: Pavel Emelyanov <xemul@parallels.com>
> Date:   Mon Oct 4 16:56:38 2010 +0400
> 
>    sunrpc: Merge xs_create_sock code
> 
>    After xs_bind is merged it's easy to merge its callers.
> 
>    Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>    [bfields@redhat.com: fix address family initialization]
>    Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index fc1e767..324d97a 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1591,6 +1591,14 @@ static inline void xs_reclassify_socket6(struct socket *sock)
> 	sock_lock_init_class_and_name(sk, "slock-AF_INET6-RPC",
> 		&xs_slock_key[1], "sk_lock-AF_INET6-RPC", &xs_key[1]);
> }
> +
> +static inline void xs_reclassify_socket(int family, struct socket *sock)
> +{
> +	if (family == PF_INET)
> +		xs_reclassify_socket4(sock);
> +	else
> +		xs_reclassify_socket6(sock);


Just noticed: This should be a switch, just like this type of logic is everywhere else in this code.



> +}
> #else
> static inline void xs_reclassify_socket4(struct socket *sock)
> {
> @@ -1599,22 +1607,26 @@ static inline void xs_reclassify_socket4(struct socket *sock)
> static inline void xs_reclassify_socket6(struct socket *sock)
> {
> }
> +
> +static inline void xs_reclassify_socket(int family, struct socket *sock)
> +{
> +}
> #endif
> 
> -static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
> -		struct sock_xprt *transport, int type, int protocol)
> +static struct socket *xs_create_sock(struct rpc_xprt *xprt,
> +		struct sock_xprt *transport, int family, int type, int protocol)
> {
> 	struct socket *sock;
> 	int err;
> 
> -	err = __sock_create(xprt->xprt_net, PF_INET, type, protocol, &sock, 1);
> +	err = __sock_create(xprt->xprt_net, family, type, protocol, &sock, 1);
> 	if (err < 0) {
> 		dprintk("RPC:       can't create %d transport socket (%d).\n",
> 				protocol, -err);
> 		goto out;
> 	}
> -	transport->srcaddr.ss_family = AF_INET;
> -	xs_reclassify_socket4(sock);
> +	transport->srcaddr.ss_family = family;
> +	xs_reclassify_socket(family, sock);


See above.  I think this just covers up the problem, and the real fix is in xs_setup_xprt().



> 	if (xs_bind(transport, sock)) {
> 		sock_release(sock);
> @@ -1626,29 +1638,16 @@ out:
> 	return ERR_PTR(err);
> }
> 
> -static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
> +static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
> 		struct sock_xprt *transport, int type, int protocol)
> {
> -	struct socket *sock;
> -	int err;
> -
> -	err = __sock_create(xprt->xprt_net, PF_INET6, type, protocol, &sock, 1);
> -	if (err < 0) {
> -		dprintk("RPC:       can't create %d transport socket (%d).\n",
> -				protocol, -err);
> -		goto out;
> -	}
> -	transport->srcaddr.ss_family = AF_INET6;
> -	xs_reclassify_socket6(sock);
> -
> -	if (xs_bind(transport, sock)) {
> -		sock_release(sock);
> -		goto out;
> -	}
> +	return xs_create_sock(xprt, transport, PF_INET, type, protocol);
> +}
> 
> -	return sock;
> -out:
> -	return ERR_PTR(err);
> +static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
> +		struct sock_xprt *transport, int type, int protocol)
> +{
> +	return xs_create_sock(xprt, transport, PF_INET6, type, protocol);
> }
> 
> static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)

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

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[J. Bruce Fields]

On Mon, Oct 18, 2010 at 06:37:01PM -0400, Chuck Lever wrote:
> 
> On Oct 18, 2010, at 5:57 PM, J. Bruce Fields wrote:
> 
> > On Mon, Oct 18, 2010 at 05:53:45PM -0400, Chuck Lever wrote:
> >> Can you post the full revised patch?  I'm wondering if it's OK to init the sockaddr's family that way, but I can't tell from just the snippet.
> > 
> > I end up modifying these two patches (then the change propagates through
> > the others without conflicts).
> > 
> > --b.
> > 
> > commit c923f8c579bd65e4d4096cdcc1ca2b17780143ce
> > Author: Pavel Emelyanov <xemul@parallels.com>
> > Date:   Tue Oct 5 15:53:08 2010 +0400
> > 
> >    sunrpc: Merge the xs_bind code
> > 
> >    There's the only difference betseen the xs_bind4 and the
> >    xs_bind6 - the size of sockaddr structure they use.
> > 
> >    Fortunatelly its size can be indirectly get from the transport.
> > 
> >    Change since v1:
> >    * use sockaddr_storage instead of sockaddr
> >    * use rpc_set_port instead of manual port assigning
> > 
> >    Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> >    [bfields@redhat.com: fix address family initialization]
> >    Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > 
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > index 7fdf2bb..fc1e767 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
> > 		return xprt_max_resvport;
> > 	return --port;
> > }
> > -
> > -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> > +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
> > {
> > -	struct sockaddr_in myaddr = {
> > -		.sin_family = AF_INET,
> > -	};
> > -	struct sockaddr_in *sa;
> > +	struct sockaddr_storage myaddr;
> > 	int err, nloop = 0;
> > 	unsigned short port = xs_get_srcport(transport);
> > 	unsigned short last;
> > 
> > -	sa = (struct sockaddr_in *)&transport->srcaddr;
> > -	myaddr.sin_addr = sa->sin_addr;
> > +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
> 
> This routine used to construct my_addr by hand in automatic storage by copying just the address part from srcaddr.  Since there was a separate xs_bind for IPv4 and one for IPv6, we already had the source address family planted in my_addr.
> 
> Now we're copying the full source address, including sa_family, from the passed in srcaddr.  But most ULPs don't actually set up srcaddr.  In fact, I think only lockd does.
> 
> The calling convention for rpc_create() allows this.  Notice in xs_setup_xprt() the source address is copied only if the args->srcaddr pointer is not NULL.  If it _is_ NULL, then the sock_xprt's srcaddr field should be filled with all zeros (which is the ANYADDR in both IPv4 and IPv6) because we allocate the new sock_xprt structure via kzalloc().  But the sa_family field is left zero as well in this case.
> 
> This is where the actual bug is, I think.  When srcaddr is NULL, instead of leaving that srcaddr field uninitialized in xs_setup_xprt(), it should plant an appropriate ANYADDR address there.  The family of that ANYADDR address is determined by the family of dstaddr, which is always initialized before this function is called.  I can send you a simple example patch that might apply before Pavel's work, if you like.
> 
> In the current code, the fact that ANYADDR just happens to be all zeroes saved our ass on this one.

It sounds like you're saying that before we ensured that fields other
tha .sin_family were zero in one way (with the automatic initialiation
of myaddr above), and now ensure it in another (kzalloc of the thing we
copy from)--I'm not sure the difference is as important as all that?

But, honestly, I'm not following all this--I'll happily take whatever
revision you and Pavel want to give me, either incremental or as a
replacement for the 17 patches.

--b.

> 
> > 	do {
> > -		myaddr.sin_port = htons(port);
> > -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> > -						sizeof(myaddr));
> > +		rpc_set_port((struct sockaddr *)&myaddr, port);
> > +		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
> > +				transport->xprt.addrlen);
> > 		if (port == 0)
> > 			break;
> > 		if (err == 0) {
> > @@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> > 		if (port > last)
> > 			nloop++;
> > 	} while (err == -EADDRINUSE && nloop != 2);
> > -	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
> > -			__func__, &myaddr.sin_addr,
> > -			port, err ? "failed" : "ok", err);
> > -	return err;
> > -}
> > -
> > -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
> > -{
> > -	struct sockaddr_in6 myaddr = {
> > -		.sin6_family = AF_INET6,
> > -	};
> > -	struct sockaddr_in6 *sa;
> > -	int err, nloop = 0;
> > -	unsigned short port = xs_get_srcport(transport);
> > -	unsigned short last;
> > 
> > -	sa = (struct sockaddr_in6 *)&transport->srcaddr;
> > -	myaddr.sin6_addr = sa->sin6_addr;
> > -	do {
> > -		myaddr.sin6_port = htons(port);
> > -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> > -						sizeof(myaddr));
> > -		if (port == 0)
> > -			break;
> > -		if (err == 0) {
> > -			transport->srcport = port;
> > -			break;
> > -		}
> > -		last = port;
> > -		port = xs_next_srcport(transport, port);
> > -		if (port > last)
> > -			nloop++;
> > -	} while (err == -EADDRINUSE && nloop != 2);
> > -	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
> > -		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
> > +	if (myaddr.ss_family == PF_INET)
> > +		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
> > +				&((struct sockaddr_in *)&myaddr)->sin_addr,
> > +				port, err ? "failed" : "ok", err);
> > +	else
> > +		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
> > +				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
> > +				port, err ? "failed" : "ok", err);
> > 	return err;
> > }
> > 
> > +
> > #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > static struct lock_class_key xs_key[2];
> > static struct lock_class_key xs_slock_key[2];
> > @@ -1643,9 +1613,10 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
> > 				protocol, -err);
> > 		goto out;
> > 	}
> > +	transport->srcaddr.ss_family = AF_INET;
> > 	xs_reclassify_socket4(sock);
> > 
> > -	if (xs_bind4(transport, sock)) {
> > +	if (xs_bind(transport, sock)) {
> > 		sock_release(sock);
> > 		goto out;
> > 	}
> > @@ -1667,9 +1638,10 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
> > 				protocol, -err);
> > 		goto out;
> > 	}
> > +	transport->srcaddr.ss_family = AF_INET6;
> > 	xs_reclassify_socket6(sock);
> > 
> > -	if (xs_bind6(transport, sock)) {
> > +	if (xs_bind(transport, sock)) {
> > 		sock_release(sock);
> > 		goto out;
> > 	}
> > commit fa8045a09755f64f8a74c7a8f5046da9d632d4c5
> > Author: Pavel Emelyanov <xemul@parallels.com>
> > Date:   Mon Oct 4 16:56:38 2010 +0400
> > 
> >    sunrpc: Merge xs_create_sock code
> > 
> >    After xs_bind is merged it's easy to merge its callers.
> > 
> >    Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> >    [bfields@redhat.com: fix address family initialization]
> >    Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > 
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > index fc1e767..324d97a 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -1591,6 +1591,14 @@ static inline void xs_reclassify_socket6(struct socket *sock)
> > 	sock_lock_init_class_and_name(sk, "slock-AF_INET6-RPC",
> > 		&xs_slock_key[1], "sk_lock-AF_INET6-RPC", &xs_key[1]);
> > }
> > +
> > +static inline void xs_reclassify_socket(int family, struct socket *sock)
> > +{
> > +	if (family == PF_INET)
> > +		xs_reclassify_socket4(sock);
> > +	else
> > +		xs_reclassify_socket6(sock);
> 
> 
> Just noticed: This should be a switch, just like this type of logic is everywhere else in this code.
> 
> 
> 
> > +}
> > #else
> > static inline void xs_reclassify_socket4(struct socket *sock)
> > {
> > @@ -1599,22 +1607,26 @@ static inline void xs_reclassify_socket4(struct socket *sock)
> > static inline void xs_reclassify_socket6(struct socket *sock)
> > {
> > }
> > +
> > +static inline void xs_reclassify_socket(int family, struct socket *sock)
> > +{
> > +}
> > #endif
> > 
> > -static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
> > -		struct sock_xprt *transport, int type, int protocol)
> > +static struct socket *xs_create_sock(struct rpc_xprt *xprt,
> > +		struct sock_xprt *transport, int family, int type, int protocol)
> > {
> > 	struct socket *sock;
> > 	int err;
> > 
> > -	err = __sock_create(xprt->xprt_net, PF_INET, type, protocol, &sock, 1);
> > +	err = __sock_create(xprt->xprt_net, family, type, protocol, &sock, 1);
> > 	if (err < 0) {
> > 		dprintk("RPC:       can't create %d transport socket (%d).\n",
> > 				protocol, -err);
> > 		goto out;
> > 	}
> > -	transport->srcaddr.ss_family = AF_INET;
> > -	xs_reclassify_socket4(sock);
> > +	transport->srcaddr.ss_family = family;
> > +	xs_reclassify_socket(family, sock);
> 
> 
> See above.  I think this just covers up the problem, and the real fix is in xs_setup_xprt().
> 
> 
> 
> > 	if (xs_bind(transport, sock)) {
> > 		sock_release(sock);
> > @@ -1626,29 +1638,16 @@ out:
> > 	return ERR_PTR(err);
> > }
> > 
> > -static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
> > +static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
> > 		struct sock_xprt *transport, int type, int protocol)
> > {
> > -	struct socket *sock;
> > -	int err;
> > -
> > -	err = __sock_create(xprt->xprt_net, PF_INET6, type, protocol, &sock, 1);
> > -	if (err < 0) {
> > -		dprintk("RPC:       can't create %d transport socket (%d).\n",
> > -				protocol, -err);
> > -		goto out;
> > -	}
> > -	transport->srcaddr.ss_family = AF_INET6;
> > -	xs_reclassify_socket6(sock);
> > -
> > -	if (xs_bind(transport, sock)) {
> > -		sock_release(sock);
> > -		goto out;
> > -	}
> > +	return xs_create_sock(xprt, transport, PF_INET, type, protocol);
> > +}
> > 
> > -	return sock;
> > -out:
> > -	return ERR_PTR(err);
> > +static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
> > +		struct sock_xprt *transport, int type, int protocol)
> > +{
> > +	return xs_create_sock(xprt, transport, PF_INET6, type, protocol);
> > }
> > 
> > static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> 
> -- 
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
> 

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[Chuck Lever]

On Oct 18, 2010, at 7:55 PM, J. Bruce Fields wrote:

> On Mon, Oct 18, 2010 at 06:37:01PM -0400, Chuck Lever wrote:
>> 
>> On Oct 18, 2010, at 5:57 PM, J. Bruce Fields wrote:
>> 
>>> On Mon, Oct 18, 2010 at 05:53:45PM -0400, Chuck Lever wrote:
>>>> Can you post the full revised patch?  I'm wondering if it's OK to init the sockaddr's family that way, but I can't tell from just the snippet.
>>> 
>>> I end up modifying these two patches (then the change propagates through
>>> the others without conflicts).
>>> 
>>> --b.
>>> 
>>> commit c923f8c579bd65e4d4096cdcc1ca2b17780143ce
>>> Author: Pavel Emelyanov <xemul@parallels.com>
>>> Date:   Tue Oct 5 15:53:08 2010 +0400
>>> 
>>>   sunrpc: Merge the xs_bind code
>>> 
>>>   There's the only difference betseen the xs_bind4 and the
>>>   xs_bind6 - the size of sockaddr structure they use.
>>> 
>>>   Fortunatelly its size can be indirectly get from the transport.
>>> 
>>>   Change since v1:
>>>   * use sockaddr_storage instead of sockaddr
>>>   * use rpc_set_port instead of manual port assigning
>>> 
>>>   Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>>>   [bfields@redhat.com: fix address family initialization]
>>>   Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>>> 
>>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>>> index 7fdf2bb..fc1e767 100644
>>> --- a/net/sunrpc/xprtsock.c
>>> +++ b/net/sunrpc/xprtsock.c
>>> @@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
>>> 		return xprt_max_resvport;
>>> 	return --port;
>>> }
>>> -
>>> -static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
>>> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
>>> {
>>> -	struct sockaddr_in myaddr = {
>>> -		.sin_family = AF_INET,
>>> -	};
>>> -	struct sockaddr_in *sa;
>>> +	struct sockaddr_storage myaddr;
>>> 	int err, nloop = 0;
>>> 	unsigned short port = xs_get_srcport(transport);
>>> 	unsigned short last;
>>> 
>>> -	sa = (struct sockaddr_in *)&transport->srcaddr;
>>> -	myaddr.sin_addr = sa->sin_addr;
>>> +	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
>> 
>> This routine used to construct my_addr by hand in automatic storage by copying just the address part from srcaddr.  Since there was a separate xs_bind for IPv4 and one for IPv6, we already had the source address family planted in my_addr.
>> 
>> Now we're copying the full source address, including sa_family, from the passed in srcaddr.  But most ULPs don't actually set up srcaddr.  In fact, I think only lockd does.
>> 
>> The calling convention for rpc_create() allows this.  Notice in xs_setup_xprt() the source address is copied only if the args->srcaddr pointer is not NULL.  If it _is_ NULL, then the sock_xprt's srcaddr field should be filled with all zeros (which is the ANYADDR in both IPv4 and IPv6) because we allocate the new sock_xprt structure via kzalloc().  But the sa_family field is left zero as well in this case.
>> 
>> This is where the actual bug is, I think.  When srcaddr is NULL, instead of leaving that srcaddr field uninitialized in xs_setup_xprt(), it should plant an appropriate ANYADDR address there.  The family of that ANYADDR address is determined by the family of dstaddr, which is always initialized before this function is called.  I can send you a simple example patch that might apply before Pavel's work, if you like.
>> 
>> In the current code, the fact that ANYADDR just happens to be all zeroes saved our ass on this one.
> 
> It sounds like you're saying that before we ensured that fields other
> tha .sin_family were zero in one way (with the automatic initialiation
> of myaddr above), and now ensure it in another (kzalloc of the thing we
> copy from)--I'm not sure the difference is as important as all that?

When the ULPs did not provide a source address, we've always used a zeroed out sockaddr.  That's marginally incorrect, but since ANYADDR is all zeroes, functionally it doesn't matter.  Formerly xs_bind?() copied only the address part (not the family or the port) when building the bind address.  With Pavel's changes it's copying more than just the address field in the source address.

All I'm saying is that the transport's source address should always be fully initialized (family included) at xprt setup time.

> But, honestly, I'm not following all this--I'll happily take whatever
> revision you and Pavel want to give me, either incremental or as a
> replacement for the 17 patches.

It's a minor change.  I can send you something later today to try.

> --b.
> 
>> 
>>> 	do {
>>> -		myaddr.sin_port = htons(port);
>>> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
>>> -						sizeof(myaddr));
>>> +		rpc_set_port((struct sockaddr *)&myaddr, port);
>>> +		err = kernel_bind(sock, (struct sockaddr *)&myaddr,
>>> +				transport->xprt.addrlen);
>>> 		if (port == 0)
>>> 			break;
>>> 		if (err == 0) {
>>> @@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
>>> 		if (port > last)
>>> 			nloop++;
>>> 	} while (err == -EADDRINUSE && nloop != 2);
>>> -	dprintk("RPC:       %s %pI4:%u: %s (%d)\n",
>>> -			__func__, &myaddr.sin_addr,
>>> -			port, err ? "failed" : "ok", err);
>>> -	return err;
>>> -}
>>> -
>>> -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
>>> -{
>>> -	struct sockaddr_in6 myaddr = {
>>> -		.sin6_family = AF_INET6,
>>> -	};
>>> -	struct sockaddr_in6 *sa;
>>> -	int err, nloop = 0;
>>> -	unsigned short port = xs_get_srcport(transport);
>>> -	unsigned short last;
>>> 
>>> -	sa = (struct sockaddr_in6 *)&transport->srcaddr;
>>> -	myaddr.sin6_addr = sa->sin6_addr;
>>> -	do {
>>> -		myaddr.sin6_port = htons(port);
>>> -		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
>>> -						sizeof(myaddr));
>>> -		if (port == 0)
>>> -			break;
>>> -		if (err == 0) {
>>> -			transport->srcport = port;
>>> -			break;
>>> -		}
>>> -		last = port;
>>> -		port = xs_next_srcport(transport, port);
>>> -		if (port > last)
>>> -			nloop++;
>>> -	} while (err == -EADDRINUSE && nloop != 2);
>>> -	dprintk("RPC:       xs_bind6 %pI6:%u: %s (%d)\n",
>>> -		&myaddr.sin6_addr, port, err ? "failed" : "ok", err);
>>> +	if (myaddr.ss_family == PF_INET)
>>> +		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
>>> +				&((struct sockaddr_in *)&myaddr)->sin_addr,
>>> +				port, err ? "failed" : "ok", err);
>>> +	else
>>> +		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
>>> +				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
>>> +				port, err ? "failed" : "ok", err);
>>> 	return err;
>>> }
>>> 
>>> +
>>> #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>> static struct lock_class_key xs_key[2];
>>> static struct lock_class_key xs_slock_key[2];
>>> @@ -1643,9 +1613,10 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
>>> 				protocol, -err);
>>> 		goto out;
>>> 	}
>>> +	transport->srcaddr.ss_family = AF_INET;
>>> 	xs_reclassify_socket4(sock);
>>> 
>>> -	if (xs_bind4(transport, sock)) {
>>> +	if (xs_bind(transport, sock)) {
>>> 		sock_release(sock);
>>> 		goto out;
>>> 	}
>>> @@ -1667,9 +1638,10 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
>>> 				protocol, -err);
>>> 		goto out;
>>> 	}
>>> +	transport->srcaddr.ss_family = AF_INET6;
>>> 	xs_reclassify_socket6(sock);
>>> 
>>> -	if (xs_bind6(transport, sock)) {
>>> +	if (xs_bind(transport, sock)) {
>>> 		sock_release(sock);
>>> 		goto out;
>>> 	}
>>> commit fa8045a09755f64f8a74c7a8f5046da9d632d4c5
>>> Author: Pavel Emelyanov <xemul@parallels.com>
>>> Date:   Mon Oct 4 16:56:38 2010 +0400
>>> 
>>>   sunrpc: Merge xs_create_sock code
>>> 
>>>   After xs_bind is merged it's easy to merge its callers.
>>> 
>>>   Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>>>   [bfields@redhat.com: fix address family initialization]
>>>   Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>>> 
>>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>>> index fc1e767..324d97a 100644
>>> --- a/net/sunrpc/xprtsock.c
>>> +++ b/net/sunrpc/xprtsock.c
>>> @@ -1591,6 +1591,14 @@ static inline void xs_reclassify_socket6(struct socket *sock)
>>> 	sock_lock_init_class_and_name(sk, "slock-AF_INET6-RPC",
>>> 		&xs_slock_key[1], "sk_lock-AF_INET6-RPC", &xs_key[1]);
>>> }
>>> +
>>> +static inline void xs_reclassify_socket(int family, struct socket *sock)
>>> +{
>>> +	if (family == PF_INET)
>>> +		xs_reclassify_socket4(sock);
>>> +	else
>>> +		xs_reclassify_socket6(sock);
>> 
>> 
>> Just noticed: This should be a switch, just like this type of logic is everywhere else in this code.
>> 
>> 
>> 
>>> +}
>>> #else
>>> static inline void xs_reclassify_socket4(struct socket *sock)
>>> {
>>> @@ -1599,22 +1607,26 @@ static inline void xs_reclassify_socket4(struct socket *sock)
>>> static inline void xs_reclassify_socket6(struct socket *sock)
>>> {
>>> }
>>> +
>>> +static inline void xs_reclassify_socket(int family, struct socket *sock)
>>> +{
>>> +}
>>> #endif
>>> 
>>> -static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
>>> -		struct sock_xprt *transport, int type, int protocol)
>>> +static struct socket *xs_create_sock(struct rpc_xprt *xprt,
>>> +		struct sock_xprt *transport, int family, int type, int protocol)
>>> {
>>> 	struct socket *sock;
>>> 	int err;
>>> 
>>> -	err = __sock_create(xprt->xprt_net, PF_INET, type, protocol, &sock, 1);
>>> +	err = __sock_create(xprt->xprt_net, family, type, protocol, &sock, 1);
>>> 	if (err < 0) {
>>> 		dprintk("RPC:       can't create %d transport socket (%d).\n",
>>> 				protocol, -err);
>>> 		goto out;
>>> 	}
>>> -	transport->srcaddr.ss_family = AF_INET;
>>> -	xs_reclassify_socket4(sock);
>>> +	transport->srcaddr.ss_family = family;
>>> +	xs_reclassify_socket(family, sock);
>> 
>> 
>> See above.  I think this just covers up the problem, and the real fix is in xs_setup_xprt().
>> 
>> 
>> 
>>> 	if (xs_bind(transport, sock)) {
>>> 		sock_release(sock);
>>> @@ -1626,29 +1638,16 @@ out:
>>> 	return ERR_PTR(err);
>>> }
>>> 
>>> -static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
>>> +static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
>>> 		struct sock_xprt *transport, int type, int protocol)
>>> {
>>> -	struct socket *sock;
>>> -	int err;
>>> -
>>> -	err = __sock_create(xprt->xprt_net, PF_INET6, type, protocol, &sock, 1);
>>> -	if (err < 0) {
>>> -		dprintk("RPC:       can't create %d transport socket (%d).\n",
>>> -				protocol, -err);
>>> -		goto out;
>>> -	}
>>> -	transport->srcaddr.ss_family = AF_INET6;
>>> -	xs_reclassify_socket6(sock);
>>> -
>>> -	if (xs_bind(transport, sock)) {
>>> -		sock_release(sock);
>>> -		goto out;
>>> -	}
>>> +	return xs_create_sock(xprt, transport, PF_INET, type, protocol);
>>> +}
>>> 
>>> -	return sock;
>>> -out:
>>> -	return ERR_PTR(err);
>>> +static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
>>> +		struct sock_xprt *transport, int type, int protocol)
>>> +{
>>> +	return xs_create_sock(xprt, transport, PF_INET6, type, protocol);
>>> }
>>> 
>>> static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
>> 
>> -- 
>> chuck[dot]lever[at]oracle[dot]com
>> 
>> 
>> 
>> 

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

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[J. Bruce Fields]

On Tue, Oct 19, 2010 at 10:35:51AM -0400, Chuck Lever wrote:
> 
> On Oct 18, 2010, at 7:55 PM, J. Bruce Fields wrote:
> 
> > On Mon, Oct 18, 2010 at 06:37:01PM -0400, Chuck Lever wrote:
> >> 
> >> The calling convention for rpc_create() allows this.  Notice in xs_setup_xprt() the source address is copied only if the args->srcaddr pointer is not NULL.  If it _is_ NULL, then the sock_xprt's srcaddr field should be filled with all zeros (which is the ANYADDR in both IPv4 and IPv6) because we allocate the new sock_xprt structure via kzalloc().  But the sa_family field is left zero as well in this case.
> >> 
> >> This is where the actual bug is, I think.  When srcaddr is NULL, instead of leaving that srcaddr field uninitialized in xs_setup_xprt(), it should plant an appropriate ANYADDR address there.  The family of that ANYADDR address is determined by the family of dstaddr, which is always initialized before this function is called.  I can send you a simple example patch that might apply before Pavel's work, if you like.
> >> 
> >> In the current code, the fact that ANYADDR just happens to be all zeroes saved our ass on this one.
> > 
> > It sounds like you're saying that before we ensured that fields other
> > tha .sin_family were zero in one way (with the automatic initialiation
> > of myaddr above), and now ensure it in another (kzalloc of the thing we
> > copy from)--I'm not sure the difference is as important as all that?
> 
> When the ULPs did not provide a source address, we've always used a zeroed out sockaddr.  That's marginally incorrect, but since ANYADDR is all zeroes, functionally it doesn't matter.  Formerly xs_bind?() copied only the address part (not the family or the port) when building the bind address.  With Pavel's changes it's copying more than just the address field in the source address.
> 
> All I'm saying is that the transport's source address should always be fully initialized (family included) at xprt setup time.
> 
> > But, honestly, I'm not following all this--I'll happily take whatever
> > revision you and Pavel want to give me, either incremental or as a
> > replacement for the 17 patches.
> 
> It's a minor change.  I can send you something later today to try.

OK, thanks.  I think it'll be easiest to proceed if I just go ahead and
commit what we've got so far; so I've just pushed out Pavel's stuff to

	git://linux-nfs.org/~bfields/linux.git for-2.6.37

Incremental patches on top of that welcomed.

--b.

Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

[Pavel Emelyanov]

> OK, thanks.  I think it'll be easiest to proceed if I just go ahead and
> commit what we've got so far; so I've just pushed out Pavel's stuff to
> 
> 	git://linux-nfs.org/~bfields/linux.git for-2.6.37

Thanks a lot!

> Incremental patches on top of that welcomed.
> 
> --b.
> .
> 

[PATCH 10/13] sunrpc: Merge xs_create_sock code

[Pavel Emelyanov]

After xs_bind is merged it's easy to merge its callers.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |   46 +++++++++++++++++++++++-----------------------
 1 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 3618cfd..7095471 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1592,6 +1592,14 @@ static inline void xs_reclassify_socket6(struct socket *sock)
 	sock_lock_init_class_and_name(sk, "slock-AF_INET6-RPC",
 		&xs_slock_key[1], "sk_lock-AF_INET6-RPC", &xs_key[1]);
 }
+
+static inline void xs_reclassify_socket(int family, struct socket *sock)
+{
+	if (family == PF_INET)
+		xs_reclassify_socket4(sock);
+	else
+		xs_reclassify_socket6(sock);
+}
 #else
 static inline void xs_reclassify_socket4(struct socket *sock)
 {
@@ -1600,21 +1608,25 @@ static inline void xs_reclassify_socket4(struct socket *sock)
 static inline void xs_reclassify_socket6(struct socket *sock)
 {
 }
+
+static inline void xs_reclassify_socket(int family, struct socket *sock)
+{
+}
 #endif
 
-static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
-		struct sock_xprt *transport, int type, int protocol)
+static struct socket *xs_create_sock(struct rpc_xprt *xprt,
+		struct sock_xprt *transport, int family, int type, int protocol)
 {
 	struct socket *sock;
 	int err;
 
-	err = __sock_create(xprt->xprt_net, PF_INET, type, protocol, &sock, 1);
+	err = __sock_create(xprt->xprt_net, family, type, protocol, &sock, 1);
 	if (err < 0) {
 		dprintk("RPC:       can't create %d transport socket (%d).\n",
 				protocol, -err);
 		goto out;
 	}
-	xs_reclassify_socket4(sock);
+	xs_reclassify_socket(family, sock);
 
 	if (xs_bind(transport, sock)) {
 		sock_release(sock);
@@ -1626,28 +1638,16 @@ out:
 	return ERR_PTR(err);
 }
 
-static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
+static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
 		struct sock_xprt *transport, int type, int protocol)
 {
-	struct socket *sock;
-	int err;
-
-	err = __sock_create(xprt->xprt_net, PF_INET6, type, protocol, &sock, 1);
-	if (err < 0) {
-		dprintk("RPC:       can't create %d transport socket (%d).\n",
-				protocol, -err);
-		goto out;
-	}
-	xs_reclassify_socket6(sock);
-
-	if (xs_bind(transport, sock)) {
-		sock_release(sock);
-		goto out;
-	}
+	return xs_create_sock(xprt, transport, PF_INET, type, protocol);
+}
 
-	return sock;
-out:
-	return ERR_PTR(err);
+static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
+		struct sock_xprt *transport, int type, int protocol)
+{
+	return xs_create_sock(xprt, transport, PF_INET6, type, protocol);
 }
 
 static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
-- 
1.5.5.6

[PATCH 11/13] sunrpc: Pass family to setup_socket calls

[Pavel Emelyanov]

Now we have a single socket creation routine and can call it
directly from the setup_socket routines.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |   32 ++++++++------------------------
 1 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 7095471..d4d7c98 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1638,18 +1638,6 @@ out:
 	return ERR_PTR(err);
 }
 
-static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
-		struct sock_xprt *transport, int type, int protocol)
-{
-	return xs_create_sock(xprt, transport, PF_INET, type, protocol);
-}
-
-static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
-		struct sock_xprt *transport, int type, int protocol)
-{
-	return xs_create_sock(xprt, transport, PF_INET6, type, protocol);
-}
-
 static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 {
 	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
@@ -1679,9 +1667,7 @@ static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 	xs_udp_do_set_buffer_size(xprt);
 }
 
-static void xs_udp_setup_socket(struct sock_xprt *transport,
-		struct socket *(*create_sock)(struct rpc_xprt *,
-			struct sock_xprt *, int type, int protocol))
+static void xs_udp_setup_socket(struct sock_xprt *transport, int family)
 {
 	struct rpc_xprt *xprt = &transport->xprt;
 	struct socket *sock = transport->sock;
@@ -1692,7 +1678,7 @@ static void xs_udp_setup_socket(struct sock_xprt *transport,
 
 	/* Start by resetting any existing state */
 	xs_reset_transport(transport);
-	sock = create_sock(xprt, transport, SOCK_DGRAM, IPPROTO_UDP);
+	sock = xs_create_sock(xprt, transport, family, SOCK_DGRAM, IPPROTO_UDP);
 	if (IS_ERR(sock))
 		goto out;
 
@@ -1721,7 +1707,7 @@ static void xs_udp_connect_worker4(struct work_struct *work)
 	struct sock_xprt *transport =
 		container_of(work, struct sock_xprt, connect_worker.work);
 
-	xs_udp_setup_socket(transport, xs_create_sock4);
+	xs_udp_setup_socket(transport, PF_INET);
 }
 
 /**
@@ -1736,7 +1722,7 @@ static void xs_udp_connect_worker6(struct work_struct *work)
 	struct sock_xprt *transport =
 		container_of(work, struct sock_xprt, connect_worker.work);
 
-	xs_udp_setup_socket(transport, xs_create_sock6);
+	xs_udp_setup_socket(transport, PF_INET6);
 }
 
 /*
@@ -1840,9 +1826,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
  *
  * Invoked by a work queue tasklet.
  */
-static void xs_tcp_setup_socket(struct sock_xprt *transport,
-		struct socket *(*create_sock)(struct rpc_xprt *,
-			struct sock_xprt *, int type, int protocol))
+static void xs_tcp_setup_socket(struct sock_xprt *transport, int family)
 {
 	struct socket *sock = transport->sock;
 	struct rpc_xprt *xprt = &transport->xprt;
@@ -1853,7 +1837,7 @@ static void xs_tcp_setup_socket(struct sock_xprt *transport,
 
 	if (!sock) {
 		clear_bit(XPRT_CONNECTION_ABORT, &xprt->state);
-		sock = create_sock(xprt, transport, SOCK_STREAM, IPPROTO_TCP);
+		sock = xs_create_sock(xprt, transport, family, SOCK_STREAM, IPPROTO_TCP);
 		if (IS_ERR(sock)) {
 			status = PTR_ERR(sock);
 			goto out;
@@ -1924,7 +1908,7 @@ static void xs_tcp_connect_worker4(struct work_struct *work)
 	struct sock_xprt *transport =
 		container_of(work, struct sock_xprt, connect_worker.work);
 
-	xs_tcp_setup_socket(transport, xs_create_sock4);
+	xs_tcp_setup_socket(transport, PF_INET);
 }
 
 /**
@@ -1938,7 +1922,7 @@ static void xs_tcp_connect_worker6(struct work_struct *work)
 	struct sock_xprt *transport =
 		container_of(work, struct sock_xprt, connect_worker.work);
 
-	xs_tcp_setup_socket(transport, xs_create_sock6);
+	xs_tcp_setup_socket(transport, PF_INET6);
 }
 
 /**
-- 
1.5.5.6

[PATCH 12/13] sunrpc: Remove TCP worker wrappers

[Pavel Emelyanov]

The v4 and the v6 wrappers only pass the respective family
to the xs_tcp_setup_socket. This family can be taken from the
xprt's sockaddr.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |   39 +++++++--------------------------------
 1 files changed, 7 insertions(+), 32 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index d4d7c98..4760ce8 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1826,8 +1826,10 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
  *
  * Invoked by a work queue tasklet.
  */
-static void xs_tcp_setup_socket(struct sock_xprt *transport, int family)
+static void xs_tcp_setup_socket(struct work_struct *work)
 {
+	struct sock_xprt *transport =
+		container_of(work, struct sock_xprt, connect_worker.work);
 	struct socket *sock = transport->sock;
 	struct rpc_xprt *xprt = &transport->xprt;
 	int status = -EIO;
@@ -1837,7 +1839,8 @@ static void xs_tcp_setup_socket(struct sock_xprt *transport, int family)
 
 	if (!sock) {
 		clear_bit(XPRT_CONNECTION_ABORT, &xprt->state);
-		sock = xs_create_sock(xprt, transport, family, SOCK_STREAM, IPPROTO_TCP);
+		sock = xs_create_sock(xprt, transport,
+				xs_addr(xprt)->sa_family, SOCK_STREAM, IPPROTO_TCP);
 		if (IS_ERR(sock)) {
 			status = PTR_ERR(sock);
 			goto out;
@@ -1898,34 +1901,6 @@ out:
 }
 
 /**
- * xs_tcp_connect_worker4 - connect a TCP socket to a remote endpoint
- * @work: RPC transport to connect
- *
- * Invoked by a work queue tasklet.
- */
-static void xs_tcp_connect_worker4(struct work_struct *work)
-{
-	struct sock_xprt *transport =
-		container_of(work, struct sock_xprt, connect_worker.work);
-
-	xs_tcp_setup_socket(transport, PF_INET);
-}
-
-/**
- * xs_tcp_connect_worker6 - connect a TCP socket to a remote endpoint
- * @work: RPC transport to connect
- *
- * Invoked by a work queue tasklet.
- */
-static void xs_tcp_connect_worker6(struct work_struct *work)
-{
-	struct sock_xprt *transport =
-		container_of(work, struct sock_xprt, connect_worker.work);
-
-	xs_tcp_setup_socket(transport, PF_INET6);
-}
-
-/**
  * xs_connect - connect a socket to a remote endpoint
  * @task: address of RPC task that manages state of connect request
  *
@@ -2328,7 +2303,7 @@ static struct rpc_xprt *xs_setup_tcp(struct xprt_create *args)
 			xprt_set_bound(xprt);
 
 		INIT_DELAYED_WORK(&transport->connect_worker,
-					xs_tcp_connect_worker4);
+					xs_tcp_setup_socket);
 		xs_format_peer_addresses(xprt, "tcp", RPCBIND_NETID_TCP);
 		break;
 	case AF_INET6:
@@ -2336,7 +2311,7 @@ static struct rpc_xprt *xs_setup_tcp(struct xprt_create *args)
 			xprt_set_bound(xprt);
 
 		INIT_DELAYED_WORK(&transport->connect_worker,
-					xs_tcp_connect_worker6);
+					xs_tcp_setup_socket);
 		xs_format_peer_addresses(xprt, "tcp", RPCBIND_NETID_TCP6);
 		break;
 	default:
-- 
1.5.5.6

[PATCH 13/13] sunrpc: Remove UDP worker wrappers

[Pavel Emelyanov]

Same for UDP sockets creation paths.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/xprtsock.c |   41 +++++++----------------------------------
 1 files changed, 7 insertions(+), 34 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 4760ce8..1f41f1a 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1667,8 +1667,10 @@ static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 	xs_udp_do_set_buffer_size(xprt);
 }
 
-static void xs_udp_setup_socket(struct sock_xprt *transport, int family)
+static void xs_udp_setup_socket(struct work_struct *work)
 {
+	struct sock_xprt *transport =
+		container_of(work, struct sock_xprt, connect_worker.work);
 	struct rpc_xprt *xprt = &transport->xprt;
 	struct socket *sock = transport->sock;
 	int status = -EIO;
@@ -1678,7 +1680,8 @@ static void xs_udp_setup_socket(struct sock_xprt *transport, int family)
 
 	/* Start by resetting any existing state */
 	xs_reset_transport(transport);
-	sock = xs_create_sock(xprt, transport, family, SOCK_DGRAM, IPPROTO_UDP);
+	sock = xs_create_sock(xprt, transport,
+			xs_addr(xprt)->sa_family, SOCK_DGRAM, IPPROTO_UDP);
 	if (IS_ERR(sock))
 		goto out;
 
@@ -1695,36 +1698,6 @@ out:
 	xprt_wake_pending_tasks(xprt, status);
 }
 
-/**
- * xs_udp_connect_worker4 - set up a UDP socket
- * @work: RPC transport to connect
- *
- * Invoked by a work queue tasklet.
- */
-
-static void xs_udp_connect_worker4(struct work_struct *work)
-{
-	struct sock_xprt *transport =
-		container_of(work, struct sock_xprt, connect_worker.work);
-
-	xs_udp_setup_socket(transport, PF_INET);
-}
-
-/**
- * xs_udp_connect_worker6 - set up a UDP socket
- * @work: RPC transport to connect
- *
- * Invoked by a work queue tasklet.
- */
-
-static void xs_udp_connect_worker6(struct work_struct *work)
-{
-	struct sock_xprt *transport =
-		container_of(work, struct sock_xprt, connect_worker.work);
-
-	xs_udp_setup_socket(transport, PF_INET6);
-}
-
 /*
  * We need to preserve the port number so the reply cache on the server can
  * find our cached RPC replies when we get around to reconnecting.
@@ -2229,7 +2202,7 @@ static struct rpc_xprt *xs_setup_udp(struct xprt_create *args)
 			xprt_set_bound(xprt);
 
 		INIT_DELAYED_WORK(&transport->connect_worker,
-					xs_udp_connect_worker4);
+					xs_udp_setup_socket);
 		xs_format_peer_addresses(xprt, "udp", RPCBIND_NETID_UDP);
 		break;
 	case AF_INET6:
@@ -2237,7 +2210,7 @@ static struct rpc_xprt *xs_setup_udp(struct xprt_create *args)
 			xprt_set_bound(xprt);
 
 		INIT_DELAYED_WORK(&transport->connect_worker,
-					xs_udp_connect_worker6);
+					xs_udp_setup_socket);
 		xs_format_peer_addresses(xprt, "udp", RPCBIND_NETID_UDP6);
 		break;
 	default:
-- 
1.5.5.6

Re: [PATCH 0/13] sunrpc: Compact the xprtsock code a bit

[J. Bruce Fields]

On Mon, Oct 04, 2010 at 04:50:48PM +0400, Pavel Emelyanov wrote:
> Hi!
> 
> This set removes some unused parts of code and merges together
> routines that are related to the sockets creation.
> 
> After all the changes the result is
> 
>  net/sunrpc/xprtsock.c: 77 insertions(+), 206 deletions(-)
> 
> from the code-lines point of view and
> 
>   add/remove: 2/11 grow/shrink: 4/1 up/down: 879/-1626 (-747)
> 
> from the bloat-o-meter's point of view.

I'm assuming Trond is taking these.  Let me know if that doesn't
work....

--b.

Re: [PATCH 0/13] sunrpc: Compact the xprtsock code a bit

[Pavel Emelyanov]

On 10/12/2010 03:47 AM, J. Bruce Fields wrote:
> On Mon, Oct 04, 2010 at 04:50:48PM +0400, Pavel Emelyanov wrote:
>> Hi!
>>
>> This set removes some unused parts of code and merges together
>> routines that are related to the sockets creation.
>>
>> After all the changes the result is
>>
>>  net/sunrpc/xprtsock.c: 77 insertions(+), 206 deletions(-)
>>
>> from the code-lines point of view and
>>
>>   add/remove: 2/11 grow/shrink: 4/1 up/down: 879/-1626 (-747)
>>
>> from the bloat-o-meter's point of view.
> 
> I'm assuming Trond is taking these.  Let me know if that doesn't
> work....

When will these appear in your three then? The thing is my next patches
depend on this set.

> --b.
> 

Re: [PATCH 0/13] sunrpc: Compact the xprtsock code a bit

[J. Bruce Fields]

On Tue, Oct 12, 2010 at 11:51:25AM +0400, Pavel Emelyanov wrote:
> On 10/12/2010 03:47 AM, J. Bruce Fields wrote:
> > On Mon, Oct 04, 2010 at 04:50:48PM +0400, Pavel Emelyanov wrote:
> >> Hi!
> >>
> >> This set removes some unused parts of code and merges together
> >> routines that are related to the sockets creation.
> >>
> >> After all the changes the result is
> >>
> >>  net/sunrpc/xprtsock.c: 77 insertions(+), 206 deletions(-)
> >>
> >> from the code-lines point of view and
> >>
> >>   add/remove: 2/11 grow/shrink: 4/1 up/down: 879/-1626 (-747)
> >>
> >> from the bloat-o-meter's point of view.
> > 
> > I'm assuming Trond is taking these.  Let me know if that doesn't
> > work....
> 
> When will these appear in your three then? The thing is my next patches
> depend on this set.

If Trond ACK's them I'd happily merge them into my tree.

(Or Trond could merge them, then one of us could merge the other's tree,
and you could work on top of that.)

--b.

Re: [PATCH 0/13] sunrpc: Compact the xprtsock code a bit

[Pavel Emelyanov]

>>> I'm assuming Trond is taking these.  Let me know if that doesn't
>>> work....
>>
>> When will these appear in your three then? The thing is my next patches
>> depend on this set.
> 
> If Trond ACK's them I'd happily merge them into my tree.
> 
> (Or Trond could merge them, then one of us could merge the other's tree,
> and you could work on top of that.)

OK. Then I'll wait for Trond's decision.

> --b.
> .
> 

Re: [PATCH 0/13] sunrpc: Compact the xprtsock code a bit

[J. Bruce Fields]

On Tue, Oct 12, 2010 at 07:21:31PM +0400, Pavel Emelyanov wrote:
> >>> I'm assuming Trond is taking these.  Let me know if that doesn't
> >>> work....
> >>
> >> When will these appear in your three then? The thing is my next patches
> >> depend on this set.
> > 
> > If Trond ACK's them I'd happily merge them into my tree.
> > 
> > (Or Trond could merge them, then one of us could merge the other's tree,
> > and you could work on top of that.)
> 
> OK. Then I'll wait for Trond's decision.

OK, talking to Trond, he's fine with the patches, so I'll just go ahead
and merge them, including the 3-4 additional patches you posted
afterwards.  That also means all your patches so far are in my tree so
you can continue working off that one tree if that's simplest.

I want to run some quick tests and then I'll push them out, and you can
let me know if I've forgotten any.

(Oh, and Trond also reminds me that there's still the worry about we'll
handle the keyring upcalls.  Any ideas there?)

--b.