[PATCH v3 00/11] Rework forwarding option parsing

[PATCH v3 00/11] Rework forwarding option parsing

[David Gibson]

This series makes a number of significant reworks to how we process
forwarding options (-t, -u, -T and -U) in passt & pasta.  This is
largely motivated by moving towards being able to share this code with
a configuration update tool.  However, along the way it also enables
some forwarding configurations that were technically possible with the
forwarding table but couldn't be specified on the command line, in
particular bug 180.

There is still a bunch of work needed to make the parsing code truly
shareable with pesto, but this is a solid start.

v3:
 * Removed already merged patches
 * Further revisions based on Stefano's review, including
   * Improved example text in manpage and usage
 * More flexible rule dumping
 * Re-integrated conflict checking in fwd_rule_add()
v2:
 * Assorted minor changes based on Stefano's review, including
   * Explicitly state "guest or namespace" in the manpage
   * Clearer description of @rulesocks field
 * Worked around a gcc < 15 bug causing a false positive warning
 * Update man page and usage() for new capabilities
 * Additional patches moving rule parsing out of conf.c

David Gibson (11):
  doc: Rework man page description of port specifiers
  conf: Move "all" handling to port specifier
  conf: Allow user-specified auto-scanned port forwarding ranges
  conf: Move SO_BINDTODEVICE workaround to conf_ports()
  conf: Don't pass raw commandline argument to conf_ports_spec()
  fwd, conf: Add capabilities bits to each forwarding table
  conf, fwd: Stricter rule checking in fwd_rule_add()
  fwd_rule: Move ephemeral port probing to fwd_rule.c
  fwd, conf: Move rule parsing code to fwd_rule.[ch]
  fwd_rule: Move conflict checking back within fwd_rule_add()
  fwd: Generalise fwd_rules_info()

 conf.c     | 434 +++--------------------------------------
 fwd.c      | 142 ++------------
 fwd.h      |  37 ----
 fwd_rule.c | 563 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 fwd_rule.h |  68 ++++++-
 passt.1    |  72 +++++--
 6 files changed, 704 insertions(+), 612 deletions(-)

-- 
2.53.0

[PATCH v3 01/11] doc: Rework man page description of port specifiers

[David Gibson]

Currently the man page describes the internal syntax of port specifiers
in prose, which isn't particularly easy to follow.  Rework it to use
more syntax "diagrams" to show how it works.  This will also allow us to
more easily update the manual page for some coming changes in syntax.

usage() output is updated similarly, though more briefly.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c  | 10 +++++-----
 passt.1 | 32 ++++++++++++++++++++++----------
 2 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/conf.c b/conf.c
index cea124a5..6b5d2bd1 100644
--- a/conf.c
+++ b/conf.c
@@ -1041,11 +1041,11 @@ static void usage(const char *name, FILE *f, int status)
 		"      'none': don't forward any ports\n"
 		"      'all': forward all unbound, non-ephemeral ports\n"
 		"%s"
-		"      a comma-separated list, optionally ranged with '-'\n"
-		"        and optional target ports after ':', with optional\n"
-		"        address specification suffixed by '/' and optional\n"
-		"        interface prefixed by '%%'. Ranges can be reduced by\n"
-		"        excluding ports or ranges prefixed by '~'\n"
+		"      [ADDR[%%IFACE]/]PORTS: forward specific ports\n"
+		"        PORTS is a comma-separated list of ports, optionally\n"
+		"        ranged with '-' and optional target ports after ':'.\n"
+		"        Ranges can be reduced by excluding ports or ranges\n"
+		"        prefixed by '~'\n"
 		"        Examples:\n"
 		"        -t 22		Forward local port 22 to 22 on %s\n"
 		"        -t 22:23	Forward local port 22 to 23 on %s\n"
diff --git a/passt.1 b/passt.1
index 7da4fe5f..c47452ce 100644
--- a/passt.1
+++ b/passt.1
@@ -447,16 +447,28 @@ periodically derived (every second) from listening sockets reported by
 \fI/proc/net/tcp\fR and \fI/proc/net/tcp6\fR, see \fBproc\fR(5).
 
 .TP
-.BR ports
-A comma-separated list of ports, optionally ranged with \fI-\fR, and,
-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. In
-this case, no failures are reported for unavailable ports, unless no ports could
-be forwarded at all.
+[\fIaddress\fR[\fB%\fR\fIinterface\fR]\fB/\fR]\fIports\fR ...
+Specific ports to forward.  Optionally, a specific listening address
+and interface name (since Linux 5.7) can be specified.  \fIports\fR is
+a comma-separated list of entries which may be any of:
+.RS
+.TP
+\fIfirst\fR[\fB-\fR\fIlast\fR][\fB:\fR\fItofirst\fR[\fB-\fR\fItolast\fR]]
+Include range. Forward port numbers between \fIfirst\fR and \fIlast\fR
+(inclusive) to ports between \fItofirst\fR and \fItolast\fR.  If
+\fItofirst\fR and \fItolast\fR are omitted, assume the same as
+\fIfirst\fR and \fIlast\fR.  If \fIlast\fR is omitted, assume the same
+as \fIfirst\fR.
+
+.TP
+\fB~\fR\fIfirst\fR[\fB-\fR\fIlast\fR]
+Exclude range.  Don't forward port numbers between \fIfirst\fR and
+\fIlast\fR.  This takes precedences over include ranges.
+.RE
+
+Specifying excluded ranges only implies that all other non-ephemeral
+ports are forwarded. In this case, no failures are reported for
+unavailable ports, unless no ports could be forwarded at all.
 
 Examples:
 .RS
-- 
2.53.0

[PATCH v3 02/11] conf: Move "all" handling to port specifier

[David Gibson]

Currently -[tTuU] all is handled separately in conf_ports() before calling
conf_ports_spec().  Earlier changes mean we can now move this handling to
conf_ports_spec().  This makes the code slightly simpler, but more
importantly it allows some useful combinations we couldn't previously do,
such as
	-t 127.0.0.1/all
or
	-u %eth2/all

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c  | 24 +++++++++---------------
 passt.1 | 28 ++++++++++++++++++++--------
 2 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/conf.c b/conf.c
index 6b5d2bd1..dacea182 100644
--- a/conf.c
+++ b/conf.c
@@ -251,6 +251,11 @@ static void conf_ports_spec(const struct ctx *c,
 	const char *p, *ep;
 	unsigned i;
 
+	if (!strcmp(spec, "all")) {
+		/* Treat "all" as equivalent to "": all non-ephemeral ports */
+		spec = "";
+	}
+
 	/* Mark all exclusions first, they might be given after base ranges */
 	for_each_chunk(p, ep, spec, ",") {
 		struct port_range xrange;
@@ -372,19 +377,6 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 		return;
 	}
 
-	if (!strcmp(optarg, "all")) {
-		uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
-
-		/* Exclude ephemeral ports */
-		fwd_port_map_ephemeral(exclude);
-
-		conf_ports_range_except(c, optname, optarg, fwd,
-					proto, NULL, NULL,
-					1, NUM_PORTS - 1, exclude,
-					1, FWD_WEAK);
-		return;
-	}
-
 	strncpy(buf, optarg, sizeof(buf) - 1);
 
 	if ((spec = strchr(buf, '/'))) {
@@ -1039,14 +1031,16 @@ static void usage(const char *name, FILE *f, int status)
 		"    can be specified multiple times\n"
 		"    SPEC can be:\n"
 		"      'none': don't forward any ports\n"
-		"      'all': forward all unbound, non-ephemeral ports\n"
 		"%s"
 		"      [ADDR[%%IFACE]/]PORTS: forward specific ports\n"
-		"        PORTS is a comma-separated list of ports, optionally\n"
+		"        PORTS is either 'all' (forward all unbound, non-ephemeral\n"
+		"        ports), or a comma-separated list of ports, optionally\n"
 		"        ranged with '-' and optional target ports after ':'.\n"
 		"        Ranges can be reduced by excluding ports or ranges\n"
 		"        prefixed by '~'\n"
 		"        Examples:\n"
+		"        -t all		Forward all ports\n"
+		"        -t ::1/all	Forward all ports from local address ::1\n"
 		"        -t 22		Forward local port 22 to 22 on %s\n"
 		"        -t 22:23	Forward local port 22 to 23 on %s\n"
 		"        -t 22,25	Forward ports 22, 25 to ports 22, 25\n"
diff --git a/passt.1 b/passt.1
index c47452ce..20dc72ca 100644
--- a/passt.1
+++ b/passt.1
@@ -434,12 +434,6 @@ Configure TCP port forwarding to guest or namespace. \fIspec\fR can be one of:
 .BR none
 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. No failures are reported for
-unavailable ports, unless no ports could be forwarded at all.
-
 .TP
 .BR auto " " (\fBpasta\fR " " only)
 Dynamically forward ports bound in the namespace. The list of ports is
@@ -449,10 +443,20 @@ periodically derived (every second) from listening sockets reported by
 .TP
 [\fIaddress\fR[\fB%\fR\fIinterface\fR]\fB/\fR]\fIports\fR ...
 Specific ports to forward.  Optionally, a specific listening address
-and interface name (since Linux 5.7) can be specified.  \fIports\fR is
-a comma-separated list of entries which may be any of:
+and interface name (since Linux 5.7) can be specified.  \fIports\fR
+may be either:
 .RS
 .TP
+\fBall\fR
+Forward all unbound, non-ephemeral ports, as permitted by current
+capabilities.  For low (< 1024) ports, see \fBNOTES\fR. No failures
+are reported for unavailable ports, unless no ports could be forwarded
+at all.
+.RE
+
+.RS
+or a comma-separated list of entries which may be any of:
+.TP
 \fIfirst\fR[\fB-\fR\fIlast\fR][\fB:\fR\fItofirst\fR[\fB-\fR\fItolast\fR]]
 Include range. Forward port numbers between \fIfirst\fR and \fIlast\fR
 (inclusive) to ports between \fItofirst\fR and \fItolast\fR.  If
@@ -473,6 +477,14 @@ unavailable ports, unless no ports could be forwarded at all.
 Examples:
 .RS
 .TP
+-t all
+Forward all unbound, non-ephemeral ports as permitted by current
+capabilities to the corresponding port on the guest or namespace
+.TP
+-t ::1/all
+For the local address ::1, forward all unbound, non-ephemeral ports as
+permitted by current capabilities
+.TP
 -t 22
 Forward local port 22 to port 22 on the guest or namespace
 .TP
-- 
2.53.0

[PATCH v3 03/11] conf: Allow user-specified auto-scanned port forwarding ranges

[David Gibson]

The forwarding table now allows for arbitrary port ranges to be marked as
FWD_SCAN, meaning we don't open sockets for every port, but only those we
scan as listening on the target side.  However, there's currently no way
to create such rules, except -[tTuU] auto which always scans every port
with an unspecified listening address and interface.

Allow user-specified "auto" ranges by moving the parsing of the "auto"
keyword from conf_ports(), to conf_ports_spec() as part of the port
specified.  "auto" can be combined freely with other port ranges, e.g.
    -t 127.0.0.1/auto
    -u %lo/5000-7000,auto
    -T auto,12345
    -U auto,~1-9000

Note that any address and interface given only affects where the automatic
forwards listen, not what addresses we consider when scanning.  That is,
if the target side is listening on *any* address, we will create a forward
on the specified address.

Link: https://bugs.passt.top/show_bug.cgi?id=180

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c  | 85 ++++++++++++++++++++++++++++++++++++++++++---------------
 passt.1 | 30 ++++++++++++++------
 2 files changed, 85 insertions(+), 30 deletions(-)

diff --git a/conf.c b/conf.c
index dacea182..33b96eac 100644
--- a/conf.c
+++ b/conf.c
@@ -13,6 +13,7 @@
  */
 
 #include <arpa/inet.h>
+#include <ctype.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <getopt.h>
@@ -112,6 +113,28 @@ static int parse_port_range(const char *s, const char **endptr,
 	return 0;
 }
 
+/**
+ * parse_keyword() - Parse a literal keyword
+ * @s:		String to parse
+ * @endptr:	Update to the character after the keyword
+ * @kw:		Keyword to accept
+ *
+ * Return: 0, if @s starts with @kw, -EINVAL if it does not
+ */
+static int parse_keyword(const char *s, const char **endptr, const char *kw)
+{
+	size_t len = strlen(kw);
+
+	if (strlen(s) < len)
+		return -EINVAL;
+
+	if (memcmp(s, kw, len))
+		return -EINVAL;
+
+	*endptr = s + len;
+	return 0;
+}
+
 /**
  * conf_ports_range_except() - Set up forwarding for a range of ports minus a
  *                             bitmap of exclusions
@@ -249,6 +272,7 @@ static void conf_ports_spec(const struct ctx *c,
 	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
 	bool exclude_only = true;
 	const char *p, *ep;
+	uint8_t flags = 0;
 	unsigned i;
 
 	if (!strcmp(spec, "all")) {
@@ -256,15 +280,32 @@ static void conf_ports_spec(const struct ctx *c,
 		spec = "";
 	}
 
-	/* Mark all exclusions first, they might be given after base ranges */
+	/* Parse excluded ranges and "auto" in the first pass */
 	for_each_chunk(p, ep, spec, ",") {
 		struct port_range xrange;
 
-		if (*p != '~') {
-			/* Not an exclude range, parse later */
+		if (isdigit(*p))  {
+			/* Include range, parse later */
 			exclude_only = false;
 			continue;
 		}
+
+		if (parse_keyword(p, &p, "auto") == 0) {
+			if (p != ep) /* Garbage after the keyword */
+				goto bad;
+
+			if (c->mode != MODE_PASTA) {
+				die(
+"'auto' port forwarding is only allowed for pasta");
+			}
+
+			flags |= FWD_SCAN;
+			continue;
+		}
+
+		/* Should be an exclude range */
+		if (*p != '~')
+			goto bad;
 		p++;
 
 		if (parse_port_range(p, &p, &xrange))
@@ -283,7 +324,7 @@ static void conf_ports_spec(const struct ctx *c,
 		conf_ports_range_except(c, optname, optarg, fwd,
 					proto, addr, ifname,
 					1, NUM_PORTS - 1, exclude,
-					1, FWD_WEAK);
+					1, flags | FWD_WEAK);
 		return;
 	}
 
@@ -291,8 +332,8 @@ static void conf_ports_spec(const struct ctx *c,
 	for_each_chunk(p, ep, spec, ",") {
 		struct port_range orig_range, mapped_range;
 
-		if (*p == '~')
-			/* Exclude range, already parsed */
+		if (!isdigit(*p))
+			/* Already parsed */
 			continue;
 
 		if (parse_port_range(p, &p, &orig_range))
@@ -320,7 +361,7 @@ static void conf_ports_spec(const struct ctx *c,
 					proto, addr, ifname,
 					orig_range.first, orig_range.last,
 					exclude,
-					mapped_range.first, 0);
+					mapped_range.first, flags);
 	}
 
 	return;
@@ -366,17 +407,6 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 	if (proto == IPPROTO_UDP && c->no_udp)
 		die("UDP port forwarding requested but UDP is disabled");
 
-	if (!strcmp(optarg, "auto")) {
-		if (c->mode != MODE_PASTA)
-			die("'auto' port forwarding is only allowed for pasta");
-
-		conf_ports_range_except(c, optname, optarg, fwd,
-					proto, NULL, NULL,
-					1, NUM_PORTS - 1, NULL, 1, FWD_SCAN);
-
-		return;
-	}
-
 	strncpy(buf, optarg, sizeof(buf) - 1);
 
 	if ((spec = strchr(buf, '/'))) {
@@ -1031,13 +1061,13 @@ static void usage(const char *name, FILE *f, int status)
 		"    can be specified multiple times\n"
 		"    SPEC can be:\n"
 		"      'none': don't forward any ports\n"
-		"%s"
 		"      [ADDR[%%IFACE]/]PORTS: forward specific ports\n"
 		"        PORTS is either 'all' (forward all unbound, non-ephemeral\n"
 		"        ports), or a comma-separated list of ports, optionally\n"
 		"        ranged with '-' and optional target ports after ':'.\n"
 		"        Ranges can be reduced by excluding ports or ranges\n"
-		"        prefixed by '~'\n"
+		"        prefixed by '~'.\n"
+		"%s"
 		"        Examples:\n"
 		"        -t all		Forward all ports\n"
 		"        -t ::1/all	Forward all ports from local address ::1\n"
@@ -1050,15 +1080,26 @@ static void usage(const char *name, FILE *f, int status)
 		"        -t 192.0.2.1/5	Bind port 5 of 192.0.2.1 to %s\n"
 		"        -t 5-25,~10-20	Forward ports 5 to 9, and 21 to 25\n"
 		"        -t ~25		Forward all ports except for 25\n"
+		"%s"
 		"    default: %s\n"
 		"  -u, --udp-ports SPEC	UDP port forwarding to %s\n"
 		"    SPEC is as described for TCP above\n"
 		"    default: %s\n",
 		guest,
 		strstr(name, "pasta") ?
-		"      'auto': forward all ports currently bound in namespace\n"
+		"        The 'auto' keyword may be given to only forward\n"
+		"        ports which are bound in the target namespace\n"
+		: "",
+		guest, guest, guest,
+		strstr(name, "pasta") ?
+		"        -t auto\t	Forward all ports bound in namespace\n"
+		"        -t ::1/auto	Forward ports from ::1 if they are\n"
+		"        		bound in the namespace\n"
+		"        -t 80-82,auto	Forward ports 80-82 if they are bound\n"
+		"        		in the namespace\n"
 		: "",
-		guest, guest, guest, fwd_default, guest, fwd_default);
+
+		fwd_default, guest, fwd_default);
 
 	if (strstr(name, "pasta"))
 		goto pasta_opts;
diff --git a/passt.1 b/passt.1
index 20dc72ca..6303aeb0 100644
--- a/passt.1
+++ b/passt.1
@@ -434,12 +434,6 @@ Configure TCP port forwarding to guest or namespace. \fIspec\fR can be one of:
 .BR none
 Don't forward any ports
 
-.TP
-.BR auto " " (\fBpasta\fR " " only)
-Dynamically forward ports bound in the namespace. The list of ports is
-periodically derived (every second) from listening sockets reported by
-\fI/proc/net/tcp\fR and \fI/proc/net/tcp6\fR, see \fBproc\fR(5).
-
 .TP
 [\fIaddress\fR[\fB%\fR\fIinterface\fR]\fB/\fR]\fIports\fR ...
 Specific ports to forward.  Optionally, a specific listening address
@@ -468,11 +462,20 @@ as \fIfirst\fR.
 \fB~\fR\fIfirst\fR[\fB-\fR\fIlast\fR]
 Exclude range.  Don't forward port numbers between \fIfirst\fR and
 \fIlast\fR.  This takes precedences over include ranges.
+
+.TP
+.BR auto
+\fBpasta\fR only.  Only forward ports in the specified set if the
+target ports are bound in the namespace. The list of ports is
+periodically derived (every second) from listening sockets reported by
+\fI/proc/net/tcp\fR and \fI/proc/net/tcp6\fR, see \fBproc\fR(5).
 .RE
 
 Specifying excluded ranges only implies that all other non-ephemeral
-ports are forwarded. In this case, no failures are reported for
-unavailable ports, unless no ports could be forwarded at all.
+ports are forwarded. Specifying no ranges at all implies forwarding
+all non-ephemeral ports permitted by current capabilities.  In this
+case, no failures are reported for unavailable ports, unless no ports
+could be forwarded at all.
 
 Examples:
 .RS
@@ -519,6 +522,17 @@ and 30
 .TP
 -t ~20000-20010
 Forward all ports to the guest, except for the range from 20000 to 20010
+.TP
+-t auto
+Automatically forward any ports which are bound in the namespace
+.TP
+-t ::1/auto
+Automatically forward any ports which are bound in the namespace,
+listening only on local port ::1
+.TP
+-t 8000-8010,auto
+Forward ports in the range 8000-8010 if and only if they are bound in
+the namespace
 .RE
 
 Default is \fBnone\fR for \fBpasst\fR and \fBauto\fR for \fBpasta\fR.
-- 
2.53.0

[PATCH v3 04/11] conf: Move SO_BINDTODEVICE workaround to conf_ports()

[David Gibson]

For historical reasons we apply our workaround for -[TU] handling when
SO_BINDTODEVICE is unavailable inside conf_ports_range_except().  We've
now removed the reasons it had to be there, so it can move to conf_ports(),
the caller's caller.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c | 77 ++++++++++++++++++++++------------------------------------
 1 file changed, 29 insertions(+), 48 deletions(-)

diff --git a/conf.c b/conf.c
index 33b96eac..5ff62873 100644
--- a/conf.c
+++ b/conf.c
@@ -138,9 +138,6 @@ static int parse_keyword(const char *s, const char **endptr, const char *kw)
 /**
  * conf_ports_range_except() - Set up forwarding for a range of ports minus a
  *                             bitmap of exclusions
- * @c:		Execution context
- * @optname:	Short option name, t, T, u, or U
- * @optarg:	Option argument (port specification)
  * @fwd:	Forwarding table to be updated
  * @proto:	Protocol to forward
  * @addr:	Listening address
@@ -151,9 +148,8 @@ static int parse_keyword(const char *s, const char **endptr, const char *kw)
  * @to:		Port to translate @first to when forwarding
  * @flags:	Flags for forwarding entries
  */
-static void conf_ports_range_except(const struct ctx *c, char optname,
-				    const char *optarg, struct fwd_table *fwd,
-				    uint8_t proto, const union inany_addr *addr,
+static void conf_ports_range_except(struct fwd_table *fwd, uint8_t proto,
+				    const union inany_addr *addr,
 				    const char *ifname,
 				    uint16_t first, uint16_t last,
 				    const uint8_t *exclude, uint16_t to,
@@ -195,42 +191,10 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
 		rule.last = i - 1;
 		rule.to = base + delta;
 
-		if ((optname == 'T' || optname == 'U') && c->no_bindtodevice) {
-			/* FIXME: Once the fwd bitmaps are removed, move this
-			 * workaround to the caller
-			 */
-			struct fwd_rule rulev = {
-				.ifname = { 0 },
-				.flags = flags,
-				.first = base,
-				.last = i - 1,
-				.to = base + delta,
-			};
-
-			assert(!addr && ifname && !strcmp(ifname, "lo"));
-			warn(
-"SO_BINDTODEVICE unavailable, forwarding only 127.0.0.1 and ::1 for '-%c %s'",
-			     optname, optarg);
+		fwd_rule_conflict_check(&rule, fwd->rules, fwd->count);
+		if (fwd_rule_add(fwd, &rule) < 0)
+			goto fail;
 
-			if (c->ifi4) {
-				rulev.addr = inany_loopback4;
-				fwd_rule_conflict_check(&rulev,
-							fwd->rules, fwd->count);
-				if (fwd_rule_add(fwd, &rulev) < 0)
-					goto fail;
-			}
-			if (c->ifi6) {
-				rulev.addr = inany_loopback6;
-				fwd_rule_conflict_check(&rulev,
-							fwd->rules, fwd->count);
-				if (fwd_rule_add(fwd, &rulev) < 0)
-					goto fail;
-			}
-		} else {
-			fwd_rule_conflict_check(&rule, fwd->rules, fwd->count);
-			if (fwd_rule_add(fwd, &rule) < 0)
-				goto fail;
-		}
 		base = i - 1;
 	}
 	return;
@@ -321,8 +285,7 @@ static void conf_ports_spec(const struct ctx *c,
 		/* Exclude ephemeral ports */
 		fwd_port_map_ephemeral(exclude);
 
-		conf_ports_range_except(c, optname, optarg, fwd,
-					proto, addr, ifname,
+		conf_ports_range_except(fwd, proto, addr, ifname,
 					1, NUM_PORTS - 1, exclude,
 					1, flags | FWD_WEAK);
 		return;
@@ -357,8 +320,7 @@ static void conf_ports_spec(const struct ctx *c,
 			    optname, optarg);
 		}
 
-		conf_ports_range_except(c, optname, optarg, fwd,
-					proto, addr, ifname,
+		conf_ports_range_except(fwd, proto, addr, ifname,
 					orig_range.first, orig_range.last,
 					exclude,
 					mapped_range.first, flags);
@@ -461,14 +423,33 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 		}
 	}
 
+	if (optname == 'T' || optname == 'U') {
+		assert(!addr && !ifname);
+
+		if (c->no_bindtodevice) {
+			warn(
+"SO_BINDTODEVICE unavailable, forwarding only 127.0.0.1 and ::1 for '-%c %s'",
+			     optname, optarg);
+
+			if (c->ifi4) {
+				conf_ports_spec(c, optname, optarg, fwd, proto,
+						&inany_loopback4, NULL, spec);
+			}
+			if (c->ifi6) {
+				conf_ports_spec(c, optname, optarg, fwd, proto,
+						&inany_loopback6, NULL, spec);
+			}
+			return;
+		}
+
+		ifname = "lo";
+	}
+
 	if (ifname && c->no_bindtodevice) {
 		die(
 "Device binding for '-%c %s' unsupported (requires kernel 5.7+)",
 		    optname, optarg);
 	}
-	/* Outbound forwards come from guest loopback */
-	if ((optname == 'T' || optname == 'U') && !ifname)
-		ifname = "lo";
 
 	conf_ports_spec(c, optname, optarg, fwd, proto, addr, ifname, spec);
 }
-- 
2.53.0

[PATCH v3 05/11] conf: Don't pass raw commandline argument to conf_ports_spec()

[David Gibson]

We only use the optname and optarg parameters for printing error messages,
and they're not even particularly necessary there.  Remove them.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/conf.c b/conf.c
index 5ff62873..99542075 100644
--- a/conf.c
+++ b/conf.c
@@ -219,8 +219,6 @@ fail:
 /**
  * conf_ports_spec() - Parse port range(s) specifier
  * @c:		Execution context
- * @optname:	Short option name, t, T, u, or U
- * @optarg:	Option argument (port specification)
  * @fwd:	Forwarding table to be updated
  * @proto:	Protocol to forward
  * @addr:	Listening address for forwarding
@@ -228,7 +226,6 @@ fail:
  * @spec:	Port range(s) specifier
  */
 static void conf_ports_spec(const struct ctx *c,
-			    char optname, const char *optarg,
 			    struct fwd_table *fwd, uint8_t proto,
 			    const union inany_addr *addr, const char *ifname,
 			    const char *spec)
@@ -316,8 +313,7 @@ static void conf_ports_spec(const struct ctx *c,
 			goto bad;
 
 		if (orig_range.first == 0) {
-			die("Can't forward port 0 for option '-%c %s'",
-			    optname, optarg);
+			die("Can't forward port 0 included in '%s'", spec);
 		}
 
 		conf_ports_range_except(fwd, proto, addr, ifname,
@@ -328,7 +324,7 @@ static void conf_ports_spec(const struct ctx *c,
 
 	return;
 bad:
-	die("Invalid port specifier %s", optarg);
+	die("Invalid port specifier '%s'", spec);
 }
 
 /**
@@ -432,11 +428,11 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 			     optname, optarg);
 
 			if (c->ifi4) {
-				conf_ports_spec(c, optname, optarg, fwd, proto,
+				conf_ports_spec(c, fwd, proto,
 						&inany_loopback4, NULL, spec);
 			}
 			if (c->ifi6) {
-				conf_ports_spec(c, optname, optarg, fwd, proto,
+				conf_ports_spec(c, fwd, proto,
 						&inany_loopback6, NULL, spec);
 			}
 			return;
@@ -451,7 +447,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 		    optname, optarg);
 	}
 
-	conf_ports_spec(c, optname, optarg, fwd, proto, addr, ifname, spec);
+	conf_ports_spec(c, fwd, proto, addr, ifname, spec);
 }
 
 /**
-- 
2.53.0

[PATCH v3 06/11] fwd, conf: Add capabilities bits to each forwarding table

[David Gibson]

conf_ports_spec() and conf_ports() take the global context structure, but
their only use for it is seeing if various things are possible: which
protocols and address formats are allowed in formatting rules.  Localise
that information into the forwarding table, with a capabilities bitmap.

For now we set that caps map to the same thing for all tables, but keep it
per-table to allow for the possibility of different pif types in future
that might have different capabilities (e.g. if we add a forwarding table
for the tap interface, it won't be able to accept interface names to bind).

Use this information to remove the global context parameter from
conf_ports() and conf_ports_spec().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c     | 48 ++++++++++++++++++++++--------------------------
 fwd.c      | 17 +++++++++++++++++
 fwd.h      |  2 ++
 fwd_rule.h |  8 ++++++++
 4 files changed, 49 insertions(+), 26 deletions(-)

diff --git a/conf.c b/conf.c
index 99542075..ecc3a342 100644
--- a/conf.c
+++ b/conf.c
@@ -218,15 +218,13 @@ fail:
 
 /**
  * conf_ports_spec() - Parse port range(s) specifier
- * @c:		Execution context
  * @fwd:	Forwarding table to be updated
  * @proto:	Protocol to forward
  * @addr:	Listening address for forwarding
  * @ifname:	Interface name for listening
  * @spec:	Port range(s) specifier
  */
-static void conf_ports_spec(const struct ctx *c,
-			    struct fwd_table *fwd, uint8_t proto,
+static void conf_ports_spec(struct fwd_table *fwd, uint8_t proto,
 			    const union inany_addr *addr, const char *ifname,
 			    const char *spec)
 {
@@ -255,7 +253,7 @@ static void conf_ports_spec(const struct ctx *c,
 			if (p != ep) /* Garbage after the keyword */
 				goto bad;
 
-			if (c->mode != MODE_PASTA) {
+			if (!(fwd->caps & FWD_CAP_SCAN)) {
 				die(
 "'auto' port forwarding is only allowed for pasta");
 			}
@@ -329,13 +327,11 @@ bad:
 
 /**
  * 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)
  * @fwd:	Forwarding table to be updated
  */
-static void conf_ports(const struct ctx *c, char optname, const char *optarg,
-		       struct fwd_table *fwd)
+static void conf_ports(char optname, const char *optarg, struct fwd_table *fwd)
 {
 	union inany_addr addr_buf = inany_any6, *addr = &addr_buf;
 	char buf[BUFSIZ], *spec, *ifname = NULL;
@@ -360,9 +356,9 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 		return;
 	}
 
-	if (proto == IPPROTO_TCP && c->no_tcp)
+	if (proto == IPPROTO_TCP && !(fwd->caps & FWD_CAP_TCP))
 		die("TCP port forwarding requested but TCP is disabled");
-	if (proto == IPPROTO_UDP && c->no_udp)
+	if (proto == IPPROTO_UDP && !(fwd->caps & FWD_CAP_UDP))
 		die("UDP port forwarding requested but UDP is disabled");
 
 	strncpy(buf, optarg, sizeof(buf) - 1);
@@ -410,10 +406,10 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 	}
 
 	if (addr) {
-		if (!c->ifi4 && inany_v4(addr)) {
+		if (!(fwd->caps & FWD_CAP_IPV4) && inany_v4(addr)) {
 			die("IPv4 is disabled, can't use -%c %s",
 			    optname, optarg);
-		} else if (!c->ifi6 && !inany_v4(addr)) {
+		} else if (!(fwd->caps & FWD_CAP_IPV6) && !inany_v4(addr)) {
 			die("IPv6 is disabled, can't use -%c %s",
 			    optname, optarg);
 		}
@@ -422,17 +418,17 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 	if (optname == 'T' || optname == 'U') {
 		assert(!addr && !ifname);
 
-		if (c->no_bindtodevice) {
+		if (!(fwd->caps & FWD_CAP_IFNAME)) {
 			warn(
 "SO_BINDTODEVICE unavailable, forwarding only 127.0.0.1 and ::1 for '-%c %s'",
 			     optname, optarg);
 
-			if (c->ifi4) {
-				conf_ports_spec(c, fwd, proto,
+			if (fwd->caps & FWD_CAP_IPV4) {
+				conf_ports_spec(fwd, proto,
 						&inany_loopback4, NULL, spec);
 			}
-			if (c->ifi6) {
-				conf_ports_spec(c, fwd, proto,
+			if (fwd->caps & FWD_CAP_IPV6) {
+				conf_ports_spec(fwd, proto,
 						&inany_loopback6, NULL, spec);
 			}
 			return;
@@ -441,13 +437,13 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 		ifname = "lo";
 	}
 
-	if (ifname && c->no_bindtodevice) {
+	if (ifname && !(fwd->caps & FWD_CAP_IFNAME)) {
 		die(
 "Device binding for '-%c %s' unsupported (requires kernel 5.7+)",
 		    optname, optarg);
 	}
 
-	conf_ports_spec(c, fwd, proto, addr, ifname, spec);
+	conf_ports_spec(fwd, proto, addr, ifname, spec);
 }
 
 /**
@@ -2185,16 +2181,16 @@ void conf(struct ctx *c, int argc, char **argv)
 
 		if (name == 't') {
 			opt_t = true;
-			conf_ports(c, name, optarg, c->fwd[PIF_HOST]);
+			conf_ports(name, optarg, c->fwd[PIF_HOST]);
 		} else if (name == 'u') {
 			opt_u = true;
-			conf_ports(c, name, optarg, c->fwd[PIF_HOST]);
+			conf_ports(name, optarg, c->fwd[PIF_HOST]);
 		} else if (name == 'T') {
 			opt_T = true;
-			conf_ports(c, name, optarg, c->fwd[PIF_SPLICE]);
+			conf_ports(name, optarg, c->fwd[PIF_SPLICE]);
 		} else if (name == 'U') {
 			opt_U = true;
-			conf_ports(c, name, optarg, c->fwd[PIF_SPLICE]);
+			conf_ports(name, optarg, c->fwd[PIF_SPLICE]);
 		}
 	} while (name != -1);
 
@@ -2246,13 +2242,13 @@ void conf(struct ctx *c, int argc, char **argv)
 
 	if (c->mode == MODE_PASTA) {
 		if (!opt_t)
-			conf_ports(c, 't', "auto", c->fwd[PIF_HOST]);
+			conf_ports('t', "auto", c->fwd[PIF_HOST]);
 		if (!opt_T)
-			conf_ports(c, 'T', "auto", c->fwd[PIF_SPLICE]);
+			conf_ports('T', "auto", c->fwd[PIF_SPLICE]);
 		if (!opt_u)
-			conf_ports(c, 'u', "auto", c->fwd[PIF_HOST]);
+			conf_ports('u', "auto", c->fwd[PIF_HOST]);
 		if (!opt_U)
-			conf_ports(c, 'U', "auto", c->fwd[PIF_SPLICE]);
+			conf_ports('U', "auto", c->fwd[PIF_SPLICE]);
 	}
 
 	if (!c->quiet)
diff --git a/fwd.c b/fwd.c
index 3e87169b..c7fd1a9d 100644
--- a/fwd.c
+++ b/fwd.c
@@ -326,6 +326,23 @@ static struct fwd_table fwd_out;
  */
 void fwd_rule_init(struct ctx *c)
 {
+	uint32_t caps = 0;
+
+	if (c->ifi4)
+		caps |= FWD_CAP_IPV4;
+	if (c->ifi6)
+		caps |= FWD_CAP_IPV6;
+	if (!c->no_tcp)
+		caps |= FWD_CAP_TCP;
+	if (!c->no_udp)
+		caps |= FWD_CAP_UDP;
+	if (c->mode == MODE_PASTA)
+		caps |= FWD_CAP_SCAN;
+	if (!c->no_bindtodevice)
+		caps |= FWD_CAP_IFNAME;
+
+	fwd_in.caps = fwd_out.caps = caps;
+
 	c->fwd[PIF_HOST] = &fwd_in;
 	if (c->mode == MODE_PASTA)
 		c->fwd[PIF_SPLICE] = &fwd_out;
diff --git a/fwd.h b/fwd.h
index 43bfeadb..3e365d35 100644
--- a/fwd.h
+++ b/fwd.h
@@ -52,6 +52,7 @@ struct fwd_listen_ref {
 
 /**
  * struct fwd_table - Forwarding state (per initiating pif)
+ * @caps:	Forwarding capabilities for this initiating pif
  * @count:	Number of forwarding rules
  * @rules:	Array of forwarding rules
  * @rulesocks:	Parallel array of @rules (@count valid entries) of pointers to
@@ -61,6 +62,7 @@ struct fwd_listen_ref {
  * @socks:	Listening sockets for forwarding
  */
 struct fwd_table {
+	uint32_t caps;
 	unsigned count;
 	struct fwd_rule rules[MAX_FWD_RULES];
 	int *rulesocks[MAX_FWD_RULES];
diff --git a/fwd_rule.h b/fwd_rule.h
index 8506a0c4..edba6782 100644
--- a/fwd_rule.h
+++ b/fwd_rule.h
@@ -17,6 +17,14 @@
 #include "inany.h"
 #include "bitmap.h"
 
+/* Forwarding capability bits */
+#define FWD_CAP_IPV4		BIT(0)
+#define FWD_CAP_IPV6		BIT(1)
+#define FWD_CAP_TCP		BIT(2)
+#define FWD_CAP_UDP		BIT(3)
+#define FWD_CAP_SCAN		BIT(4)
+#define FWD_CAP_IFNAME		BIT(5)
+
 /**
  * struct fwd_rule - Forwarding rule governing a range of ports
  * @addr:	Address to forward from
-- 
2.53.0

[PATCH v3 07/11] conf, fwd: Stricter rule checking in fwd_rule_add()

[David Gibson]

Although fwd_rule_add() performs some sanity checks on the rule it is
given, there are invalid rules we don't check for, assuming that its
callers will do that.

That won't be enough when we can get rules inserted by a dynamic update
client without going through the existing parsing code.  So, add stricter
checks to fwd_rule_add(), which is now possible thanks to the capabilities
bits in the struct fwd_table.  Where those duplicate existing checks in the
callers, remove the old copies.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c | 19 -------------------
 fwd.c  | 51 ++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 46 insertions(+), 24 deletions(-)

diff --git a/conf.c b/conf.c
index ecc3a342..3b373b22 100644
--- a/conf.c
+++ b/conf.c
@@ -310,10 +310,6 @@ static void conf_ports_spec(struct fwd_table *fwd, uint8_t proto,
 		if (p != ep) /* Garbage after the ranges */
 			goto bad;
 
-		if (orig_range.first == 0) {
-			die("Can't forward port 0 included in '%s'", spec);
-		}
-
 		conf_ports_range_except(fwd, proto, addr, ifname,
 					orig_range.first, orig_range.last,
 					exclude,
@@ -356,11 +352,6 @@ static void conf_ports(char optname, const char *optarg, struct fwd_table *fwd)
 		return;
 	}
 
-	if (proto == IPPROTO_TCP && !(fwd->caps & FWD_CAP_TCP))
-		die("TCP port forwarding requested but TCP is disabled");
-	if (proto == IPPROTO_UDP && !(fwd->caps & FWD_CAP_UDP))
-		die("UDP port forwarding requested but UDP is disabled");
-
 	strncpy(buf, optarg, sizeof(buf) - 1);
 
 	if ((spec = strchr(buf, '/'))) {
@@ -405,16 +396,6 @@ static void conf_ports(char optname, const char *optarg, struct fwd_table *fwd)
 		addr = NULL;
 	}
 
-	if (addr) {
-		if (!(fwd->caps & FWD_CAP_IPV4) && inany_v4(addr)) {
-			die("IPv4 is disabled, can't use -%c %s",
-			    optname, optarg);
-		} else if (!(fwd->caps & FWD_CAP_IPV6) && !inany_v4(addr)) {
-			die("IPv6 is disabled, can't use -%c %s",
-			    optname, optarg);
-		}
-	}
-
 	if (optname == 'T' || optname == 'U') {
 		assert(!addr && !ifname);
 
diff --git a/fwd.c b/fwd.c
index c7fd1a9d..aa966731 100644
--- a/fwd.c
+++ b/fwd.c
@@ -367,17 +367,58 @@ int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
 		     new->first, new->last);
 		return -EINVAL;
 	}
+	if (!new->first) {
+		warn("Forwarding rule attempts to map from port 0");
+		return -EINVAL;
+	}
+	if (!new->to || (new->to + new->last - new->first) < new->to) {
+		warn("Forwarding rule attempts to map to port 0");
+		return -EINVAL;
+	}
 	if (new->flags & ~allowed_flags) {
 		warn("Rule has invalid flags 0x%hhx",
 		     new->flags & ~allowed_flags);
 		return -EINVAL;
 	}
-	if (new->flags & FWD_DUAL_STACK_ANY &&
-	    !inany_equals(&new->addr, &inany_any6)) {
-		char astr[INANY_ADDRSTRLEN];
+	if (new->flags & FWD_DUAL_STACK_ANY) {
+		if (!inany_equals(&new->addr, &inany_any6)) {
+			char astr[INANY_ADDRSTRLEN];
 
-		warn("Dual stack rule has non-wildcard address %s",
-		     inany_ntop(&new->addr, astr, sizeof(astr)));
+			warn("Dual stack rule has non-wildcard address %s",
+			     inany_ntop(&new->addr, astr, sizeof(astr)));
+			return -EINVAL;
+		}
+		if (!(fwd->caps & FWD_CAP_IPV4)) {
+			warn("Dual stack forward, but IPv4 not enabled");
+			return -EINVAL;
+		}
+		if (!(fwd->caps & FWD_CAP_IPV6)) {
+			warn("Dual stack forward, but IPv6 not enabled");
+			return -EINVAL;
+		}
+	} else {
+		if (inany_v4(&new->addr) && !(fwd->caps & FWD_CAP_IPV4)) {
+			warn("IPv4 forward, but IPv4 not enabled");
+			return -EINVAL;
+		}
+		if (!inany_v4(&new->addr) && !(fwd->caps & FWD_CAP_IPV6)) {
+			warn("IPv6 forward, but IPv6 not enabled");
+			return -EINVAL;
+		}
+	}
+	if (new->proto == IPPROTO_TCP) {
+		if (!(fwd->caps & FWD_CAP_TCP)) {
+			warn("Can't add TCP forwarding rule, TCP not enabled");
+			return -EINVAL;
+		}
+	} else if (new->proto == IPPROTO_UDP) {
+		if (!(fwd->caps & FWD_CAP_UDP)) {
+			warn("Can't add UDP forwarding rule, UDP not enabled");
+			return -EINVAL;
+		}
+	} else {
+		warn("Unsupported protocol 0x%hhx (%s) for forwarding rule",
+		     new->proto, ipproto_name(new->proto));
 		return -EINVAL;
 	}
 
-- 
2.53.0

[PATCH v3 08/11] fwd_rule: Move ephemeral port probing to fwd_rule.c

[David Gibson]

We want to move parsing of forward rule options to fwd_rule.c so it can
eventually be shared with a configuration client.  As a preliminary step,
move the ephemeral port probing there, which that will need to use.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd.c      | 73 --------------------------------------------------
 fwd.h      |  6 -----
 fwd_rule.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fwd_rule.h |  6 +++++
 4 files changed, 84 insertions(+), 79 deletions(-)

diff --git a/fwd.c b/fwd.c
index aa966731..9a7053fd 100644
--- a/fwd.c
+++ b/fwd.c
@@ -34,12 +34,6 @@
 #include "arp.h"
 #include "ndp.h"
 
-/* Ephemeral port range: values from RFC 6335 */
-static in_port_t fwd_ephemeral_min = (1 << 15) + (1 << 14);
-static in_port_t fwd_ephemeral_max = NUM_PORTS - 1;
-
-#define PORT_RANGE_SYSCTL	"/proc/sys/net/ipv4/ip_local_port_range"
-
 #define NEIGH_TABLE_SLOTS    1024
 #define NEIGH_TABLE_SIZE     (NEIGH_TABLE_SLOTS / 2)
 static_assert((NEIGH_TABLE_SLOTS & (NEIGH_TABLE_SLOTS - 1)) == 0,
@@ -249,73 +243,6 @@ void fwd_neigh_table_init(const struct ctx *c)
 		fwd_neigh_table_update(c, &mga, c->our_tap_mac, true);
 }
 
-/** fwd_probe_ephemeral() - Determine what ports this host considers ephemeral
- *
- * Work out what ports the host thinks are emphemeral and record it for later
- * use by fwd_port_is_ephemeral().  If we're unable to probe, assume the range
- * recommended by RFC 6335.
- */
-void fwd_probe_ephemeral(void)
-{
-	char *line, *tab, *end;
-	struct lineread lr;
-	long min, max;
-	ssize_t len;
-	int fd;
-
-	fd = open(PORT_RANGE_SYSCTL, O_RDONLY | O_CLOEXEC);
-	if (fd < 0) {
-		warn_perror("Unable to open %s", PORT_RANGE_SYSCTL);
-		return;
-	}
-
-	lineread_init(&lr, fd);
-	len = lineread_get(&lr, &line);
-	close(fd);
-
-	if (len < 0)
-		goto parse_err;
-
-	tab = strchr(line, '\t');
-	if (!tab)
-		goto parse_err;
-	*tab = '\0';
-
-	errno = 0;
-	min = strtol(line, &end, 10);
-	if (*end || errno)
-		goto parse_err;
-
-	errno = 0;
-	max = strtol(tab + 1, &end, 10);
-	if (*end || errno)
-		goto parse_err;
-
-	if (min < 0 || min >= (long)NUM_PORTS ||
-	    max < 0 || max >= (long)NUM_PORTS)
-		goto parse_err;
-
-	fwd_ephemeral_min = min;
-	fwd_ephemeral_max = max;
-
-	return;
-
-parse_err:
-	warn("Unable to parse %s", PORT_RANGE_SYSCTL);
-}
-
-/**
- * fwd_port_map_ephemeral() - Mark ephemeral ports in a bitmap
- * @map:	Bitmap to update
- */
-void fwd_port_map_ephemeral(uint8_t *map)
-{
-	unsigned port;
-
-	for (port = fwd_ephemeral_min; port <= fwd_ephemeral_max; port++)
-		bitmap_set(map, port);
-}
-
 /* Forwarding table storage, generally accessed via pointers in struct ctx */
 static struct fwd_table fwd_in;
 static struct fwd_table fwd_out;
diff --git a/fwd.h b/fwd.h
index 3e365d35..e664d1d0 100644
--- a/fwd.h
+++ b/fwd.h
@@ -20,12 +20,6 @@
 
 struct flowside;
 
-/* Number of ports for both TCP and UDP */
-#define	NUM_PORTS	(1U << 16)
-
-void fwd_probe_ephemeral(void);
-void fwd_port_map_ephemeral(uint8_t *map);
-
 #define FWD_RULE_BITS	8
 #define MAX_FWD_RULES	MAX_FROM_BITS(FWD_RULE_BITS)
 #define FWD_NO_HINT	(-1)
diff --git a/fwd_rule.c b/fwd_rule.c
index 47d8df1c..9d489827 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -15,9 +15,87 @@
  * Author: David Gibson <david@gibson.dropbear.id.au>
  */
 
+#include <errno.h>
+#include <fcntl.h>
 #include <stdio.h>
+#include <unistd.h>
 
 #include "fwd_rule.h"
+#include "lineread.h"
+#include "log.h"
+
+/* Ephemeral port range: values from RFC 6335 */
+static in_port_t fwd_ephemeral_min = (1 << 15) + (1 << 14);
+static in_port_t fwd_ephemeral_max = NUM_PORTS - 1;
+
+#define PORT_RANGE_SYSCTL	"/proc/sys/net/ipv4/ip_local_port_range"
+
+/** fwd_probe_ephemeral() - Determine what ports this host considers ephemeral
+ *
+ * Work out what ports the host thinks are emphemeral and record it for later
+ * use by fwd_port_is_ephemeral().  If we're unable to probe, assume the range
+ * recommended by RFC 6335.
+ */
+void fwd_probe_ephemeral(void)
+{
+	char *line, *tab, *end;
+	struct lineread lr;
+	long min, max;
+	ssize_t len;
+	int fd;
+
+	fd = open(PORT_RANGE_SYSCTL, O_RDONLY | O_CLOEXEC);
+	if (fd < 0) {
+		warn_perror("Unable to open %s", PORT_RANGE_SYSCTL);
+		return;
+	}
+
+	lineread_init(&lr, fd);
+	len = lineread_get(&lr, &line);
+	close(fd);
+
+	if (len < 0)
+		goto parse_err;
+
+	tab = strchr(line, '\t');
+	if (!tab)
+		goto parse_err;
+	*tab = '\0';
+
+	errno = 0;
+	min = strtol(line, &end, 10);
+	if (*end || errno)
+		goto parse_err;
+
+	errno = 0;
+	max = strtol(tab + 1, &end, 10);
+	if (*end || errno)
+		goto parse_err;
+
+	if (min < 0 || min >= (long)NUM_PORTS ||
+	    max < 0 || max >= (long)NUM_PORTS)
+		goto parse_err;
+
+	fwd_ephemeral_min = min;
+	fwd_ephemeral_max = max;
+
+	return;
+
+parse_err:
+	warn("Unable to parse %s", PORT_RANGE_SYSCTL);
+}
+
+/**
+ * fwd_port_map_ephemeral() - Mark ephemeral ports in a bitmap
+ * @map:	Bitmap to update
+ */
+void fwd_port_map_ephemeral(uint8_t *map)
+{
+	unsigned port;
+
+	for (port = fwd_ephemeral_min; port <= fwd_ephemeral_max; port++)
+		bitmap_set(map, port);
+}
 
 /**
  * fwd_rule_addr() - Return match address for a rule
diff --git a/fwd_rule.h b/fwd_rule.h
index edba6782..5c7b67aa 100644
--- a/fwd_rule.h
+++ b/fwd_rule.h
@@ -17,6 +17,9 @@
 #include "inany.h"
 #include "bitmap.h"
 
+/* Number of ports for both TCP and UDP */
+#define	NUM_PORTS	(1U << 16)
+
 /* Forwarding capability bits */
 #define FWD_CAP_IPV4		BIT(0)
 #define FWD_CAP_IPV6		BIT(1)
@@ -51,6 +54,9 @@ struct fwd_rule {
 	uint8_t flags;
 };
 
+void fwd_probe_ephemeral(void);
+void fwd_port_map_ephemeral(uint8_t *map);
+
 #define FWD_RULE_STRLEN					    \
 	(IPPROTO_STRLEN - 1				    \
 	 + INANY_ADDRSTRLEN - 1				    \
-- 
2.53.0

[PATCH v3 09/11] fwd, conf: Move rule parsing code to fwd_rule.[ch]

[David Gibson]

The code parsing command line options into forwarding rules has now been
decoupled from most of passt/pasta's internals.  This is good, because
we'll soon want to share it with a configuration update client.

Make the next step by moving this code into fwd_rule.[ch].

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c     | 378 +------------------------------------------
 fwd.c      |  93 -----------
 fwd.h      |  33 ----
 fwd_rule.c | 465 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 fwd_rule.h |  36 ++++-
 5 files changed, 503 insertions(+), 502 deletions(-)

diff --git a/conf.c b/conf.c
index 3b373b22..5aacfe0f 100644
--- a/conf.c
+++ b/conf.c
@@ -13,7 +13,6 @@
  */
 
 #include <arpa/inet.h>
-#include <ctype.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <getopt.h>
@@ -66,367 +65,6 @@
 
 const char *pasta_default_ifn = "tap0";
 
-/**
- * port_range() - Represents a non-empty range of ports
- * @first:	First port number in the range
- * @last:	Last port number in the range (inclusive)
- *
- * Invariant:	@last >= @first
- */
-struct port_range {
-	in_port_t first, last;
-};
-
-/**
- * parse_port_range() - Parse a range of port numbers '<first>[-<last>]'
- * @s:		String to parse
- * @endptr:	Update to the character after the parsed range (similar to
- *		strtol() etc.)
- * @range:	Update with the parsed values on success
- *
- * Return: -EINVAL on parsing error, -ERANGE on out of range port
- *	   numbers, 0 on success
- */
-static int parse_port_range(const char *s, const char **endptr,
-			    struct port_range *range)
-{
-	unsigned long first, last;
-	char *ep;
-
-	last = first = strtoul(s, &ep, 10);
-	if (ep == s) /* Parsed nothing */
-		return -EINVAL;
-	if (*ep == '-') { /* we have a last value too */
-		const char *lasts = ep + 1;
-		last = strtoul(lasts, &ep, 10);
-		if (ep == lasts) /* Parsed nothing */
-			return -EINVAL;
-	}
-
-	if ((last < first) || (last >= NUM_PORTS))
-		return -ERANGE;
-
-	range->first = first;
-	range->last = last;
-	*endptr = ep;
-
-	return 0;
-}
-
-/**
- * parse_keyword() - Parse a literal keyword
- * @s:		String to parse
- * @endptr:	Update to the character after the keyword
- * @kw:		Keyword to accept
- *
- * Return: 0, if @s starts with @kw, -EINVAL if it does not
- */
-static int parse_keyword(const char *s, const char **endptr, const char *kw)
-{
-	size_t len = strlen(kw);
-
-	if (strlen(s) < len)
-		return -EINVAL;
-
-	if (memcmp(s, kw, len))
-		return -EINVAL;
-
-	*endptr = s + len;
-	return 0;
-}
-
-/**
- * conf_ports_range_except() - Set up forwarding for a range of ports minus a
- *                             bitmap of exclusions
- * @fwd:	Forwarding table to be updated
- * @proto:	Protocol to forward
- * @addr:	Listening address
- * @ifname:	Listening interface
- * @first:	First port to forward
- * @last:	Last port to forward
- * @exclude:	Bitmap of ports to exclude (may be NULL)
- * @to:		Port to translate @first to when forwarding
- * @flags:	Flags for forwarding entries
- */
-static void conf_ports_range_except(struct fwd_table *fwd, uint8_t proto,
-				    const union inany_addr *addr,
-				    const char *ifname,
-				    uint16_t first, uint16_t last,
-				    const uint8_t *exclude, uint16_t to,
-				    uint8_t flags)
-{
-	struct fwd_rule rule = {
-		.addr = addr ? *addr : inany_any6,
-		.ifname = { 0 },
-		.proto = proto,
-		.flags = flags,
-	};
-	char rulestr[FWD_RULE_STRLEN];
-	unsigned delta = to - first;
-	unsigned base, i;
-
-	if (!addr)
-		rule.flags |= FWD_DUAL_STACK_ANY;
-	if (ifname) {
-		int ret;
-
-		ret = snprintf(rule.ifname, sizeof(rule.ifname),
-			       "%s", ifname);
-		if (ret <= 0 || (size_t)ret >= sizeof(rule.ifname))
-			die("Invalid interface name: %s", ifname);
-	}
-
-	assert(first != 0);
-
-	for (base = first; base <= last; base++) {
-		if (exclude && bitmap_isset(exclude, base))
-			continue;
-
-		for (i = base; i <= last; i++) {
-			if (exclude && bitmap_isset(exclude, i))
-				break;
-		}
-
-		rule.first = base;
-		rule.last = i - 1;
-		rule.to = base + delta;
-
-		fwd_rule_conflict_check(&rule, fwd->rules, fwd->count);
-		if (fwd_rule_add(fwd, &rule) < 0)
-			goto fail;
-
-		base = i - 1;
-	}
-	return;
-
-fail:
-	die("Unable to add rule %s",
-	    fwd_rule_fmt(&rule, rulestr, sizeof(rulestr)));
-}
-
-/*
- * for_each_chunk - Step through delimited chunks of a string
- * @p_:		Pointer to start of each chunk (updated)
- * @ep_:	Pointer to end of each chunk (updated)
- * @s_:		String to step through
- * @sep_:	String of all allowed delimiters
- */
-#define for_each_chunk(p_, ep_, s_, sep_)			\
-	for ((p_) = (s_);					\
-	     (ep_) = (p_) + strcspn((p_), (sep_)), *(p_);	\
-	     (p_) = *(ep_) ? (ep_) + 1 : (ep_))
-
-/**
- * conf_ports_spec() - Parse port range(s) specifier
- * @fwd:	Forwarding table to be updated
- * @proto:	Protocol to forward
- * @addr:	Listening address for forwarding
- * @ifname:	Interface name for listening
- * @spec:	Port range(s) specifier
- */
-static void conf_ports_spec(struct fwd_table *fwd, uint8_t proto,
-			    const union inany_addr *addr, const char *ifname,
-			    const char *spec)
-{
-	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
-	bool exclude_only = true;
-	const char *p, *ep;
-	uint8_t flags = 0;
-	unsigned i;
-
-	if (!strcmp(spec, "all")) {
-		/* Treat "all" as equivalent to "": all non-ephemeral ports */
-		spec = "";
-	}
-
-	/* Parse excluded ranges and "auto" in the first pass */
-	for_each_chunk(p, ep, spec, ",") {
-		struct port_range xrange;
-
-		if (isdigit(*p))  {
-			/* Include range, parse later */
-			exclude_only = false;
-			continue;
-		}
-
-		if (parse_keyword(p, &p, "auto") == 0) {
-			if (p != ep) /* Garbage after the keyword */
-				goto bad;
-
-			if (!(fwd->caps & FWD_CAP_SCAN)) {
-				die(
-"'auto' port forwarding is only allowed for pasta");
-			}
-
-			flags |= FWD_SCAN;
-			continue;
-		}
-
-		/* Should be an exclude range */
-		if (*p != '~')
-			goto bad;
-		p++;
-
-		if (parse_port_range(p, &p, &xrange))
-			goto bad;
-		if (p != ep) /* Garbage after the range */
-			goto bad;
-
-		for (i = xrange.first; i <= xrange.last; i++)
-			bitmap_set(exclude, i);
-	}
-
-	if (exclude_only) {
-		/* Exclude ephemeral ports */
-		fwd_port_map_ephemeral(exclude);
-
-		conf_ports_range_except(fwd, proto, addr, ifname,
-					1, NUM_PORTS - 1, exclude,
-					1, flags | FWD_WEAK);
-		return;
-	}
-
-	/* Now process base ranges, skipping exclusions */
-	for_each_chunk(p, ep, spec, ",") {
-		struct port_range orig_range, mapped_range;
-
-		if (!isdigit(*p))
-			/* Already parsed */
-			continue;
-
-		if (parse_port_range(p, &p, &orig_range))
-			goto bad;
-
-		if (*p == ':') { /* There's a range to map to as well */
-			if (parse_port_range(p + 1, &p, &mapped_range))
-				goto bad;
-			if ((mapped_range.last - mapped_range.first) !=
-			    (orig_range.last - orig_range.first))
-				goto bad;
-		} else {
-			mapped_range = orig_range;
-		}
-
-		if (p != ep) /* Garbage after the ranges */
-			goto bad;
-
-		conf_ports_range_except(fwd, proto, addr, ifname,
-					orig_range.first, orig_range.last,
-					exclude,
-					mapped_range.first, flags);
-	}
-
-	return;
-bad:
-	die("Invalid port specifier '%s'", spec);
-}
-
-/**
- * conf_ports() - Parse port configuration options, initialise UDP/TCP sockets
- * @optname:	Short option name, t, T, u, or U
- * @optarg:	Option argument (port specification)
- * @fwd:	Forwarding table to be updated
- */
-static void conf_ports(char optname, const char *optarg, struct fwd_table *fwd)
-{
-	union inany_addr addr_buf = inany_any6, *addr = &addr_buf;
-	char buf[BUFSIZ], *spec, *ifname = NULL;
-	uint8_t proto;
-
-	if (optname == 't' || optname == 'T')
-		proto = IPPROTO_TCP;
-	else if (optname == 'u' || optname == 'U')
-		proto = IPPROTO_UDP;
-	else
-		assert(0);
-
-	if (!strcmp(optarg, "none")) {
-		unsigned i;
-
-		for (i = 0; i < fwd->count; i++) {
-			if (fwd->rules[i].proto == proto) {
-				die("-%c none conflicts with previous options",
-					optname);
-			}
-		}
-		return;
-	}
-
-	strncpy(buf, optarg, sizeof(buf) - 1);
-
-	if ((spec = strchr(buf, '/'))) {
-		*spec = 0;
-		spec++;
-
-		if (optname != 't' && optname != 'u')
-			die("Listening address not allowed for -%c %s",
-			    optname, optarg);
-
-		if ((ifname = strchr(buf, '%'))) {
-			*ifname = 0;
-			ifname++;
-
-			/* spec is already advanced one past the '/',
-			 * so the length of the given ifname is:
-			 * (spec - ifname - 1)
-			 */
-			if (spec - ifname - 1 >= IFNAMSIZ) {
-				die("Interface name '%s' is too long (max %u)",
-				    ifname, IFNAMSIZ - 1);
-			}
-		}
-
-		if (ifname == buf + 1) {	/* Interface without address */
-			addr = NULL;
-		} else {
-			char *p = buf;
-
-			/* Allow square brackets for IPv4 too for convenience */
-			if (*p == '[' && p[strlen(p) - 1] == ']') {
-				p[strlen(p) - 1] = '\0';
-				p++;
-			}
-
-			if (!inany_pton(p, addr))
-				die("Bad forwarding address '%s'", p);
-		}
-	} else {
-		spec = buf;
-
-		addr = NULL;
-	}
-
-	if (optname == 'T' || optname == 'U') {
-		assert(!addr && !ifname);
-
-		if (!(fwd->caps & FWD_CAP_IFNAME)) {
-			warn(
-"SO_BINDTODEVICE unavailable, forwarding only 127.0.0.1 and ::1 for '-%c %s'",
-			     optname, optarg);
-
-			if (fwd->caps & FWD_CAP_IPV4) {
-				conf_ports_spec(fwd, proto,
-						&inany_loopback4, NULL, spec);
-			}
-			if (fwd->caps & FWD_CAP_IPV6) {
-				conf_ports_spec(fwd, proto,
-						&inany_loopback6, NULL, spec);
-			}
-			return;
-		}
-
-		ifname = "lo";
-	}
-
-	if (ifname && !(fwd->caps & FWD_CAP_IFNAME)) {
-		die(
-"Device binding for '-%c %s' unsupported (requires kernel 5.7+)",
-		    optname, optarg);
-	}
-
-	conf_ports_spec(fwd, proto, addr, ifname, spec);
-}
-
 /**
  * add_dns4() - Possibly add the IPv4 address of a DNS resolver to configuration
  * @c:		Execution context
@@ -2162,16 +1800,16 @@ void conf(struct ctx *c, int argc, char **argv)
 
 		if (name == 't') {
 			opt_t = true;
-			conf_ports(name, optarg, c->fwd[PIF_HOST]);
+			fwd_rule_parse(name, optarg, c->fwd[PIF_HOST]);
 		} else if (name == 'u') {
 			opt_u = true;
-			conf_ports(name, optarg, c->fwd[PIF_HOST]);
+			fwd_rule_parse(name, optarg, c->fwd[PIF_HOST]);
 		} else if (name == 'T') {
 			opt_T = true;
-			conf_ports(name, optarg, c->fwd[PIF_SPLICE]);
+			fwd_rule_parse(name, optarg, c->fwd[PIF_SPLICE]);
 		} else if (name == 'U') {
 			opt_U = true;
-			conf_ports(name, optarg, c->fwd[PIF_SPLICE]);
+			fwd_rule_parse(name, optarg, c->fwd[PIF_SPLICE]);
 		}
 	} while (name != -1);
 
@@ -2223,13 +1861,13 @@ void conf(struct ctx *c, int argc, char **argv)
 
 	if (c->mode == MODE_PASTA) {
 		if (!opt_t)
-			conf_ports('t', "auto", c->fwd[PIF_HOST]);
+			fwd_rule_parse('t', "auto", c->fwd[PIF_HOST]);
 		if (!opt_T)
-			conf_ports('T', "auto", c->fwd[PIF_SPLICE]);
+			fwd_rule_parse('T', "auto", c->fwd[PIF_SPLICE]);
 		if (!opt_u)
-			conf_ports('u', "auto", c->fwd[PIF_HOST]);
+			fwd_rule_parse('u', "auto", c->fwd[PIF_HOST]);
 		if (!opt_U)
-			conf_ports('U', "auto", c->fwd[PIF_SPLICE]);
+			fwd_rule_parse('U', "auto", c->fwd[PIF_SPLICE]);
 	}
 
 	if (!c->quiet)
diff --git a/fwd.c b/fwd.c
index 9a7053fd..728a783c 100644
--- a/fwd.c
+++ b/fwd.c
@@ -275,99 +275,6 @@ void fwd_rule_init(struct ctx *c)
 		c->fwd[PIF_SPLICE] = &fwd_out;
 }
 
-/**
- * fwd_rule_add() - Validate and add a rule to a forwarding table
- * @fwd:	Table to add to
- * @new:	Rule to add
- *
- * Return: 0 on success, negative error code on failure
- */
-int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
-{
-	/* Flags which can be set from the caller */
-	const uint8_t allowed_flags = FWD_WEAK | FWD_SCAN | FWD_DUAL_STACK_ANY;
-	unsigned num = (unsigned)new->last - new->first + 1;
-	unsigned port;
-
-	if (new->first > new->last) {
-		warn("Rule has invalid port range %u-%u",
-		     new->first, new->last);
-		return -EINVAL;
-	}
-	if (!new->first) {
-		warn("Forwarding rule attempts to map from port 0");
-		return -EINVAL;
-	}
-	if (!new->to || (new->to + new->last - new->first) < new->to) {
-		warn("Forwarding rule attempts to map to port 0");
-		return -EINVAL;
-	}
-	if (new->flags & ~allowed_flags) {
-		warn("Rule has invalid flags 0x%hhx",
-		     new->flags & ~allowed_flags);
-		return -EINVAL;
-	}
-	if (new->flags & FWD_DUAL_STACK_ANY) {
-		if (!inany_equals(&new->addr, &inany_any6)) {
-			char astr[INANY_ADDRSTRLEN];
-
-			warn("Dual stack rule has non-wildcard address %s",
-			     inany_ntop(&new->addr, astr, sizeof(astr)));
-			return -EINVAL;
-		}
-		if (!(fwd->caps & FWD_CAP_IPV4)) {
-			warn("Dual stack forward, but IPv4 not enabled");
-			return -EINVAL;
-		}
-		if (!(fwd->caps & FWD_CAP_IPV6)) {
-			warn("Dual stack forward, but IPv6 not enabled");
-			return -EINVAL;
-		}
-	} else {
-		if (inany_v4(&new->addr) && !(fwd->caps & FWD_CAP_IPV4)) {
-			warn("IPv4 forward, but IPv4 not enabled");
-			return -EINVAL;
-		}
-		if (!inany_v4(&new->addr) && !(fwd->caps & FWD_CAP_IPV6)) {
-			warn("IPv6 forward, but IPv6 not enabled");
-			return -EINVAL;
-		}
-	}
-	if (new->proto == IPPROTO_TCP) {
-		if (!(fwd->caps & FWD_CAP_TCP)) {
-			warn("Can't add TCP forwarding rule, TCP not enabled");
-			return -EINVAL;
-		}
-	} else if (new->proto == IPPROTO_UDP) {
-		if (!(fwd->caps & FWD_CAP_UDP)) {
-			warn("Can't add UDP forwarding rule, UDP not enabled");
-			return -EINVAL;
-		}
-	} else {
-		warn("Unsupported protocol 0x%hhx (%s) for forwarding rule",
-		     new->proto, ipproto_name(new->proto));
-		return -EINVAL;
-	}
-
-	if (fwd->count >= ARRAY_SIZE(fwd->rules)) {
-		warn("Too many rules (maximum %u)", ARRAY_SIZE(fwd->rules));
-		return -ENOSPC;
-	}
-	if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) {
-		warn("Rules require too many listening sockets (maximum %u)",
-		     ARRAY_SIZE(fwd->socks));
-		return -ENOSPC;
-	}
-
-	fwd->rulesocks[fwd->count] = &fwd->socks[fwd->sock_count];
-	for (port = new->first; port <= new->last; port++)
-		fwd->rulesocks[fwd->count][port - new->first] = -1;
-
-	fwd->rules[fwd->count++] = *new;
-	fwd->sock_count += num;
-	return 0;
-}
-
 /**
  * fwd_rule_match() - Does a prospective flow match a given forwarding rule?
  * @rule:	Forwarding rule
diff --git a/fwd.h b/fwd.h
index e664d1d0..8f845d09 100644
--- a/fwd.h
+++ b/fwd.h
@@ -20,8 +20,6 @@
 
 struct flowside;
 
-#define FWD_RULE_BITS	8
-#define MAX_FWD_RULES	MAX_FROM_BITS(FWD_RULE_BITS)
 #define FWD_NO_HINT	(-1)
 
 /**
@@ -36,36 +34,6 @@ struct fwd_listen_ref {
 	unsigned	rule :FWD_RULE_BITS;
 };
 
-/* Maximum number of listening sockets (per pif)
- *
- * Rationale: This lets us listen on every port for two addresses and two
- * protocols (which we need for -T auto -U auto without SO_BINDTODEVICE), plus a
- * comfortable number of extras.
- */
-#define MAX_LISTEN_SOCKS	(NUM_PORTS * 5)
-
-/**
- * struct fwd_table - Forwarding state (per initiating pif)
- * @caps:	Forwarding capabilities for this initiating pif
- * @count:	Number of forwarding rules
- * @rules:	Array of forwarding rules
- * @rulesocks:	Parallel array of @rules (@count valid entries) of pointers to
- *		@socks entries giving the start of the corresponding rule's
- *		sockets within the larger array
- * @sock_count:	Number of entries used in @socks (for all rules combined)
- * @socks:	Listening sockets for forwarding
- */
-struct fwd_table {
-	uint32_t caps;
-	unsigned count;
-	struct fwd_rule rules[MAX_FWD_RULES];
-	int *rulesocks[MAX_FWD_RULES];
-	unsigned sock_count;
-	int socks[MAX_LISTEN_SOCKS];
-};
-
-#define PORT_BITMAP_SIZE	DIV_ROUND_UP(NUM_PORTS, 8)
-
 /**
  * struct fwd_scan - Port scanning state for a protocol+direction
  * @scan4:	/proc/net fd to scan for IPv4 ports when in AUTO mode
@@ -81,7 +49,6 @@ struct fwd_scan {
 #define FWD_PORT_SCAN_INTERVAL		1000	/* ms */
 
 void fwd_rule_init(struct ctx *c);
-int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new);
 const struct fwd_rule *fwd_rule_search(const struct fwd_table *fwd,
 				       const struct flowside *ini,
 				       uint8_t proto, int hint);
diff --git a/fwd_rule.c b/fwd_rule.c
index 9d489827..7bba2602 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -15,6 +15,7 @@
  * Author: David Gibson <david@gibson.dropbear.id.au>
  */
 
+#include <ctype.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <stdio.h>
@@ -89,7 +90,7 @@ parse_err:
  * fwd_port_map_ephemeral() - Mark ephemeral ports in a bitmap
  * @map:	Bitmap to update
  */
-void fwd_port_map_ephemeral(uint8_t *map)
+static void fwd_port_map_ephemeral(uint8_t *map)
 {
 	unsigned port;
 
@@ -123,6 +124,7 @@ const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule)
  */
 __attribute__((noinline))
 #endif
+/* cppcheck-suppress staticFunction */
 const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size)
 {
 	const char *percent = *rule->ifname ? "%" : "";
@@ -199,8 +201,8 @@ static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fwd_rule *
  * @rules:	Existing rules against which to test
  * @count:	Number of rules in @rules
  */
-void fwd_rule_conflict_check(const struct fwd_rule *new,
-			     const struct fwd_rule *rules, size_t count)
+static void fwd_rule_conflict_check(const struct fwd_rule *new,
+				    const struct fwd_rule *rules, size_t count)
 {
 	unsigned i;
 
@@ -215,3 +217,460 @@ void fwd_rule_conflict_check(const struct fwd_rule *new,
 		    fwd_rule_fmt(&rules[i], rulestr, sizeof(rulestr)));
 	}
 }
+
+/**
+ * fwd_rule_add() - Validate and add a rule to a forwarding table
+ * @fwd:	Table to add to
+ * @new:	Rule to add
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
+{
+	/* Flags which can be set from the caller */
+	const uint8_t allowed_flags = FWD_WEAK | FWD_SCAN | FWD_DUAL_STACK_ANY;
+	unsigned num = (unsigned)new->last - new->first + 1;
+	unsigned port;
+
+	if (new->first > new->last) {
+		warn("Rule has invalid port range %u-%u",
+		     new->first, new->last);
+		return -EINVAL;
+	}
+	if (!new->first) {
+		warn("Forwarding rule attempts to map from port 0");
+		return -EINVAL;
+	}
+	if (!new->to || (new->to + new->last - new->first) < new->to) {
+		warn("Forwarding rule attempts to map to port 0");
+		return -EINVAL;
+	}
+	if (new->flags & ~allowed_flags) {
+		warn("Rule has invalid flags 0x%hhx",
+		     new->flags & ~allowed_flags);
+		return -EINVAL;
+	}
+	if (new->flags & FWD_DUAL_STACK_ANY) {
+		if (!inany_equals(&new->addr, &inany_any6)) {
+			char astr[INANY_ADDRSTRLEN];
+
+			warn("Dual stack rule has non-wildcard address %s",
+			     inany_ntop(&new->addr, astr, sizeof(astr)));
+			return -EINVAL;
+		}
+		if (!(fwd->caps & FWD_CAP_IPV4)) {
+			warn("Dual stack forward, but IPv4 not enabled");
+			return -EINVAL;
+		}
+		if (!(fwd->caps & FWD_CAP_IPV6)) {
+			warn("Dual stack forward, but IPv6 not enabled");
+			return -EINVAL;
+		}
+	} else {
+		if (inany_v4(&new->addr) && !(fwd->caps & FWD_CAP_IPV4)) {
+			warn("IPv4 forward, but IPv4 not enabled");
+			return -EINVAL;
+		}
+		if (!inany_v4(&new->addr) && !(fwd->caps & FWD_CAP_IPV6)) {
+			warn("IPv6 forward, but IPv6 not enabled");
+			return -EINVAL;
+		}
+	}
+	if (new->proto == IPPROTO_TCP) {
+		if (!(fwd->caps & FWD_CAP_TCP)) {
+			warn("Can't add TCP forwarding rule, TCP not enabled");
+			return -EINVAL;
+		}
+	} else if (new->proto == IPPROTO_UDP) {
+		if (!(fwd->caps & FWD_CAP_UDP)) {
+			warn("Can't add UDP forwarding rule, UDP not enabled");
+			return -EINVAL;
+		}
+	} else {
+		warn("Unsupported protocol 0x%hhx (%s) for forwarding rule",
+		     new->proto, ipproto_name(new->proto));
+		return -EINVAL;
+	}
+
+	if (fwd->count >= ARRAY_SIZE(fwd->rules)) {
+		warn("Too many rules (maximum %u)", ARRAY_SIZE(fwd->rules));
+		return -ENOSPC;
+	}
+	if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) {
+		warn("Rules require too many listening sockets (maximum %u)",
+		     ARRAY_SIZE(fwd->socks));
+		return -ENOSPC;
+	}
+
+	fwd->rulesocks[fwd->count] = &fwd->socks[fwd->sock_count];
+	for (port = new->first; port <= new->last; port++)
+		fwd->rulesocks[fwd->count][port - new->first] = -1;
+
+	fwd->rules[fwd->count++] = *new;
+	fwd->sock_count += num;
+	return 0;
+}
+
+/**
+ * port_range() - Represents a non-empty range of ports
+ * @first:	First port number in the range
+ * @last:	Last port number in the range (inclusive)
+ *
+ * Invariant:	@last >= @first
+ */
+struct port_range {
+	in_port_t first, last;
+};
+
+/**
+ * parse_port_range() - Parse a range of port numbers '<first>[-<last>]'
+ * @s:		String to parse
+ * @endptr:	Update to the character after the parsed range (similar to
+ *		strtol() etc.)
+ * @range:	Update with the parsed values on success
+ *
+ * Return: -EINVAL on parsing error, -ERANGE on out of range port
+ *	   numbers, 0 on success
+ */
+static int parse_port_range(const char *s, const char **endptr,
+			    struct port_range *range)
+{
+	unsigned long first, last;
+	char *ep;
+
+	last = first = strtoul(s, &ep, 10);
+	if (ep == s) /* Parsed nothing */
+		return -EINVAL;
+	if (*ep == '-') { /* we have a last value too */
+		const char *lasts = ep + 1;
+		last = strtoul(lasts, &ep, 10);
+		if (ep == lasts) /* Parsed nothing */
+			return -EINVAL;
+	}
+
+	if ((last < first) || (last >= NUM_PORTS))
+		return -ERANGE;
+
+	range->first = first;
+	range->last = last;
+	*endptr = ep;
+
+	return 0;
+}
+
+/**
+ * parse_keyword() - Parse a literal keyword
+ * @s:		String to parse
+ * @endptr:	Update to the character after the keyword
+ * @kw:		Keyword to accept
+ *
+ * Return: 0, if @s starts with @kw, -EINVAL if it does not
+ */
+static int parse_keyword(const char *s, const char **endptr, const char *kw)
+{
+	size_t len = strlen(kw);
+
+	if (strlen(s) < len)
+		return -EINVAL;
+
+	if (memcmp(s, kw, len))
+		return -EINVAL;
+
+	*endptr = s + len;
+	return 0;
+}
+
+/**
+ * fwd_rule_range_except() - Set up forwarding for a range of ports minus a
+ *                           bitmap of exclusions
+ * @fwd:	Forwarding table to be updated
+ * @proto:	Protocol to forward
+ * @addr:	Listening address
+ * @ifname:	Listening interface
+ * @first:	First port to forward
+ * @last:	Last port to forward
+ * @exclude:	Bitmap of ports to exclude (may be NULL)
+ * @to:		Port to translate @first to when forwarding
+ * @flags:	Flags for forwarding entries
+ */
+static void fwd_rule_range_except(struct fwd_table *fwd, uint8_t proto,
+				  const union inany_addr *addr,
+				  const char *ifname,
+				  uint16_t first, uint16_t last,
+				  const uint8_t *exclude, uint16_t to,
+				  uint8_t flags)
+{
+	struct fwd_rule rule = {
+		.addr = addr ? *addr : inany_any6,
+		.ifname = { 0 },
+		.proto = proto,
+		.flags = flags,
+	};
+	char rulestr[FWD_RULE_STRLEN];
+	unsigned delta = to - first;
+	unsigned base, i;
+
+	if (!addr)
+		rule.flags |= FWD_DUAL_STACK_ANY;
+	if (ifname) {
+		int ret;
+
+		ret = snprintf(rule.ifname, sizeof(rule.ifname),
+			       "%s", ifname);
+		if (ret <= 0 || (size_t)ret >= sizeof(rule.ifname))
+			die("Invalid interface name: %s", ifname);
+	}
+
+	assert(first != 0);
+
+	for (base = first; base <= last; base++) {
+		if (exclude && bitmap_isset(exclude, base))
+			continue;
+
+		for (i = base; i <= last; i++) {
+			if (exclude && bitmap_isset(exclude, i))
+				break;
+		}
+
+		rule.first = base;
+		rule.last = i - 1;
+		rule.to = base + delta;
+
+		fwd_rule_conflict_check(&rule, fwd->rules, fwd->count);
+		if (fwd_rule_add(fwd, &rule) < 0)
+			goto fail;
+
+		base = i - 1;
+	}
+	return;
+
+fail:
+	die("Unable to add rule %s",
+	    fwd_rule_fmt(&rule, rulestr, sizeof(rulestr)));
+}
+
+/*
+ * for_each_chunk - Step through delimited chunks of a string
+ * @p_:		Pointer to start of each chunk (updated)
+ * @ep_:	Pointer to end of each chunk (updated)
+ * @s_:		String to step through
+ * @sep_:	String of all allowed delimiters
+ */
+#define for_each_chunk(p_, ep_, s_, sep_)			\
+	for ((p_) = (s_);					\
+	     (ep_) = (p_) + strcspn((p_), (sep_)), *(p_);	\
+	     (p_) = *(ep_) ? (ep_) + 1 : (ep_))
+
+/**
+ * fwd_rule_parse_ports() - Parse port range(s) specifier
+ * @fwd:	Forwarding table to be updated
+ * @proto:	Protocol to forward
+ * @addr:	Listening address for forwarding
+ * @ifname:	Interface name for listening
+ * @spec:	Port range(s) specifier
+ */
+static void fwd_rule_parse_ports(struct fwd_table *fwd, uint8_t proto,
+				 const union inany_addr *addr,
+				 const char *ifname,
+				 const char *spec)
+{
+	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
+	bool exclude_only = true;
+	const char *p, *ep;
+	uint8_t flags = 0;
+	unsigned i;
+
+	if (!strcmp(spec, "all")) {
+		/* Treat "all" as equivalent to "": all non-ephemeral ports */
+		spec = "";
+	}
+
+	/* Parse excluded ranges and "auto" in the first pass */
+	for_each_chunk(p, ep, spec, ",") {
+		struct port_range xrange;
+
+		if (isdigit(*p))  {
+			/* Include range, parse later */
+			exclude_only = false;
+			continue;
+		}
+
+		if (parse_keyword(p, &p, "auto") == 0) {
+			if (p != ep) /* Garbage after the keyword */
+				goto bad;
+
+			if (!(fwd->caps & FWD_CAP_SCAN)) {
+				die(
+"'auto' port forwarding is only allowed for pasta");
+			}
+
+			flags |= FWD_SCAN;
+			continue;
+		}
+
+		/* Should be an exclude range */
+		if (*p != '~')
+			goto bad;
+		p++;
+
+		if (parse_port_range(p, &p, &xrange))
+			goto bad;
+		if (p != ep) /* Garbage after the range */
+			goto bad;
+
+		for (i = xrange.first; i <= xrange.last; i++)
+			bitmap_set(exclude, i);
+	}
+
+	if (exclude_only) {
+		/* Exclude ephemeral ports */
+		fwd_port_map_ephemeral(exclude);
+
+		fwd_rule_range_except(fwd, proto, addr, ifname,
+				      1, NUM_PORTS - 1, exclude,
+				      1, flags | FWD_WEAK);
+		return;
+	}
+
+	/* Now process base ranges, skipping exclusions */
+	for_each_chunk(p, ep, spec, ",") {
+		struct port_range orig_range, mapped_range;
+
+		if (!isdigit(*p))
+			/* Already parsed */
+			continue;
+
+		if (parse_port_range(p, &p, &orig_range))
+			goto bad;
+
+		if (*p == ':') { /* There's a range to map to as well */
+			if (parse_port_range(p + 1, &p, &mapped_range))
+				goto bad;
+			if ((mapped_range.last - mapped_range.first) !=
+			    (orig_range.last - orig_range.first))
+				goto bad;
+		} else {
+			mapped_range = orig_range;
+		}
+
+		if (p != ep) /* Garbage after the ranges */
+			goto bad;
+
+		fwd_rule_range_except(fwd, proto, addr, ifname,
+				      orig_range.first, orig_range.last,
+				      exclude,
+				      mapped_range.first, flags);
+	}
+
+	return;
+bad:
+	die("Invalid port specifier '%s'", spec);
+}
+
+/**
+ * fwd_rule_parse() - Parse port configuration option
+ * @optname:	Short option name, t, T, u, or U
+ * @optarg:	Option argument (port specification)
+ * @fwd:	Forwarding table to be updated
+ */
+void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd)
+{
+	union inany_addr addr_buf = inany_any6, *addr = &addr_buf;
+	char buf[BUFSIZ], *spec, *ifname = NULL;
+	uint8_t proto;
+
+	if (optname == 't' || optname == 'T')
+		proto = IPPROTO_TCP;
+	else if (optname == 'u' || optname == 'U')
+		proto = IPPROTO_UDP;
+	else
+		assert(0);
+
+	if (!strcmp(optarg, "none")) {
+		unsigned i;
+
+		for (i = 0; i < fwd->count; i++) {
+			if (fwd->rules[i].proto == proto) {
+				die("-%c none conflicts with previous options",
+					optname);
+			}
+		}
+		return;
+	}
+
+	strncpy(buf, optarg, sizeof(buf) - 1);
+
+	if ((spec = strchr(buf, '/'))) {
+		*spec = 0;
+		spec++;
+
+		if (optname != 't' && optname != 'u')
+			die("Listening address not allowed for -%c %s",
+			    optname, optarg);
+
+		if ((ifname = strchr(buf, '%'))) {
+			*ifname = 0;
+			ifname++;
+
+			/* spec is already advanced one past the '/',
+			 * so the length of the given ifname is:
+			 * (spec - ifname - 1)
+			 */
+			if (spec - ifname - 1 >= IFNAMSIZ) {
+				die("Interface name '%s' is too long (max %u)",
+				    ifname, IFNAMSIZ - 1);
+			}
+		}
+
+		if (ifname == buf + 1) {	/* Interface without address */
+			addr = NULL;
+		} else {
+			char *p = buf;
+
+			/* Allow square brackets for IPv4 too for convenience */
+			if (*p == '[' && p[strlen(p) - 1] == ']') {
+				p[strlen(p) - 1] = '\0';
+				p++;
+			}
+
+			if (!inany_pton(p, addr))
+				die("Bad forwarding address '%s'", p);
+		}
+	} else {
+		spec = buf;
+
+		addr = NULL;
+	}
+
+	if (optname == 'T' || optname == 'U') {
+		assert(!addr && !ifname);
+
+		if (!(fwd->caps & FWD_CAP_IFNAME)) {
+			warn(
+"SO_BINDTODEVICE unavailable, forwarding only 127.0.0.1 and ::1 for '-%c %s'",
+			     optname, optarg);
+
+			if (fwd->caps & FWD_CAP_IPV4) {
+				fwd_rule_parse_ports(fwd, proto,
+						     &inany_loopback4, NULL,
+						     spec);
+			}
+			if (fwd->caps & FWD_CAP_IPV6) {
+				fwd_rule_parse_ports(fwd, proto,
+						     &inany_loopback6, NULL,
+						     spec);
+			}
+			return;
+		}
+
+		ifname = "lo";
+	}
+
+	if (ifname && !(fwd->caps & FWD_CAP_IFNAME)) {
+		die(
+"Device binding for '-%c %s' unsupported (requires kernel 5.7+)",
+		    optname, optarg);
+	}
+
+	fwd_rule_parse_ports(fwd, proto, addr, ifname, spec);
+}
diff --git a/fwd_rule.h b/fwd_rule.h
index 5c7b67aa..f0f4efda 100644
--- a/fwd_rule.h
+++ b/fwd_rule.h
@@ -19,6 +19,7 @@
 
 /* Number of ports for both TCP and UDP */
 #define	NUM_PORTS	(1U << 16)
+#define PORT_BITMAP_SIZE	DIV_ROUND_UP(NUM_PORTS, 8)
 
 /* Forwarding capability bits */
 #define FWD_CAP_IPV4		BIT(0)
@@ -54,8 +55,38 @@ struct fwd_rule {
 	uint8_t flags;
 };
 
+#define FWD_RULE_BITS	8
+#define MAX_FWD_RULES	MAX_FROM_BITS(FWD_RULE_BITS)
+
+/* Maximum number of listening sockets (per pif)
+ *
+ * Rationale: This lets us listen on every port for two addresses and two
+ * protocols (which we need for -T auto -U auto without SO_BINDTODEVICE), plus a
+ * comfortable number of extras.
+ */
+#define MAX_LISTEN_SOCKS	(NUM_PORTS * 5)
+
+/**
+ * struct fwd_table - Forwarding state (per initiating pif)
+ * @caps:	Forwarding capabilities for this initiating pif
+ * @count:	Number of forwarding rules
+ * @rules:	Array of forwarding rules
+ * @rulesocks:	Parallel array of @rules (@count valid entries) of pointers to
+ *		@socks entries giving the start of the corresponding rule's
+ *		sockets within the larger array
+ * @sock_count:	Number of entries used in @socks (for all rules combined)
+ * @socks:	Listening sockets for forwarding
+ */
+struct fwd_table {
+	uint32_t caps;
+	unsigned count;
+	struct fwd_rule rules[MAX_FWD_RULES];
+	int *rulesocks[MAX_FWD_RULES];
+	unsigned sock_count;
+	int socks[MAX_LISTEN_SOCKS];
+};
+
 void fwd_probe_ephemeral(void);
-void fwd_port_map_ephemeral(uint8_t *map);
 
 #define FWD_RULE_STRLEN					    \
 	(IPPROTO_STRLEN - 1				    \
@@ -67,7 +98,6 @@ void fwd_port_map_ephemeral(uint8_t *map);
 const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule);
 const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size);
 void fwd_rules_info(const struct fwd_rule *rules, size_t count);
-void fwd_rule_conflict_check(const struct fwd_rule *new,
-			     const struct fwd_rule *rules, size_t count);
+void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd);
 
 #endif /* FWD_RULE_H */
-- 
2.53.0

[PATCH v3 10/11] fwd_rule: Move conflict checking back within fwd_rule_add()

[David Gibson]

2bffb631d31e ("fwd_rule: Move rule conflict checking from fwd_rule_add()
to caller") moved rule conflict checking out of fwd_rule_add().  This
seemed like a good idea at the time, but turns out to be kind of awkward:
it means we're now checking for conflicts *before* we've checked the rule
for internal consistency (including first <= last), which leaves an awkward
assert() which might fire in unexpected places.

While it's true that it's not really necessary to include this in order to
safely add a rule, the benefits from skipping it are pretty marginal.  So,
for simplicity, fold this check back into fwd_rule_add(), making it
non-fatal.  If we ever have cases with enough rules that the O(n^2) nature
of the check matters, we might need to revisit.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd_rule.c | 38 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/fwd_rule.c b/fwd_rule.c
index 7bba2602..cb462401 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -195,29 +195,6 @@ static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fwd_rule *
 	return true;
 }
 
-/**
- * fwd_rule_conflict_check() - Die if given rule conflicts with any in list
- * @new:	New rule
- * @rules:	Existing rules against which to test
- * @count:	Number of rules in @rules
- */
-static void fwd_rule_conflict_check(const struct fwd_rule *new,
-				    const struct fwd_rule *rules, size_t count)
-{
-	unsigned i;
-
-	for (i = 0; i < count; i++) {
-		char newstr[FWD_RULE_STRLEN], rulestr[FWD_RULE_STRLEN];
-
-		if (!fwd_rule_conflicts(new, &rules[i]))
-			continue;
-
-		die("Forwarding configuration conflict: %s versus %s",
-		    fwd_rule_fmt(new, newstr, sizeof(newstr)),
-		    fwd_rule_fmt(&rules[i], rulestr, sizeof(rulestr)));
-	}
-}
-
 /**
  * fwd_rule_add() - Validate and add a rule to a forwarding table
  * @fwd:	Table to add to
@@ -230,7 +207,7 @@ static int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
 	/* Flags which can be set from the caller */
 	const uint8_t allowed_flags = FWD_WEAK | FWD_SCAN | FWD_DUAL_STACK_ANY;
 	unsigned num = (unsigned)new->last - new->first + 1;
-	unsigned port;
+	unsigned port, i;
 
 	if (new->first > new->last) {
 		warn("Rule has invalid port range %u-%u",
@@ -292,6 +269,18 @@ static int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
 		return -EINVAL;
 	}
 
+	for (i = 0; i < fwd->count; i++) {
+		char newstr[FWD_RULE_STRLEN], rulestr[FWD_RULE_STRLEN];
+
+		if (!fwd_rule_conflicts(new, &fwd->rules[i]))
+			continue;
+
+		warn("Forwarding configuration conflict: %s versus %s",
+		     fwd_rule_fmt(new, newstr, sizeof(newstr)),
+		     fwd_rule_fmt(&fwd->rules[i], rulestr, sizeof(rulestr)));
+		return -EEXIST;
+	}
+
 	if (fwd->count >= ARRAY_SIZE(fwd->rules)) {
 		warn("Too many rules (maximum %u)", ARRAY_SIZE(fwd->rules));
 		return -ENOSPC;
@@ -436,7 +425,6 @@ static void fwd_rule_range_except(struct fwd_table *fwd, uint8_t proto,
 		rule.last = i - 1;
 		rule.to = base + delta;
 
-		fwd_rule_conflict_check(&rule, fwd->rules, fwd->count);
 		if (fwd_rule_add(fwd, &rule) < 0)
 			goto fail;
 
-- 
2.53.0

[PATCH v3 11/11] fwd: Generalise fwd_rules_info()

[David Gibson]

fwd_rules_info() is used to print a full table of forwarding rules for
debugging or the like.  Currently it has one caller, and uses info() to
dump the messages.  However for the upcoming configuration client, we're
going to want to dump the rules in some similar, but not quite identical
ways.  For example, at different severity levels, or to stdout instead of
stderr / system log / logfile.

So, generalise fwd_rules_info() to fwd_rules_dump() which takes a printing
function as a parameter.  Because we want this to work with "functions"
like info, which is actually a macro, we have to convert fwd_rules_dump()
to a macro as well.  We also allow the prefix and suffix for each rule /
line to be provided as a parameter.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c     |  3 ++-
 fwd_rule.c | 16 ----------------
 fwd_rule.h | 20 +++++++++++++++++++-
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/conf.c b/conf.c
index 5aacfe0f..05861072 100644
--- a/conf.c
+++ b/conf.c
@@ -902,7 +902,8 @@ dns6:
 			dir = "Inbound";
 
 		info("%s forwarding rules (%s):", dir, pif_name(i));
-		fwd_rules_info(c->fwd[i]->rules, c->fwd[i]->count);
+		fwd_rules_dump(info, c->fwd[i]->rules, c->fwd[i]->count,
+			       "    ", "");
 	}
 }
 
diff --git a/fwd_rule.c b/fwd_rule.c
index cb462401..0d2e1531 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -124,7 +124,6 @@ const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule)
  */
 __attribute__((noinline))
 #endif
-/* cppcheck-suppress staticFunction */
 const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size)
 {
 	const char *percent = *rule->ifname ? "%" : "";
@@ -158,21 +157,6 @@ const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size)
 	return dst;
 }
 
-/**
- * fwd_rules_info() - Print forwarding rules for debugging
- * @fwd:	Table to print
- */
-void fwd_rules_info(const struct fwd_rule *rules, size_t count)
-{
-	unsigned i;
-
-	for (i = 0; i < count; i++) {
-		char buf[FWD_RULE_STRLEN];
-
-		info("    %s", fwd_rule_fmt(&rules[i], buf, sizeof(buf)));
-	}
-}
-
 /**
  * fwd_rule_conflicts() - Test if two rules conflict with each other
  * @a, @b:	Rules to test
diff --git a/fwd_rule.h b/fwd_rule.h
index f0f4efda..58551382 100644
--- a/fwd_rule.h
+++ b/fwd_rule.h
@@ -97,7 +97,25 @@ void fwd_probe_ephemeral(void);
 
 const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule);
 const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size);
-void fwd_rules_info(const struct fwd_rule *rules, size_t count);
 void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd);
 
+/**
+ * fwd_rules_dump() - Dump forwarding rules
+ * @fn:		Printing/logging function to call
+ * @rules:	Array of rules to dump
+ * @count:	Number of rules to dump
+ * @prefix:	String to print at the start of each rule
+ * @suffix:	String to print at the end of each rule
+ */
+#define fwd_rules_dump(fn, rules, count, prefix, suffix)		\
+	do {								\
+		unsigned i_;						\
+		for (i_ = 0; i_ < (count); i_++) {			\
+			char buf_[FWD_RULE_STRLEN];			\
+			fn("%s%s%s", prefix,				\
+			   fwd_rule_fmt(&(rules)[i_], buf_, sizeof(buf_)), \
+			   suffix);					\
+		}							\
+	} while (0)
+
 #endif /* FWD_RULE_H */
-- 
2.53.0