[PATCH 0/8] rpcbind v4 support in net/sunrpc/svc*

[PATCH 0/8] rpcbind v4 support in net/sunrpc/svc*

[Chuck Lever]

Hi Bruce, Trond -

These implement basic support in net/sunrpc/svc* for registering IPv6-
enabled kernel RPC services with the local user-space rpcbind.  These
all depend on the new rpcb_v4_register() API I just posted.

The last patch enables IPv6 support for the NFSv4 callback service, even
though it doesn't register with rpcbind.

---

Chuck Lever (8):
      NFS: Enable NFSv4 callback server to listen on AF_INET6 sockets
      SUNRPC: Set V6ONLY socket option for RPC listener sockets
      SUNRPC: Add kernel build option to disable server-side use of rpcbind v3/v4
      SUNRPC: Use new rpcb_v4_register() interface in svc_register()
      SUNRPC: Clean up svc_register
      SUNRPC: Split portmap unregister API into separate function
      SUNRPC: Use proper INADDR_ANY when setting up RPC services on IPv6
      SUNRPC: Add address family field to svc_serv data structure


 fs/Kconfig                 |   22 +++++
 fs/lockd/svc.c             |    2 
 fs/nfs/callback.c          |   12 +-
 fs/nfs/callback.h          |    4 -
 fs/nfs/client.c            |    2 
 fs/nfsd/nfssvc.c           |    1 
 include/linux/sunrpc/svc.h |   11 +-
 net/sunrpc/svc.c           |  212 ++++++++++++++++++++++++++++++++++++--------
 net/sunrpc/svc_xprt.c      |   39 +++++++-
 net/sunrpc/svcsock.c       |   12 ++
 10 files changed, 260 insertions(+), 57 deletions(-)

-- 
Chuck Lever
chu ckl eve rat ora cle dot com

[PATCH 1/8] SUNRPC: Add address family field to svc_serv data structure

[Chuck Lever]

Introduce and initialize an address family field in the svc_serv structure.

This field will determine what family to use for the service's listener
sockets and what families are advertised via the local rpcbind daemon.

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

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


diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 2169af4..51bccee 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -276,7 +276,7 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
 			"lockd_up: no pid, %d users??\n", nlmsvc_users);
 
 	error = -ENOMEM;
-	serv = svc_create(&nlmsvc_program, LOCKD_BUFSIZE, NULL);
+	serv = svc_create(&nlmsvc_program, LOCKD_BUFSIZE, AF_INET, NULL);
 	if (!serv) {
 		printk(KERN_WARNING "lockd_up: create service failed\n");
 		goto out;
diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index f447f4b..6a09760 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -105,7 +105,8 @@ int nfs_callback_up(void)
 	mutex_lock(&nfs_callback_mutex);
 	if (nfs_callback_info.users++ || nfs_callback_info.task != NULL)
 		goto out;
-	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE, NULL);
+	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE,
+				AF_INET, NULL);
 	ret = -ENOMEM;
 	if (!serv)
 		goto out_err;
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 941041f..f38f47a 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -219,6 +219,7 @@ int nfsd_create_serv(void)
 	atomic_set(&nfsd_busy, 0);
 	nfsd_serv = svc_create_pooled(&nfsd_program,
 				      nfsd_max_blksize,
+				      AF_INET,
 				      nfsd_last_thread,
 				      nfsd, SIG_NOCLEAN, THIS_MODULE);
 	if (nfsd_serv == NULL)
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 4b54c5f..a27178b 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -66,6 +66,7 @@ struct svc_serv {
 	struct list_head	sv_tempsocks;	/* all temporary sockets */
 	int			sv_tmpcnt;	/* count of temporary sockets */
 	struct timer_list	sv_temptimer;	/* timer for aging temporary sockets */
+	sa_family_t		sv_family;	/* listener's address family */
 
 	char *			sv_name;	/* service name */
 
@@ -382,13 +383,13 @@ struct svc_procedure {
 /*
  * Function prototypes.
  */
-struct svc_serv *  svc_create(struct svc_program *, unsigned int,
-			      void (*shutdown)(struct svc_serv*));
+struct svc_serv *svc_create(struct svc_program *, unsigned int, sa_family_t,
+					void (*shutdown)(struct svc_serv *));
 struct svc_rqst *svc_prepare_thread(struct svc_serv *serv,
 					struct svc_pool *pool);
 void		   svc_exit_thread(struct svc_rqst *);
 struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
-			void (*shutdown)(struct svc_serv*),
+			sa_family_t, void (*shutdown)(struct svc_serv *),
 			svc_thread_fn, int sig, struct module *);
 int		   svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
 void		   svc_destroy(struct svc_serv *);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 01c7e31..d0e7865 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -366,7 +366,7 @@ svc_pool_for_cpu(struct svc_serv *serv, int cpu)
  */
 static struct svc_serv *
 __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
-	   void (*shutdown)(struct svc_serv *serv))
+	   sa_family_t family, void (*shutdown)(struct svc_serv *serv))
 {
 	struct svc_serv	*serv;
 	unsigned int vers;
@@ -375,6 +375,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
 
 	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
 		return NULL;
+	serv->sv_family    = family;
 	serv->sv_name      = prog->pg_name;
 	serv->sv_program   = prog;
 	serv->sv_nrthreads = 1;
@@ -434,21 +435,21 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
 
 struct svc_serv *
 svc_create(struct svc_program *prog, unsigned int bufsize,
-		void (*shutdown)(struct svc_serv *serv))
+		sa_family_t family, void (*shutdown)(struct svc_serv *serv))
 {
-	return __svc_create(prog, bufsize, /*npools*/1, shutdown);
+	return __svc_create(prog, bufsize, /*npools*/1, family, shutdown);
 }
 EXPORT_SYMBOL(svc_create);
 
 struct svc_serv *
 svc_create_pooled(struct svc_program *prog, unsigned int bufsize,
-		void (*shutdown)(struct svc_serv *serv),
+		  sa_family_t family, void (*shutdown)(struct svc_serv *serv),
 		  svc_thread_fn func, int sig, struct module *mod)
 {
 	struct svc_serv *serv;
 	unsigned int npools = svc_pool_map_get();
 
-	serv = __svc_create(prog, bufsize, npools, shutdown);
+	serv = __svc_create(prog, bufsize, npools, family, shutdown);
 
 	if (serv != NULL) {
 		serv->sv_function = func;

[PATCH 2/8] SUNRPC: Use proper INADDR_ANY when setting up RPC services on IPv6

[Chuck Lever]

Teach svc_create_xprt() to use the correct ANY address for AF_INET6 based
RPC services.

No caller uses AF_INET6 yet.

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

 net/sunrpc/svc_xprt.c |   39 +++++++++++++++++++++++++++++++++------
 1 files changed, 33 insertions(+), 6 deletions(-)


diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index e46c825..bf5b5cd 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -159,15 +159,44 @@ void svc_xprt_init(struct svc_xprt_class *xcl, struct svc_xprt *xprt,
 }
 EXPORT_SYMBOL_GPL(svc_xprt_init);
 
-int svc_create_xprt(struct svc_serv *serv, char *xprt_name, unsigned short port,
-		    int flags)
+static struct svc_xprt *__svc_xpo_create(struct svc_xprt_class *xcl,
+					 struct svc_serv *serv,
+					 unsigned short port, int flags)
 {
-	struct svc_xprt_class *xcl;
 	struct sockaddr_in sin = {
 		.sin_family		= AF_INET,
 		.sin_addr.s_addr	= htonl(INADDR_ANY),
 		.sin_port		= htons(port),
 	};
+	struct sockaddr_in6 sin6 = {
+		.sin6_family		= AF_INET6,
+		.sin6_addr		= IN6ADDR_ANY_INIT,
+		.sin6_port		= htons(port),
+	};
+	struct sockaddr *sap;
+	size_t len;
+
+	switch (serv->sv_family) {
+	case AF_INET:
+		sap = (struct sockaddr *)&sin;
+		len = sizeof(sin);
+		break;
+	case AF_INET6:
+		sap = (struct sockaddr *)&sin6;
+		len = sizeof(sin6);
+		break;
+	default:
+		return ERR_PTR(-EAFNOSUPPORT);
+	}
+
+	return xcl->xcl_ops->xpo_create(serv, sap, len, flags);
+}
+
+int svc_create_xprt(struct svc_serv *serv, char *xprt_name, unsigned short port,
+		    int flags)
+{
+	struct svc_xprt_class *xcl;
+
 	dprintk("svc: creating transport %s[%d]\n", xprt_name, port);
 	spin_lock(&svc_xprt_class_lock);
 	list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) {
@@ -180,9 +209,7 @@ int svc_create_xprt(struct svc_serv *serv, char *xprt_name, unsigned short port,
 			goto err;
 
 		spin_unlock(&svc_xprt_class_lock);
-		newxprt = xcl->xcl_ops->
-			xpo_create(serv, (struct sockaddr *)&sin, sizeof(sin),
-				   flags);
+		newxprt = __svc_xpo_create(xcl, serv, port, flags);
 		if (IS_ERR(newxprt)) {
 			module_put(xcl->xcl_owner);
 			return PTR_ERR(newxprt);

[PATCH 3/8] SUNRPC: Split portmap unregister API into separate function

[Chuck Lever]

Create a separate server-level interface for unregistering RPC services.

The mechanics of and the API for registering and unregistering RPC
services will diverge further as support for IPv6 is added.

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

 net/sunrpc/svc.c |   71 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 59 insertions(+), 12 deletions(-)


diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index d0e7865..a41b163 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -27,6 +27,8 @@
 
 #define RPCDBG_FACILITY	RPCDBG_SVCDSP
 
+static void svc_unregister(const struct svc_serv *serv);
+
 #define svc_serv_is_pooled(serv)    ((serv)->sv_function)
 
 /*
@@ -426,9 +428,8 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
 		spin_lock_init(&pool->sp_lock);
 	}
 
-
 	/* Remove any stale portmap registrations */
-	svc_register(serv, 0, 0);
+	svc_unregister(serv);
 
 	return serv;
 }
@@ -496,8 +497,7 @@ svc_destroy(struct svc_serv *serv)
 	if (svc_serv_is_pooled(serv))
 		svc_pool_map_put();
 
-	/* Unregister service with the portmapper */
-	svc_register(serv, 0, 0);
+	svc_unregister(serv);
 	kfree(serv->sv_pools);
 	kfree(serv);
 }
@@ -758,12 +758,10 @@ int
 svc_register(struct svc_serv *serv, int proto, unsigned short port)
 {
 	struct svc_program	*progp;
-	unsigned long		flags;
 	unsigned int		i;
 	int			error = 0, dummy;
 
-	if (!port)
-		clear_thread_flag(TIF_SIGPENDING);
+	BUG_ON(proto == 0 && port == 0);
 
 	for (progp = serv->sv_program; progp; progp = progp->pg_next) {
 		for (i = 0; i < progp->pg_nvers; i++) {
@@ -791,13 +789,62 @@ svc_register(struct svc_serv *serv, int proto, unsigned short port)
 		}
 	}
 
-	if (!port) {
-		spin_lock_irqsave(&current->sighand->siglock, flags);
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, flags);
+	return error;
+}
+
+/*
+ * The local rpcbind daemon listens on either only IPv6 or only
+ * IPv4.  The kernel can't tell how it's configured.
+ *
+ * However, AF_INET addresses are mapped to AF_INET6 in IPv6-only
+ * configurations, so even an unregistration request on AF_INET
+ * will get to a local rpcbind daemon listening only on AF_INET6.
+ *
+ * So we always unregister via AF_INET (the loopback address is
+ * fairly unambiguous anyway).
+ *
+ * At this point we don't need rpcbind version 4 for unregistration:
+ * A v2 UNSET request will clear all transports (netids), addresses,
+ * and address families for [program, version].
+ *
+ * This should allow automatic support for both an all-IPv4 and
+ * an all-IPv6 configuration.
+ */
+static void __svc_unregister(struct svc_program *program, u32 version)
+{
+	int error, boolean;
+
+	error = rpcb_register(program->pg_prog, version, 0, 0, &boolean);
+	dprintk("svc: svc_unregister(%sv%u), error %d, %s\n",
+			program->pg_name, version, error,
+			(boolean ? "succeeded" : "failed"));
+}
+
+/*
+ * All transport protocols and ports for this service are removed from
+ * the local rpcbind database.  The result of unregistration is reported
+ * via dprintk for those who want verification of the result, but is
+ * otherwise not important.
+ */
+static void svc_unregister(const struct svc_serv *serv)
+{
+	struct svc_program *program;
+	unsigned long flags;
+	u32 version;
+
+	clear_thread_flag(TIF_SIGPENDING);
+
+	for (program = serv->sv_program; program; program = program->pg_next) {
+		for (version = 0; version < program->pg_nvers; version++) {
+			if (program->pg_vers[version] == NULL)
+				continue;
+			__svc_unregister(program, version);
+		}
 	}
 
-	return error;
+	spin_lock_irqsave(&current->sighand->siglock, flags);
+	recalc_sigpending();
+	spin_unlock_irqrestore(&current->sighand->siglock, flags);
 }
 
 /*

[PATCH 4/8] SUNRPC: Clean up svc_register

[Chuck Lever]

In preparation for adding rpcbind version 4 support to svc_register, clean
it up.

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

 include/linux/sunrpc/svc.h |    4 ++-
 net/sunrpc/svc.c           |   61 +++++++++++++++++++++++++++-----------------
 2 files changed, 41 insertions(+), 24 deletions(-)


diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index a27178b..05312e7 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -394,7 +394,9 @@ struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
 int		   svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
 void		   svc_destroy(struct svc_serv *);
 int		   svc_process(struct svc_rqst *);
-int		   svc_register(struct svc_serv *, int, unsigned short);
+int		   svc_register(const struct svc_serv *, const unsigned short,
+				const unsigned short);
+
 void		   svc_wake_up(struct svc_serv *);
 void		   svc_reserve(struct svc_rqst *rqstp, int space);
 struct svc_pool *  svc_pool_for_cpu(struct svc_serv *serv, int cpu);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index a41b163..c31a1e4 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -749,43 +749,58 @@ svc_exit_thread(struct svc_rqst *rqstp)
 }
 EXPORT_SYMBOL(svc_exit_thread);
 
-/*
- * Register an RPC service with the local portmapper.
- * To unregister a service, call this routine with
- * proto and port == 0.
+static int __svc_register(const u32 program, const u32 version,
+			  sa_family_t family,
+			  const unsigned short protocol,
+			  const unsigned short port)
+{
+	int error, result;
+
+	error = rpcb_register(program, version, protocol, port, &result);
+	if (!result)
+		error = -EACCES;
+	return error;
+}
+
+/**
+ * svc_register - register an RPC service with the local portmapper
+ * @serv: svc_serv struct for the service to register
+ * @protocol: transport protocol number to advertise
+ * @port: port to advertise
+ *
  */
-int
-svc_register(struct svc_serv *serv, int proto, unsigned short port)
+int svc_register(const struct svc_serv *serv, const unsigned short protocol,
+		 const unsigned short port)
 {
-	struct svc_program	*progp;
-	unsigned int		i;
-	int			error = 0, dummy;
+	struct svc_program *progp;
+	u32 version;
+	int error = 0;
 
-	BUG_ON(proto == 0 && port == 0);
+	BUG_ON(protocol == 0 && port == 0);
 
 	for (progp = serv->sv_program; progp; progp = progp->pg_next) {
-		for (i = 0; i < progp->pg_nvers; i++) {
-			if (progp->pg_vers[i] == NULL)
+		for (version = 0; version < progp->pg_nvers; version++) {
+			if (progp->pg_vers[version] == NULL)
 				continue;
 
-			dprintk("svc: svc_register(%s, %s, %d, %d)%s\n",
+			dprintk("svc: svc_register(%s, %u, %u, %s, %u)%s\n",
 					progp->pg_name,
-					proto == IPPROTO_UDP?  "udp" : "tcp",
+					version,
+					serv->sv_family,
+					protocol == IPPROTO_UDP ?
+							"udp" : "tcp",
 					port,
-					i,
-					progp->pg_vers[i]->vs_hidden?
-						" (but not telling portmap)" : "");
+					progp->pg_vers[version]->vs_hidden ?
+					  " (but not telling portmap)" : "");
 
-			if (progp->pg_vers[i]->vs_hidden)
+			if (progp->pg_vers[version]->vs_hidden)
 				continue;
 
-			error = rpcb_register(progp->pg_prog, i, proto, port, &dummy);
+			error = __svc_register(progp->pg_prog, version,
+						serv->sv_family, protocol,
+						port);
 			if (error < 0)
 				break;
-			if (port && !dummy) {
-				error = -EACCES;
-				break;
-			}
 		}
 	}
 

[PATCH 5/8] SUNRPC: Use new rpcb_v4_register() interface in svc_register()

[Chuck Lever]

In order to advertise NFS-related services on IPv6 interfaces via
rpcbind, the kernel RPC server implementation must use
rpcb_v4_register() instead of rpcb_register().

Legacy support (register using portmapper/rpcbindv2) is still available.
The new rpcbind version is not used unless enabled via a CONFIG option.

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

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


diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index c31a1e4..13cca03 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -749,6 +749,74 @@ svc_exit_thread(struct svc_rqst *rqstp)
 }
 EXPORT_SYMBOL(svc_exit_thread);
 
+#ifdef CONFIG_SUNRPC_REGISTER_V4
+/*
+ * Registering kernel RPC services with rpcbind version 2 will work
+ * over either IPv4 or IPv6, since the Linux kernel always registers
+ * services for the "any" address.
+ *
+ * However, the local rpcbind daemon listens on either only AF_INET
+ * or AF_INET6 (never both).  When it listens on AF_INET6, an rpcbind
+ * version 2 registration will result in registering the service at
+ * IN6ADDR_ANY, even if the RPC service being registered is not
+ * IPv6-enabled.
+ *
+ * Rpcbind version 4 allows us to be a little more specific.  Kernel
+ * RPC services that don't yet support AF_INET6 can register
+ * themselves as IPv4-only with the local rpcbind daemon, even if the
+ * daemon is listening only on AF_INET6.
+ *
+ * And, registering IPv6-enabled kernel RPC services via AF_INET6
+ * verifies that the local user space rpcbind daemon is properly
+ * configured to support remote AF_INET6 rpcbind requests.
+ *
+ * An AF_INET6 registration request will fail if the local rpcbind
+ * daemon is not set up to listen on AF_INET6.  Likewise, we fail
+ * AF_INET6 registration requests if svc_register() is configured to
+ * support only rpcbind version 2.
+ */
+static int __svc_register(const u32 program, const u32 version,
+			  const sa_family_t family,
+			  const unsigned short protocol,
+			  const unsigned short port)
+{
+	struct sockaddr_in sin = {
+		.sin_family		= AF_INET,
+		.sin_addr.s_addr	= htonl(INADDR_ANY),
+		.sin_port		= htons(port),
+	};
+	struct sockaddr_in6 sin6 = {
+		.sin6_family		= AF_INET6,
+		.sin6_addr		= IN6ADDR_ANY_INIT,
+		.sin6_port		= htons(port),
+	};
+	struct sockaddr *sap;
+	char *netid;
+	int error, result;
+
+	switch (family) {
+	case AF_INET:
+		sap = (struct sockaddr *)&sin;
+		netid = RPCBIND_NETID_TCP;
+		if (protocol == IPPROTO_UDP)
+			netid = RPCBIND_NETID_UDP;
+		break;
+	case AF_INET6:
+		sap = (struct sockaddr *)&sin6;
+		netid = RPCBIND_NETID_TCP6;
+		if (protocol == IPPROTO_UDP)
+			netid = RPCBIND_NETID_UDP6;
+		break;
+	default:
+		return -EAFNOSUPPORT;
+	}
+
+	error = rpcb_v4_register(program, version, sap, netid, &result);
+	if (!result)
+		error = -EACCES;
+	return error;
+}
+#else
 static int __svc_register(const u32 program, const u32 version,
 			  sa_family_t family,
 			  const unsigned short protocol,
@@ -756,11 +824,15 @@ static int __svc_register(const u32 program, const u32 version,
 {
 	int error, result;
 
+	if (family != AF_INET)
+		return -EAFNOSUPPORT;
+
 	error = rpcb_register(program, version, protocol, port, &result);
 	if (!result)
 		error = -EACCES;
 	return error;
 }
+#endif
 
 /**
  * svc_register - register an RPC service with the local portmapper
@@ -768,6 +840,7 @@ static int __svc_register(const u32 program, const u32 version,
  * @protocol: transport protocol number to advertise
  * @port: port to advertise
  *
+ * Service is registered for any address in serv's address family
  */
 int svc_register(const struct svc_serv *serv, const unsigned short protocol,
 		 const unsigned short port)

[PATCH 6/8] SUNRPC: Add kernel build option to disable server-side use of rpcbind v3/v4

[Chuck Lever]

Allow distributions to use the legacy behavior until they integrate an
appropriate user-space rpcbind daemon that can support IPv6 RPC services.

I tried adding some logic to fall back if (un)registering with a v4
protocol request failed, but there are too many corner cases.  So I just
made it a compile-time switch that distributions can throw when they've
replaced portmapper with rpcbind.

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

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


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

[PATCH 7/8] SUNRPC: Set V6ONLY socket option for RPC listener sockets

[Chuck Lever]

My plan is to use an AF_INET listener on systems that support only IPv4,
and an AF_INET6 listener on systems that can support IPv6. Incoming
IPv4 packets will be posted to an AF_INET6 listener with a mapped IPv4
address.

Max Matveev <makc@sgi.com> says:
  Creating a single listener can be dangerous - if net.ipv6.bindv6only
  is enabled then it's possible to create another listener in v4
  namespace on the same port and steal the traffic from the "unifed"
  listener. You need to disable V6ONLY explicitly via a sockopt to stop
  that.

Set appropriate socket option on RPC server listener sockets to prevent
this.

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

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


diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 3e65719..43c21f7 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1114,6 +1114,7 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
 	struct svc_sock	*svsk;
 	struct sock	*inet;
 	int		pmap_register = !(flags & SVC_SOCK_ANONYMOUS);
+	int zero	= 0;
 
 	dprintk("svc: svc_setup_socket %p\n", sock);
 	if (!(svsk = kzalloc(sizeof(*svsk), GFP_KERNEL))) {
@@ -1146,6 +1147,17 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
 	else
 		svc_tcp_init(svsk, serv);
 
+	/*
+	 * We start one listener per sv_serv.  We want AF_INET
+	 * requests to be automatically shunted to our AF_INET6
+	 * listener using a mapped IPv4 address.  Make sure
+	 * no-one starts an equivalent IPv4 listener, which
+	 * would steal our incoming connections.
+	 */
+	if (serv->sv_family == AF_INET6)
+		kernel_setsockopt(sock, SOL_IPV6, IPV6_V6ONLY,
+					(char *)&zero, sizeof(zero));
+
 	dprintk("svc: svc_setup_socket created %p (inet %p)\n",
 				svsk, svsk->sk_sk);
 

[PATCH 8/8] NFS: Enable NFSv4 callback server to listen on AF_INET6 sockets

[Chuck Lever]

Allow the NFS callback server to listen for requests via an AF_INET6 or
AF_INET socket.

The NFS callback server determines the listener's address family from the
address the client uses to contact the server.  The server will always
call the client back using the same address family.

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

 fs/nfs/callback.c |   11 ++++++-----
 fs/nfs/callback.h |    4 ++--
 fs/nfs/client.c   |    2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)


diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 6a09760..59948ef 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -97,7 +97,7 @@ nfs_callback_svc(void *vrqstp)
 /*
  * Bring up the callback thread if it is not already up.
  */
-int nfs_callback_up(void)
+int nfs_callback_up(const sa_family_t family)
 {
 	struct svc_serv *serv = NULL;
 	int ret = 0;
@@ -106,7 +106,7 @@ int nfs_callback_up(void)
 	if (nfs_callback_info.users++ || nfs_callback_info.task != NULL)
 		goto out;
 	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE,
-				AF_INET, NULL);
+				family, NULL);
 	ret = -ENOMEM;
 	if (!serv)
 		goto out_err;
@@ -116,7 +116,8 @@ int nfs_callback_up(void)
 	if (ret <= 0)
 		goto out_err;
 	nfs_callback_tcpport = ret;
-	dprintk("Callback port = 0x%x\n", nfs_callback_tcpport);
+	dprintk("NFS: Callback listener port = %u (af %u)\n",
+			nfs_callback_tcpport, family);
 
 	nfs_callback_info.rqst = svc_prepare_thread(serv, &serv->sv_pools[0]);
 	if (IS_ERR(nfs_callback_info.rqst)) {
@@ -149,8 +150,8 @@ out:
 	mutex_unlock(&nfs_callback_mutex);
 	return ret;
 out_err:
-	dprintk("Couldn't create callback socket or server thread; err = %d\n",
-		ret);
+	dprintk("NFS: Couldn't create callback socket or server thread; "
+		"err = %d\n", ret);
 	nfs_callback_info.users--;
 	goto out;
 }
diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
index bb25d21..85d8102 100644
--- a/fs/nfs/callback.h
+++ b/fs/nfs/callback.h
@@ -63,10 +63,10 @@ extern __be32 nfs4_callback_getattr(struct cb_getattrargs *args, struct cb_getat
 extern __be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy);
 
 #ifdef CONFIG_NFS_V4
-extern int nfs_callback_up(void);
+extern int nfs_callback_up(const sa_family_t family);
 extern void nfs_callback_down(void);
 #else
-#define nfs_callback_up()	(0)
+#define nfs_callback_up(x)	(0)
 #define nfs_callback_down()	do {} while(0)
 #endif
 
diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index f2a092c..f1bb2af 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -120,7 +120,7 @@ static struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_
 	clp->rpc_ops = cl_init->rpc_ops;
 
 	if (cl_init->rpc_ops->version == 4) {
-		if (nfs_callback_up() < 0)
+		if (nfs_callback_up(cl_init->addr->sa_family) < 0)
 			goto error_2;
 		__set_bit(NFS_CS_CALLBACK, &clp->cl_res_state);
 	}

Re: [PATCH 1/8] SUNRPC: Add address family field to svc_serv data structure

[J. Bruce Fields]

On Mon, Jun 30, 2008 at 06:45:30PM -0400, Chuck Lever wrote:
> Introduce and initialize an address family field in the svc_serv structure.
> 
> This field will determine what family to use for the service's listener
> sockets and what families are advertised via the local rpcbind daemon.

Possibly dumb question: so it never makes sense to listen on sockets
with both address families?

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/svc.c             |    2 +-
>  fs/nfs/callback.c          |    3 ++-
>  fs/nfsd/nfssvc.c           |    1 +
>  include/linux/sunrpc/svc.h |    7 ++++---
>  net/sunrpc/svc.c           |   11 ++++++-----
>  5 files changed, 14 insertions(+), 10 deletions(-)
> 
> 
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index 2169af4..51bccee 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -276,7 +276,7 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
>  			"lockd_up: no pid, %d users??\n", nlmsvc_users);
>  
>  	error = -ENOMEM;
> -	serv = svc_create(&nlmsvc_program, LOCKD_BUFSIZE, NULL);
> +	serv = svc_create(&nlmsvc_program, LOCKD_BUFSIZE, AF_INET, NULL);
>  	if (!serv) {
>  		printk(KERN_WARNING "lockd_up: create service failed\n");
>  		goto out;
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index f447f4b..6a09760 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -105,7 +105,8 @@ int nfs_callback_up(void)
>  	mutex_lock(&nfs_callback_mutex);
>  	if (nfs_callback_info.users++ || nfs_callback_info.task != NULL)
>  		goto out;
> -	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE, NULL);
> +	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE,
> +				AF_INET, NULL);
>  	ret = -ENOMEM;
>  	if (!serv)
>  		goto out_err;
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 941041f..f38f47a 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -219,6 +219,7 @@ int nfsd_create_serv(void)
>  	atomic_set(&nfsd_busy, 0);
>  	nfsd_serv = svc_create_pooled(&nfsd_program,
>  				      nfsd_max_blksize,
> +				      AF_INET,
>  				      nfsd_last_thread,
>  				      nfsd, SIG_NOCLEAN, THIS_MODULE);
>  	if (nfsd_serv == NULL)
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 4b54c5f..a27178b 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -66,6 +66,7 @@ struct svc_serv {
>  	struct list_head	sv_tempsocks;	/* all temporary sockets */
>  	int			sv_tmpcnt;	/* count of temporary sockets */
>  	struct timer_list	sv_temptimer;	/* timer for aging temporary sockets */
> +	sa_family_t		sv_family;	/* listener's address family */
>  
>  	char *			sv_name;	/* service name */
>  
> @@ -382,13 +383,13 @@ struct svc_procedure {
>  /*
>   * Function prototypes.
>   */
> -struct svc_serv *  svc_create(struct svc_program *, unsigned int,
> -			      void (*shutdown)(struct svc_serv*));
> +struct svc_serv *svc_create(struct svc_program *, unsigned int, sa_family_t,
> +					void (*shutdown)(struct svc_serv *));
>  struct svc_rqst *svc_prepare_thread(struct svc_serv *serv,
>  					struct svc_pool *pool);
>  void		   svc_exit_thread(struct svc_rqst *);
>  struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
> -			void (*shutdown)(struct svc_serv*),
> +			sa_family_t, void (*shutdown)(struct svc_serv *),
>  			svc_thread_fn, int sig, struct module *);
>  int		   svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
>  void		   svc_destroy(struct svc_serv *);
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 01c7e31..d0e7865 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -366,7 +366,7 @@ svc_pool_for_cpu(struct svc_serv *serv, int cpu)
>   */
>  static struct svc_serv *
>  __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> -	   void (*shutdown)(struct svc_serv *serv))
> +	   sa_family_t family, void (*shutdown)(struct svc_serv *serv))
>  {
>  	struct svc_serv	*serv;
>  	unsigned int vers;
> @@ -375,6 +375,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>  
>  	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
>  		return NULL;
> +	serv->sv_family    = family;
>  	serv->sv_name      = prog->pg_name;
>  	serv->sv_program   = prog;
>  	serv->sv_nrthreads = 1;
> @@ -434,21 +435,21 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>  
>  struct svc_serv *
>  svc_create(struct svc_program *prog, unsigned int bufsize,
> -		void (*shutdown)(struct svc_serv *serv))
> +		sa_family_t family, void (*shutdown)(struct svc_serv *serv))
>  {
> -	return __svc_create(prog, bufsize, /*npools*/1, shutdown);
> +	return __svc_create(prog, bufsize, /*npools*/1, family, shutdown);
>  }
>  EXPORT_SYMBOL(svc_create);
>  
>  struct svc_serv *
>  svc_create_pooled(struct svc_program *prog, unsigned int bufsize,
> -		void (*shutdown)(struct svc_serv *serv),
> +		  sa_family_t family, void (*shutdown)(struct svc_serv *serv),
>  		  svc_thread_fn func, int sig, struct module *mod)
>  {
>  	struct svc_serv *serv;
>  	unsigned int npools = svc_pool_map_get();
>  
> -	serv = __svc_create(prog, bufsize, npools, shutdown);
> +	serv = __svc_create(prog, bufsize, npools, family, shutdown);
>  
>  	if (serv != NULL) {
>  		serv->sv_function = func;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] SUNRPC: Add address family field to svc_serv data structure

[Chuck Lever]

On Thu, Jul 3, 2008 at 5:14 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Mon, Jun 30, 2008 at 06:45:30PM -0400, Chuck Lever wrote:
>> Introduce and initialize an address family field in the svc_serv structure.
>>
>> This field will determine what family to use for the service's listener
>> sockets and what families are advertised via the local rpcbind daemon.
>
> Possibly dumb question: so it never makes sense to listen on sockets
> with both address families?

There are several ways to map an AF_INET address to an AF_INET6
address, so you can easily set up a single AF_INET6 listener that can
also handle your AF_INET traffic.

You can listen on both, but I think having a single listener is less complex.

-- 
Chuck Lever

Re: [PATCH 3/8] SUNRPC: Split portmap unregister API into separate function

[J. Bruce Fields]

On Mon, Jun 30, 2008 at 06:45:45PM -0400, Chuck Lever wrote:
> Create a separate server-level interface for unregistering RPC services.
> 
> The mechanics of and the API for registering and unregistering RPC
> services will diverge further as support for IPv6 is added.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  net/sunrpc/svc.c |   71 +++++++++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 59 insertions(+), 12 deletions(-)
> 
> 
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index d0e7865..a41b163 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -27,6 +27,8 @@
>  
>  #define RPCDBG_FACILITY	RPCDBG_SVCDSP
>  
> +static void svc_unregister(const struct svc_serv *serv);
> +
>  #define svc_serv_is_pooled(serv)    ((serv)->sv_function)
>  
>  /*
> @@ -426,9 +428,8 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>  		spin_lock_init(&pool->sp_lock);
>  	}
>  
> -
>  	/* Remove any stale portmap registrations */
> -	svc_register(serv, 0, 0);
> +	svc_unregister(serv);
>  
>  	return serv;
>  }
> @@ -496,8 +497,7 @@ svc_destroy(struct svc_serv *serv)
>  	if (svc_serv_is_pooled(serv))
>  		svc_pool_map_put();
>  
> -	/* Unregister service with the portmapper */
> -	svc_register(serv, 0, 0);
> +	svc_unregister(serv);
>  	kfree(serv->sv_pools);
>  	kfree(serv);
>  }
> @@ -758,12 +758,10 @@ int
>  svc_register(struct svc_serv *serv, int proto, unsigned short port)
>  {
>  	struct svc_program	*progp;
> -	unsigned long		flags;
>  	unsigned int		i;
>  	int			error = 0, dummy;
>  
> -	if (!port)
> -		clear_thread_flag(TIF_SIGPENDING);
> +	BUG_ON(proto == 0 && port == 0);
>  
>  	for (progp = serv->sv_program; progp; progp = progp->pg_next) {
>  		for (i = 0; i < progp->pg_nvers; i++) {
> @@ -791,13 +789,62 @@ svc_register(struct svc_serv *serv, int proto, unsigned short port)
>  		}
>  	}
>  
> -	if (!port) {
> -		spin_lock_irqsave(&current->sighand->siglock, flags);
> -		recalc_sigpending();
> -		spin_unlock_irqrestore(&current->sighand->siglock, flags);
> +	return error;
> +}

The "port" in the (port && !dummy) check in this loop should also go.

> +
> +/*
> + * The local rpcbind daemon listens on either only IPv6 or only
> + * IPv4.  The kernel can't tell how it's configured.
> + *
> + * However, AF_INET addresses are mapped to AF_INET6 in IPv6-only
> + * configurations, so even an unregistration request on AF_INET
> + * will get to a local rpcbind daemon listening only on AF_INET6.
> + *
> + * So we always unregister via AF_INET (the loopback address is
> + * fairly unambiguous anyway).
> + *
> + * At this point we don't need rpcbind version 4 for unregistration:
> + * A v2 UNSET request will clear all transports (netids), addresses,
> + * and address families for [program, version].
> + *
> + * This should allow automatic support for both an all-IPv4 and
> + * an all-IPv6 configuration.
> + */
> +static void __svc_unregister(struct svc_program *program, u32 version)
> +{
> +	int error, boolean;
> +
> +	error = rpcb_register(program->pg_prog, version, 0, 0, &boolean);
> +	dprintk("svc: svc_unregister(%sv%u), error %d, %s\n",
> +			program->pg_name, version, error,
> +			(boolean ? "succeeded" : "failed"));
> +}
> +
> +/*
> + * All transport protocols and ports for this service are removed from
> + * the local rpcbind database.  The result of unregistration is reported
> + * via dprintk for those who want verification of the result, but is
> + * otherwise not important.
> + */
> +static void svc_unregister(const struct svc_serv *serv)
> +{
> +	struct svc_program *program;
> +	unsigned long flags;
> +	u32 version;

It may just be brain-damage from too many years of mathematics, but
let's leave this as "i" as before: its scope is only a few lines, its
meaning is obvious from use, and this is what CodingStyle asks for
anyway.

> +
> +	clear_thread_flag(TIF_SIGPENDING);
> +
> +	for (program = serv->sv_program; program; program = program->pg_next) {
> +		for (version = 0; version < program->pg_nvers; version++) {
> +			if (program->pg_vers[version] == NULL)
> +				continue;
> +			__svc_unregister(program, version);

Isn't there a change in behavior from the omitted vs_hidden check?
I assume it's harmless to unregister something that was never
registered (if that's indeed what this does), but it seems better to
skip it.

Needs a comment in the changelog in any case.

> +		}
>  	}
>  
> -	return error;
> +	spin_lock_irqsave(&current->sighand->siglock, flags);
> +	recalc_sigpending();
> +	spin_unlock_irqrestore(&current->sighand->siglock, flags);
>  }
>  
>  /*
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/8] SUNRPC: Clean up svc_register

[J. Bruce Fields]

On Mon, Jun 30, 2008 at 06:45:53PM -0400, Chuck Lever wrote:
> In preparation for adding rpcbind version 4 support to svc_register, clean
> it up.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  include/linux/sunrpc/svc.h |    4 ++-
>  net/sunrpc/svc.c           |   61 +++++++++++++++++++++++++++-----------------
>  2 files changed, 41 insertions(+), 24 deletions(-)
> 
> 
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index a27178b..05312e7 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -394,7 +394,9 @@ struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
>  int		   svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
>  void		   svc_destroy(struct svc_serv *);
>  int		   svc_process(struct svc_rqst *);
> -int		   svc_register(struct svc_serv *, int, unsigned short);
> +int		   svc_register(const struct svc_serv *, const unsigned short,
> +				const unsigned short);
> +
>  void		   svc_wake_up(struct svc_serv *);
>  void		   svc_reserve(struct svc_rqst *rqstp, int space);
>  struct svc_pool *  svc_pool_for_cpu(struct svc_serv *serv, int cpu);
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index a41b163..c31a1e4 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -749,43 +749,58 @@ svc_exit_thread(struct svc_rqst *rqstp)
>  }
>  EXPORT_SYMBOL(svc_exit_thread);
>  
> -/*
> - * Register an RPC service with the local portmapper.
> - * To unregister a service, call this routine with
> - * proto and port == 0.
> +static int __svc_register(const u32 program, const u32 version,
> +			  sa_family_t family,
> +			  const unsigned short protocol,
> +			  const unsigned short port)
> +{
> +	int error, result;
> +
> +	error = rpcb_register(program, version, protocol, port, &result);
> +	if (!result)
> +		error = -EACCES;
> +	return error;
> +}
> +
> +/**
> + * svc_register - register an RPC service with the local portmapper
> + * @serv: svc_serv struct for the service to register
> + * @protocol: transport protocol number to advertise
> + * @port: port to advertise
> + *
>   */
> -int
> -svc_register(struct svc_serv *serv, int proto, unsigned short port)
> +int svc_register(const struct svc_serv *serv, const unsigned short protocol,
> +		 const unsigned short port)
>  {
> -	struct svc_program	*progp;
> -	unsigned int		i;
> -	int			error = 0, dummy;
> +	struct svc_program *progp;
> +	u32 version;

Same style nit (i/version) as before.  (Also: u32?  If we ever have to
care how big the array index is here, we've got bigger problems....)

I don't really see "protocol" as much clearer than "proto" either, but
OK.

Seems good otherwise.

--b.

> +	int error = 0;
>  
> -	BUG_ON(proto == 0 && port == 0);
> +	BUG_ON(protocol == 0 && port == 0);
>  
>  	for (progp = serv->sv_program; progp; progp = progp->pg_next) {
> -		for (i = 0; i < progp->pg_nvers; i++) {
> -			if (progp->pg_vers[i] == NULL)
> +		for (version = 0; version < progp->pg_nvers; version++) {
> +			if (progp->pg_vers[version] == NULL)
>  				continue;
>  
> -			dprintk("svc: svc_register(%s, %s, %d, %d)%s\n",
> +			dprintk("svc: svc_register(%s, %u, %u, %s, %u)%s\n",
>  					progp->pg_name,
> -					proto == IPPROTO_UDP?  "udp" : "tcp",
> +					version,
> +					serv->sv_family,
> +					protocol == IPPROTO_UDP ?
> +							"udp" : "tcp",
>  					port,
> -					i,
> -					progp->pg_vers[i]->vs_hidden?
> -						" (but not telling portmap)" : "");
> +					progp->pg_vers[version]->vs_hidden ?
> +					  " (but not telling portmap)" : "");
>  
> -			if (progp->pg_vers[i]->vs_hidden)
> +			if (progp->pg_vers[version]->vs_hidden)
>  				continue;
>  
> -			error = rpcb_register(progp->pg_prog, i, proto, port, &dummy);
> +			error = __svc_register(progp->pg_prog, version,
> +						serv->sv_family, protocol,
> +						port);
>  			if (error < 0)
>  				break;
> -			if (port && !dummy) {
> -				error = -EACCES;
> -				break;
> -			}
>  		}
>  	}
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/8] SUNRPC: Use new rpcb_v4_register() interface in svc_register()

[J. Bruce Fields]

On Mon, Jun 30, 2008 at 06:46:01PM -0400, Chuck Lever wrote:
> In order to advertise NFS-related services on IPv6 interfaces via
> rpcbind, the kernel RPC server implementation must use
> rpcb_v4_register() instead of rpcb_register().
> 
> Legacy support (register using portmapper/rpcbindv2) is still available.
> The new rpcbind version is not used unless enabled via a CONFIG option.

So a kernel built with CONFIG_SUNRPC_REGISTER_V4 will no longer support
old userspace?

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  net/sunrpc/svc.c |   73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 73 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index c31a1e4..13cca03 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -749,6 +749,74 @@ svc_exit_thread(struct svc_rqst *rqstp)
>  }
>  EXPORT_SYMBOL(svc_exit_thread);
>  
> +#ifdef CONFIG_SUNRPC_REGISTER_V4
> +/*
> + * Registering kernel RPC services with rpcbind version 2 will work
> + * over either IPv4 or IPv6, since the Linux kernel always registers
> + * services for the "any" address.
> + *
> + * However, the local rpcbind daemon listens on either only AF_INET
> + * or AF_INET6 (never both).  When it listens on AF_INET6, an rpcbind
> + * version 2 registration will result in registering the service at
> + * IN6ADDR_ANY, even if the RPC service being registered is not
> + * IPv6-enabled.
> + *
> + * Rpcbind version 4 allows us to be a little more specific.  Kernel
> + * RPC services that don't yet support AF_INET6 can register
> + * themselves as IPv4-only with the local rpcbind daemon, even if the
> + * daemon is listening only on AF_INET6.
> + *
> + * And, registering IPv6-enabled kernel RPC services via AF_INET6
> + * verifies that the local user space rpcbind daemon is properly
> + * configured to support remote AF_INET6 rpcbind requests.
> + *
> + * An AF_INET6 registration request will fail if the local rpcbind
> + * daemon is not set up to listen on AF_INET6.  Likewise, we fail
> + * AF_INET6 registration requests if svc_register() is configured to
> + * support only rpcbind version 2.
> + */
> +static int __svc_register(const u32 program, const u32 version,
> +			  const sa_family_t family,
> +			  const unsigned short protocol,
> +			  const unsigned short port)
> +{
> +	struct sockaddr_in sin = {
> +		.sin_family		= AF_INET,
> +		.sin_addr.s_addr	= htonl(INADDR_ANY),
> +		.sin_port		= htons(port),
> +	};
> +	struct sockaddr_in6 sin6 = {
> +		.sin6_family		= AF_INET6,
> +		.sin6_addr		= IN6ADDR_ANY_INIT,
> +		.sin6_port		= htons(port),
> +	};
> +	struct sockaddr *sap;
> +	char *netid;
> +	int error, result;
> +
> +	switch (family) {
> +	case AF_INET:
> +		sap = (struct sockaddr *)&sin;
> +		netid = RPCBIND_NETID_TCP;
> +		if (protocol == IPPROTO_UDP)
> +			netid = RPCBIND_NETID_UDP;
> +		break;
> +	case AF_INET6:
> +		sap = (struct sockaddr *)&sin6;
> +		netid = RPCBIND_NETID_TCP6;
> +		if (protocol == IPPROTO_UDP)
> +			netid = RPCBIND_NETID_UDP6;
> +		break;
> +	default:
> +		return -EAFNOSUPPORT;
> +	}
> +
> +	error = rpcb_v4_register(program, version, sap, netid, &result);
> +	if (!result)
> +		error = -EACCES;
> +	return error;
> +}
> +#else
>  static int __svc_register(const u32 program, const u32 version,
>  			  sa_family_t family,
>  			  const unsigned short protocol,
> @@ -756,11 +824,15 @@ static int __svc_register(const u32 program, const u32 version,
>  {
>  	int error, result;
>  
> +	if (family != AF_INET)
> +		return -EAFNOSUPPORT;
> +
>  	error = rpcb_register(program, version, protocol, port, &result);
>  	if (!result)
>  		error = -EACCES;
>  	return error;
>  }
> +#endif
>  
>  /**
>   * svc_register - register an RPC service with the local portmapper
> @@ -768,6 +840,7 @@ static int __svc_register(const u32 program, const u32 version,
>   * @protocol: transport protocol number to advertise
>   * @port: port to advertise
>   *
> + * Service is registered for any address in serv's address family
>   */
>  int svc_register(const struct svc_serv *serv, const unsigned short protocol,
>  		 const unsigned short port)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] SUNRPC: Add kernel build option to disable server-side use of rpcbind v3/v4

[J. Bruce Fields]

On Mon, Jun 30, 2008 at 06:46:09PM -0400, Chuck Lever wrote:
> Allow distributions to use the legacy behavior until they integrate an
> appropriate user-space rpcbind daemon that can support IPv6 RPC services.
> 
> I tried adding some logic to fall back if (un)registering with a v4
> protocol request failed, but there are too many corner cases.  So I just
> made it a compile-time switch that distributions can throw when they've
> replaced portmapper with rpcbind.

OK, I see you answered my last question....  Hm.  What were the corner
cases?

--b.


> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/Kconfig |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/fs/Kconfig b/fs/Kconfig
> index 8cf8ee2..6148f4c 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -1799,6 +1799,28 @@ config SUNRPC_XPRT_RDMA
>  
>  	  If unsure, say N.
>  
> +config SUNRPC_REGISTER_V4
> +	bool "Register local RPC services via rpcbind v4 (EXPERIMENTAL)"
> +	depends on SUNRPC && EXPERIMENTAL
> +	default n
> +	help 
> +	  Sun added support for registering RPC services at an IPv6
> +	  address by creating two new versions of the rpcbind protocol
> +	  (RFC 1833).
> +
> +	  This option enables support in the kernel RPC server for
> +	  registering kernel RPC services via version 4 of the rpcbind
> +	  protocol.  If you enable this option, you must run a portmapper
> +	  daemon that supports rpcbind protocol version 4.
> +
> +	  Serving NFS over IPv6 from knfsd (the kernel's NFS server)
> +	  requires that you enable this option and use a portmapper that
> +	  supports rpcbind version 4.
> +
> +	  If unsure, say N to get traditional behavior (register kernel
> +	  RPC services using only rpcbind version 2).  Distributions
> +	  using the legacy Linux portmapper daemon must say N here.
> +
>  config RPCSEC_GSS_KRB5
>  	tristate "Secure RPC: Kerberos V mechanism (EXPERIMENTAL)"
>  	depends on SUNRPC && EXPERIMENTAL
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] SUNRPC: Set V6ONLY socket option for RPC listener sockets

[J. Bruce Fields]

On Mon, Jun 30, 2008 at 06:46:17PM -0400, Chuck Lever wrote:
> My plan is to use an AF_INET listener on systems that support only IPv4,
> and an AF_INET6 listener on systems that can support IPv6. Incoming
> IPv4 packets will be posted to an AF_INET6 listener with a mapped IPv4
> address.
> 
> Max Matveev <makc@sgi.com> says:
>   Creating a single listener can be dangerous - if net.ipv6.bindv6only
>   is enabled then it's possible to create another listener in v4
>   namespace on the same port and steal the traffic from the "unifed"
>   listener. You need to disable V6ONLY explicitly via a sockopt to stop
>   that.

Is the V6ONLY option documented anywhere?  A quick grep through the
kernel sources and a couple man pages didn't turn up anything.

--b.

> 
> Set appropriate socket option on RPC server listener sockets to prevent
> this.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  net/sunrpc/svcsock.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 3e65719..43c21f7 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -1114,6 +1114,7 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
>  	struct svc_sock	*svsk;
>  	struct sock	*inet;
>  	int		pmap_register = !(flags & SVC_SOCK_ANONYMOUS);
> +	int zero	= 0;
>  
>  	dprintk("svc: svc_setup_socket %p\n", sock);
>  	if (!(svsk = kzalloc(sizeof(*svsk), GFP_KERNEL))) {
> @@ -1146,6 +1147,17 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
>  	else
>  		svc_tcp_init(svsk, serv);
>  
> +	/*
> +	 * We start one listener per sv_serv.  We want AF_INET
> +	 * requests to be automatically shunted to our AF_INET6
> +	 * listener using a mapped IPv4 address.  Make sure
> +	 * no-one starts an equivalent IPv4 listener, which
> +	 * would steal our incoming connections.
> +	 */
> +	if (serv->sv_family == AF_INET6)
> +		kernel_setsockopt(sock, SOL_IPV6, IPV6_V6ONLY,
> +					(char *)&zero, sizeof(zero));
> +
>  	dprintk("svc: svc_setup_socket created %p (inet %p)\n",
>  				svsk, svsk->sk_sk);
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/8] rpcbind v4 support in net/sunrpc/svc*

[J. Bruce Fields]

On Mon, Jun 30, 2008 at 06:45:21PM -0400, Chuck Lever wrote:
> Hi Bruce, Trond -
> 
> These implement basic support in net/sunrpc/svc* for registering IPv6-
> enabled kernel RPC services with the local user-space rpcbind.  These
> all depend on the new rpcb_v4_register() API I just posted.
> 
> The last patch enables IPv6 support for the NFSv4 callback service, even
> though it doesn't register with rpcbind.

There's some conflicts against my latest tree.  Would it be possible to
rebase to

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

sometime?

--b.

> 
> ---
> 
> Chuck Lever (8):
>       NFS: Enable NFSv4 callback server to listen on AF_INET6 sockets
>       SUNRPC: Set V6ONLY socket option for RPC listener sockets
>       SUNRPC: Add kernel build option to disable server-side use of rpcbind v3/v4
>       SUNRPC: Use new rpcb_v4_register() interface in svc_register()
>       SUNRPC: Clean up svc_register
>       SUNRPC: Split portmap unregister API into separate function
>       SUNRPC: Use proper INADDR_ANY when setting up RPC services on IPv6
>       SUNRPC: Add address family field to svc_serv data structure
> 
> 
>  fs/Kconfig                 |   22 +++++
>  fs/lockd/svc.c             |    2 
>  fs/nfs/callback.c          |   12 +-
>  fs/nfs/callback.h          |    4 -
>  fs/nfs/client.c            |    2 
>  fs/nfsd/nfssvc.c           |    1 
>  include/linux/sunrpc/svc.h |   11 +-
>  net/sunrpc/svc.c           |  212 ++++++++++++++++++++++++++++++++++++--------
>  net/sunrpc/svc_xprt.c      |   39 +++++++-
>  net/sunrpc/svcsock.c       |   12 ++
>  10 files changed, 260 insertions(+), 57 deletions(-)
> 
> -- 
> Chuck Lever
> chu ckl eve rat ora cle dot com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/8] rpcbind v4 support in net/sunrpc/svc*

[J. Bruce Fields]

On Fri, Jul 18, 2008 at 09:07:38PM -0400, J. Bruce Fields wrote:
> On Mon, Jun 30, 2008 at 06:45:21PM -0400, Chuck Lever wrote:
> > Hi Bruce, Trond -
> > 
> > These implement basic support in net/sunrpc/svc* for registering IPv6-
> > enabled kernel RPC services with the local user-space rpcbind.  These
> > all depend on the new rpcb_v4_register() API I just posted.
> > 
> > The last patch enables IPv6 support for the NFSv4 callback service, even
> > though it doesn't register with rpcbind.
> 
> There's some conflicts against my latest tree.  Would it be possible to
> rebase to
> 
> 	git://linux-nfs.org/~bfields/linux.git for-2.6.27
> 
> sometime?

Actually, never mind, I fixed up the conflicts in the first couple and
staged them here:

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

(I think the rest'll still apply cleanly to that.)

We could maybe still try to get the rest of this in at the end of the
2.6.27 merge window, but I'm inclined to wait.  Apologies, partly my
fault for not getting around to these sooner.

--b.

Re: [PATCH 3/8] SUNRPC: Split portmap unregister API into separate function

[Chuck Lever]

On Fri, Jul 18, 2008 at 7:21 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Mon, Jun 30, 2008 at 06:45:45PM -0400, Chuck Lever wrote:
>> Create a separate server-level interface for unregistering RPC services.
>>
>> The mechanics of and the API for registering and unregistering RPC
>> services will diverge further as support for IPv6 is added.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>>  net/sunrpc/svc.c |   71 +++++++++++++++++++++++++++++++++++++++++++++---------
>>  1 files changed, 59 insertions(+), 12 deletions(-)
>>
>>
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index d0e7865..a41b163 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -27,6 +27,8 @@
>>
>>  #define RPCDBG_FACILITY      RPCDBG_SVCDSP
>>
>> +static void svc_unregister(const struct svc_serv *serv);
>> +
>>  #define svc_serv_is_pooled(serv)    ((serv)->sv_function)
>>
>>  /*
>> @@ -426,9 +428,8 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>>               spin_lock_init(&pool->sp_lock);
>>       }
>>
>> -
>>       /* Remove any stale portmap registrations */
>> -     svc_register(serv, 0, 0);
>> +     svc_unregister(serv);
>>
>>       return serv;
>>  }
>> @@ -496,8 +497,7 @@ svc_destroy(struct svc_serv *serv)
>>       if (svc_serv_is_pooled(serv))
>>               svc_pool_map_put();
>>
>> -     /* Unregister service with the portmapper */
>> -     svc_register(serv, 0, 0);
>> +     svc_unregister(serv);
>>       kfree(serv->sv_pools);
>>       kfree(serv);
>>  }
>> @@ -758,12 +758,10 @@ int
>>  svc_register(struct svc_serv *serv, int proto, unsigned short port)
>>  {
>>       struct svc_program      *progp;
>> -     unsigned long           flags;
>>       unsigned int            i;
>>       int                     error = 0, dummy;
>>
>> -     if (!port)
>> -             clear_thread_flag(TIF_SIGPENDING);
>> +     BUG_ON(proto == 0 && port == 0);
>>
>>       for (progp = serv->sv_program; progp; progp = progp->pg_next) {
>>               for (i = 0; i < progp->pg_nvers; i++) {
>> @@ -791,13 +789,62 @@ svc_register(struct svc_serv *serv, int proto, unsigned short port)
>>               }
>>       }
>>
>> -     if (!port) {
>> -             spin_lock_irqsave(&current->sighand->siglock, flags);
>> -             recalc_sigpending();
>> -             spin_unlock_irqrestore(&current->sighand->siglock, flags);
>> +     return error;
>> +}
>
> The "port" in the (port && !dummy) check in this loop should also go.

If this patch were by itself, yes.  But all this is cleaned out in the
next subsequent patch.  I don't think it makes a difference here,
unless you think there is a good possibility these patches will be
separated.

>
>> +
>> +/*
>> + * The local rpcbind daemon listens on either only IPv6 or only
>> + * IPv4.  The kernel can't tell how it's configured.
>> + *
>> + * However, AF_INET addresses are mapped to AF_INET6 in IPv6-only
>> + * configurations, so even an unregistration request on AF_INET
>> + * will get to a local rpcbind daemon listening only on AF_INET6.
>> + *
>> + * So we always unregister via AF_INET (the loopback address is
>> + * fairly unambiguous anyway).
>> + *
>> + * At this point we don't need rpcbind version 4 for unregistration:
>> + * A v2 UNSET request will clear all transports (netids), addresses,
>> + * and address families for [program, version].
>> + *
>> + * This should allow automatic support for both an all-IPv4 and
>> + * an all-IPv6 configuration.
>> + */
>> +static void __svc_unregister(struct svc_program *program, u32 version)
>> +{
>> +     int error, boolean;
>> +
>> +     error = rpcb_register(program->pg_prog, version, 0, 0, &boolean);
>> +     dprintk("svc: svc_unregister(%sv%u), error %d, %s\n",
>> +                     program->pg_name, version, error,
>> +                     (boolean ? "succeeded" : "failed"));
>> +}
>> +
>> +/*
>> + * All transport protocols and ports for this service are removed from
>> + * the local rpcbind database.  The result of unregistration is reported
>> + * via dprintk for those who want verification of the result, but is
>> + * otherwise not important.
>> + */
>> +static void svc_unregister(const struct svc_serv *serv)
>> +{
>> +     struct svc_program *program;
>> +     unsigned long flags;
>> +     u32 version;
>
> It may just be brain-damage from too many years of mathematics, but
> let's leave this as "i" as before: its scope is only a few lines, its
> meaning is obvious from use, and this is what CodingStyle asks for
> anyway.

It may seem like a small thing, but I must disagree here.  I assume
you are quibbling with the new name only and not the type change.

My reading of CodingStyle Chapter 4 is that "i" is appropriate instead
of "tmp" or "x" or "index" -- in other words where you need a generic
iterator.  It doesn't require the name "i" for _all_ loop iterators.
I certainly wouldn't use "i" if I were iterating over characters or
addresses.

In mathematics (as you well know), "i, j, k" are used as subscripts or
for sequences or summations; often they refer to _every_ possible
value.  We don't have any of that here.  We are passing in RPC version
numbers.  These may not even be in sequence:  mountd has versions 1,
3, and 4, but not 2, nor 5 and above.

Any modern structured programming text recommends that we should name
the variable something that reflects its use.  "i" is really quite
generic; "version" is "short and to the point," as Chapter 4
recommends.

[ "vers" is perhaps more concise, but I think nothing but ambiguity is
gained from dropping the last three letters.  "lovers" could easily be
"low version" or "star-crossed lovers", for example].

Over the past several kernel releases I've included patches that
change variables storing RPC version numbers to "u32 version" wherever
they are used.  I really don't see the need to be different here, and
I'd rather be consistent with nearly every other usage.  If you're
storing an RPC version number, it is a u32 field or variable called
"version."  The type and the name match what is in the RFCs.

>
>> +
>> +     clear_thread_flag(TIF_SIGPENDING);
>> +
>> +     for (program = serv->sv_program; program; program = program->pg_next) {
>> +             for (version = 0; version < program->pg_nvers; version++) {
>> +                     if (program->pg_vers[version] == NULL)
>> +                             continue;
>> +                     __svc_unregister(program, version);
>
> Isn't there a change in behavior from the omitted vs_hidden check?
> I assume it's harmless to unregister something that was never
> registered (if that's indeed what this does), but it seems better to
> skip it.

svc_unregister() is used in svc_create() before registering a new
service, and in svc_destroy() when unregistering a service being shut
down.

It's advisable to do this now even for so-called hidden services
because of the ability for rpcbind to advertise RPC services at
particular addresses.  The kernel registers an RPC service for the ANY
address, so all addresses for that service that are already registered
should be removed first.

Perhaps for hidden services, svc_unregister() should warn loudly or
fail immediately as a safety precaution, as these services should not
have been registered already, and if they are, we may be colliding
with something in user space.

> Needs a comment in the changelog in any case.

OK.

>
>> +             }
>>       }
>>
>> -     return error;
>> +     spin_lock_irqsave(&current->sighand->siglock, flags);
>> +     recalc_sigpending();
>> +     spin_unlock_irqrestore(&current->sighand->siglock, flags);
>>  }
>>
>>  /*

-- 
Chuck Lever

Re: [PATCH 0/8] rpcbind v4 support in net/sunrpc/svc*

[Chuck Lever]

On Sun, Jul 20, 2008 at 5:17 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Fri, Jul 18, 2008 at 09:07:38PM -0400, J. Bruce Fields wrote:
>> On Mon, Jun 30, 2008 at 06:45:21PM -0400, Chuck Lever wrote:
>> > Hi Bruce, Trond -
>> >
>> > These implement basic support in net/sunrpc/svc* for registering IPv6-
>> > enabled kernel RPC services with the local user-space rpcbind.  These
>> > all depend on the new rpcb_v4_register() API I just posted.
>> >
>> > The last patch enables IPv6 support for the NFSv4 callback service, even
>> > though it doesn't register with rpcbind.
>>
>> There's some conflicts against my latest tree.  Would it be possible to
>> rebase to
>>
>>       git://linux-nfs.org/~bfields/linux.git for-2.6.27
>>
>> sometime?
>
> Actually, never mind, I fixed up the conflicts in the first couple and
> staged them here:
>
>        git://linux-nfs.org/~bfields/linux.git for-2.6.28
>
> (I think the rest'll still apply cleanly to that.)
>
> We could maybe still try to get the rest of this in at the end of the
> 2.6.27 merge window, but I'm inclined to wait.  Apologies, partly my
> fault for not getting around to these sooner.

Given the complexity and controversy of these changes, the number of
other changes that went into .27 and will require us to rewrite some
of these, and the pending holiday/travel for our team of maintainers,
I think waiting for .28 is our only choice.

-- 
 "Alright guard, begin the unnecessarily slow-moving dipping mechanism."
--Dr. Evil

Re: [PATCH 4/8] SUNRPC: Clean up svc_register

[Chuck Lever]

On Fri, Jul 18, 2008 at 7:29 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Mon, Jun 30, 2008 at 06:45:53PM -0400, Chuck Lever wrote:
>> In preparation for adding rpcbind version 4 support to svc_register, clean
>> it up.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>>  include/linux/sunrpc/svc.h |    4 ++-
>>  net/sunrpc/svc.c           |   61 +++++++++++++++++++++++++++-----------------
>>  2 files changed, 41 insertions(+), 24 deletions(-)
>>
>>
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index a27178b..05312e7 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -394,7 +394,9 @@ struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
>>  int             svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
>>  void            svc_destroy(struct svc_serv *);
>>  int             svc_process(struct svc_rqst *);
>> -int             svc_register(struct svc_serv *, int, unsigned short);
>> +int             svc_register(const struct svc_serv *, const unsigned short,
>> +                             const unsigned short);
>> +
>>  void            svc_wake_up(struct svc_serv *);
>>  void            svc_reserve(struct svc_rqst *rqstp, int space);
>>  struct svc_pool *  svc_pool_for_cpu(struct svc_serv *serv, int cpu);
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index a41b163..c31a1e4 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -749,43 +749,58 @@ svc_exit_thread(struct svc_rqst *rqstp)
>>  }
>>  EXPORT_SYMBOL(svc_exit_thread);
>>
>> -/*
>> - * Register an RPC service with the local portmapper.
>> - * To unregister a service, call this routine with
>> - * proto and port == 0.
>> +static int __svc_register(const u32 program, const u32 version,
>> +                       sa_family_t family,
>> +                       const unsigned short protocol,
>> +                       const unsigned short port)
>> +{
>> +     int error, result;
>> +
>> +     error = rpcb_register(program, version, protocol, port, &result);
>> +     if (!result)
>> +             error = -EACCES;
>> +     return error;
>> +}
>> +
>> +/**
>> + * svc_register - register an RPC service with the local portmapper
>> + * @serv: svc_serv struct for the service to register
>> + * @protocol: transport protocol number to advertise
>> + * @port: port to advertise
>> + *
>>   */
>> -int
>> -svc_register(struct svc_serv *serv, int proto, unsigned short port)
>> +int svc_register(const struct svc_serv *serv, const unsigned short protocol,
>> +              const unsigned short port)
>>  {
>> -     struct svc_program      *progp;
>> -     unsigned int            i;
>> -     int                     error = 0, dummy;
>> +     struct svc_program *progp;
>> +     u32 version;
>
> Same style nit (i/version) as before.

It doesn't make any sense to me to name "proto", "port", "progp", and
even "vs_hidden" after their usage, but leave the RPC version number
as "i".

>  (Also: u32?  If we ever have to
> care how big the array index is here, we've got bigger problems....)

The array index is already bounds-checked by the for() statement, so
the range of the index variable should be inconsequential, right?

"u32" is a better type for an RPC version number than "int".

The RPC header is defined in RFC 1831, and the version field is
defined as an XDR unsigned int.  In RFC 1832, an XDR unsigned int is
defined as a 32-bit value.  Since on some hardware platforms an "int"
is not 32 bits, "u32" is the safest choice, and matches the Internet
standard definition of an RPC version number.  (__be32 is, of course,
the correct choice for an RPC version number that has already been
marshaled).

And, as mentioned before, I have already changed function arguments,
variables, and structure fields in other areas that contain RPC
version numbers to u32.  I would like to stay consistent, and
eliminate unnecessary implicit type conversions.

Thanks for the review.

> I don't really see "protocol" as much clearer than "proto" either, but
> OK.
>
> Seems good otherwise.
>
> --b.
>
>> +     int error = 0;
>>
>> -     BUG_ON(proto == 0 && port == 0);
>> +     BUG_ON(protocol == 0 && port == 0);
>>
>>       for (progp = serv->sv_program; progp; progp = progp->pg_next) {
>> -             for (i = 0; i < progp->pg_nvers; i++) {
>> -                     if (progp->pg_vers[i] == NULL)
>> +             for (version = 0; version < progp->pg_nvers; version++) {
>> +                     if (progp->pg_vers[version] == NULL)
>>                               continue;
>>
>> -                     dprintk("svc: svc_register(%s, %s, %d, %d)%s\n",
>> +                     dprintk("svc: svc_register(%s, %u, %u, %s, %u)%s\n",
>>                                       progp->pg_name,
>> -                                     proto == IPPROTO_UDP?  "udp" : "tcp",
>> +                                     version,
>> +                                     serv->sv_family,
>> +                                     protocol == IPPROTO_UDP ?
>> +                                                     "udp" : "tcp",
>>                                       port,
>> -                                     i,
>> -                                     progp->pg_vers[i]->vs_hidden?
>> -                                             " (but not telling portmap)" : "");
>> +                                     progp->pg_vers[version]->vs_hidden ?
>> +                                       " (but not telling portmap)" : "");
>>
>> -                     if (progp->pg_vers[i]->vs_hidden)
>> +                     if (progp->pg_vers[version]->vs_hidden)
>>                               continue;
>>
>> -                     error = rpcb_register(progp->pg_prog, i, proto, port, &dummy);
>> +                     error = __svc_register(progp->pg_prog, version,
>> +                                             serv->sv_family, protocol,
>> +                                             port);
>>                       if (error < 0)
>>                               break;
>> -                     if (port && !dummy) {
>> -                             error = -EACCES;
>> -                             break;
>> -                     }
>>               }
>>       }
>>
>>

-- 
Chuck Lever

Re: [PATCH 5/8] SUNRPC: Use new rpcb_v4_register() interface in svc_register()

[Chuck Lever]

On Fri, Jul 18, 2008 at 7:40 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Mon, Jun 30, 2008 at 06:46:01PM -0400, Chuck Lever wrote:
>> In order to advertise NFS-related services on IPv6 interfaces via
>> rpcbind, the kernel RPC server implementation must use
>> rpcb_v4_register() instead of rpcb_register().
>>
>> Legacy support (register using portmapper/rpcbindv2) is still available.
>> The new rpcbind version is not used unless enabled via a CONFIG option.
>
> So a kernel built with CONFIG_SUNRPC_REGISTER_V4 will no longer support
> old userspace?

Precisely speaking it will require that user space is running rpcbind
(or some other rpcbind-like daemon that support version 4 of the
rpcbind protocol) instead of the legacy portmapper.

>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>>  net/sunrpc/svc.c |   73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 73 insertions(+), 0 deletions(-)
>>
>>
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index c31a1e4..13cca03 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -749,6 +749,74 @@ svc_exit_thread(struct svc_rqst *rqstp)
>>  }
>>  EXPORT_SYMBOL(svc_exit_thread);
>>
>> +#ifdef CONFIG_SUNRPC_REGISTER_V4
>> +/*
>> + * Registering kernel RPC services with rpcbind version 2 will work
>> + * over either IPv4 or IPv6, since the Linux kernel always registers
>> + * services for the "any" address.
>> + *
>> + * However, the local rpcbind daemon listens on either only AF_INET
>> + * or AF_INET6 (never both).  When it listens on AF_INET6, an rpcbind
>> + * version 2 registration will result in registering the service at
>> + * IN6ADDR_ANY, even if the RPC service being registered is not
>> + * IPv6-enabled.
>> + *
>> + * Rpcbind version 4 allows us to be a little more specific.  Kernel
>> + * RPC services that don't yet support AF_INET6 can register
>> + * themselves as IPv4-only with the local rpcbind daemon, even if the
>> + * daemon is listening only on AF_INET6.
>> + *
>> + * And, registering IPv6-enabled kernel RPC services via AF_INET6
>> + * verifies that the local user space rpcbind daemon is properly
>> + * configured to support remote AF_INET6 rpcbind requests.
>> + *
>> + * An AF_INET6 registration request will fail if the local rpcbind
>> + * daemon is not set up to listen on AF_INET6.  Likewise, we fail
>> + * AF_INET6 registration requests if svc_register() is configured to
>> + * support only rpcbind version 2.
>> + */
>> +static int __svc_register(const u32 program, const u32 version,
>> +                       const sa_family_t family,
>> +                       const unsigned short protocol,
>> +                       const unsigned short port)
>> +{
>> +     struct sockaddr_in sin = {
>> +             .sin_family             = AF_INET,
>> +             .sin_addr.s_addr        = htonl(INADDR_ANY),
>> +             .sin_port               = htons(port),
>> +     };
>> +     struct sockaddr_in6 sin6 = {
>> +             .sin6_family            = AF_INET6,
>> +             .sin6_addr              = IN6ADDR_ANY_INIT,
>> +             .sin6_port              = htons(port),
>> +     };
>> +     struct sockaddr *sap;
>> +     char *netid;
>> +     int error, result;
>> +
>> +     switch (family) {
>> +     case AF_INET:
>> +             sap = (struct sockaddr *)&sin;
>> +             netid = RPCBIND_NETID_TCP;
>> +             if (protocol == IPPROTO_UDP)
>> +                     netid = RPCBIND_NETID_UDP;
>> +             break;
>> +     case AF_INET6:
>> +             sap = (struct sockaddr *)&sin6;
>> +             netid = RPCBIND_NETID_TCP6;
>> +             if (protocol == IPPROTO_UDP)
>> +                     netid = RPCBIND_NETID_UDP6;
>> +             break;
>> +     default:
>> +             return -EAFNOSUPPORT;
>> +     }
>> +
>> +     error = rpcb_v4_register(program, version, sap, netid, &result);
>> +     if (!result)
>> +             error = -EACCES;
>> +     return error;
>> +}
>> +#else
>>  static int __svc_register(const u32 program, const u32 version,
>>                         sa_family_t family,
>>                         const unsigned short protocol,
>> @@ -756,11 +824,15 @@ static int __svc_register(const u32 program, const u32 version,
>>  {
>>       int error, result;
>>
>> +     if (family != AF_INET)
>> +             return -EAFNOSUPPORT;
>> +
>>       error = rpcb_register(program, version, protocol, port, &result);
>>       if (!result)
>>               error = -EACCES;
>>       return error;
>>  }
>> +#endif
>>
>>  /**
>>   * svc_register - register an RPC service with the local portmapper
>> @@ -768,6 +840,7 @@ static int __svc_register(const u32 program, const u32 version,
>>   * @protocol: transport protocol number to advertise
>>   * @port: port to advertise
>>   *
>> + * Service is registered for any address in serv's address family
>>   */
>>  int svc_register(const struct svc_serv *serv, const unsigned short protocol,
>>                const unsigned short port)
>>

-- 
Chuck Lever

Re: [PATCH 6/8] SUNRPC: Add kernel build option to disable server-side use of rpcbind v3/v4

[Chuck Lever]

On Fri, Jul 18, 2008 at 7:42 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Mon, Jun 30, 2008 at 06:46:09PM -0400, Chuck Lever wrote:
>> Allow distributions to use the legacy behavior until they integrate an
>> appropriate user-space rpcbind daemon that can support IPv6 RPC services.
>>
>> I tried adding some logic to fall back if (un)registering with a v4
>> protocol request failed, but there are too many corner cases.  So I just
>> made it a compile-time switch that distributions can throw when they've
>> replaced portmapper with rpcbind.
>
> OK, I see you answered my last question....  Hm.  What were the corner
> cases?

The obvious ones:  is there a daemon running at all, and is it
listening?  If I try v4 but that doesn't work, then I retry with v3...
if that doesn't work, then I try v2.  Is it OK if I was able to
register an AF_INET6 service via v2?

Certainly all this is possible to code up, but it didn't seem worth
the complexity.

>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>>  fs/Kconfig |   22 ++++++++++++++++++++++
>>  1 files changed, 22 insertions(+), 0 deletions(-)
>>
>>
>> diff --git a/fs/Kconfig b/fs/Kconfig
>> index 8cf8ee2..6148f4c 100644
>> --- a/fs/Kconfig
>> +++ b/fs/Kconfig
>> @@ -1799,6 +1799,28 @@ config SUNRPC_XPRT_RDMA
>>
>>         If unsure, say N.
>>
>> +config SUNRPC_REGISTER_V4
>> +     bool "Register local RPC services via rpcbind v4 (EXPERIMENTAL)"
>> +     depends on SUNRPC && EXPERIMENTAL
>> +     default n
>> +     help
>> +       Sun added support for registering RPC services at an IPv6
>> +       address by creating two new versions of the rpcbind protocol
>> +       (RFC 1833).
>> +
>> +       This option enables support in the kernel RPC server for
>> +       registering kernel RPC services via version 4 of the rpcbind
>> +       protocol.  If you enable this option, you must run a portmapper
>> +       daemon that supports rpcbind protocol version 4.
>> +
>> +       Serving NFS over IPv6 from knfsd (the kernel's NFS server)
>> +       requires that you enable this option and use a portmapper that
>> +       supports rpcbind version 4.
>> +
>> +       If unsure, say N to get traditional behavior (register kernel
>> +       RPC services using only rpcbind version 2).  Distributions
>> +       using the legacy Linux portmapper daemon must say N here.
>> +
>>  config RPCSEC_GSS_KRB5
>>       tristate "Secure RPC: Kerberos V mechanism (EXPERIMENTAL)"
>>       depends on SUNRPC && EXPERIMENTAL
>>
>> --
>> 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
> --
> 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
>



-- 
 "Alright guard, begin the unnecessarily slow-moving dipping mechanism."
--Dr. Evil

Re: [PATCH 7/8] SUNRPC: Set V6ONLY socket option for RPC listener sockets

[Chuck Lever]

On Fri, Jul 18, 2008 at 9:05 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Mon, Jun 30, 2008 at 06:46:17PM -0400, Chuck Lever wrote:
>> My plan is to use an AF_INET listener on systems that support only IPv4,
>> and an AF_INET6 listener on systems that can support IPv6. Incoming
>> IPv4 packets will be posted to an AF_INET6 listener with a mapped IPv4
>> address.
>>
>> Max Matveev <makc@sgi.com> says:
>>   Creating a single listener can be dangerous - if net.ipv6.bindv6only
>>   is enabled then it's possible to create another listener in v4
>>   namespace on the same port and steal the traffic from the "unifed"
>>   listener. You need to disable V6ONLY explicitly via a sockopt to stop
>>   that.
>
> Is the V6ONLY option documented anywhere?  A quick grep through the
> kernel sources and a couple man pages didn't turn up anything.

I didn't find anything that documented it, so I based this patch on
studying pieces of the IPv6 code that use this flag.  It would be
worth passing this patch by netdev.

>>
>> Set appropriate socket option on RPC server listener sockets to prevent
>> this.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>>  net/sunrpc/svcsock.c |   12 ++++++++++++
>>  1 files changed, 12 insertions(+), 0 deletions(-)
>>
>>
>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>> index 3e65719..43c21f7 100644
>> --- a/net/sunrpc/svcsock.c
>> +++ b/net/sunrpc/svcsock.c
>> @@ -1114,6 +1114,7 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
>>       struct svc_sock *svsk;
>>       struct sock     *inet;
>>       int             pmap_register = !(flags & SVC_SOCK_ANONYMOUS);
>> +     int zero        = 0;
>>
>>       dprintk("svc: svc_setup_socket %p\n", sock);
>>       if (!(svsk = kzalloc(sizeof(*svsk), GFP_KERNEL))) {
>> @@ -1146,6 +1147,17 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
>>       else
>>               svc_tcp_init(svsk, serv);
>>
>> +     /*
>> +      * We start one listener per sv_serv.  We want AF_INET
>> +      * requests to be automatically shunted to our AF_INET6
>> +      * listener using a mapped IPv4 address.  Make sure
>> +      * no-one starts an equivalent IPv4 listener, which
>> +      * would steal our incoming connections.
>> +      */
>> +     if (serv->sv_family == AF_INET6)
>> +             kernel_setsockopt(sock, SOL_IPV6, IPV6_V6ONLY,
>> +                                     (char *)&zero, sizeof(zero));
>> +
>>       dprintk("svc: svc_setup_socket created %p (inet %p)\n",
>>                               svsk, svsk->sk_sk);
>>
>>
>> --
>> 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
> --
> 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
>



-- 
 "Alright guard, begin the unnecessarily slow-moving dipping mechanism."
--Dr. Evil

Re: [PATCH 3/8] SUNRPC: Split portmap unregister API into separate function

[J. Bruce Fields]

On Sun, Jul 20, 2008 at 11:17:02PM -0400, Chuck Lever wrote:
> On Fri, Jul 18, 2008 at 7:21 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Mon, Jun 30, 2008 at 06:45:45PM -0400, Chuck Lever wrote:
> >> Create a separate server-level interface for unregistering RPC services.
> >>
> >> The mechanics of and the API for registering and unregistering RPC
> >> services will diverge further as support for IPv6 is added.
> >>
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >>
> >>  net/sunrpc/svc.c |   71 +++++++++++++++++++++++++++++++++++++++++++++---------
> >>  1 files changed, 59 insertions(+), 12 deletions(-)
> >>
> >>
> >> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> >> index d0e7865..a41b163 100644
> >> --- a/net/sunrpc/svc.c
> >> +++ b/net/sunrpc/svc.c
> >> @@ -27,6 +27,8 @@
> >>
> >>  #define RPCDBG_FACILITY      RPCDBG_SVCDSP
> >>
> >> +static void svc_unregister(const struct svc_serv *serv);
> >> +
> >>  #define svc_serv_is_pooled(serv)    ((serv)->sv_function)
> >>
> >>  /*
> >> @@ -426,9 +428,8 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> >>               spin_lock_init(&pool->sp_lock);
> >>       }
> >>
> >> -
> >>       /* Remove any stale portmap registrations */
> >> -     svc_register(serv, 0, 0);
> >> +     svc_unregister(serv);
> >>
> >>       return serv;
> >>  }
> >> @@ -496,8 +497,7 @@ svc_destroy(struct svc_serv *serv)
> >>       if (svc_serv_is_pooled(serv))
> >>               svc_pool_map_put();
> >>
> >> -     /* Unregister service with the portmapper */
> >> -     svc_register(serv, 0, 0);
> >> +     svc_unregister(serv);
> >>       kfree(serv->sv_pools);
> >>       kfree(serv);
> >>  }
> >> @@ -758,12 +758,10 @@ int
> >>  svc_register(struct svc_serv *serv, int proto, unsigned short port)
> >>  {
> >>       struct svc_program      *progp;
> >> -     unsigned long           flags;
> >>       unsigned int            i;
> >>       int                     error = 0, dummy;
> >>
> >> -     if (!port)
> >> -             clear_thread_flag(TIF_SIGPENDING);
> >> +     BUG_ON(proto == 0 && port == 0);
> >>
> >>       for (progp = serv->sv_program; progp; progp = progp->pg_next) {
> >>               for (i = 0; i < progp->pg_nvers; i++) {
> >> @@ -791,13 +789,62 @@ svc_register(struct svc_serv *serv, int proto, unsigned short port)
> >>               }
> >>       }
> >>
> >> -     if (!port) {
> >> -             spin_lock_irqsave(&current->sighand->siglock, flags);
> >> -             recalc_sigpending();
> >> -             spin_unlock_irqrestore(&current->sighand->siglock, flags);
> >> +     return error;
> >> +}
> >
> > The "port" in the (port && !dummy) check in this loop should also go.
> 
> If this patch were by itself, yes.  But all this is cleaned out in the
> next subsequent patch.  I don't think it makes a difference here,
> unless you think there is a good possibility these patches will be
> separated.

Yeah, I hadn't noticed that you caught that in the next patch, thanks.

That change does logically belong in this patch, though.

> >> +/*
> >> + * The local rpcbind daemon listens on either only IPv6 or only
> >> + * IPv4.  The kernel can't tell how it's configured.
> >> + *
> >> + * However, AF_INET addresses are mapped to AF_INET6 in IPv6-only
> >> + * configurations, so even an unregistration request on AF_INET
> >> + * will get to a local rpcbind daemon listening only on AF_INET6.
> >> + *
> >> + * So we always unregister via AF_INET (the loopback address is
> >> + * fairly unambiguous anyway).
> >> + *
> >> + * At this point we don't need rpcbind version 4 for unregistration:
> >> + * A v2 UNSET request will clear all transports (netids), addresses,
> >> + * and address families for [program, version].
> >> + *
> >> + * This should allow automatic support for both an all-IPv4 and
> >> + * an all-IPv6 configuration.
> >> + */
> >> +static void __svc_unregister(struct svc_program *program, u32 version)
> >> +{
> >> +     int error, boolean;
> >> +
> >> +     error = rpcb_register(program->pg_prog, version, 0, 0, &boolean);
> >> +     dprintk("svc: svc_unregister(%sv%u), error %d, %s\n",
> >> +                     program->pg_name, version, error,
> >> +                     (boolean ? "succeeded" : "failed"));
> >> +}
> >> +
> >> +/*
> >> + * All transport protocols and ports for this service are removed from
> >> + * the local rpcbind database.  The result of unregistration is reported
> >> + * via dprintk for those who want verification of the result, but is
> >> + * otherwise not important.
> >> + */
> >> +static void svc_unregister(const struct svc_serv *serv)
> >> +{
> >> +     struct svc_program *program;
> >> +     unsigned long flags;
> >> +     u32 version;
> >
> > It may just be brain-damage from too many years of mathematics, but
> > let's leave this as "i" as before: its scope is only a few lines, its
> > meaning is obvious from use, and this is what CodingStyle asks for
> > anyway.
> 
> It may seem like a small thing, but I must disagree here.  I assume
> you are quibbling with the new name only and not the type change.
>
> My reading of CodingStyle Chapter 4 is that "i" is appropriate instead
> of "tmp" or "x" or "index" -- in other words where you need a generic
> iterator.  It doesn't require the name "i" for _all_ loop iterators.
> I certainly wouldn't use "i" if I were iterating over characters or
> addresses.
> 
> In mathematics (as you well know), "i, j, k" are used as subscripts or
> for sequences or summations; often they refer to _every_ possible
> value.  We don't have any of that here.  We are passing in RPC version
> numbers.  These may not even be in sequence:  mountd has versions 1,
> 3, and 4, but not 2, nor 5 and above.

I don't agree that use of "i", "j", or "k" comes with any connotation of
unrestricted range.

> Any modern structured programming text recommends that we should name
> the variable something that reflects its use.  "i" is really quite
> generic; "version" is "short and to the point," as Chapter 4
> recommends.

One-letter variables have readability advantages which for me on balance
win out in the case of a single short loop such as this.

I don't care enough to oppose this in new code if you strongly prefer
it, but at least leave existing uses alone; it's just one more thing I
have to filter out when I read the patch.

> [ "vers" is perhaps more concise, but I think nothing but ambiguity is
> gained from dropping the last three letters.  "lovers" could easily be
> "low version" or "star-crossed lovers", for example].
> 
> Over the past several kernel releases I've included patches that
> change variables storing RPC version numbers to "u32 version" wherever
> they are used.  I really don't see the need to be different here, and
> I'd rather be consistent with nearly every other usage.  If you're
> storing an RPC version number, it is a u32 field or variable called
> "version."  The type and the name match what is in the RFCs.
> 
> >
> >> +
> >> +     clear_thread_flag(TIF_SIGPENDING);
> >> +
> >> +     for (program = serv->sv_program; program; program = program->pg_next) {
> >> +             for (version = 0; version < program->pg_nvers; version++) {
> >> +                     if (program->pg_vers[version] == NULL)
> >> +                             continue;
> >> +                     __svc_unregister(program, version);
> >
> > Isn't there a change in behavior from the omitted vs_hidden check?
> > I assume it's harmless to unregister something that was never
> > registered (if that's indeed what this does), but it seems better to
> > skip it.
> 
> svc_unregister() is used in svc_create() before registering a new
> service, and in svc_destroy() when unregistering a service being shut
> down.
> 
> It's advisable to do this now even for so-called hidden services
> because of the ability for rpcbind to advertise RPC services at
> particular addresses.  The kernel registers an RPC service for the ANY
> address, so all addresses for that service that are already registered
> should be removed first.
> 
> Perhaps for hidden services, svc_unregister() should warn loudly or
> fail immediately as a safety precaution, as these services should not
> have been registered already, and if they are, we may be colliding
> with something in user space.
> 
> > Needs a comment in the changelog in any case.
> 
> OK.

Could you also make it a separate patch?  I'd like any functional
changes split out from pure code rearrangement.

--b.

Re: [PATCH 0/8] rpcbind v4 support in net/sunrpc/svc*

[J. Bruce Fields]

On Mon, Jul 21, 2008 at 03:07:48PM -0400, Chuck Lever wrote:
> On Sun, Jul 20, 2008 at 5:17 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Fri, Jul 18, 2008 at 09:07:38PM -0400, J. Bruce Fields wrote:
> >> On Mon, Jun 30, 2008 at 06:45:21PM -0400, Chuck Lever wrote:
> >> > Hi Bruce, Trond -
> >> >
> >> > These implement basic support in net/sunrpc/svc* for registering IPv6-
> >> > enabled kernel RPC services with the local user-space rpcbind.  These
> >> > all depend on the new rpcb_v4_register() API I just posted.
> >> >
> >> > The last patch enables IPv6 support for the NFSv4 callback service, even
> >> > though it doesn't register with rpcbind.
> >>
> >> There's some conflicts against my latest tree.  Would it be possible to
> >> rebase to
> >>
> >>       git://linux-nfs.org/~bfields/linux.git for-2.6.27
> >>
> >> sometime?
> >
> > Actually, never mind, I fixed up the conflicts in the first couple and
> > staged them here:
> >
> >        git://linux-nfs.org/~bfields/linux.git for-2.6.28
> >
> > (I think the rest'll still apply cleanly to that.)
> >
> > We could maybe still try to get the rest of this in at the end of the
> > 2.6.27 merge window, but I'm inclined to wait.  Apologies, partly my
> > fault for not getting around to these sooner.
> 
> Given the complexity and controversy of these changes, the number of
> other changes that went into .27 and will require us to rewrite some
> of these, and the pending holiday/travel for our team of maintainers,
> I think waiting for .28 is our only choice.

They don't seem as controversial as that!  But, OK, I guess .28 it is.
Please let me know as soon as there's another set ready.

--b.

Re: [PATCH 0/8] rpcbind v4 support in net/sunrpc/svc*

[Chuck Lever]

On Wed, Jul 23, 2008 at 5:53 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Mon, Jul 21, 2008 at 03:07:48PM -0400, Chuck Lever wrote:
>> On Sun, Jul 20, 2008 at 5:17 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > On Fri, Jul 18, 2008 at 09:07:38PM -0400, J. Bruce Fields wrote:
>> >> On Mon, Jun 30, 2008 at 06:45:21PM -0400, Chuck Lever wrote:
>> >> > Hi Bruce, Trond -
>> >> >
>> >> > These implement basic support in net/sunrpc/svc* for registering IPv6-
>> >> > enabled kernel RPC services with the local user-space rpcbind.  These
>> >> > all depend on the new rpcb_v4_register() API I just posted.
>> >> >
>> >> > The last patch enables IPv6 support for the NFSv4 callback service, even
>> >> > though it doesn't register with rpcbind.
>> >>
>> >> There's some conflicts against my latest tree.  Would it be possible to
>> >> rebase to
>> >>
>> >>       git://linux-nfs.org/~bfields/linux.git for-2.6.27
>> >>
>> >> sometime?
>> >
>> > Actually, never mind, I fixed up the conflicts in the first couple and
>> > staged them here:
>> >
>> >        git://linux-nfs.org/~bfields/linux.git for-2.6.28
>> >
>> > (I think the rest'll still apply cleanly to that.)
>> >
>> > We could maybe still try to get the rest of this in at the end of the
>> > 2.6.27 merge window, but I'm inclined to wait.  Apologies, partly my
>> > fault for not getting around to these sooner.
>>
>> Given the complexity and controversy of these changes, the number of
>> other changes that went into .27 and will require us to rewrite some
>> of these, and the pending holiday/travel for our team of maintainers,
>> I think waiting for .28 is our only choice.
>
> They don't seem as controversial as that!  But, OK, I guess .28 it is.
> Please let me know as soon as there's another set ready.

Well, Sankar just informed me that the lock recovery bug we discussed
last week was most likely another problem not related to my IPv6
changes.  Bug found on RHEL 5, and fixed there, needs to go
upstream... he may have a patch I can apply to my repo so we can
confirm that NFSv2/v3 lock recovery is not affected by my IPv6-related
changes.

I have mostly finished clean ups as suggested during review, and as
soon as Sankar and I can get a working set up we will confirm all is
well, and I will repost what I have.

Shooting for .28 will give us a chance to get these into linux-next
for broader verification that NFS over IPv4 is unaffected by the IPv6
support.

-- 
 "Alright guard, begin the unnecessarily slow-moving dipping mechanism."
--Dr. Evil

Re: [PATCH 0/8] rpcbind v4 support in net/sunrpc/svc*

[Trond Myklebust]

On Wed, 2008-07-23 at 18:47 -0400, Chuck Lever wrote:
> Well, Sankar just informed me that the lock recovery bug we discussed
> last week was most likely another problem not related to my IPv6
> changes.  Bug found on RHEL 5, and fixed there, needs to go
> upstream... he may have a patch I can apply to my repo so we can
> confirm that NFSv2/v3 lock recovery is not affected by my IPv6-related
> changes.

Yes. I just talked to Sankar about this: apparently he was hitting the
bug that lockd no longer listens on UDP sockets if you are using
NFS-over-TCP. Unfortunately, some servers send statd reboot
notifications on UDP only, and so his client was no longer attempting to
recover locks...


-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 6/8] SUNRPC: Add kernel build option to disable server-side use of rpcbind v3/v4

[J. Bruce Fields]

On Mon, Jul 21, 2008 at 03:30:10PM -0400, Chuck Lever wrote:
> On Fri, Jul 18, 2008 at 7:42 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Mon, Jun 30, 2008 at 06:46:09PM -0400, Chuck Lever wrote:
> >> Allow distributions to use the legacy behavior until they integrate an
> >> appropriate user-space rpcbind daemon that can support IPv6 RPC services.
> >>
> >> I tried adding some logic to fall back if (un)registering with a v4
> >> protocol request failed, but there are too many corner cases.  So I just
> >> made it a compile-time switch that distributions can throw when they've
> >> replaced portmapper with rpcbind.
> >
> > OK, I see you answered my last question....  Hm.  What were the corner
> > cases?
> 
> The obvious ones:  is there a daemon running at all, and is it
> listening?  If I try v4 but that doesn't work, then I retry with v3...
> if that doesn't work, then I try v2.  Is it OK if I was able to
> register an AF_INET6 service via v2?
> 
> Certainly all this is possible to code up, but it didn't seem worth
> the complexity.

If it's really painful, OK.

But I'm nervous about slighting backwards compatibility.  It's another
thing people can mess up when they build their own stuff.  And are
people going to be able to bisect problems in nfs-utils without building
their own kernel as well?

--b.

> 
> >>
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >>
> >>  fs/Kconfig |   22 ++++++++++++++++++++++
> >>  1 files changed, 22 insertions(+), 0 deletions(-)
> >>
> >>
> >> diff --git a/fs/Kconfig b/fs/Kconfig
> >> index 8cf8ee2..6148f4c 100644
> >> --- a/fs/Kconfig
> >> +++ b/fs/Kconfig
> >> @@ -1799,6 +1799,28 @@ config SUNRPC_XPRT_RDMA
> >>
> >>         If unsure, say N.
> >>
> >> +config SUNRPC_REGISTER_V4
> >> +     bool "Register local RPC services via rpcbind v4 (EXPERIMENTAL)"
> >> +     depends on SUNRPC && EXPERIMENTAL
> >> +     default n
> >> +     help
> >> +       Sun added support for registering RPC services at an IPv6
> >> +       address by creating two new versions of the rpcbind protocol
> >> +       (RFC 1833).
> >> +
> >> +       This option enables support in the kernel RPC server for
> >> +       registering kernel RPC services via version 4 of the rpcbind
> >> +       protocol.  If you enable this option, you must run a portmapper
> >> +       daemon that supports rpcbind protocol version 4.
> >> +
> >> +       Serving NFS over IPv6 from knfsd (the kernel's NFS server)
> >> +       requires that you enable this option and use a portmapper that
> >> +       supports rpcbind version 4.
> >> +
> >> +       If unsure, say N to get traditional behavior (register kernel
> >> +       RPC services using only rpcbind version 2).  Distributions
> >> +       using the legacy Linux portmapper daemon must say N here.
> >> +
> >>  config RPCSEC_GSS_KRB5
> >>       tristate "Secure RPC: Kerberos V mechanism (EXPERIMENTAL)"
> >>       depends on SUNRPC && EXPERIMENTAL
> >>
> >> --
> >> 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
> > --
> > 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
> >
> 
> 
> 
> -- 
>  "Alright guard, begin the unnecessarily slow-moving dipping mechanism."
> --Dr. Evil