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

[Niels de Vos <ndevos@redhat.com>]

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