The man page currently has two fairly large, near-identical sections separately describing the -t and -u options for passt and pasta. This is bulky and potentially confusing. It will make this information more tedious to update as we alter what's possible here with the forwarding table. Consolidate both descriptions to a single one in the common options, noting the few passt/pasta difference inline. There's similar duplication usage(), consolidate that as well. Signed-off-by: David Gibson --- conf.c | 54 +++++--------- passt.1 | 216 ++++++++++++++++++-------------------------------------- 2 files changed, 85 insertions(+), 185 deletions(-) diff --git a/conf.c b/conf.c index f3b36bb6..751e500f 100644 --- a/conf.c +++ b/conf.c @@ -1023,18 +1023,12 @@ static void usage(const char *name, FILE *f, int status) " --freebind Bind to any address for forwarding\n" " --no-map-gw Don't map gateway address to host\n" " -4, --ipv4-only Enable IPv4 operation only\n" - " -6, --ipv6-only Enable IPv6 operation only\n"); - - if (strstr(name, "pasta")) - goto pasta_opts; - - FPRINTF(f, - " -1, --one-off Quit after handling one single client\n" + " -6, --ipv6-only Enable IPv6 operation only\n" " -t, --tcp-ports SPEC TCP port forwarding to guest\n" " 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" " a comma-separated list, optionally ranged with '-'\n" " and optional target ports after ':', with optional\n" " address specification suffixed by '/' and optional\n" @@ -1050,43 +1044,29 @@ 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 guest\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" - " default: none\n" + " default: %s\n" " -u, --udp-ports SPEC UDP port forwarding to guest\n" " SPEC is as described for TCP above\n" - " default: none\n"); + " default: %s\n", + strstr(name, "pasta") ? + " 'auto': forward all ports currently bound in namespace\n" + : + " 'all': forward all unbound, non-ephemeral ports\n", + strstr(name, "pasta") ? "auto" : "none", + strstr(name, "pasta") ? "auto" : "none"); + + if (strstr(name, "pasta")) + goto pasta_opts; + + FPRINTF(f, + " -1, --one-off Quit after handling one single client\n" + ); passt_exit(status); pasta_opts: FPRINTF(f, - " -t, --tcp-ports SPEC TCP port forwarding to namespace\n" - " can be specified multiple times\n" - " SPEC can be:\n" - " 'none': don't forward any ports\n" - " 'auto': forward all ports currently bound in namespace\n" - " 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 '%%'. Examples:\n" - " -t 22 Forward local port 22 to port 22 in netns\n" - " -t 22:23 Forward local port 22 to port 23\n" - " -t 22,25 Forward ports 22, 25 to ports 22, 25\n" - " -t 22-80 Forward ports 22 to 80\n" - " -t 22-80:32-90 Forward ports 22 to 80 to\n" - " corresponding port numbers plus 10\n" - " -t 192.0.2.1/5 Bind port 5 of 192.0.2.1 to namespace\n" - " -t 5-25,~10-20 Forward ports 5 to 9, and 21 to 25\n" - " -t ~25 Forward all bound ports except for 25\n" - " default: auto\n" - " IPv6 bound ports are also forwarded for IPv4\n" - " -u, --udp-ports SPEC UDP port forwarding to namespace\n" - " SPEC is as described for TCP above\n" - " default: auto\n" - " IPv6 bound ports are also forwarded for IPv4\n" - " unless specified, with '-t auto', UDP ports with numbers\n" - " corresponding to forwarded TCP port numbers are\n" - " forwarded too\n" " -T, --tcp-ns SPEC TCP port forwarding to init namespace\n" " SPEC is as described above\n" " default: auto\n" diff --git a/passt.1 b/passt.1 index 13e8df9d..a9a8a42a 100644 --- a/passt.1 +++ b/passt.1 @@ -425,78 +425,6 @@ Send \fIname\fR as DHCP option 12 (hostname). FQDN to configure the client with. Send \fIname\fR as Client FQDN: DHCP option 81 and DHCPv6 option 39. -.SS \fBpasst\fR-only options - -.TP -.BR \-s ", " \-\-socket-path ", " \-\-socket " " \fIpath -Path for UNIX domain socket used by \fBqemu\fR(1) or \fBqrap\fR(1) to connect to -\fBpasst\fR. -Default is to probe a free socket, not accepting connections, starting from -\fI/tmp/passt_1.socket\fR to \fI/tmp/passt_64.socket\fR. - -.TP -.BR \-\-vhost-user -Enable vhost-user. The vhost-user command socket is provided by \fB--socket\fR. - -.TP -.BR \-\-print-capabilities -Print back-end capabilities in JSON format, only meaningful for vhost-user mode. - -.TP -.BR \-\-repair-path " " \fIpath -Path for UNIX domain socket used by the \fBpasst-repair\fR(1) helper to connect -to \fBpasst\fR in order to set or clear the TCP_REPAIR option on sockets, during -migration. \fB--repair-path none\fR disables this interface (if you need to -specify a socket path called "none" you can prefix the path by \fI./\fR). - -Default, for \-\-vhost-user mode only, is to append \fI.repair\fR to the path -chosen for the hypervisor UNIX domain socket. No socket is created if not in -\-\-vhost-user mode. - -.TP -.BR \-\-migrate-exit " " (DEPRECATED) -Exit after a completed migration as source. By default, \fBpasst\fR keeps -running and the migrated guest can continue using its connection, or a new guest -can connect. - -Note that this configuration option is \fBdeprecated\fR and will be removed in a -future version. It is not expected to be of any use, and it simply reflects a -legacy behaviour. If you have any use for this, refer to \fBREPORTING BUGS\fR -below. - -.TP -.BR \-\-migrate-no-linger " " (DEPRECATED) -Close TCP sockets on the source instance once migration completes. - -By default, sockets are kept open, and events on data sockets are ignored, so -that any further message reaching sockets after the source migrated is silently -ignored, to avoid connection resets in case data is received after migration. - -Note that this configuration option is \fBdeprecated\fR and will be removed in a -future version. It is not expected to be of any use, and it simply reflects a -legacy behaviour. If you have any use for this, refer to \fBREPORTING BUGS\fR -below. - -.TP -.BR \-F ", " \-\-fd " " \fIFD -Pass a pre-opened, connected socket to \fBpasst\fR. Usually the socket is opened -in the parent process and \fBpasst\fR inherits it when run as a child. This -allows the parent process to open sockets using another address family or -requiring special privileges. - -This option implies the behaviour described for \-\-one-off, once this socket -is closed. - -.TP -.BR \-1 ", " \-\-one-off -Quit after handling a single client connection, that is, once the client closes -the socket, or once we get a socket error. - -\fBNote\fR: this option has no effect after \fBpasst\fR completes a migration as -source, because, in that case, exiting would close sockets for active -connections, which would in turn cause connection resets if any further data is -received. See also the description of \fI\-\-migrate-no-linger\fR. - .TP .BR \-t ", " \-\-tcp-ports " " \fIspec Configure TCP port forwarding to guest. \fIspec\fR can be one of: @@ -507,11 +435,17 @@ Configure TCP port forwarding to guest. \fIspec\fR can be one of: Don't forward any ports .TP -.BR all +.BR all (\fBpasst\fR only) 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 +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, @@ -563,7 +497,7 @@ and 30 Forward all ports to the guest, except for the range from 20000 to 20010 .RE -Default is \fBnone\fR. +Default is \fBnone\fR for \fBpasst\fR and \fBauto\fR for \fBpasta\fR. .RE .TP @@ -575,101 +509,87 @@ Note: unless overridden, UDP ports with numbers corresponding to forwarded TCP port numbers are forwarded too, without, however, any port translation. IPv6 bound ports are also forwarded for IPv4. -Default is \fBnone\fR. +Default is \fBnone\fR for \fBpasst\fR and \fBauto\fR for \fBpasta\fR. -.SS \fBpasta\fR-only options +.SS \fBpasst\fR-only options .TP -.BR \-I ", " \-\-ns-ifname " " \fIname -Name of tap interface to be created in target namespace. -By default, the same interface name as the external, routable interface is used. -If no such interface exists, the name \fItap0\fR will be used instead. +.BR \-s ", " \-\-socket-path ", " \-\-socket " " \fIpath +Path for UNIX domain socket used by \fBqemu\fR(1) or \fBqrap\fR(1) to connect to +\fBpasst\fR. +Default is to probe a free socket, not accepting connections, starting from +\fI/tmp/passt_1.socket\fR to \fI/tmp/passt_64.socket\fR. .TP -.BR \-t ", " \-\-tcp-ports " " \fIspec -Configure TCP port forwarding to namespace. \fIspec\fR can be one of: -.RS +.BR \-\-vhost-user +Enable vhost-user. The vhost-user command socket is provided by \fB--socket\fR. .TP -.BR none -Don't forward any ports +.BR \-\-print-capabilities +Print back-end capabilities in JSON format, only meaningful for vhost-user mode. .TP -.BR auto -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). +.BR \-\-repair-path " " \fIpath +Path for UNIX domain socket used by the \fBpasst-repair\fR(1) helper to connect +to \fBpasst\fR in order to set or clear the TCP_REPAIR option on sockets, during +migration. \fB--repair-path none\fR disables this interface (if you need to +specify a socket path called "none" you can prefix the path by \fI./\fR). + +Default, for \-\-vhost-user mode only, is to append \fI.repair\fR to the path +chosen for the hypervisor UNIX domain socket. No socket is created if not in +\-\-vhost-user mode. .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. +.BR \-\-migrate-exit " " (DEPRECATED) +Exit after a completed migration as source. By default, \fBpasst\fR keeps +running and the migrated guest can continue using its connection, or a new guest +can connect. -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. +Note that this configuration option is \fBdeprecated\fR and will be removed in a +future version. It is not expected to be of any use, and it simply reflects a +legacy behaviour. If you have any use for this, refer to \fBREPORTING BUGS\fR +below. -Examples: -.RS -.TP --t 22 -Forward local port 22 to 22 in the target namespace -.TP --t 22:23 -Forward local port 22 to port 23 in the target namespace -.TP --t 22,25 -Forward local ports 22 and 25 to ports 22 and 25 in the target namespace -.TP --t 22-80 -Forward local ports between 22 and 80 to corresponding ports in the target -namespace -.TP --t 22-80:32-90 -Forward local ports between 22 and 80 to ports between 32 and 90 in the target -namespace -.TP --t 192.0.2.1/22 -Forward local port 22, bound to 192.0.2.1, to port 22 in the target namespace -.TP --t 192.0.2.1%eth0/22 -Forward local port 22, bound to 192.0.2.1 and interface eth0, to port 22 -.TP --t %eth0/22 -Forward local port 22, bound to any address on interface eth0, to port 22 -.TP --t 2000-5000,~3000-3010 -Forward local ports between 2000 and 5000, except for those between 3000 and -3010 -.TP --t 192.0.2.1/20-30,~25 -For the local address 192.0.2.1, forward ports between 20 and 24 and between 26 -and 30 .TP --t ~20000-20010 -Forward all ports to the namespace, except for those between 20000 and 20010 -.RE +.BR \-\-migrate-no-linger " " (DEPRECATED) +Close TCP sockets on the source instance once migration completes. -IPv6 bound ports are also forwarded for IPv4. +By default, sockets are kept open, and events on data sockets are ignored, so +that any further message reaching sockets after the source migrated is silently +ignored, to avoid connection resets in case data is received after migration. -Default is \fBauto\fR. -.RE +Note that this configuration option is \fBdeprecated\fR and will be removed in a +future version. It is not expected to be of any use, and it simply reflects a +legacy behaviour. If you have any use for this, refer to \fBREPORTING BUGS\fR +below. .TP -.BR \-u ", " \-\-udp-ports " " \fIspec -Configure UDP port forwarding to namespace. \fIspec\fR is as described for TCP -above, and the list of ports is derived from listening sockets reported by -\fI/proc/net/udp\fR and \fI/proc/net/udp6\fR, see \fBproc\fR(5). +.BR \-F ", " \-\-fd " " \fIFD +Pass a pre-opened, connected socket to \fBpasst\fR. Usually the socket is opened +in the parent process and \fBpasst\fR inherits it when run as a child. This +allows the parent process to open sockets using another address family or +requiring special privileges. -Note: unless overridden, UDP ports with numbers corresponding to forwarded TCP -port numbers are forwarded too, without, however, any port translation. +This option implies the behaviour described for \-\-one-off, once this socket +is closed. -IPv6 bound ports are also forwarded for IPv4. +.TP +.BR \-1 ", " \-\-one-off +Quit after handling a single client connection, that is, once the client closes +the socket, or once we get a socket error. -Default is \fBauto\fR. +\fBNote\fR: this option has no effect after \fBpasst\fR completes a migration as +source, because, in that case, exiting would close sockets for active +connections, which would in turn cause connection resets if any further data is +received. See also the description of \fI\-\-migrate-no-linger\fR. + +.SS \fBpasta\fR-only options + +.TP +.BR \-I ", " \-\-ns-ifname " " \fIname +Name of tap interface to be created in target namespace. +By default, the same interface name as the external, routable interface is used. +If no such interface exists, the name \fItap0\fR will be used instead. .TP .BR \-T ", " \-\-tcp-ns " " \fIspec -- 2.53.0

Amongst other checks, fwd_rule_add() checks that the newly added rule doesn't conflict with any existing rules. However, unlike the other things we verify, this isn't really required for safe operation. Rule conflicts are a useful thing for the user to know about, but the forwarding logic is perfectly sound with conflicting rules (the first one will win). In order to support dynamic rule updates, we want fwd_rule_add() to become a more low-level function, only checking the things it really needs to. So, move rule conflict checking to its caller via new helpers in fwd_rule.c. Signed-off-by: David Gibson --- conf.c | 5 +++++ fwd.c | 26 +------------------------- fwd_rule.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ fwd_rule.h | 2 ++ 4 files changed, 52 insertions(+), 25 deletions(-) diff --git a/conf.c b/conf.c index c9ee8c59..a93837cc 100644 --- a/conf.c +++ b/conf.c @@ -205,13 +205,18 @@ static void conf_ports_range_except(const struct ctx *c, char optname, if (c->ifi4) { rulev.addr = inany_loopback4; + fwd_rule_conflict_check(&rulev, + fwd->rules, fwd->count); fwd_rule_add(fwd, &rulev); } if (c->ifi6) { rulev.addr = inany_loopback6; + fwd_rule_conflict_check(&rulev, + fwd->rules, fwd->count); fwd_rule_add(fwd, &rulev); } } else { + fwd_rule_conflict_check(&rule, fwd->rules, fwd->count); fwd_rule_add(fwd, &rule); } base = i - 1; diff --git a/fwd.c b/fwd.c index c05107d1..c9637525 100644 --- a/fwd.c +++ b/fwd.c @@ -341,7 +341,7 @@ void 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 i, port; + unsigned port; assert(!(new->flags & ~allowed_flags)); /* Passing a non-wildcard address with DUAL_STACK_ANY is a bug */ @@ -354,30 +354,6 @@ void fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new) if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) die("Too many listening sockets"); - /* Check for any conflicting entries */ - for (i = 0; i < fwd->count; i++) { - char newstr[INANY_ADDRSTRLEN], rulestr[INANY_ADDRSTRLEN]; - const struct fwd_rule *rule = &fwd->rules[i]; - - if (new->proto != rule->proto) - /* Non-conflicting protocols */ - continue; - - if (!inany_matches(fwd_rule_addr(new), fwd_rule_addr(rule))) - /* Non-conflicting addresses */ - continue; - - if (new->last < rule->first || rule->last < new->first) - /* Port ranges don't overlap */ - continue; - - die("Forwarding configuration conflict: %s/%u-%u versus %s/%u-%u", - inany_ntop(fwd_rule_addr(new), newstr, sizeof(newstr)), - new->first, new->last, - inany_ntop(fwd_rule_addr(rule), rulestr, sizeof(rulestr)), - rule->first, rule->last); - } - 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; diff --git a/fwd_rule.c b/fwd_rule.c index abe9dfbf..4d5048f9 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -87,3 +87,47 @@ void fwd_rules_info(const struct fwd_rule *rules, size_t count) 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 + */ +static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fwd_rule *b) +{ + if (a->proto != b->proto) + /* Non-conflicting protocols */ + return false; + + if (!inany_matches(fwd_rule_addr(a), fwd_rule_addr(b))) + /* Non-conflicting addresses */ + return false; + + assert(a->first <= a->last && b->first <= b->last); + if (a->last < b->first || b->last < a->first) + /* Port ranges don't overlap */ + return false; + + return true; +} + +/* fwd_rule_conflict_check() - Die with errir if rule conflicts with any in list + * @new: New 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) +{ + 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))); + } +} diff --git a/fwd_rule.h b/fwd_rule.h index e92efb6d..f852be39 100644 --- a/fwd_rule.h +++ b/fwd_rule.h @@ -52,5 +52,7 @@ struct fwd_rule { const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule); 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); #endif /* FWD_RULE_H */ -- 2.53.0

Currently we explicitly forbid -[tTuU] all in pasta mode. While these are primarily useful for passt, there's no particular reason they can't be used in pasta mode as well. Indeed you can do the same thing in pasta by using "-t ~32768-60999" (assuming default Linux configuration of ephemeral ports). For consistency, permit "all" for pasta as well. Signed-off-by: David Gibson --- conf.c | 7 ++----- passt.1 | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/conf.c b/conf.c index 751e500f..870712af 100644 --- a/conf.c +++ b/conf.c @@ -358,9 +358,6 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, if (*mode) goto mode_conflict; - if (c->mode == MODE_PASTA) - die("'all' port forwarding is only allowed for passt"); - *mode = FWD_MODE_ALL; /* Exclude ephemeral ports */ @@ -1028,6 +1025,7 @@ 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" " a comma-separated list, optionally ranged with '-'\n" " and optional target ports after ':', with optional\n" @@ -1050,8 +1048,7 @@ static void usage(const char *name, FILE *f, int status) " default: %s\n", strstr(name, "pasta") ? " 'auto': forward all ports currently bound in namespace\n" - : - " 'all': forward all unbound, non-ephemeral ports\n", + : "", strstr(name, "pasta") ? "auto" : "none", strstr(name, "pasta") ? "auto" : "none"); diff --git a/passt.1 b/passt.1 index a9a8a42a..44113929 100644 --- a/passt.1 +++ b/passt.1 @@ -435,7 +435,7 @@ Configure TCP port forwarding to guest. \fIspec\fR can be one of: Don't forward any ports .TP -.BR all (\fBpasst\fR only) +.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. -- 2.53.0

6dad076df037 ("fwd: Split forwarding rule specification from its implementation state") created struct fwd_rule_state with a forwarding rule plus the table of sockets used for its implementation. It turns out this is quite awkward for sharing rule parsing code between passt and the upcoming configuration client. Instead keep the index of listening sockets in a parallel array in struct fwd_table. Signed-off-by: David Gibson --- fwd.c | 73 ++++++++++++++++++++++++++++++----------------------------- fwd.h | 16 ++++--------- 2 files changed, 41 insertions(+), 48 deletions(-) diff --git a/fwd.c b/fwd.c index e09b42fe..7e0edc38 100644 --- a/fwd.c +++ b/fwd.c @@ -363,7 +363,7 @@ void fwd_rule_add(struct fwd_table *fwd, uint8_t proto, uint8_t flags, /* Flags which can be set from the caller */ const uint8_t allowed_flags = FWD_WEAK | FWD_SCAN | FWD_DUAL_STACK_ANY; unsigned num = (unsigned)last - first + 1; - struct fwd_rule_state *new; + struct fwd_rule *new; unsigned i, port; assert(!(flags & ~allowed_flags)); @@ -379,7 +379,7 @@ void fwd_rule_add(struct fwd_table *fwd, uint8_t proto, uint8_t flags, /* Check for any conflicting entries */ for (i = 0; i < fwd->count; i++) { char newstr[INANY_ADDRSTRLEN], rulestr[INANY_ADDRSTRLEN]; - const struct fwd_rule *rule = &fwd->rules[i].rule; + const struct fwd_rule *rule = &fwd->rules[i]; if (proto != rule->proto) /* Non-conflicting protocols */ @@ -399,38 +399,38 @@ void fwd_rule_add(struct fwd_table *fwd, uint8_t proto, uint8_t flags, rule->first, rule->last); } - new = &fwd->rules[fwd->count++]; - new->rule.proto = proto; - new->rule.flags = flags; + new = &fwd->rules[fwd->count]; + new->proto = proto; + new->flags = flags; if (addr) { - new->rule.addr = *addr; + new->addr = *addr; } else { - new->rule.addr = inany_any6; - new->rule.flags |= FWD_DUAL_STACK_ANY; + new->addr = inany_any6; + new->flags |= FWD_DUAL_STACK_ANY; } - memset(new->rule.ifname, 0, sizeof(new->rule.ifname)); + memset(new->ifname, 0, sizeof(new->ifname)); if (ifname) { int ret; - ret = snprintf(new->rule.ifname, sizeof(new->rule.ifname), + ret = snprintf(new->ifname, sizeof(new->ifname), "%s", ifname); - if (ret <= 0 || (size_t)ret >= sizeof(new->rule.ifname)) + if (ret <= 0 || (size_t)ret >= sizeof(new->ifname)) die("Invalid interface name: %s", ifname); } assert(first <= last); - new->rule.first = first; - new->rule.last = last; + new->first = first; + new->last = last; + new->to = to; - new->rule.to = to; + 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; - new->socks = &fwd->socks[fwd->sock_count]; + fwd->count++; fwd->sock_count += num; - - for (port = new->rule.first; port <= new->rule.last; port++) - new->socks[port - new->rule.first] = -1; } /** @@ -466,7 +466,7 @@ const struct fwd_rule *fwd_rule_search(const struct fwd_table *fwd, if (hint >= 0) { char ostr[INANY_ADDRSTRLEN], rstr[INANY_ADDRSTRLEN]; - const struct fwd_rule *rule = &fwd->rules[hint].rule; + const struct fwd_rule *rule = &fwd->rules[hint]; assert((unsigned)hint < fwd->count); if (fwd_rule_match(rule, ini, proto)) @@ -480,8 +480,8 @@ const struct fwd_rule *fwd_rule_search(const struct fwd_table *fwd, } for (i = 0; i < fwd->count; i++) { - if (fwd_rule_match(&fwd->rules[i].rule, ini, proto)) - return &fwd->rules[i].rule; + if (fwd_rule_match(&fwd->rules[i], ini, proto)) + return &fwd->rules[i]; } return NULL; @@ -496,7 +496,7 @@ void fwd_rules_print(const struct fwd_table *fwd) unsigned i; for (i = 0; i < fwd->count; i++) { - const struct fwd_rule *rule = &fwd->rules[i].rule; + const struct fwd_rule *rule = &fwd->rules[i]; const char *percent = *rule->ifname ? "%" : ""; const char *weak = "", *scan = ""; char addr[INANY_ADDRSTRLEN]; @@ -533,9 +533,9 @@ void fwd_rules_print(const struct fwd_table *fwd) static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx, const uint8_t *tcp, const uint8_t *udp) { - const struct fwd_rule_state *rs = &c->fwd[pif]->rules[idx]; - const struct fwd_rule *rule = &rs->rule; + const struct fwd_rule *rule = &c->fwd[pif]->rules[idx]; const union inany_addr *addr = fwd_rule_addr(rule); + int *socks = c->fwd[pif]->rulesocks[idx]; const char *ifname = rule->ifname; const uint8_t *map = NULL; bool bound_one = false; @@ -555,7 +555,7 @@ static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx, } for (port = rule->first; port <= rule->last; port++) { - int fd = rs->socks[port - rule->first]; + int fd = socks[port - rule->first]; if (map && !bitmap_isset(map, port)) { /* We don't want to listen on this port */ @@ -563,7 +563,7 @@ static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx, /* We already are, so stop */ epoll_del(c->epollfd, fd); close(fd); - rs->socks[port - rule->first] = -1; + socks[port - rule->first] = -1; } continue; } @@ -595,7 +595,7 @@ static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx, continue; } - rs->socks[port - rule->first] = fd; + socks[port - rule->first] = fd; bound_one = true; } @@ -685,11 +685,12 @@ void fwd_listen_close(const struct fwd_table *fwd) unsigned i; for (i = 0; i < fwd->count; i++) { - const struct fwd_rule_state *rs = &fwd->rules[i]; + const struct fwd_rule *rule = &fwd->rules[i]; + int *socks = fwd->rulesocks[i]; unsigned port; - for (port = rs->rule.first; port <= rs->rule.last; port++) { - int *fdp = &rs->socks[port - rs->rule.first]; + for (port = rule->first; port <= rule->last; port++) { + int *fdp = &socks[port - rule->first]; if (*fdp >= 0) { close(*fdp); *fdp = -1; @@ -769,8 +770,8 @@ static bool has_scan_rules(const struct fwd_table *fwd, uint8_t proto) unsigned i; for (i = 0; i < fwd->count; i++) { - if (fwd->rules[i].rule.proto == proto && - fwd->rules[i].rule.flags & FWD_SCAN) + if (fwd->rules[i].proto == proto && + fwd->rules[i].flags & FWD_SCAN) return true; } return false; @@ -838,14 +839,14 @@ static void current_listen_map(uint8_t *map, const struct fwd_table *fwd, memset(map, 0, PORT_BITMAP_SIZE); for (i = 0; i < fwd->count; i++) { - const struct fwd_rule_state *rs = &fwd->rules[i]; + const struct fwd_rule *rule = &fwd->rules[i]; unsigned port; - if (rs->rule.proto != proto) + if (rule->proto != proto) continue; - for (port = rs->rule.first; port <= rs->rule.last; port++) { - if (rs->socks[port - rs->rule.first] >= 0) + for (port = rule->first; port <= rule->last; port++) { + if (fwd->rulesocks[i][port - rule->first] >= 0) bitmap_set(map, port); } } diff --git a/fwd.h b/fwd.h index 33600cbf..83ee9b2e 100644 --- a/fwd.h +++ b/fwd.h @@ -26,16 +26,6 @@ struct flowside; void fwd_probe_ephemeral(void); void fwd_port_map_ephemeral(uint8_t *map); -/** - * struct fwd_rule_state - Forwarding rule and associated state - * @rule: Rule specification - * @socks: Array of listening sockets for this entry - */ -struct fwd_rule_state { - struct fwd_rule rule; - int *socks; -}; - #define FWD_RULE_BITS 8 #define MAX_FWD_RULES MAX_FROM_BITS(FWD_RULE_BITS) #define FWD_NO_HINT (-1) @@ -61,15 +51,17 @@ struct fwd_listen_ref { #define MAX_LISTEN_SOCKS (NUM_PORTS * 5) /** - * struct fwd_table - Table of forwarding rules (per initiating pif) + * struct fwd_table - Forwarding state (per initiating pif) * @count: Number of forwarding rules * @rules: Array of forwarding rules + * @rulesocks: Pointers to socket arrays per-rule * @sock_count: Number of entries used in @socks * @socks: Listening sockets for forwarding */ struct fwd_table { unsigned count; - struct fwd_rule_state rules[MAX_FWD_RULES]; + struct fwd_rule rules[MAX_FWD_RULES]; + int *rulesocks[MAX_FWD_RULES]; unsigned sock_count; int socks[MAX_LISTEN_SOCKS]; }; -- 2.53.0

After parsing port ranges conf_ports_spec() checks if we've reached a chunk delimiter (',') to verify that there isn't extra garbage there. Rework how we do this to use the recently introduces chunk-end pointer. This has two advantages: 1) Small, but practical: we don't need to repeat what the valid delimiters are, that's already handled in the chunk splitting code. 2) Large, if theoretical: this will also given an error if port parsing overruns a chunk boundary. We don't really expect that to happen, but it would give very confusing behaviour if it did. strtoul(3), on which parse_port_range() is based does say it may accept thousands separators based on locale which means can't entirely be sure it will only accept strings of digits. Signed-off-by: David Gibson --- conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/conf.c b/conf.c index 1ab1b071..51612047 100644 --- a/conf.c +++ b/conf.c @@ -264,7 +264,7 @@ static void conf_ports_spec(const struct ctx *c, if (parse_port_range(p, &p, &xrange)) goto bad; - if ((*p != '\0') && (*p != ',')) /* Garbage after the range */ + if (p != ep) /* Garbage after the range */ goto bad; for (i = xrange.first; i <= xrange.last; i++) @@ -303,7 +303,7 @@ static void conf_ports_spec(const struct ctx *c, mapped_range = orig_range; } - if ((*p != '\0') && (*p != ',')) /* Garbage after the ranges */ + if (p != ep) /* Garbage after the ranges */ goto bad; if (orig_range.first == 0) { -- 2.53.0

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 --- conf.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/conf.c b/conf.c index 51612047..fcc75d25 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, '/'))) { -- 2.53.0

Currently as well as building the forwarding tables, conf() maintains a "forwarding mode" value for each protocol and direction. This prevents, for example "-t all" and "-t 40000" being given on the same command line. This restriction predates the forwarding table and is no longer really necessary. Remove the restriction, instead doing our best to apply all the given options simultaneously. * Many combinations previously disallowed will still be disallowed because of conflicts between the specific generated rules, e.g. -t all -t 8888 (because -t all already listens on port 8888) * Some new combinations are now allowed and will work, e.g. -t all -t 40000 because 'all' excludes ephemeral ports (which includes 40000 on default Linux configurations). * We remove our mode variables, but keep boolean variables to track if any forwarding config option has been given. This is needed in order to correctly default to -t auto -T auto -u auto -U auto for pasta. * -[tTuU] none after any other rules is still considered an error. However -t none *before* other rules is allowed. This is potentially confusing, but is awkward to avoid for the time being. Signed-off-by: David Gibson --- conf.c | 97 ++++++++++++++++------------------------------------------ 1 file changed, 27 insertions(+), 70 deletions(-) diff --git a/conf.c b/conf.c index cd30f2f8..1e456361 100644 --- a/conf.c +++ b/conf.c @@ -232,22 +232,6 @@ fail: fwd_rule_fmt(&rule, rulestr, sizeof(rulestr))); } -/** - * enum fwd_mode - Overall forwarding mode for a direction and protocol - * @FWD_MODE_UNSET Initial value, not parsed/configured yet - * @FWD_MODE_SPEC Forward specified ports - * @FWD_MODE_NONE No forwarded ports - * @FWD_MODE_AUTO Automatic detection and forwarding based on bound ports - * @FWD_MODE_ALL Bind all free ports - */ -enum fwd_mode { - FWD_MODE_UNSET = 0, - FWD_MODE_SPEC, - FWD_MODE_NONE, - FWD_MODE_AUTO, - FWD_MODE_ALL, -}; - /** * conf_ports_spec() - Parse port range(s) specifier * @c: Execution context @@ -350,13 +334,12 @@ bad: * @optname: Short option name, t, T, u, or U * @optarg: Option argument (port specification) * @fwd: Forwarding table to be updated - * @mode: Overall port forwarding mode (updated) */ static void conf_ports(const struct ctx *c, char optname, const char *optarg, - struct fwd_table *fwd, enum fwd_mode *mode) + struct fwd_table *fwd) { union inany_addr addr_buf = inany_any6, *addr = &addr_buf; - char buf[BUFSIZ], *spec, *ifname = NULL, *p; + char buf[BUFSIZ], *spec, *ifname = NULL; uint8_t proto; if (optname == 't' || optname == 'T') @@ -367,10 +350,14 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, assert(0); if (!strcmp(optarg, "none")) { - if (*mode) - goto mode_conflict; + unsigned i; - *mode = FWD_MODE_NONE; + for (i = 0; i < fwd->count; i++) { + if (fwd->rules[i].proto == proto) { + die("-%c none conflicts with previous options", + optname); + } + } return; } @@ -380,14 +367,9 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, die("UDP port forwarding requested but UDP is disabled"); if (!strcmp(optarg, "auto")) { - if (*mode) - goto mode_conflict; - if (c->mode != MODE_PASTA) die("'auto' port forwarding is only allowed for pasta"); - *mode = FWD_MODE_AUTO; - conf_ports_range_except(c, optname, optarg, fwd, proto, NULL, NULL, 1, NUM_PORTS - 1, NULL, 1, FWD_SCAN); @@ -398,11 +380,6 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, if (!strcmp(optarg, "all")) { uint8_t exclude[PORT_BITMAP_SIZE] = { 0 }; - if (*mode) - goto mode_conflict; - - *mode = FWD_MODE_ALL; - /* Exclude ephemeral ports */ fwd_port_map_ephemeral(exclude); @@ -413,11 +390,6 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, return; } - if (*mode > FWD_MODE_SPEC) - die("Specific ports cannot be specified together with all/none/auto"); - - *mode = FWD_MODE_SPEC; - strncpy(buf, optarg, sizeof(buf) - 1); if ((spec = strchr(buf, '/'))) { @@ -445,7 +417,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, if (ifname == buf + 1) { /* Interface without address */ addr = NULL; } else { - p = buf; + char *p = buf; /* Allow square brackets for IPv4 too for convenience */ if (*p == '[' && p[strlen(p) - 1] == ']') { @@ -482,10 +454,6 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, ifname = "lo"; conf_ports_spec(c, optname, optarg, fwd, proto, addr, ifname, spec); - return; - -mode_conflict: - die("Port forwarding mode '%s' conflicts with previous mode", optarg); } /** @@ -1594,12 +1562,9 @@ void conf(struct ctx *c, int argc, char **argv) }; const char *optstring = "+dqfel:hs:F:I:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:T:U:"; const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt"; + bool opt_t = false, opt_T = false, opt_u = false, opt_U = false; char userns[PATH_MAX] = { 0 }, netns[PATH_MAX] = { 0 }; bool copy_addrs_opt = false, copy_routes_opt = false; - enum fwd_mode tcp_out_mode = FWD_MODE_UNSET; - enum fwd_mode udp_out_mode = FWD_MODE_UNSET; - enum fwd_mode tcp_in_mode = FWD_MODE_UNSET; - enum fwd_mode udp_in_mode = FWD_MODE_UNSET; bool v4_only = false, v6_only = false; unsigned dns4_idx = 0, dns6_idx = 0; unsigned long max_mtu = IP_MAX_MTU; @@ -2204,17 +2169,17 @@ void conf(struct ctx *c, int argc, char **argv) name = getopt_long(argc, argv, optstring, options, NULL); if (name == 't') { - conf_ports(c, name, optarg, c->fwd[PIF_HOST], - &tcp_in_mode); + opt_t = true; + conf_ports(c, name, optarg, c->fwd[PIF_HOST]); } else if (name == 'u') { - conf_ports(c, name, optarg, c->fwd[PIF_HOST], - &udp_in_mode); + opt_u = true; + conf_ports(c, name, optarg, c->fwd[PIF_HOST]); } else if (name == 'T') { - conf_ports(c, name, optarg, c->fwd[PIF_SPLICE], - &tcp_out_mode); + opt_T = true; + conf_ports(c, name, optarg, c->fwd[PIF_SPLICE]); } else if (name == 'U') { - conf_ports(c, name, optarg, c->fwd[PIF_SPLICE], - &udp_out_mode); + opt_U = true; + conf_ports(c, name, optarg, c->fwd[PIF_SPLICE]); } } while (name != -1); @@ -2265,22 +2230,14 @@ void conf(struct ctx *c, int argc, char **argv) } if (c->mode == MODE_PASTA) { - if (!tcp_in_mode) { - conf_ports(c, 't', "auto", - c->fwd[PIF_HOST], &tcp_in_mode); - } - if (!tcp_out_mode) { - conf_ports(c, 'T', "auto", - c->fwd[PIF_SPLICE], &tcp_out_mode); - } - if (!udp_in_mode) { - conf_ports(c, 'u', "auto", - c->fwd[PIF_HOST], &udp_in_mode); - } - if (!udp_out_mode) { - conf_ports(c, 'U', "auto", - c->fwd[PIF_SPLICE], &udp_out_mode); - } + if (!opt_t) + conf_ports(c, 't', "auto", c->fwd[PIF_HOST]); + if (!opt_T) + conf_ports(c, 'T', "auto", c->fwd[PIF_SPLICE]); + if (!opt_u) + conf_ports(c, 'u', "auto", c->fwd[PIF_HOST]); + if (!opt_U) + conf_ports(c, 'U', "auto", c->fwd[PIF_SPLICE]); } if (!c->quiet) -- 2.53.0

fwd_rule_add() sanity checks the given rule, however all errors are fatal: either they're assert()s in the case of things that callers should have already verified, or die()s if we run out of space for the new rule. This won't suffice any more when we allow rule updates from a configuraiton client. We don't want to trust the input we get from the client any more than we have to. Replace the assert()s and die()s with a return value. Also include warn()s so that the user gets a more specific idea of the problem in the logs or stderr. Signed-off-by: David Gibson --- conf.c | 15 ++++++++++++--- fwd.c | 40 ++++++++++++++++++++++++++++++---------- fwd.h | 2 +- fwd_rule.c | 3 +-- fwd_rule.h | 1 + 5 files changed, 45 insertions(+), 16 deletions(-) diff --git a/conf.c b/conf.c index a93837cc..cd30f2f8 100644 --- a/conf.c +++ b/conf.c @@ -157,6 +157,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname, .proto = proto, .flags = flags, }; + char rulestr[FWD_RULE_STRLEN]; unsigned delta = to - first; unsigned base, i; @@ -207,20 +208,28 @@ static void conf_ports_range_except(const struct ctx *c, char optname, rulev.addr = inany_loopback4; fwd_rule_conflict_check(&rulev, fwd->rules, fwd->count); - fwd_rule_add(fwd, &rulev); + 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); - fwd_rule_add(fwd, &rulev); + if (fwd_rule_add(fwd, &rulev) < 0) + goto fail; } } else { fwd_rule_conflict_check(&rule, fwd->rules, fwd->count); - fwd_rule_add(fwd, &rule); + 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))); } /** diff --git a/fwd.c b/fwd.c index c9637525..cee2c7f6 100644 --- a/fwd.c +++ b/fwd.c @@ -335,24 +335,43 @@ void fwd_rule_init(struct ctx *c) * 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 */ -void fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new) +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; - assert(!(new->flags & ~allowed_flags)); - /* Passing a non-wildcard address with DUAL_STACK_ANY is a bug */ - assert(!(new->flags & FWD_DUAL_STACK_ANY) || - inany_equals(&new->addr, &inany_any6)); - assert(new->first <= new->last); + if (new->first > new->last) { + warn("Forwarding rule has invalid port range %u-%u", + new->first, new->last); + return -EINVAL; + } + if (new->flags & ~allowed_flags) { + warn("Forwarding rule has invalid flags 0x%x", + new->flags & ~allowed_flags); + return -EINVAL; + } + if (new->flags & FWD_DUAL_STACK_ANY && + !inany_equals(&new->addr, &inany_any6)) { + warn("Dual stack forward must have unspecified address"); + return -EINVAL; + } - if (fwd->count >= ARRAY_SIZE(fwd->rules)) - die("Too many port forwarding ranges"); - if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) - die("Too many listening sockets"); + if (fwd->count >= ARRAY_SIZE(fwd->rules)) { + warn("Too many forwarding rules (maximum %u)", + ARRAY_SIZE(fwd->rules)); + return -ENOSPC; + } + if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) { + warn( +"Forwarding 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++) @@ -360,6 +379,7 @@ void fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new) fwd->rules[fwd->count++] = *new; fwd->sock_count += num; + return 0; } /** diff --git a/fwd.h b/fwd.h index 1d74cbd2..c56749e1 100644 --- a/fwd.h +++ b/fwd.h @@ -83,7 +83,7 @@ struct fwd_scan { #define FWD_PORT_SCAN_INTERVAL 1000 /* ms */ void fwd_rule_init(struct ctx *c); -void fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new); +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 4d5048f9..ef9308c2 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -39,8 +39,7 @@ const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule) * @dst: Buffer to store output (should have FWD_RULE_STRLEN bytes) * @size: Size of @dst */ -static const char *fwd_rule_fmt(const struct fwd_rule *rule, - char *dst, size_t size) +const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size) { const char *percent = *rule->ifname ? "%" : ""; const char *weak = "", *scan = ""; diff --git a/fwd_rule.h b/fwd_rule.h index f852be39..8506a0c4 100644 --- a/fwd_rule.h +++ b/fwd_rule.h @@ -51,6 +51,7 @@ struct fwd_rule { + sizeof(" []%:- => - (best effort) (auto-scan)")) 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); -- 2.53.0

On Tue, 7 Apr 2026 13:16:16 +1000 David Gibson wrote:

The man page currently has two fairly large, near-identical sections

Not entirely identical also because:

separately describing the -t and -u options for passt and pasta. This is bulky and potentially confusing. It will make this information more tedious to update as we alter what's possible here with the forwarding table. Consolidate both descriptions to a single one in the common options, noting the few passt/pasta difference inline.

There's similar duplication usage(), consolidate that as well.

Signed-off-by: David Gibson --- conf.c | 54 +++++--------- passt.1 | 216 ++++++++++++++++++-------------------------------------- 2 files changed, 85 insertions(+), 185 deletions(-)

diff --git a/conf.c b/conf.c index f3b36bb6..751e500f 100644 --- a/conf.c +++ b/conf.c @@ -1023,18 +1023,12 @@ static void usage(const char *name, FILE *f, int status) " --freebind Bind to any address for forwarding\n" " --no-map-gw Don't map gateway address to host\n" " -4, --ipv4-only Enable IPv4 operation only\n" - " -6, --ipv6-only Enable IPv6 operation only\n"); - - if (strstr(name, "pasta")) - goto pasta_opts; - - FPRINTF(f, - " -1, --one-off Quit after handling one single client\n" + " -6, --ipv6-only Enable IPv6 operation only\n" " -t, --tcp-ports SPEC TCP port forwarding to guest\n" " 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" " a comma-separated list, optionally ranged with '-'\n" " and optional target ports after ':', with optional\n" " address specification suffixed by '/' and optional\n" @@ -1050,43 +1044,29 @@ 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 guest\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" - " default: none\n" + " default: %s\n" " -u, --udp-ports SPEC UDP port forwarding to guest\n"

...this is "guest" for passt and "namespace" for pasta. Now, I know that you use those terms interchangeably and the code does as well, but this is directly visible to users so I think we shouldn't confuse them with "guests" for... containers. To reaffirm the difference between terms used in the code and terms used in documentation, could we perhaps do something like: char *guest = (mode == MODE_PASTA) ? "namespace" : "guest"; ? And then use that later? It might be worth doing that for the "default" strings too.

" SPEC is as described for TCP above\n" - " default: none\n"); + " default: %s\n", + strstr(name, "pasta") ? + " 'auto': forward all ports currently bound in namespace\n" + : + " 'all': forward all unbound, non-ephemeral ports\n", + strstr(name, "pasta") ? "auto" : "none", + strstr(name, "pasta") ? "auto" : "none"); + + if (strstr(name, "pasta")) + goto pasta_opts; + + FPRINTF(f, + " -1, --one-off Quit after handling one single client\n" + );

passt_exit(status);

pasta_opts:

FPRINTF(f, - " -t, --tcp-ports SPEC TCP port forwarding to namespace\n" - " can be specified multiple times\n" - " SPEC can be:\n" - " 'none': don't forward any ports\n" - " 'auto': forward all ports currently bound in namespace\n" - " 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 '%%'. Examples:\n" - " -t 22 Forward local port 22 to port 22 in netns\n" - " -t 22:23 Forward local port 22 to port 23\n" - " -t 22,25 Forward ports 22, 25 to ports 22, 25\n" - " -t 22-80 Forward ports 22 to 80\n" - " -t 22-80:32-90 Forward ports 22 to 80 to\n" - " corresponding port numbers plus 10\n" - " -t 192.0.2.1/5 Bind port 5 of 192.0.2.1 to namespace\n" - " -t 5-25,~10-20 Forward ports 5 to 9, and 21 to 25\n" - " -t ~25 Forward all bound ports except for 25\n" - " default: auto\n" - " IPv6 bound ports are also forwarded for IPv4\n" - " -u, --udp-ports SPEC UDP port forwarding to namespace\n" - " SPEC is as described for TCP above\n" - " default: auto\n" - " IPv6 bound ports are also forwarded for IPv4\n" - " unless specified, with '-t auto', UDP ports with numbers\n" - " corresponding to forwarded TCP port numbers are\n" - " forwarded too\n" " -T, --tcp-ns SPEC TCP port forwarding to init namespace\n" " SPEC is as described above\n" " default: auto\n" diff --git a/passt.1 b/passt.1 index 13e8df9d..a9a8a42a 100644 --- a/passt.1 +++ b/passt.1 @@ -425,78 +425,6 @@ Send \fIname\fR as DHCP option 12 (hostname). FQDN to configure the client with. Send \fIname\fR as Client FQDN: DHCP option 81 and DHCPv6 option 39.

-.SS \fBpasst\fR-only options - -.TP -.BR \-s ", " \-\-socket-path ", " \-\-socket " " \fIpath -Path for UNIX domain socket used by \fBqemu\fR(1) or \fBqrap\fR(1) to connect to -\fBpasst\fR. -Default is to probe a free socket, not accepting connections, starting from -\fI/tmp/passt_1.socket\fR to \fI/tmp/passt_64.socket\fR. - -.TP -.BR \-\-vhost-user -Enable vhost-user. The vhost-user command socket is provided by \fB--socket\fR. - -.TP -.BR \-\-print-capabilities -Print back-end capabilities in JSON format, only meaningful for vhost-user mode. - -.TP -.BR \-\-repair-path " " \fIpath -Path for UNIX domain socket used by the \fBpasst-repair\fR(1) helper to connect -to \fBpasst\fR in order to set or clear the TCP_REPAIR option on sockets, during -migration. \fB--repair-path none\fR disables this interface (if you need to -specify a socket path called "none" you can prefix the path by \fI./\fR). - -Default, for \-\-vhost-user mode only, is to append \fI.repair\fR to the path -chosen for the hypervisor UNIX domain socket. No socket is created if not in -\-\-vhost-user mode. - -.TP -.BR \-\-migrate-exit " " (DEPRECATED) -Exit after a completed migration as source. By default, \fBpasst\fR keeps -running and the migrated guest can continue using its connection, or a new guest -can connect. - -Note that this configuration option is \fBdeprecated\fR and will be removed in a -future version. It is not expected to be of any use, and it simply reflects a -legacy behaviour. If you have any use for this, refer to \fBREPORTING BUGS\fR -below. - -.TP -.BR \-\-migrate-no-linger " " (DEPRECATED) -Close TCP sockets on the source instance once migration completes. - -By default, sockets are kept open, and events on data sockets are ignored, so -that any further message reaching sockets after the source migrated is silently -ignored, to avoid connection resets in case data is received after migration. - -Note that this configuration option is \fBdeprecated\fR and will be removed in a -future version. It is not expected to be of any use, and it simply reflects a -legacy behaviour. If you have any use for this, refer to \fBREPORTING BUGS\fR -below. - -.TP -.BR \-F ", " \-\-fd " " \fIFD -Pass a pre-opened, connected socket to \fBpasst\fR. Usually the socket is opened -in the parent process and \fBpasst\fR inherits it when run as a child. This -allows the parent process to open sockets using another address family or -requiring special privileges. - -This option implies the behaviour described for \-\-one-off, once this socket -is closed. - -.TP -.BR \-1 ", " \-\-one-off -Quit after handling a single client connection, that is, once the client closes -the socket, or once we get a socket error. - -\fBNote\fR: this option has no effect after \fBpasst\fR completes a migration as -source, because, in that case, exiting would close sockets for active -connections, which would in turn cause connection resets if any further data is -received. See also the description of \fI\-\-migrate-no-linger\fR. - .TP .BR \-t ", " \-\-tcp-ports " " \fIspec Configure TCP port forwarding to guest. \fIspec\fR can be one of: @@ -507,11 +435,17 @@ Configure TCP port forwarding to guest. \fIspec\fR can be one of: Don't forward any ports

.TP -.BR all +.BR all (\fBpasst\fR only)

This is rendered as "passtonly". You need \fBpasst\fR " " only. While this one goes away in 5/18,

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)

...this one doesn't.

+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 .BR ports A comma-separated list of ports, optionally ranged with \fI-\fR, and, @@ -563,7 +497,7 @@ and 30 Forward all ports to the guest, except for the range from 20000 to 20010 .RE

-Default is \fBnone\fR. +Default is \fBnone\fR for \fBpasst\fR and \fBauto\fR for \fBpasta\fR. .RE

.TP @@ -575,101 +509,87 @@ Note: unless overridden, UDP ports with numbers corresponding to forwarded TCP port numbers are forwarded too, without, however, any port translation. IPv6 bound ports are also forwarded for IPv4.

-Default is \fBnone\fR. +Default is \fBnone\fR for \fBpasst\fR and \fBauto\fR for \fBpasta\fR.

-.SS \fBpasta\fR-only options +.SS \fBpasst\fR-only options

.TP -.BR \-I ", " \-\-ns-ifname " " \fIname -Name of tap interface to be created in target namespace. -By default, the same interface name as the external, routable interface is used. -If no such interface exists, the name \fItap0\fR will be used instead. +.BR \-s ", " \-\-socket-path ", " \-\-socket " " \fIpath +Path for UNIX domain socket used by \fBqemu\fR(1) or \fBqrap\fR(1) to connect to +\fBpasst\fR. +Default is to probe a free socket, not accepting connections, starting from +\fI/tmp/passt_1.socket\fR to \fI/tmp/passt_64.socket\fR.

.TP -.BR \-t ", " \-\-tcp-ports " " \fIspec -Configure TCP port forwarding to namespace. \fIspec\fR can be one of: -.RS +.BR \-\-vhost-user +Enable vhost-user. The vhost-user command socket is provided by \fB--socket\fR.

.TP -.BR none -Don't forward any ports +.BR \-\-print-capabilities +Print back-end capabilities in JSON format, only meaningful for vhost-user mode.

.TP -.BR auto -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). +.BR \-\-repair-path " " \fIpath +Path for UNIX domain socket used by the \fBpasst-repair\fR(1) helper to connect +to \fBpasst\fR in order to set or clear the TCP_REPAIR option on sockets, during +migration. \fB--repair-path none\fR disables this interface (if you need to +specify a socket path called "none" you can prefix the path by \fI./\fR). + +Default, for \-\-vhost-user mode only, is to append \fI.repair\fR to the path +chosen for the hypervisor UNIX domain socket. No socket is created if not in +\-\-vhost-user mode.

.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. +.BR \-\-migrate-exit " " (DEPRECATED) +Exit after a completed migration as source. By default, \fBpasst\fR keeps +running and the migrated guest can continue using its connection, or a new guest +can connect.

-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. +Note that this configuration option is \fBdeprecated\fR and will be removed in a +future version. It is not expected to be of any use, and it simply reflects a +legacy behaviour. If you have any use for this, refer to \fBREPORTING BUGS\fR +below.

-Examples: -.RS -.TP --t 22 -Forward local port 22 to 22 in the target namespace

...the passt equivalent says "guest". I think we should replace all these occurrences by "guest or namespace" at this point. -- Stefano

On Tue, 7 Apr 2026 13:16:18 +1000 David Gibson wrote:

6dad076df037 ("fwd: Split forwarding rule specification from its implementation state") created struct fwd_rule_state with a forwarding rule plus the table of sockets used for its implementation. It turns out this is quite awkward for sharing rule parsing code between passt and the upcoming configuration client.

Indeed, I hated it, in that short moment I had to fiddle with it. Thanks for coming up with a cleaner solution.

Instead keep the index of listening sockets in a parallel array in struct fwd_table.

Signed-off-by: David Gibson --- fwd.c | 73 ++++++++++++++++++++++++++++++----------------------------- fwd.h | 16 ++++--------- 2 files changed, 41 insertions(+), 48 deletions(-)

diff --git a/fwd.c b/fwd.c index e09b42fe..7e0edc38 100644 --- a/fwd.c +++ b/fwd.c @@ -363,7 +363,7 @@ void fwd_rule_add(struct fwd_table *fwd, uint8_t proto, uint8_t flags, /* Flags which can be set from the caller */ const uint8_t allowed_flags = FWD_WEAK | FWD_SCAN | FWD_DUAL_STACK_ANY; unsigned num = (unsigned)last - first + 1; - struct fwd_rule_state *new; + struct fwd_rule *new; unsigned i, port;

assert(!(flags & ~allowed_flags)); @@ -379,7 +379,7 @@ void fwd_rule_add(struct fwd_table *fwd, uint8_t proto, uint8_t flags, /* Check for any conflicting entries */ for (i = 0; i < fwd->count; i++) { char newstr[INANY_ADDRSTRLEN], rulestr[INANY_ADDRSTRLEN]; - const struct fwd_rule *rule = &fwd->rules[i].rule; + const struct fwd_rule *rule = &fwd->rules[i];

if (proto != rule->proto) /* Non-conflicting protocols */ @@ -399,38 +399,38 @@ void fwd_rule_add(struct fwd_table *fwd, uint8_t proto, uint8_t flags, rule->first, rule->last); }

- new = &fwd->rules[fwd->count++]; - new->rule.proto = proto; - new->rule.flags = flags; + new = &fwd->rules[fwd->count]; + new->proto = proto; + new->flags = flags;

if (addr) { - new->rule.addr = *addr; + new->addr = *addr; } else { - new->rule.addr = inany_any6; - new->rule.flags |= FWD_DUAL_STACK_ANY; + new->addr = inany_any6; + new->flags |= FWD_DUAL_STACK_ANY; }

- memset(new->rule.ifname, 0, sizeof(new->rule.ifname)); + memset(new->ifname, 0, sizeof(new->ifname)); if (ifname) { int ret;

- ret = snprintf(new->rule.ifname, sizeof(new->rule.ifname), + ret = snprintf(new->ifname, sizeof(new->ifname), "%s", ifname); - if (ret <= 0 || (size_t)ret >= sizeof(new->rule.ifname)) + if (ret <= 0 || (size_t)ret >= sizeof(new->ifname)) die("Invalid interface name: %s", ifname); }

assert(first <= last); - new->rule.first = first; - new->rule.last = last; + new->first = first; + new->last = last; + new->to = to;

- new->rule.to = to; + 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;

- new->socks = &fwd->socks[fwd->sock_count]; + fwd->count++; fwd->sock_count += num; - - for (port = new->rule.first; port <= new->rule.last; port++) - new->socks[port - new->rule.first] = -1; }

/** @@ -466,7 +466,7 @@ const struct fwd_rule *fwd_rule_search(const struct fwd_table *fwd,

if (hint >= 0) { char ostr[INANY_ADDRSTRLEN], rstr[INANY_ADDRSTRLEN]; - const struct fwd_rule *rule = &fwd->rules[hint].rule; + const struct fwd_rule *rule = &fwd->rules[hint];

assert((unsigned)hint < fwd->count); if (fwd_rule_match(rule, ini, proto)) @@ -480,8 +480,8 @@ const struct fwd_rule *fwd_rule_search(const struct fwd_table *fwd, }

for (i = 0; i < fwd->count; i++) { - if (fwd_rule_match(&fwd->rules[i].rule, ini, proto)) - return &fwd->rules[i].rule; + if (fwd_rule_match(&fwd->rules[i], ini, proto)) + return &fwd->rules[i]; }

return NULL; @@ -496,7 +496,7 @@ void fwd_rules_print(const struct fwd_table *fwd) unsigned i;

for (i = 0; i < fwd->count; i++) { - const struct fwd_rule *rule = &fwd->rules[i].rule; + const struct fwd_rule *rule = &fwd->rules[i]; const char *percent = *rule->ifname ? "%" : ""; const char *weak = "", *scan = ""; char addr[INANY_ADDRSTRLEN]; @@ -533,9 +533,9 @@ void fwd_rules_print(const struct fwd_table *fwd) static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx, const uint8_t *tcp, const uint8_t *udp) { - const struct fwd_rule_state *rs = &c->fwd[pif]->rules[idx]; - const struct fwd_rule *rule = &rs->rule; + const struct fwd_rule *rule = &c->fwd[pif]->rules[idx]; const union inany_addr *addr = fwd_rule_addr(rule); + int *socks = c->fwd[pif]->rulesocks[idx]; const char *ifname = rule->ifname; const uint8_t *map = NULL; bool bound_one = false; @@ -555,7 +555,7 @@ static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx, }

for (port = rule->first; port <= rule->last; port++) { - int fd = rs->socks[port - rule->first]; + int fd = socks[port - rule->first];

if (map && !bitmap_isset(map, port)) { /* We don't want to listen on this port */ @@ -563,7 +563,7 @@ static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx, /* We already are, so stop */ epoll_del(c->epollfd, fd); close(fd); - rs->socks[port - rule->first] = -1; + socks[port - rule->first] = -1; } continue; } @@ -595,7 +595,7 @@ static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx, continue; }

- rs->socks[port - rule->first] = fd; + socks[port - rule->first] = fd; bound_one = true; }

@@ -685,11 +685,12 @@ void fwd_listen_close(const struct fwd_table *fwd) unsigned i;

for (i = 0; i < fwd->count; i++) { - const struct fwd_rule_state *rs = &fwd->rules[i]; + const struct fwd_rule *rule = &fwd->rules[i]; + int *socks = fwd->rulesocks[i]; unsigned port;

- for (port = rs->rule.first; port <= rs->rule.last; port++) { - int *fdp = &rs->socks[port - rs->rule.first]; + for (port = rule->first; port <= rule->last; port++) { + int *fdp = &socks[port - rule->first]; if (*fdp >= 0) { close(*fdp); *fdp = -1; @@ -769,8 +770,8 @@ static bool has_scan_rules(const struct fwd_table *fwd, uint8_t proto) unsigned i;

for (i = 0; i < fwd->count; i++) { - if (fwd->rules[i].rule.proto == proto && - fwd->rules[i].rule.flags & FWD_SCAN) + if (fwd->rules[i].proto == proto && + fwd->rules[i].flags & FWD_SCAN) return true; } return false; @@ -838,14 +839,14 @@ static void current_listen_map(uint8_t *map, const struct fwd_table *fwd, memset(map, 0, PORT_BITMAP_SIZE);

for (i = 0; i < fwd->count; i++) { - const struct fwd_rule_state *rs = &fwd->rules[i]; + const struct fwd_rule *rule = &fwd->rules[i]; unsigned port;

- if (rs->rule.proto != proto) + if (rule->proto != proto) continue;

- for (port = rs->rule.first; port <= rs->rule.last; port++) { - if (rs->socks[port - rs->rule.first] >= 0) + for (port = rule->first; port <= rule->last; port++) { + if (fwd->rulesocks[i][port - rule->first] >= 0) bitmap_set(map, port); } } diff --git a/fwd.h b/fwd.h index 33600cbf..83ee9b2e 100644 --- a/fwd.h +++ b/fwd.h @@ -26,16 +26,6 @@ struct flowside; void fwd_probe_ephemeral(void); void fwd_port_map_ephemeral(uint8_t *map);

-/** - * struct fwd_rule_state - Forwarding rule and associated state - * @rule: Rule specification - * @socks: Array of listening sockets for this entry - */ -struct fwd_rule_state { - struct fwd_rule rule; - int *socks; -}; - #define FWD_RULE_BITS 8 #define MAX_FWD_RULES MAX_FROM_BITS(FWD_RULE_BITS) #define FWD_NO_HINT (-1) @@ -61,15 +51,17 @@ struct fwd_listen_ref { #define MAX_LISTEN_SOCKS (NUM_PORTS * 5)

/** - * struct fwd_table - Table of forwarding rules (per initiating pif) + * struct fwd_table - Forwarding state (per initiating pif) * @count: Number of forwarding rules * @rules: Array of forwarding rules + * @rulesocks: Pointers to socket arrays per-rule

I don't see this as particularly descriptive (which sockets? What's the array size?). I'm thinking of something like: @socks_ref: Per-rule pointers to associated @socks, @sock_count of them

* @sock_count: Number of entries used in @socks * @socks: Listening sockets for forwarding */ struct fwd_table { unsigned count; - struct fwd_rule_state rules[MAX_FWD_RULES]; + struct fwd_rule rules[MAX_FWD_RULES]; + int *rulesocks[MAX_FWD_RULES]; unsigned sock_count; int socks[MAX_LISTEN_SOCKS]; };

-- Stefano

On Wed, Apr 08, 2026 at 01:14:50AM +0200, Stefano Brivio wrote:

On Tue, 7 Apr 2026 13:16:22 +1000 David Gibson wrote:

...

Amongst other checks, fwd_rule_add() checks that the newly added rule doesn't conflict with any existing rules. However, unlike the other things we verify, this isn't really required for safe operation. Rule conflicts are a useful thing for the user to know about, but the forwarding logic is perfectly sound with conflicting rules (the first one will win).

In order to support dynamic rule updates, we want fwd_rule_add() to become a more low-level function, only checking the things it really needs to. So, move rule conflict checking to its caller via new helpers in fwd_rule.c.

Signed-off-by: David Gibson --- conf.c | 5 +++++ fwd.c | 26 +------------------------- fwd_rule.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ fwd_rule.h | 2 ++ 4 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/conf.c b/conf.c index c9ee8c59..a93837cc 100644 --- a/conf.c +++ b/conf.c @@ -205,13 +205,18 @@ static void conf_ports_range_except(const struct ctx *c, char optname,

if (c->ifi4) { rulev.addr = inany_loopback4; + fwd_rule_conflict_check(&rulev, + fwd->rules, fwd->count); fwd_rule_add(fwd, &rulev); } if (c->ifi6) { rulev.addr = inany_loopback6; + fwd_rule_conflict_check(&rulev, + fwd->rules, fwd->count); fwd_rule_add(fwd, &rulev); } } else { + fwd_rule_conflict_check(&rule, fwd->rules, fwd->count); fwd_rule_add(fwd, &rule); } base = i - 1; diff --git a/fwd.c b/fwd.c index c05107d1..c9637525 100644 --- a/fwd.c +++ b/fwd.c @@ -341,7 +341,7 @@ void 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 i, port; + unsigned port;

assert(!(new->flags & ~allowed_flags)); /* Passing a non-wildcard address with DUAL_STACK_ANY is a bug */ @@ -354,30 +354,6 @@ void fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new) if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) die("Too many listening sockets");

- /* Check for any conflicting entries */ - for (i = 0; i < fwd->count; i++) { - char newstr[INANY_ADDRSTRLEN], rulestr[INANY_ADDRSTRLEN]; - const struct fwd_rule *rule = &fwd->rules[i]; - - if (new->proto != rule->proto) - /* Non-conflicting protocols */ - continue; - - if (!inany_matches(fwd_rule_addr(new), fwd_rule_addr(rule))) - /* Non-conflicting addresses */ - continue; - - if (new->last < rule->first || rule->last < new->first) - /* Port ranges don't overlap */ - continue; - - die("Forwarding configuration conflict: %s/%u-%u versus %s/%u-%u", - inany_ntop(fwd_rule_addr(new), newstr, sizeof(newstr)), - new->first, new->last, - inany_ntop(fwd_rule_addr(rule), rulestr, sizeof(rulestr)), - rule->first, rule->last); - } - 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; diff --git a/fwd_rule.c b/fwd_rule.c index abe9dfbf..4d5048f9 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -87,3 +87,47 @@ void fwd_rules_info(const struct fwd_rule *rules, size_t count) 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 + */ +static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fwd_rule *b) +{ + if (a->proto != b->proto) + /* Non-conflicting protocols */ + return false; + + if (!inany_matches(fwd_rule_addr(a), fwd_rule_addr(b))) + /* Non-conflicting addresses */ + return false; + + assert(a->first <= a->last && b->first <= b->last); + if (a->last < b->first || b->last < a->first) + /* Port ranges don't overlap */ + return false; + + return true; +} + +/* fwd_rule_conflict_check() - Die with errir if rule conflicts with any in list

Nit: an errir happens only when Mimir (ask Jon) makes a mistake, which is quite rare. :)

Fixed. Also the incorrect formatting for the start of the comment block.

...

+ * @new: New 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) +{ + 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))); + } +} diff --git a/fwd_rule.h b/fwd_rule.h index e92efb6d..f852be39 100644 --- a/fwd_rule.h +++ b/fwd_rule.h @@ -52,5 +52,7 @@ struct fwd_rule {

const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule); 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);

#endif /* FWD_RULE_H */

I reviewed only up to here so far, the rest will come in a bit.

I had a quick look at the whole series and it all looks good to me so far but that wasn't quite a review.

Meanwhile, I noticed some warnings that strangely appear only during the build of passt.avx2:

cc -Wall -Wextra -Wno-format-zero-length -Wformat-security -pedantic -std=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -pie -fPIE -DPAGE_SIZE=4096 -DVERSION=\"2026_01_20.386b5f5-84-ge87c74f\" -DDUAL_STACK_SOCKETS=1 -DHAS_GETRANDOM -fstack-protector-strong -Ofast -mavx2 -ftree-vectorize -funroll-loops \ arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c epoll_ctl.c flow.c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c log.c mld.c ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c pif.c repair.c serialise.c tap.c tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c vhost_user.c virtio.c vu_common.c -o passt.avx2 In file included from util.h:22, from ip.h:12, from fwd_rule.h:16, from fwd_rule.c:20: fwd_rule.c: In function ‘fwd_rules_info’: fwd_rule.c:86:22: warning: ‘%s’ directive argument is null [-Wformat-overflow=] 86 | info(" %s", fwd_rule_fmt(&rules[i], buf, sizeof(buf))); | ^~~~~~~~ log.h:31:66: note: in definition of macro ‘info’ 31 | #define info(...) logmsg(true, false, LOG_INFO, __VA_ARGS__) | ^~~~~~~~~~~ fwd_rule.c:86:27: note: format string is defined here 86 | info(" %s", fwd_rule_fmt(&rules[i], buf, sizeof(buf))); | ^~

...but I don't think I looked at the code changes causing them, yet (and didn't bisect the series either). This is with gcc 13.3.0.

Huh. I don't see those those with gcc 15.2.1. I _think_ it's a false positive. Investigating... -- David Gibson (he or they) | 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

On Tue, 7 Apr 2026 13:16:26 +1000 David Gibson wrote:

After parsing port ranges conf_ports_spec() checks if we've reached a chunk delimiter (',') to verify that there isn't extra garbage there. Rework how we do this to use the recently introduces chunk-end pointer.

Nit: introduced

This has two advantages:

1) Small, but practical: we don't need to repeat what the valid delimiters are, that's already handled in the chunk splitting code.

2) Large, if theoretical: this will also given an error if port parsing overruns a chunk boundary. We don't really expect that to happen, but it would give very confusing behaviour if it did. strtoul(3), on which parse_port_range() is based does say it may accept thousands separators based on locale which means can't entirely be sure it will only accept

Nit: we can't

strings of digits.

Signed-off-by: David Gibson --- conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/conf.c b/conf.c index 1ab1b071..51612047 100644 --- a/conf.c +++ b/conf.c @@ -264,7 +264,7 @@ static void conf_ports_spec(const struct ctx *c,

if (parse_port_range(p, &p, &xrange)) goto bad; - if ((*p != '\0') && (*p != ',')) /* Garbage after the range */ + if (p != ep) /* Garbage after the range */ goto bad;

for (i = xrange.first; i <= xrange.last; i++) @@ -303,7 +303,7 @@ static void conf_ports_spec(const struct ctx *c, mapped_range = orig_range; }

- if ((*p != '\0') && (*p != ',')) /* Garbage after the ranges */ + if (p != ep) /* Garbage after the ranges */ goto bad;

if (orig_range.first == 0) {

-- Stefano

On Tue, 7 Apr 2026 13:16:28 +1000 David Gibson wrote:

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 --- conf.c | 66 ++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/conf.c b/conf.c index fcc75d25..86c30c7f 100644 --- a/conf.c +++ b/conf.c @@ -13,6 +13,7 @@ */

#include +#include #include #include #include @@ -112,6 +113,28 @@ static int parse_port_range(const char *s, const char **endptr, return 0; }

+/** + * parse_keyword() - Parse a literal keyword

Here, "parse" sounds overly generic. If I understand this correctly, it's a strstr() / memmem() implementation with the added 'endptr' functionality, so maybe... "Find the end of a substring"?

+ * @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, '/'))) {

The rest of the series looks good to me! -- Stefano