[PATCH 0/5] nfs-utils: convert gssd to TI-RPC and add IPv6 support (try #4)

[PATCH 0/5] nfs-utils: convert gssd to TI-RPC and add IPv6 support (try #4)

[Jeff Layton]

This patchset is the fourth attempt at adding support for IPv6 to
rpc.gssd. The main change from the last set is that this one now uses a
rpcbind query to determine the server's port rather than doing a
getaddrinfo call to query the local services db.

The series should be fully bisectable, but I've only really tested the
end result for anything other than to see if it compiles. With these
patches I've been able to mount an OpenSolaris server using NFSv3/4 +
IPv6 + krb5 auth. I've also done testing with builds with only TIRPC
enabled and with TIRPC and IPv6 both disabled and haven't seen any
regressions.

List of changes since the last set:
- use rpcbind query to determine port for RPC client
- added comment explaining gssd doesn't deal with IPv6 scope_id's
- slight cleanups and clarifications to comments
- properly handle EAI_SYSTEM return code from  getnameinfo() call
- changed autoconf check for getnameinfo to check whenever --enable-gss
  is set, not just when NFSv4 is also enabled.

Jeff Layton (5):
  nfs-utils: make getnameinfo() required for --enable-gss
  nfs-utils: store the address given in the upcall for later use
  nfs-utils: query for remote port using rpcbind instead of getaddrinfo
  nfs-utils: switch gssd to use standard function for getting an RPC
    client
  nfs-utils: add IPv6 code to gssd

 configure.ac           |    3 +
 utils/gssd/gssd.h      |    2 +-
 utils/gssd/gssd_proc.c |  286 +++++++++++++++++++++++++++++++++---------------
 3 files changed, 204 insertions(+), 87 deletions(-)

[PATCH 1/5] nfs-utils: make getnameinfo() required for --enable-gss

[Jeff Layton]

Systems that are so old that they don't have getnameinfo() in glibc are
probably also running kernels that are so old that they don't support
gssapi upcalls anyway.

Make --enable-gss dependent on the presence of the getnameinfo()
function.  This allows us to reduce some conditional compilation.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 configure.ac |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac
index e34b7e2..611798d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -236,6 +236,9 @@ AC_SUBST(LIBBSD)
 AC_SUBST(LIBBLKID)
 
 if test "$enable_gss" = yes; then
+  dnl 'gss' requires getnameinfo - at least for gssd_proc.c
+  AC_CHECK_FUNC([getnameinfo], , [AC_MSG_ERROR([GSSAPI support requires 'getnameinfo' function])])
+
   dnl 'gss' also depends on nfsidmap.h - at least for svcgssd_proc.c
   AC_LIBNFSIDMAP
 
-- 
1.6.0.6

[PATCH 2/5] nfs-utils: store the address given in the upcall for later use

[Jeff Layton]

The current upcall could be more efficient. We first convert the address
to a hostname, and then later when we set up the RPC client, we do a
hostname lookup to convert it back to an address.

Begin to change this by keeping the address in the clnt_info that we get
out of the upcall. Since a sockaddr has a port field, we can also
eliminate the port from the clnt_info.

Finally, switch to getnameinfo() instead of gethostbyaddr(). We'll need
to use that call anyway when we add support for IPv6.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd.h      |    2 +-
 utils/gssd/gssd_proc.c |   93 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 082039a..3c52f46 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -83,7 +83,7 @@ struct clnt_info {
 	int			krb5_poll_index;
 	int			spkm3_fd;
 	int			spkm3_poll_index;
-	int			port;
+	struct sockaddr_storage addr;
 };
 
 void init_client_list(void);
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 509946e..9d3712b 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -102,10 +102,66 @@ struct pollfd * pollarray;
 
 int pollsize;  /* the size of pollaray (in pollfd's) */
 
+/*
+ * convert a presentation address string to a sockaddr_storage struct. Returns
+ * true on success and false on failure.
+ */
+static int
+addrstr_to_sockaddr(struct sockaddr *sa, const char *addr, const int port)
+{
+	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
+
+	if (inet_pton(AF_INET, addr, &s4->sin_addr)) {
+		s4->sin_family = AF_INET;
+		s4->sin_port = htons(port);
+	} else {
+		printerr(0, "ERROR: unable to convert %s to address\n", addr);
+		return 0;
+	}
+
+	return 1;
+}
+
+/*
+ * convert a sockaddr to a hostname
+ */
+static char *
+sockaddr_to_hostname(const struct sockaddr *sa, const char *addr)
+{
+	socklen_t		addrlen;
+	int			err;
+	char 			*hostname;
+	char			hbuf[NI_MAXHOST];
+
+	switch (sa->sa_family) {
+	case AF_INET:
+		addrlen = sizeof(struct sockaddr_in);
+		break;
+	default:
+		printerr(0, "ERROR: unrecognized addr family %d\n",
+			 sa->sa_family);
+		return NULL;
+	}
+
+	err = getnameinfo(sa, addrlen, hbuf, sizeof(hbuf), NULL, 0,
+			  NI_NAMEREQD);
+	if (err) {
+		printerr(0, "ERROR: unable to resolve %s to hostname: %s\n",
+			 addr, err == EAI_SYSTEM ? strerror(err) :
+						   gai_strerror(err));
+		return NULL;
+	}
+
+	hostname = strdup(hbuf);
+
+	return hostname;
+}
+
 /* XXX buffer problems: */
 static int
 read_service_info(char *info_file_name, char **servicename, char **servername,
-		  int *prog, int *vers, char **protocol, int *port) {
+		  int *prog, int *vers, char **protocol,
+		  struct sockaddr *addr) {
 #define INFOBUFLEN 256
 	char		buf[INFOBUFLEN + 1];
 	static char	dummy[128];
@@ -117,10 +173,9 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
 	char		protoname[16];
 	char		cb_port[128];
 	char		*p;
-	in_addr_t	inaddr;
 	int		fd = -1;
-	struct hostent	*ent = NULL;
 	int		numfields;
+	int		port = 0;
 
 	*servicename = *servername = *protocol = NULL;
 
@@ -160,21 +215,26 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
 	if((*prog != 100003) || ((*vers != 2) && (*vers != 3) && (*vers != 4)))
 		goto fail;
 
-	/* create service name */
-	inaddr = inet_addr(address);
-	if (!(ent = gethostbyaddr(&inaddr, sizeof(inaddr), AF_INET))) {
-		printerr(0, "ERROR: can't resolve server %s name\n", address);
-		goto fail;
+	if (cb_port[0] != '\0') {
+		port = atoi(cb_port);
+		if (port < 0 || port > 65535)
+			goto fail;
 	}
-	if (!(*servername = calloc(strlen(ent->h_name) + 1, 1)))
+
+	if (!addrstr_to_sockaddr(addr, address, port))
+		goto fail;
+
+	*servername = sockaddr_to_hostname(addr, address);
+	if (*servername == NULL)
 		goto fail;
-	memcpy(*servername, ent->h_name, strlen(ent->h_name));
-	snprintf(buf, INFOBUFLEN, "%s@%s", service, ent->h_name);
+
+	nbytes = snprintf(buf, INFOBUFLEN, "%s@%s", service, *servername);
+	if (nbytes > INFOBUFLEN)
+		goto fail;
+
 	if (!(*servicename = calloc(strlen(buf) + 1, 1)))
 		goto fail;
 	memcpy(*servicename, buf, strlen(buf));
-	if (cb_port[0] != '\0')
-		*port = atoi(cb_port);
 
 	if (!(*protocol = strdup(protoname)))
 		goto fail;
@@ -251,7 +311,7 @@ process_clnt_dir_files(struct clnt_info * clp)
 	if ((clp->servicename == NULL) &&
 	     read_service_info(info_file_name, &clp->servicename,
 				&clp->servername, &clp->prog, &clp->vers,
-				&clp->protocol, &clp->port))
+				&clp->protocol, (struct sockaddr *) &clp->addr))
 		return -1;
 	return 0;
 }
@@ -599,8 +659,9 @@ int create_auth_rpc_client(struct clnt_info *clp,
 			 clp->servername, uid);
 		goto out_fail;
 	}
-	if (clp->port)
-		((struct sockaddr_in *)a->ai_addr)->sin_port = htons(clp->port);
+	if (((struct sockaddr_in) clp->addr).sin_port != 0)
+		((struct sockaddr_in *) a->ai_addr)->sin_port =
+				((struct sockaddr_in) clp->addr).sin_port;
 	if (a->ai_protocol == IPPROTO_TCP) {
 		if ((rpc_clnt = clnttcp_create(
 					(struct sockaddr_in *) a->ai_addr,
-- 
1.6.0.6

[PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Jeff Layton]

We already have the server's address from the upcall, so we don't really
need to look it up again, and querying the local services DB for the
port that the remote server is listening on is just plain wrong.

Use rpcbind to set the port for the program and version that we were
given in the upcall. The exception here is NFSv4. Since NFSv4 mounts
are supposed to use a well-defined port then skip the rpcbind query
for that and just set the port to the standard one (2049).

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c |  126 +++++++++++++++++++++++++++++-------------------
 1 files changed, 76 insertions(+), 50 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 9d3712b..1571329 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -72,6 +72,7 @@
 #include "gss_util.h"
 #include "krb5_util.h"
 #include "context.h"
+#include "nfsrpc.h"
 
 /*
  * pollarray:
@@ -541,6 +542,64 @@ out_err:
 }
 
 /*
+ * If the port isn't already set, do an rpcbind query to the remote server
+ * using the program and version and get the port. Newer kernels send the
+ * port in the upcall, so this is really only for compatibility with older
+ * ones.
+ */
+static int
+populate_port(struct sockaddr *sa, const socklen_t salen,
+	      const rpcprog_t program, const rpcvers_t version,
+	      const unsigned short protocol)
+{
+	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
+	unsigned short		port;
+
+	/*
+	 * Newer kernels send the port in the upcall. If we already have
+	 * the port, there's no need to look it up.
+	 */
+	switch (sa->sa_family) {
+	case AF_INET:
+		if (s4->sin_port != 0) {
+			printerr(2, "DEBUG: port already set to %d\n",
+				 ntohs(s4->sin_port));
+			return 1;
+		}
+		break;
+	default:
+		printerr(0, "ERROR: unsupported address family %d\n",
+			    sa->sa_family);
+		return 0;
+	}
+
+	/* Use standard NFS port for NFSv4 */
+	if (program == 100003 && version == 4) {
+		port = 2049;
+		goto set_port;
+	}
+
+	port = nfs_getport(sa, salen, program, version, protocol);
+	if (!port) {
+		printerr(0, "ERROR: unable to obtain port for prog %ld "
+			    "vers %ld\n", program, version);
+		return 0;
+	}
+
+set_port:
+	printerr(2, "DEBUG: setting port to %hu for prog %lu vers %lu\n", port,
+		 program, version);
+
+	switch (sa->sa_family) {
+	case AF_INET:
+		s4->sin_port = htons(port);
+		break;
+	}
+
+	return 1;
+}
+
+/*
  * Create an RPC connection and establish an authenticated
  * gss context with a server.
  */
@@ -555,14 +614,13 @@ int create_auth_rpc_client(struct clnt_info *clp,
 	AUTH			*auth = NULL;
 	uid_t			save_uid = -1;
 	int			retval = -1;
-	int			errcode;
 	OM_uint32		min_stat;
 	char			rpc_errmsg[1024];
 	int			sockp = RPC_ANYSOCK;
 	int			sendsz = 32768, recvsz = 32768;
-	struct addrinfo		ai_hints, *a = NULL;
-	char			service[64];
-	char			*at_sign;
+	int			protocol;
+	struct sockaddr		*addr = (struct sockaddr *) &clp->addr;
+	socklen_t		salen;
 
 	/* Create the context as the user (not as root) */
 	save_uid = geteuid();
@@ -616,15 +674,10 @@ int create_auth_rpc_client(struct clnt_info *clp,
 	printerr(2, "creating %s client for server %s\n", clp->protocol,
 			clp->servername);
 
-	memset(&ai_hints, '\0', sizeof(ai_hints));
-	ai_hints.ai_family = PF_INET;
-	ai_hints.ai_flags |= AI_CANONNAME;
 	if ((strcmp(clp->protocol, "tcp")) == 0) {
-		ai_hints.ai_socktype = SOCK_STREAM;
-		ai_hints.ai_protocol = IPPROTO_TCP;
+		protocol = IPPROTO_TCP;
 	} else if ((strcmp(clp->protocol, "udp")) == 0) {
-		ai_hints.ai_socktype = SOCK_DGRAM;
-		ai_hints.ai_protocol = IPPROTO_UDP;
+		protocol = IPPROTO_UDP;
 	} else {
 		printerr(0, "WARNING: unrecognized protocol, '%s', requested "
 			 "for connection to server %s for user with uid %d\n",
@@ -632,39 +685,22 @@ int create_auth_rpc_client(struct clnt_info *clp,
 		goto out_fail;
 	}
 
-	/* extract the service name from clp->servicename */
-	if ((at_sign = strchr(clp->servicename, '@')) == NULL) {
-		printerr(0, "WARNING: servicename (%s) not formatted as "
-			"expected with service@host\n", clp->servicename);
-		goto out_fail;
-	}
-	if ((at_sign - clp->servicename) >= sizeof(service)) {
-		printerr(0, "WARNING: service portion of servicename (%s) "
-			"is too long!\n", clp->servicename);
+	switch (addr->sa_family) {
+	case AF_INET:
+		salen = sizeof(struct sockaddr_in);
+		break;
+	default:
+		printerr(1, "ERROR: Unknown address family %d\n",
+			 addr->sa_family);
 		goto out_fail;
 	}
-	strncpy(service, clp->servicename, at_sign - clp->servicename);
-	service[at_sign - clp->servicename] = '\0';
 
-	errcode = getaddrinfo(clp->servername, service, &ai_hints, &a);
-	if (errcode) {
-		printerr(0, "WARNING: Error from getaddrinfo for server "
-			 "'%s': %s\n", clp->servername, gai_strerror(errcode));
+	if (!populate_port(addr, salen, clp->prog, clp->vers, protocol))
 		goto out_fail;
-	}
 
-	if (a == NULL) {
-		printerr(0, "WARNING: No address information found for "
-			 "connection to server %s for user with uid %d\n",
-			 clp->servername, uid);
-		goto out_fail;
-	}
-	if (((struct sockaddr_in) clp->addr).sin_port != 0)
-		((struct sockaddr_in *) a->ai_addr)->sin_port =
-				((struct sockaddr_in) clp->addr).sin_port;
-	if (a->ai_protocol == IPPROTO_TCP) {
+	if (protocol == IPPROTO_TCP) {
 		if ((rpc_clnt = clnttcp_create(
-					(struct sockaddr_in *) a->ai_addr,
+					(struct sockaddr_in *) addr,
 					clp->prog, clp->vers, &sockp,
 					sendsz, recvsz)) == NULL) {
 			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
@@ -675,10 +711,10 @@ int create_auth_rpc_client(struct clnt_info *clp,
 				 clnt_spcreateerror(rpc_errmsg));
 			goto out_fail;
 		}
-	} else if (a->ai_protocol == IPPROTO_UDP) {
+	} else if (protocol == IPPROTO_UDP) {
 		const struct timeval timeout = {5, 0};
 		if ((rpc_clnt = clntudp_bufcreate(
-					(struct sockaddr_in *) a->ai_addr,
+					(struct sockaddr_in *) addr,
 					clp->prog, clp->vers, timeout,
 					&sockp, sendsz, recvsz)) == NULL) {
 			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
@@ -689,16 +725,7 @@ int create_auth_rpc_client(struct clnt_info *clp,
 				 clnt_spcreateerror(rpc_errmsg));
 			goto out_fail;
 		}
-	} else {
-		/* Shouldn't happen! */
-		printerr(0, "ERROR: requested protocol '%s', but "
-			 "got addrinfo with protocol %d\n",
-			 clp->protocol, a->ai_protocol);
-		goto out_fail;
 	}
-	/* We're done with this */
-	freeaddrinfo(a);
-	a = NULL;
 
 	printerr(2, "creating context with server %s\n", clp->servicename);
 	auth = authgss_create_default(rpc_clnt, clp->servicename, &sec);
@@ -720,7 +747,6 @@ int create_auth_rpc_client(struct clnt_info *clp,
   out:
 	if (sec.cred != GSS_C_NO_CREDENTIAL)
 		gss_release_cred(&min_stat, &sec.cred);
-  	if (a != NULL) freeaddrinfo(a);
 	/* Restore euid to original value */
 	if ((save_uid != -1) && (setfsuid(save_uid) != uid)) {
 		printerr(0, "WARNING: Failed to restore fsuid"
-- 
1.6.0.6

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Chuck Lever]

On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:

> We already have the server's address from the upcall, so we don't  
> really
> need to look it up again, and querying the local services DB for the
> port that the remote server is listening on is just plain wrong.
>
> Use rpcbind to set the port for the program and version that we were
> given in the upcall. The exception here is NFSv4. Since NFSv4 mounts
> are supposed to use a well-defined port then skip the rpcbind query
> for that and just set the port to the standard one (2049).
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> utils/gssd/gssd_proc.c |  126 ++++++++++++++++++++++++++++ 
> +-------------------
> 1 files changed, 76 insertions(+), 50 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 9d3712b..1571329 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -72,6 +72,7 @@
> #include "gss_util.h"
> #include "krb5_util.h"
> #include "context.h"
> +#include "nfsrpc.h"
>
> /*
>  * pollarray:
> @@ -541,6 +542,64 @@ out_err:
> }
>
> /*
> + * If the port isn't already set, do an rpcbind query to the remote  
> server
> + * using the program and version and get the port. Newer kernels  
> send the
> + * port in the upcall, so this is really only for compatibility  
> with older
> + * ones.
> + */
> +static int
> +populate_port(struct sockaddr *sa, const socklen_t salen,
> +	      const rpcprog_t program, const rpcvers_t version,
> +	      const unsigned short protocol)
> +{
> +	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
> +	unsigned short		port;
> +
> +	/*
> +	 * Newer kernels send the port in the upcall. If we already have
> +	 * the port, there's no need to look it up.
> +	 */
> +	switch (sa->sa_family) {
> +	case AF_INET:
> +		if (s4->sin_port != 0) {
> +			printerr(2, "DEBUG: port already set to %d\n",
> +				 ntohs(s4->sin_port));
> +			return 1;
> +		}
> +		break;
> +	default:
> +		printerr(0, "ERROR: unsupported address family %d\n",
> +			    sa->sa_family);
> +		return 0;
> +	}
> +
> +	/* Use standard NFS port for NFSv4 */
> +	if (program == 100003 && version == 4) {
> +		port = 2049;
> +		goto set_port;
> +	}

I think this patch set looks pretty reasonable.  Here's my one  
remaining quibble.

You can specify "port=" for nfs4 mounts, in which case we want to use  
that value here, too, I think.  It would be simpler overall if the  
kernel always passed up the value it is using for port= on this mount  
point.

The rules for how the kernel uses the port= setting are:

  + if port= is not specified on NFSv2/v3, port= setting is zero
  + if port= is not specified on NFSv4, port= setting is 2049

Then, when setting up a tranport:

  + if the port= setting is zero, do an rpcbind
  + if the port= setting is not zero, use that value

If the kernel always passes the port= setting to gssd, then it can  
follow the "if port value is zero, rpcbind; otherwise use port value  
as is" rule, and be sure to get correct NFSv4 behavior, even without  
the special case you added for NFSv4.

> +
> +	port = nfs_getport(sa, salen, program, version, protocol);
> +	if (!port) {
> +		printerr(0, "ERROR: unable to obtain port for prog %ld "
> +			    "vers %ld\n", program, version);
> +		return 0;
> +	}
> +
> +set_port:
> +	printerr(2, "DEBUG: setting port to %hu for prog %lu vers %lu\n",  
> port,
> +		 program, version);
> +
> +	switch (sa->sa_family) {
> +	case AF_INET:
> +		s4->sin_port = htons(port);
> +		break;
> +	}
> +
> +	return 1;
> +}
> +
> +/*
>  * Create an RPC connection and establish an authenticated
>  * gss context with a server.
>  */
> @@ -555,14 +614,13 @@ int create_auth_rpc_client(struct clnt_info  
> *clp,
> 	AUTH			*auth = NULL;
> 	uid_t			save_uid = -1;
> 	int			retval = -1;
> -	int			errcode;
> 	OM_uint32		min_stat;
> 	char			rpc_errmsg[1024];
> 	int			sockp = RPC_ANYSOCK;
> 	int			sendsz = 32768, recvsz = 32768;
> -	struct addrinfo		ai_hints, *a = NULL;
> -	char			service[64];
> -	char			*at_sign;
> +	int			protocol;
> +	struct sockaddr		*addr = (struct sockaddr *) &clp->addr;
> +	socklen_t		salen;
>
> 	/* Create the context as the user (not as root) */
> 	save_uid = geteuid();
> @@ -616,15 +674,10 @@ int create_auth_rpc_client(struct clnt_info  
> *clp,
> 	printerr(2, "creating %s client for server %s\n", clp->protocol,
> 			clp->servername);
>
> -	memset(&ai_hints, '\0', sizeof(ai_hints));
> -	ai_hints.ai_family = PF_INET;
> -	ai_hints.ai_flags |= AI_CANONNAME;
> 	if ((strcmp(clp->protocol, "tcp")) == 0) {
> -		ai_hints.ai_socktype = SOCK_STREAM;
> -		ai_hints.ai_protocol = IPPROTO_TCP;
> +		protocol = IPPROTO_TCP;
> 	} else if ((strcmp(clp->protocol, "udp")) == 0) {
> -		ai_hints.ai_socktype = SOCK_DGRAM;
> -		ai_hints.ai_protocol = IPPROTO_UDP;
> +		protocol = IPPROTO_UDP;
> 	} else {
> 		printerr(0, "WARNING: unrecognized protocol, '%s', requested "
> 			 "for connection to server %s for user with uid %d\n",
> @@ -632,39 +685,22 @@ int create_auth_rpc_client(struct clnt_info  
> *clp,
> 		goto out_fail;
> 	}
>
> -	/* extract the service name from clp->servicename */
> -	if ((at_sign = strchr(clp->servicename, '@')) == NULL) {
> -		printerr(0, "WARNING: servicename (%s) not formatted as "
> -			"expected with service@host\n", clp->servicename);
> -		goto out_fail;
> -	}
> -	if ((at_sign - clp->servicename) >= sizeof(service)) {
> -		printerr(0, "WARNING: service portion of servicename (%s) "
> -			"is too long!\n", clp->servicename);
> +	switch (addr->sa_family) {
> +	case AF_INET:
> +		salen = sizeof(struct sockaddr_in);
> +		break;
> +	default:
> +		printerr(1, "ERROR: Unknown address family %d\n",
> +			 addr->sa_family);
> 		goto out_fail;
> 	}
> -	strncpy(service, clp->servicename, at_sign - clp->servicename);
> -	service[at_sign - clp->servicename] = '\0';
>
> -	errcode = getaddrinfo(clp->servername, service, &ai_hints, &a);
> -	if (errcode) {
> -		printerr(0, "WARNING: Error from getaddrinfo for server "
> -			 "'%s': %s\n", clp->servername, gai_strerror(errcode));
> +	if (!populate_port(addr, salen, clp->prog, clp->vers, protocol))
> 		goto out_fail;
> -	}
>
> -	if (a == NULL) {
> -		printerr(0, "WARNING: No address information found for "
> -			 "connection to server %s for user with uid %d\n",
> -			 clp->servername, uid);
> -		goto out_fail;
> -	}
> -	if (((struct sockaddr_in) clp->addr).sin_port != 0)
> -		((struct sockaddr_in *) a->ai_addr)->sin_port =
> -				((struct sockaddr_in) clp->addr).sin_port;
> -	if (a->ai_protocol == IPPROTO_TCP) {
> +	if (protocol == IPPROTO_TCP) {
> 		if ((rpc_clnt = clnttcp_create(
> -					(struct sockaddr_in *) a->ai_addr,
> +					(struct sockaddr_in *) addr,
> 					clp->prog, clp->vers, &sockp,
> 					sendsz, recvsz)) == NULL) {
> 			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
> @@ -675,10 +711,10 @@ int create_auth_rpc_client(struct clnt_info  
> *clp,
> 				 clnt_spcreateerror(rpc_errmsg));
> 			goto out_fail;
> 		}
> -	} else if (a->ai_protocol == IPPROTO_UDP) {
> +	} else if (protocol == IPPROTO_UDP) {
> 		const struct timeval timeout = {5, 0};
> 		if ((rpc_clnt = clntudp_bufcreate(
> -					(struct sockaddr_in *) a->ai_addr,
> +					(struct sockaddr_in *) addr,
> 					clp->prog, clp->vers, timeout,
> 					&sockp, sendsz, recvsz)) == NULL) {
> 			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
> @@ -689,16 +725,7 @@ int create_auth_rpc_client(struct clnt_info *clp,
> 				 clnt_spcreateerror(rpc_errmsg));
> 			goto out_fail;
> 		}
> -	} else {
> -		/* Shouldn't happen! */
> -		printerr(0, "ERROR: requested protocol '%s', but "
> -			 "got addrinfo with protocol %d\n",
> -			 clp->protocol, a->ai_protocol);
> -		goto out_fail;
> 	}
> -	/* We're done with this */
> -	freeaddrinfo(a);
> -	a = NULL;
>
> 	printerr(2, "creating context with server %s\n", clp->servicename);
> 	auth = authgss_create_default(rpc_clnt, clp->servicename, &sec);
> @@ -720,7 +747,6 @@ int create_auth_rpc_client(struct clnt_info *clp,
>   out:
> 	if (sec.cred != GSS_C_NO_CREDENTIAL)
> 		gss_release_cred(&min_stat, &sec.cred);
> -  	if (a != NULL) freeaddrinfo(a);
> 	/* Restore euid to original value */
> 	if ((save_uid != -1) && (setfsuid(save_uid) != uid)) {
> 		printerr(0, "WARNING: Failed to restore fsuid"
> -- 
> 1.6.0.6
>

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

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[J. Bruce Fields]

On Tue, Apr 07, 2009 at 12:02:46PM -0400, Chuck Lever wrote:
> 
> On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
> 
> > We already have the server's address from the upcall, so we don't  
> > really
> > need to look it up again, and querying the local services DB for the
> > port that the remote server is listening on is just plain wrong.
> >
> > Use rpcbind to set the port for the program and version that we were
> > given in the upcall. The exception here is NFSv4. Since NFSv4 mounts
> > are supposed to use a well-defined port then skip the rpcbind query
> > for that and just set the port to the standard one (2049).
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > utils/gssd/gssd_proc.c |  126 ++++++++++++++++++++++++++++ 
> > +-------------------
> > 1 files changed, 76 insertions(+), 50 deletions(-)
> >
> > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > index 9d3712b..1571329 100644
> > --- a/utils/gssd/gssd_proc.c
> > +++ b/utils/gssd/gssd_proc.c
> > @@ -72,6 +72,7 @@
> > #include "gss_util.h"
> > #include "krb5_util.h"
> > #include "context.h"
> > +#include "nfsrpc.h"
> >
> > /*
> >  * pollarray:
> > @@ -541,6 +542,64 @@ out_err:
> > }
> >
> > /*
> > + * If the port isn't already set, do an rpcbind query to the remote  
> > server
> > + * using the program and version and get the port. Newer kernels  
> > send the
> > + * port in the upcall, so this is really only for compatibility  
> > with older
> > + * ones.
> > + */
> > +static int
> > +populate_port(struct sockaddr *sa, const socklen_t salen,
> > +	      const rpcprog_t program, const rpcvers_t version,
> > +	      const unsigned short protocol)
> > +{
> > +	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
> > +	unsigned short		port;
> > +
> > +	/*
> > +	 * Newer kernels send the port in the upcall. If we already have
> > +	 * the port, there's no need to look it up.
> > +	 */
> > +	switch (sa->sa_family) {
> > +	case AF_INET:
> > +		if (s4->sin_port != 0) {
> > +			printerr(2, "DEBUG: port already set to %d\n",
> > +				 ntohs(s4->sin_port));
> > +			return 1;
> > +		}
> > +		break;
> > +	default:
> > +		printerr(0, "ERROR: unsupported address family %d\n",
> > +			    sa->sa_family);
> > +		return 0;
> > +	}
> > +
> > +	/* Use standard NFS port for NFSv4 */
> > +	if (program == 100003 && version == 4) {
> > +		port = 2049;
> > +		goto set_port;
> > +	}
> 
> I think this patch set looks pretty reasonable.  Here's my one  
> remaining quibble.
> 
> You can specify "port=" for nfs4 mounts, in which case we want to use  
> that value here, too, I think.  It would be simpler overall if the  
> kernel always passed up the value it is using for port= on this mount  
> point.
> 
> The rules for how the kernel uses the port= setting are:
> 
>   + if port= is not specified on NFSv2/v3, port= setting is zero
>   + if port= is not specified on NFSv4, port= setting is 2049
> 
> Then, when setting up a tranport:
> 
>   + if the port= setting is zero, do an rpcbind
>   + if the port= setting is not zero, use that value
> 
> If the kernel always passes the port= setting to gssd, then it can  
> follow the "if port value is zero, rpcbind; otherwise use port value  
> as is" rule, and be sure to get correct NFSv4 behavior, even without  
> the special case you added for NFSv4.

In recent kernels that information is in the "info" file for the given
client in rpc_pipefs.  Kevin and Olga have patches to support that in
gssd, if they aren't already in upstream nfs-utils.

And of course it's important to use that when it's available, to avoid
unnecessary rpcbind calls (in a pure nfsv4 environment the only firewall
hole you should have to poke is for the nfsv4 port), to respect the
port= mount option for people running a v4 server on an alternate port,
and for authenticating the callback channel (which won't normally be to
port 2049, and won't (I assume) normally be registered with the
portmapper).

--b.

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Chuck Lever]

On Apr 7, 2009, at 12:20 PM, J. Bruce Fields wrote:
> On Tue, Apr 07, 2009 at 12:02:46PM -0400, Chuck Lever wrote:
>>
>> On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>>
>>> We already have the server's address from the upcall, so we don't
>>> really
>>> need to look it up again, and querying the local services DB for the
>>> port that the remote server is listening on is just plain wrong.
>>>
>>> Use rpcbind to set the port for the program and version that we were
>>> given in the upcall. The exception here is NFSv4. Since NFSv4 mounts
>>> are supposed to use a well-defined port then skip the rpcbind query
>>> for that and just set the port to the standard one (2049).
>>>
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>> ---
>>> utils/gssd/gssd_proc.c |  126 ++++++++++++++++++++++++++++
>>> +-------------------
>>> 1 files changed, 76 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
>>> index 9d3712b..1571329 100644
>>> --- a/utils/gssd/gssd_proc.c
>>> +++ b/utils/gssd/gssd_proc.c
>>> @@ -72,6 +72,7 @@
>>> #include "gss_util.h"
>>> #include "krb5_util.h"
>>> #include "context.h"
>>> +#include "nfsrpc.h"
>>>
>>> /*
>>> * pollarray:
>>> @@ -541,6 +542,64 @@ out_err:
>>> }
>>>
>>> /*
>>> + * If the port isn't already set, do an rpcbind query to the remote
>>> server
>>> + * using the program and version and get the port. Newer kernels
>>> send the
>>> + * port in the upcall, so this is really only for compatibility
>>> with older
>>> + * ones.
>>> + */
>>> +static int
>>> +populate_port(struct sockaddr *sa, const socklen_t salen,
>>> +	      const rpcprog_t program, const rpcvers_t version,
>>> +	      const unsigned short protocol)
>>> +{
>>> +	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
>>> +	unsigned short		port;
>>> +
>>> +	/*
>>> +	 * Newer kernels send the port in the upcall. If we already have
>>> +	 * the port, there's no need to look it up.
>>> +	 */
>>> +	switch (sa->sa_family) {
>>> +	case AF_INET:
>>> +		if (s4->sin_port != 0) {
>>> +			printerr(2, "DEBUG: port already set to %d\n",
>>> +				 ntohs(s4->sin_port));
>>> +			return 1;
>>> +		}
>>> +		break;
>>> +	default:
>>> +		printerr(0, "ERROR: unsupported address family %d\n",
>>> +			    sa->sa_family);
>>> +		return 0;
>>> +	}
>>> +
>>> +	/* Use standard NFS port for NFSv4 */
>>> +	if (program == 100003 && version == 4) {
>>> +		port = 2049;
>>> +		goto set_port;
>>> +	}

And actually, since this test is _after_ the check to see if zero was  
passed in, I think we probably get correct behavior here if the kernel  
passes a non-zero port value for an NFSv4 mount.  My mistake.

>> I think this patch set looks pretty reasonable.  Here's my one
>> remaining quibble.
>>
>> You can specify "port=" for nfs4 mounts, in which case we want to use
>> that value here, too, I think.  It would be simpler overall if the
>> kernel always passed up the value it is using for port= on this mount
>> point.
>>
>> The rules for how the kernel uses the port= setting are:
>>
>>  + if port= is not specified on NFSv2/v3, port= setting is zero
>>  + if port= is not specified on NFSv4, port= setting is 2049
>>
>> Then, when setting up a tranport:
>>
>>  + if the port= setting is zero, do an rpcbind
>>  + if the port= setting is not zero, use that value
>>
>> If the kernel always passes the port= setting to gssd, then it can
>> follow the "if port value is zero, rpcbind; otherwise use port value
>> as is" rule, and be sure to get correct NFSv4 behavior, even without
>> the special case you added for NFSv4.
>
> In recent kernels that information is in the "info" file for the given
> client in rpc_pipefs.  Kevin and Olga have patches to support that in
> gssd, if they aren't already in upstream nfs-utils.

The issue is we're not exactly sure what value the kernel is passing  
up in the info file.  It seems to change over time.

> And of course it's important to use that when it's available, to avoid
> unnecessary rpcbind calls (in a pure nfsv4 environment the only  
> firewall
> hole you should have to poke is for the nfsv4 port), to respect the
> port= mount option for people running a v4 server on an alternate  
> port,
> and for authenticating the callback channel (which won't normally be  
> to
> port 2049, and won't (I assume) normally be registered with the
> portmapper).

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

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[J. Bruce Fields]

On Tue, Apr 07, 2009 at 12:29:23PM -0400, Chuck Lever wrote:
> On Apr 7, 2009, at 12:20 PM, J. Bruce Fields wrote:
>> In recent kernels that information is in the "info" file for the given
>> client in rpc_pipefs.  Kevin and Olga have patches to support that in
>> gssd, if they aren't already in upstream nfs-utils.
>
> The issue is we're not exactly sure what value the kernel is passing up 
> in the info file.  It seems to change over time.

I don't understand.  How does it change over time?

--b.

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Chuck Lever]

On Apr 7, 2009, at 12:32 PM, J. Bruce Fields wrote:
> On Tue, Apr 07, 2009 at 12:29:23PM -0400, Chuck Lever wrote:
>> On Apr 7, 2009, at 12:20 PM, J. Bruce Fields wrote:
>>> In recent kernels that information is in the "info" file for the  
>>> given
>>> client in rpc_pipefs.  Kevin and Olga have patches to support that  
>>> in
>>> gssd, if they aren't already in upstream nfs-utils.
>>
>> The issue is we're not exactly sure what value the kernel is  
>> passing up
>> in the info file.  It seems to change over time.
>
> I don't understand.  How does it change over time?

Jeff observed some strange behavior while testing yesterday, so I'll  
let him address this question.

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

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Jeff Layton]

On Tue, 7 Apr 2009 12:34:54 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> On Apr 7, 2009, at 12:32 PM, J. Bruce Fields wrote:
> > On Tue, Apr 07, 2009 at 12:29:23PM -0400, Chuck Lever wrote:
> >> On Apr 7, 2009, at 12:20 PM, J. Bruce Fields wrote:
> >>> In recent kernels that information is in the "info" file for the  
> >>> given
> >>> client in rpc_pipefs.  Kevin and Olga have patches to support that  
> >>> in
> >>> gssd, if they aren't already in upstream nfs-utils.
> >>
> >> The issue is we're not exactly sure what value the kernel is  
> >> passing up
> >> in the info file.  It seems to change over time.
> >
> > I don't understand.  How does it change over time?
> 
> Jeff observed some strange behavior while testing yesterday, so I'll  
> let him address this question.
> 

I sort of had expected to see the "port" value in the info file change
once the kernel had done the rpcbind query and set the port in the
socket. It doesn't do this though. In hindsight I think the current
behavior is actually correct. The port value here more or less
represents the port that was given to the kernel at mount time.

> The rules for how the kernel uses the port= setting are:
> 
>   + if port= is not specified on NFSv2/v3, port= setting is zero
>   + if port= is not specified on NFSv4, port= setting is 2049
> 
> Then, when setting up a tranport:
> 
>   + if the port= setting is zero, do an rpcbind
>   + if the port= setting is not zero, use that value

I think this patchset provides the behavior you want. For recent
kernels that provide the port: field in the "info" file:

nfsv4: port: 2049
nfsv2/3: port: 0

...if you set port= in the mount options, then the info file displays
that port.

When we have an older kernel that does not display the port, then
populate_port() will see that sin(6)_port is still 0. For nfsv2/3 it'll
do an rpcbind query to get the port, but we don't want to do that for
NFSv4, so I added this:

+       /* Use standard NFS port for NFSv4 */
+       if (program == 100003 && version == 4) {
+               port = 2049;
+               goto set_port;
+       }

The important bit is that the above chunk will only matter for kernels
that don't have a port: field in the info file.

In short, I think the behavior with this set is what you've outlined
above.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Chuck Lever]

On Apr 7, 2009, at 1:00 PM, Jeff Layton wrote:
> On Tue, 7 Apr 2009 12:34:54 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
>
>> On Apr 7, 2009, at 12:32 PM, J. Bruce Fields wrote:
>>> On Tue, Apr 07, 2009 at 12:29:23PM -0400, Chuck Lever wrote:
>>>> On Apr 7, 2009, at 12:20 PM, J. Bruce Fields wrote:
>>>>> In recent kernels that information is in the "info" file for the
>>>>> given
>>>>> client in rpc_pipefs.  Kevin and Olga have patches to support that
>>>>> in
>>>>> gssd, if they aren't already in upstream nfs-utils.
>>>>
>>>> The issue is we're not exactly sure what value the kernel is
>>>> passing up
>>>> in the info file.  It seems to change over time.
>>>
>>> I don't understand.  How does it change over time?
>>
>> Jeff observed some strange behavior while testing yesterday, so I'll
>> let him address this question.
>>
>
> I sort of had expected to see the "port" value in the info file change
> once the kernel had done the rpcbind query and set the port in the
> socket. It doesn't do this though. In hindsight I think the current
> behavior is actually correct. The port value here more or less
> represents the port that was given to the kernel at mount time.
>
>> The rules for how the kernel uses the port= setting are:
>>
>>  + if port= is not specified on NFSv2/v3, port= setting is zero
>>  + if port= is not specified on NFSv4, port= setting is 2049
>>
>> Then, when setting up a tranport:
>>
>>  + if the port= setting is zero, do an rpcbind
>>  + if the port= setting is not zero, use that value
>
> I think this patchset provides the behavior you want. For recent
> kernels that provide the port: field in the "info" file:
>
> nfsv4: port: 2049
> nfsv2/3: port: 0
>
> ...if you set port= in the mount options, then the info file displays
> that port.
>
> When we have an older kernel that does not display the port, then
> populate_port() will see that sin(6)_port is still 0. For nfsv2/3  
> it'll
> do an rpcbind query to get the port, but we don't want to do that for
> NFSv4, so I added this:
>
> +       /* Use standard NFS port for NFSv4 */
> +       if (program == 100003 && version == 4) {
> +               port = 2049;
> +               goto set_port;
> +       }
>
> The important bit is that the above chunk will only matter for kernels
> that don't have a port: field in the info file.

Ah.  I think the comment should mention this is for supporting legacy  
kernels.

> In short, I think the behavior with this set is what you've outlined
> above.

Good.  Ship it ;-)

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

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Tom Talpey]

At 12:02 PM 4/7/2009, Chuck Lever wrote:
>
>On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>> +	/* Use standard NFS port for NFSv4 */
>> +	if (program == 100003 && version == 4) {
>> +		port = 2049;
>> +		goto set_port;
>> +	}
>
>I think this patch set looks pretty reasonable.  Here's my one  
>remaining quibble.
>
>You can specify "port=" for nfs4 mounts, in which case we want to use  
>that value here, too, I think.  It would be simpler overall if the  

*Must* use a port= specification. The 2049 definition is only true for
NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
port 20049. So slamming the port to 2049 would break NFSv4/RDMA.

>kernel always passed up the value it is using for port= on this mount  
>point.
>
>The rules for how the kernel uses the port= setting are:
>
>  + if port= is not specified on NFSv2/v3, port= setting is zero
>  + if port= is not specified on NFSv4, port= setting is 2049

This is a tiny bit questionable, since 2049 is only defined for TCP.
But, if port= can override, then that's a workaround, so OK.

>
>Then, when setting up a tranport:
>
>  + if the port= setting is zero, do an rpcbind
>  + if the port= setting is not zero, use that value
>
>If the kernel always passes the port= setting to gssd, then it can  
>follow the "if port value is zero, rpcbind; otherwise use port value  
>as is" rule, and be sure to get correct NFSv4 behavior, even without  
>the special case you added for NFSv4.

Agree.

Tom.

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Chuck Lever]

On Apr 7, 2009, at 12:27 PM, Tom Talpey wrote:
> At 12:02 PM 4/7/2009, Chuck Lever wrote:
>>
>> On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>>> +	/* Use standard NFS port for NFSv4 */
>>> +	if (program == 100003 && version == 4) {
>>> +		port = 2049;
>>> +		goto set_port;
>>> +	}
>>
>> I think this patch set looks pretty reasonable.  Here's my one
>> remaining quibble.
>>
>> You can specify "port=" for nfs4 mounts, in which case we want to use
>> that value here, too, I think.  It would be simpler overall if the
>
> *Must* use a port= specification. The 2049 definition is only true for
> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.

>> kernel always passed up the value it is using for port= on this mount
>> point.
>>
>> The rules for how the kernel uses the port= setting are:
>>
>> + if port= is not specified on NFSv2/v3, port= setting is zero
>> + if port= is not specified on NFSv4, port= setting is 2049
>
> This is a tiny bit questionable, since 2049 is only defined for TCP.
> But, if port= can override, then that's a workaround, so OK.

Right.  The above rule is done in the kernel's mount option parsing  
logic, so that can/should be changed to address this.  If the kernel  
always passes the correct port up to gssd, then gssd doesn't need to  
worry about it.

>> Then, when setting up a tranport:
>>
>> + if the port= setting is zero, do an rpcbind
>> + if the port= setting is not zero, use that value
>>
>> If the kernel always passes the port= setting to gssd, then it can
>> follow the "if port value is zero, rpcbind; otherwise use port value
>> as is" rule, and be sure to get correct NFSv4 behavior, even without
>> the special case you added for NFSv4.
>
> Agree.

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

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Jeff Layton]

On Tue, 07 Apr 2009 12:27:49 -0400
Tom Talpey <tmtalpey@gmail.com> wrote:

> At 12:02 PM 4/7/2009, Chuck Lever wrote:
> >
> >On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
> >> +	/* Use standard NFS port for NFSv4 */
> >> +	if (program == 100003 && version == 4) {
> >> +		port = 2049;
> >> +		goto set_port;
> >> +	}
> >
> >I think this patch set looks pretty reasonable.  Here's my one  
> >remaining quibble.
> >
> >You can specify "port=" for nfs4 mounts, in which case we want to use  
> >that value here, too, I think.  It would be simpler overall if the  
> 
> *Must* use a port= specification. The 2049 definition is only true for
> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
> 

rpc.gssd doesn't seem to be rdma-enabled at this point. It only seems
to handle "tcp" and "udp" in the existing code.

Does libtirpc handle RDMA properly? If so, this might not be too hard
to enable, but I'd probably rather see it in a follow on patchset (and
maybe by someone with more of a clue about RDMA than I currently have).

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Tom Talpey]

At 01:11 PM 4/7/2009, Jeff Layton wrote:
>On Tue, 07 Apr 2009 12:27:49 -0400
>Tom Talpey <tmtalpey@gmail.com> wrote:
>
>> At 12:02 PM 4/7/2009, Chuck Lever wrote:
>> >
>> >On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>> >> +	/* Use standard NFS port for NFSv4 */
>> >> +	if (program == 100003 && version == 4) {
>> >> +		port = 2049;
>> >> +		goto set_port;
>> >> +	}
>> >
>> >I think this patch set looks pretty reasonable.  Here's my one  
>> >remaining quibble.
>> >
>> >You can specify "port=" for nfs4 mounts, in which case we want to use  
>> >that value here, too, I think.  It would be simpler overall if the  
>> 
>> *Must* use a port= specification. The 2049 definition is only true for
>> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
>> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
>> 
>
>rpc.gssd doesn't seem to be rdma-enabled at this point. It only seems
>to handle "tcp" and "udp" in the existing code.

Fair enough. But hardwiring 2049 for all transports is going to very
problematic. What's the motivation for bypassing the rpcbind query
altogether (that "goto set_port" skips over it)? Why not at least
try the query first?

>Does libtirpc handle RDMA properly? If so, this might not be too hard
>to enable, but I'd probably rather see it in a follow on patchset (and
>maybe by someone with more of a clue about RDMA than I currently have).

No, libtirpc doesn't have any RDMA support. But, there's no need for
RDMA support in it - only NFS does RDMA, in practice, and currently
that's just in-kernel.

My concern is simply that there be a way to specify, or discover a port
that isn't 2049 here. If mount.nfs supports it, other nfs-utils should too.

Tom.

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Jeff Layton]

On Tue, 07 Apr 2009 15:43:39 -0400
Tom Talpey <tmtalpey@gmail.com> wrote:

> At 01:11 PM 4/7/2009, Jeff Layton wrote:
> >On Tue, 07 Apr 2009 12:27:49 -0400
> >Tom Talpey <tmtalpey@gmail.com> wrote:
> >
> >> At 12:02 PM 4/7/2009, Chuck Lever wrote:
> >> >
> >> >On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
> >> >> +	/* Use standard NFS port for NFSv4 */
> >> >> +	if (program == 100003 && version == 4) {
> >> >> +		port = 2049;
> >> >> +		goto set_port;
> >> >> +	}
> >> >
> >> >I think this patch set looks pretty reasonable.  Here's my one  
> >> >remaining quibble.
> >> >
> >> >You can specify "port=" for nfs4 mounts, in which case we want to use  
> >> >that value here, too, I think.  It would be simpler overall if the  
> >> 
> >> *Must* use a port= specification. The 2049 definition is only true for
> >> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
> >> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
> >> 
> >
> >rpc.gssd doesn't seem to be rdma-enabled at this point. It only seems
> >to handle "tcp" and "udp" in the existing code.
> 
> Fair enough. But hardwiring 2049 for all transports is going to very
> problematic. What's the motivation for bypassing the rpcbind query
> altogether (that "goto set_port" skips over it)? Why not at least
> try the query first?
>
> >Does libtirpc handle RDMA properly? If so, this might not be too hard
> >to enable, but I'd probably rather see it in a follow on patchset (and
> >maybe by someone with more of a clue about RDMA than I currently have).
> 
> No, libtirpc doesn't have any RDMA support. But, there's no need for
> RDMA support in it - only NFS does RDMA, in practice, and currently
> that's just in-kernel.
> 
> My concern is simply that there be a way to specify, or discover a port
> that isn't 2049 here. If mount.nfs supports it, other nfs-utils should too.
> 

I forget which version is the cutoff, but newer kernels tell gssd which
port to use. That hardwiring is only for older kernels that don't send
the port in the upcall.

Could we try a short-timeout rpcbind call for those older kernels that
don't send the port? Sure. Is it worth it? I'm not as sure there.

I'd rather see these people just update their kernels so that this
isn't needed...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Tom Talpey]

At 03:53 PM 4/7/2009, Jeff Layton wrote:
>On Tue, 07 Apr 2009 15:43:39 -0400
>Tom Talpey <tmtalpey@gmail.com> wrote:
>
>> At 01:11 PM 4/7/2009, Jeff Layton wrote:
>> >On Tue, 07 Apr 2009 12:27:49 -0400
>> >Tom Talpey <tmtalpey@gmail.com> wrote:
>> >
>> >> At 12:02 PM 4/7/2009, Chuck Lever wrote:
>> >> >
>> >> >On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>> >> >> +	/* Use standard NFS port for NFSv4 */
>> >> >> +	if (program == 100003 && version == 4) {
>> >> >> +		port = 2049;
>> >> >> +		goto set_port;
>> >> >> +	}
>> >> >
>> >> >I think this patch set looks pretty reasonable.  Here's my one  
>> >> >remaining quibble.
>> >> >
>> >> >You can specify "port=" for nfs4 mounts, in which case we want to use  
>> >> >that value here, too, I think.  It would be simpler overall if the  
>> >> 
>> >> *Must* use a port= specification. The 2049 definition is only true for
>> >> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
>> >> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
>> >> 
>> >
>> >rpc.gssd doesn't seem to be rdma-enabled at this point. It only seems
>> >to handle "tcp" and "udp" in the existing code.
>> 
>> Fair enough. But hardwiring 2049 for all transports is going to very
>> problematic. What's the motivation for bypassing the rpcbind query
>> altogether (that "goto set_port" skips over it)? Why not at least
>> try the query first?
>>
>> >Does libtirpc handle RDMA properly? If so, this might not be too hard
>> >to enable, but I'd probably rather see it in a follow on patchset (and
>> >maybe by someone with more of a clue about RDMA than I currently have).
>> 
>> No, libtirpc doesn't have any RDMA support. But, there's no need for
>> RDMA support in it - only NFS does RDMA, in practice, and currently
>> that's just in-kernel.
>> 
>> My concern is simply that there be a way to specify, or discover a port
>> that isn't 2049 here. If mount.nfs supports it, other nfs-utils should too.
>> 
>
>I forget which version is the cutoff, but newer kernels tell gssd which
>port to use. That hardwiring is only for older kernels that don't send
>the port in the upcall.

Ah - well, if "older" is < 2.6.24, then no problem since those kernels don't
support NFS/RDMA. There are some backports to earlier kernels, but they
could address this as part of the backport.

>
>Could we try a short-timeout rpcbind call for those older kernels that
>don't send the port? Sure. Is it worth it? I'm not as sure there.

I wouldn't recommend a short timeout, arbitrary numbers rarely work
the way you might want them to. I guess my opinion is that in the
absence of any information, a standard rpcbind call is best, before
concluding a default 2049 is the only option.

>
>I'd rather see these people just update their kernels so that this
>isn't needed...

Agreed. Would it make sense to log a message when the default 2049
is used? At least then, there's a chance an admin will know it's needed.

Tom.

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Jeff Layton]

On Tue, 07 Apr 2009 16:01:31 -0400
Tom Talpey <tmtalpey@gmail.com> wrote:
> 
> >
> >I'd rather see these people just update their kernels so that this
> >isn't needed...
> 
> Agreed. Would it make sense to log a message when the default 2049
> is used? At least then, there's a chance an admin will know it's needed.
> 

That's sounds reasonable. I've already asked Steve to commit the latest
set, but we should be able to add something like this untested patch on
top of it.

If it looks ok, I'll test it out and officially resend it...

Thoughts?

----------------[snip]--------------------

>From 7ac66dd50eafec3db3c816203fc24b9b72ff1f36 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@redhat.com>
Date: Tue, 7 Apr 2009 16:27:13 -0400
Subject: [PATCH] nfs-utils: log warning when gssd detects an older kernel that doesn't send port in upcall

Older kernels don't send port information in the upcall. This could
cause mounts to fail for mysterious reasons. Log a warning the first
time that gssd encounters an upcall without any port information in it.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 02239d2..90182b0 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -99,6 +99,8 @@
  *      and rescan the whole {pipefs_nfsdir} when this happens.
  */
 
+static int warned_on_port_field;
+
 struct pollfd * pollarray;
 
 int pollsize;  /* the size of pollaray (in pollfd's) */
@@ -228,8 +230,16 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
 	}
 
 	cb_port[0] = '\0';
-	if ((p = strstr(buf, "port")) != NULL)
+	if ((p = strstr(buf, "port")) != NULL) {
 		sscanf(p, "port: %127s\n", cb_port);
+	} else if (!warned_on_port_field) {
+		warned_on_port_field = 1;
+		printerr(0, "WARNING: This kernel does not send port info "
+			    "in the upcall. That support was added in "
+			    "upstream kernel version 2.6.24. Using "
+			    "non-standard ports may not work with this "
+			    "kernel.\n");
+	}
 
 	/* check service, program, and version */
 	if(memcmp(service, "nfs", 3)) return -1;
-- 
1.6.0.6

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[J. Bruce Fields]

On Tue, Apr 07, 2009 at 04:30:19PM -0400, Jeff Layton wrote:
> On Tue, 07 Apr 2009 16:01:31 -0400
> Tom Talpey <tmtalpey@gmail.com> wrote:
> > 
> > >
> > >I'd rather see these people just update their kernels so that this
> > >isn't needed...
> > 
> > Agreed. Would it make sense to log a message when the default 2049
> > is used? At least then, there's a chance an admin will know it's needed.
> > 
> 
> That's sounds reasonable. I've already asked Steve to commit the latest
> set, but we should be able to add something like this untested patch on
> top of it.
> 
> If it looks ok, I'll test it out and officially resend it...
> 
> Thoughts?

Would it work to do this in mount?--do the mount, then check any new
info file in rpc_pipefs to see if there's a port field?

Then we could give the error to the user where it's actually useful (as
output from the mount command).  And also maybe only bother with it when
port= was specified.

--b.

> 
> ----------------[snip]--------------------
> 
> >From 7ac66dd50eafec3db3c816203fc24b9b72ff1f36 Mon Sep 17 00:00:00 2001
> From: Jeff Layton <jlayton@redhat.com>
> Date: Tue, 7 Apr 2009 16:27:13 -0400
> Subject: [PATCH] nfs-utils: log warning when gssd detects an older kernel that doesn't send port in upcall
> 
> Older kernels don't send port information in the upcall. This could
> cause mounts to fail for mysterious reasons. Log a warning the first
> time that gssd encounters an upcall without any port information in it.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  utils/gssd/gssd_proc.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 02239d2..90182b0 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -99,6 +99,8 @@
>   *      and rescan the whole {pipefs_nfsdir} when this happens.
>   */
>  
> +static int warned_on_port_field;
> +
>  struct pollfd * pollarray;
>  
>  int pollsize;  /* the size of pollaray (in pollfd's) */
> @@ -228,8 +230,16 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
>  	}
>  
>  	cb_port[0] = '\0';
> -	if ((p = strstr(buf, "port")) != NULL)
> +	if ((p = strstr(buf, "port")) != NULL) {
>  		sscanf(p, "port: %127s\n", cb_port);
> +	} else if (!warned_on_port_field) {
> +		warned_on_port_field = 1;
> +		printerr(0, "WARNING: This kernel does not send port info "
> +			    "in the upcall. That support was added in "
> +			    "upstream kernel version 2.6.24. Using "
> +			    "non-standard ports may not work with this "
> +			    "kernel.\n");
> +	}
>  
>  	/* check service, program, and version */
>  	if(memcmp(service, "nfs", 3)) return -1;
> -- 
> 1.6.0.6
> _______________________________________________
> NFSv4 mailing list
> NFSv4@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Jeff Layton]

On Tue, 7 Apr 2009 19:16:06 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Apr 07, 2009 at 04:30:19PM -0400, Jeff Layton wrote:
> > On Tue, 07 Apr 2009 16:01:31 -0400
> > Tom Talpey <tmtalpey@gmail.com> wrote:
> > > 
> > > >
> > > >I'd rather see these people just update their kernels so that this
> > > >isn't needed...
> > > 
> > > Agreed. Would it make sense to log a message when the default 2049
> > > is used? At least then, there's a chance an admin will know it's needed.
> > > 
> > 
> > That's sounds reasonable. I've already asked Steve to commit the latest
> > set, but we should be able to add something like this untested patch on
> > top of it.
> > 
> > If it looks ok, I'll test it out and officially resend it...
> > 
> > Thoughts?
> 
> Would it work to do this in mount?--do the mount, then check any new
> info file in rpc_pipefs to see if there's a port field?
> 
> Then we could give the error to the user where it's actually useful (as
> output from the mount command).  And also maybe only bother with it when
> port= was specified.
> 
> --b.
> 

It's not really an error per-se. Given that 99%+ NFS connections are
done on standard ports, I consider this to just be a helpful warning
that might point someone in the right direction when this does occur.

Adding code to mount for this seems like overkill, IMO...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[J. Bruce Fields]

On Tue, Apr 07, 2009 at 03:43:39PM -0400, Tom Talpey wrote:
> At 01:11 PM 4/7/2009, Jeff Layton wrote:
> >On Tue, 07 Apr 2009 12:27:49 -0400
> >Tom Talpey <tmtalpey@gmail.com> wrote:
> >
> >> At 12:02 PM 4/7/2009, Chuck Lever wrote:
> >> >
> >> >On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
> >> >> +	/* Use standard NFS port for NFSv4 */
> >> >> +	if (program == 100003 && version == 4) {
> >> >> +		port = 2049;
> >> >> +		goto set_port;
> >> >> +	}
> >> >
> >> >I think this patch set looks pretty reasonable.  Here's my one  
> >> >remaining quibble.
> >> >
> >> >You can specify "port=" for nfs4 mounts, in which case we want to use  
> >> >that value here, too, I think.  It would be simpler overall if the  
> >> 
> >> *Must* use a port= specification. The 2049 definition is only true for
> >> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
> >> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
> >> 
> >
> >rpc.gssd doesn't seem to be rdma-enabled at this point. It only seems
> >to handle "tcp" and "udp" in the existing code.
> 
> Fair enough. But hardwiring 2049 for all transports is going to very
> problematic. What's the motivation for bypassing the rpcbind query
> altogether (that "goto set_port" skips over it)? Why not at least
> try the query first?

We're just doing the rpcsec_gss context initiation.  Normally that would
be done over an already-established connection--the only reason we don't
is because our implementation is split between the client and the
server, so it's more convenient for us to set up a new connection in
rpc.gssd.  But we really shouldn't be doing an entirely new rpcbind
call--somebody else already did that for us and is telling us the
results through the rpc_pipefs info file.

--b.

> 
> >Does libtirpc handle RDMA properly? If so, this might not be too hard
> >to enable, but I'd probably rather see it in a follow on patchset (and
> >maybe by someone with more of a clue about RDMA than I currently have).
> 
> No, libtirpc doesn't have any RDMA support. But, there's no need for
> RDMA support in it - only NFS does RDMA, in practice, and currently
> that's just in-kernel.
> 
> My concern is simply that there be a way to specify, or discover a port
> that isn't 2049 here. If mount.nfs supports it, other nfs-utils should too.
> 
> Tom.
> 
> _______________________________________________
> NFSv4 mailing list
> NFSv4@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Jeff Layton]

On Tue, 7 Apr 2009 19:14:06 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Apr 07, 2009 at 03:43:39PM -0400, Tom Talpey wrote:
> > At 01:11 PM 4/7/2009, Jeff Layton wrote:
> > >On Tue, 07 Apr 2009 12:27:49 -0400
> > >Tom Talpey <tmtalpey@gmail.com> wrote:
> > >
> > >> At 12:02 PM 4/7/2009, Chuck Lever wrote:
> > >> >
> > >> >On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
> > >> >> +	/* Use standard NFS port for NFSv4 */
> > >> >> +	if (program == 100003 && version == 4) {
> > >> >> +		port = 2049;
> > >> >> +		goto set_port;
> > >> >> +	}
> > >> >
> > >> >I think this patch set looks pretty reasonable.  Here's my one  
> > >> >remaining quibble.
> > >> >
> > >> >You can specify "port=" for nfs4 mounts, in which case we want to use  
> > >> >that value here, too, I think.  It would be simpler overall if the  
> > >> 
> > >> *Must* use a port= specification. The 2049 definition is only true for
> > >> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
> > >> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
> > >> 
> > >
> > >rpc.gssd doesn't seem to be rdma-enabled at this point. It only seems
> > >to handle "tcp" and "udp" in the existing code.
> > 
> > Fair enough. But hardwiring 2049 for all transports is going to very
> > problematic. What's the motivation for bypassing the rpcbind query
> > altogether (that "goto set_port" skips over it)? Why not at least
> > try the query first?
> 
> We're just doing the rpcsec_gss context initiation.  Normally that would
> be done over an already-established connection--the only reason we don't
> is because our implementation is split between the client and the
> server, so it's more convenient for us to set up a new connection in
> rpc.gssd.  But we really shouldn't be doing an entirely new rpcbind
> call--somebody else already did that for us and is telling us the
> results through the rpc_pipefs info file.
> 

Technically, the info file doesn't display the port from an rpcbind
call. That field is populated before the kernel's rpcbind call is
done. The contents will be one of:

- the value of the port= mount option if one was specified
- 2049 if it's an nfs4 mount and there is no port= option specified
- 0 if nfsv2/3 and no port= option was specified

...its real value is in allowing gssd to skip the rpcbind call if
someone specified port= when doing the mount. This could probably
be changed, but I'm not sure it's such a big deal. It only means an
extra rpcbind call at mount time for NFSv2/3. Further upcalls for
other creds by the same client don't incur extra rpcbind queries.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Chuck Lever]

On Apr 7, 2009, at 7:14 PM, J. Bruce Fields wrote:
> On Tue, Apr 07, 2009 at 03:43:39PM -0400, Tom Talpey wrote:
>> At 01:11 PM 4/7/2009, Jeff Layton wrote:
>>> On Tue, 07 Apr 2009 12:27:49 -0400
>>> Tom Talpey <tmtalpey@gmail.com> wrote:
>>>
>>>> At 12:02 PM 4/7/2009, Chuck Lever wrote:
>>>>>
>>>>> On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>>>>>> +	/* Use standard NFS port for NFSv4 */
>>>>>> +	if (program == 100003 && version == 4) {
>>>>>> +		port = 2049;
>>>>>> +		goto set_port;
>>>>>> +	}
>>>>>
>>>>> I think this patch set looks pretty reasonable.  Here's my one
>>>>> remaining quibble.
>>>>>
>>>>> You can specify "port=" for nfs4 mounts, in which case we want  
>>>>> to use
>>>>> that value here, too, I think.  It would be simpler overall if the
>>>>
>>>> *Must* use a port= specification. The 2049 definition is only  
>>>> true for
>>>> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
>>>> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
>>>>
>>>
>>> rpc.gssd doesn't seem to be rdma-enabled at this point. It only  
>>> seems
>>> to handle "tcp" and "udp" in the existing code.
>>
>> Fair enough. But hardwiring 2049 for all transports is going to very
>> problematic. What's the motivation for bypassing the rpcbind query
>> altogether (that "goto set_port" skips over it)? Why not at least
>> try the query first?
>
> We're just doing the rpcsec_gss context initiation.  Normally that  
> would
> be done over an already-established connection--the only reason we  
> don't
> is because our implementation is split between the client and the
> server, so it's more convenient for us to set up a new connection in
> rpc.gssd.  But we really shouldn't be doing an entirely new rpcbind
> call--somebody else already did that for us and is telling us the
> results through the rpc_pipefs info file.

This is an entirely separate RPC transport being set up for gssd, and  
there seem to be a lot of cases where the kernel (rightfully) passes a  
zero for the port number.  In fact, even if the kernel did the rpcbind  
for gssd at some point in the past, gss doesn't have any information  
about how old that information is.

So, yes, we do want an rpcbind here.

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

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[J. Bruce Fields]

On Wed, Apr 08, 2009 at 03:32:43PM -0400, Chuck Lever wrote:
> On Apr 7, 2009, at 7:14 PM, J. Bruce Fields wrote:
>> On Tue, Apr 07, 2009 at 03:43:39PM -0400, Tom Talpey wrote:
>>> At 01:11 PM 4/7/2009, Jeff Layton wrote:
>>>> On Tue, 07 Apr 2009 12:27:49 -0400
>>>> Tom Talpey <tmtalpey@gmail.com> wrote:
>>>>
>>>>> At 12:02 PM 4/7/2009, Chuck Lever wrote:
>>>>>>
>>>>>> On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>>>>>>> +	/* Use standard NFS port for NFSv4 */
>>>>>>> +	if (program == 100003 && version == 4) {
>>>>>>> +		port = 2049;
>>>>>>> +		goto set_port;
>>>>>>> +	}
>>>>>>
>>>>>> I think this patch set looks pretty reasonable.  Here's my one
>>>>>> remaining quibble.
>>>>>>
>>>>>> You can specify "port=" for nfs4 mounts, in which case we want  
>>>>>> to use
>>>>>> that value here, too, I think.  It would be simpler overall if the
>>>>>
>>>>> *Must* use a port= specification. The 2049 definition is only  
>>>>> true for
>>>>> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
>>>>> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
>>>>>
>>>>
>>>> rpc.gssd doesn't seem to be rdma-enabled at this point. It only  
>>>> seems
>>>> to handle "tcp" and "udp" in the existing code.
>>>
>>> Fair enough. But hardwiring 2049 for all transports is going to very
>>> problematic. What's the motivation for bypassing the rpcbind query
>>> altogether (that "goto set_port" skips over it)? Why not at least
>>> try the query first?
>>
>> We're just doing the rpcsec_gss context initiation.  Normally that  
>> would
>> be done over an already-established connection--the only reason we  
>> don't
>> is because our implementation is split between the client and the
>> server, so it's more convenient for us to set up a new connection in
>> rpc.gssd.  But we really shouldn't be doing an entirely new rpcbind
>> call--somebody else already did that for us and is telling us the
>> results through the rpc_pipefs info file.
>
> This is an entirely separate RPC transport being set up for gssd, and  
> there seem to be a lot of cases where the kernel (rightfully) passes a  
> zero for the port number.  In fact, even if the kernel did the rpcbind  
> for gssd at some point in the past, gss doesn't have any information  
> about how old that information is.
>
> So, yes, we do want an rpcbind here.

As long as we don't do it in the NFSv4 case, I'm happy.

My only point is that the above argument is putting the cart before the
horse a bit: the fact that there's an entirely separate RPC transport
being set up is entirely an artifact of our implementation, and if it
became a problem at some point, then we'd need to consider modifying the
kernel/gssd interface to eliminate the need for that.

--b.

Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

[Chuck Lever]

Hi Bruce-

On Apr 9, 2009, at 12:19 PM, J. Bruce Fields wrote:
> On Wed, Apr 08, 2009 at 03:32:43PM -0400, Chuck Lever wrote:
>> On Apr 7, 2009, at 7:14 PM, J. Bruce Fields wrote:
>>> On Tue, Apr 07, 2009 at 03:43:39PM -0400, Tom Talpey wrote:
>>>> At 01:11 PM 4/7/2009, Jeff Layton wrote:
>>>>> On Tue, 07 Apr 2009 12:27:49 -0400
>>>>> Tom Talpey <tmtalpey@gmail.com> wrote:
>>>>>
>>>>>> At 12:02 PM 4/7/2009, Chuck Lever wrote:
>>>>>>>
>>>>>>> On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>>>>>>>> +	/* Use standard NFS port for NFSv4 */
>>>>>>>> +	if (program == 100003 && version == 4) {
>>>>>>>> +		port = 2049;
>>>>>>>> +		goto set_port;
>>>>>>>> +	}
>>>>>>>
>>>>>>> I think this patch set looks pretty reasonable.  Here's my one
>>>>>>> remaining quibble.
>>>>>>>
>>>>>>> You can specify "port=" for nfs4 mounts, in which case we want
>>>>>>> to use
>>>>>>> that value here, too, I think.  It would be simpler overall if  
>>>>>>> the
>>>>>>
>>>>>> *Must* use a port= specification. The 2049 definition is only
>>>>>> true for
>>>>>> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
>>>>>> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
>>>>>>
>>>>>
>>>>> rpc.gssd doesn't seem to be rdma-enabled at this point. It only
>>>>> seems
>>>>> to handle "tcp" and "udp" in the existing code.
>>>>
>>>> Fair enough. But hardwiring 2049 for all transports is going to  
>>>> very
>>>> problematic. What's the motivation for bypassing the rpcbind query
>>>> altogether (that "goto set_port" skips over it)? Why not at least
>>>> try the query first?
>>>
>>> We're just doing the rpcsec_gss context initiation.  Normally that
>>> would
>>> be done over an already-established connection--the only reason we
>>> don't
>>> is because our implementation is split between the client and the
>>> server, so it's more convenient for us to set up a new connection in
>>> rpc.gssd.  But we really shouldn't be doing an entirely new rpcbind
>>> call--somebody else already did that for us and is telling us the
>>> results through the rpc_pipefs info file.
>>
>> This is an entirely separate RPC transport being set up for gssd, and
>> there seem to be a lot of cases where the kernel (rightfully)  
>> passes a
>> zero for the port number.  In fact, even if the kernel did the  
>> rpcbind
>> for gssd at some point in the past, gss doesn't have any information
>> about how old that information is.
>>
>> So, yes, we do want an rpcbind here.
>
> As long as we don't do it in the NFSv4 case, I'm happy.

As far as I understand it, the kernel won't send up a zero for NFSv4  
unless the mount command line says "port=0" which forces an rpcbind  
for each transport we use to connect to that server.

For NFSv2/v3, however, not specifying port= on the mount command line  
means the kernel will send up zero in most common cases, I think.   
Using /etc/services on the client to discover the server's NFSD port  
means that if the server has registered the NFS service on some other  
port, specifying "port=" will be required on the mount command line  
for gssd to work, even though rpcbind on the server is advertising the  
correct port already.

> My only point is that the above argument is putting the cart before  
> the
> horse a bit: the fact that there's an entirely separate RPC transport
> being set up is entirely an artifact of our implementation, and if it
> became a problem at some point, then we'd need to consider modifying  
> the
> kernel/gssd interface to eliminate the need for that.


port=0 has a specific well-defined meaning for RPC, which is "do an  
rpcbind".  If you really think you need to distinguish between "use  
this port number," "do an rpcbind," "use the standard port for this  
transport," and "consult /etc/services" then I don't think port=0 is  
adequate.

Should we even bother to consult /etc/services for NFSv4, since that  
port value is required by standard?  nfs4mount.c doesn't look at /etc/ 
services, for example.  So today, gssd looks in /etc/services,  
nfs4mount.c and the kernel hard-code the port number.

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

[PATCH 4/5] nfs-utils: switch gssd to use standard function for getting an RPC client

[Jeff Layton]

We already have a common function for setting up an RPC client. That
function uses the tirpc API when tirpc is enabled and is also already
IPv6 enabled. Switch gssd to use it.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c |   41 ++++++++++++-----------------------------
 1 files changed, 12 insertions(+), 29 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 1571329..5b97d6c 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -616,9 +616,8 @@ int create_auth_rpc_client(struct clnt_info *clp,
 	int			retval = -1;
 	OM_uint32		min_stat;
 	char			rpc_errmsg[1024];
-	int			sockp = RPC_ANYSOCK;
-	int			sendsz = 32768, recvsz = 32768;
 	int			protocol;
+	struct timeval		timeout = {5, 0};
 	struct sockaddr		*addr = (struct sockaddr *) &clp->addr;
 	socklen_t		salen;
 
@@ -698,33 +697,17 @@ int create_auth_rpc_client(struct clnt_info *clp,
 	if (!populate_port(addr, salen, clp->prog, clp->vers, protocol))
 		goto out_fail;
 
-	if (protocol == IPPROTO_TCP) {
-		if ((rpc_clnt = clnttcp_create(
-					(struct sockaddr_in *) addr,
-					clp->prog, clp->vers, &sockp,
-					sendsz, recvsz)) == NULL) {
-			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
-				 "WARNING: can't create tcp rpc_clnt "
-				 "for server %s for user with uid %d",
-				 clp->servername, uid);
-			printerr(0, "%s\n",
-				 clnt_spcreateerror(rpc_errmsg));
-			goto out_fail;
-		}
-	} else if (protocol == IPPROTO_UDP) {
-		const struct timeval timeout = {5, 0};
-		if ((rpc_clnt = clntudp_bufcreate(
-					(struct sockaddr_in *) addr,
-					clp->prog, clp->vers, timeout,
-					&sockp, sendsz, recvsz)) == NULL) {
-			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
-				 "WARNING: can't create udp rpc_clnt "
-				 "for server %s for user with uid %d",
-				 clp->servername, uid);
-			printerr(0, "%s\n",
-				 clnt_spcreateerror(rpc_errmsg));
-			goto out_fail;
-		}
+	rpc_clnt = nfs_get_rpcclient(addr, salen, protocol, clp->prog,
+				     clp->vers, &timeout);
+	if (!rpc_clnt) {
+		snprintf(rpc_errmsg, sizeof(rpc_errmsg),
+			 "WARNING: can't create %s rpc_clnt to server %s for "
+			 "user with uid %d",
+			 protocol == IPPROTO_TCP ? "tcp" : "udp",
+			 clp->servername, uid);
+		printerr(0, "%s\n",
+			 clnt_spcreateerror(rpc_errmsg));
+		goto out_fail;
 	}
 
 	printerr(2, "creating context with server %s\n", clp->servicename);
-- 
1.6.0.6

[PATCH 5/5] nfs-utils: add IPv6 code to gssd

[Jeff Layton]

All of the pieces to handle IPv6 are now in place. Add IPv6-specific
code wrapped in the proper #ifdef's so that IPv6 support works when
it's enabled at build-time.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 5b97d6c..70094cf 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -106,15 +106,32 @@ int pollsize;  /* the size of pollaray (in pollfd's) */
 /*
  * convert a presentation address string to a sockaddr_storage struct. Returns
  * true on success and false on failure.
+ *
+ * Note that we do not populate the sin6_scope_id field here for IPv6 addrs.
+ * gssd nececessarily relies on hostname resolution and DNS AAAA records
+ * do not generally contain scope-id's. This means that GSSAPI auth really
+ * can't work with IPv6 link-local addresses.
+ *
+ * We *could* consider changing this if we did something like adopt the
+ * Microsoft "standard" of using the ipv6-literal.net domainname, but it's
+ * not really feasible at present.
  */
 static int
 addrstr_to_sockaddr(struct sockaddr *sa, const char *addr, const int port)
 {
 	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
+#ifdef IPV6_SUPPORTED
+	struct sockaddr_in6	*s6 = (struct sockaddr_in6 *) sa;
+#endif /* IPV6_SUPPORTED */
 
 	if (inet_pton(AF_INET, addr, &s4->sin_addr)) {
 		s4->sin_family = AF_INET;
 		s4->sin_port = htons(port);
+#ifdef IPV6_SUPPORTED
+	} else if (inet_pton(AF_INET6, addr, &s6->sin6_addr)) {
+		s6->sin6_family = AF_INET6;
+		s6->sin6_port = htons(port);
+#endif /* IPV6_SUPPORTED */
 	} else {
 		printerr(0, "ERROR: unable to convert %s to address\n", addr);
 		return 0;
@@ -138,6 +155,11 @@ sockaddr_to_hostname(const struct sockaddr *sa, const char *addr)
 	case AF_INET:
 		addrlen = sizeof(struct sockaddr_in);
 		break;
+#ifdef IPV6_SUPPORTED
+	case AF_INET6:
+		addrlen = sizeof(struct sockaddr_in6);
+		break;
+#endif /* IPV6_SUPPORTED */
 	default:
 		printerr(0, "ERROR: unrecognized addr family %d\n",
 			 sa->sa_family);
@@ -553,6 +575,9 @@ populate_port(struct sockaddr *sa, const socklen_t salen,
 	      const unsigned short protocol)
 {
 	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
+#ifdef IPV6_SUPPORTED
+	struct sockaddr_in6	*s6 = (struct sockaddr_in6 *) sa;
+#endif /* IPV6_SUPPORTED */
 	unsigned short		port;
 
 	/*
@@ -567,6 +592,15 @@ populate_port(struct sockaddr *sa, const socklen_t salen,
 			return 1;
 		}
 		break;
+#ifdef IPV6_SUPPORTED
+	case AF_INET6:
+		if (s6->sin6_port != 0) {
+			printerr(2, "DEBUG: port already set to %d\n",
+				 ntohs(s6->sin6_port));
+			return 1;
+		}
+		break;
+#endif /* IPV6_SUPPORTED */
 	default:
 		printerr(0, "ERROR: unsupported address family %d\n",
 			    sa->sa_family);
@@ -594,6 +628,11 @@ set_port:
 	case AF_INET:
 		s4->sin_port = htons(port);
 		break;
+#ifdef IPV6_SUPPORTED
+	case AF_INET6:
+		s6->sin6_port = htons(port);
+		break;
+#endif /* IPV6_SUPPORTED */
 	}
 
 	return 1;
@@ -688,6 +727,11 @@ int create_auth_rpc_client(struct clnt_info *clp,
 	case AF_INET:
 		salen = sizeof(struct sockaddr_in);
 		break;
+#ifdef IPV6_SUPPORTED
+	case AF_INET6:
+		salen = sizeof(struct sockaddr_in6);
+		break;
+#endif /* IPV6_SUPPORTED */
 	default:
 		printerr(1, "ERROR: Unknown address family %d\n",
 			 addr->sa_family);
-- 
1.6.0.6

Re: [PATCH 0/5] nfs-utils: convert gssd to TI-RPC and add IPv6 support (try #4)

[Steve Dickson]

Jeff Layton wrote:
> This patchset is the fourth attempt at adding support for IPv6 to
> rpc.gssd. The main change from the last set is that this one now uses a
> rpcbind query to determine the server's port rather than doing a
> getaddrinfo call to query the local services db.
> 
> The series should be fully bisectable, but I've only really tested the
> end result for anything other than to see if it compiles. With these
> patches I've been able to mount an OpenSolaris server using NFSv3/4 +
> IPv6 + krb5 auth. I've also done testing with builds with only TIRPC
> enabled and with TIRPC and IPv6 both disabled and haven't seen any
> regressions.
> 
> List of changes since the last set:
> - use rpcbind query to determine port for RPC client
> - added comment explaining gssd doesn't deal with IPv6 scope_id's
> - slight cleanups and clarifications to comments
> - properly handle EAI_SYSTEM return code from  getnameinfo() call
> - changed autoconf check for getnameinfo to check whenever --enable-gss
>   is set, not just when NFSv4 is also enabled.
> 
> Jeff Layton (5):
>   nfs-utils: make getnameinfo() required for --enable-gss
>   nfs-utils: store the address given in the upcall for later use
>   nfs-utils: query for remote port using rpcbind instead of getaddrinfo
>   nfs-utils: switch gssd to use standard function for getting an RPC
>     client
>   nfs-utils: add IPv6 code to gssd
> 
>  configure.ac           |    3 +
>  utils/gssd/gssd.h      |    2 +-
>  utils/gssd/gssd_proc.c |  286 +++++++++++++++++++++++++++++++++---------------
>  3 files changed, 204 insertions(+), 87 deletions(-)
> 
Committed.... 

steved.