[PATCH] rpcbind: Only listen on requested hostnames for NC_TPI_COTS connections

[PATCH] rpcbind: Only listen on requested hostnames for NC_TPI_COTS connections

[Niels de Vos]

There is a check for NC_TPI_CLTS connections which correctly binds only
to the hostnames/addresses given on the commandline (-h option). This
implementation does not take non NC_TPI_CLTS like NC_TPI_COTS and
NC_TPI_COTS_ORD into account. This causes non NC_TPI_CLTS protocols to
listen on the "0.0.0.0" and "::" wildcard addresses.

This patch simplifies the code, and moves the check for NC_TPI_CLTS into
the loop when a bind() for each requested address is done. All
connections, including NC_TPI_COTS and NC_TPI_COTS_ORD can now benefit
from resticted bind()'ing on addresses.

Signed-off-by: Niels de Vos <ndevos@redhat.com>
---
 src/rpcbind.c |  230 ++++++++++++++++++++------------------------------------
 1 files changed, 82 insertions(+), 148 deletions(-)

diff --git a/src/rpcbind.c b/src/rpcbind.c
index 9a0504d..3f6c2a9 100644
--- a/src/rpcbind.c
+++ b/src/rpcbind.c
@@ -339,172 +339,103 @@ init_transport(struct netconfig *nconf)
 		hints.ai_socktype = si.si_socktype;
 		hints.ai_protocol = si.si_proto;
 	}
-	if (nconf->nc_semantics == NC_TPI_CLTS) {
+
+	/*
+	 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
+	 * make sure 127.0.0.1 is added to the list.
+	 */
+	nhostsbak = nhosts;
+	nhostsbak++;
+	hosts = realloc(hosts, nhostsbak * sizeof(char *));
+	if (nhostsbak == 1)
+		hosts[0] = "*";
+	else {
+		if (hints.ai_family == AF_INET) {
+			hosts[nhostsbak - 1] = "127.0.0.1";
+		} else if (hints.ai_family == AF_INET6) {
+			hosts[nhostsbak - 1] = "::1";
+		} else
+			return 1;
+	}
+
+       /*
+	* Bind to specific IPs if asked to
+	*/
+	checkbind = 0;
+	while (nhostsbak > 0) {
+		--nhostsbak;
 		/*
-		 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
-		 * make sure 127.0.0.1 is added to the list.
+		 * XXX - using RPC library internal functions.
 		 */
-		nhostsbak = nhosts;
-		nhostsbak++;
-		hosts = realloc(hosts, nhostsbak * sizeof(char *));
-		if (nhostsbak == 1)
-			hosts[0] = "*";
-		else {
-			if (hints.ai_family == AF_INET) {
-				hosts[nhostsbak - 1] = "127.0.0.1";
-			} else if (hints.ai_family == AF_INET6) {
-				hosts[nhostsbak - 1] = "::1";
-			} else
-				return 1;
+		if ((fd = __rpc_nconf2fd(nconf)) < 0) {
+			syslog(LOG_ERR, "cannot create socket for %s",
+			    nconf->nc_netid);
+			return (1);
 		}
-
-	       /*
-		* Bind to specific IPs if asked to
-		*/
-		checkbind = 0;
-		while (nhostsbak > 0) {
-			--nhostsbak;
-			/*
-			 * XXX - using RPC library internal functions.
-			 */
-			if ((fd = __rpc_nconf2fd(nconf)) < 0) {
-				syslog(LOG_ERR, "cannot create socket for %s",
-				    nconf->nc_netid);
-				return (1);
+		switch (hints.ai_family) {
+		case AF_INET:
+			if (inet_pton(AF_INET, hosts[nhostsbak],
+			    host_addr) == 1) {
+				hints.ai_flags &= AI_NUMERICHOST;
+			} else {
+				/*
+				 * Skip if we have an AF_INET6 adress.
+				 */
+				if (inet_pton(AF_INET6,
+				    hosts[nhostsbak], host_addr) == 1)
+					continue;
 			}
-			switch (hints.ai_family) {
-			case AF_INET:
+			break;
+		case AF_INET6:
+			if (inet_pton(AF_INET6, hosts[nhostsbak],
+			    host_addr) == 1) {
+				hints.ai_flags &= AI_NUMERICHOST;
+			} else {
+				/*
+				 * Skip if we have an AF_INET adress.
+				 */
 				if (inet_pton(AF_INET, hosts[nhostsbak],
-				    host_addr) == 1) {
-					hints.ai_flags &= AI_NUMERICHOST;
-				} else {
-					/*
-					 * Skip if we have an AF_INET6 adress.
-					 */
-					if (inet_pton(AF_INET6,
-					    hosts[nhostsbak], host_addr) == 1)
-						continue;
-				}
-				break;
-			case AF_INET6:
-				if (inet_pton(AF_INET6, hosts[nhostsbak],
-				    host_addr) == 1) {
-					hints.ai_flags &= AI_NUMERICHOST;
-				} else {
-					/*
-					 * Skip if we have an AF_INET adress.
-					 */
-					if (inet_pton(AF_INET, hosts[nhostsbak],
-					    host_addr) == 1)
-						continue;
-				}
-	        		break;
-			default:
-				break;
+				    host_addr) == 1)
+					continue;
 			}
+			break;
+		default:
+			break;
+		}
 
-			/*
-			 * If no hosts were specified, just bind to INADDR_ANY
-			 */
-			if (strcmp("*", hosts[nhostsbak]) == 0)
-				hosts[nhostsbak] = NULL;
+		/*
+		 * If no hosts were specified, just bind to INADDR_ANY
+		 */
+		if (strcmp("*", hosts[nhostsbak]) == 0)
+			hosts[nhostsbak] = NULL;
 
+		if ((strcmp(nconf->nc_netid, "local") != 0) &&
+		    (strcmp(nconf->nc_netid, "unix") != 0)) {
 			if ((aicode = getaddrinfo(hosts[nhostsbak],
-			    servname, &hints, &res)) != 0) {
+			    servname, &hints, &res))!= 0) {
 			  if ((aicode = getaddrinfo(hosts[nhostsbak],
-						    "portmapper", &hints, &res)) != 0) {
-				syslog(LOG_ERR,
+					    "portmapper", &hints, &res))!= 0) {
+			    syslog(LOG_ERR,
 				    "cannot get local address for %s: %s",
 				    nconf->nc_netid, gai_strerror(aicode));
-				continue;
+			    continue;
 			  }
 			}
 			addrlen = res->ai_addrlen;
 			sa = (struct sockaddr *)res->ai_addr;
-			oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
-                        if (bind(fd, sa, addrlen) != 0) {
-				syslog(LOG_ERR, "cannot bind %s on %s: %m",
-					(hosts[nhostsbak] == NULL) ? "*" :
-					hosts[nhostsbak], nconf->nc_netid);
-				if (res != NULL)
-					freeaddrinfo(res);
-				continue;
-			} else
-				checkbind++;
-			(void) umask(oldmask);
-
-			/* Copy the address */
-			taddr.addr.maxlen = taddr.addr.len = addrlen;
-			taddr.addr.buf = malloc(addrlen);
-			if (taddr.addr.buf == NULL) {
-				syslog(LOG_ERR,
-				    "cannot allocate memory for %s address",
-				    nconf->nc_netid);
+		}
+		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
+		if (nconf->nc_semantics != NC_TPI_CLTS) {
+			/* NC_TPI_COTS and NC_TPI_COTS_ORD etc */
+			__rpc_fd2sockinfo(fd, &si);
+			if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &on,
+			    sizeof(on)) != 0) {
+				syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
+					nconf->nc_netid);
 				if (res != NULL)
 					freeaddrinfo(res);
 				return 1;
 			}
-			memcpy(taddr.addr.buf, sa, addrlen);
-#ifdef RPCBIND_DEBUG
-			if (debugging) {
-				/*
-				 * for debugging print out our universal
-				 * address
-				 */
-				char *uaddr;
-				struct netbuf nb;
-				int sa_size = 0;
-
-				nb.buf = sa;
-				switch( sa->sa_family){
-				case AF_INET:
-				  sa_size = sizeof (struct sockaddr_in);
-				  break;
-				case AF_INET6:
-				  sa_size = sizeof (struct sockaddr_in6);				 
-				  break;
-				}
-				nb.len = nb.maxlen = sa_size;
-				uaddr = taddr2uaddr(nconf, &nb);
-				(void) fprintf(stderr,
-				    "rpcbind : my address is %s\n", uaddr);
-				(void) free(uaddr);
-			}
-#endif
-			my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf, &taddr, 
-                                RPC_MAXDATASIZE, RPC_MAXDATASIZE);
-			if (my_xprt == (SVCXPRT *)NULL) {
-				syslog(LOG_ERR, "%s: could not create service", 
-                                        nconf->nc_netid);
-				goto error;
-			}
-		}
-		if (!checkbind)
-			return 1;
-	} else {	/* NC_TPI_COTS */
-		if ((strcmp(nconf->nc_netid, "local") != 0) &&
-		    (strcmp(nconf->nc_netid, "unix") != 0)) {
-			if ((aicode = getaddrinfo(NULL, servname, &hints, &res))!= 0) {
-			  if ((aicode = getaddrinfo(NULL, "portmapper", &hints, &res))!= 0) {
-			  printf("cannot get local address for %s: %s",  nconf->nc_netid, gai_strerror(aicode));
-			  syslog(LOG_ERR,
-				    "cannot get local address for %s: %s",
-				    nconf->nc_netid, gai_strerror(aicode));
-				return 1;
-			  }
-			}
-			addrlen = res->ai_addrlen;
-			sa = (struct sockaddr *)res->ai_addr;
-		}
-		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
-		__rpc_fd2sockinfo(fd, &si);
-		if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &on,
-				sizeof(on)) != 0) {
-			syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
-				nconf->nc_netid);
-			if (res != NULL)
-				freeaddrinfo(res);
-			return 1;
 		}
 		if (bind(fd, sa, addrlen) < 0) {
 			syslog(LOG_ERR, "cannot bind %s: %m", nconf->nc_netid);
@@ -530,7 +461,7 @@ init_transport(struct netconfig *nconf)
 			/* for debugging print out our universal address */
 			char *uaddr;
 			struct netbuf nb;
-		        int sa_size2 = 0;
+			int sa_size2 = 0;
 
 			nb.buf = sa;
 			switch( sa->sa_family){
@@ -551,13 +482,16 @@ init_transport(struct netconfig *nconf)
 
 		listen(fd, SOMAXCONN);
 
-		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf, &taddr, RPC_MAXDATASIZE, RPC_MAXDATASIZE);
+		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf, &taddr,
+			    RPC_MAXDATASIZE, RPC_MAXDATASIZE);
 		if (my_xprt == (SVCXPRT *)NULL) {
 			syslog(LOG_ERR, "%s: could not create service",
 					nconf->nc_netid);
 			goto error;
 		}
 	}
+	if (!checkbind)
+		return 1;
 
 #ifdef PORTMAP
 	/*
-- 
1.7.7.6

Re: [PATCH] rpcbind: Only listen on requested hostnames for NC_TPI_COTS connections

[Chuck Lever]

Hi-

On Apr 23, 2012, at 11:24 AM, Niels de Vos wrote:

> There is a check for NC_TPI_CLTS connections which correctly binds only
> to the hostnames/addresses given on the commandline (-h option). This
> implementation does not take non NC_TPI_CLTS like NC_TPI_COTS and
> NC_TPI_COTS_ORD into account. This causes non NC_TPI_CLTS protocols to
> listen on the "0.0.0.0" and "::" wildcard addresses.
> 
> This patch simplifies the code, and moves the check for NC_TPI_CLTS into
> the loop when a bind() for each requested address is done. All
> connections, including NC_TPI_COTS and NC_TPI_COTS_ORD can now benefit
> from resticted bind()'ing on addresses.

Please check the mailing list archives for linux-nfs and/or libtirpc-devel.  There may be a good reason for this asymmetrical behavior, or at least some enlightening discussion of this idea.

Also, can you tell us why you need this change in the patch description?

> Signed-off-by: Niels de Vos <ndevos@redhat.com>
> ---
> src/rpcbind.c |  230 ++++++++++++++++++++------------------------------------
> 1 files changed, 82 insertions(+), 148 deletions(-)
> 
> diff --git a/src/rpcbind.c b/src/rpcbind.c
> index 9a0504d..3f6c2a9 100644
> --- a/src/rpcbind.c
> +++ b/src/rpcbind.c
> @@ -339,172 +339,103 @@ init_transport(struct netconfig *nconf)
> 		hints.ai_socktype = si.si_socktype;
> 		hints.ai_protocol = si.si_proto;
> 	}
> -	if (nconf->nc_semantics == NC_TPI_CLTS) {
> +
> +	/*
> +	 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
> +	 * make sure 127.0.0.1 is added to the list.
> +	 */
> +	nhostsbak = nhosts;
> +	nhostsbak++;
> +	hosts = realloc(hosts, nhostsbak * sizeof(char *));
> +	if (nhostsbak == 1)
> +		hosts[0] = "*";
> +	else {
> +		if (hints.ai_family == AF_INET) {
> +			hosts[nhostsbak - 1] = "127.0.0.1";
> +		} else if (hints.ai_family == AF_INET6) {
> +			hosts[nhostsbak - 1] = "::1";
> +		} else
> +			return 1;
> +	}
> +
> +       /*
> +	* Bind to specific IPs if asked to
> +	*/
> +	checkbind = 0;
> +	while (nhostsbak > 0) {
> +		--nhostsbak;
> 		/*
> -		 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
> -		 * make sure 127.0.0.1 is added to the list.
> +		 * XXX - using RPC library internal functions.
> 		 */
> -		nhostsbak = nhosts;
> -		nhostsbak++;
> -		hosts = realloc(hosts, nhostsbak * sizeof(char *));
> -		if (nhostsbak == 1)
> -			hosts[0] = "*";
> -		else {
> -			if (hints.ai_family == AF_INET) {
> -				hosts[nhostsbak - 1] = "127.0.0.1";
> -			} else if (hints.ai_family == AF_INET6) {
> -				hosts[nhostsbak - 1] = "::1";
> -			} else
> -				return 1;
> +		if ((fd = __rpc_nconf2fd(nconf)) < 0) {
> +			syslog(LOG_ERR, "cannot create socket for %s",
> +			    nconf->nc_netid);
> +			return (1);
> 		}
> -
> -	       /*
> -		* Bind to specific IPs if asked to
> -		*/
> -		checkbind = 0;
> -		while (nhostsbak > 0) {
> -			--nhostsbak;
> -			/*
> -			 * XXX - using RPC library internal functions.
> -			 */
> -			if ((fd = __rpc_nconf2fd(nconf)) < 0) {
> -				syslog(LOG_ERR, "cannot create socket for %s",
> -				    nconf->nc_netid);
> -				return (1);
> +		switch (hints.ai_family) {
> +		case AF_INET:
> +			if (inet_pton(AF_INET, hosts[nhostsbak],
> +			    host_addr) == 1) {
> +				hints.ai_flags &= AI_NUMERICHOST;
> +			} else {
> +				/*
> +				 * Skip if we have an AF_INET6 adress.
> +				 */
> +				if (inet_pton(AF_INET6,
> +				    hosts[nhostsbak], host_addr) == 1)
> +					continue;
> 			}
> -			switch (hints.ai_family) {
> -			case AF_INET:
> +			break;
> +		case AF_INET6:
> +			if (inet_pton(AF_INET6, hosts[nhostsbak],
> +			    host_addr) == 1) {
> +				hints.ai_flags &= AI_NUMERICHOST;
> +			} else {
> +				/*
> +				 * Skip if we have an AF_INET adress.
> +				 */
> 				if (inet_pton(AF_INET, hosts[nhostsbak],
> -				    host_addr) == 1) {
> -					hints.ai_flags &= AI_NUMERICHOST;
> -				} else {
> -					/*
> -					 * Skip if we have an AF_INET6 adress.
> -					 */
> -					if (inet_pton(AF_INET6,
> -					    hosts[nhostsbak], host_addr) == 1)
> -						continue;
> -				}
> -				break;
> -			case AF_INET6:
> -				if (inet_pton(AF_INET6, hosts[nhostsbak],
> -				    host_addr) == 1) {
> -					hints.ai_flags &= AI_NUMERICHOST;
> -				} else {
> -					/*
> -					 * Skip if we have an AF_INET adress.
> -					 */
> -					if (inet_pton(AF_INET, hosts[nhostsbak],
> -					    host_addr) == 1)
> -						continue;
> -				}
> -	        		break;
> -			default:
> -				break;
> +				    host_addr) == 1)
> +					continue;
> 			}
> +			break;
> +		default:
> +			break;
> +		}
> 
> -			/*
> -			 * If no hosts were specified, just bind to INADDR_ANY
> -			 */
> -			if (strcmp("*", hosts[nhostsbak]) == 0)
> -				hosts[nhostsbak] = NULL;
> +		/*
> +		 * If no hosts were specified, just bind to INADDR_ANY
> +		 */
> +		if (strcmp("*", hosts[nhostsbak]) == 0)
> +			hosts[nhostsbak] = NULL;
> 
> +		if ((strcmp(nconf->nc_netid, "local") != 0) &&
> +		    (strcmp(nconf->nc_netid, "unix") != 0)) {
> 			if ((aicode = getaddrinfo(hosts[nhostsbak],
> -			    servname, &hints, &res)) != 0) {
> +			    servname, &hints, &res))!= 0) {
> 			  if ((aicode = getaddrinfo(hosts[nhostsbak],
> -						    "portmapper", &hints, &res)) != 0) {
> -				syslog(LOG_ERR,
> +					    "portmapper", &hints, &res))!= 0) {
> +			    syslog(LOG_ERR,
> 				    "cannot get local address for %s: %s",
> 				    nconf->nc_netid, gai_strerror(aicode));
> -				continue;
> +			    continue;
> 			  }
> 			}
> 			addrlen = res->ai_addrlen;
> 			sa = (struct sockaddr *)res->ai_addr;
> -			oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
> -                        if (bind(fd, sa, addrlen) != 0) {
> -				syslog(LOG_ERR, "cannot bind %s on %s: %m",
> -					(hosts[nhostsbak] == NULL) ? "*" :
> -					hosts[nhostsbak], nconf->nc_netid);
> -				if (res != NULL)
> -					freeaddrinfo(res);
> -				continue;
> -			} else
> -				checkbind++;
> -			(void) umask(oldmask);
> -
> -			/* Copy the address */
> -			taddr.addr.maxlen = taddr.addr.len = addrlen;
> -			taddr.addr.buf = malloc(addrlen);
> -			if (taddr.addr.buf == NULL) {
> -				syslog(LOG_ERR,
> -				    "cannot allocate memory for %s address",
> -				    nconf->nc_netid);
> +		}
> +		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
> +		if (nconf->nc_semantics != NC_TPI_CLTS) {
> +			/* NC_TPI_COTS and NC_TPI_COTS_ORD etc */
> +			__rpc_fd2sockinfo(fd, &si);
> +			if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &on,
> +			    sizeof(on)) != 0) {
> +				syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
> +					nconf->nc_netid);
> 				if (res != NULL)
> 					freeaddrinfo(res);
> 				return 1;
> 			}
> -			memcpy(taddr.addr.buf, sa, addrlen);
> -#ifdef RPCBIND_DEBUG
> -			if (debugging) {
> -				/*
> -				 * for debugging print out our universal
> -				 * address
> -				 */
> -				char *uaddr;
> -				struct netbuf nb;
> -				int sa_size = 0;
> -
> -				nb.buf = sa;
> -				switch( sa->sa_family){
> -				case AF_INET:
> -				  sa_size = sizeof (struct sockaddr_in);
> -				  break;
> -				case AF_INET6:
> -				  sa_size = sizeof (struct sockaddr_in6);				 
> -				  break;
> -				}
> -				nb.len = nb.maxlen = sa_size;
> -				uaddr = taddr2uaddr(nconf, &nb);
> -				(void) fprintf(stderr,
> -				    "rpcbind : my address is %s\n", uaddr);
> -				(void) free(uaddr);
> -			}
> -#endif
> -			my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf, &taddr, 
> -                                RPC_MAXDATASIZE, RPC_MAXDATASIZE);
> -			if (my_xprt == (SVCXPRT *)NULL) {
> -				syslog(LOG_ERR, "%s: could not create service", 
> -                                        nconf->nc_netid);
> -				goto error;
> -			}
> -		}
> -		if (!checkbind)
> -			return 1;
> -	} else {	/* NC_TPI_COTS */
> -		if ((strcmp(nconf->nc_netid, "local") != 0) &&
> -		    (strcmp(nconf->nc_netid, "unix") != 0)) {
> -			if ((aicode = getaddrinfo(NULL, servname, &hints, &res))!= 0) {
> -			  if ((aicode = getaddrinfo(NULL, "portmapper", &hints, &res))!= 0) {
> -			  printf("cannot get local address for %s: %s",  nconf->nc_netid, gai_strerror(aicode));
> -			  syslog(LOG_ERR,
> -				    "cannot get local address for %s: %s",
> -				    nconf->nc_netid, gai_strerror(aicode));
> -				return 1;
> -			  }
> -			}
> -			addrlen = res->ai_addrlen;
> -			sa = (struct sockaddr *)res->ai_addr;
> -		}
> -		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
> -		__rpc_fd2sockinfo(fd, &si);
> -		if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &on,
> -				sizeof(on)) != 0) {
> -			syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
> -				nconf->nc_netid);
> -			if (res != NULL)
> -				freeaddrinfo(res);
> -			return 1;
> 		}
> 		if (bind(fd, sa, addrlen) < 0) {
> 			syslog(LOG_ERR, "cannot bind %s: %m", nconf->nc_netid);
> @@ -530,7 +461,7 @@ init_transport(struct netconfig *nconf)
> 			/* for debugging print out our universal address */
> 			char *uaddr;
> 			struct netbuf nb;
> -		        int sa_size2 = 0;
> +			int sa_size2 = 0;
> 
> 			nb.buf = sa;
> 			switch( sa->sa_family){
> @@ -551,13 +482,16 @@ init_transport(struct netconfig *nconf)
> 
> 		listen(fd, SOMAXCONN);
> 
> -		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf, &taddr, RPC_MAXDATASIZE, RPC_MAXDATASIZE);
> +		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf, &taddr,
> +			    RPC_MAXDATASIZE, RPC_MAXDATASIZE);
> 		if (my_xprt == (SVCXPRT *)NULL) {
> 			syslog(LOG_ERR, "%s: could not create service",
> 					nconf->nc_netid);
> 			goto error;
> 		}
> 	}
> +	if (!checkbind)
> +		return 1;
> 
> #ifdef PORTMAP
> 	/*
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH] rpcbind: Only listen on requested hostnames for NC_TPI_COTS connections

[Niels de Vos]

On 04/23/2012 05:33 PM, Chuck Lever wrote:
> Hi-
>
> On Apr 23, 2012, at 11:24 AM, Niels de Vos wrote:
>
>> There is a check for NC_TPI_CLTS connections which correctly binds only
>> to the hostnames/addresses given on the commandline (-h option). This
>> implementation does not take non NC_TPI_CLTS like NC_TPI_COTS and
>> NC_TPI_COTS_ORD into account. This causes non NC_TPI_CLTS protocols to
>> listen on the "0.0.0.0" and "::" wildcard addresses.
>>
>> This patch simplifies the code, and moves the check for NC_TPI_CLTS into
>> the loop when a bind() for each requested address is done. All
>> connections, including NC_TPI_COTS and NC_TPI_COTS_ORD can now benefit
>> from resticted bind()'ing on addresses.
>
> Please check the mailing list archives for linux-nfs and/or
> libtirpc-devel. There may be a good reason for this asymmetrical
> behavior, or at least some enlightening discussion of this idea.

The only difference in the code before and after is for NC_TPI_CLTS vs 
(NC_TPI_CLTS or NC_TPI_COTS) and has moved to this if statement:

		if (nconf->nc_semantics != NC_TPI_CLTS) {

I could not find any reasons why binding on UDP may be restricted, where 
binding on TCP would not.

> Also, can you tell us why you need this change in the patch
 > description?

Currently rpcbind provides the -h option to contain a hostname (like 
localhost). When the option is used, rpcbind will only listen on the 
"127.0.0.1" address for UDP. TCP will continue to listen on "0.0.0.0" 
and "::". IMHO this is inconsistent and wrong, hence my proposal to fix 
this. While doing so, I noticed that the if and else branches are 
extremely similar and therefore I combined them.

Let me know if you need any further details. Thanks,
Niels


>
>> Signed-off-by: Niels de Vos<ndevos@redhat.com>
>> ---
>> src/rpcbind.c |  230 ++++++++++++++++++++------------------------------------
>> 1 files changed, 82 insertions(+), 148 deletions(-)
>>
>> diff --git a/src/rpcbind.c b/src/rpcbind.c
>> index 9a0504d..3f6c2a9 100644
>> --- a/src/rpcbind.c
>> +++ b/src/rpcbind.c
>> @@ -339,172 +339,103 @@ init_transport(struct netconfig *nconf)
>> 		hints.ai_socktype = si.si_socktype;
>> 		hints.ai_protocol = si.si_proto;
>> 	}
>> -	if (nconf->nc_semantics == NC_TPI_CLTS) {
>> +
>> +	/*
>> +	 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
>> +	 * make sure 127.0.0.1 is added to the list.
>> +	 */
>> +	nhostsbak = nhosts;
>> +	nhostsbak++;
>> +	hosts = realloc(hosts, nhostsbak * sizeof(char *));
>> +	if (nhostsbak == 1)
>> +		hosts[0] = "*";
>> +	else {
>> +		if (hints.ai_family == AF_INET) {
>> +			hosts[nhostsbak - 1] = "127.0.0.1";
>> +		} else if (hints.ai_family == AF_INET6) {
>> +			hosts[nhostsbak - 1] = "::1";
>> +		} else
>> +			return 1;
>> +	}
>> +
>> +       /*
>> +	* Bind to specific IPs if asked to
>> +	*/
>> +	checkbind = 0;
>> +	while (nhostsbak>  0) {
>> +		--nhostsbak;
>> 		/*
>> -		 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
>> -		 * make sure 127.0.0.1 is added to the list.
>> +		 * XXX - using RPC library internal functions.
>> 		 */
>> -		nhostsbak = nhosts;
>> -		nhostsbak++;
>> -		hosts = realloc(hosts, nhostsbak * sizeof(char *));
>> -		if (nhostsbak == 1)
>> -			hosts[0] = "*";
>> -		else {
>> -			if (hints.ai_family == AF_INET) {
>> -				hosts[nhostsbak - 1] = "127.0.0.1";
>> -			} else if (hints.ai_family == AF_INET6) {
>> -				hosts[nhostsbak - 1] = "::1";
>> -			} else
>> -				return 1;
>> +		if ((fd = __rpc_nconf2fd(nconf))<  0) {
>> +			syslog(LOG_ERR, "cannot create socket for %s",
>> +			    nconf->nc_netid);
>> +			return (1);
>> 		}
>> -
>> -	       /*
>> -		* Bind to specific IPs if asked to
>> -		*/
>> -		checkbind = 0;
>> -		while (nhostsbak>  0) {
>> -			--nhostsbak;
>> -			/*
>> -			 * XXX - using RPC library internal functions.
>> -			 */
>> -			if ((fd = __rpc_nconf2fd(nconf))<  0) {
>> -				syslog(LOG_ERR, "cannot create socket for %s",
>> -				    nconf->nc_netid);
>> -				return (1);
>> +		switch (hints.ai_family) {
>> +		case AF_INET:
>> +			if (inet_pton(AF_INET, hosts[nhostsbak],
>> +			    host_addr) == 1) {
>> +				hints.ai_flags&= AI_NUMERICHOST;
>> +			} else {
>> +				/*
>> +				 * Skip if we have an AF_INET6 adress.
>> +				 */
>> +				if (inet_pton(AF_INET6,
>> +				    hosts[nhostsbak], host_addr) == 1)
>> +					continue;
>> 			}
>> -			switch (hints.ai_family) {
>> -			case AF_INET:
>> +			break;
>> +		case AF_INET6:
>> +			if (inet_pton(AF_INET6, hosts[nhostsbak],
>> +			    host_addr) == 1) {
>> +				hints.ai_flags&= AI_NUMERICHOST;
>> +			} else {
>> +				/*
>> +				 * Skip if we have an AF_INET adress.
>> +				 */
>> 				if (inet_pton(AF_INET, hosts[nhostsbak],
>> -				    host_addr) == 1) {
>> -					hints.ai_flags&= AI_NUMERICHOST;
>> -				} else {
>> -					/*
>> -					 * Skip if we have an AF_INET6 adress.
>> -					 */
>> -					if (inet_pton(AF_INET6,
>> -					    hosts[nhostsbak], host_addr) == 1)
>> -						continue;
>> -				}
>> -				break;
>> -			case AF_INET6:
>> -				if (inet_pton(AF_INET6, hosts[nhostsbak],
>> -				    host_addr) == 1) {
>> -					hints.ai_flags&= AI_NUMERICHOST;
>> -				} else {
>> -					/*
>> -					 * Skip if we have an AF_INET adress.
>> -					 */
>> -					if (inet_pton(AF_INET, hosts[nhostsbak],
>> -					    host_addr) == 1)
>> -						continue;
>> -				}
>> -	        		break;
>> -			default:
>> -				break;
>> +				    host_addr) == 1)
>> +					continue;
>> 			}
>> +			break;
>> +		default:
>> +			break;
>> +		}
>>
>> -			/*
>> -			 * If no hosts were specified, just bind to INADDR_ANY
>> -			 */
>> -			if (strcmp("*", hosts[nhostsbak]) == 0)
>> -				hosts[nhostsbak] = NULL;
>> +		/*
>> +		 * If no hosts were specified, just bind to INADDR_ANY
>> +		 */
>> +		if (strcmp("*", hosts[nhostsbak]) == 0)
>> +			hosts[nhostsbak] = NULL;
>>
>> +		if ((strcmp(nconf->nc_netid, "local") != 0)&&
>> +		    (strcmp(nconf->nc_netid, "unix") != 0)) {
>> 			if ((aicode = getaddrinfo(hosts[nhostsbak],
>> -			    servname,&hints,&res)) != 0) {
>> +			    servname,&hints,&res))!= 0) {
>> 			  if ((aicode = getaddrinfo(hosts[nhostsbak],
>> -						    "portmapper",&hints,&res)) != 0) {
>> -				syslog(LOG_ERR,
>> +					    "portmapper",&hints,&res))!= 0) {
>> +			    syslog(LOG_ERR,
>> 				    "cannot get local address for %s: %s",
>> 				    nconf->nc_netid, gai_strerror(aicode));
>> -				continue;
>> +			    continue;
>> 			  }
>> 			}
>> 			addrlen = res->ai_addrlen;
>> 			sa = (struct sockaddr *)res->ai_addr;
>> -			oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
>> -                        if (bind(fd, sa, addrlen) != 0) {
>> -				syslog(LOG_ERR, "cannot bind %s on %s: %m",
>> -					(hosts[nhostsbak] == NULL) ? "*" :
>> -					hosts[nhostsbak], nconf->nc_netid);
>> -				if (res != NULL)
>> -					freeaddrinfo(res);
>> -				continue;
>> -			} else
>> -				checkbind++;
>> -			(void) umask(oldmask);
>> -
>> -			/* Copy the address */
>> -			taddr.addr.maxlen = taddr.addr.len = addrlen;
>> -			taddr.addr.buf = malloc(addrlen);
>> -			if (taddr.addr.buf == NULL) {
>> -				syslog(LOG_ERR,
>> -				    "cannot allocate memory for %s address",
>> -				    nconf->nc_netid);
>> +		}
>> +		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
>> +		if (nconf->nc_semantics != NC_TPI_CLTS) {
>> +			/* NC_TPI_COTS and NC_TPI_COTS_ORD etc */
>> +			__rpc_fd2sockinfo(fd,&si);
>> +			if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,&on,
>> +			    sizeof(on)) != 0) {
>> +				syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
>> +					nconf->nc_netid);
>> 				if (res != NULL)
>> 					freeaddrinfo(res);
>> 				return 1;
>> 			}
>> -			memcpy(taddr.addr.buf, sa, addrlen);
>> -#ifdef RPCBIND_DEBUG
>> -			if (debugging) {
>> -				/*
>> -				 * for debugging print out our universal
>> -				 * address
>> -				 */
>> -				char *uaddr;
>> -				struct netbuf nb;
>> -				int sa_size = 0;
>> -
>> -				nb.buf = sa;
>> -				switch( sa->sa_family){
>> -				case AF_INET:
>> -				  sa_size = sizeof (struct sockaddr_in);
>> -				  break;
>> -				case AF_INET6:
>> -				  sa_size = sizeof (struct sockaddr_in6);				
>> -				  break;
>> -				}
>> -				nb.len = nb.maxlen = sa_size;
>> -				uaddr = taddr2uaddr(nconf,&nb);
>> -				(void) fprintf(stderr,
>> -				    "rpcbind : my address is %s\n", uaddr);
>> -				(void) free(uaddr);
>> -			}
>> -#endif
>> -			my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
>> -                                RPC_MAXDATASIZE, RPC_MAXDATASIZE);
>> -			if (my_xprt == (SVCXPRT *)NULL) {
>> -				syslog(LOG_ERR, "%s: could not create service",
>> -                                        nconf->nc_netid);
>> -				goto error;
>> -			}
>> -		}
>> -		if (!checkbind)
>> -			return 1;
>> -	} else {	/* NC_TPI_COTS */
>> -		if ((strcmp(nconf->nc_netid, "local") != 0)&&
>> -		    (strcmp(nconf->nc_netid, "unix") != 0)) {
>> -			if ((aicode = getaddrinfo(NULL, servname,&hints,&res))!= 0) {
>> -			  if ((aicode = getaddrinfo(NULL, "portmapper",&hints,&res))!= 0) {
>> -			  printf("cannot get local address for %s: %s",  nconf->nc_netid, gai_strerror(aicode));
>> -			  syslog(LOG_ERR,
>> -				    "cannot get local address for %s: %s",
>> -				    nconf->nc_netid, gai_strerror(aicode));
>> -				return 1;
>> -			  }
>> -			}
>> -			addrlen = res->ai_addrlen;
>> -			sa = (struct sockaddr *)res->ai_addr;
>> -		}
>> -		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
>> -		__rpc_fd2sockinfo(fd,&si);
>> -		if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,&on,
>> -				sizeof(on)) != 0) {
>> -			syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
>> -				nconf->nc_netid);
>> -			if (res != NULL)
>> -				freeaddrinfo(res);
>> -			return 1;
>> 		}
>> 		if (bind(fd, sa, addrlen)<  0) {
>> 			syslog(LOG_ERR, "cannot bind %s: %m", nconf->nc_netid);
>> @@ -530,7 +461,7 @@ init_transport(struct netconfig *nconf)
>> 			/* for debugging print out our universal address */
>> 			char *uaddr;
>> 			struct netbuf nb;
>> -		        int sa_size2 = 0;
>> +			int sa_size2 = 0;
>>
>> 			nb.buf = sa;
>> 			switch( sa->sa_family){
>> @@ -551,13 +482,16 @@ init_transport(struct netconfig *nconf)
>>
>> 		listen(fd, SOMAXCONN);
>>
>> -		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr, RPC_MAXDATASIZE, RPC_MAXDATASIZE);
>> +		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
>> +			    RPC_MAXDATASIZE, RPC_MAXDATASIZE);
>> 		if (my_xprt == (SVCXPRT *)NULL) {
>> 			syslog(LOG_ERR, "%s: could not create service",
>> 					nconf->nc_netid);
>> 			goto error;
>> 		}
>> 	}
>> +	if (!checkbind)
>> +		return 1;
>>
>> #ifdef PORTMAP
>> 	/*
>> --
>> 1.7.7.6
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Niels de Vos
Software Maintenance Engineer
Global Support Services
Red Hat

Re: [PATCH] rpcbind: Only listen on requested hostnames for NC_TPI_COTS connections

[Chuck Lever]

On Apr 23, 2012, at 12:02 PM, Niels de Vos wrote:

> On 04/23/2012 05:33 PM, Chuck Lever wrote:
>> Hi-
>> 
>> On Apr 23, 2012, at 11:24 AM, Niels de Vos wrote:
>> 
>>> There is a check for NC_TPI_CLTS connections which correctly binds only
>>> to the hostnames/addresses given on the commandline (-h option). This
>>> implementation does not take non NC_TPI_CLTS like NC_TPI_COTS and
>>> NC_TPI_COTS_ORD into account. This causes non NC_TPI_CLTS protocols to
>>> listen on the "0.0.0.0" and "::" wildcard addresses.
>>> 
>>> This patch simplifies the code, and moves the check for NC_TPI_CLTS into
>>> the loop when a bind() for each requested address is done. All
>>> connections, including NC_TPI_COTS and NC_TPI_COTS_ORD can now benefit
>>> from resticted bind()'ing on addresses.
>> 
>> Please check the mailing list archives for linux-nfs and/or
>> libtirpc-devel. There may be a good reason for this asymmetrical
>> behavior, or at least some enlightening discussion of this idea.
> 
> The only difference in the code before and after is for NC_TPI_CLTS vs (NC_TPI_CLTS or NC_TPI_COTS) and has moved to this if statement:
> 
> 		if (nconf->nc_semantics != NC_TPI_CLTS) {
> 
> I could not find any reasons why binding on UDP may be restricted, where binding on TCP would not.

So you did check the mail archive?  I seem to recall other patches like this in the past, and that there is a reason why rpcbind works this way.  I simply don't remember the specifics right at the moment.

One reason might be that UDP doesn't guarantee that the forward and reply network paths will use the same source and destination addresses, but TCP does.

>> Also, can you tell us why you need this change in the patch
> > description?
> 
> Currently rpcbind provides the -h option to contain a hostname (like localhost). When the option is used, rpcbind will only listen on the "127.0.0.1" address for UDP. TCP will continue to listen on "0.0.0.0" and "::". IMHO this is inconsistent and wrong, hence my proposal to fix this.

Let me ask a more specific question: is there a real-world use case that requires this change?

> While doing so, I noticed that the if and else branches are extremely similar and therefore I combined them.

That is a clean up, and being a logically distinct change, generally belongs in a separate patch from a patch that changes the code behavior.

> Let me know if you need any further details. Thanks,
> Niels
> 
> 
>> 
>>> Signed-off-by: Niels de Vos<ndevos@redhat.com>
>>> ---
>>> src/rpcbind.c |  230 ++++++++++++++++++++------------------------------------
>>> 1 files changed, 82 insertions(+), 148 deletions(-)
>>> 
>>> diff --git a/src/rpcbind.c b/src/rpcbind.c
>>> index 9a0504d..3f6c2a9 100644
>>> --- a/src/rpcbind.c
>>> +++ b/src/rpcbind.c
>>> @@ -339,172 +339,103 @@ init_transport(struct netconfig *nconf)
>>> 		hints.ai_socktype = si.si_socktype;
>>> 		hints.ai_protocol = si.si_proto;
>>> 	}
>>> -	if (nconf->nc_semantics == NC_TPI_CLTS) {
>>> +
>>> +	/*
>>> +	 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
>>> +	 * make sure 127.0.0.1 is added to the list.
>>> +	 */
>>> +	nhostsbak = nhosts;
>>> +	nhostsbak++;
>>> +	hosts = realloc(hosts, nhostsbak * sizeof(char *));
>>> +	if (nhostsbak == 1)
>>> +		hosts[0] = "*";
>>> +	else {
>>> +		if (hints.ai_family == AF_INET) {
>>> +			hosts[nhostsbak - 1] = "127.0.0.1";
>>> +		} else if (hints.ai_family == AF_INET6) {
>>> +			hosts[nhostsbak - 1] = "::1";
>>> +		} else
>>> +			return 1;
>>> +	}
>>> +
>>> +       /*
>>> +	* Bind to specific IPs if asked to
>>> +	*/
>>> +	checkbind = 0;
>>> +	while (nhostsbak>  0) {
>>> +		--nhostsbak;
>>> 		/*
>>> -		 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
>>> -		 * make sure 127.0.0.1 is added to the list.
>>> +		 * XXX - using RPC library internal functions.
>>> 		 */
>>> -		nhostsbak = nhosts;
>>> -		nhostsbak++;
>>> -		hosts = realloc(hosts, nhostsbak * sizeof(char *));
>>> -		if (nhostsbak == 1)
>>> -			hosts[0] = "*";
>>> -		else {
>>> -			if (hints.ai_family == AF_INET) {
>>> -				hosts[nhostsbak - 1] = "127.0.0.1";
>>> -			} else if (hints.ai_family == AF_INET6) {
>>> -				hosts[nhostsbak - 1] = "::1";
>>> -			} else
>>> -				return 1;
>>> +		if ((fd = __rpc_nconf2fd(nconf))<  0) {
>>> +			syslog(LOG_ERR, "cannot create socket for %s",
>>> +			    nconf->nc_netid);
>>> +			return (1);
>>> 		}
>>> -
>>> -	       /*
>>> -		* Bind to specific IPs if asked to
>>> -		*/
>>> -		checkbind = 0;
>>> -		while (nhostsbak>  0) {
>>> -			--nhostsbak;
>>> -			/*
>>> -			 * XXX - using RPC library internal functions.
>>> -			 */
>>> -			if ((fd = __rpc_nconf2fd(nconf))<  0) {
>>> -				syslog(LOG_ERR, "cannot create socket for %s",
>>> -				    nconf->nc_netid);
>>> -				return (1);
>>> +		switch (hints.ai_family) {
>>> +		case AF_INET:
>>> +			if (inet_pton(AF_INET, hosts[nhostsbak],
>>> +			    host_addr) == 1) {
>>> +				hints.ai_flags&= AI_NUMERICHOST;
>>> +			} else {
>>> +				/*
>>> +				 * Skip if we have an AF_INET6 adress.
>>> +				 */
>>> +				if (inet_pton(AF_INET6,
>>> +				    hosts[nhostsbak], host_addr) == 1)
>>> +					continue;
>>> 			}
>>> -			switch (hints.ai_family) {
>>> -			case AF_INET:
>>> +			break;
>>> +		case AF_INET6:
>>> +			if (inet_pton(AF_INET6, hosts[nhostsbak],
>>> +			    host_addr) == 1) {
>>> +				hints.ai_flags&= AI_NUMERICHOST;
>>> +			} else {
>>> +				/*
>>> +				 * Skip if we have an AF_INET adress.
>>> +				 */
>>> 				if (inet_pton(AF_INET, hosts[nhostsbak],
>>> -				    host_addr) == 1) {
>>> -					hints.ai_flags&= AI_NUMERICHOST;
>>> -				} else {
>>> -					/*
>>> -					 * Skip if we have an AF_INET6 adress.
>>> -					 */
>>> -					if (inet_pton(AF_INET6,
>>> -					    hosts[nhostsbak], host_addr) == 1)
>>> -						continue;
>>> -				}
>>> -				break;
>>> -			case AF_INET6:
>>> -				if (inet_pton(AF_INET6, hosts[nhostsbak],
>>> -				    host_addr) == 1) {
>>> -					hints.ai_flags&= AI_NUMERICHOST;
>>> -				} else {
>>> -					/*
>>> -					 * Skip if we have an AF_INET adress.
>>> -					 */
>>> -					if (inet_pton(AF_INET, hosts[nhostsbak],
>>> -					    host_addr) == 1)
>>> -						continue;
>>> -				}
>>> -	        		break;
>>> -			default:
>>> -				break;
>>> +				    host_addr) == 1)
>>> +					continue;
>>> 			}
>>> +			break;
>>> +		default:
>>> +			break;
>>> +		}
>>> 
>>> -			/*
>>> -			 * If no hosts were specified, just bind to INADDR_ANY
>>> -			 */
>>> -			if (strcmp("*", hosts[nhostsbak]) == 0)
>>> -				hosts[nhostsbak] = NULL;
>>> +		/*
>>> +		 * If no hosts were specified, just bind to INADDR_ANY
>>> +		 */
>>> +		if (strcmp("*", hosts[nhostsbak]) == 0)
>>> +			hosts[nhostsbak] = NULL;
>>> 
>>> +		if ((strcmp(nconf->nc_netid, "local") != 0)&&
>>> +		    (strcmp(nconf->nc_netid, "unix") != 0)) {
>>> 			if ((aicode = getaddrinfo(hosts[nhostsbak],
>>> -			    servname,&hints,&res)) != 0) {
>>> +			    servname,&hints,&res))!= 0) {
>>> 			  if ((aicode = getaddrinfo(hosts[nhostsbak],
>>> -						    "portmapper",&hints,&res)) != 0) {
>>> -				syslog(LOG_ERR,
>>> +					    "portmapper",&hints,&res))!= 0) {
>>> +			    syslog(LOG_ERR,
>>> 				    "cannot get local address for %s: %s",
>>> 				    nconf->nc_netid, gai_strerror(aicode));
>>> -				continue;
>>> +			    continue;
>>> 			  }
>>> 			}
>>> 			addrlen = res->ai_addrlen;
>>> 			sa = (struct sockaddr *)res->ai_addr;
>>> -			oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
>>> -                        if (bind(fd, sa, addrlen) != 0) {
>>> -				syslog(LOG_ERR, "cannot bind %s on %s: %m",
>>> -					(hosts[nhostsbak] == NULL) ? "*" :
>>> -					hosts[nhostsbak], nconf->nc_netid);
>>> -				if (res != NULL)
>>> -					freeaddrinfo(res);
>>> -				continue;
>>> -			} else
>>> -				checkbind++;
>>> -			(void) umask(oldmask);
>>> -
>>> -			/* Copy the address */
>>> -			taddr.addr.maxlen = taddr.addr.len = addrlen;
>>> -			taddr.addr.buf = malloc(addrlen);
>>> -			if (taddr.addr.buf == NULL) {
>>> -				syslog(LOG_ERR,
>>> -				    "cannot allocate memory for %s address",
>>> -				    nconf->nc_netid);
>>> +		}
>>> +		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
>>> +		if (nconf->nc_semantics != NC_TPI_CLTS) {
>>> +			/* NC_TPI_COTS and NC_TPI_COTS_ORD etc */
>>> +			__rpc_fd2sockinfo(fd,&si);
>>> +			if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,&on,
>>> +			    sizeof(on)) != 0) {
>>> +				syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
>>> +					nconf->nc_netid);
>>> 				if (res != NULL)
>>> 					freeaddrinfo(res);
>>> 				return 1;
>>> 			}
>>> -			memcpy(taddr.addr.buf, sa, addrlen);
>>> -#ifdef RPCBIND_DEBUG
>>> -			if (debugging) {
>>> -				/*
>>> -				 * for debugging print out our universal
>>> -				 * address
>>> -				 */
>>> -				char *uaddr;
>>> -				struct netbuf nb;
>>> -				int sa_size = 0;
>>> -
>>> -				nb.buf = sa;
>>> -				switch( sa->sa_family){
>>> -				case AF_INET:
>>> -				  sa_size = sizeof (struct sockaddr_in);
>>> -				  break;
>>> -				case AF_INET6:
>>> -				  sa_size = sizeof (struct sockaddr_in6);				
>>> -				  break;
>>> -				}
>>> -				nb.len = nb.maxlen = sa_size;
>>> -				uaddr = taddr2uaddr(nconf,&nb);
>>> -				(void) fprintf(stderr,
>>> -				    "rpcbind : my address is %s\n", uaddr);
>>> -				(void) free(uaddr);
>>> -			}
>>> -#endif
>>> -			my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
>>> -                                RPC_MAXDATASIZE, RPC_MAXDATASIZE);
>>> -			if (my_xprt == (SVCXPRT *)NULL) {
>>> -				syslog(LOG_ERR, "%s: could not create service",
>>> -                                        nconf->nc_netid);
>>> -				goto error;
>>> -			}
>>> -		}
>>> -		if (!checkbind)
>>> -			return 1;
>>> -	} else {	/* NC_TPI_COTS */
>>> -		if ((strcmp(nconf->nc_netid, "local") != 0)&&
>>> -		    (strcmp(nconf->nc_netid, "unix") != 0)) {
>>> -			if ((aicode = getaddrinfo(NULL, servname,&hints,&res))!= 0) {
>>> -			  if ((aicode = getaddrinfo(NULL, "portmapper",&hints,&res))!= 0) {
>>> -			  printf("cannot get local address for %s: %s",  nconf->nc_netid, gai_strerror(aicode));
>>> -			  syslog(LOG_ERR,
>>> -				    "cannot get local address for %s: %s",
>>> -				    nconf->nc_netid, gai_strerror(aicode));
>>> -				return 1;
>>> -			  }
>>> -			}
>>> -			addrlen = res->ai_addrlen;
>>> -			sa = (struct sockaddr *)res->ai_addr;
>>> -		}
>>> -		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
>>> -		__rpc_fd2sockinfo(fd,&si);
>>> -		if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,&on,
>>> -				sizeof(on)) != 0) {
>>> -			syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
>>> -				nconf->nc_netid);
>>> -			if (res != NULL)
>>> -				freeaddrinfo(res);
>>> -			return 1;
>>> 		}
>>> 		if (bind(fd, sa, addrlen)<  0) {
>>> 			syslog(LOG_ERR, "cannot bind %s: %m", nconf->nc_netid);
>>> @@ -530,7 +461,7 @@ init_transport(struct netconfig *nconf)
>>> 			/* for debugging print out our universal address */
>>> 			char *uaddr;
>>> 			struct netbuf nb;
>>> -		        int sa_size2 = 0;
>>> +			int sa_size2 = 0;
>>> 
>>> 			nb.buf = sa;
>>> 			switch( sa->sa_family){
>>> @@ -551,13 +482,16 @@ init_transport(struct netconfig *nconf)
>>> 
>>> 		listen(fd, SOMAXCONN);
>>> 
>>> -		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr, RPC_MAXDATASIZE, RPC_MAXDATASIZE);
>>> +		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
>>> +			    RPC_MAXDATASIZE, RPC_MAXDATASIZE);
>>> 		if (my_xprt == (SVCXPRT *)NULL) {
>>> 			syslog(LOG_ERR, "%s: could not create service",
>>> 					nconf->nc_netid);
>>> 			goto error;
>>> 		}
>>> 	}
>>> +	if (!checkbind)
>>> +		return 1;
>>> 
>>> #ifdef PORTMAP
>>> 	/*
>>> --
>>> 1.7.7.6
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
> 
> 
> -- 
> Niels de Vos
> Software Maintenance Engineer
> Global Support Services
> Red Hat
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH] rpcbind: Only listen on requested hostnames for NC_TPI_COTS connections

[Niels de Vos]

Hi again,

On 04/23/2012 06:22 PM, Chuck Lever wrote:
 >
 > On Apr 23, 2012, at 12:02 PM, Niels de Vos wrote:
 >
 >> On 04/23/2012 05:33 PM, Chuck Lever wrote:
 >>> Hi-
 >>>
 >>> On Apr 23, 2012, at 11:24 AM, Niels de Vos wrote:
 >>>
 >>>> There is a check for NC_TPI_CLTS connections which correctly binds
only
 >>>> to the hostnames/addresses given on the commandline (-h option). This
 >>>> implementation does not take non NC_TPI_CLTS like NC_TPI_COTS and
 >>>> NC_TPI_COTS_ORD into account. This causes non NC_TPI_CLTS protocols to
 >>>> listen on the "0.0.0.0" and "::" wildcard addresses.
 >>>>
 >>>> This patch simplifies the code, and moves the check for
NC_TPI_CLTS into
 >>>> the loop when a bind() for each requested address is done. All
 >>>> connections, including NC_TPI_COTS and NC_TPI_COTS_ORD can now benefit
 >>>> from resticted bind()'ing on addresses.
 >>>
 >>> Please check the mailing list archives for linux-nfs and/or
 >>> libtirpc-devel. There may be a good reason for this asymmetrical
 >>> behavior, or at least some enlightening discussion of this idea.
 >>
 >> The only difference in the code before and after is for NC_TPI_CLTS vs
 >> (NC_TPI_CLTS or NC_TPI_COTS) and has moved to this if statement:
 >>
 >> 		if (nconf->nc_semantics != NC_TPI_CLTS) {
 >>
 >> I could not find any reasons why binding on UDP may be restricted, where
 >> binding on TCP would not.
 >
 > So you did check the mail archive?  I seem to recall other patches like
 > this in the past, and that there is a reason why rpcbind works this way.
 > I simply don't remember the specifics right at the moment.

I did, but no messages about this subject come up for me... Maybe I'm looking
in the wrong places :-/

 > One reason might be that UDP doesn't guarantee that the forward and
 > reply network paths will use the same source and destination addresses,
 > but TCP does.

Would this not rather speak against binding on a specific address for UDP?
That functionality already exists, its just TCP that ignores the option from
the command line.

 >>> Also, can you tell us why you need this change in the patch
 >>> description?
 >>
 >> Currently rpcbind provides the -h option to contain a hostname (like
 >> localhost). When the option is used, rpcbind will only listen on the
 >> "127.0.0.1" address for UDP. TCP will continue to listen on "0.0.0.0"
 >> and "::". IMHO this is inconsistent and wrong, hence my proposal to
 >> fix this.
 >
 > Let me ask a more specific question: is there a real-world use case
 > that requires this change?

It was brought to my attention that the old portmapper has this feature. When
migrating the configuration to rpcbind, it was noticed that the -h option does
not affect binding TCP sockets, only UDP. This behavior is documented in the
man-page, but is still feels inconsistent.

 >> While doing so, I noticed that the if and else branches are extremely
 >> similar and therefore I combined them.
 >
 > That is a clean up, and being a logically distinct change, generally
 > belongs in a separate patch from a patch that changes the code behavior.

If you or someone else can let me know if the change itself is acceptable,
I'll resend the patch in two pieces (behavior change, cleanup).

After more testing it seems that the patched rpcbind is not functioning
correctly. I'll have to look into that before sending a 2nd patch-set.

Thanks!


 >> Let me know if you need any further details. Thanks,
 >> Niels
 >>
 >>
 >>>
 >>>> Signed-off-by: Niels de Vos<ndevos@redhat.com>
 >>>> ---
 >>>> src/rpcbind.c |  230
++++++++++++++++++++------------------------------------
 >>>> 1 files changed, 82 insertions(+), 148 deletions(-)
 >>>>
 >>>> diff --git a/src/rpcbind.c b/src/rpcbind.c
 >>>> index 9a0504d..3f6c2a9 100644
 >>>> --- a/src/rpcbind.c
 >>>> +++ b/src/rpcbind.c
 >>>> @@ -339,172 +339,103 @@ init_transport(struct netconfig *nconf)
 >>>> 		hints.ai_socktype = si.si_socktype;
 >>>> 		hints.ai_protocol = si.si_proto;
 >>>> 	}
 >>>> -	if (nconf->nc_semantics == NC_TPI_CLTS) {
 >>>> +
 >>>> +	/*
 >>>> +	 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
 >>>> +	 * make sure 127.0.0.1 is added to the list.
 >>>> +	 */
 >>>> +	nhostsbak = nhosts;
 >>>> +	nhostsbak++;
 >>>> +	hosts = realloc(hosts, nhostsbak * sizeof(char *));
 >>>> +	if (nhostsbak == 1)
 >>>> +		hosts[0] = "*";
 >>>> +	else {
 >>>> +		if (hints.ai_family == AF_INET) {
 >>>> +			hosts[nhostsbak - 1] = "127.0.0.1";
 >>>> +		} else if (hints.ai_family == AF_INET6) {
 >>>> +			hosts[nhostsbak - 1] = "::1";
 >>>> +		} else
 >>>> +			return 1;
 >>>> +	}
 >>>> +
 >>>> +       /*
 >>>> +	* Bind to specific IPs if asked to
 >>>> +	*/
 >>>> +	checkbind = 0;
 >>>> +	while (nhostsbak>   0) {
 >>>> +		--nhostsbak;
 >>>> 		/*
 >>>> -		 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
 >>>> -		 * make sure 127.0.0.1 is added to the list.
 >>>> +		 * XXX - using RPC library internal functions.
 >>>> 		 */
 >>>> -		nhostsbak = nhosts;
 >>>> -		nhostsbak++;
 >>>> -		hosts = realloc(hosts, nhostsbak * sizeof(char *));
 >>>> -		if (nhostsbak == 1)
 >>>> -			hosts[0] = "*";
 >>>> -		else {
 >>>> -			if (hints.ai_family == AF_INET) {
 >>>> -				hosts[nhostsbak - 1] = "127.0.0.1";
 >>>> -			} else if (hints.ai_family == AF_INET6) {
 >>>> -				hosts[nhostsbak - 1] = "::1";
 >>>> -			} else
 >>>> -				return 1;
 >>>> +		if ((fd = __rpc_nconf2fd(nconf))<   0) {
 >>>> +			syslog(LOG_ERR, "cannot create socket for %s",
 >>>> +			    nconf->nc_netid);
 >>>> +			return (1);
 >>>> 		}
 >>>> -
 >>>> -	       /*
 >>>> -		* Bind to specific IPs if asked to
 >>>> -		*/
 >>>> -		checkbind = 0;
 >>>> -		while (nhostsbak>   0) {
 >>>> -			--nhostsbak;
 >>>> -			/*
 >>>> -			 * XXX - using RPC library internal functions.
 >>>> -			 */
 >>>> -			if ((fd = __rpc_nconf2fd(nconf))<   0) {
 >>>> -				syslog(LOG_ERR, "cannot create socket for %s",
 >>>> -				    nconf->nc_netid);
 >>>> -				return (1);
 >>>> +		switch (hints.ai_family) {
 >>>> +		case AF_INET:
 >>>> +			if (inet_pton(AF_INET, hosts[nhostsbak],
 >>>> +			    host_addr) == 1) {
 >>>> +				hints.ai_flags&= AI_NUMERICHOST;
 >>>> +			} else {
 >>>> +				/*
 >>>> +				 * Skip if we have an AF_INET6 adress.
 >>>> +				 */
 >>>> +				if (inet_pton(AF_INET6,
 >>>> +				    hosts[nhostsbak], host_addr) == 1)
 >>>> +					continue;
 >>>> 			}
 >>>> -			switch (hints.ai_family) {
 >>>> -			case AF_INET:
 >>>> +			break;
 >>>> +		case AF_INET6:
 >>>> +			if (inet_pton(AF_INET6, hosts[nhostsbak],
 >>>> +			    host_addr) == 1) {
 >>>> +				hints.ai_flags&= AI_NUMERICHOST;
 >>>> +			} else {
 >>>> +				/*
 >>>> +				 * Skip if we have an AF_INET adress.
 >>>> +				 */
 >>>> 				if (inet_pton(AF_INET, hosts[nhostsbak],
 >>>> -				    host_addr) == 1) {
 >>>> -					hints.ai_flags&= AI_NUMERICHOST;
 >>>> -				} else {
 >>>> -					/*
 >>>> -					 * Skip if we have an AF_INET6 adress.
 >>>> -					 */
 >>>> -					if (inet_pton(AF_INET6,
 >>>> -					    hosts[nhostsbak], host_addr) == 1)
 >>>> -						continue;
 >>>> -				}
 >>>> -				break;
 >>>> -			case AF_INET6:
 >>>> -				if (inet_pton(AF_INET6, hosts[nhostsbak],
 >>>> -				    host_addr) == 1) {
 >>>> -					hints.ai_flags&= AI_NUMERICHOST;
 >>>> -				} else {
 >>>> -					/*
 >>>> -					 * Skip if we have an AF_INET adress.
 >>>> -					 */
 >>>> -					if (inet_pton(AF_INET, hosts[nhostsbak],
 >>>> -					    host_addr) == 1)
 >>>> -						continue;
 >>>> -				}
 >>>> -	        		break;
 >>>> -			default:
 >>>> -				break;
 >>>> +				    host_addr) == 1)
 >>>> +					continue;
 >>>> 			}
 >>>> +			break;
 >>>> +		default:
 >>>> +			break;
 >>>> +		}
 >>>>
 >>>> -			/*
 >>>> -			 * If no hosts were specified, just bind to INADDR_ANY
 >>>> -			 */
 >>>> -			if (strcmp("*", hosts[nhostsbak]) == 0)
 >>>> -				hosts[nhostsbak] = NULL;
 >>>> +		/*
 >>>> +		 * If no hosts were specified, just bind to INADDR_ANY
 >>>> +		 */
 >>>> +		if (strcmp("*", hosts[nhostsbak]) == 0)
 >>>> +			hosts[nhostsbak] = NULL;
 >>>>
 >>>> +		if ((strcmp(nconf->nc_netid, "local") != 0)&&
 >>>> +		    (strcmp(nconf->nc_netid, "unix") != 0)) {
 >>>> 			if ((aicode = getaddrinfo(hosts[nhostsbak],
 >>>> -			    servname,&hints,&res)) != 0) {
 >>>> +			    servname,&hints,&res))!= 0) {
 >>>> 			  if ((aicode = getaddrinfo(hosts[nhostsbak],
 >>>> -						    "portmapper",&hints,&res)) != 0) {
 >>>> -				syslog(LOG_ERR,
 >>>> +					    "portmapper",&hints,&res))!= 0) {
 >>>> +			    syslog(LOG_ERR,
 >>>> 				    "cannot get local address for %s: %s",
 >>>> 				    nconf->nc_netid, gai_strerror(aicode));
 >>>> -				continue;
 >>>> +			    continue;
 >>>> 			  }
 >>>> 			}
 >>>> 			addrlen = res->ai_addrlen;
 >>>> 			sa = (struct sockaddr *)res->ai_addr;
 >>>> -			oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
 >>>> -                        if (bind(fd, sa, addrlen) != 0) {
 >>>> -				syslog(LOG_ERR, "cannot bind %s on %s: %m",
 >>>> -					(hosts[nhostsbak] == NULL) ? "*" :
 >>>> -					hosts[nhostsbak], nconf->nc_netid);
 >>>> -				if (res != NULL)
 >>>> -					freeaddrinfo(res);
 >>>> -				continue;
 >>>> -			} else
 >>>> -				checkbind++;
 >>>> -			(void) umask(oldmask);
 >>>> -
 >>>> -			/* Copy the address */
 >>>> -			taddr.addr.maxlen = taddr.addr.len = addrlen;
 >>>> -			taddr.addr.buf = malloc(addrlen);
 >>>> -			if (taddr.addr.buf == NULL) {
 >>>> -				syslog(LOG_ERR,
 >>>> -				    "cannot allocate memory for %s address",
 >>>> -				    nconf->nc_netid);
 >>>> +		}
 >>>> +		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
 >>>> +		if (nconf->nc_semantics != NC_TPI_CLTS) {
 >>>> +			/* NC_TPI_COTS and NC_TPI_COTS_ORD etc */
 >>>> +			__rpc_fd2sockinfo(fd,&si);
 >>>> +			if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,&on,
 >>>> +			    sizeof(on)) != 0) {
 >>>> +				syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
 >>>> +					nconf->nc_netid);
 >>>> 				if (res != NULL)
 >>>> 					freeaddrinfo(res);
 >>>> 				return 1;
 >>>> 			}
 >>>> -			memcpy(taddr.addr.buf, sa, addrlen);
 >>>> -#ifdef RPCBIND_DEBUG
 >>>> -			if (debugging) {
 >>>> -				/*
 >>>> -				 * for debugging print out our universal
 >>>> -				 * address
 >>>> -				 */
 >>>> -				char *uaddr;
 >>>> -				struct netbuf nb;
 >>>> -				int sa_size = 0;
 >>>> -
 >>>> -				nb.buf = sa;
 >>>> -				switch( sa->sa_family){
 >>>> -				case AF_INET:
 >>>> -				  sa_size = sizeof (struct sockaddr_in);
 >>>> -				  break;
 >>>> -				case AF_INET6:
 >>>> -				  sa_size = sizeof (struct sockaddr_in6);				
 >>>> -				  break;
 >>>> -				}
 >>>> -				nb.len = nb.maxlen = sa_size;
 >>>> -				uaddr = taddr2uaddr(nconf,&nb);
 >>>> -				(void) fprintf(stderr,
 >>>> -				    "rpcbind : my address is %s\n", uaddr);
 >>>> -				(void) free(uaddr);
 >>>> -			}
 >>>> -#endif
 >>>> -			my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
 >>>> -                                RPC_MAXDATASIZE, RPC_MAXDATASIZE);
 >>>> -			if (my_xprt == (SVCXPRT *)NULL) {
 >>>> -				syslog(LOG_ERR, "%s: could not create service",
 >>>> -                                        nconf->nc_netid);
 >>>> -				goto error;
 >>>> -			}
 >>>> -		}
 >>>> -		if (!checkbind)
 >>>> -			return 1;
 >>>> -	} else {	/* NC_TPI_COTS */
 >>>> -		if ((strcmp(nconf->nc_netid, "local") != 0)&&
 >>>> -		    (strcmp(nconf->nc_netid, "unix") != 0)) {
 >>>> -			if ((aicode = getaddrinfo(NULL, servname,&hints,&res))!= 0) {
 >>>> -			  if ((aicode = getaddrinfo(NULL, "portmapper",&hints,&res))!=
0) {
 >>>> -			  printf("cannot get local address for %s: %s",
nconf->nc_netid, gai_strerror(aicode));
 >>>> -			  syslog(LOG_ERR,
 >>>> -				    "cannot get local address for %s: %s",
 >>>> -				    nconf->nc_netid, gai_strerror(aicode));
 >>>> -				return 1;
 >>>> -			  }
 >>>> -			}
 >>>> -			addrlen = res->ai_addrlen;
 >>>> -			sa = (struct sockaddr *)res->ai_addr;
 >>>> -		}
 >>>> -		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
 >>>> -		__rpc_fd2sockinfo(fd,&si);
 >>>> -		if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,&on,
 >>>> -				sizeof(on)) != 0) {
 >>>> -			syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
 >>>> -				nconf->nc_netid);
 >>>> -			if (res != NULL)
 >>>> -				freeaddrinfo(res);
 >>>> -			return 1;
 >>>> 		}
 >>>> 		if (bind(fd, sa, addrlen)<   0) {
 >>>> 			syslog(LOG_ERR, "cannot bind %s: %m", nconf->nc_netid);
 >>>> @@ -530,7 +461,7 @@ init_transport(struct netconfig *nconf)
 >>>> 			/* for debugging print out our universal address */
 >>>> 			char *uaddr;
 >>>> 			struct netbuf nb;
 >>>> -		        int sa_size2 = 0;
 >>>> +			int sa_size2 = 0;
 >>>>
 >>>> 			nb.buf = sa;
 >>>> 			switch( sa->sa_family){
 >>>> @@ -551,13 +482,16 @@ init_transport(struct netconfig *nconf)
 >>>>
 >>>> 		listen(fd, SOMAXCONN);
 >>>>
 >>>> -		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
RPC_MAXDATASIZE, RPC_MAXDATASIZE);
 >>>> +		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
 >>>> +			    RPC_MAXDATASIZE, RPC_MAXDATASIZE);
 >>>> 		if (my_xprt == (SVCXPRT *)NULL) {
 >>>> 			syslog(LOG_ERR, "%s: could not create service",
 >>>> 					nconf->nc_netid);
 >>>> 			goto error;
 >>>> 		}
 >>>> 	}
 >>>> +	if (!checkbind)
 >>>> +		return 1;
 >>>>
 >>>> #ifdef PORTMAP
 >>>> 	/*
 >>>> --
 >>>> 1.7.7.6
 >>>>

Re: [PATCH] rpcbind: Only listen on requested hostnames for NC_TPI_COTS connections

[Chuck Lever]

On Apr 24, 2012, at 11:04 AM, Niels de Vos wrote:

> Hi again,
> 
> On 04/23/2012 06:22 PM, Chuck Lever wrote:
> >
> > On Apr 23, 2012, at 12:02 PM, Niels de Vos wrote:
> >
> >> On 04/23/2012 05:33 PM, Chuck Lever wrote:
> >>> Hi-
> >>>
> >>> On Apr 23, 2012, at 11:24 AM, Niels de Vos wrote:
> >>>
> >>>> There is a check for NC_TPI_CLTS connections which correctly binds
> only
> >>>> to the hostnames/addresses given on the commandline (-h option). This
> >>>> implementation does not take non NC_TPI_CLTS like NC_TPI_COTS and
> >>>> NC_TPI_COTS_ORD into account. This causes non NC_TPI_CLTS protocols to
> >>>> listen on the "0.0.0.0" and "::" wildcard addresses.
> >>>>
> >>>> This patch simplifies the code, and moves the check for
> NC_TPI_CLTS into
> >>>> the loop when a bind() for each requested address is done. All
> >>>> connections, including NC_TPI_COTS and NC_TPI_COTS_ORD can now benefit
> >>>> from resticted bind()'ing on addresses.
> >>>
> >>> Please check the mailing list archives for linux-nfs and/or
> >>> libtirpc-devel. There may be a good reason for this asymmetrical
> >>> behavior, or at least some enlightening discussion of this idea.
> >>
> >> The only difference in the code before and after is for NC_TPI_CLTS vs
> >> (NC_TPI_CLTS or NC_TPI_COTS) and has moved to this if statement:
> >>
> >> 		if (nconf->nc_semantics != NC_TPI_CLTS) {
> >>
> >> I could not find any reasons why binding on UDP may be restricted, where
> >> binding on TCP would not.
> >
> > So you did check the mail archive?  I seem to recall other patches like
> > this in the past, and that there is a reason why rpcbind works this way.
> > I simply don't remember the specifics right at the moment.
> 
> I did, but no messages about this subject come up for me... Maybe I'm looking
> in the wrong places :-/
> 
> > One reason might be that UDP doesn't guarantee that the forward and
> > reply network paths will use the same source and destination addresses,
> > but TCP does.
> 
> Would this not rather speak against binding on a specific address for UDP?
> That functionality already exists, its just TCP that ignores the option from
> the command line.
> 
> >>> Also, can you tell us why you need this change in the patch
> >>> description?
> >>
> >> Currently rpcbind provides the -h option to contain a hostname (like
> >> localhost). When the option is used, rpcbind will only listen on the
> >> "127.0.0.1" address for UDP. TCP will continue to listen on "0.0.0.0"
> >> and "::". IMHO this is inconsistent and wrong, hence my proposal to
> >> fix this.
> >
> > Let me ask a more specific question: is there a real-world use case
> > that requires this change?
> 
> It was brought to my attention that the old portmapper has this feature. When
> migrating the configuration to rpcbind, it was noticed that the -h option does
> not affect binding TCP sockets, only UDP.

In general, rpcbind is not supposed to be precisely backwards-compatible with portmapper, since the functionality is significantly expanded and made more secure.

It sounds like you don't have a use case that requires the change.  Is that correct?

> This behavior is documented in the
> man-page, but is still feels inconsistent.

The fact this is documented in the man page suggests it is purposely designed to behave this way.  I don't have time to research this at the moment, but I'll look into it soon.

In the meantime, I'd like to exercise a firm community member's veto on this behavior change, just until we figure out why "-h" is the way it is.

> >> While doing so, I noticed that the if and else branches are extremely
> >> similar and therefore I combined them.
> >
> > That is a clean up, and being a logically distinct change, generally
> > belongs in a separate patch from a patch that changes the code behavior.
> 
> If you or someone else can let me know if the change itself is acceptable,
> I'll resend the patch in two pieces (behavior change, cleanup).
> After more testing it seems that the patched rpcbind is not functioning
> correctly. I'll have to look into that before sending a 2nd patch-set.
> 
> Thanks!
> 
> 
> >> Let me know if you need any further details. Thanks,
> >> Niels
> >>
> >>
> >>>
> >>>> Signed-off-by: Niels de Vos<ndevos@redhat.com>
> >>>> ---
> >>>> src/rpcbind.c |  230
> ++++++++++++++++++++------------------------------------
> >>>> 1 files changed, 82 insertions(+), 148 deletions(-)
> >>>>
> >>>> diff --git a/src/rpcbind.c b/src/rpcbind.c
> >>>> index 9a0504d..3f6c2a9 100644
> >>>> --- a/src/rpcbind.c
> >>>> +++ b/src/rpcbind.c
> >>>> @@ -339,172 +339,103 @@ init_transport(struct netconfig *nconf)
> >>>> 		hints.ai_socktype = si.si_socktype;
> >>>> 		hints.ai_protocol = si.si_proto;
> >>>> 	}
> >>>> -	if (nconf->nc_semantics == NC_TPI_CLTS) {
> >>>> +
> >>>> +	/*
> >>>> +	 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
> >>>> +	 * make sure 127.0.0.1 is added to the list.
> >>>> +	 */
> >>>> +	nhostsbak = nhosts;
> >>>> +	nhostsbak++;
> >>>> +	hosts = realloc(hosts, nhostsbak * sizeof(char *));
> >>>> +	if (nhostsbak == 1)
> >>>> +		hosts[0] = "*";
> >>>> +	else {
> >>>> +		if (hints.ai_family == AF_INET) {
> >>>> +			hosts[nhostsbak - 1] = "127.0.0.1";
> >>>> +		} else if (hints.ai_family == AF_INET6) {
> >>>> +			hosts[nhostsbak - 1] = "::1";
> >>>> +		} else
> >>>> +			return 1;
> >>>> +	}
> >>>> +
> >>>> +       /*
> >>>> +	* Bind to specific IPs if asked to
> >>>> +	*/
> >>>> +	checkbind = 0;
> >>>> +	while (nhostsbak>   0) {
> >>>> +		--nhostsbak;
> >>>> 		/*
> >>>> -		 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
> >>>> -		 * make sure 127.0.0.1 is added to the list.
> >>>> +		 * XXX - using RPC library internal functions.
> >>>> 		 */
> >>>> -		nhostsbak = nhosts;
> >>>> -		nhostsbak++;
> >>>> -		hosts = realloc(hosts, nhostsbak * sizeof(char *));
> >>>> -		if (nhostsbak == 1)
> >>>> -			hosts[0] = "*";
> >>>> -		else {
> >>>> -			if (hints.ai_family == AF_INET) {
> >>>> -				hosts[nhostsbak - 1] = "127.0.0.1";
> >>>> -			} else if (hints.ai_family == AF_INET6) {
> >>>> -				hosts[nhostsbak - 1] = "::1";
> >>>> -			} else
> >>>> -				return 1;
> >>>> +		if ((fd = __rpc_nconf2fd(nconf))<   0) {
> >>>> +			syslog(LOG_ERR, "cannot create socket for %s",
> >>>> +			    nconf->nc_netid);
> >>>> +			return (1);
> >>>> 		}
> >>>> -
> >>>> -	       /*
> >>>> -		* Bind to specific IPs if asked to
> >>>> -		*/
> >>>> -		checkbind = 0;
> >>>> -		while (nhostsbak>   0) {
> >>>> -			--nhostsbak;
> >>>> -			/*
> >>>> -			 * XXX - using RPC library internal functions.
> >>>> -			 */
> >>>> -			if ((fd = __rpc_nconf2fd(nconf))<   0) {
> >>>> -				syslog(LOG_ERR, "cannot create socket for %s",
> >>>> -				    nconf->nc_netid);
> >>>> -				return (1);
> >>>> +		switch (hints.ai_family) {
> >>>> +		case AF_INET:
> >>>> +			if (inet_pton(AF_INET, hosts[nhostsbak],
> >>>> +			    host_addr) == 1) {
> >>>> +				hints.ai_flags&= AI_NUMERICHOST;
> >>>> +			} else {
> >>>> +				/*
> >>>> +				 * Skip if we have an AF_INET6 adress.
> >>>> +				 */
> >>>> +				if (inet_pton(AF_INET6,
> >>>> +				    hosts[nhostsbak], host_addr) == 1)
> >>>> +					continue;
> >>>> 			}
> >>>> -			switch (hints.ai_family) {
> >>>> -			case AF_INET:
> >>>> +			break;
> >>>> +		case AF_INET6:
> >>>> +			if (inet_pton(AF_INET6, hosts[nhostsbak],
> >>>> +			    host_addr) == 1) {
> >>>> +				hints.ai_flags&= AI_NUMERICHOST;
> >>>> +			} else {
> >>>> +				/*
> >>>> +				 * Skip if we have an AF_INET adress.
> >>>> +				 */
> >>>> 				if (inet_pton(AF_INET, hosts[nhostsbak],
> >>>> -				    host_addr) == 1) {
> >>>> -					hints.ai_flags&= AI_NUMERICHOST;
> >>>> -				} else {
> >>>> -					/*
> >>>> -					 * Skip if we have an AF_INET6 adress.
> >>>> -					 */
> >>>> -					if (inet_pton(AF_INET6,
> >>>> -					    hosts[nhostsbak], host_addr) == 1)
> >>>> -						continue;
> >>>> -				}
> >>>> -				break;
> >>>> -			case AF_INET6:
> >>>> -				if (inet_pton(AF_INET6, hosts[nhostsbak],
> >>>> -				    host_addr) == 1) {
> >>>> -					hints.ai_flags&= AI_NUMERICHOST;
> >>>> -				} else {
> >>>> -					/*
> >>>> -					 * Skip if we have an AF_INET adress.
> >>>> -					 */
> >>>> -					if (inet_pton(AF_INET, hosts[nhostsbak],
> >>>> -					    host_addr) == 1)
> >>>> -						continue;
> >>>> -				}
> >>>> -	        		break;
> >>>> -			default:
> >>>> -				break;
> >>>> +				    host_addr) == 1)
> >>>> +					continue;
> >>>> 			}
> >>>> +			break;
> >>>> +		default:
> >>>> +			break;
> >>>> +		}
> >>>>
> >>>> -			/*
> >>>> -			 * If no hosts were specified, just bind to INADDR_ANY
> >>>> -			 */
> >>>> -			if (strcmp("*", hosts[nhostsbak]) == 0)
> >>>> -				hosts[nhostsbak] = NULL;
> >>>> +		/*
> >>>> +		 * If no hosts were specified, just bind to INADDR_ANY
> >>>> +		 */
> >>>> +		if (strcmp("*", hosts[nhostsbak]) == 0)
> >>>> +			hosts[nhostsbak] = NULL;
> >>>>
> >>>> +		if ((strcmp(nconf->nc_netid, "local") != 0)&&
> >>>> +		    (strcmp(nconf->nc_netid, "unix") != 0)) {
> >>>> 			if ((aicode = getaddrinfo(hosts[nhostsbak],
> >>>> -			    servname,&hints,&res)) != 0) {
> >>>> +			    servname,&hints,&res))!= 0) {
> >>>> 			  if ((aicode = getaddrinfo(hosts[nhostsbak],
> >>>> -						    "portmapper",&hints,&res)) != 0) {
> >>>> -				syslog(LOG_ERR,
> >>>> +					    "portmapper",&hints,&res))!= 0) {
> >>>> +			    syslog(LOG_ERR,
> >>>> 				    "cannot get local address for %s: %s",
> >>>> 				    nconf->nc_netid, gai_strerror(aicode));
> >>>> -				continue;
> >>>> +			    continue;
> >>>> 			  }
> >>>> 			}
> >>>> 			addrlen = res->ai_addrlen;
> >>>> 			sa = (struct sockaddr *)res->ai_addr;
> >>>> -			oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
> >>>> -                        if (bind(fd, sa, addrlen) != 0) {
> >>>> -				syslog(LOG_ERR, "cannot bind %s on %s: %m",
> >>>> -					(hosts[nhostsbak] == NULL) ? "*" :
> >>>> -					hosts[nhostsbak], nconf->nc_netid);
> >>>> -				if (res != NULL)
> >>>> -					freeaddrinfo(res);
> >>>> -				continue;
> >>>> -			} else
> >>>> -				checkbind++;
> >>>> -			(void) umask(oldmask);
> >>>> -
> >>>> -			/* Copy the address */
> >>>> -			taddr.addr.maxlen = taddr.addr.len = addrlen;
> >>>> -			taddr.addr.buf = malloc(addrlen);
> >>>> -			if (taddr.addr.buf == NULL) {
> >>>> -				syslog(LOG_ERR,
> >>>> -				    "cannot allocate memory for %s address",
> >>>> -				    nconf->nc_netid);
> >>>> +		}
> >>>> +		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
> >>>> +		if (nconf->nc_semantics != NC_TPI_CLTS) {
> >>>> +			/* NC_TPI_COTS and NC_TPI_COTS_ORD etc */
> >>>> +			__rpc_fd2sockinfo(fd,&si);
> >>>> +			if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,&on,
> >>>> +			    sizeof(on)) != 0) {
> >>>> +				syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
> >>>> +					nconf->nc_netid);
> >>>> 				if (res != NULL)
> >>>> 					freeaddrinfo(res);
> >>>> 				return 1;
> >>>> 			}
> >>>> -			memcpy(taddr.addr.buf, sa, addrlen);
> >>>> -#ifdef RPCBIND_DEBUG
> >>>> -			if (debugging) {
> >>>> -				/*
> >>>> -				 * for debugging print out our universal
> >>>> -				 * address
> >>>> -				 */
> >>>> -				char *uaddr;
> >>>> -				struct netbuf nb;
> >>>> -				int sa_size = 0;
> >>>> -
> >>>> -				nb.buf = sa;
> >>>> -				switch( sa->sa_family){
> >>>> -				case AF_INET:
> >>>> -				  sa_size = sizeof (struct sockaddr_in);
> >>>> -				  break;
> >>>> -				case AF_INET6:
> >>>> -				  sa_size = sizeof (struct sockaddr_in6);				
> >>>> -				  break;
> >>>> -				}
> >>>> -				nb.len = nb.maxlen = sa_size;
> >>>> -				uaddr = taddr2uaddr(nconf,&nb);
> >>>> -				(void) fprintf(stderr,
> >>>> -				    "rpcbind : my address is %s\n", uaddr);
> >>>> -				(void) free(uaddr);
> >>>> -			}
> >>>> -#endif
> >>>> -			my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
> >>>> -                                RPC_MAXDATASIZE, RPC_MAXDATASIZE);
> >>>> -			if (my_xprt == (SVCXPRT *)NULL) {
> >>>> -				syslog(LOG_ERR, "%s: could not create service",
> >>>> -                                        nconf->nc_netid);
> >>>> -				goto error;
> >>>> -			}
> >>>> -		}
> >>>> -		if (!checkbind)
> >>>> -			return 1;
> >>>> -	} else {	/* NC_TPI_COTS */
> >>>> -		if ((strcmp(nconf->nc_netid, "local") != 0)&&
> >>>> -		    (strcmp(nconf->nc_netid, "unix") != 0)) {
> >>>> -			if ((aicode = getaddrinfo(NULL, servname,&hints,&res))!= 0) {
> >>>> -			  if ((aicode = getaddrinfo(NULL, "portmapper",&hints,&res))!=
> 0) {
> >>>> -			  printf("cannot get local address for %s: %s",
> nconf->nc_netid, gai_strerror(aicode));
> >>>> -			  syslog(LOG_ERR,
> >>>> -				    "cannot get local address for %s: %s",
> >>>> -				    nconf->nc_netid, gai_strerror(aicode));
> >>>> -				return 1;
> >>>> -			  }
> >>>> -			}
> >>>> -			addrlen = res->ai_addrlen;
> >>>> -			sa = (struct sockaddr *)res->ai_addr;
> >>>> -		}
> >>>> -		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
> >>>> -		__rpc_fd2sockinfo(fd,&si);
> >>>> -		if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,&on,
> >>>> -				sizeof(on)) != 0) {
> >>>> -			syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
> >>>> -				nconf->nc_netid);
> >>>> -			if (res != NULL)
> >>>> -				freeaddrinfo(res);
> >>>> -			return 1;
> >>>> 		}
> >>>> 		if (bind(fd, sa, addrlen)<   0) {
> >>>> 			syslog(LOG_ERR, "cannot bind %s: %m", nconf->nc_netid);
> >>>> @@ -530,7 +461,7 @@ init_transport(struct netconfig *nconf)
> >>>> 			/* for debugging print out our universal address */
> >>>> 			char *uaddr;
> >>>> 			struct netbuf nb;
> >>>> -		        int sa_size2 = 0;
> >>>> +			int sa_size2 = 0;
> >>>>
> >>>> 			nb.buf = sa;
> >>>> 			switch( sa->sa_family){
> >>>> @@ -551,13 +482,16 @@ init_transport(struct netconfig *nconf)
> >>>>
> >>>> 		listen(fd, SOMAXCONN);
> >>>>
> >>>> -		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
> RPC_MAXDATASIZE, RPC_MAXDATASIZE);
> >>>> +		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
> >>>> +			    RPC_MAXDATASIZE, RPC_MAXDATASIZE);
> >>>> 		if (my_xprt == (SVCXPRT *)NULL) {
> >>>> 			syslog(LOG_ERR, "%s: could not create service",
> >>>> 					nconf->nc_netid);
> >>>> 			goto error;
> >>>> 		}
> >>>> 	}
> >>>> +	if (!checkbind)
> >>>> +		return 1;
> >>>>
> >>>> #ifdef PORTMAP
> >>>> 	/*
> >>>> --
> >>>> 1.7.7.6
> >>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH] rpcbind: Only listen on requested hostnames for NC_TPI_COTS connections

[Jim Rees]

Niels de Vos wrote:

  On 04/23/2012 06:22 PM, Chuck Lever wrote:
  >
  > So you did check the mail archive?  I seem to recall other patches like
  > this in the past, and that there is a reason why rpcbind works this way.
  > I simply don't remember the specifics right at the moment.
  
  I did, but no messages about this subject come up for me... Maybe I'm looking
  in the wrong places :-/

I asked about this last November, and at that time Chuck referred me to the
mail archive too.  I couldn't find any discussion either.  But the behavior
is intentional, so I don't think you'll get a patch accepted.  I never did
discover the reason but I do have a workaround.  I just don't run rpcbind.

This was the most informative response I got:

Date: Tue, 8 Nov 2011 19:01:51 +1100
From: Max Matveev <makc@redhat.com>
Subject: Re: rpcbind -h
To: Jim Rees <rees@umich.edu>
Cc: Chuck Lever <chuck.lever@oracle.com>, linux-nfs@vger.kernel.org

Chuck's quote from the manpage reminded me - -h was used to work            
around the address selection: if server has more then one address the       
reply may use any of them. Some clients don't like it.

This issue should go away after

commit 74ef3df0236c55185225c62fba34953f2582da72
Author: Olaf Kirch <okir@suse.de>      
Date:   Wed Mar 2 10:09:24 2011 -0500

was added to libtirpc.

 rees> As I said before, I was hoping for the equivalent of "portmap
 rees> -l".  I was ready to code up a patch of some kind but now have
 rees> a workaround (mount with nolock and don't run rpcbind at all).

iptables is another option.

max

Re: [PATCH] rpcbind: Only listen on requested hostnames for NC_TPI_COTS connections

[Niels de Vos]

On 04/24/2012 05:29 PM, Jim Rees wrote:
> Niels de Vos wrote:
>
>    On 04/23/2012 06:22 PM, Chuck Lever wrote:
>    >
>    >  So you did check the mail archive?  I seem to recall other patches like
>    >  this in the past, and that there is a reason why rpcbind works this way.
>    >  I simply don't remember the specifics right at the moment.
>
>    I did, but no messages about this subject come up for me... Maybe I'm looking
>    in the wrong places :-/
>
> I asked about this last November, and at that time Chuck referred me to the
> mail archive too.  I couldn't find any discussion either.  But the behavior
> is intentional, so I don't think you'll get a patch accepted.  I never did
> discover the reason but I do have a workaround.  I just don't run rpcbind.

Many thanks for the pointer, I think this should be an acceptable solution.

Cheers,
Niels

> This was the most informative response I got:
>
> Date: Tue, 8 Nov 2011 19:01:51 +1100
> From: Max Matveev<makc@redhat.com>
> Subject: Re: rpcbind -h
> To: Jim Rees<rees@umich.edu>
> Cc: Chuck Lever<chuck.lever@oracle.com>, linux-nfs@vger.kernel.org
>
> Chuck's quote from the manpage reminded me - -h was used to work
> around the address selection: if server has more then one address the
> reply may use any of them. Some clients don't like it.
>
> This issue should go away after
>
> commit 74ef3df0236c55185225c62fba34953f2582da72
> Author: Olaf Kirch<okir@suse.de>
> Date:   Wed Mar 2 10:09:24 2011 -0500
>
> was added to libtirpc.
>
>   rees>  As I said before, I was hoping for the equivalent of "portmap
>   rees>  -l".  I was ready to code up a patch of some kind but now have
>   rees>  a workaround (mount with nolock and don't run rpcbind at all).
>
> iptables is another option.
>
> max

Re: [PATCH] rpcbind: Only listen on requested hostnames for NC_TPI_COTS connections

[Chuck Lever]

On Apr 24, 2012, at 11:29 AM, Jim Rees wrote:

> Niels de Vos wrote:
> 
>  On 04/23/2012 06:22 PM, Chuck Lever wrote:
>> 
>> So you did check the mail archive?  I seem to recall other patches like
>> this in the past, and that there is a reason why rpcbind works this way.
>> I simply don't remember the specifics right at the moment.
> 
>  I did, but no messages about this subject come up for me... Maybe I'm looking
>  in the wrong places :-/
> 
> I asked about this last November, and at that time Chuck referred me to the
> mail archive too.  I couldn't find any discussion either.  But the behavior
> is intentional, so I don't think you'll get a patch accepted.

Going back to the rpcbind(8) man page, the "-h" is meant to work around a brokenness of RPC over UDP.  UDP replies can come from any server address, as I mentioned before, and most Linux clients, at least, don't like an RPC reply to come from a different address than the request was sent to.

We don't need or want this option for connection-oriented transports.  The reply will always be returned on the same connection that request was made on.

Restricting the bind address for rpcbind's listener is a different, and perhaps orthogonal, issue.  In addition to trimming rpcbind's listener address space via IP tables, you can also run the rpcbind server, and the RPC services it shepherds, in a separate network namespace.

> I never did
> discover the reason but I do have a workaround.  I just don't run rpcbind.
> 
> This was the most informative response I got:
> 
> Date: Tue, 8 Nov 2011 19:01:51 +1100
> From: Max Matveev <makc@redhat.com>
> Subject: Re: rpcbind -h
> To: Jim Rees <rees@umich.edu>
> Cc: Chuck Lever <chuck.lever@oracle.com>, linux-nfs@vger.kernel.org
> 
> Chuck's quote from the manpage reminded me - -h was used to work            
> around the address selection: if server has more then one address the       
> reply may use any of them. Some clients don't like it.
> 
> This issue should go away after
> 
> commit 74ef3df0236c55185225c62fba34953f2582da72
> Author: Olaf Kirch <okir@suse.de>      
> Date:   Wed Mar 2 10:09:24 2011 -0500
> 
> was added to libtirpc.
> 
> rees> As I said before, I was hoping for the equivalent of "portmap
> rees> -l".  I was ready to code up a patch of some kind but now have
> rees> a workaround (mount with nolock and don't run rpcbind at all).
> 
> iptables is another option.
> 
> max
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH] rpcbind: Only listen on requested hostnames for NC_TPI_COTS connections

[Niels de Vos]

On 04/24/2012 05:18 PM, Chuck Lever wrote:
>
> On Apr 24, 2012, at 11:04 AM, Niels de Vos wrote:
>
>> Hi again,
>>
>> On 04/23/2012 06:22 PM, Chuck Lever wrote:
>>>
>>> On Apr 23, 2012, at 12:02 PM, Niels de Vos wrote:
>>>
>>>> On 04/23/2012 05:33 PM, Chuck Lever wrote:
>>>>> Hi-
>>>>>
>>>>> On Apr 23, 2012, at 11:24 AM, Niels de Vos wrote:
>>>>>
>>>>>> There is a check for NC_TPI_CLTS connections which correctly binds
>> only
>>>>>> to the hostnames/addresses given on the commandline (-h option). This
>>>>>> implementation does not take non NC_TPI_CLTS like NC_TPI_COTS and
>>>>>> NC_TPI_COTS_ORD into account. This causes non NC_TPI_CLTS protocols to
>>>>>> listen on the "0.0.0.0" and "::" wildcard addresses.
>>>>>>
>>>>>> This patch simplifies the code, and moves the check for
>> NC_TPI_CLTS into
>>>>>> the loop when a bind() for each requested address is done. All
>>>>>> connections, including NC_TPI_COTS and NC_TPI_COTS_ORD can now benefit
>>>>>> from resticted bind()'ing on addresses.
>>>>>
>>>>> Please check the mailing list archives for linux-nfs and/or
>>>>> libtirpc-devel. There may be a good reason for this asymmetrical
>>>>> behavior, or at least some enlightening discussion of this idea.
>>>>
>>>> The only difference in the code before and after is for NC_TPI_CLTS vs
>>>> (NC_TPI_CLTS or NC_TPI_COTS) and has moved to this if statement:
>>>>
>>>> 		if (nconf->nc_semantics != NC_TPI_CLTS) {
>>>>
>>>> I could not find any reasons why binding on UDP may be restricted, where
>>>> binding on TCP would not.
>>>
>>> So you did check the mail archive?  I seem to recall other patches like
>>> this in the past, and that there is a reason why rpcbind works this way.
>>> I simply don't remember the specifics right at the moment.
>>
>> I did, but no messages about this subject come up for me... Maybe I'm looking
>> in the wrong places :-/
>>
>>> One reason might be that UDP doesn't guarantee that the forward and
>>> reply network paths will use the same source and destination addresses,
>>> but TCP does.
>>
>> Would this not rather speak against binding on a specific address for UDP?
>> That functionality already exists, its just TCP that ignores the option from
>> the command line.
>>
>>>>> Also, can you tell us why you need this change in the patch
>>>>> description?
>>>>
>>>> Currently rpcbind provides the -h option to contain a hostname (like
>>>> localhost). When the option is used, rpcbind will only listen on the
>>>> "127.0.0.1" address for UDP. TCP will continue to listen on "0.0.0.0"
>>>> and "::". IMHO this is inconsistent and wrong, hence my proposal to
>>>> fix this.
>>>
>>> Let me ask a more specific question: is there a real-world use case
>>> that requires this change?
>>
>> It was brought to my attention that the old portmapper has this feature. When
>> migrating the configuration to rpcbind, it was noticed that the -h option does
>> not affect binding TCP sockets, only UDP.
>
> In general, rpcbind is not supposed to be precisely backwards-compatible
> with  portmapper, since the functionality is significantly expanded and
 > made more secure.
>
> It sounds like you don't have a use case that requires the change. Is that
> correct?

Requiring the change would suggest that there are no other solutions. Jim 
pointed to an alternative in this email thread, and I think that should be 
acceptable.

>> This behavior is documented in the
>> man-page, but is still feels inconsistent.
>
> The fact this is documented in the man page suggests it is purposely
> designed  to behave this way. I don't have time to research this at the
 > moment, but I'll look into it soon.

Don't spend too much time on that, I'll have the alternative checked first. in 
case there is no working alternative, I'll research again and post fully working 
patches.

> In the meantime, I'd like to exercise a firm community member's veto on this
> behavior change, just until we figure out why "-h" is the way it is.

Sure, I agree that only useful functionalities should be included. Changes 
should not break the design without clearly improving it.

Thanks for the responses,
Niels

>>>> While doing so, I noticed that the if and else branches are extremely
>>>> similar and therefore I combined them.
>>>
>>> That is a clean up, and being a logically distinct change, generally
>>> belongs in a separate patch from a patch that changes the code behavior.
>>
>> If you or someone else can let me know if the change itself is acceptable,
>> I'll resend the patch in two pieces (behavior change, cleanup).
>> After more testing it seems that the patched rpcbind is not functioning
>> correctly. I'll have to look into that before sending a 2nd patch-set.
>>
>> Thanks!
>>
>>
>>>> Let me know if you need any further details. Thanks,
>>>> Niels
>>>>
>>>>
>>>>>
>>>>>> Signed-off-by: Niels de Vos<ndevos@redhat.com>
>>>>>> ---
>>>>>> src/rpcbind.c |  230
>> ++++++++++++++++++++------------------------------------
>>>>>> 1 files changed, 82 insertions(+), 148 deletions(-)
>>>>>>
>>>>>> diff --git a/src/rpcbind.c b/src/rpcbind.c
>>>>>> index 9a0504d..3f6c2a9 100644
>>>>>> --- a/src/rpcbind.c
>>>>>> +++ b/src/rpcbind.c
>>>>>> @@ -339,172 +339,103 @@ init_transport(struct netconfig *nconf)
>>>>>> 		hints.ai_socktype = si.si_socktype;
>>>>>> 		hints.ai_protocol = si.si_proto;
>>>>>> 	}
>>>>>> -	if (nconf->nc_semantics == NC_TPI_CLTS) {
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
>>>>>> +	 * make sure 127.0.0.1 is added to the list.
>>>>>> +	 */
>>>>>> +	nhostsbak = nhosts;
>>>>>> +	nhostsbak++;
>>>>>> +	hosts = realloc(hosts, nhostsbak * sizeof(char *));
>>>>>> +	if (nhostsbak == 1)
>>>>>> +		hosts[0] = "*";
>>>>>> +	else {
>>>>>> +		if (hints.ai_family == AF_INET) {
>>>>>> +			hosts[nhostsbak - 1] = "127.0.0.1";
>>>>>> +		} else if (hints.ai_family == AF_INET6) {
>>>>>> +			hosts[nhostsbak - 1] = "::1";
>>>>>> +		} else
>>>>>> +			return 1;
>>>>>> +	}
>>>>>> +
>>>>>> +       /*
>>>>>> +	* Bind to specific IPs if asked to
>>>>>> +	*/
>>>>>> +	checkbind = 0;
>>>>>> +	while (nhostsbak>    0) {
>>>>>> +		--nhostsbak;
>>>>>> 		/*
>>>>>> -		 * If no hosts were specified, just bind to INADDR_ANY.  Otherwise
>>>>>> -		 * make sure 127.0.0.1 is added to the list.
>>>>>> +		 * XXX - using RPC library internal functions.
>>>>>> 		 */
>>>>>> -		nhostsbak = nhosts;
>>>>>> -		nhostsbak++;
>>>>>> -		hosts = realloc(hosts, nhostsbak * sizeof(char *));
>>>>>> -		if (nhostsbak == 1)
>>>>>> -			hosts[0] = "*";
>>>>>> -		else {
>>>>>> -			if (hints.ai_family == AF_INET) {
>>>>>> -				hosts[nhostsbak - 1] = "127.0.0.1";
>>>>>> -			} else if (hints.ai_family == AF_INET6) {
>>>>>> -				hosts[nhostsbak - 1] = "::1";
>>>>>> -			} else
>>>>>> -				return 1;
>>>>>> +		if ((fd = __rpc_nconf2fd(nconf))<    0) {
>>>>>> +			syslog(LOG_ERR, "cannot create socket for %s",
>>>>>> +			    nconf->nc_netid);
>>>>>> +			return (1);
>>>>>> 		}
>>>>>> -
>>>>>> -	       /*
>>>>>> -		* Bind to specific IPs if asked to
>>>>>> -		*/
>>>>>> -		checkbind = 0;
>>>>>> -		while (nhostsbak>    0) {
>>>>>> -			--nhostsbak;
>>>>>> -			/*
>>>>>> -			 * XXX - using RPC library internal functions.
>>>>>> -			 */
>>>>>> -			if ((fd = __rpc_nconf2fd(nconf))<    0) {
>>>>>> -				syslog(LOG_ERR, "cannot create socket for %s",
>>>>>> -				    nconf->nc_netid);
>>>>>> -				return (1);
>>>>>> +		switch (hints.ai_family) {
>>>>>> +		case AF_INET:
>>>>>> +			if (inet_pton(AF_INET, hosts[nhostsbak],
>>>>>> +			    host_addr) == 1) {
>>>>>> +				hints.ai_flags&= AI_NUMERICHOST;
>>>>>> +			} else {
>>>>>> +				/*
>>>>>> +				 * Skip if we have an AF_INET6 adress.
>>>>>> +				 */
>>>>>> +				if (inet_pton(AF_INET6,
>>>>>> +				    hosts[nhostsbak], host_addr) == 1)
>>>>>> +					continue;
>>>>>> 			}
>>>>>> -			switch (hints.ai_family) {
>>>>>> -			case AF_INET:
>>>>>> +			break;
>>>>>> +		case AF_INET6:
>>>>>> +			if (inet_pton(AF_INET6, hosts[nhostsbak],
>>>>>> +			    host_addr) == 1) {
>>>>>> +				hints.ai_flags&= AI_NUMERICHOST;
>>>>>> +			} else {
>>>>>> +				/*
>>>>>> +				 * Skip if we have an AF_INET adress.
>>>>>> +				 */
>>>>>> 				if (inet_pton(AF_INET, hosts[nhostsbak],
>>>>>> -				    host_addr) == 1) {
>>>>>> -					hints.ai_flags&= AI_NUMERICHOST;
>>>>>> -				} else {
>>>>>> -					/*
>>>>>> -					 * Skip if we have an AF_INET6 adress.
>>>>>> -					 */
>>>>>> -					if (inet_pton(AF_INET6,
>>>>>> -					    hosts[nhostsbak], host_addr) == 1)
>>>>>> -						continue;
>>>>>> -				}
>>>>>> -				break;
>>>>>> -			case AF_INET6:
>>>>>> -				if (inet_pton(AF_INET6, hosts[nhostsbak],
>>>>>> -				    host_addr) == 1) {
>>>>>> -					hints.ai_flags&= AI_NUMERICHOST;
>>>>>> -				} else {
>>>>>> -					/*
>>>>>> -					 * Skip if we have an AF_INET adress.
>>>>>> -					 */
>>>>>> -					if (inet_pton(AF_INET, hosts[nhostsbak],
>>>>>> -					    host_addr) == 1)
>>>>>> -						continue;
>>>>>> -				}
>>>>>> -	        		break;
>>>>>> -			default:
>>>>>> -				break;
>>>>>> +				    host_addr) == 1)
>>>>>> +					continue;
>>>>>> 			}
>>>>>> +			break;
>>>>>> +		default:
>>>>>> +			break;
>>>>>> +		}
>>>>>>
>>>>>> -			/*
>>>>>> -			 * If no hosts were specified, just bind to INADDR_ANY
>>>>>> -			 */
>>>>>> -			if (strcmp("*", hosts[nhostsbak]) == 0)
>>>>>> -				hosts[nhostsbak] = NULL;
>>>>>> +		/*
>>>>>> +		 * If no hosts were specified, just bind to INADDR_ANY
>>>>>> +		 */
>>>>>> +		if (strcmp("*", hosts[nhostsbak]) == 0)
>>>>>> +			hosts[nhostsbak] = NULL;
>>>>>>
>>>>>> +		if ((strcmp(nconf->nc_netid, "local") != 0)&&
>>>>>> +		    (strcmp(nconf->nc_netid, "unix") != 0)) {
>>>>>> 			if ((aicode = getaddrinfo(hosts[nhostsbak],
>>>>>> -			    servname,&hints,&res)) != 0) {
>>>>>> +			    servname,&hints,&res))!= 0) {
>>>>>> 			  if ((aicode = getaddrinfo(hosts[nhostsbak],
>>>>>> -						    "portmapper",&hints,&res)) != 0) {
>>>>>> -				syslog(LOG_ERR,
>>>>>> +					    "portmapper",&hints,&res))!= 0) {
>>>>>> +			    syslog(LOG_ERR,
>>>>>> 				    "cannot get local address for %s: %s",
>>>>>> 				    nconf->nc_netid, gai_strerror(aicode));
>>>>>> -				continue;
>>>>>> +			    continue;
>>>>>> 			  }
>>>>>> 			}
>>>>>> 			addrlen = res->ai_addrlen;
>>>>>> 			sa = (struct sockaddr *)res->ai_addr;
>>>>>> -			oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
>>>>>> -                        if (bind(fd, sa, addrlen) != 0) {
>>>>>> -				syslog(LOG_ERR, "cannot bind %s on %s: %m",
>>>>>> -					(hosts[nhostsbak] == NULL) ? "*" :
>>>>>> -					hosts[nhostsbak], nconf->nc_netid);
>>>>>> -				if (res != NULL)
>>>>>> -					freeaddrinfo(res);
>>>>>> -				continue;
>>>>>> -			} else
>>>>>> -				checkbind++;
>>>>>> -			(void) umask(oldmask);
>>>>>> -
>>>>>> -			/* Copy the address */
>>>>>> -			taddr.addr.maxlen = taddr.addr.len = addrlen;
>>>>>> -			taddr.addr.buf = malloc(addrlen);
>>>>>> -			if (taddr.addr.buf == NULL) {
>>>>>> -				syslog(LOG_ERR,
>>>>>> -				    "cannot allocate memory for %s address",
>>>>>> -				    nconf->nc_netid);
>>>>>> +		}
>>>>>> +		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
>>>>>> +		if (nconf->nc_semantics != NC_TPI_CLTS) {
>>>>>> +			/* NC_TPI_COTS and NC_TPI_COTS_ORD etc */
>>>>>> +			__rpc_fd2sockinfo(fd,&si);
>>>>>> +			if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,&on,
>>>>>> +			    sizeof(on)) != 0) {
>>>>>> +				syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
>>>>>> +					nconf->nc_netid);
>>>>>> 				if (res != NULL)
>>>>>> 					freeaddrinfo(res);
>>>>>> 				return 1;
>>>>>> 			}
>>>>>> -			memcpy(taddr.addr.buf, sa, addrlen);
>>>>>> -#ifdef RPCBIND_DEBUG
>>>>>> -			if (debugging) {
>>>>>> -				/*
>>>>>> -				 * for debugging print out our universal
>>>>>> -				 * address
>>>>>> -				 */
>>>>>> -				char *uaddr;
>>>>>> -				struct netbuf nb;
>>>>>> -				int sa_size = 0;
>>>>>> -
>>>>>> -				nb.buf = sa;
>>>>>> -				switch( sa->sa_family){
>>>>>> -				case AF_INET:
>>>>>> -				  sa_size = sizeof (struct sockaddr_in);
>>>>>> -				  break;
>>>>>> -				case AF_INET6:
>>>>>> -				  sa_size = sizeof (struct sockaddr_in6);				
>>>>>> -				  break;
>>>>>> -				}
>>>>>> -				nb.len = nb.maxlen = sa_size;
>>>>>> -				uaddr = taddr2uaddr(nconf,&nb);
>>>>>> -				(void) fprintf(stderr,
>>>>>> -				    "rpcbind : my address is %s\n", uaddr);
>>>>>> -				(void) free(uaddr);
>>>>>> -			}
>>>>>> -#endif
>>>>>> -			my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
>>>>>> -                                RPC_MAXDATASIZE, RPC_MAXDATASIZE);
>>>>>> -			if (my_xprt == (SVCXPRT *)NULL) {
>>>>>> -				syslog(LOG_ERR, "%s: could not create service",
>>>>>> -                                        nconf->nc_netid);
>>>>>> -				goto error;
>>>>>> -			}
>>>>>> -		}
>>>>>> -		if (!checkbind)
>>>>>> -			return 1;
>>>>>> -	} else {	/* NC_TPI_COTS */
>>>>>> -		if ((strcmp(nconf->nc_netid, "local") != 0)&&
>>>>>> -		    (strcmp(nconf->nc_netid, "unix") != 0)) {
>>>>>> -			if ((aicode = getaddrinfo(NULL, servname,&hints,&res))!= 0) {
>>>>>> -			  if ((aicode = getaddrinfo(NULL, "portmapper",&hints,&res))!=
>> 0) {
>>>>>> -			  printf("cannot get local address for %s: %s",
>> nconf->nc_netid, gai_strerror(aicode));
>>>>>> -			  syslog(LOG_ERR,
>>>>>> -				    "cannot get local address for %s: %s",
>>>>>> -				    nconf->nc_netid, gai_strerror(aicode));
>>>>>> -				return 1;
>>>>>> -			  }
>>>>>> -			}
>>>>>> -			addrlen = res->ai_addrlen;
>>>>>> -			sa = (struct sockaddr *)res->ai_addr;
>>>>>> -		}
>>>>>> -		oldmask = umask(S_IXUSR|S_IXGRP|S_IXOTH);
>>>>>> -		__rpc_fd2sockinfo(fd,&si);
>>>>>> -		if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,&on,
>>>>>> -				sizeof(on)) != 0) {
>>>>>> -			syslog(LOG_ERR, "cannot set SO_REUSEADDR on %s",
>>>>>> -				nconf->nc_netid);
>>>>>> -			if (res != NULL)
>>>>>> -				freeaddrinfo(res);
>>>>>> -			return 1;
>>>>>> 		}
>>>>>> 		if (bind(fd, sa, addrlen)<    0) {
>>>>>> 			syslog(LOG_ERR, "cannot bind %s: %m", nconf->nc_netid);
>>>>>> @@ -530,7 +461,7 @@ init_transport(struct netconfig *nconf)
>>>>>> 			/* for debugging print out our universal address */
>>>>>> 			char *uaddr;
>>>>>> 			struct netbuf nb;
>>>>>> -		        int sa_size2 = 0;
>>>>>> +			int sa_size2 = 0;
>>>>>>
>>>>>> 			nb.buf = sa;
>>>>>> 			switch( sa->sa_family){
>>>>>> @@ -551,13 +482,16 @@ init_transport(struct netconfig *nconf)
>>>>>>
>>>>>> 		listen(fd, SOMAXCONN);
>>>>>>
>>>>>> -		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
>> RPC_MAXDATASIZE, RPC_MAXDATASIZE);
>>>>>> +		my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf,&taddr,
>>>>>> +			    RPC_MAXDATASIZE, RPC_MAXDATASIZE);
>>>>>> 		if (my_xprt == (SVCXPRT *)NULL) {
>>>>>> 			syslog(LOG_ERR, "%s: could not create service",
>>>>>> 					nconf->nc_netid);
>>>>>> 			goto error;
>>>>>> 		}
>>>>>> 	}
>>>>>> +	if (!checkbind)
>>>>>> +		return 1;
>>>>>>
>>>>>> #ifdef PORTMAP
>>>>>> 	/*
>>>>>> --
>>>>>> 1.7.7.6
>>>>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] rpcbind: Only listen on requested hostnames for NC_TPI_COTS connections

[Myklebust, Trond]

T24gVHVlLCAyMDEyLTA0LTI0IGF0IDExOjQwIC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
T24gQXByIDI0LCAyMDEyLCBhdCAxMToyOSBBTSwgSmltIFJlZXMgd3JvdGU6DQo+IA0KPiA+IE5p
ZWxzIGRlIFZvcyB3cm90ZToNCj4gPiANCj4gPiAgT24gMDQvMjMvMjAxMiAwNjoyMiBQTSwgQ2h1
Y2sgTGV2ZXIgd3JvdGU6DQo+ID4+IA0KPiA+PiBTbyB5b3UgZGlkIGNoZWNrIHRoZSBtYWlsIGFy
Y2hpdmU/ICBJIHNlZW0gdG8gcmVjYWxsIG90aGVyIHBhdGNoZXMgbGlrZQ0KPiA+PiB0aGlzIGlu
IHRoZSBwYXN0LCBhbmQgdGhhdCB0aGVyZSBpcyBhIHJlYXNvbiB3aHkgcnBjYmluZCB3b3JrcyB0
aGlzIHdheS4NCj4gPj4gSSBzaW1wbHkgZG9uJ3QgcmVtZW1iZXIgdGhlIHNwZWNpZmljcyByaWdo
dCBhdCB0aGUgbW9tZW50Lg0KPiA+IA0KPiA+ICBJIGRpZCwgYnV0IG5vIG1lc3NhZ2VzIGFib3V0
IHRoaXMgc3ViamVjdCBjb21lIHVwIGZvciBtZS4uLiBNYXliZSBJJ20gbG9va2luZw0KPiA+ICBp
biB0aGUgd3JvbmcgcGxhY2VzIDotLw0KPiA+IA0KPiA+IEkgYXNrZWQgYWJvdXQgdGhpcyBsYXN0
IE5vdmVtYmVyLCBhbmQgYXQgdGhhdCB0aW1lIENodWNrIHJlZmVycmVkIG1lIHRvIHRoZQ0KPiA+
IG1haWwgYXJjaGl2ZSB0b28uICBJIGNvdWxkbid0IGZpbmQgYW55IGRpc2N1c3Npb24gZWl0aGVy
LiAgQnV0IHRoZSBiZWhhdmlvcg0KPiA+IGlzIGludGVudGlvbmFsLCBzbyBJIGRvbid0IHRoaW5r
IHlvdSdsbCBnZXQgYSBwYXRjaCBhY2NlcHRlZC4NCj4gDQo+IEdvaW5nIGJhY2sgdG8gdGhlIHJw
Y2JpbmQoOCkgbWFuIHBhZ2UsIHRoZSAiLWgiIGlzIG1lYW50IHRvIHdvcmsgYXJvdW5kIGEgYnJv
a2VubmVzcyBvZiBSUEMgb3ZlciBVRFAuICBVRFAgcmVwbGllcyBjYW4gY29tZSBmcm9tIGFueSBz
ZXJ2ZXIgYWRkcmVzcywgYXMgSSBtZW50aW9uZWQgYmVmb3JlLCBhbmQgbW9zdCBMaW51eCBjbGll
bnRzLCBhdCBsZWFzdCwgZG9uJ3QgbGlrZSBhbiBSUEMgcmVwbHkgdG8gY29tZSBmcm9tIGEgZGlm
ZmVyZW50IGFkZHJlc3MgdGhhbiB0aGUgcmVxdWVzdCB3YXMgc2VudCB0by4NCg0KU2VlIHRoZSBJ
UF9QS1RJTkZPIHNvY2tldCBjb250cm9sIG1lc3NhZ2UgZGVmaW5pdGlvbiBmb3IgdGhlIGNvcnJl
Y3Qgd2F5DQp0byBkbyB0aGlzICgnbWFuIDcgaXAnKS4gVGhhdCBpcyBob3cgdGhlIGtlcm5lbCBS
UEMgc2VydmVyIGNvZGUNCmRldGVybWluZXMgdG8gd2hpY2ggYWRkcmVzcyB0aGUgbWVzc2FnZSB3
YXMgc2VudCwgYW5kIGlzIGhvdyBpdCBzZXRzIHRoZQ0KcmVwbHkncyBzb3VyY2UgYWRkcmVzcy4u
Lg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoN
Ck5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==