On Thu, Feb 16, 2023 at 02:07:20AM +0100, Stefano Brivio wrote:
> passt supports ranges of forwarded ports as well as 'all' for TCP and
> UDP, so it might be convenient to proceed if we fail to bind only
> some of the desired ports.
> 
> But if we fail to bind even a single port for a given specification,
> we're clearly, unexpectedly, conflicting with another network
> service. In that case, report failure and exit.
> 
> Reported-by: Yalan Zhang <yalzhang@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  conf.c | 47 ++++++++++++++++++++++++++++++++++-------------
>  tcp.c  | 25 ++++++++++++++++++-------
>  tcp.h  |  4 ++--
>  udp.c  | 16 +++++++++++++---
>  udp.h  |  4 ++--
>  5 files changed, 69 insertions(+), 27 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index f175405..675d961 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -179,9 +179,9 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  {
>  	char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
>  	char buf[BUFSIZ], *spec, *ifname = NULL, *p;
> +	bool exclude_only = true, bound_one = false;
>  	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
>  	sa_family_t af = AF_UNSPEC;
> -	bool exclude_only = true;
>  
>  	if (!strcmp(optarg, "none")) {
>  		if (fwd->mode)
> @@ -215,12 +215,19 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  		memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8);
>  
>  		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
> -			if (optname == 't')
> -				tcp_sock_init(c, AF_UNSPEC, NULL, NULL, i);
> -			else if (optname == 'u')
> -				udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, i);
> +			if (optname == 't') {
> +				if (!tcp_sock_init(c, AF_UNSPEC, NULL, NULL, i))
> +					bound_one = true;
> +			} else if (optname == 'u') {
> +				if (!udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL,
> +						   i))
> +					bound_one = true;
> +			}
>  		}
>  
> +		if (!bound_one)
> +			goto bind_fail;
> +
>  		return;
>  	}
>  
> @@ -293,12 +300,18 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  
>  			bitmap_set(fwd->map, i);
>  
> -			if (optname == 't')
> -				tcp_sock_init(c, af, addr, ifname, i);
> -			else if (optname == 'u')
> -				udp_sock_init(c, 0, af, addr, ifname, i);
> +			if (optname == 't') {
> +				if (!tcp_sock_init(c, af, addr, ifname, i))
> +					bound_one = true;
> +			} else if (optname == 'u') {
> +				if (!udp_sock_init(c, 0, af, addr, ifname, i))
> +					bound_one = true;
> +			}
>  		}
>  
> +		if (!bound_one)
> +			goto bind_fail;
> +
>  		return;
>  	}
>  
> @@ -339,13 +352,19 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  
>  			fwd->delta[i] = mapped_range.first - orig_range.first;
>  
> -			if (optname == 't')
> -				tcp_sock_init(c, af, addr, ifname, i);
> -			else if (optname == 'u')
> -				udp_sock_init(c, 0, af, addr, ifname, i);
> +			if (optname == 't') {
> +				if (!tcp_sock_init(c, af, addr, ifname, i))
> +					bound_one = true;
> +			} else if (optname == 'u') {
> +				if (udp_sock_init(c, 0, af, addr, ifname, i))
> +					bound_one = true;
> +			}
>  		}
>  	} while ((p = next_chunk(p, ',')));
>  
> +	if (!bound_one)
> +		goto bind_fail;
> +
>  	return;
>  bad:
>  	die("Invalid port specifier %s", optarg);
> @@ -353,6 +372,8 @@ overlap:
>  	die("Overlapping port specifier %s", optarg);
>  mode_conflict:
>  	die("Port forwarding mode '%s' conflicts with previous mode", optarg);
> +bind_fail:
> +	die("Failed to bind any port for '-%c %s', exiting", optname, optarg);
>  }
>  
>  /**
> diff --git a/tcp.c b/tcp.c
> index f028e01..32ce1e9 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2916,20 +2916,31 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
>   * @addr:	Pointer to address for binding, NULL if not configured
>   * @ifname:	Name of interface to bind to, NULL if not configured
>   * @port:	Port, host order
> + *
> + * Return: 0 on (partial) success, -1 on (complete) failure
>   */
> -void tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
> -		   const char *ifname, in_port_t port)
> +int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
> +		  const char *ifname, in_port_t port)
>  {
> +	int ret = 0;
> +
>  	if (af == AF_UNSPEC && c->ifi4 && c->ifi6)
>  		/* Attempt to get a dual stack socket */
>  		if (tcp_sock_init_af(c, AF_UNSPEC, port, addr, ifname) >= 0)
> -			return;
> +			return 0;
>  
>  	/* Otherwise create a socket per IP version */
> -	if ((af == AF_INET  || af == AF_UNSPEC) && c->ifi4)
> -		tcp_sock_init_af(c, AF_INET, port, addr, ifname);
> -	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6)
> -		tcp_sock_init_af(c, AF_INET6, port, addr, ifname);
> +	if ((af == AF_INET  || af == AF_UNSPEC) && c->ifi4) {
> +		if (tcp_sock_init_af(c, AF_INET, port, addr, ifname) < 0)
> +			ret = -1;
> +	}
> +
> +	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
> +		if (tcp_sock_init_af(c, AF_INET6, port, addr, ifname) < 0)
> +			ret = -1;
> +	}
> +
> +	return ret;
>  }
>  
>  /**
> diff --git a/tcp.h b/tcp.h
> index 236038f..5527c5b 100644
> --- a/tcp.h
> +++ b/tcp.h
> @@ -17,8 +17,8 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
>  		      const struct timespec *now);
>  int tcp_tap_handler(struct ctx *c, int af, const void *addr,
>  		    const struct pool *p, const struct timespec *now);
> -void tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
> -		   const char *ifname, in_port_t port);
> +int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
> +		  const char *ifname, in_port_t port);
>  int tcp_init(struct ctx *c);
>  void tcp_timer(struct ctx *c, const struct timespec *ts);
>  void tcp_defer_handler(struct ctx *c);
> diff --git a/udp.c b/udp.c
> index 2e9b7e6..c913d27 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -955,12 +955,14 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
>   * @addr:	Pointer to address for binding, NULL if not configured
>   * @ifname:	Name of interface to bind to, NULL if not configured
>   * @port:	Port, host order
> + *
> + * Return: 0 on (partial) success, -1 on (complete) failure
>   */
> -void udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> -		   const void *addr, const char *ifname, in_port_t port)
> +int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> +		  const void *addr, const char *ifname, in_port_t port)
>  {
>  	union udp_epoll_ref uref = { .u32 = 0 };
> -	int s;
> +	int s, ret = 0;
>  
>  	if (ns) {
>  		uref.udp.port = (in_port_t)(port +
> @@ -989,6 +991,9 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  				    ifname, port, uref.u32);
>  			udp_splice_ns[V4][port].sock = s;
>  		}
> +
> +		if (s < 0)
> +			ret = -1;
>  	}
>  
>  	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
> @@ -1009,7 +1014,12 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  				    ifname, port, uref.u32);
>  			udp_splice_ns[V6][port].sock = s;
>  		}
> +
> +		if (s < 0)
> +			ret = -1;
>  	}
> +
> +	return ret;
>  }
>  
>  /**
> diff --git a/udp.h b/udp.h
> index 68082ea..0e81be8 100644
> --- a/udp.h
> +++ b/udp.h
> @@ -12,8 +12,8 @@ void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
>  		      const struct timespec *now);
>  int udp_tap_handler(struct ctx *c, int af, const void *addr,
>  		    const struct pool *p, const struct timespec *now);
> -void udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> -		   const void *addr, const char *ifname, in_port_t port);
> +int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> +		  const void *addr, const char *ifname, in_port_t port);
>  int udp_init(struct ctx *c);
>  void udp_timer(struct ctx *c, const struct timespec *ts);
>  void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson