Re: [PATCH] conf, passt.1: Exit if we can't bind a forwarded port, except for -[tu] all

[Paul Holzinger <pholzing@redhat.com>]

Thanks Stefano,

On 14/02/2024 10:15, Stefano Brivio wrote:
> ...or similar, that is, if only excluded ranges are given (implying
> we'll forward any other available port). In that case, we'll usually
> forward large sets of ports, and it might be inconvenient for the
> user to skip excluding single ports that are already taken.
>
> The existing behaviour, that is, exiting only if we fail to bind all
> the ports for one given forwarding option, turns out to be
> problematic for several aspects raised by Paul:
>
> - Podman merges ranges anyway, so we might fail to bind all the ports
>    from a specific range given by the user, but we'll not fail anyway
>    because Podman merges it with another one where we succeed to bind
>    at least one port. At the same time, there should be no semantic
>    difference between multiple ranges given by a single option and
>    multiple ranges given as multiple options: it's unexpected and
>    not documented
>
> - the user might actually rely on a given port to be forwarded to a
>    given container or a virtual machine, and if connections are
>    forwarded to an unrelated process, this might raise security
>    concerns
>
> - given that we can try and fail to bind multiple ports before
>    exiting (in case we can't bind any), we don't have a specific error
>    code we can return to the user, so we don't give the user helpful
>    indication as to why we couldn't bind ports.
>
> Exit as soon as we fail to create or bind a socket for a given
> forwarded port, and report the actual error.
>
> Keep the current behaviour, however, in case the user wants to
> forward all the (available) ports for a given protocol, or all the
> ports with excluded ranges only. There, it's more reasonable that
> the user is expecting partial failures, and it's probably convenient
> that we continue with the ports we could forward.
>
> Update the manual page to reflect the new behaviour, and the old
> behaviour too in the cases where we keep it.
>
> Suggested-by: Paul Holzinger <pholzing@redhat.com>
> Link: https://github.com/containers/podman/pull/21563#issuecomment-1937024642
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Tested-by: Paul Holzinger <pholzing@redhat.com>
> ---
>   conf.c  | 36 +++++++++++-------------------------
>   passt.1 | 15 ++++++++++++---
>   2 files changed, 23 insertions(+), 28 deletions(-)
>
> diff --git a/conf.c b/conf.c
> index 5e15b66..f68f5d3 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -119,6 +119,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>   	bool exclude_only = true, bound_one = false;
>   	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
>   	sa_family_t af = AF_UNSPEC;
> +	unsigned i;
>   	int ret;
>   
>   	if (!strcmp(optarg, "none")) {
> @@ -141,8 +142,6 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>   	}
>   
>   	if (!strcmp(optarg, "all")) {
> -		unsigned i;
> -
>   		if (fwd->mode)
>   			goto mode_conflict;
>   
> @@ -171,7 +170,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>   		}
>   
>   		if (!bound_one)
> -			goto bind_fail;
> +			goto bind_all_fail;
>   
>   		return;
>   	}
> @@ -221,7 +220,6 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>   	p = spec;
>   	do {
>   		struct port_range xrange;
> -		unsigned i;
>   
>   		if (*p != '~') {
>   			/* Not an exclude range, parse later */
> @@ -244,8 +242,6 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>   	} while ((p = next_chunk(p, ',')));
>   
>   	if (exclude_only) {
> -		unsigned i;
> -
>   		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
>   			if (bitmap_isset(exclude, i))
>   				continue;
> @@ -271,7 +267,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>   		}
>   
>   		if (!bound_one)
> -			goto bind_fail;
> +			goto bind_all_fail;
>   
>   		return;
>   	}
> @@ -280,7 +276,6 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>   	p = spec;
>   	do {
>   		struct port_range orig_range, mapped_range;
> -		unsigned i;
>   
>   		if (*p == '~')
>   			/* Exclude range, already parsed */
> @@ -314,28 +309,16 @@ 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') {
> +			ret = 0;
> +			if (optname == 't')
>   				ret = tcp_sock_init(c, af, addr, ifname, i);
> -				if (ret == -ENFILE || ret == -EMFILE)
> -					goto enfile;
> -				if (!ret)
> -					bound_one = true;
> -			} else if (optname == 'u') {
> +			else if (optname == 'u')
>   				ret = udp_sock_init(c, 0, af, addr, ifname, i);
> -				if (ret == -ENFILE || ret == -EMFILE)
> -					goto enfile;
> -				if (!ret)
> -					bound_one = true;
> -			} else {
> -				/* No way to check in advance for -T and -U */
> -				bound_one = true;
> -			}
> +			if (ret)
> +				goto bind_fail;
>   		}
>   	} while ((p = next_chunk(p, ',')));
>   
> -	if (!bound_one)
> -		goto bind_fail;
> -
>   	return;
>   enfile:
>   	die("Can't open enough sockets for port specifier: %s", optarg);
> @@ -344,6 +327,9 @@ bad:
>   mode_conflict:
>   	die("Port forwarding mode '%s' conflicts with previous mode", optarg);
>   bind_fail:
> +	die("Failed to bind port %u (%s) for option '-%c %s', exiting",
> +	    i, strerror(-ret), optname, optarg);
> +bind_all_fail:
>   	die("Failed to bind any port for '-%c %s', exiting", optname, optarg);
>   }
>   
> diff --git a/passt.1 b/passt.1
> index cc678ed..dc2d719 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -355,7 +355,8 @@ Don't forward any ports
>   .TP
>   .BR all
>   Forward all unbound, non-ephemeral ports, as permitted by current capabilities.
> -For low (< 1024) ports, see \fBNOTES\fR.
> +For low (< 1024) ports, see \fBNOTES\fR. No failures are reported for
> +unavailable ports, unless no ports could be forwarded at all.
>   
>   .TP
>   .BR ports
> @@ -364,7 +365,11 @@ optionally, with target ports after \fI:\fR, if they differ. Specific addresses
>   can be bound as well, separated by \fI/\fR, and also, since Linux 5.7, limited
>   to specific interfaces, prefixed by \fI%\fR. Within given ranges, selected ports
>   and ranges can be excluded by an additional specification prefixed by \fI~\fR.
> -Specifying excluded ranges only implies that all other ports are forwarded.
> +
> +Specifying excluded ranges only implies that all other ports are forwarded. In
> +this case, no failures are reported for unavailable ports, unless no ports could
> +be forwarded at all.
> +
>   Examples:
>   .RS
>   .TP
> @@ -447,7 +452,11 @@ optionally, with target ports after \fI:\fR, if they differ. Specific addresses
>   can be bound as well, separated by \fI/\fR, and also, since Linux 5.7, limited
>   to specific interfaces, prefixed by \fI%\fR. Within given ranges, selected ports
>   and ranges can be excluded by an additional specification prefixed by \fI~\fR.
> -Specifying excluded ranges only implies that all other ports are forwarded.
> +
> +Specifying excluded ranges only implies that all other ports are forwarded. In
> +this case, no failures are reported for unavailable ports, unless no ports could
> +be forwarded at all.
> +
>   Examples:
>   .RS
>   .TP