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

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

[Stefano Brivio]

...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>
---
 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
-- 
@@ -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
-- 
2.39.2

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

[Paul Holzinger]

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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2567 bytes --]

On Wed, Feb 14, 2024 at 10:15:38AM +0100, 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>

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

I think this is about as good a compromise for the semantics as we can
hope for.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]