public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
* [PATCH 0/8] Clean up and fix bugs in port forwarding data structures
@ 2022-09-23  4:57 David Gibson
  2022-09-23  4:57 ` [PATCH 1/8] Improve types and names for port forwarding configuration David Gibson
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: David Gibson @ 2022-09-23  4:57 UTC (permalink / raw)
  To: passt-dev

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

The information we need to keep track of which ports are forwarded
where is currently split across several different values and data
structures in several different places.  Worse, a number of those
structures are incorrectly sized due to an off by one error, which
could lead to buffer overruns.

Fix the sizing, and re-organize the data structures in a way that
should make it less likely to repeat that mistake.

While we're there, correct a similar off-by-one mis-sizing of a number
of other arrays.

David Gibson (8):
  Improve types and names for port forwarding configuration
  Consolidate port forwarding configuration into a common structure
  udp: Delay initialization of UDP reversed port mapping table
  Don't use indirect remap functions for conf_ports()
  Pass entire port forwarding configuration substructure to conf_ports()
  Treat port numbers as unsigned
  Fix widespread off-by-one error dealing with port numbers
  icmp: Correct off by one errors dealing with number of echo request
    ids

 Makefile     |   2 +-
 conf.c       | 136 +++++++++++++++++++++++----------------------------
 icmp.c       |   5 +-
 passt.h      |   1 +
 port_fwd.h   |  34 +++++++++++++
 tcp.c        |  68 ++++++++------------------
 tcp.h        |  13 ++---
 tcp_splice.c |   4 +-
 udp.c        |  59 ++++++++++------------
 udp.h        |  27 +++++-----
 10 files changed, 168 insertions(+), 181 deletions(-)
 create mode 100644 port_fwd.h

-- 
2.37.3


^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH 1/8] Improve types and names for port forwarding configuration
  2022-09-23  4:57 [PATCH 0/8] Clean up and fix bugs in port forwarding data structures David Gibson
@ 2022-09-23  4:57 ` David Gibson
  2022-09-23 22:52   ` Stefano Brivio
  2022-09-23  4:58 ` [PATCH 2/8] Consolidate port forwarding configuration into a common structure David Gibson
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: David Gibson @ 2022-09-23  4:57 UTC (permalink / raw)
  To: passt-dev

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

enum conf_port_type is local to conf.c and is used to track the port
forwarding mode during configuration.  We don't keep it around in the
context structure, however the 'init_detect_ports' and 'ns_detect_ports'
fields in the context are based solely on this.  Rather than changing
encoding, just include the forwarding mode into the context structure.
Move the type definition to a new port_fwd.h, which is kind of trivial at
the moment but will have more stuff later.

While we're there, "conf_port_type" doesn't really convey that this enum is
describing how port forwarding is configured.  Rename it to port_fwd_mode.
The variables (now fields) of this type also have mildly confusing names
since it's not immediately obvious whether 'ns' and 'init' refer to the
source or destination of the packets.  Use "in" (host to guest / init to
ns) and "out" (guest to host / ns to init) instead.

This has the added bonus that we no longer have locals 'udp_init' and
'tcp_init' which shadow global functions.

In addition, add a typedef 'port_fwd_map' for a bitmap of each port number,
which is used in several places.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile   |  2 +-
 conf.c     | 73 +++++++++++++++++++++++++++---------------------------
 passt.h    |  1 +
 port_fwd.h | 19 ++++++++++++++
 tcp.c      | 12 ++++-----
 tcp.h      | 12 ++++-----
 udp.h      | 12 ++++-----
 7 files changed, 76 insertions(+), 55 deletions(-)
 create mode 100644 port_fwd.h

diff --git a/Makefile b/Makefile
index c47a5f6..432ee7a 100644
--- a/Makefile
+++ b/Makefile
@@ -41,7 +41,7 @@ MANPAGES = passt.1 pasta.1 qrap.1
 
 PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h icmp.h \
 	isolation.h lineread.h ndp.h netlink.h packet.h passt.h pasta.h \
-	pcap.h siphash.h tap.h tcp.h tcp_splice.h udp.h util.h
+	pcap.h port_fwd.h siphash.h tap.h tcp.h tcp_splice.h udp.h util.h
 HEADERS = $(PASST_HEADERS) seccomp.h
 
 # On gcc 11.2, with -O2 and -flto, tcp_hash() and siphash_20b(), if inlined,
diff --git a/conf.c b/conf.c
index 7ecfa1e..1ff43b7 100644
--- a/conf.c
+++ b/conf.c
@@ -64,14 +64,14 @@ void get_bound_ports(struct ctx *c, int ns, uint8_t proto)
 	}
 
 	if (proto == IPPROTO_UDP) {
-		memset(udp_map, 0, USHRT_MAX / 8);
+		memset(udp_map, 0, sizeof(port_fwd_map));
 		procfs_scan_listen(c, IPPROTO_UDP, V4, ns, udp_map, udp_excl);
 		procfs_scan_listen(c, IPPROTO_UDP, V6, ns, udp_map, udp_excl);
 
 		procfs_scan_listen(c, IPPROTO_TCP, V4, ns, udp_map, udp_excl);
 		procfs_scan_listen(c, IPPROTO_TCP, V6, ns, udp_map, udp_excl);
 	} else if (proto == IPPROTO_TCP) {
-		memset(tcp_map, 0, USHRT_MAX / 8);
+		memset(tcp_map, 0, sizeof(port_fwd_map));
 		procfs_scan_listen(c, IPPROTO_TCP, V4, ns, tcp_map, tcp_excl);
 		procfs_scan_listen(c, IPPROTO_TCP, V6, ns, tcp_map, tcp_excl);
 	}
@@ -106,31 +106,25 @@ static int get_bound_ports_ns(void *arg)
 	return 0;
 }
 
-enum conf_port_type {
-	PORT_SPEC = 1,
-	PORT_NONE,
-	PORT_AUTO,
-	PORT_ALL,
-};
-
 /**
  * conf_ports() - Parse port configuration options, initialise UDP/TCP sockets
  * @c:		Execution context
  * @optname:	Short option name, t, T, u, or U
  * @optarg:	Option argument (port specification)
- * @set:	Pointer to @conf_port_type to be set (port binding type)
+ * @set:	Pointer to @port_fwd_mode to be set (port forwarding mode)
  *
  * Return: -EINVAL on parsing error, 0 otherwise
  */
 static int conf_ports(struct ctx *c, char optname, const char *optarg,
-		      enum conf_port_type *set)
+		      enum port_fwd_mode *set)
 {
 	int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port;
 	char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
-	uint8_t *map, exclude[DIV_ROUND_UP(USHRT_MAX, 8)] = { 0 };
 	void (*remap)(in_port_t port, in_port_t delta);
 	char buf[BUFSIZ], *sep, *spec, *p;
+	port_fwd_map exclude = { 0 };
 	sa_family_t af = AF_UNSPEC;
+	uint8_t *map;
 
 	if (optname == 't') {
 		map = c->tcp.port_to_tap;
@@ -151,14 +145,14 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 	if (!strcmp(optarg, "none")) {
 		if (*set)
 			return -EINVAL;
-		*set = PORT_NONE;
+		*set = FWD_NONE;
 		return 0;
 	}
 
 	if (!strcmp(optarg, "auto")) {
 		if (*set || c->mode != MODE_PASTA)
 			return -EINVAL;
-		*set = PORT_AUTO;
+		*set = FWD_AUTO;
 		return 0;
 	}
 
@@ -167,7 +161,7 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 
 		if (*set || c->mode != MODE_PASST)
 			return -EINVAL;
-		*set = PORT_ALL;
+		*set = FWD_ALL;
 		memset(map, 0xff, PORT_EPHEMERAL_MIN / 8);
 
 		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
@@ -180,10 +174,10 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 		return 0;
 	}
 
-	if (*set > PORT_SPEC)
+	if (*set > FWD_SPEC)
 		return -EINVAL;
 
-	*set = PORT_SPEC;
+	*set = FWD_SPEC;
 
 	strncpy(buf, optarg, sizeof(buf) - 1);
 
@@ -1088,8 +1082,6 @@ void conf(struct ctx *c, int argc, char **argv)
 	};
 	struct get_bound_ports_ns_arg ns_ports_arg = { .c = c };
 	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX] = { 0 };
-	enum conf_port_type tcp_tap = 0, tcp_init = 0;
-	enum conf_port_type udp_tap = 0, udp_init = 0;
 	bool v4_only = false, v6_only = false;
 	struct in6_addr *dns6 = c->ip6.dns;
 	struct fqdn *dnss = c->dns_search;
@@ -1103,6 +1095,9 @@ void conf(struct ctx *c, int argc, char **argv)
 	if (c->mode == MODE_PASTA)
 		c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
 
+	c->tcp.fwd_mode_in = c->tcp.fwd_mode_out = 0;
+	c->udp.fwd_mode_in = c->udp.fwd_mode_out = 0;
+
 	do {
 		const char *optstring;
 
@@ -1553,7 +1548,7 @@ void conf(struct ctx *c, int argc, char **argv)
 	/* Now we can process port configuration options */
 	optind = 1;
 	do {
-		enum conf_port_type *set = NULL;
+		enum port_fwd_mode *fwd = NULL;
 		const char *optstring;
 
 		if (c->mode == MODE_PASST)
@@ -1568,15 +1563,15 @@ void conf(struct ctx *c, int argc, char **argv)
 		case 'T':
 		case 'U':
 			if (name == 't')
-				set = &tcp_tap;
+				fwd = &c->tcp.fwd_mode_in;
 			else if (name == 'T')
-				set = &tcp_init;
+				fwd = &c->tcp.fwd_mode_out;
 			else if (name == 'u')
-				set = &udp_tap;
+				fwd = &c->udp.fwd_mode_in;
 			else if (name == 'U')
-				set = &udp_init;
+				fwd = &c->udp.fwd_mode_out;
 
-			if (!optarg || conf_ports(c, name, optarg, set))
+			if (!optarg || conf_ports(c, name, optarg, fwd))
 				usage(argv[0]);
 
 			break;
@@ -1605,33 +1600,39 @@ void conf(struct ctx *c, int argc, char **argv)
 			if_indextoname(c->ifi6, c->pasta_ifn);
 	}
 
-	c->tcp.ns_detect_ports   = c->udp.ns_detect_ports   = 0;
-	c->tcp.init_detect_ports = c->udp.init_detect_ports = 0;
-
 	if (c->mode == MODE_PASTA) {
 		c->proc_net_tcp[V4][0] = c->proc_net_tcp[V4][1] = -1;
 		c->proc_net_tcp[V6][0] = c->proc_net_tcp[V6][1] = -1;
 		c->proc_net_udp[V4][0] = c->proc_net_udp[V4][1] = -1;
 		c->proc_net_udp[V6][0] = c->proc_net_udp[V6][1] = -1;
 
-		if (!tcp_tap || tcp_tap == PORT_AUTO) {
-			c->tcp.ns_detect_ports = 1;
+		if (!c->tcp.fwd_mode_in || c->tcp.fwd_mode_in == FWD_AUTO) {
+			c->tcp.fwd_mode_in = FWD_AUTO;
 			ns_ports_arg.proto = IPPROTO_TCP;
 			NS_CALL(get_bound_ports_ns, &ns_ports_arg);
 		}
-		if (!udp_tap || udp_tap == PORT_AUTO) {
-			c->udp.ns_detect_ports = 1;
+		if (!c->udp.fwd_mode_in || c->udp.fwd_mode_in == FWD_AUTO) {
+			c->udp.fwd_mode_in = FWD_AUTO;
 			ns_ports_arg.proto = IPPROTO_UDP;
 			NS_CALL(get_bound_ports_ns, &ns_ports_arg);
 		}
-		if (!tcp_init || tcp_init == PORT_AUTO) {
-			c->tcp.init_detect_ports = 1;
+		if (!c->tcp.fwd_mode_out || c->tcp.fwd_mode_out == FWD_AUTO) {
+			c->tcp.fwd_mode_out = FWD_AUTO;
 			get_bound_ports(c, 0, IPPROTO_TCP);
 		}
-		if (!udp_init || udp_init == PORT_AUTO) {
-			c->udp.init_detect_ports = 1;
+		if (!c->udp.fwd_mode_out || c->udp.fwd_mode_out == FWD_AUTO) {
+			c->udp.fwd_mode_out = FWD_AUTO;
 			get_bound_ports(c, 0, IPPROTO_UDP);
 		}
+	} else {
+		if (!c->tcp.fwd_mode_in)
+			c->tcp.fwd_mode_in = FWD_NONE;
+		if (!c->tcp.fwd_mode_out)
+			c->tcp.fwd_mode_out= FWD_NONE;
+		if (!c->udp.fwd_mode_in)
+			c->udp.fwd_mode_in = FWD_NONE;
+		if (!c->udp.fwd_mode_out)
+			c->udp.fwd_mode_out = FWD_NONE;
 	}
 
 	if (!c->quiet)
diff --git a/passt.h b/passt.h
index 8c8d710..48e1096 100644
--- a/passt.h
+++ b/passt.h
@@ -33,6 +33,7 @@ union epoll_ref;
 
 #include "packet.h"
 #include "icmp.h"
+#include "port_fwd.h"
 #include "tcp.h"
 #include "udp.h"
 
diff --git a/port_fwd.h b/port_fwd.h
new file mode 100644
index 0000000..5f6ad7e
--- /dev/null
+++ b/port_fwd.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: AGPL-3.0-or-later
+ * Copyright Red Hat
+ * Author: Stefano Brivio <sbrivio(a)redhat.com>
+ * Author: David Gibson <david(a)gibson.dropbear.id.au>
+ */
+
+#ifndef PORT_FWD_H
+#define PORT_FWD_H
+
+enum port_fwd_mode {
+	FWD_SPEC = 1,
+	FWD_NONE,
+	FWD_AUTO,
+	FWD_ALL,
+};
+
+typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];
+
+#endif /* PORT_FWD_H */
diff --git a/tcp.c b/tcp.c
index ec8c32e..0bdef7f 100644
--- a/tcp.c
+++ b/tcp.c
@@ -3133,7 +3133,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			else
 				s = -1;
 
-			if (c->tcp.init_detect_ports)
+			if (c->tcp.fwd_mode_in == FWD_AUTO)
 				tcp_sock_init_ext[port][V4] = s;
 		}
 
@@ -3148,7 +3148,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			else
 				s = -1;
 
-			if (c->tcp.ns_detect_ports) {
+			if (c->tcp.fwd_mode_out == FWD_AUTO) {
 				if (ns)
 					tcp_sock_ns[port][V4] = s;
 				else
@@ -3174,7 +3174,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			else
 				s = -1;
 
-			if (c->tcp.init_detect_ports)
+			if (c->tcp.fwd_mode_in == FWD_AUTO)
 				tcp_sock_init_ext[port][V6] = s;
 		}
 
@@ -3189,7 +3189,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			else
 				s = -1;
 
-			if (c->tcp.ns_detect_ports) {
+			if (c->tcp.fwd_mode_out == FWD_AUTO) {
 				if (ns)
 					tcp_sock_ns[port][V6] = s;
 				else
@@ -3489,14 +3489,14 @@ void tcp_timer(struct ctx *c, const struct timespec *ts)
 		struct tcp_port_detect_arg detect_arg = { c, 0 };
 		struct tcp_port_rebind_arg rebind_arg = { c, 0 };
 
-		if (c->tcp.init_detect_ports) {
+		if (c->tcp.fwd_mode_in == FWD_AUTO) {
 			detect_arg.detect_in_ns = 0;
 			tcp_port_detect(&detect_arg);
 			rebind_arg.bind_in_ns = 1;
 			NS_CALL(tcp_port_rebind, &rebind_arg);
 		}
 
-		if (c->tcp.ns_detect_ports) {
+		if (c->tcp.fwd_mode_out == FWD_AUTO) {
 			detect_arg.detect_in_ns = 1;
 			NS_CALL(tcp_port_detect, &detect_arg);
 			rebind_arg.bind_in_ns = 0;
diff --git a/tcp.h b/tcp.h
index 6431b75..58989db 100644
--- a/tcp.h
+++ b/tcp.h
@@ -58,9 +58,9 @@ union tcp_epoll_ref {
  * @conn_count:		Count of connections (not spliced) in connection table
  * @splice_conn_count:	Count of spliced connections in connection table
  * @port_to_tap:	Ports bound host-side, packets to tap or spliced
- * @init_detect_ports:	If set, periodically detect ports bound in init
+ * @fwd_mode_in:	Port forwarding mode for inbound packets
  * @port_to_init:	Ports bound namespace-side, spliced to init
- * @ns_detect_ports:	If set, periodically detect ports bound in namespace
+ * @fwd_mode_out:	Port forwarding mode for outbound packets
  * @timer_run:		Timestamp of most recent timer run
  * @kernel_snd_wnd:	Kernel reports sending window (with commit 8f7baad7f035)
  * @pipe_size:		Size of pipes for spliced connections
@@ -69,10 +69,10 @@ struct tcp_ctx {
 	uint64_t hash_secret[2];
 	int conn_count;
 	int splice_conn_count;
-	uint8_t port_to_tap	[DIV_ROUND_UP(USHRT_MAX, 8)];
-	int init_detect_ports;
-	uint8_t port_to_init	[DIV_ROUND_UP(USHRT_MAX, 8)];
-	int ns_detect_ports;
+	port_fwd_map port_to_tap;
+	enum port_fwd_mode fwd_mode_in;
+	port_fwd_map port_to_init;
+	enum port_fwd_mode fwd_mode_out;
 	struct timespec timer_run;
 #ifdef HAS_SND_WND
 	int kernel_snd_wnd;
diff --git a/udp.h b/udp.h
index 8f82842..0283ce6 100644
--- a/udp.h
+++ b/udp.h
@@ -47,16 +47,16 @@ union udp_epoll_ref {
 /**
  * struct udp_ctx - Execution context for UDP
  * @port_to_tap:	Ports bound host-side, data to tap or ns L4 socket
- * @init_detect_ports:	If set, periodically detect ports bound in init (TODO)
+ * @fwd_mode_in:	Port forwarding mode for inbound packets
  * @port_to_init:	Ports bound namespace-side, data to init L4 socket
- * @ns_detect_ports:	If set, periodically detect ports bound in namespace
+ * @fwd_mode_out:	Port forwarding mode for outbound packets
  * @timer_run:		Timestamp of most recent timer run
  */
 struct udp_ctx {
-	uint8_t port_to_tap		[DIV_ROUND_UP(USHRT_MAX, 8)];
-	int init_detect_ports;
-	uint8_t port_to_init		[DIV_ROUND_UP(USHRT_MAX, 8)];
-	int ns_detect_ports;
+	port_fwd_map port_to_tap;
+	enum port_fwd_mode fwd_mode_in;
+	port_fwd_map port_to_init;
+	enum port_fwd_mode fwd_mode_out;
 	struct timespec timer_run;
 };
 
-- 
@@ -47,16 +47,16 @@ union udp_epoll_ref {
 /**
  * struct udp_ctx - Execution context for UDP
  * @port_to_tap:	Ports bound host-side, data to tap or ns L4 socket
- * @init_detect_ports:	If set, periodically detect ports bound in init (TODO)
+ * @fwd_mode_in:	Port forwarding mode for inbound packets
  * @port_to_init:	Ports bound namespace-side, data to init L4 socket
- * @ns_detect_ports:	If set, periodically detect ports bound in namespace
+ * @fwd_mode_out:	Port forwarding mode for outbound packets
  * @timer_run:		Timestamp of most recent timer run
  */
 struct udp_ctx {
-	uint8_t port_to_tap		[DIV_ROUND_UP(USHRT_MAX, 8)];
-	int init_detect_ports;
-	uint8_t port_to_init		[DIV_ROUND_UP(USHRT_MAX, 8)];
-	int ns_detect_ports;
+	port_fwd_map port_to_tap;
+	enum port_fwd_mode fwd_mode_in;
+	port_fwd_map port_to_init;
+	enum port_fwd_mode fwd_mode_out;
 	struct timespec timer_run;
 };
 
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH 2/8] Consolidate port forwarding configuration into a common structure
  2022-09-23  4:57 [PATCH 0/8] Clean up and fix bugs in port forwarding data structures David Gibson
  2022-09-23  4:57 ` [PATCH 1/8] Improve types and names for port forwarding configuration David Gibson
@ 2022-09-23  4:58 ` David Gibson
  2022-09-23  4:58 ` [PATCH 3/8] udp: Delay initialization of UDP reversed port mapping table David Gibson
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2022-09-23  4:58 UTC (permalink / raw)
  To: passt-dev

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

The configuration for how to forward ports in and out of the guest/ns is
divided between several different variables.  For each connect direction
and protocol we have a mode in the udp/tcp context structure, a bitmap
of which ports to forward also in the context structure and an array of
deltas to apply if the outward facing and inward facing port numbers are
different.  This last is a separate global variable, rather than being in
the context structure, for no particular reason.  UDP also requires an
additional array which has the reverse mapping used for return packets.

Consolidate these into a re-used substructure in the context structure.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c       | 74 ++++++++++++++++++++++++++--------------------------
 port_fwd.h   | 12 +++++++++
 tcp.c        | 42 ++++++++++++++---------------
 tcp.h        | 15 +++++------
 tcp_splice.c |  4 +--
 udp.c        | 30 +++++++++------------
 udp.h        | 27 ++++++++++++-------
 7 files changed, 106 insertions(+), 98 deletions(-)

diff --git a/conf.c b/conf.c
index 1ff43b7..f74a32f 100644
--- a/conf.c
+++ b/conf.c
@@ -52,15 +52,15 @@ void get_bound_ports(struct ctx *c, int ns, uint8_t proto)
 	uint8_t *udp_map, *udp_excl, *tcp_map, *tcp_excl;
 
 	if (ns) {
-		udp_map = c->udp.port_to_tap;
-		udp_excl = c->udp.port_to_init;
-		tcp_map = c->tcp.port_to_tap;
-		tcp_excl = c->tcp.port_to_init;
+		udp_map = c->udp.fwd_in.f.map;
+		udp_excl = c->udp.fwd_out.f.map;
+		tcp_map = c->tcp.fwd_in.map;
+		tcp_excl = c->tcp.fwd_out.map;
 	} else {
-		udp_map = c->udp.port_to_init;
-		udp_excl = c->udp.port_to_tap;
-		tcp_map = c->tcp.port_to_init;
-		tcp_excl = c->tcp.port_to_tap;
+		udp_map = c->udp.fwd_out.f.map;
+		udp_excl = c->udp.fwd_in.f.map;
+		tcp_map = c->tcp.fwd_out.map;
+		tcp_excl = c->tcp.fwd_in.map;
 	}
 
 	if (proto == IPPROTO_UDP) {
@@ -120,23 +120,23 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 {
 	int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port;
 	char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
-	void (*remap)(in_port_t port, in_port_t delta);
+	void (*remap)(struct ctx *c, in_port_t port, in_port_t delta);
 	char buf[BUFSIZ], *sep, *spec, *p;
 	port_fwd_map exclude = { 0 };
 	sa_family_t af = AF_UNSPEC;
 	uint8_t *map;
 
 	if (optname == 't') {
-		map = c->tcp.port_to_tap;
+		map = c->tcp.fwd_in.map;
 		remap = tcp_remap_to_tap;
 	} else if (optname == 'T') {
-		map = c->tcp.port_to_init;
+		map = c->tcp.fwd_out.map;
 		remap = tcp_remap_to_init;
 	} else if (optname == 'u') {
-		map = c->udp.port_to_tap;
+		map = c->udp.fwd_in.f.map;
 		remap = udp_remap_to_tap;
 	} else if (optname == 'U') {
-		map = c->udp.port_to_init;
+		map = c->udp.fwd_out.f.map;
 		remap = udp_remap_to_init;
 	} else {	/* For gcc -O3 */
 		return 0;
@@ -365,8 +365,8 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 
 				if (start_dst != -1) {
 					/* 80:8080 or 22-80:8080:8080 */
-					remap(i, (in_port_t)(start_dst -
-							     start_src));
+					remap(c, i, (in_port_t)(start_dst -
+								start_src));
 				}
 
 				if (optname == 't')
@@ -1095,8 +1095,8 @@ void conf(struct ctx *c, int argc, char **argv)
 	if (c->mode == MODE_PASTA)
 		c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
 
-	c->tcp.fwd_mode_in = c->tcp.fwd_mode_out = 0;
-	c->udp.fwd_mode_in = c->udp.fwd_mode_out = 0;
+	c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = 0;
+	c->udp.fwd_in.f.mode = c->udp.fwd_out.f.mode = 0;
 
 	do {
 		const char *optstring;
@@ -1563,13 +1563,13 @@ void conf(struct ctx *c, int argc, char **argv)
 		case 'T':
 		case 'U':
 			if (name == 't')
-				fwd = &c->tcp.fwd_mode_in;
+				fwd = &c->tcp.fwd_in.mode;
 			else if (name == 'T')
-				fwd = &c->tcp.fwd_mode_out;
+				fwd = &c->tcp.fwd_out.mode;
 			else if (name == 'u')
-				fwd = &c->udp.fwd_mode_in;
+				fwd = &c->udp.fwd_in.f.mode;
 			else if (name == 'U')
-				fwd = &c->udp.fwd_mode_out;
+				fwd = &c->udp.fwd_out.f.mode;
 
 			if (!optarg || conf_ports(c, name, optarg, fwd))
 				usage(argv[0]);
@@ -1606,33 +1606,33 @@ void conf(struct ctx *c, int argc, char **argv)
 		c->proc_net_udp[V4][0] = c->proc_net_udp[V4][1] = -1;
 		c->proc_net_udp[V6][0] = c->proc_net_udp[V6][1] = -1;
 
-		if (!c->tcp.fwd_mode_in || c->tcp.fwd_mode_in == FWD_AUTO) {
-			c->tcp.fwd_mode_in = FWD_AUTO;
+		if (!c->tcp.fwd_in.mode || c->tcp.fwd_in.mode == FWD_AUTO) {
+			c->tcp.fwd_in.mode = FWD_AUTO;
 			ns_ports_arg.proto = IPPROTO_TCP;
 			NS_CALL(get_bound_ports_ns, &ns_ports_arg);
 		}
-		if (!c->udp.fwd_mode_in || c->udp.fwd_mode_in == FWD_AUTO) {
-			c->udp.fwd_mode_in = FWD_AUTO;
+		if (!c->udp.fwd_in.f.mode || c->udp.fwd_in.f.mode == FWD_AUTO) {
+			c->udp.fwd_in.f.mode = FWD_AUTO;
 			ns_ports_arg.proto = IPPROTO_UDP;
 			NS_CALL(get_bound_ports_ns, &ns_ports_arg);
 		}
-		if (!c->tcp.fwd_mode_out || c->tcp.fwd_mode_out == FWD_AUTO) {
-			c->tcp.fwd_mode_out = FWD_AUTO;
+		if (!c->tcp.fwd_out.mode || c->tcp.fwd_out.mode == FWD_AUTO) {
+			c->tcp.fwd_out.mode = FWD_AUTO;
 			get_bound_ports(c, 0, IPPROTO_TCP);
 		}
-		if (!c->udp.fwd_mode_out || c->udp.fwd_mode_out == FWD_AUTO) {
-			c->udp.fwd_mode_out = FWD_AUTO;
+		if (!c->udp.fwd_out.f.mode || c->udp.fwd_out.f.mode == FWD_AUTO) {
+			c->udp.fwd_out.f.mode = FWD_AUTO;
 			get_bound_ports(c, 0, IPPROTO_UDP);
 		}
 	} else {
-		if (!c->tcp.fwd_mode_in)
-			c->tcp.fwd_mode_in = FWD_NONE;
-		if (!c->tcp.fwd_mode_out)
-			c->tcp.fwd_mode_out= FWD_NONE;
-		if (!c->udp.fwd_mode_in)
-			c->udp.fwd_mode_in = FWD_NONE;
-		if (!c->udp.fwd_mode_out)
-			c->udp.fwd_mode_out = FWD_NONE;
+		if (!c->tcp.fwd_in.mode)
+			c->tcp.fwd_in.mode = FWD_NONE;
+		if (!c->tcp.fwd_out.mode)
+			c->tcp.fwd_out.mode = FWD_NONE;
+		if (!c->udp.fwd_in.f.mode)
+			c->udp.fwd_in.f.mode = FWD_NONE;
+		if (!c->udp.fwd_out.f.mode)
+			c->udp.fwd_out.f.mode = FWD_NONE;
 	}
 
 	if (!c->quiet)
diff --git a/port_fwd.h b/port_fwd.h
index 5f6ad7e..5507bf2 100644
--- a/port_fwd.h
+++ b/port_fwd.h
@@ -16,4 +16,16 @@ enum port_fwd_mode {
 
 typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];
 
+/**
+ * port_fwd - Describes port forwarding for one protocol and direction
+ * @mode:	Overall forwarding mode (all, none, auto, specific ports)
+ * @map:	Bitmap describing which ports are forwarded
+ * @delta:	Offset between the original destination and mapped port number
+ */
+struct port_fwd {
+	enum port_fwd_mode mode;
+	port_fwd_map map;
+	in_port_t delta[USHRT_MAX];
+};
+
 #endif /* PORT_FWD_H */
diff --git a/tcp.c b/tcp.c
index 0bdef7f..e44177f 100644
--- a/tcp.c
+++ b/tcp.c
@@ -546,10 +546,6 @@ static const char *tcp_flag_str[] __attribute((__unused__)) = {
 	"ACK_TO_TAP_DUE", "ACK_FROM_TAP_DUE",
 };
 
-/* Port re-mappings as delta, indexed by original destination port */
-static in_port_t		tcp_port_delta_to_tap	[USHRT_MAX];
-static in_port_t		tcp_port_delta_to_init	[USHRT_MAX];
-
 /* Listening sockets, used for automatic port forwarding in pasta mode only */
 static int tcp_sock_init_lo	[USHRT_MAX][IP_VERSIONS];
 static int tcp_sock_init_ext	[USHRT_MAX][IP_VERSIONS];
@@ -954,22 +950,24 @@ static void conn_event_do(const struct ctx *c, struct tcp_conn *conn,
 
 /**
  * tcp_remap_to_tap() - Set delta for port translation toward guest/tap
+ * @c:		Execution context
  * @port:	Original destination port, host order
  * @delta:	Delta to be added to original destination port
  */
-void tcp_remap_to_tap(in_port_t port, in_port_t delta)
+void tcp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta)
 {
-	tcp_port_delta_to_tap[port] = delta;
+	c->tcp.fwd_in.delta[port] = delta;
 }
 
 /**
  * tcp_remap_to_tap() - Set delta for port translation toward init namespace
+ * @c:		Execution context
  * @port:	Original destination port, host order
  * @delta:	Delta to be added to original destination port
  */
-void tcp_remap_to_init(in_port_t port, in_port_t delta)
+void tcp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta)
 {
-	tcp_port_delta_to_init[port] = delta;
+	c->tcp.fwd_out.delta[port] = delta;
 }
 
 /**
@@ -3109,11 +3107,9 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 	int s;
 
 	if (ns) {
-		tref.tcp.index = (in_port_t)(port +
-					     tcp_port_delta_to_init[port]);
+		tref.tcp.index = (in_port_t)(port + c->tcp.fwd_out.delta[port]);
 	} else {
-		tref.tcp.index = (in_port_t)(port +
-					     tcp_port_delta_to_tap[port]);
+		tref.tcp.index = (in_port_t)(port + c->tcp.fwd_in.delta[port]);
 	}
 
 	if (af == AF_INET || af == AF_UNSPEC) {
@@ -3133,7 +3129,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			else
 				s = -1;
 
-			if (c->tcp.fwd_mode_in == FWD_AUTO)
+			if (c->tcp.fwd_in.mode == FWD_AUTO)
 				tcp_sock_init_ext[port][V4] = s;
 		}
 
@@ -3148,7 +3144,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			else
 				s = -1;
 
-			if (c->tcp.fwd_mode_out == FWD_AUTO) {
+			if (c->tcp.fwd_out.mode == FWD_AUTO) {
 				if (ns)
 					tcp_sock_ns[port][V4] = s;
 				else
@@ -3174,7 +3170,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			else
 				s = -1;
 
-			if (c->tcp.fwd_mode_in == FWD_AUTO)
+			if (c->tcp.fwd_in.mode == FWD_AUTO)
 				tcp_sock_init_ext[port][V6] = s;
 		}
 
@@ -3189,7 +3185,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			else
 				s = -1;
 
-			if (c->tcp.fwd_mode_out == FWD_AUTO) {
+			if (c->tcp.fwd_out.mode == FWD_AUTO) {
 				if (ns)
 					tcp_sock_ns[port][V6] = s;
 				else
@@ -3213,7 +3209,7 @@ static int tcp_sock_init_ns(void *arg)
 	ns_enter(c);
 
 	for (port = 0; port < USHRT_MAX; port++) {
-		if (!bitmap_isset(c->tcp.port_to_init, port))
+		if (!bitmap_isset(c->tcp.fwd_out.map, port))
 			continue;
 
 		tcp_sock_init(c, 1, AF_UNSPEC, NULL, port);
@@ -3413,7 +3409,7 @@ static int tcp_port_rebind(void *arg)
 		ns_enter(a->c);
 
 		for (port = 0; port < USHRT_MAX; port++) {
-			if (!bitmap_isset(a->c->tcp.port_to_init, port)) {
+			if (!bitmap_isset(a->c->tcp.fwd_out.map, port)) {
 				if (tcp_sock_ns[port][V4] >= 0) {
 					close(tcp_sock_ns[port][V4]);
 					tcp_sock_ns[port][V4] = -1;
@@ -3428,7 +3424,7 @@ static int tcp_port_rebind(void *arg)
 			}
 
 			/* Don't loop back our own ports */
-			if (bitmap_isset(a->c->tcp.port_to_tap, port))
+			if (bitmap_isset(a->c->tcp.fwd_in.map, port))
 				continue;
 
 			if ((a->c->ifi4 && tcp_sock_ns[port][V4] == -1) ||
@@ -3437,7 +3433,7 @@ static int tcp_port_rebind(void *arg)
 		}
 	} else {
 		for (port = 0; port < USHRT_MAX; port++) {
-			if (!bitmap_isset(a->c->tcp.port_to_tap, port)) {
+			if (!bitmap_isset(a->c->tcp.fwd_in.map, port)) {
 				if (tcp_sock_init_ext[port][V4] >= 0) {
 					close(tcp_sock_init_ext[port][V4]);
 					tcp_sock_init_ext[port][V4] = -1;
@@ -3461,7 +3457,7 @@ static int tcp_port_rebind(void *arg)
 			}
 
 			/* Don't loop back our own ports */
-			if (bitmap_isset(a->c->tcp.port_to_init, port))
+			if (bitmap_isset(a->c->tcp.fwd_out.map, port))
 				continue;
 
 			if ((a->c->ifi4 && tcp_sock_init_ext[port][V4] == -1) ||
@@ -3489,14 +3485,14 @@ void tcp_timer(struct ctx *c, const struct timespec *ts)
 		struct tcp_port_detect_arg detect_arg = { c, 0 };
 		struct tcp_port_rebind_arg rebind_arg = { c, 0 };
 
-		if (c->tcp.fwd_mode_in == FWD_AUTO) {
+		if (c->tcp.fwd_in.mode == FWD_AUTO) {
 			detect_arg.detect_in_ns = 0;
 			tcp_port_detect(&detect_arg);
 			rebind_arg.bind_in_ns = 1;
 			NS_CALL(tcp_port_rebind, &rebind_arg);
 		}
 
-		if (c->tcp.fwd_mode_out == FWD_AUTO) {
+		if (c->tcp.fwd_out.mode == FWD_AUTO) {
 			detect_arg.detect_in_ns = 1;
 			NS_CALL(tcp_port_detect, &detect_arg);
 			rebind_arg.bind_in_ns = 0;
diff --git a/tcp.h b/tcp.h
index 58989db..502b096 100644
--- a/tcp.h
+++ b/tcp.h
@@ -29,8 +29,8 @@ void tcp_defer_handler(struct ctx *c);
 void tcp_sock_set_bufsize(const struct ctx *c, int s);
 void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s,
 		       const uint32_t *ip_da);
-void tcp_remap_to_tap(in_port_t port, in_port_t delta);
-void tcp_remap_to_init(in_port_t port, in_port_t delta);
+void tcp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta);
+void tcp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta);
 
 /**
  * union tcp_epoll_ref - epoll reference portion for TCP connections
@@ -58,9 +58,8 @@ union tcp_epoll_ref {
  * @conn_count:		Count of connections (not spliced) in connection table
  * @splice_conn_count:	Count of spliced connections in connection table
  * @port_to_tap:	Ports bound host-side, packets to tap or spliced
- * @fwd_mode_in:	Port forwarding mode for inbound packets
- * @port_to_init:	Ports bound namespace-side, spliced to init
- * @fwd_mode_out:	Port forwarding mode for outbound packets
+ * @fwd_in:		Port forwarding configuration for inbound packets
+ * @fwd_out:		Port forwarding configuration for outbound packets
  * @timer_run:		Timestamp of most recent timer run
  * @kernel_snd_wnd:	Kernel reports sending window (with commit 8f7baad7f035)
  * @pipe_size:		Size of pipes for spliced connections
@@ -69,10 +68,8 @@ struct tcp_ctx {
 	uint64_t hash_secret[2];
 	int conn_count;
 	int splice_conn_count;
-	port_fwd_map port_to_tap;
-	enum port_fwd_mode fwd_mode_in;
-	port_fwd_map port_to_init;
-	enum port_fwd_mode fwd_mode_out;
+	struct port_fwd fwd_in;
+	struct port_fwd fwd_out;
 	struct timespec timer_run;
 #ifdef HAS_SND_WND
 	int kernel_snd_wnd;
diff --git a/tcp_splice.c b/tcp_splice.c
index 61e9b23..edbcfd4 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -514,7 +514,7 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn,
 	struct tcp_splice_connect_ns_arg ns_arg = { c, conn, port, 0 };
 	int *p, i, s = -1;
 
-	if (bitmap_isset(c->tcp.port_to_tap, port))
+	if (bitmap_isset(c->tcp.fwd_in.map, port))
 		p = CONN_V6(conn) ? ns_sock_pool6 : ns_sock_pool4;
 	else
 		p = CONN_V6(conn) ? init_sock_pool6 : init_sock_pool4;
@@ -525,7 +525,7 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn,
 			break;
 	}
 
-	if (s < 0 && bitmap_isset(c->tcp.port_to_tap, port)) {
+	if (s < 0 && bitmap_isset(c->tcp.fwd_in.map, port)) {
 		NS_CALL(tcp_splice_connect_ns, &ns_arg);
 		return ns_arg.ret;
 	}
diff --git a/udp.c b/udp.c
index 0b4e134..38bb8d8 100644
--- a/udp.c
+++ b/udp.c
@@ -166,12 +166,6 @@ struct udp_splice_port {
 static struct udp_tap_port	udp_tap_map	[IP_VERSIONS][USHRT_MAX];
 static struct udp_splice_port	udp_splice_map	[IP_VERSIONS][USHRT_MAX];
 
-/* Port re-mappings as delta, indexed by original destination port */
-static in_port_t		udp_port_delta_to_tap	[USHRT_MAX];
-static in_port_t		udp_port_delta_from_tap	[USHRT_MAX];
-static in_port_t		udp_port_delta_to_init	[USHRT_MAX];
-static in_port_t		udp_port_delta_from_init[USHRT_MAX];
-
 enum udp_act_type {
 	UDP_ACT_TAP,
 	UDP_ACT_NS_CONN,
@@ -265,24 +259,26 @@ static struct mmsghdr	udp_mmh_sendto		[UDP_SPLICE_FRAMES];
 
 /**
  * udp_remap_to_tap() - Set delta for port translation to/from guest/tap
+ * @c:		Execution context
  * @port:	Original destination port, host order
  * @delta:	Delta to be added to original destination port
  */
-void udp_remap_to_tap(in_port_t port, in_port_t delta)
+void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta)
 {
-	udp_port_delta_to_tap[port] = delta;
-	udp_port_delta_from_tap[port + delta] = USHRT_MAX - delta;
+	c->udp.fwd_in.f.delta[port] = delta;
+	c->udp.fwd_in.rdelta[port + delta] = USHRT_MAX - delta;
 }
 
 /**
  * udp_remap_to_init() - Set delta for port translation to/from init namespace
+ * @c:		Execution context
  * @port:	Original destination port, host order
  * @delta:	Delta to be added to original destination port
  */
-void udp_remap_to_init(in_port_t port, in_port_t delta)
+void udp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta)
 {
-	udp_port_delta_to_init[port] = delta;
-	udp_port_delta_from_init[port + delta] = USHRT_MAX - delta;
+	c->udp.fwd_out.f.delta[port] = delta;
+	c->udp.fwd_out.rdelta[port + delta] = USHRT_MAX - delta;
 }
 
 /**
@@ -583,7 +579,7 @@ static void udp_sock_handler_splice(const struct ctx *c, union epoll_ref ref,
 
 	switch (ref.r.p.udp.udp.splice) {
 	case UDP_TO_NS:
-		src += udp_port_delta_from_init[src];
+		src += c->udp.fwd_out.rdelta[src];
 
 		if (!(s = udp_splice_map[v6][src].ns_conn_sock)) {
 			struct udp_splice_connect_ns_arg arg = {
@@ -603,7 +599,7 @@ static void udp_sock_handler_splice(const struct ctx *c, union epoll_ref ref,
 		send_dst = udp_splice_map[v6][dst].init_dst_port;
 		break;
 	case UDP_TO_INIT:
-		src += udp_port_delta_from_tap[src];
+		src += c->udp.fwd_in.rdelta[src];
 
 		if (!(s = udp_splice_map[v6][src].init_conn_sock)) {
 			s = udp_splice_connect(c, v6, ref.r.s, src, dst,
@@ -1121,10 +1117,10 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 
 	if (ns) {
 		uref.udp.port = (in_port_t)(port +
-					    udp_port_delta_to_init[port]);
+					    c->udp.fwd_out.f.delta[port]);
 	} else {
 		uref.udp.port = (in_port_t)(port +
-					    udp_port_delta_to_tap[port]);
+					    c->udp.fwd_in.f.delta[port]);
 	}
 
 	if (af == AF_INET || af == AF_UNSPEC) {
@@ -1209,7 +1205,7 @@ int udp_sock_init_ns(void *arg)
 		return 0;
 
 	for (dst = 0; dst < USHRT_MAX; dst++) {
-		if (!bitmap_isset(c->udp.port_to_init, dst))
+		if (!bitmap_isset(c->udp.fwd_out.f.map, dst))
 			continue;
 
 		udp_sock_init(c, 1, AF_UNSPEC, NULL, dst);
diff --git a/udp.h b/udp.h
index 0283ce6..cfd1a97 100644
--- a/udp.h
+++ b/udp.h
@@ -18,8 +18,8 @@ int udp_init(const 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,
 		       const uint32_t *ip_da);
-void udp_remap_to_tap(in_port_t port, in_port_t delta);
-void udp_remap_to_init(in_port_t port, in_port_t delta);
+void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta);
+void udp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta);
 
 /**
  * union udp_epoll_ref - epoll reference portion for TCP connections
@@ -44,19 +44,26 @@ union udp_epoll_ref {
 	uint32_t u32;
 };
 
+
+/**
+ * udp_port_fwd - UDP specific port forwarding configuration
+ * @f:		Generic forwarding configuration
+ * @rdelta:	Reversed delta map to translate source ports on return packets
+ */
+struct udp_port_fwd {
+	struct port_fwd f;
+	in_port_t rdelta[USHRT_MAX];
+};
+
 /**
  * struct udp_ctx - Execution context for UDP
- * @port_to_tap:	Ports bound host-side, data to tap or ns L4 socket
- * @fwd_mode_in:	Port forwarding mode for inbound packets
- * @port_to_init:	Ports bound namespace-side, data to init L4 socket
- * @fwd_mode_out:	Port forwarding mode for outbound packets
+ * @fwd_in:		Port forwarding configuration for inbound packets
+ * @fwd_out:		Port forwarding configuration for outbound packets
  * @timer_run:		Timestamp of most recent timer run
  */
 struct udp_ctx {
-	port_fwd_map port_to_tap;
-	enum port_fwd_mode fwd_mode_in;
-	port_fwd_map port_to_init;
-	enum port_fwd_mode fwd_mode_out;
+	struct udp_port_fwd fwd_in;
+	struct udp_port_fwd fwd_out;
 	struct timespec timer_run;
 };
 
-- 
@@ -18,8 +18,8 @@ int udp_init(const 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,
 		       const uint32_t *ip_da);
-void udp_remap_to_tap(in_port_t port, in_port_t delta);
-void udp_remap_to_init(in_port_t port, in_port_t delta);
+void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta);
+void udp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta);
 
 /**
  * union udp_epoll_ref - epoll reference portion for TCP connections
@@ -44,19 +44,26 @@ union udp_epoll_ref {
 	uint32_t u32;
 };
 
+
+/**
+ * udp_port_fwd - UDP specific port forwarding configuration
+ * @f:		Generic forwarding configuration
+ * @rdelta:	Reversed delta map to translate source ports on return packets
+ */
+struct udp_port_fwd {
+	struct port_fwd f;
+	in_port_t rdelta[USHRT_MAX];
+};
+
 /**
  * struct udp_ctx - Execution context for UDP
- * @port_to_tap:	Ports bound host-side, data to tap or ns L4 socket
- * @fwd_mode_in:	Port forwarding mode for inbound packets
- * @port_to_init:	Ports bound namespace-side, data to init L4 socket
- * @fwd_mode_out:	Port forwarding mode for outbound packets
+ * @fwd_in:		Port forwarding configuration for inbound packets
+ * @fwd_out:		Port forwarding configuration for outbound packets
  * @timer_run:		Timestamp of most recent timer run
  */
 struct udp_ctx {
-	port_fwd_map port_to_tap;
-	enum port_fwd_mode fwd_mode_in;
-	port_fwd_map port_to_init;
-	enum port_fwd_mode fwd_mode_out;
+	struct udp_port_fwd fwd_in;
+	struct udp_port_fwd fwd_out;
 	struct timespec timer_run;
 };
 
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH 3/8] udp: Delay initialization of UDP reversed port mapping table
  2022-09-23  4:57 [PATCH 0/8] Clean up and fix bugs in port forwarding data structures David Gibson
  2022-09-23  4:57 ` [PATCH 1/8] Improve types and names for port forwarding configuration David Gibson
  2022-09-23  4:58 ` [PATCH 2/8] Consolidate port forwarding configuration into a common structure David Gibson
@ 2022-09-23  4:58 ` David Gibson
  2022-09-23  4:58 ` [PATCH 4/8] Don't use indirect remap functions for conf_ports() David Gibson
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2022-09-23  4:58 UTC (permalink / raw)
  To: passt-dev

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

Because it's connectionless, when mapping UDP ports we need, in addition
to the table of deltas for destination ports needed by TCP, we need an
inverted table to translate the source ports on return packets.

Currently we fill out the inverted table at the same time we construct the
main table in udp_remap_to_tap() and udp_remap_to_init().  However, we
don't use either table until after we've initialized UDP, so we can delay
the construction of the reverse table to udp_init().  This makes the
configuration more symmetric between TCP and UDP which will enable further
cleanups.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 udp.c | 25 ++++++++++++++++++++++---
 udp.h |  2 +-
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/udp.c b/udp.c
index 38bb8d8..eb32dda 100644
--- a/udp.c
+++ b/udp.c
@@ -109,6 +109,7 @@
 #include <sys/uio.h>
 #include <unistd.h>
 #include <time.h>
+#include <assert.h>
 
 #include "checksum.h"
 #include "util.h"
@@ -266,7 +267,6 @@ static struct mmsghdr	udp_mmh_sendto		[UDP_SPLICE_FRAMES];
 void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta)
 {
 	c->udp.fwd_in.f.delta[port] = delta;
-	c->udp.fwd_in.rdelta[port + delta] = USHRT_MAX - delta;
 }
 
 /**
@@ -278,7 +278,23 @@ void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta)
 void udp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta)
 {
 	c->udp.fwd_out.f.delta[port] = delta;
-	c->udp.fwd_out.rdelta[port + delta] = USHRT_MAX - delta;
+}
+
+/**
+ * udp_invert_portmap() - Compute reverse port translations for return packets
+ * @fwd:	Port forwarding configuration to compute reverse map for
+ */
+static void udp_invert_portmap(struct udp_port_fwd *fwd)
+{
+	int i;
+
+	assert(ARRAY_SIZE(fwd->f.delta) == ARRAY_SIZE(fwd->rdelta));
+	for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) {
+		in_port_t delta = fwd->f.delta[i];
+
+		if (delta)
+			fwd->rdelta[(in_port_t)i + delta] = USHRT_MAX - delta;
+	}
 }
 
 /**
@@ -1267,7 +1283,7 @@ static void udp_splice_iov_init(void)
  *
  * Return: 0
  */
-int udp_init(const struct ctx *c)
+int udp_init(struct ctx *c)
 {
 	if (c->ifi4)
 		udp_sock4_iov_init();
@@ -1275,6 +1291,9 @@ int udp_init(const struct ctx *c)
 	if (c->ifi6)
 		udp_sock6_iov_init();
 
+	udp_invert_portmap(&c->udp.fwd_in);
+	udp_invert_portmap(&c->udp.fwd_out);
+
 	if (c->mode == MODE_PASTA) {
 		udp_splice_iov_init();
 		NS_CALL(udp_sock_init_ns, c);
diff --git a/udp.h b/udp.h
index cfd1a97..25422b6 100644
--- a/udp.h
+++ b/udp.h
@@ -14,7 +14,7 @@ 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, in_port_t port);
-int udp_init(const struct ctx *c);
+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,
 		       const uint32_t *ip_da);
-- 
@@ -14,7 +14,7 @@ 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, in_port_t port);
-int udp_init(const struct ctx *c);
+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,
 		       const uint32_t *ip_da);
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH 4/8] Don't use indirect remap functions for conf_ports()
  2022-09-23  4:57 [PATCH 0/8] Clean up and fix bugs in port forwarding data structures David Gibson
                   ` (2 preceding siblings ...)
  2022-09-23  4:58 ` [PATCH 3/8] udp: Delay initialization of UDP reversed port mapping table David Gibson
@ 2022-09-23  4:58 ` David Gibson
  2022-09-23  4:58 ` [PATCH 5/8] Pass entire port forwarding configuration substructure to conf_ports() David Gibson
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2022-09-23  4:58 UTC (permalink / raw)
  To: passt-dev

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

Now that we've delayed initialization of the UDP specific "reverse" map
until udp_init(), the only difference between the various 'remap' functions
used in conf_ports() is which array they target.  So, simplify by open
coding the logic into conf_ports() with a pointer to the correct mapping
array.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c | 14 +++++++-------
 tcp.c  | 22 ----------------------
 tcp.h  |  2 --
 udp.c  | 22 ----------------------
 udp.h  |  2 --
 5 files changed, 7 insertions(+), 55 deletions(-)

diff --git a/conf.c b/conf.c
index f74a32f..c7c2cee 100644
--- a/conf.c
+++ b/conf.c
@@ -120,24 +120,24 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 {
 	int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port;
 	char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
-	void (*remap)(struct ctx *c, in_port_t port, in_port_t delta);
 	char buf[BUFSIZ], *sep, *spec, *p;
 	port_fwd_map exclude = { 0 };
 	sa_family_t af = AF_UNSPEC;
+	in_port_t *delta;
 	uint8_t *map;
 
 	if (optname == 't') {
 		map = c->tcp.fwd_in.map;
-		remap = tcp_remap_to_tap;
+		delta = c->tcp.fwd_in.delta;
 	} else if (optname == 'T') {
 		map = c->tcp.fwd_out.map;
-		remap = tcp_remap_to_init;
+		delta = c->tcp.fwd_out.delta;
 	} else if (optname == 'u') {
 		map = c->udp.fwd_in.f.map;
-		remap = udp_remap_to_tap;
+		delta = c->udp.fwd_in.f.delta;
 	} else if (optname == 'U') {
 		map = c->udp.fwd_out.f.map;
-		remap = udp_remap_to_init;
+		delta = c->udp.fwd_out.f.delta;
 	} else {	/* For gcc -O3 */
 		return 0;
 	}
@@ -365,8 +365,8 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 
 				if (start_dst != -1) {
 					/* 80:8080 or 22-80:8080:8080 */
-					remap(c, i, (in_port_t)(start_dst -
-								start_src));
+					delta[i] = (in_port_t)(start_dst -
+							       start_src);
 				}
 
 				if (optname == 't')
diff --git a/tcp.c b/tcp.c
index e44177f..509a0b3 100644
--- a/tcp.c
+++ b/tcp.c
@@ -948,28 +948,6 @@ static void conn_event_do(const struct ctx *c, struct tcp_conn *conn,
 		conn_event_do(c, conn, event);				\
 	} while (0)
 
-/**
- * tcp_remap_to_tap() - Set delta for port translation toward guest/tap
- * @c:		Execution context
- * @port:	Original destination port, host order
- * @delta:	Delta to be added to original destination port
- */
-void tcp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta)
-{
-	c->tcp.fwd_in.delta[port] = delta;
-}
-
-/**
- * tcp_remap_to_tap() - Set delta for port translation toward init namespace
- * @c:		Execution context
- * @port:	Original destination port, host order
- * @delta:	Delta to be added to original destination port
- */
-void tcp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta)
-{
-	c->tcp.fwd_out.delta[port] = delta;
-}
-
 /**
  * tcp_rtt_dst_low() - Check if low RTT was seen for connection endpoint
  * @conn:	Connection pointer
diff --git a/tcp.h b/tcp.h
index 502b096..2548d4d 100644
--- a/tcp.h
+++ b/tcp.h
@@ -29,8 +29,6 @@ void tcp_defer_handler(struct ctx *c);
 void tcp_sock_set_bufsize(const struct ctx *c, int s);
 void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s,
 		       const uint32_t *ip_da);
-void tcp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta);
-void tcp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta);
 
 /**
  * union tcp_epoll_ref - epoll reference portion for TCP connections
diff --git a/udp.c b/udp.c
index eb32dda..d17b3b4 100644
--- a/udp.c
+++ b/udp.c
@@ -258,28 +258,6 @@ static struct mmsghdr	udp_mmh_send		[UDP_SPLICE_FRAMES];
 static struct iovec	udp_iov_sendto		[UDP_SPLICE_FRAMES];
 static struct mmsghdr	udp_mmh_sendto		[UDP_SPLICE_FRAMES];
 
-/**
- * udp_remap_to_tap() - Set delta for port translation to/from guest/tap
- * @c:		Execution context
- * @port:	Original destination port, host order
- * @delta:	Delta to be added to original destination port
- */
-void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta)
-{
-	c->udp.fwd_in.f.delta[port] = delta;
-}
-
-/**
- * udp_remap_to_init() - Set delta for port translation to/from init namespace
- * @c:		Execution context
- * @port:	Original destination port, host order
- * @delta:	Delta to be added to original destination port
- */
-void udp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta)
-{
-	c->udp.fwd_out.f.delta[port] = delta;
-}
-
 /**
  * udp_invert_portmap() - Compute reverse port translations for return packets
  * @fwd:	Port forwarding configuration to compute reverse map for
diff --git a/udp.h b/udp.h
index 25422b6..bc7b259 100644
--- a/udp.h
+++ b/udp.h
@@ -18,8 +18,6 @@ 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,
 		       const uint32_t *ip_da);
-void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta);
-void udp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta);
 
 /**
  * union udp_epoll_ref - epoll reference portion for TCP connections
-- 
@@ -18,8 +18,6 @@ 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,
 		       const uint32_t *ip_da);
-void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta);
-void udp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta);
 
 /**
  * union udp_epoll_ref - epoll reference portion for TCP connections
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH 5/8] Pass entire port forwarding configuration substructure to conf_ports()
  2022-09-23  4:57 [PATCH 0/8] Clean up and fix bugs in port forwarding data structures David Gibson
                   ` (3 preceding siblings ...)
  2022-09-23  4:58 ` [PATCH 4/8] Don't use indirect remap functions for conf_ports() David Gibson
@ 2022-09-23  4:58 ` David Gibson
  2022-09-23  4:58 ` [PATCH 6/8] Treat port numbers as unsigned David Gibson
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2022-09-23  4:58 UTC (permalink / raw)
  To: passt-dev

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

conf_ports() switches on the optname argument to select the target array
for several updates.  Now that all these maps are in a common structure, we
can simplify by just passing in a pointer to the whole struct port_fwd to
update.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c | 62 +++++++++++++++++++++-------------------------------------
 1 file changed, 22 insertions(+), 40 deletions(-)

diff --git a/conf.c b/conf.c
index c7c2cee..2cbc519 100644
--- a/conf.c
+++ b/conf.c
@@ -111,58 +111,40 @@ static int get_bound_ports_ns(void *arg)
  * @c:		Execution context
  * @optname:	Short option name, t, T, u, or U
  * @optarg:	Option argument (port specification)
- * @set:	Pointer to @port_fwd_mode to be set (port forwarding mode)
+ * @fwd:	Pointer to @port_fwd to be updated
  *
  * Return: -EINVAL on parsing error, 0 otherwise
  */
-static int conf_ports(struct ctx *c, char optname, const char *optarg,
-		      enum port_fwd_mode *set)
+static int conf_ports(const struct ctx *c, char optname, const char *optarg,
+		      struct port_fwd *fwd)
 {
 	int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port;
 	char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
 	char buf[BUFSIZ], *sep, *spec, *p;
 	port_fwd_map exclude = { 0 };
 	sa_family_t af = AF_UNSPEC;
-	in_port_t *delta;
-	uint8_t *map;
-
-	if (optname == 't') {
-		map = c->tcp.fwd_in.map;
-		delta = c->tcp.fwd_in.delta;
-	} else if (optname == 'T') {
-		map = c->tcp.fwd_out.map;
-		delta = c->tcp.fwd_out.delta;
-	} else if (optname == 'u') {
-		map = c->udp.fwd_in.f.map;
-		delta = c->udp.fwd_in.f.delta;
-	} else if (optname == 'U') {
-		map = c->udp.fwd_out.f.map;
-		delta = c->udp.fwd_out.f.delta;
-	} else {	/* For gcc -O3 */
-		return 0;
-	}
 
 	if (!strcmp(optarg, "none")) {
-		if (*set)
+		if (fwd->mode)
 			return -EINVAL;
-		*set = FWD_NONE;
+		fwd->mode = FWD_NONE;
 		return 0;
 	}
 
 	if (!strcmp(optarg, "auto")) {
-		if (*set || c->mode != MODE_PASTA)
+		if (fwd->mode || c->mode != MODE_PASTA)
 			return -EINVAL;
-		*set = FWD_AUTO;
+		fwd->mode = FWD_AUTO;
 		return 0;
 	}
 
 	if (!strcmp(optarg, "all")) {
 		int i;
 
-		if (*set || c->mode != MODE_PASST)
+		if (fwd->mode || c->mode != MODE_PASST)
 			return -EINVAL;
-		*set = FWD_ALL;
-		memset(map, 0xff, PORT_EPHEMERAL_MIN / 8);
+		fwd->mode = FWD_ALL;
+		memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8);
 
 		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
 			if (optname == 't')
@@ -174,10 +156,10 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 		return 0;
 	}
 
-	if (*set > FWD_SPEC)
+	if (fwd->mode > FWD_SPEC)
 		return -EINVAL;
 
-	*set = FWD_SPEC;
+	fwd->mode = FWD_SPEC;
 
 	strncpy(buf, optarg, sizeof(buf) - 1);
 
@@ -265,7 +247,7 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 			if (bitmap_isset(exclude, i))
 				continue;
 
-			bitmap_set(map, i);
+			bitmap_set(fwd->map, i);
 
 			if (optname == 't')
 				tcp_sock_init(c, 0, af, addr, i);
@@ -355,18 +337,18 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 				goto bad;		/* 22-81:8022:8080 */
 
 			for (i = start_src; i <= end_src; i++) {
-				if (bitmap_isset(map, i))
+				if (bitmap_isset(fwd->map, i))
 					goto overlap;
 
 				if (bitmap_isset(exclude, i))
 					continue;
 
-				bitmap_set(map, i);
+				bitmap_set(fwd->map, i);
 
 				if (start_dst != -1) {
 					/* 80:8080 or 22-80:8080:8080 */
-					delta[i] = (in_port_t)(start_dst -
-							       start_src);
+					fwd->delta[i] = (in_port_t)(start_dst -
+								    start_src);
 				}
 
 				if (optname == 't')
@@ -1548,7 +1530,7 @@ void conf(struct ctx *c, int argc, char **argv)
 	/* Now we can process port configuration options */
 	optind = 1;
 	do {
-		enum port_fwd_mode *fwd = NULL;
+		struct port_fwd *fwd = NULL;
 		const char *optstring;
 
 		if (c->mode == MODE_PASST)
@@ -1563,13 +1545,13 @@ void conf(struct ctx *c, int argc, char **argv)
 		case 'T':
 		case 'U':
 			if (name == 't')
-				fwd = &c->tcp.fwd_in.mode;
+				fwd = &c->tcp.fwd_in;
 			else if (name == 'T')
-				fwd = &c->tcp.fwd_out.mode;
+				fwd = &c->tcp.fwd_out;
 			else if (name == 'u')
-				fwd = &c->udp.fwd_in.f.mode;
+				fwd = &c->udp.fwd_in.f;
 			else if (name == 'U')
-				fwd = &c->udp.fwd_out.f.mode;
+				fwd = &c->udp.fwd_out.f;
 
 			if (!optarg || conf_ports(c, name, optarg, fwd))
 				usage(argv[0]);
-- 
@@ -111,58 +111,40 @@ static int get_bound_ports_ns(void *arg)
  * @c:		Execution context
  * @optname:	Short option name, t, T, u, or U
  * @optarg:	Option argument (port specification)
- * @set:	Pointer to @port_fwd_mode to be set (port forwarding mode)
+ * @fwd:	Pointer to @port_fwd to be updated
  *
  * Return: -EINVAL on parsing error, 0 otherwise
  */
-static int conf_ports(struct ctx *c, char optname, const char *optarg,
-		      enum port_fwd_mode *set)
+static int conf_ports(const struct ctx *c, char optname, const char *optarg,
+		      struct port_fwd *fwd)
 {
 	int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port;
 	char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
 	char buf[BUFSIZ], *sep, *spec, *p;
 	port_fwd_map exclude = { 0 };
 	sa_family_t af = AF_UNSPEC;
-	in_port_t *delta;
-	uint8_t *map;
-
-	if (optname == 't') {
-		map = c->tcp.fwd_in.map;
-		delta = c->tcp.fwd_in.delta;
-	} else if (optname == 'T') {
-		map = c->tcp.fwd_out.map;
-		delta = c->tcp.fwd_out.delta;
-	} else if (optname == 'u') {
-		map = c->udp.fwd_in.f.map;
-		delta = c->udp.fwd_in.f.delta;
-	} else if (optname == 'U') {
-		map = c->udp.fwd_out.f.map;
-		delta = c->udp.fwd_out.f.delta;
-	} else {	/* For gcc -O3 */
-		return 0;
-	}
 
 	if (!strcmp(optarg, "none")) {
-		if (*set)
+		if (fwd->mode)
 			return -EINVAL;
-		*set = FWD_NONE;
+		fwd->mode = FWD_NONE;
 		return 0;
 	}
 
 	if (!strcmp(optarg, "auto")) {
-		if (*set || c->mode != MODE_PASTA)
+		if (fwd->mode || c->mode != MODE_PASTA)
 			return -EINVAL;
-		*set = FWD_AUTO;
+		fwd->mode = FWD_AUTO;
 		return 0;
 	}
 
 	if (!strcmp(optarg, "all")) {
 		int i;
 
-		if (*set || c->mode != MODE_PASST)
+		if (fwd->mode || c->mode != MODE_PASST)
 			return -EINVAL;
-		*set = FWD_ALL;
-		memset(map, 0xff, PORT_EPHEMERAL_MIN / 8);
+		fwd->mode = FWD_ALL;
+		memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8);
 
 		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
 			if (optname == 't')
@@ -174,10 +156,10 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 		return 0;
 	}
 
-	if (*set > FWD_SPEC)
+	if (fwd->mode > FWD_SPEC)
 		return -EINVAL;
 
-	*set = FWD_SPEC;
+	fwd->mode = FWD_SPEC;
 
 	strncpy(buf, optarg, sizeof(buf) - 1);
 
@@ -265,7 +247,7 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 			if (bitmap_isset(exclude, i))
 				continue;
 
-			bitmap_set(map, i);
+			bitmap_set(fwd->map, i);
 
 			if (optname == 't')
 				tcp_sock_init(c, 0, af, addr, i);
@@ -355,18 +337,18 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
 				goto bad;		/* 22-81:8022:8080 */
 
 			for (i = start_src; i <= end_src; i++) {
-				if (bitmap_isset(map, i))
+				if (bitmap_isset(fwd->map, i))
 					goto overlap;
 
 				if (bitmap_isset(exclude, i))
 					continue;
 
-				bitmap_set(map, i);
+				bitmap_set(fwd->map, i);
 
 				if (start_dst != -1) {
 					/* 80:8080 or 22-80:8080:8080 */
-					delta[i] = (in_port_t)(start_dst -
-							       start_src);
+					fwd->delta[i] = (in_port_t)(start_dst -
+								    start_src);
 				}
 
 				if (optname == 't')
@@ -1548,7 +1530,7 @@ void conf(struct ctx *c, int argc, char **argv)
 	/* Now we can process port configuration options */
 	optind = 1;
 	do {
-		enum port_fwd_mode *fwd = NULL;
+		struct port_fwd *fwd = NULL;
 		const char *optstring;
 
 		if (c->mode == MODE_PASST)
@@ -1563,13 +1545,13 @@ void conf(struct ctx *c, int argc, char **argv)
 		case 'T':
 		case 'U':
 			if (name == 't')
-				fwd = &c->tcp.fwd_in.mode;
+				fwd = &c->tcp.fwd_in;
 			else if (name == 'T')
-				fwd = &c->tcp.fwd_out.mode;
+				fwd = &c->tcp.fwd_out;
 			else if (name == 'u')
-				fwd = &c->udp.fwd_in.f.mode;
+				fwd = &c->udp.fwd_in.f;
 			else if (name == 'U')
-				fwd = &c->udp.fwd_out.f.mode;
+				fwd = &c->udp.fwd_out.f;
 
 			if (!optarg || conf_ports(c, name, optarg, fwd))
 				usage(argv[0]);
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH 6/8] Treat port numbers as unsigned
  2022-09-23  4:57 [PATCH 0/8] Clean up and fix bugs in port forwarding data structures David Gibson
                   ` (4 preceding siblings ...)
  2022-09-23  4:58 ` [PATCH 5/8] Pass entire port forwarding configuration substructure to conf_ports() David Gibson
@ 2022-09-23  4:58 ` David Gibson
  2022-09-23  4:58 ` [PATCH 7/8] Fix widespread off-by-one error dealing with port numbers David Gibson
  2022-09-23  4:58 ` [PATCH 8/8] icmp: Correct off by one errors dealing with number of echo request ids David Gibson
  7 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2022-09-23  4:58 UTC (permalink / raw)
  To: passt-dev

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

Port numbers are unsigned values, but we're storing them in (signed) int
variables in some places.  This isn't actually harmful, because int is
large enough to hold the entire range of ports.  However in places we don't
want to use an in_port_t (usually to avoid overflow on the last iteration
of a loop) it makes more conceptual sense to use an unsigned int. This will
also avoid some problems with later cleanups.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c | 11 ++++++-----
 tcp.c  |  4 ++--
 udp.c  |  2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/conf.c b/conf.c
index 2cbc519..503d894 100644
--- a/conf.c
+++ b/conf.c
@@ -118,11 +118,12 @@ static int get_bound_ports_ns(void *arg)
 static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 		      struct port_fwd *fwd)
 {
-	int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port;
 	char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
+	int start_src, end_src, start_dst, end_dst, exclude_only = 1, i;
 	char buf[BUFSIZ], *sep, *spec, *p;
 	port_fwd_map exclude = { 0 };
 	sa_family_t af = AF_UNSPEC;
+	unsigned port;
 
 	if (!strcmp(optarg, "none")) {
 		if (fwd->mode)
@@ -204,11 +205,11 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 			p++;
 
 		errno = 0;
-		port = strtol(p, &sep, 10);
+		port = strtoul(p, &sep, 10);
 		if (sep == p)
 			break;
 
-		if (port < 0 || port > USHRT_MAX || errno)
+		if (port > USHRT_MAX || errno)
 			goto bad;
 
 		switch (*sep) {
@@ -271,11 +272,11 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 			break;
 
 		errno = 0;
-		port = strtol(p, &sep, 10);
+		port = strtoul(p, &sep, 10);
 		if (sep == p)
 			break;
 
-		if (port < 0 || port > USHRT_MAX || errno)
+		if (port > USHRT_MAX || errno)
 			goto bad;
 
 		/* -p 22
diff --git a/tcp.c b/tcp.c
index 509a0b3..d96232c 100644
--- a/tcp.c
+++ b/tcp.c
@@ -3182,7 +3182,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 static int tcp_sock_init_ns(void *arg)
 {
 	struct ctx *c = (struct ctx *)arg;
-	int port;
+	unsigned port;
 
 	ns_enter(c);
 
@@ -3381,7 +3381,7 @@ struct tcp_port_rebind_arg {
 static int tcp_port_rebind(void *arg)
 {
 	struct tcp_port_rebind_arg *a = (struct tcp_port_rebind_arg *)arg;
-	int port;
+	unsigned port;
 
 	if (a->bind_in_ns) {
 		ns_enter(a->c);
diff --git a/udp.c b/udp.c
index d17b3b4..27c3aa3 100644
--- a/udp.c
+++ b/udp.c
@@ -1193,7 +1193,7 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 int udp_sock_init_ns(void *arg)
 {
 	struct ctx *c = (struct ctx *)arg;
-	int dst;
+	unsigned dst;
 
 	if (ns_enter(c))
 		return 0;
-- 
@@ -1193,7 +1193,7 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 int udp_sock_init_ns(void *arg)
 {
 	struct ctx *c = (struct ctx *)arg;
-	int dst;
+	unsigned dst;
 
 	if (ns_enter(c))
 		return 0;
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH 7/8] Fix widespread off-by-one error dealing with port numbers
  2022-09-23  4:57 [PATCH 0/8] Clean up and fix bugs in port forwarding data structures David Gibson
                   ` (5 preceding siblings ...)
  2022-09-23  4:58 ` [PATCH 6/8] Treat port numbers as unsigned David Gibson
@ 2022-09-23  4:58 ` David Gibson
  2022-09-23  4:58 ` [PATCH 8/8] icmp: Correct off by one errors dealing with number of echo request ids David Gibson
  7 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2022-09-23  4:58 UTC (permalink / raw)
  To: passt-dev

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

Port numbers (for both TCP and UDP) are 16-bit, and so fit exactly into a
'short'.  USHRT_MAX is therefore the maximum port number and this is widely
used in the code.  Unfortunately, a lot of those places don't actually
want the maximum port number (USHRT_MAX == 65535), they want the total
number of ports (65536).  This leads to a number of potentially nasty
consequences:

 * We have buffer overruns on the port_fwd::delta array if we try to use
   port 65535
 * We have similar potential overruns for the tcp_sock_* arrays
 * Interestingly udp_act had the correct size, but we can calculate it in
   a more direct manner
 * We have a logical overrun of the ports bitmap as well, although it will
   just use an unused bit in the last byte so isnt harmful
 * Many loops don't consider port 65535 (which does mitigate some but not
   all of the buffer overruns above)
 * In udp_invert_portmap() we incorrectly compute the reverse port
   translation for return packets

Correct all these by using a new NUM_PORTS defined explicitly for this
purpose.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c     |  4 ++--
 port_fwd.h |  7 +++++--
 tcp.c      | 12 ++++++------
 udp.c      | 10 +++++-----
 udp.h      |  2 +-
 5 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/conf.c b/conf.c
index 503d894..5cdfe31 100644
--- a/conf.c
+++ b/conf.c
@@ -209,7 +209,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 		if (sep == p)
 			break;
 
-		if (port > USHRT_MAX || errno)
+		if (port >= NUM_PORTS || errno)
 			goto bad;
 
 		switch (*sep) {
@@ -276,7 +276,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 		if (sep == p)
 			break;
 
-		if (port > USHRT_MAX || errno)
+		if (port >= NUM_PORTS || errno)
 			goto bad;
 
 		/* -p 22
diff --git a/port_fwd.h b/port_fwd.h
index 5507bf2..976138f 100644
--- a/port_fwd.h
+++ b/port_fwd.h
@@ -7,6 +7,9 @@
 #ifndef PORT_FWD_H
 #define PORT_FWD_H
 
+/* Number of ports for both TCP and UDP */
+#define	NUM_PORTS	(1U << 16)
+
 enum port_fwd_mode {
 	FWD_SPEC = 1,
 	FWD_NONE,
@@ -14,7 +17,7 @@ enum port_fwd_mode {
 	FWD_ALL,
 };
 
-typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];
+typedef uint8_t port_fwd_map[DIV_ROUND_UP(NUM_PORTS, 8)];
 
 /**
  * port_fwd - Describes port forwarding for one protocol and direction
@@ -25,7 +28,7 @@ typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];
 struct port_fwd {
 	enum port_fwd_mode mode;
 	port_fwd_map map;
-	in_port_t delta[USHRT_MAX];
+	in_port_t delta[NUM_PORTS];
 };
 
 #endif /* PORT_FWD_H */
diff --git a/tcp.c b/tcp.c
index d96232c..e45dfda 100644
--- a/tcp.c
+++ b/tcp.c
@@ -547,9 +547,9 @@ static const char *tcp_flag_str[] __attribute((__unused__)) = {
 };
 
 /* Listening sockets, used for automatic port forwarding in pasta mode only */
-static int tcp_sock_init_lo	[USHRT_MAX][IP_VERSIONS];
-static int tcp_sock_init_ext	[USHRT_MAX][IP_VERSIONS];
-static int tcp_sock_ns		[USHRT_MAX][IP_VERSIONS];
+static int tcp_sock_init_lo	[NUM_PORTS][IP_VERSIONS];
+static int tcp_sock_init_ext	[NUM_PORTS][IP_VERSIONS];
+static int tcp_sock_ns		[NUM_PORTS][IP_VERSIONS];
 
 /* Table of destinations with very low RTT (assumed to be local), LRU */
 static struct in6_addr low_rtt_dst[LOW_RTT_TABLE_SIZE];
@@ -3186,7 +3186,7 @@ static int tcp_sock_init_ns(void *arg)
 
 	ns_enter(c);
 
-	for (port = 0; port < USHRT_MAX; port++) {
+	for (port = 0; port < NUM_PORTS; port++) {
 		if (!bitmap_isset(c->tcp.fwd_out.map, port))
 			continue;
 
@@ -3386,7 +3386,7 @@ static int tcp_port_rebind(void *arg)
 	if (a->bind_in_ns) {
 		ns_enter(a->c);
 
-		for (port = 0; port < USHRT_MAX; port++) {
+		for (port = 0; port < NUM_PORTS; port++) {
 			if (!bitmap_isset(a->c->tcp.fwd_out.map, port)) {
 				if (tcp_sock_ns[port][V4] >= 0) {
 					close(tcp_sock_ns[port][V4]);
@@ -3410,7 +3410,7 @@ static int tcp_port_rebind(void *arg)
 				tcp_sock_init(a->c, 1, AF_UNSPEC, NULL, port);
 		}
 	} else {
-		for (port = 0; port < USHRT_MAX; port++) {
+		for (port = 0; port < NUM_PORTS; port++) {
 			if (!bitmap_isset(a->c->tcp.fwd_in.map, port)) {
 				if (tcp_sock_init_ext[port][V4] >= 0) {
 					close(tcp_sock_init_ext[port][V4]);
diff --git a/udp.c b/udp.c
index 27c3aa3..bd3dc72 100644
--- a/udp.c
+++ b/udp.c
@@ -164,8 +164,8 @@ struct udp_splice_port {
 };
 
 /* Port tracking, arrays indexed by packet source port (host order) */
-static struct udp_tap_port	udp_tap_map	[IP_VERSIONS][USHRT_MAX];
-static struct udp_splice_port	udp_splice_map	[IP_VERSIONS][USHRT_MAX];
+static struct udp_tap_port	udp_tap_map	[IP_VERSIONS][NUM_PORTS];
+static struct udp_splice_port	udp_splice_map	[IP_VERSIONS][NUM_PORTS];
 
 enum udp_act_type {
 	UDP_ACT_TAP,
@@ -175,7 +175,7 @@ enum udp_act_type {
 };
 
 /* Activity-based aging for bindings */
-static uint8_t udp_act[IP_VERSIONS][UDP_ACT_TYPE_MAX][(USHRT_MAX + 1) / 8];
+static uint8_t udp_act[IP_VERSIONS][UDP_ACT_TYPE_MAX][DIV_ROUND_UP(NUM_PORTS, 8)];
 
 /* Static buffers */
 
@@ -271,7 +271,7 @@ static void udp_invert_portmap(struct udp_port_fwd *fwd)
 		in_port_t delta = fwd->f.delta[i];
 
 		if (delta)
-			fwd->rdelta[(in_port_t)i + delta] = USHRT_MAX - delta;
+			fwd->rdelta[(in_port_t)i + delta] = NUM_PORTS - delta;
 	}
 }
 
@@ -1198,7 +1198,7 @@ int udp_sock_init_ns(void *arg)
 	if (ns_enter(c))
 		return 0;
 
-	for (dst = 0; dst < USHRT_MAX; dst++) {
+	for (dst = 0; dst < NUM_PORTS; dst++) {
 		if (!bitmap_isset(c->udp.fwd_out.f.map, dst))
 			continue;
 
diff --git a/udp.h b/udp.h
index bc7b259..d14df0a 100644
--- a/udp.h
+++ b/udp.h
@@ -50,7 +50,7 @@ union udp_epoll_ref {
  */
 struct udp_port_fwd {
 	struct port_fwd f;
-	in_port_t rdelta[USHRT_MAX];
+	in_port_t rdelta[NUM_PORTS];
 };
 
 /**
-- 
@@ -50,7 +50,7 @@ union udp_epoll_ref {
  */
 struct udp_port_fwd {
 	struct port_fwd f;
-	in_port_t rdelta[USHRT_MAX];
+	in_port_t rdelta[NUM_PORTS];
 };
 
 /**
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH 8/8] icmp: Correct off by one errors dealing with number of echo request ids
  2022-09-23  4:57 [PATCH 0/8] Clean up and fix bugs in port forwarding data structures David Gibson
                   ` (6 preceding siblings ...)
  2022-09-23  4:58 ` [PATCH 7/8] Fix widespread off-by-one error dealing with port numbers David Gibson
@ 2022-09-23  4:58 ` David Gibson
  7 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2022-09-23  4:58 UTC (permalink / raw)
  To: passt-dev

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

ICMP echo request and reply packets include a 16-bit 'id' value.  We have
some arrays indexed by this id value.  Unfortunately we size those arrays
with USHRT_MAX (65535) when they need to be sized by the total number of
id values (65536).  This could lead to buffer overruns.  Resize the arrays
correctly, using a new define for the purpose.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 icmp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/icmp.c b/icmp.c
index 2da8b58..39a8694 100644
--- a/icmp.c
+++ b/icmp.c
@@ -39,6 +39,7 @@
 #include "icmp.h"
 
 #define ICMP_ECHO_TIMEOUT	60 /* s, timeout for ICMP socket activity */
+#define ICMP_NUM_IDS		(1U << 16)
 
 /**
  * struct icmp_id_sock - Tracking information for single ICMP echo identifier
@@ -53,10 +54,10 @@ struct icmp_id_sock {
 };
 
 /* Indexed by ICMP echo identifier */
-static struct icmp_id_sock icmp_id_map	[IP_VERSIONS][USHRT_MAX];
+static struct icmp_id_sock icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
 
 /* Bitmaps, activity monitoring needed for identifier */
-static uint8_t icmp_act			[IP_VERSIONS][USHRT_MAX / 8];
+static uint8_t icmp_act[IP_VERSIONS][DIV_ROUND_UP(ICMP_NUM_IDS, 8)];
 
 /**
  * icmp_sock_handler() - Handle new data from socket
-- 
@@ -39,6 +39,7 @@
 #include "icmp.h"
 
 #define ICMP_ECHO_TIMEOUT	60 /* s, timeout for ICMP socket activity */
+#define ICMP_NUM_IDS		(1U << 16)
 
 /**
  * struct icmp_id_sock - Tracking information for single ICMP echo identifier
@@ -53,10 +54,10 @@ struct icmp_id_sock {
 };
 
 /* Indexed by ICMP echo identifier */
-static struct icmp_id_sock icmp_id_map	[IP_VERSIONS][USHRT_MAX];
+static struct icmp_id_sock icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
 
 /* Bitmaps, activity monitoring needed for identifier */
-static uint8_t icmp_act			[IP_VERSIONS][USHRT_MAX / 8];
+static uint8_t icmp_act[IP_VERSIONS][DIV_ROUND_UP(ICMP_NUM_IDS, 8)];
 
 /**
  * icmp_sock_handler() - Handle new data from socket
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH 1/8] Improve types and names for port forwarding configuration
  2022-09-23  4:57 ` [PATCH 1/8] Improve types and names for port forwarding configuration David Gibson
@ 2022-09-23 22:52   ` Stefano Brivio
  2022-09-24  2:57     ` David Gibson
  0 siblings, 1 reply; 13+ messages in thread
From: Stefano Brivio @ 2022-09-23 22:52 UTC (permalink / raw)
  To: passt-dev

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

On Fri, 23 Sep 2022 14:57:59 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> [...]
> 
> --- /dev/null
> +++ b/port_fwd.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: AGPL-3.0-or-later
> + * Copyright Red Hat
> + * Author: Stefano Brivio <sbrivio(a)redhat.com>
> + * Author: David Gibson <david(a)gibson.dropbear.id.au>
> + */
> +
> +#ifndef PORT_FWD_H
> +#define PORT_FWD_H
> +
> +enum port_fwd_mode {
> +	FWD_SPEC = 1,
> +	FWD_NONE,
> +	FWD_AUTO,
> +	FWD_ALL,
> +};
> +
> +typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];

Given that this gets conveniently embedded in a struct in 2/8, could we
avoid the typedef (or perhaps drop it after 2/8)? It makes the actual
type less obvious to figure out, and in general I agree with most
points from this slide deck:

  http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/mgp00025.html

:) ...well, unless there's some resulting complexity I'm missing.

I reviewed the rest of the series, it all makes sense to me, thanks.

-- 
Stefano


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 1/8] Improve types and names for port forwarding configuration
  2022-09-23 22:52   ` Stefano Brivio
@ 2022-09-24  2:57     ` David Gibson
  2022-09-24  6:28       ` Stefano Brivio
  0 siblings, 1 reply; 13+ messages in thread
From: David Gibson @ 2022-09-24  2:57 UTC (permalink / raw)
  To: passt-dev

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

On Sat, Sep 24, 2022 at 12:52:57AM +0200, Stefano Brivio wrote:
> On Fri, 23 Sep 2022 14:57:59 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > [...]
> > 
> > --- /dev/null
> > +++ b/port_fwd.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: AGPL-3.0-or-later
> > + * Copyright Red Hat
> > + * Author: Stefano Brivio <sbrivio(a)redhat.com>
> > + * Author: David Gibson <david(a)gibson.dropbear.id.au>
> > + */
> > +
> > +#ifndef PORT_FWD_H
> > +#define PORT_FWD_H
> > +
> > +enum port_fwd_mode {
> > +	FWD_SPEC = 1,
> > +	FWD_NONE,
> > +	FWD_AUTO,
> > +	FWD_ALL,
> > +};
> > +
> > +typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];
> 
> Given that this gets conveniently embedded in a struct in 2/8, could we
> avoid the typedef (or perhaps drop it after 2/8)? It makes the actual
> type less obvious to figure out, and in general I agree with most
> points from this slide deck:
> 
>   http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/mgp00025.html

As do I, in fact especially for array types, due to their
idiosyncratic handling in C.

> :) ...well, unless there's some resulting complexity I'm missing.

Well, maybe.  I added the typedef because of two things: 1) the fact
that in one place we use a bitmap of this format separately from the
rest for the 'exclude' map in conf_ports().  2) the fact that we need
the size of this map in get_bound_ports().  Having the typedef lets us
handle both without duplicating the calculation of the size, which
would mean more opportunities to get it wrong.

I feel the advantages outweigh the disadvantages of a typedef in this
case, but I won't be offended if you disagree.  We could use a #define
or const of the bitmap size instead.

> I reviewed the rest of the series, it all makes sense to me, thanks.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 1/8] Improve types and names for port forwarding configuration
  2022-09-24  2:57     ` David Gibson
@ 2022-09-24  6:28       ` Stefano Brivio
  2022-09-24  9:06         ` David Gibson
  0 siblings, 1 reply; 13+ messages in thread
From: Stefano Brivio @ 2022-09-24  6:28 UTC (permalink / raw)
  To: passt-dev

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

On Sat, 24 Sep 2022 12:57:25 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> On Sat, Sep 24, 2022 at 12:52:57AM +0200, Stefano Brivio wrote:
> > On Fri, 23 Sep 2022 14:57:59 +1000
> > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> >   
> > > [...]
> > > 
> > > --- /dev/null
> > > +++ b/port_fwd.h
> > > @@ -0,0 +1,19 @@
> > > +/* SPDX-License-Identifier: AGPL-3.0-or-later
> > > + * Copyright Red Hat
> > > + * Author: Stefano Brivio <sbrivio(a)redhat.com>
> > > + * Author: David Gibson <david(a)gibson.dropbear.id.au>
> > > + */
> > > +
> > > +#ifndef PORT_FWD_H
> > > +#define PORT_FWD_H
> > > +
> > > +enum port_fwd_mode {
> > > +	FWD_SPEC = 1,
> > > +	FWD_NONE,
> > > +	FWD_AUTO,
> > > +	FWD_ALL,
> > > +};
> > > +
> > > +typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];  
> > 
> > Given that this gets conveniently embedded in a struct in 2/8, could we
> > avoid the typedef (or perhaps drop it after 2/8)? It makes the actual
> > type less obvious to figure out, and in general I agree with most
> > points from this slide deck:
> > 
> >   http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/mgp00025.html  
> 
> As do I, in fact especially for array types, due to their
> idiosyncratic handling in C.
> 
> > :) ...well, unless there's some resulting complexity I'm missing.  
> 
> Well, maybe.  I added the typedef because of two things: 1) the fact
> that in one place we use a bitmap of this format separately from the
> rest for the 'exclude' map in conf_ports().  2) the fact that we need
> the size of this map in get_bound_ports().  Having the typedef lets us
> handle both without duplicating the calculation of the size, which
> would mean more opportunities to get it wrong.

I see, but at the same time:

- forgetting to use the new type should be as easy as forgetting to use
  a define representing the size

- the risk of doing stupid things (such as trying to pass this by
  value), despite the clear name of the typedef, looks to me slightly
  bigger than with "uint8_t ports[SIZE_PORTS]"

> I feel the advantages outweigh the disadvantages of a typedef in this
> case, but I won't be offended if you disagree.  We could use a #define
> or const of the bitmap size instead.

Well, I see now, and I don't have such a strong preference anymore...
even though I would still prefer the define (perhaps SIZE_PORTS, based
on NUM_PORTS from 7/8?).

Also introducing the first typedef in the whole project for something
we can implement almost equivalently, hmm.

I don't know, if you still think it's clearly better than the
alternative, let's go with it, I just have a slight preference against
it at this point.

-- 
Stefano


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 1/8] Improve types and names for port forwarding configuration
  2022-09-24  6:28       ` Stefano Brivio
@ 2022-09-24  9:06         ` David Gibson
  0 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2022-09-24  9:06 UTC (permalink / raw)
  To: passt-dev

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

On Sat, Sep 24, 2022 at 08:28:32AM +0200, Stefano Brivio wrote:
> On Sat, 24 Sep 2022 12:57:25 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > On Sat, Sep 24, 2022 at 12:52:57AM +0200, Stefano Brivio wrote:
> > > On Fri, 23 Sep 2022 14:57:59 +1000
> > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> > >   
> > > > [...]
> > > > 
> > > > --- /dev/null
> > > > +++ b/port_fwd.h
> > > > @@ -0,0 +1,19 @@
> > > > +/* SPDX-License-Identifier: AGPL-3.0-or-later
> > > > + * Copyright Red Hat
> > > > + * Author: Stefano Brivio <sbrivio(a)redhat.com>
> > > > + * Author: David Gibson <david(a)gibson.dropbear.id.au>
> > > > + */
> > > > +
> > > > +#ifndef PORT_FWD_H
> > > > +#define PORT_FWD_H
> > > > +
> > > > +enum port_fwd_mode {
> > > > +	FWD_SPEC = 1,
> > > > +	FWD_NONE,
> > > > +	FWD_AUTO,
> > > > +	FWD_ALL,
> > > > +};
> > > > +
> > > > +typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];  
> > > 
> > > Given that this gets conveniently embedded in a struct in 2/8, could we
> > > avoid the typedef (or perhaps drop it after 2/8)? It makes the actual
> > > type less obvious to figure out, and in general I agree with most
> > > points from this slide deck:
> > > 
> > >   http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/mgp00025.html  
> > 
> > As do I, in fact especially for array types, due to their
> > idiosyncratic handling in C.
> > 
> > > :) ...well, unless there's some resulting complexity I'm missing.  
> > 
> > Well, maybe.  I added the typedef because of two things: 1) the fact
> > that in one place we use a bitmap of this format separately from the
> > rest for the 'exclude' map in conf_ports().  2) the fact that we need
> > the size of this map in get_bound_ports().  Having the typedef lets us
> > handle both without duplicating the calculation of the size, which
> > would mean more opportunities to get it wrong.
> 
> I see, but at the same time:
> 
> - forgetting to use the new type should be as easy as forgetting to use
>   a define representing the size
> 
> - the risk of doing stupid things (such as trying to pass this by
>   value), despite the clear name of the typedef, looks to me slightly
>   bigger than with "uint8_t ports[SIZE_PORTS]"
> 
> > I feel the advantages outweigh the disadvantages of a typedef in this
> > case, but I won't be offended if you disagree.  We could use a #define
> > or const of the bitmap size instead.
> 
> Well, I see now, and I don't have such a strong preference anymore...
> even though I would still prefer the define (perhaps SIZE_PORTS, based
> on NUM_PORTS from 7/8?).
> 
> Also introducing the first typedef in the whole project for something
> we can implement almost equivalently, hmm.
> 
> I don't know, if you still think it's clearly better than the
> alternative, let's go with it, I just have a slight preference against
> it at this point.

I think you convinced me.  TBH, I only thought of the size define when
I replied here, not when I originally wrote it.  I'll respin with that
instead of the typedef.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-09-24  9:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-23  4:57 [PATCH 0/8] Clean up and fix bugs in port forwarding data structures David Gibson
2022-09-23  4:57 ` [PATCH 1/8] Improve types and names for port forwarding configuration David Gibson
2022-09-23 22:52   ` Stefano Brivio
2022-09-24  2:57     ` David Gibson
2022-09-24  6:28       ` Stefano Brivio
2022-09-24  9:06         ` David Gibson
2022-09-23  4:58 ` [PATCH 2/8] Consolidate port forwarding configuration into a common structure David Gibson
2022-09-23  4:58 ` [PATCH 3/8] udp: Delay initialization of UDP reversed port mapping table David Gibson
2022-09-23  4:58 ` [PATCH 4/8] Don't use indirect remap functions for conf_ports() David Gibson
2022-09-23  4:58 ` [PATCH 5/8] Pass entire port forwarding configuration substructure to conf_ports() David Gibson
2022-09-23  4:58 ` [PATCH 6/8] Treat port numbers as unsigned David Gibson
2022-09-23  4:58 ` [PATCH 7/8] Fix widespread off-by-one error dealing with port numbers David Gibson
2022-09-23  4:58 ` [PATCH 8/8] icmp: Correct off by one errors dealing with number of echo request ids David Gibson

Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).