From: 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 Reviewed-by: Laurent Vivier Signed-off-by: Stefano Brivio --- conf.c | 21 --------------------- fwd.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 47 insertions(+), 26 deletions(-) diff --git a/conf.c b/conf.c index 6e884e5..b470b0d 100644 --- a/conf.c +++ b/conf.c @@ -176,8 +176,6 @@ static void conf_ports_range_except(struct fwd_table *fwd, uint8_t proto, die("Invalid interface name: %s", ifname); } - assert(first != 0); - for (base = first; base <= last; base++) { if (exclude && bitmap_isset(exclude, base)) continue; @@ -310,10 +308,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 +350,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 +394,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 c7fd1a9..979c149 100644 --- a/fwd.c +++ b/fwd.c @@ -367,17 +367,59 @@ 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 || + (in_port_t)(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.43.0

From: 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 Reviewed-by: Laurent Vivier Signed-off-by: Stefano Brivio --- conf.c | 376 +------------------------------------------ fwd.c | 94 ----------- fwd.h | 33 ---- fwd_rule.c | 464 ++++++++++++++++++++++++++++++++++++++++++++++++++++- fwd_rule.h | 36 ++++- 5 files changed, 502 insertions(+), 501 deletions(-) diff --git a/conf.c b/conf.c index b470b0d..5aacfe0 100644 --- a/conf.c +++ b/conf.c @@ -13,7 +13,6 @@ */ #include -#include #include #include #include @@ -66,365 +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); - } - - 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 @@ -2160,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); @@ -2221,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 a6d75b7..728a783 100644 --- a/fwd.c +++ b/fwd.c @@ -275,100 +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 || - (in_port_t)(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 e664d1d..8f845d0 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 9d48982..cd3dec0 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -15,6 +15,7 @@ * Author: David Gibson */ +#include #include #include #include @@ -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,459 @@ 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 || + (in_port_t)(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); + } + + 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 5c7b67a..f0f4efd 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.43.0

From: 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 Reviewed-by: Laurent Vivier Signed-off-by: Stefano Brivio --- 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 5aacfe0..0586107 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 1413584..777282d 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 f0f4efd..5855138 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.43.0

From: David Gibson The new warnings in fwd_rule_add() have some not quite technically correct format specifiers. For some weird reason these don't trip warnings on passt itself, but do when we re-use this code in the upcoming configuration client. Fix them. Signed-off-by: David Gibson Reviewed-by: Laurent Vivier Signed-off-by: Stefano Brivio --- fwd_rule.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fwd_rule.c b/fwd_rule.c index 777282d..7fd20dd 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -208,7 +208,7 @@ static int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new) return -EINVAL; } if (new->flags & ~allowed_flags) { - warn("Rule has invalid flags 0x%hhx", + warn("Rule has invalid flags 0x%x", new->flags & ~allowed_flags); return -EINVAL; } @@ -267,11 +267,11 @@ static int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new) } if (fwd->count >= ARRAY_SIZE(fwd->rules)) { - warn("Too many rules (maximum %u)", ARRAY_SIZE(fwd->rules)); + warn("Too many rules (maximum %d)", ARRAY_SIZE(fwd->rules)); return -ENOSPC; } if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) { - warn("Rules require too many listening sockets (maximum %u)", + warn("Rules require too many listening sockets (maximum %d)", ARRAY_SIZE(fwd->socks)); return -ENOSPC; } -- 2.43.0

From: David Gibson In pesto we're going to want several levels of error/warning messages, much like passt itself. Particularly as we start to share mode code between passt and pesto, we want to use a similar interface to emit those. However we don't want to use the same implementation - logging to a file or syslog doesn't make sense for the command line tool. To accomplish this loosely share log.h, but not log.c between pesto and passt. In fact, an #ifdef means even most of log.h isn't actually shared, but we do provide similar warn(), die() etc. macros. This includes the *_perror() variants, which need strerror(). However, we want to avoid allocations for pesto as we do for passt, and strerror() allocates in some libc versions. Therefore, also move our workaround for this to be shared with pesto. Reviewed-by: Laurent Vivier [dwg: Based on changes part of a larger patch by Stefano] Signed-off-by: David Gibson [sbrivio: Dropped debug_perror_() as it's not used anyway, Laurent was asking about its name] Signed-off-by: Stefano Brivio --- Makefile | 6 +++++- common.h | 32 ++++++++++++++++++++++++++++++++ log.h | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++- pesto.c | 14 ++++---------- util.h | 32 -------------------------------- 5 files changed, 93 insertions(+), 44 deletions(-) diff --git a/Makefile b/Makefile index 030681b..f6cec8a 100644 --- a/Makefile +++ b/Makefile @@ -61,7 +61,7 @@ PASST_HEADERS = arch.h arp.h bitmap.h checksum.h common.h conf.h dhcp.h \ vhost_user.h virtio.h vu_common.h QRAP_HEADERS = arp.h ip.h passt.h util.h PASST_REPAIR_HEADERS = linux_dep.h -PESTO_HEADERS = common.h pesto.h +PESTO_HEADERS = common.h pesto.h log.h C := \#include \nint main(){int a=getrandom(0, 0, 0);} ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0) @@ -121,6 +121,7 @@ qrap: $(QRAP_SRCS) $(QRAP_HEADERS) passt-repair: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h +pesto: BASE_CPPFLAGS += -DPESTO pesto: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction \ @@ -221,9 +222,12 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck qrap.cppcheck %.cppcheck: $(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^ +passt.cppcheck: BASE_CPPFLAGS += -UPESTO passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h + passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h +pesto.cppcheck: BASE_CPPFLAGS += -DPESTO pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unmatchedSuppression pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h diff --git a/common.h b/common.h index f3506b4..4251781 100644 --- a/common.h +++ b/common.h @@ -21,4 +21,36 @@ /* FPRINTF() intentionally silences cert-err33-c clang-tidy warnings */ #define FPRINTF(f, ...) (void)fprintf(f, __VA_ARGS__) +/* + * Starting from glibc 2.40.9000 and commit 25a5eb4010df ("string: strerror, + * strsignal cannot use buffer after dlmopen (bug 32026)"), strerror() needs + * getrandom(2) and brk(2) as it allocates memory for the locale-translated + * error description, but our seccomp profiles forbid both. + * + * Use the strerror_() wrapper instead, calling into strerrordesc_np() to get + * a static untranslated string. It's a GNU implementation, but also defined by + * bionic. + * + * If strerrordesc_np() is not defined (e.g. musl), call strerror(). C libraries + * not defining strerrordesc_np() are expected to provide strerror() + * implementations that are simple enough for us to call. + */ +__attribute__ ((weak)) const char *strerrordesc_np(int errnum); + +/** + * strerror_() - strerror() wrapper calling strerrordesc_np() if available + * @errnum: Error code + * + * Return: error description string + */ +static inline const char *strerror_(int errnum) +{ + if (strerrordesc_np) + return strerrordesc_np(errnum); + + return strerror(errnum); +} + +#define strerror(x) @ "Don't call strerror() directly, use strerror_() instead" + #endif /* COMMON_H */ diff --git a/log.h b/log.h index dbab006..c6befe3 100644 --- a/log.h +++ b/log.h @@ -6,8 +6,57 @@ #ifndef LOG_H #define LOG_H -#include #include +#include +#include + +#ifdef PESTO + +#include + +#include "common.h" + +extern bool debug_flag; + +#define msg(...) \ + do { \ + FPRINTF(stderr, __VA_ARGS__); \ + FPRINTF(stderr, "\n"); \ + } while (0) + +#define msg_perror(...) \ + do { \ + int errno_ = errno; \ + FPRINTF(stderr, __VA_ARGS__); \ + FPRINTF(stderr, ": %s\n", strerror_(errno_)); \ + } while (0) + +#define die(...) \ + do { \ + msg(__VA_ARGS__); \ + exit(EXIT_FAILURE); \ + } while (0) + +#define die_perror(...) \ + do { \ + msg_perror(__VA_ARGS__); \ + exit(EXIT_FAILURE); \ + } while (0) + +#define warn(...) msg(__VA_ARGS__) +#define warn_perror(...) msg_perror(__VA_ARGS__) +#define info(...) msg(__VA_ARGS__) +#define info_perror(...) msg_perror(__VA_ARGS__) + +#define debug(...) \ + do { \ + if (debug_flag) \ + msg(__VA_ARGS__); \ + } while (0) + +#else /* !PESTO */ + +#include #include #include @@ -109,4 +158,6 @@ void __openlog(const char *ident, int option, int facility); void logfile_init(const char *name, const char *path, size_t size); void __setlogmask(int mask); +#endif /* !PESTO */ + #endif /* LOG_H */ diff --git a/pesto.c b/pesto.c index 9f2fa5d..f0916e8 100644 --- a/pesto.c +++ b/pesto.c @@ -34,18 +34,12 @@ #include "common.h" #include "seccomp_pesto.h" #include "pesto.h" +#include "log.h" -static bool debug_flag = false; +bool debug_flag = false; static char stdout_buf[BUFSIZ]; -#define die(...) \ - do { \ - FPRINTF(stderr, __VA_ARGS__); \ - FPRINTF(stderr, "\n"); \ - exit(EXIT_FAILURE); \ - } while (0) - /** * usage() - Print usage, exit with given status code * @name: Executable name @@ -99,7 +93,7 @@ int main(int argc, char **argv) * breaking our seccomp profile. */ if (setvbuf(stdout, stdout_buf, _IOFBF, sizeof(stdout_buf))) - die("Failed to set stdout buffer"); + die_perror("Failed to set stdout buffer"); do { optname = getopt_long(argc, argv, optstring, options, NULL); @@ -126,7 +120,7 @@ int main(int argc, char **argv) if (argc - optind != 1) usage(argv[0], stderr, EXIT_FAILURE); - printf("debug_flag=%d, path=\"%s\"\n", debug_flag, argv[optind]); + debug("debug_flag=%d, path=\"%s\"", debug_flag, argv[optind]); die("pesto is not implemented yet"); } diff --git a/util.h b/util.h index 770ff93..e90be47 100644 --- a/util.h +++ b/util.h @@ -302,38 +302,6 @@ static inline bool mod_between(unsigned x, unsigned i, unsigned j, unsigned m) void raw_random(void *buf, size_t buflen); -/* - * Starting from glibc 2.40.9000 and commit 25a5eb4010df ("string: strerror, - * strsignal cannot use buffer after dlmopen (bug 32026)"), strerror() needs - * getrandom(2) and brk(2) as it allocates memory for the locale-translated - * error description, but our seccomp profiles forbid both. - * - * Use the strerror_() wrapper instead, calling into strerrordesc_np() to get - * a static untranslated string. It's a GNU implementation, but also defined by - * bionic. - * - * If strerrordesc_np() is not defined (e.g. musl), call strerror(). C libraries - * not defining strerrordesc_np() are expected to provide strerror() - * implementations that are simple enough for us to call. - */ -__attribute__ ((weak)) const char *strerrordesc_np(int errnum); - -/** - * strerror_() - strerror() wrapper calling strerrordesc_np() if available - * @errnum: Error code - * - * Return: error description string - */ -static inline const char *strerror_(int errnum) -{ - if (strerrordesc_np) - return strerrordesc_np(errnum); - - return strerror(errnum); -} - -#define strerror(x) @ "Don't call strerror() directly, use strerror_() instead" - /* * Workarounds for https://github.com/llvm/llvm-project/issues/58992 * -- 2.43.0

From: David Gibson Extend the dynamic update protocol to expose the pif indices and names from a running passt/pasta to the pesto tool. pesto records that data and prints it out. Signed-off-by: David Gibson Reviewed-by: Laurent Vivier [sbrivio: In read_pif_conf(), force a redundant termination of the interface name, the existing check isn't obvious enough for static checkers] [sbrivio: Drop @resv_ left-over in description of struct pesto_pif_info, reported by Jon Maloy] [sbrivio: Fix minor nits reported by Laurent] Signed-off-by: Stefano Brivio --- Makefile | 1 + common.h | 2 + conf.c | 41 ++++++++++++++++ pesto.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++ pesto.h | 18 ++++++- pif.h | 5 +- serialise.c | 4 ++ serialise.h | 1 + util.h | 2 - 9 files changed, 201 insertions(+), 7 deletions(-) diff --git a/Makefile b/Makefile index 1718ddb..6da76b4 100644 --- a/Makefile +++ b/Makefile @@ -223,6 +223,7 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck qrap.cppcheck $(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^ passt.cppcheck: BASE_CPPFLAGS += -UPESTO +passt.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h diff --git a/common.h b/common.h index 4251781..68573b4 100644 --- a/common.h +++ b/common.h @@ -53,4 +53,6 @@ static inline const char *strerror_(int errnum) #define strerror(x) @ "Don't call strerror() directly, use strerror_() instead" +#define ARRAY_SIZE(a) ((int)(sizeof(a) / sizeof((a)[0]))) + #endif /* COMMON_H */ diff --git a/conf.c b/conf.c index 823e08d..3b2fe42 100644 --- a/conf.c +++ b/conf.c @@ -1925,6 +1925,43 @@ void conf(struct ctx *c, int argc, char **argv) static void conf_accept(struct ctx *c); +/** + * conf_send_rules() - Send current forwarding rules to config client (pesto) + * @c: Execution context + * @fd: Socket to the client + * + * Return: 0 on success, -1 on failure + * + * FIXME: So far only sends pif ids and names + */ +static int conf_send_rules(const struct ctx *c, int fd) +{ + unsigned pif; + + for (pif = 0; pif < PIF_NUM_TYPES; pif++) { + struct pesto_pif_info info; + int rc; + + if (!c->fwd[pif]) + continue; + + assert(pif != PIF_NONE); + + rc = snprintf(info.name, sizeof(info.name), "%s", pif_name(pif)); + assert(rc >= 0 && (size_t)rc < sizeof(info.name)); + + if (write_u8(fd, pif) < 0) + return -1; + if (write_all_buf(fd, &info, sizeof(info)) < 0) + return -1; + } + + if (write_u8(fd, PIF_NONE) < 0) + return -1; + + return 0; +} + /** * conf_close() - Close configuration / control socket and clean up * @c: Execution context @@ -1968,6 +2005,7 @@ static void conf_accept(struct ctx *c) struct pesto_hello hello = { .magic = PESTO_SERVER_MAGIC, .version = htonl(PESTO_PROTOCOL_VERSION), + .pif_name_size = htonl(PIF_NAME_SIZE), }; union epoll_ref ref = { .type = EPOLL_TYPE_CONF }; struct ucred uc = { 0 }; @@ -2009,6 +2047,9 @@ retry: "Warning: Using experimental unsupported configuration protocol"); } + if (conf_send_rules(c, fd) < 0) + goto fail; + return; fail: diff --git a/pesto.c b/pesto.c index ab476c5..b33492a 100644 --- a/pesto.c +++ b/pesto.c @@ -60,6 +60,127 @@ static void usage(const char *name, FILE *f, int status) exit(status); } +/* Maximum number of pifs with rule tables */ +#define MAX_PIFS 3 + +struct pif_configuration { + uint8_t pif; + char name[PIF_NAME_SIZE]; +}; + +struct configuration { + uint32_t npifs; + struct pif_configuration pif[MAX_PIFS]; +}; + +/** + * pif_conf_by_num() - Find a pif's configuration by pif id + * @conf: Configuration description + * @pif: pif id + * + * Return: pointer to the pif_configuration for @pif, or NULL if not found + */ +static struct pif_configuration *pif_conf_by_num(struct configuration *conf, + uint8_t pif) +{ + unsigned i; + + for (i = 0; i < conf->npifs; i++) { + if (conf->pif[i].pif == pif) + return &conf->pif[i]; + } + + return NULL; +} + +/** + * pif_conf_by_name() - Find a pif's configuration by name + * @conf: Configuration description + * @name: Interface name + * + * Return: pif_configuration for pif named @name, or NULL if not found + */ +static struct pif_configuration *pif_conf_by_name(struct configuration *conf, + const char *name) +{ + unsigned i; + + for (i = 0; i < conf->npifs; i++) { + if (strcmp(conf->pif[i].name, name) == 0) + return &conf->pif[i]; + } + + return NULL; +} + +/** + * pesto_read_rules() - Read rulestate from passt/pasta + * @fd: Control socket + * @conf: Configuration description to update + */ +static bool read_pif_conf(int fd, struct configuration *conf) +{ + struct pif_configuration *pc; + struct pesto_pif_info info; + uint8_t pif; + + if (read_u8(fd, &pif) < 0) + die("Error reading from control socket"); + + if (pif == PIF_NONE) + return false; + + debug("Receiving config for PIF %"PRIu8, pif); + + if (conf->npifs >= ARRAY_SIZE(conf->pif)) { + die("passt has more pifs than pesto can manage (max %d)", + ARRAY_SIZE(conf->pif)); + } + + pc = &conf->pif[conf->npifs]; + pc->pif = pif; + + if (read_all_buf(fd, &info, sizeof(info)) < 0) + die("Error reading from control socket"); + + if (info.name[sizeof(info.name)-1]) + die("Interface name was not NULL terminated"); + /* Redundant, to make static checkers happy */ + info.name[sizeof(info.name) - 1] = '\0'; + + static_assert(sizeof(info.name) == sizeof(pc->name), + "Mismatching pif name lengths"); + memcpy(pc->name, info.name, sizeof(pc->name)); + + debug("PIF %"PRIu8": %s", pc->pif, pc->name); + + /* O(n^2), but n is bounded by MAX_PIFS */ + if (pif_conf_by_num(conf, pc->pif)) + die("Received duplicate interface identifier"); + + /* O(n^2), but n is bounded by MAX_PIFS */ + if (pif_conf_by_name(conf, pc->name)) + die("Received duplicate interface name"); + + conf->npifs++; + return true; +} + +/** + * show_conf() - Show current configuration obtained from passt/pasta + * @conf: Configuration description + */ +static void show_conf(const struct configuration *conf) +{ + unsigned i; + + for (i = 0; i < conf->npifs; i++) { + const struct pif_configuration *pc = &conf->pif[i]; + printf(" %s\n", pc->name); + printf(" TBD\n"); + } +} + /** * main() - Dynamic reconfiguration client main program * @argc: Argument count @@ -80,6 +201,7 @@ int main(int argc, char **argv) { 0 }, }; struct sockaddr_un a = { AF_UNIX, "" }; + struct configuration conf = { 0 }; const char *optstring = "dh"; struct pesto_hello hello; struct sock_fprog prog; @@ -162,6 +284,18 @@ int main(int argc, char **argv) "Warning: Using experimental protocol version, client and server must match\n"); } + if (ntohl(hello.pif_name_size) != PIF_NAME_SIZE) { + die("Server has unexpected pif name size (%" + PRIu32" not %"PRIu32 ")", + ntohl(hello.pif_name_size), PIF_NAME_SIZE); + } + + while (read_pif_conf(s, &conf)) + ; + + printf("passt/pasta configuration (%s)\n", a.sun_path); + show_conf(&conf); + if (shutdown(s, SHUT_RDWR) < 0 || close(s) < 0) die_perror("Error shutting down control socket"); diff --git a/pesto.h b/pesto.h index 92d4df3..b0064a0 100644 --- a/pesto.h +++ b/pesto.h @@ -17,18 +17,32 @@ /* Version 0 is reserved for unreleased / unsupported experimental versions */ #define PESTO_PROTOCOL_VERSION 0 +/* Maximum size of a pif name, including \0 */ +#define PIF_NAME_SIZE (128) +#define PIF_NONE 0 + /** * struct pesto_hello - Server introduction message - * @magic: PESTO_SERVER_MAGIC - * @version: Version number + * @magic: PESTO_SERVER_MAGIC + * @version: Version number + * @pif_name_size: Server's value for PIF_NAME_SIZE */ struct pesto_hello { char magic[8]; uint32_t version; + uint32_t pif_name_size; } __attribute__ ((__packed__)); static_assert(sizeof(PESTO_SERVER_MAGIC) == sizeof(((struct pesto_hello *)0)->magic), "PESTO_SERVER_MAGIC has wrong size"); +/** + * struct pesto_pif_info - Message with basic metadata about a pif + * @name: Name (\0 terminated) + */ +struct pesto_pif_info { + char name[PIF_NAME_SIZE]; +} __attribute__ ((__packed__)); + #endif /* PESTO_H */ diff --git a/pif.h b/pif.h index 553c742..48d4919 100644 --- a/pif.h +++ b/pif.h @@ -11,6 +11,7 @@ #include +#include "pesto.h" #include "epoll_type.h" union inany_addr; @@ -24,7 +25,7 @@ union sockaddr_inany; */ enum pif_type { /* Invalid or not present pif */ - PIF_NONE = 0, + PIF_NONE_ = PIF_NONE, /* Host socket interface */ PIF_HOST, /* Qemu socket or namespace tuntap interface */ @@ -35,8 +36,6 @@ enum pif_type { PIF_NUM_TYPES, }; -/* Maximum size of a pif name, including \0 */ -#define PIF_NAME_SIZE (128) extern const char pif_type_str[][PIF_NAME_SIZE]; static inline const char *pif_type(enum pif_type pt) diff --git a/serialise.c b/serialise.c index 346df99..e083112 100644 --- a/serialise.c +++ b/serialise.c @@ -121,6 +121,10 @@ int write_all_buf(int fd, const void *buf, size_t len) return write_all_buf(fd, &beval, sizeof(beval)); \ } +#define be8toh(x) (x) +#define htobe8(x) (x) + +SERIALISE_UINT(8) SERIALISE_UINT(32) #undef SERIALISE_UINT diff --git a/serialise.h b/serialise.h index a88f3de..4714f4c 100644 --- a/serialise.h +++ b/serialise.h @@ -16,6 +16,7 @@ int write_all_buf(int fd, const void *buf, size_t len); int read_u##bits(int fd, uint##bits##_t *val); \ int write_u##bits(int fd, uint##bits##_t val); +SERIALISE_UINT_DECL(8) SERIALISE_UINT_DECL(32) #endif /* SERIALISE_H */ diff --git a/util.h b/util.h index e90be47..c788382 100644 --- a/util.h +++ b/util.h @@ -87,8 +87,6 @@ void abort_with_msg(const char *fmt, ...) #define V6 1 #define IP_VERSIONS 2 -#define ARRAY_SIZE(a) ((int)(sizeof(a) / sizeof((a)[0]))) - #define foreach(item, array) \ for ((item) = (array); (item) - (array) < ARRAY_SIZE(array); (item)++) -- 2.43.0

From: David Gibson inany contains a number of helpful functions for dealing with addresses which might be IPv4 or IPv6. We're going to want to use that in pesto. For the most part inany doesn't depend on other passt/pasta internals, however it does depend on siphash.h, which pesto doesn't need. Move the single dependent function, inany_siphash_feed() to siphash.h, renaming to match. Use that include inany.[ch] into pesto as well as passt/pasta. While we're there reformat pesto.c's header comment to match the convention used in most other files. Signed-off-by: David Gibson Reviewed-by: Laurent Vivier Signed-off-by: Stefano Brivio --- common.h | 20 ++++++++++++++++++-- flow.c | 4 ++-- fwd.c | 2 +- fwd.h | 1 + fwd_rule.h | 1 + inany.c | 19 ++++++++++++++----- inany.h | 17 ++--------------- pif.h | 1 + siphash.h | 13 +++++++++++++ util.h | 16 ---------------- 10 files changed, 53 insertions(+), 41 deletions(-) diff --git a/common.h b/common.h index ae6908d..f2518f6 100644 --- a/common.h +++ b/common.h @@ -18,9 +18,27 @@ "This is free software: you are free to change and redistribute it.\n" \ "There is NO WARRANTY, to the extent permitted by law.\n\n" +#ifndef MIN +#define MIN(x, y) (((x) < (y)) ? (x) : (y)) +#endif +#ifndef MAX +#define MAX(x, y) (((x) > (y)) ? (x) : (y)) +#endif + +#define MAX_FROM_BITS(n) (((1U << (n)) - 1)) + /* FPRINTF() intentionally silences cert-err33-c clang-tidy warnings */ #define FPRINTF(f, ...) (void)fprintf(f, __VA_ARGS__) +#define ARRAY_SIZE(a) ((int)(sizeof(a) / sizeof((a)[0]))) + +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) +#define DIV_ROUND_CLOSEST(n, d) (((n) + (d) / 2) / (d)) +#define ROUND_DOWN(x, y) ((x) & ~((y) - 1)) +#define ROUND_UP(x, y) (((x) + (y) - 1) & ~((y) - 1)) + +#define UINT16_STRLEN (sizeof("65535")) + /* * Starting from glibc 2.40.9000 and commit 25a5eb4010df ("string: strerror, * strsignal cannot use buffer after dlmopen (bug 32026)"), strerror() needs @@ -53,8 +71,6 @@ static inline const char *strerror_(int errnum) #define strerror(x) @ "Don't call strerror() directly, use strerror_() instead" -#define ARRAY_SIZE(a) ((int)(sizeof(a) / sizeof((a)[0]))) - #ifndef __bswap_constant_16 #define __bswap_constant_16(x) \ ((uint16_t) ((((x) >> 8) & 0xff) | (((x) & 0xff) << 8))) diff --git a/flow.c b/flow.c index 56a6c6d..91f2b81 100644 --- a/flow.c +++ b/flow.c @@ -680,8 +680,8 @@ static uint64_t flow_hash(const struct ctx *c, uint8_t proto, uint8_t pif, { struct siphash_state state = SIPHASH_INIT(c->hash_secret); - inany_siphash_feed(&state, &side->oaddr); - inany_siphash_feed(&state, &side->eaddr); + siphash_feed_inany(&state, &side->oaddr); + siphash_feed_inany(&state, &side->eaddr); return siphash_final(&state, 38, (uint64_t)proto << 40 | (uint64_t)pif << 32 | diff --git a/fwd.c b/fwd.c index 728a783..8849cfc 100644 --- a/fwd.c +++ b/fwd.c @@ -80,7 +80,7 @@ static size_t neigh_table_slot(const struct ctx *c, struct siphash_state st = SIPHASH_INIT(c->hash_secret); uint32_t i; - inany_siphash_feed(&st, key); + siphash_feed_inany(&st, key); i = siphash_final(&st, sizeof(*key), 0); return ((size_t)i) & (NEIGH_TABLE_SIZE - 1); diff --git a/fwd.h b/fwd.h index 8f845d0..ac24782 100644 --- a/fwd.h +++ b/fwd.h @@ -19,6 +19,7 @@ #include "fwd_rule.h" struct flowside; +struct ctx; #define FWD_NO_HINT (-1) diff --git a/fwd_rule.h b/fwd_rule.h index 5855138..f51f1b4 100644 --- a/fwd_rule.h +++ b/fwd_rule.h @@ -13,6 +13,7 @@ #include #include +#include "common.h" #include "ip.h" #include "inany.h" #include "bitmap.h" diff --git a/inany.c b/inany.c index 2a586ed..23faf3f 100644 --- a/inany.c +++ b/inany.c @@ -1,9 +1,19 @@ -/* SPDX-License-Identifier: GPL-2.0-or-later - * Copyright Red Hat - * Author: David Gibson +// SPDX-License-Identifier: GPL-2.0-or-later + +/* PASST - Plug A Simple Socket Transport + * for qemu/UNIX domain socket mode + * + * PASTA - Pack A Subtle Tap Abstraction + * for network namespace/tap device mode + * + * PESTO - Programmable Extensible Socket Translation Orchestrator + * front-end for passt(1) and pasta(1) forwarding configuration * * inany.c - Types and helpers for handling addresses which could be * IPv6 or IPv4 (encoded as IPv4-mapped IPv6 addresses) + * + * Copyright Red Hat + * Author: David Gibson */ #include @@ -13,9 +23,8 @@ #include #include -#include "util.h" +#include "common.h" #include "ip.h" -#include "siphash.h" #include "inany.h" #include "fwd.h" diff --git a/inany.h b/inany.h index 1f7741d..73385b9 100644 --- a/inany.h +++ b/inany.h @@ -11,13 +11,11 @@ #include #include +#include #include -#include "util.h" +#include "common.h" #include "ip.h" -#include "siphash.h" - -struct siphash_state; /** union inany_addr - Represents either an IPv4 or IPv6 address * @a6: Address as an IPv6 address, may be IPv4-mapped @@ -301,17 +299,6 @@ static inline int inany_from_sockaddr(union inany_addr *dst, in_port_t *port, return -1; } -/** inany_siphash_feed- Fold IPv[46] address into an in-progress siphash - * @state: siphash state - * @aa: inany to hash - */ -static inline void inany_siphash_feed(struct siphash_state *state, - const union inany_addr *aa) -{ - siphash_feed(state, (uint64_t)aa->u32[0] << 32 | aa->u32[1]); - siphash_feed(state, (uint64_t)aa->u32[2] << 32 | aa->u32[3]); -} - #define INANY_ADDRSTRLEN MAX(INET_ADDRSTRLEN, INET6_ADDRSTRLEN) bool inany_matches(const union inany_addr *a, const union inany_addr *b); diff --git a/pif.h b/pif.h index 48d4919..62223d1 100644 --- a/pif.h +++ b/pif.h @@ -16,6 +16,7 @@ union inany_addr; union sockaddr_inany; +struct ctx; /** * enum pif_type - Type of passt/pasta interface ("pif") diff --git a/siphash.h b/siphash.h index bbddcac..313b894 100644 --- a/siphash.h +++ b/siphash.h @@ -47,6 +47,8 @@ #include #include +#include "inany.h" + /** * struct siphash_state - Internal state of siphash calculation */ @@ -101,6 +103,17 @@ static inline void siphash_feed(struct siphash_state *state, uint64_t in) state->v[0] ^= in; } +/** siphash_feed_inany() - Fold IPv[46] address into an in-progress siphash + * @state: siphash state + * @aa: inany to hash + */ +static inline void siphash_feed_inany(struct siphash_state *state, + const union inany_addr *aa) +{ + siphash_feed(state, (uint64_t)aa->u32[0] << 32 | aa->u32[1]); + siphash_feed(state, (uint64_t)aa->u32[2] << 32 | aa->u32[3]); +} + /** * siphash_final() - Finalize SipHash calculations * @v: siphash state (4 x 64-bit integers) diff --git a/util.h b/util.h index dc14c78..70aadeb 100644 --- a/util.h +++ b/util.h @@ -29,20 +29,6 @@ #define IP_MAX_MTU USHRT_MAX #endif -#ifndef MIN -#define MIN(x, y) (((x) < (y)) ? (x) : (y)) -#endif -#ifndef MAX -#define MAX(x, y) (((x) > (y)) ? (x) : (y)) -#endif - -#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) -#define DIV_ROUND_CLOSEST(n, d) (((n) + (d) / 2) / (d)) -#define ROUND_DOWN(x, y) ((x) & ~((y) - 1)) -#define ROUND_UP(x, y) (((x) + (y) - 1) & ~((y) - 1)) - -#define MAX_FROM_BITS(n) (((1U << (n)) - 1)) - #define SWAP(a, b) \ do { \ __typeof__(a) __x = (a); (a) = (b); (b) = __x; \ @@ -202,8 +188,6 @@ static inline const char *af_name(sa_family_t af) } } -#define UINT16_STRLEN (sizeof("65535")) - /* inet address (- '\0') + port (u16) (- '\0') + ':' + '\0' */ #define SOCKADDR_INET_STRLEN \ (INET_ADDRSTRLEN-1 + UINT16_STRLEN-1 + sizeof(":")) -- 2.43.0

From: David Gibson This adds parsing of options using fwd_rule_parse(), validates them and adds them to the existing rules. It doesn't yet send those rules back to passt or pasta. Message-ID: <20260322141843.4095972-3-sbrivio@redhat.com> [dwg: Based on an early draft by Stefano] Signed-off-by: David Gibson [sbrivio: Recycled usage messages for -T and -U from conf.c as suggested by Laurent, dropped unrelated whitespace change] [sbrivio: Add description of -t, -u, -T, -U to pesto.1] Signed-off-by: Stefano Brivio --- Makefile | 1 + fwd_rule.c | 2 +- fwd_rule.h | 1 + pesto.1 | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++ pesto.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 227 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 9e99dd1..c746b55 100644 --- a/Makefile +++ b/Makefile @@ -226,6 +226,7 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck qrap.cppcheck passt.cppcheck: BASE_CPPFLAGS += -UPESTO passt.cppcheck: CPPCHECK_FLAGS += \ --suppress=unusedFunction:fwd_rule.c \ + --suppress=staticFunction:fwd_rule.c \ --suppress=unusedFunction:serialise.c passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h diff --git a/fwd_rule.c b/fwd_rule.c index c2824d5..b55e4df 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -187,7 +187,7 @@ static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fwd_rule * * * Return: 0 on success, negative error code on failure */ -static int 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; diff --git a/fwd_rule.h b/fwd_rule.h index 330d49e..f43b37d 100644 --- a/fwd_rule.h +++ b/fwd_rule.h @@ -103,6 +103,7 @@ const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size); void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd); int fwd_rule_read(int fd, struct fwd_rule *rule); int fwd_rule_write(int fd, const struct fwd_rule *rule); +int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new); /** * fwd_rules_dump() - Dump forwarding rules diff --git a/pesto.1 b/pesto.1 index b06433d..32d6ed1 100644 --- a/pesto.1 +++ b/pesto.1 @@ -31,6 +31,123 @@ Be verbose. .BR \-h ", " \-\-help Display a help message and exit. +.TP +.BR \-t ", " \-\-tcp-ports " " \fIspec +Configure TCP port forwarding to guest or namespace. \fIspec\fR can be one of: +.RS + +.TP +.BR none +Don't forward any ports + +.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 +may be either: +.RS +.TP +\fBall\fR +Forward all unbound, non-ephemeral ports, as permitted by current capabilities. +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 +\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. + +.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. 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 +.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 +-t 22:23 +Forward local port 22 to port 23 on the guest or namespace +.TP +-t 22,25 +Forward local ports 22 and 25 to ports 22 and 25 on the guest or namespace +.TP +-t 22-80 +Forward local ports between 22 and 80 to corresponding ports on the guest or +namespace +.TP +-t 22-80:32-90 +Forward local ports between 22 and 80 to ports between 32 and 90 on the guest or +namespace +.TP +-t 192.0.2.1/22 +Forward local port 22, bound to 192.0.2.1, to port 22 on the guest or 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 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 +.RE + +.TP +.BR \-u ", " \-\-udp-ports " " \fIspec +Configure UDP port forwarding to guest. \fIspec\fR is as described for TCP +above. + .TP .BR \-\-version Show version and exit. diff --git a/pesto.c b/pesto.c index 92a8cb2..16b3a5a 100644 --- a/pesto.c +++ b/pesto.c @@ -55,6 +55,43 @@ static void usage(const char *name, FILE *f, int status) FPRINTF(f, "Usage: %s [OPTION]... PATH\n", name); FPRINTF(f, "\n" + " -t, --tcp-ports SPEC TCP inbound port forwarding\n" + " can be specified multiple times\n" + " SPEC can be:\n" + " 'none': don't forward any ports\n" + " [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" + " The 'auto' keyword may be given to only forward\n" + " ports which are bound in the target namespace\n" + " Examples:\n" + " -t all Forward all ports\n" + " -t 127.0.0.1/all Forward all ports from local address\n" + " 127.0.0.1\n" + " -t 22 Forward local port 22 to 22\n" + " -t 22:23 Forward local port 22 to 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\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" + " -t auto Forward all ports bound in namespace\n" + " -t 192.0.2.2/auto Forward ports from 192.0.2.2 if\n" + " they are bound in the namespace\n" + " -t 8000-8010,auto Forward ports 8000-8010 if they\n" + " are bound in the namespace\n" + " -u, --udp-ports SPEC UDP inbound port forwarding\n" + " SPEC is as described for TCP above\n" + " -T, --tcp-ns SPEC TCP port forwarding to init namespace\n" + " SPEC is as described above\n" + " -U, --udp-ns SPEC UDP port forwarding to init namespace\n" + " SPEC is as described above\n" + " -s, --show Show configuration before and after\n" " -d, --debug Print debugging messages\n" " -h, --help Display this help message and exit\n" " --version Show version and exit\n"); @@ -207,6 +244,8 @@ static void show_conf(const struct configuration *conf) fwd_rules_dump(printf, pc->fwd.rules, pc->fwd.count, " ", "\n"); } + /* Flush stdout, so this doesn't get misordered with later debug()s */ + (void)fflush(stdout); } /** @@ -218,7 +257,7 @@ static void show_conf(const struct configuration *conf) * * #syscalls:pesto socket s390x:socketcall i686:socketcall * #syscalls:pesto connect shutdown close - * #syscalls:pesto exit_group fstat read write + * #syscalls:pesto exit_group fstat read write openat */ int main(int argc, char **argv) { @@ -226,11 +265,18 @@ int main(int argc, char **argv) {"debug", no_argument, NULL, 'd' }, {"help", no_argument, NULL, 'h' }, {"version", no_argument, NULL, 1 }, + {"tcp-ports", required_argument, NULL, 't' }, + {"udp-ports", required_argument, NULL, 'u' }, + {"tcp-ns", required_argument, NULL, 'T' }, + {"udp-ns", required_argument, NULL, 'U' }, + {"show", no_argument, NULL, 's' }, { 0 }, }; + struct pif_configuration *inbound, *outbound; struct sockaddr_un a = { AF_UNIX, "" }; + const char *optstring = "dht:u:T:U:s"; struct configuration conf = { 0 }; - const char *optstring = "dh"; + bool update = false, show = false; struct pesto_hello hello; struct sock_fprog prog; int optname, ret, s; @@ -251,6 +297,8 @@ int main(int argc, char **argv) if (setvbuf(stdout, stdout_buf, _IOFBF, sizeof(stdout_buf))) die_perror("Failed to set stdout buffer"); + fwd_probe_ephemeral(); + do { optname = getopt_long(argc, argv, optstring, options, NULL); @@ -258,6 +306,16 @@ int main(int argc, char **argv) case -1: case 0: break; + case 't': + case 'u': + case 'T': + case 'U': + /* Parse these options after we've read state from passt/pasta */ + update = true; + break; + case 's': + show = true; + break; case 'h': usage(argv[0], stdout, EXIT_SUCCESS); break; @@ -290,6 +348,8 @@ int main(int argc, char **argv) die_perror("Failed to connect to %s", a.sun_path); } + debug("Connected to passt/pasta control socket"); + ret = read_all_buf(s, &hello, sizeof(hello)); if (ret < 0) die_perror("Couldn't read server greeting"); @@ -327,9 +387,52 @@ int main(int argc, char **argv) while (read_pif_conf(s, &conf)) ; - printf("passt/pasta configuration (%s)\n", a.sun_path); - show_conf(&conf); + if (!update) { + printf("passt/pasta configuration (%s)\n", a.sun_path); + show_conf(&conf); + goto noupdate; + } + + if (show) { + printf("Previous configuration (%s)\n", a.sun_path); + show_conf(&conf); + } + + inbound = pif_conf_by_name(&conf, "HOST"); + outbound = pif_conf_by_name(&conf, "SPLICE"); + + optind = 0; + do { + optname = getopt_long(argc, argv, optstring, options, NULL); + + switch (optname) { + case 't': + case 'u': + if (!inbound) { + die("Can't use -%c, no inbound interface", + optname); + } + fwd_rule_parse(optname, optarg, &inbound->fwd); + break; + case 'T': + case 'U': + if (!outbound) { + die("Can't use -%c, no outbound interface", + optname); + } + fwd_rule_parse(optname, optarg, &outbound->fwd); + break; + default: + continue; + } + } while (optname != -1); + + if (show) { + printf("Updated configuration (%s)\n", a.sun_path); + show_conf(&conf); + } +noupdate: if (shutdown(s, SHUT_RDWR) < 0 || close(s) < 0) die_perror("Error shutting down control socket"); -- 2.43.0

From: David Gibson We can now receive updates to the forwarding rules from the pesto client and store them in a "pending" copy of the forwarding tables. Implement switching to using the new rules. The logic is in a new fwd_listen_switch(). For now this closes all listening sockets related to the old tables, swaps the active and pending tables, then listens based on the new tables. In future we look to improve this so that we don't temporarily stop listening on ports that both the old and new tables specify. Signed-off-by: David Gibson Signed-off-by: Stefano Brivio --- conf.c | 5 ++--- fwd.c | 34 ++++++++++++++++++++++++++++++++++ fwd.h | 1 + 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/conf.c b/conf.c index f035fd3..75b8291 100644 --- a/conf.c +++ b/conf.c @@ -2159,15 +2159,14 @@ void conf_handler(struct ctx *c, uint32_t events) fwd_rules_dump(info, fwd->rules, fwd->count, " ", ""); } + + fwd_listen_switch(c); } if (events & EPOLLHUP) { debug("Configuration client hangup"); - goto close; } - return; - close: conf_close(c); diff --git a/fwd.c b/fwd.c index d93d2e5..35b9e2b 100644 --- a/fwd.c +++ b/fwd.c @@ -534,6 +534,40 @@ int fwd_listen_init(const struct ctx *c) return 0; } +/** + * fwd_listen_switch() - Switch from current to pending rules table + * @c: Execution context + */ +void fwd_listen_switch(struct ctx *c) +{ + struct fwd_table *tmp[PIF_NUM_TYPES]; + unsigned i; + + /* Stop listening on the old tables */ + for (i = 0; i < PIF_NUM_TYPES; i++) { + struct fwd_table *fwd = c->fwd[i]; + + if (!fwd) + continue; + + debug("Flushing %u old %s rules", fwd->count, pif_name(i)); + fwd_listen_close(fwd); + fwd->count = fwd->sock_count = 0; + } + + /* Swap active and pending tables */ + static_assert(sizeof(tmp) == sizeof(c->fwd) && + sizeof(tmp) == sizeof(c->fwd_pending), + "Temporary has wrong size"); + memcpy(&tmp, (void *)c->fwd, sizeof(tmp)); + memcpy((void *)c->fwd, (void *)c->fwd_pending, sizeof(tmp)); + memcpy((void *)c->fwd_pending, &tmp, sizeof(tmp)); + + /* Start listening on the new tables */ + if (fwd_listen_init(c) < 0) + err("Error switching to new forwarding rules"); +} + /* See enum in kernel's include/net/tcp_states.h */ #define UDP_LISTEN 0x07 #define TCP_LISTEN 0x0a diff --git a/fwd.h b/fwd.h index ac24782..b60697d 100644 --- a/fwd.h +++ b/fwd.h @@ -61,6 +61,7 @@ int fwd_listen_sync(const struct ctx *c, uint8_t pif, const struct fwd_scan *tcp, const struct fwd_scan *udp); void fwd_listen_close(const struct fwd_table *fwd); int fwd_listen_init(const struct ctx *c); +void fwd_listen_switch(struct ctx *c); bool nat_inbound(const struct ctx *c, const union inany_addr *addr, union inany_addr *translated); -- 2.43.0

On Tue, May 05, 2026 at 01:11:42AM +0200, Stefano Brivio wrote:

The new checks are actually sufficient but not enough for Coverity Scan. Now that fwd->sock_count and new->last are affected or supplied by clients, we need explicit (albeit redundant) checks on them.

Signed-off-by: Stefano Brivio

I'm assuming this does squash the warnings, but I think it does so in a somewhat confusing way.

--- fwd_rule.c | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/fwd_rule.c b/fwd_rule.c index b55e4df..03e8e80 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -271,13 +271,22 @@ int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new) warn("Too many rules (maximum %d)", ARRAY_SIZE(fwd->rules)); return -ENOSPC; } + if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) { warn("Rules require too many listening sockets (maximum %d)", ARRAY_SIZE(fwd->socks)); return -ENOSPC; } + /* Redundant, to make static checkers happy */ + if (fwd->sock_count > ARRAY_SIZE(fwd->socks)) + return -ENOSPC;

So there's actually two conditions that this is kind of relevant to: 1) (fwd->sock_count > ARRAY_SIZE(fwd->socks)) on entry That means something is horribly wrong before we were even called. So, I think that would be better as an assert(). 2) (fwd->sock_count + num) overflows That's a closer-to-real concern. I'm pretty sure we can't hit it for real, because num is necessarily <= 65536, so as long as (1) is true this can't overflow. But that relies on the specific value of ARRAY_SIZE(fwd->socks), so it's kind of fragile. I think an explicit check for this is a good idea, but it should actually check for this, not just side-effects of it, so: if (fwd->sock_count + num <= fwd->sock_count) { warn("Blah blah overflow"); return -EFAULT; /* or whatever */ }

fwd->rulesocks[fwd->count] = &fwd->socks[fwd->sock_count]; + + /* Redundant ('num' checked above), but not for static checkers */ + if (new->last > ARRAY_SIZE(fwd->socks) + new->first) + return -ENOSPC;

This way of organising the check is very confusing to me. I'm not really sure what it's trying to catch. We've already checked that last >= first, so using num is safer to deal with at this point than ARRAY_SIZE() + first, which could in principle overflow even if sock_count + num is perfectly ok.

for (port = new->first; port <= new->last; port++) fwd->rulesocks[fwd->count][port - new->first] = -1;

-- 2.43.0

-- 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 5/5/26 01:11, Stefano Brivio wrote:

From: David Gibson

This adds parsing of options using fwd_rule_parse(), validates them and adds them to the existing rules. It doesn't yet send those rules back to passt or pasta.

Message-ID: <20260322141843.4095972-3-sbrivio@redhat.com> [dwg: Based on an early draft by Stefano] Signed-off-by: David Gibson [sbrivio: Recycled usage messages for -T and -U from conf.c as suggested by Laurent, dropped unrelated whitespace change] [sbrivio: Add description of -t, -u, -T, -U to pesto.1]

-T, -U, -s are still missing in pesto.1 otherwise: Reviewed-by: Laurent Vivier Thanks, Laurent

Signed-off-by: Stefano Brivio --- Makefile | 1 + fwd_rule.c | 2 +- fwd_rule.h | 1 + pesto.1 | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++ pesto.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 227 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile index 9e99dd1..c746b55 100644 --- a/Makefile +++ b/Makefile @@ -226,6 +226,7 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck qrap.cppcheck passt.cppcheck: BASE_CPPFLAGS += -UPESTO passt.cppcheck: CPPCHECK_FLAGS += \ --suppress=unusedFunction:fwd_rule.c \ + --suppress=staticFunction:fwd_rule.c \ --suppress=unusedFunction:serialise.c passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h

diff --git a/fwd_rule.c b/fwd_rule.c index c2824d5..b55e4df 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -187,7 +187,7 @@ static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fwd_rule * * * Return: 0 on success, negative error code on failure */ -static int 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; diff --git a/fwd_rule.h b/fwd_rule.h index 330d49e..f43b37d 100644 --- a/fwd_rule.h +++ b/fwd_rule.h @@ -103,6 +103,7 @@ const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size); void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd); int fwd_rule_read(int fd, struct fwd_rule *rule); int fwd_rule_write(int fd, const struct fwd_rule *rule); +int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new);

/** * fwd_rules_dump() - Dump forwarding rules diff --git a/pesto.1 b/pesto.1 index b06433d..32d6ed1 100644 --- a/pesto.1 +++ b/pesto.1 @@ -31,6 +31,123 @@ Be verbose. .BR \-h ", " \-\-help Display a help message and exit.

+.TP +.BR \-t ", " \-\-tcp-ports " " \fIspec +Configure TCP port forwarding to guest or namespace. \fIspec\fR can be one of: +.RS + +.TP +.BR none +Don't forward any ports + +.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 +may be either: +.RS +.TP +\fBall\fR +Forward all unbound, non-ephemeral ports, as permitted by current capabilities. +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 +\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. + +.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. 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 +.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 +-t 22:23 +Forward local port 22 to port 23 on the guest or namespace +.TP +-t 22,25 +Forward local ports 22 and 25 to ports 22 and 25 on the guest or namespace +.TP +-t 22-80 +Forward local ports between 22 and 80 to corresponding ports on the guest or +namespace +.TP +-t 22-80:32-90 +Forward local ports between 22 and 80 to ports between 32 and 90 on the guest or +namespace +.TP +-t 192.0.2.1/22 +Forward local port 22, bound to 192.0.2.1, to port 22 on the guest or 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 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 +.RE + +.TP +.BR \-u ", " \-\-udp-ports " " \fIspec +Configure UDP port forwarding to guest. \fIspec\fR is as described for TCP +above. + .TP .BR \-\-version Show version and exit. diff --git a/pesto.c b/pesto.c index 92a8cb2..16b3a5a 100644 --- a/pesto.c +++ b/pesto.c @@ -55,6 +55,43 @@ static void usage(const char *name, FILE *f, int status) FPRINTF(f, "Usage: %s [OPTION]... PATH\n", name); FPRINTF(f, "\n" + " -t, --tcp-ports SPEC TCP inbound port forwarding\n" + " can be specified multiple times\n" + " SPEC can be:\n" + " 'none': don't forward any ports\n" + " [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" + " The 'auto' keyword may be given to only forward\n" + " ports which are bound in the target namespace\n" + " Examples:\n" + " -t all Forward all ports\n" + " -t 127.0.0.1/all Forward all ports from local address\n" + " 127.0.0.1\n" + " -t 22 Forward local port 22 to 22\n" + " -t 22:23 Forward local port 22 to 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\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" + " -t auto Forward all ports bound in namespace\n" + " -t 192.0.2.2/auto Forward ports from 192.0.2.2 if\n" + " they are bound in the namespace\n" + " -t 8000-8010,auto Forward ports 8000-8010 if they\n" + " are bound in the namespace\n" + " -u, --udp-ports SPEC UDP inbound port forwarding\n" + " SPEC is as described for TCP above\n" + " -T, --tcp-ns SPEC TCP port forwarding to init namespace\n" + " SPEC is as described above\n" + " -U, --udp-ns SPEC UDP port forwarding to init namespace\n" + " SPEC is as described above\n" + " -s, --show Show configuration before and after\n" " -d, --debug Print debugging messages\n" " -h, --help Display this help message and exit\n" " --version Show version and exit\n"); @@ -207,6 +244,8 @@ static void show_conf(const struct configuration *conf) fwd_rules_dump(printf, pc->fwd.rules, pc->fwd.count, " ", "\n"); } + /* Flush stdout, so this doesn't get misordered with later debug()s */ + (void)fflush(stdout); }

/** @@ -218,7 +257,7 @@ static void show_conf(const struct configuration *conf) * * #syscalls:pesto socket s390x:socketcall i686:socketcall * #syscalls:pesto connect shutdown close - * #syscalls:pesto exit_group fstat read write + * #syscalls:pesto exit_group fstat read write openat */ int main(int argc, char **argv) { @@ -226,11 +265,18 @@ int main(int argc, char **argv) {"debug", no_argument, NULL, 'd' }, {"help", no_argument, NULL, 'h' }, {"version", no_argument, NULL, 1 }, + {"tcp-ports", required_argument, NULL, 't' }, + {"udp-ports", required_argument, NULL, 'u' }, + {"tcp-ns", required_argument, NULL, 'T' }, + {"udp-ns", required_argument, NULL, 'U' }, + {"show", no_argument, NULL, 's' }, { 0 }, }; + struct pif_configuration *inbound, *outbound; struct sockaddr_un a = { AF_UNIX, "" }; + const char *optstring = "dht:u:T:U:s"; struct configuration conf = { 0 }; - const char *optstring = "dh"; + bool update = false, show = false; struct pesto_hello hello; struct sock_fprog prog; int optname, ret, s; @@ -251,6 +297,8 @@ int main(int argc, char **argv) if (setvbuf(stdout, stdout_buf, _IOFBF, sizeof(stdout_buf))) die_perror("Failed to set stdout buffer");

+ fwd_probe_ephemeral(); + do { optname = getopt_long(argc, argv, optstring, options, NULL);

@@ -258,6 +306,16 @@ int main(int argc, char **argv) case -1: case 0: break; + case 't': + case 'u': + case 'T': + case 'U': + /* Parse these options after we've read state from passt/pasta */ + update = true; + break; + case 's': + show = true; + break; case 'h': usage(argv[0], stdout, EXIT_SUCCESS); break; @@ -290,6 +348,8 @@ int main(int argc, char **argv) die_perror("Failed to connect to %s", a.sun_path); }

+ debug("Connected to passt/pasta control socket"); + ret = read_all_buf(s, &hello, sizeof(hello)); if (ret < 0) die_perror("Couldn't read server greeting"); @@ -327,9 +387,52 @@ int main(int argc, char **argv) while (read_pif_conf(s, &conf)) ;

- printf("passt/pasta configuration (%s)\n", a.sun_path); - show_conf(&conf); + if (!update) { + printf("passt/pasta configuration (%s)\n", a.sun_path); + show_conf(&conf); + goto noupdate; + } + + if (show) { + printf("Previous configuration (%s)\n", a.sun_path); + show_conf(&conf); + } + + inbound = pif_conf_by_name(&conf, "HOST"); + outbound = pif_conf_by_name(&conf, "SPLICE"); + + optind = 0; + do { + optname = getopt_long(argc, argv, optstring, options, NULL); + + switch (optname) { + case 't': + case 'u': + if (!inbound) { + die("Can't use -%c, no inbound interface", + optname); + } + fwd_rule_parse(optname, optarg, &inbound->fwd); + break; + case 'T': + case 'U': + if (!outbound) { + die("Can't use -%c, no outbound interface", + optname); + } + fwd_rule_parse(optname, optarg, &outbound->fwd); + break; + default: + continue; + } + } while (optname != -1); + + if (show) { + printf("Updated configuration (%s)\n", a.sun_path); + show_conf(&conf); + }

+noupdate: if (shutdown(s, SHUT_RDWR) < 0 || close(s) < 0) die_perror("Error shutting down control socket");

On 5/5/26 01:11, Stefano Brivio wrote:

From: David Gibson

We can now receive updates to the forwarding rules from the pesto client and store them in a "pending" copy of the forwarding tables. Implement switching to using the new rules.

The logic is in a new fwd_listen_switch(). For now this closes all listening sockets related to the old tables, swaps the active and pending tables, then listens based on the new tables. In future we look to improve this so that we don't temporarily stop listening on ports that both the old and new tables specify.

Signed-off-by: David Gibson Signed-off-by: Stefano Brivio --- conf.c | 5 ++--- fwd.c | 34 ++++++++++++++++++++++++++++++++++ fwd.h | 1 + 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/conf.c b/conf.c index f035fd3..75b8291 100644 --- a/conf.c +++ b/conf.c @@ -2159,15 +2159,14 @@ void conf_handler(struct ctx *c, uint32_t events) fwd_rules_dump(info, fwd->rules, fwd->count, " ", ""); } + + fwd_listen_switch(c); }

if (events & EPOLLHUP) { debug("Configuration client hangup"); - goto close; }

- return; - close: conf_close(c);

diff --git a/fwd.c b/fwd.c index d93d2e5..35b9e2b 100644 --- a/fwd.c +++ b/fwd.c @@ -534,6 +534,40 @@ int fwd_listen_init(const struct ctx *c) return 0; }

+/** + * fwd_listen_switch() - Switch from current to pending rules table + * @c: Execution context + */ +void fwd_listen_switch(struct ctx *c) +{ + struct fwd_table *tmp[PIF_NUM_TYPES]; + unsigned i; + + /* Stop listening on the old tables */ + for (i = 0; i < PIF_NUM_TYPES; i++) { + struct fwd_table *fwd = c->fwd[i]; + + if (!fwd) + continue; + + debug("Flushing %u old %s rules", fwd->count, pif_name(i)); + fwd_listen_close(fwd); + fwd->count = fwd->sock_count = 0;

Perhaps we can reset fwd->count and fwd->sock_count in fwd_listen_close() as after fwd_listen_close() these values are wrong?

+ } + + /* Swap active and pending tables */ + static_assert(sizeof(tmp) == sizeof(c->fwd) && + sizeof(tmp) == sizeof(c->fwd_pending), + "Temporary has wrong size"); + memcpy(&tmp, (void *)c->fwd, sizeof(tmp)); + memcpy((void *)c->fwd, (void *)c->fwd_pending, sizeof(tmp)); + memcpy((void *)c->fwd_pending, &tmp, sizeof(tmp));

I know we have the static_assert(), but with memcpy() we usually use the sizeof() of the destination to avoid write overflow. Why do we keep the old active table? Do we plan to have a "--restore" option?

+ + /* Start listening on the new tables */ + if (fwd_listen_init(c) < 0) + err("Error switching to new forwarding rules"); +} + /* See enum in kernel's include/net/tcp_states.h */ #define UDP_LISTEN 0x07 #define TCP_LISTEN 0x0a diff --git a/fwd.h b/fwd.h index ac24782..b60697d 100644 --- a/fwd.h +++ b/fwd.h @@ -61,6 +61,7 @@ int fwd_listen_sync(const struct ctx *c, uint8_t pif, const struct fwd_scan *tcp, const struct fwd_scan *udp); void fwd_listen_close(const struct fwd_table *fwd); int fwd_listen_init(const struct ctx *c); +void fwd_listen_switch(struct ctx *c);

bool nat_inbound(const struct ctx *c, const union inany_addr *addr, union inany_addr *translated);

On Tue, May 05, 2026 at 09:53:04AM +0200, Laurent Vivier wrote:

On 5/5/26 01:11, Stefano Brivio wrote:

...

From: David Gibson

Extend pesto to send the updated rule configuration back to passt/pasta. Extend passt/pasta to read the new configuration and store the new rules in a "pending" table. We don't yet attempt to activate them.

Signed-off-by: Stefano Brivio [dwg: Based on an early draft from Stefano] [sbrivio: Add redundant check on interface names being terminated in conf_recv_rules(), to make static checkers happy] [sbrivio: Make conf_recv_rules() return -1 if fwd_rule_read() fails, as suggested by Jon Maloy] Signed-off-by: David Gibson

Reviewed-by: Laurent Vivier

But one comment below

...

--- Makefile | 5 --- conf.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++-------- fwd.c | 10 +++++- passt.h | 2 ++ pesto.c | 35 +++++++++++++++++++++ 5 files changed, 127 insertions(+), 19 deletions(-)

diff --git a/Makefile b/Makefile index c746b55..ae755a0 100644 --- a/Makefile +++ b/Makefile @@ -224,10 +224,6 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck qrap.cppcheck $(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^ passt.cppcheck: BASE_CPPFLAGS += -UPESTO -passt.cppcheck: CPPCHECK_FLAGS += \ - --suppress=unusedFunction:fwd_rule.c \ - --suppress=staticFunction:fwd_rule.c \ - --suppress=unusedFunction:serialise.c passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h @@ -238,7 +234,6 @@ pesto.cppcheck: CPPCHECK_FLAGS += \ --suppress=unusedFunction:inany.h \ --suppress=unusedFunction:inany.c \ --suppress=unusedFunction:ip.h \ - --suppress=unusedFunction:fwd_rule.c \ --suppress=staticFunction:fwd_rule.c \ --suppress=unusedFunction:serialise.c pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h diff --git a/conf.c b/conf.c index 5e4e81e..f035fd3 100644 --- a/conf.c +++ b/conf.c @@ -1971,6 +1971,62 @@ static int conf_send_rules(const struct ctx *c, int fd) return 0; } +/** + * conf_recv_rules() - Receive forwarding rules from configuration client + * @c: Execution context + * @fd: Socket to the client + * + * Return: 0 on success, -1 on failure + */ +static int conf_recv_rules(const struct ctx *c, int fd) +{ + while (1) { + struct fwd_table *fwd; + struct fwd_rule r; + uint32_t count; + uint8_t pif; + unsigned i; + + if (read_u8(fd, &pif)) + return -1; + + if (pif == PIF_NONE) + break; + + if (pif >= ARRAY_SIZE(c->fwd_pending) || + !(fwd = c->fwd_pending[pif])) { + err("Received rules for non-existent table"); + return -1; + } + + if (read_u32(fd, &count)) + return -1; + + if (count > MAX_FWD_RULES) { + err("Received %"PRIu32" rules (maximum %u)", + count, MAX_FWD_RULES); + return -1; + } + + for (i = 0; i < count; i++) { + if (fwd_rule_read(fd, &r)) + return -1; + + if (r.ifname[sizeof(r.ifname) - 1]) { + err("Interface name was not NULL terminated"); + return -1; + } + /* Redundant, to make static checkers happy */ + r.ifname[sizeof(r.ifname) - 1] = '\0'; + + if (fwd_rule_add(fwd, &r) < 0) + return -1; + } + } + + return 0; +} + /** * conf_close() - Close configuration / control socket and clean up * @c: Execution context @@ -2075,21 +2131,33 @@ fail: void conf_handler(struct ctx *c, uint32_t events) { if (events & EPOLLIN) { - char discard[BUFSIZ]; - ssize_t n; - - do { - n = read(c->fd_control, discard, sizeof(discard)); - if (n > 0) - debug("Discarded %zd bytes of config data", n); - } while (n > 0); - if (n == 0) { - debug("Configuration client EOF"); - goto close; + unsigned pif; + + /* Clear pending tables */ + for (pif = 0; pif < PIF_NUM_TYPES; pif++) { + struct fwd_table *fwd = c->fwd_pending[pif]; + + if (!fwd) + continue; + fwd->count = 0; + fwd->sock_count = 0; } - if (errno != EAGAIN && errno != EWOULDBLOCK) { - err_perror("Error reading config data"); + + /* FIXME: this could block indefinitely if the client doesn't + * write as much as it should + */ + if (conf_recv_rules(c, c->fd_control) < 0) goto close; + + for (pif = 0; pif < PIF_NUM_TYPES; pif++) { + struct fwd_table *fwd = c->fwd_pending[pif]; + + if (!fwd) + continue; + + info("New forwarding rules for %s:", pif_name(pif)); + fwd_rules_dump(info, fwd->rules, fwd->count, + " ", ""); } } diff --git a/fwd.c b/fwd.c index 8849cfc..d93d2e5 100644 --- a/fwd.c +++ b/fwd.c @@ -247,6 +247,9 @@ void fwd_neigh_table_init(const struct ctx *c) static struct fwd_table fwd_in; static struct fwd_table fwd_out; +static struct fwd_table fwd_in_pending; +static struct fwd_table fwd_out_pending; + /** * fwd_rule_init() - Initialise forwarding tables * @c: Execution context @@ -269,10 +272,15 @@ void fwd_rule_init(struct ctx *c) caps |= FWD_CAP_IFNAME; fwd_in.caps = fwd_out.caps = caps; + fwd_in_pending.caps = fwd_out_pending.caps = caps; c->fwd[PIF_HOST] = &fwd_in; - if (c->mode == MODE_PASTA) + c->fwd_pending[PIF_HOST] = &fwd_in_pending; + + if (c->mode == MODE_PASTA) { c->fwd[PIF_SPLICE] = &fwd_out; + c->fwd_pending[PIF_SPLICE] = &fwd_out_pending; + } } /** diff --git a/passt.h b/passt.h index b3f049d..1726965 100644 --- a/passt.h +++ b/passt.h @@ -188,6 +188,7 @@ struct ip6_ctx { * @pasta_ifi: Index of namespace interface for pasta * @pasta_conf_ns: Configure namespace after creating it * @fwd: Forwarding tables + * @fwd_pending: Pending forward tables * @no_tcp: Disable TCP operation * @tcp: Context for TCP protocol handler * @no_udp: Disable UDP operation @@ -270,6 +271,7 @@ struct ctx { int pasta_conf_ns; struct fwd_table *fwd[PIF_NUM_TYPES]; + struct fwd_table *fwd_pending[PIF_NUM_TYPES]; int no_tcp; struct tcp_ctx tcp; diff --git a/pesto.c b/pesto.c index 16b3a5a..73fdc39 100644 --- a/pesto.c +++ b/pesto.c @@ -230,6 +230,39 @@ static bool read_pif_conf(int fd, struct configuration *conf) return true; } +/** + * send_conf() - Send updated configuration to passt/pasta + * @fd: Control socket + * @conf: Updated configuration + */ +static void send_conf(int fd, const struct configuration *conf) +{ + unsigned i; +

Perhaps it could be interesting to send a magic number (or a type id) if we want to be able to update something else than the rules in the future? We also can send the length of the data if we want to be able to ignore it if the type id is not supported? (Something like the chunks in IFF or PNG file format... but perhaps it's overcomplicated for our purpose...)

Undoubtedly, amongst a bunch of other ways we could make the protocol more flexible. However, I'm inclined to leave that v2.

...

+ for (i = 0; i < conf->npifs; i++) { + const struct pif_configuration *pc = &conf->pif[i]; + unsigned j; + + if (write_u8(fd, pc->pif) < 0) + goto fail; + + if (write_u32(fd, pc->fwd.count) < 0) + goto fail; + + for (j = 0; j < pc->fwd.count; j++) { + if (fwd_rule_write(fd, &pc->fwd.rules[j]) < 0) + goto fail; + } + } + + if (write_u8(fd, PIF_NONE) < 0) + goto fail; + return; + +fail: + die_perror("Error writing to control socket"); +} + /** * show_conf() - Show current configuration obtained from passt/pasta * @conf: Configuration description @@ -432,6 +465,8 @@ int main(int argc, char **argv) show_conf(&conf); } + send_conf(s, &conf); + noupdate: if (shutdown(s, SHUT_RDWR) < 0 || close(s) < 0) die_perror("Error shutting down control socket");

-- 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, 5 May 2026 11:08:27 +0200 Laurent Vivier wrote:

On 5/5/26 01:11, Stefano Brivio wrote:

...

From: David Gibson

We can now receive updates to the forwarding rules from the pesto client and store them in a "pending" copy of the forwarding tables. Implement switching to using the new rules.

The logic is in a new fwd_listen_switch(). For now this closes all listening sockets related to the old tables, swaps the active and pending tables, then listens based on the new tables. In future we look to improve this so that we don't temporarily stop listening on ports that both the old and new tables specify.

Signed-off-by: David Gibson Signed-off-by: Stefano Brivio --- conf.c | 5 ++--- fwd.c | 34 ++++++++++++++++++++++++++++++++++ fwd.h | 1 + 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/conf.c b/conf.c index f035fd3..75b8291 100644 --- a/conf.c +++ b/conf.c @@ -2159,15 +2159,14 @@ void conf_handler(struct ctx *c, uint32_t events) fwd_rules_dump(info, fwd->rules, fwd->count, " ", ""); } + + fwd_listen_switch(c); }

if (events & EPOLLHUP) { debug("Configuration client hangup"); - goto close; }

- return; - close: conf_close(c);

diff --git a/fwd.c b/fwd.c index d93d2e5..35b9e2b 100644 --- a/fwd.c +++ b/fwd.c @@ -534,6 +534,40 @@ int fwd_listen_init(const struct ctx *c) return 0; }

+/** + * fwd_listen_switch() - Switch from current to pending rules table + * @c: Execution context + */ +void fwd_listen_switch(struct ctx *c) +{ + struct fwd_table *tmp[PIF_NUM_TYPES]; + unsigned i; + + /* Stop listening on the old tables */ + for (i = 0; i < PIF_NUM_TYPES; i++) { + struct fwd_table *fwd = c->fwd[i]; + + if (!fwd) + continue; + + debug("Flushing %u old %s rules", fwd->count, pif_name(i)); + fwd_listen_close(fwd); + fwd->count = fwd->sock_count = 0;

Perhaps we can reset fwd->count and fwd->sock_count in fwd_listen_close() as after fwd_listen_close() these values are wrong?

Right, while not strictly necessary it still looks like a good idea, I'll change that.

...

+ } + + /* Swap active and pending tables */ + static_assert(sizeof(tmp) == sizeof(c->fwd) && + sizeof(tmp) == sizeof(c->fwd_pending), + "Temporary has wrong size"); + memcpy(&tmp, (void *)c->fwd, sizeof(tmp)); + memcpy((void *)c->fwd, (void *)c->fwd_pending, sizeof(tmp)); + memcpy((void *)c->fwd_pending, &tmp, sizeof(tmp));

I know we have the static_assert(), but with memcpy() we usually use the sizeof() of the destination to avoid write overflow.

I'll change this as well.

Why do we keep the old active table? Do we plan to have a "--restore" option?

It's just to add and delete rules using a temporary table so that we can abort cleanly and atomically on errors. Are you asking why we don't wipe the old table afterwards? No particular reason for that, even though I'm not sure if it's useful. Actually some kind of --restore option might be desirable, even though we would probably need to re-validate all the rules, or keep a "dirty" bit that's set on other types of changes and would tell us that the previous table can't be used as it is anymore. -- Stefano

...

+ + /* Start listening on the new tables */ + if (fwd_listen_init(c) < 0) + err("Error switching to new forwarding rules"); +} + /* See enum in kernel's include/net/tcp_states.h */ #define UDP_LISTEN 0x07 #define TCP_LISTEN 0x0a diff --git a/fwd.h b/fwd.h index ac24782..b60697d 100644 --- a/fwd.h +++ b/fwd.h @@ -61,6 +61,7 @@ int fwd_listen_sync(const struct ctx *c, uint8_t pif, const struct fwd_scan *tcp, const struct fwd_scan *udp); void fwd_listen_close(const struct fwd_table *fwd); int fwd_listen_init(const struct ctx *c); +void fwd_listen_switch(struct ctx *c);

bool nat_inbound(const struct ctx *c, const union inany_addr *addr, union inany_addr *translated);

On Tue, 5 May 2026 19:53:43 +1000 David Gibson wrote:

On Tue, May 05, 2026 at 11:08:27AM +0200, Laurent Vivier wrote:

...

On 5/5/26 01:11, Stefano Brivio wrote:

...

From: David Gibson

We can now receive updates to the forwarding rules from the pesto client and store them in a "pending" copy of the forwarding tables. Implement switching to using the new rules.

The logic is in a new fwd_listen_switch(). For now this closes all listening sockets related to the old tables, swaps the active and pending tables, then listens based on the new tables. In future we look to improve this so that we don't temporarily stop listening on ports that both the old and new tables specify.

Signed-off-by: David Gibson Signed-off-by: Stefano Brivio --- conf.c | 5 ++--- fwd.c | 34 ++++++++++++++++++++++++++++++++++ fwd.h | 1 + 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/conf.c b/conf.c index f035fd3..75b8291 100644 --- a/conf.c +++ b/conf.c @@ -2159,15 +2159,14 @@ void conf_handler(struct ctx *c, uint32_t events) fwd_rules_dump(info, fwd->rules, fwd->count, " ", ""); } + + fwd_listen_switch(c); } if (events & EPOLLHUP) { debug("Configuration client hangup"); - goto close; } - return; - close: conf_close(c); diff --git a/fwd.c b/fwd.c index d93d2e5..35b9e2b 100644 --- a/fwd.c +++ b/fwd.c @@ -534,6 +534,40 @@ int fwd_listen_init(const struct ctx *c) return 0; } +/** + * fwd_listen_switch() - Switch from current to pending rules table + * @c: Execution context + */ +void fwd_listen_switch(struct ctx *c) +{ + struct fwd_table *tmp[PIF_NUM_TYPES]; + unsigned i; + + /* Stop listening on the old tables */ + for (i = 0; i < PIF_NUM_TYPES; i++) { + struct fwd_table *fwd = c->fwd[i]; + + if (!fwd) + continue; + + debug("Flushing %u old %s rules", fwd->count, pif_name(i)); + fwd_listen_close(fwd); + fwd->count = fwd->sock_count = 0;

Perhaps we can reset fwd->count and fwd->sock_count in fwd_listen_close() as after fwd_listen_close() these values are wrong?

No, they're not. fwd_listen_close() closes the listening sockets, but it doesn't remove the rules. fwd->sock_count isn't the number of *open* listening sockets, it's the maximum potential number of sockets for all the rules. Having some or all of the sockets close (-1 stored in the array) is an allowed state. It's rare for most rules, but routine for SCAN ("auto") rules.

Ah, oops, I didn't realise that would be the case for "auto" rules. I'll leave this part as it is then. -- Stefano

On Tue, May 05, 2026 at 12:15:14PM +0200, Stefano Brivio wrote:

On Tue, 5 May 2026 19:53:43 +1000 David Gibson wrote:

...

On Tue, May 05, 2026 at 11:08:27AM +0200, Laurent Vivier wrote:

...

On 5/5/26 01:11, Stefano Brivio wrote:

...

From: David Gibson

We can now receive updates to the forwarding rules from the pesto client and store them in a "pending" copy of the forwarding tables. Implement switching to using the new rules.

The logic is in a new fwd_listen_switch(). For now this closes all listening sockets related to the old tables, swaps the active and pending tables, then listens based on the new tables. In future we look to improve this so that we don't temporarily stop listening on ports that both the old and new tables specify.

Signed-off-by: David Gibson Signed-off-by: Stefano Brivio --- conf.c | 5 ++--- fwd.c | 34 ++++++++++++++++++++++++++++++++++ fwd.h | 1 + 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/conf.c b/conf.c index f035fd3..75b8291 100644 --- a/conf.c +++ b/conf.c @@ -2159,15 +2159,14 @@ void conf_handler(struct ctx *c, uint32_t events) fwd_rules_dump(info, fwd->rules, fwd->count, " ", ""); } + + fwd_listen_switch(c); } if (events & EPOLLHUP) { debug("Configuration client hangup"); - goto close; } - return; - close: conf_close(c); diff --git a/fwd.c b/fwd.c index d93d2e5..35b9e2b 100644 --- a/fwd.c +++ b/fwd.c @@ -534,6 +534,40 @@ int fwd_listen_init(const struct ctx *c) return 0; } +/** + * fwd_listen_switch() - Switch from current to pending rules table + * @c: Execution context + */ +void fwd_listen_switch(struct ctx *c) +{ + struct fwd_table *tmp[PIF_NUM_TYPES]; + unsigned i; + + /* Stop listening on the old tables */ + for (i = 0; i < PIF_NUM_TYPES; i++) { + struct fwd_table *fwd = c->fwd[i]; + + if (!fwd) + continue; + + debug("Flushing %u old %s rules", fwd->count, pif_name(i)); + fwd_listen_close(fwd); + fwd->count = fwd->sock_count = 0;

Perhaps we can reset fwd->count and fwd->sock_count in fwd_listen_close() as after fwd_listen_close() these values are wrong?

No, they're not. fwd_listen_close() closes the listening sockets, but it doesn't remove the rules. fwd->sock_count isn't the number of *open* listening sockets, it's the maximum potential number of sockets for all the rules. Having some or all of the sockets close (-1 stored in the array) is an allowed state. It's rare for most rules, but routine for SCAN ("auto") rules.

Ah, oops, I didn't realise that would be the case for "auto" rules. I'll leave this part as it is then.

Even for non-auto rules, it's still conceptually correct to leave sock_count alonw in fwd_listen_close(). sock_count is really tracking the amount of space in the array allocated to store socket fds not the socket fds themselves. I'm open to different names that make that more obvious. -- 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, May 05, 2026 at 12:04:09PM +0200, Stefano Brivio wrote:

On Tue, 5 May 2026 11:08:27 +0200 Laurent Vivier wrote:

...

On 5/5/26 01:11, Stefano Brivio wrote:

...

From: David Gibson

We can now receive updates to the forwarding rules from the pesto client and store them in a "pending" copy of the forwarding tables. Implement switching to using the new rules.

The logic is in a new fwd_listen_switch(). For now this closes all listening sockets related to the old tables, swaps the active and pending tables, then listens based on the new tables. In future we look to improve this so that we don't temporarily stop listening on ports that both the old and new tables specify.

Signed-off-by: David Gibson Signed-off-by: Stefano Brivio --- conf.c | 5 ++--- fwd.c | 34 ++++++++++++++++++++++++++++++++++ fwd.h | 1 + 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/conf.c b/conf.c index f035fd3..75b8291 100644 --- a/conf.c +++ b/conf.c @@ -2159,15 +2159,14 @@ void conf_handler(struct ctx *c, uint32_t events) fwd_rules_dump(info, fwd->rules, fwd->count, " ", ""); } + + fwd_listen_switch(c); }

if (events & EPOLLHUP) { debug("Configuration client hangup"); - goto close; }

- return; - close: conf_close(c);

diff --git a/fwd.c b/fwd.c index d93d2e5..35b9e2b 100644 --- a/fwd.c +++ b/fwd.c @@ -534,6 +534,40 @@ int fwd_listen_init(const struct ctx *c) return 0; }

+/** + * fwd_listen_switch() - Switch from current to pending rules table + * @c: Execution context + */ +void fwd_listen_switch(struct ctx *c) +{ + struct fwd_table *tmp[PIF_NUM_TYPES]; + unsigned i; + + /* Stop listening on the old tables */ + for (i = 0; i < PIF_NUM_TYPES; i++) { + struct fwd_table *fwd = c->fwd[i]; + + if (!fwd) + continue; + + debug("Flushing %u old %s rules", fwd->count, pif_name(i)); + fwd_listen_close(fwd); + fwd->count = fwd->sock_count = 0;

Perhaps we can reset fwd->count and fwd->sock_count in fwd_listen_close() as after fwd_listen_close() these values are wrong?

Right, while not strictly necessary it still looks like a good idea, I'll change that.

As noted elswhere this is correct as it is.

...

...

+ } + + /* Swap active and pending tables */ + static_assert(sizeof(tmp) == sizeof(c->fwd) && + sizeof(tmp) == sizeof(c->fwd_pending), + "Temporary has wrong size"); + memcpy(&tmp, (void *)c->fwd, sizeof(tmp)); + memcpy((void *)c->fwd, (void *)c->fwd_pending, sizeof(tmp)); + memcpy((void *)c->fwd_pending, &tmp, sizeof(tmp));

I know we have the static_assert(), but with memcpy() we usually use the sizeof() of the destination to avoid write overflow.

I'll change this as well.

Sounds good.

...

Why do we keep the old active table? Do we plan to have a "--restore" option?

It's just to add and delete rules using a temporary table so that we can abort cleanly and atomically on errors.

Are you asking why we don't wipe the old table afterwards? No particular reason for that, even though I'm not sure if it's useful.

Also looking ahead to listening socket continuity. In that case we need to keep the old table until we've stolen all the sockets we can re-use for the new table. *Then* we can fwd_listen_close() anything left over, and wipe the table.

Actually some kind of --restore option might be desirable, even though we would probably need to re-validate all the rules, or keep a "dirty" bit that's set on other types of changes and would tell us that the previous table can't be used as it is anymore.

-- Stefano

...

...

+ + /* Start listening on the new tables */ + if (fwd_listen_init(c) < 0) + err("Error switching to new forwarding rules"); +} + /* See enum in kernel's include/net/tcp_states.h */ #define UDP_LISTEN 0x07 #define TCP_LISTEN 0x0a diff --git a/fwd.h b/fwd.h index ac24782..b60697d 100644 --- a/fwd.h +++ b/fwd.h @@ -61,6 +61,7 @@ int fwd_listen_sync(const struct ctx *c, uint8_t pif, const struct fwd_scan *tcp, const struct fwd_scan *udp); void fwd_listen_close(const struct fwd_table *fwd); int fwd_listen_init(const struct ctx *c); +void fwd_listen_switch(struct ctx *c);

bool nat_inbound(const struct ctx *c, const union inany_addr *addr, union inany_addr *translated);

-- 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, 5 May 2026 11:08:27 +0200 Laurent Vivier wrote:

On 5/5/26 01:11, Stefano Brivio wrote:

...

From: David Gibson

We can now receive updates to the forwarding rules from the pesto client and store them in a "pending" copy of the forwarding tables. Implement switching to using the new rules.

The logic is in a new fwd_listen_switch(). For now this closes all listening sockets related to the old tables, swaps the active and pending tables, then listens based on the new tables. In future we look to improve this so that we don't temporarily stop listening on ports that both the old and new tables specify.

Signed-off-by: David Gibson Signed-off-by: Stefano Brivio --- conf.c | 5 ++--- fwd.c | 34 ++++++++++++++++++++++++++++++++++ fwd.h | 1 + 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/conf.c b/conf.c index f035fd3..75b8291 100644 --- a/conf.c +++ b/conf.c @@ -2159,15 +2159,14 @@ void conf_handler(struct ctx *c, uint32_t events) fwd_rules_dump(info, fwd->rules, fwd->count, " ", ""); } + + fwd_listen_switch(c); }

if (events & EPOLLHUP) { debug("Configuration client hangup"); - goto close; }

- return; - close: conf_close(c);

diff --git a/fwd.c b/fwd.c index d93d2e5..35b9e2b 100644 --- a/fwd.c +++ b/fwd.c @@ -534,6 +534,40 @@ int fwd_listen_init(const struct ctx *c) return 0; }

+/** + * fwd_listen_switch() - Switch from current to pending rules table + * @c: Execution context + */ +void fwd_listen_switch(struct ctx *c) +{ + struct fwd_table *tmp[PIF_NUM_TYPES]; + unsigned i; + + /* Stop listening on the old tables */ + for (i = 0; i < PIF_NUM_TYPES; i++) { + struct fwd_table *fwd = c->fwd[i]; + + if (!fwd) + continue; + + debug("Flushing %u old %s rules", fwd->count, pif_name(i)); + fwd_listen_close(fwd); + fwd->count = fwd->sock_count = 0;

Perhaps we can reset fwd->count and fwd->sock_count in fwd_listen_close() as after fwd_listen_close() these values are wrong?

...

+ } + + /* Swap active and pending tables */ + static_assert(sizeof(tmp) == sizeof(c->fwd) && + sizeof(tmp) == sizeof(c->fwd_pending), + "Temporary has wrong size"); + memcpy(&tmp, (void *)c->fwd, sizeof(tmp)); + memcpy((void *)c->fwd, (void *)c->fwd_pending, sizeof(tmp)); + memcpy((void *)c->fwd_pending, &tmp, sizeof(tmp));

I know we have the static_assert(), but with memcpy() we usually use the sizeof() of the destination to avoid write overflow.

Changed in v8. -- Stefano

On Wed, May 06, 2026 at 09:46:44AM +0200, Stefano Brivio wrote:

On Wed, 6 May 2026 00:41:15 +1000 David Gibson wrote:

...

On Tue, May 05, 2026 at 12:13:41PM +0200, Stefano Brivio wrote:

...

On Tue, 5 May 2026 16:22:43 +1000 David Gibson wrote:

...

On Tue, May 05, 2026 at 01:11:42AM +0200, Stefano Brivio wrote:

...

The new checks are actually sufficient but not enough for Coverity Scan. Now that fwd->sock_count and new->last are affected or supplied by clients, we need explicit (albeit redundant) checks on them.

Signed-off-by: Stefano Brivio

I'm assuming this does squash the warnings, but I think it does so in a somewhat confusing way.

You don't need to assume that, you could try yourself without this patch and you'll see exactly two warnings with a lot of details.

I'm getting better, but I'm by no means well yet. Some emails were within my capacity. Sorting out the new license key and doing a Coverity run, not so much.

...

...

...

--- fwd_rule.c | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/fwd_rule.c b/fwd_rule.c index b55e4df..03e8e80 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -271,13 +271,22 @@ int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new) warn("Too many rules (maximum %d)", ARRAY_SIZE(fwd->rules)); return -ENOSPC; } + if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) { warn("Rules require too many listening sockets (maximum %d)", ARRAY_SIZE(fwd->socks)); return -ENOSPC; } + /* Redundant, to make static checkers happy */ + if (fwd->sock_count > ARRAY_SIZE(fwd->socks)) + return -ENOSPC;

So there's actually two conditions that this is kind of relevant to:

1) (fwd->sock_count > ARRAY_SIZE(fwd->socks)) on entry

That means something is horribly wrong before we were even called. So, I think that would be better as an assert().

But the (good) reason why Coverity Scan is complaining about a missing check here is that the client is able to manipulate sock_count (via num) in a previous call to this function,

Uh... they should never be able to make sock_count outside of [0, ARRAY_SIZE(socks)). If they can, that's a bug elsewhere.

so making us crash is exactly what we want to avoid:

/home/sbrivio/passt/conf.c:2024:4: Type: Untrusted value as argument (TAINTED_SCALAR)

/home/sbrivio/passt/conf.c:1992:3: Tainted data flows to a taint sink 1. path: Condition "read_u8(fd, &pif)", taking false branch. /home/sbrivio/passt/conf.c:1995:3: 2. path: Condition "pif == 0", taking false branch. /home/sbrivio/passt/conf.c:1998:3: 3. path: Condition "pif >= 4 /* (int)(sizeof (c->fwd_pending) / sizeof (c->fwd_pending[0])) */", taking false branch. /home/sbrivio/passt/conf.c:1998:3: 4. path: Condition "!(fwd = c->fwd_pending[pif])", taking false branch. /home/sbrivio/passt/conf.c:2004:3: 5. path: Condition "read_u32(fd, &count)", taking false branch. /home/sbrivio/passt/conf.c:2007:3: 6. path: Condition "count > 255U /* (1U << 8) - 1 */", taking false branch. /home/sbrivio/passt/conf.c:2013:3: 7. path: Condition "i < count", taking true branch. /home/sbrivio/passt/conf.c:2014:4: 8. tainted_argument: Calling function "fwd_rule_read" taints argument "r". /home/sbrivio/passt/fwd_rule.c:659:2: Tainted data flows to a taint sink 8.1. tainted_data_argument: Calling function "read_all_buf" taints parameter "*rule". /home/sbrivio/passt/serialise.c:39:2: Tainted data flows to a taint sink 8.1.1. var_assign_parm: Assigning: "p" = "buf". /home/sbrivio/passt/serialise.c:41:2: 8.1.2. path: Condition "left", taking true branch. /home/sbrivio/passt/serialise.c:44:3: 8.1.3. path: Condition "left <= len", taking true branch. /home/sbrivio/passt/serialise.c:47:4: 8.1.4. tainted_data_argument: Calling function "read" taints parameter "*p". [Note: The source code implementation of the function has been overridden by a builtin model.] /home/sbrivio/passt/serialise.c:48:37: 8.1.5. path: Condition "rc < 0", taking false branch. /home/sbrivio/passt/serialise.c:50:3: 8.1.6. path: Condition "rc < 0", taking false branch. /home/sbrivio/passt/serialise.c:53:3: 8.1.7. path: Condition "rc == 0", taking false branch. /home/sbrivio/passt/serialise.c:60:2: 8.1.8. path: Jumping back to the beginning of the loop. /home/sbrivio/passt/serialise.c:41:2: 8.1.9. path: Condition "left", taking true branch. /home/sbrivio/passt/serialise.c:44:3: 8.1.10. path: Condition "left <= len", taking true branch. /home/sbrivio/passt/serialise.c:48:37: 8.1.11. path: Condition "rc < 0", taking false branch. /home/sbrivio/passt/serialise.c:50:3: 8.1.12. path: Condition "rc < 0", taking false branch. /home/sbrivio/passt/serialise.c:53:3: 8.1.13. path: Condition "rc == 0", taking false branch. /home/sbrivio/passt/serialise.c:60:2: 8.1.14. path: Jumping back to the beginning of the loop. /home/sbrivio/passt/serialise.c:41:2: 8.1.15. path: Condition "left", taking false branch. /home/sbrivio/passt/fwd_rule.c:659:2: 8.2. path: Condition "read_all_buf(fd, rule, 40UL /* sizeof (*rule) */)", taking false branch. /home/sbrivio/passt/conf.c:2014:4: 9. path: Condition "fwd_rule_read(fd, &r)", taking false branch. /home/sbrivio/passt/conf.c:2017:4: 10. path: Condition "r.ifname[15UL /* sizeof (r.ifname) - 1 */]", taking false branch. /home/sbrivio/passt/conf.c:2024:4: 11. tainted_data_transitive: Call to function "fwd_rule_add" with tainted argument "r.last" transitively taints "fwd->sock_count". /home/sbrivio/passt/fwd_rule.c:197:2: Tainted data flows to a taint sink 11.1. path: Condition "new->first > new->last", taking false branch. /home/sbrivio/passt/fwd_rule.c:202:2: 11.2. path: Condition "!new->first", taking false branch. /home/sbrivio/passt/fwd_rule.c:206:2: 11.3. path: Condition "!new->to", taking false branch. /home/sbrivio/passt/fwd_rule.c:206:2: 11.4. path: Condition "(in_port_t)(new->to + new->last - new->first) < new->to", taking false branch. /home/sbrivio/passt/fwd_rule.c:211:2: 11.5. path: Condition "new->flags & -8 /* ~allowed_flags */", taking false branch. /home/sbrivio/passt/fwd_rule.c:216:2: 11.6. path: Condition "new->flags & (1UL /* 1UL << 0 */)", taking true branch. /home/sbrivio/passt/fwd_rule.c:217:3: 11.7. path: Condition "!inany_equals(&new->addr, (union inany_addr const *)&in6addr_any)", taking false branch. /home/sbrivio/passt/fwd_rule.c:224:3: 11.8. path: Condition "!(fwd->caps & (1UL /* 1UL << 0 */))", taking false branch. /home/sbrivio/passt/fwd_rule.c:228:3: 11.9. path: Condition "!(fwd->caps & (2UL /* 1UL << 1 */))", taking false branch. /home/sbrivio/passt/fwd_rule.c:232:2: 11.10. path: Falling through to end of if statement. /home/sbrivio/passt/fwd_rule.c:242:2: 11.11. path: Condition "new->proto == IPPROTO_TCP", taking true branch. /home/sbrivio/passt/fwd_rule.c:243:3: 11.12. path: Condition "!(fwd->caps & (4UL /* 1UL << 2 */))", taking false branch. /home/sbrivio/passt/fwd_rule.c:247:2: 11.13. path: Falling through to end of if statement. /home/sbrivio/passt/fwd_rule.c:258:2: 11.14. path: Condition "i < fwd->count", taking true branch. /home/sbrivio/passt/fwd_rule.c:261:3: 11.15. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. /home/sbrivio/passt/fwd_rule.c:262:4: 11.16. path: Continuing loop. /home/sbrivio/passt/fwd_rule.c:258:2: 11.17. path: Condition "i < fwd->count", taking true branch. /home/sbrivio/passt/fwd_rule.c:261:3: 11.18. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. /home/sbrivio/passt/fwd_rule.c:262:4: 11.19. path: Continuing loop. /home/sbrivio/passt/fwd_rule.c:258:2: 11.20. path: Condition "i < fwd->count", taking true branch. /home/sbrivio/passt/fwd_rule.c:261:3: 11.21. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. /home/sbrivio/passt/fwd_rule.c:262:4: 11.22. path: Continuing loop. /home/sbrivio/passt/fwd_rule.c:258:2: 11.23. path: Condition "i < fwd->count", taking false branch. /home/sbrivio/passt/fwd_rule.c:270:2: 11.24. path: Condition "fwd->count >= 255U /* (int)(sizeof (fwd->rules) / sizeof (fwd->rules[0])) */", taking false branch. /home/sbrivio/passt/fwd_rule.c:274:2: 11.25. path: Condition "fwd->sock_count + num > 327680U /* (int)(sizeof (fwd->socks) / sizeof (fwd->socks[0])) */", taking false branch. /home/sbrivio/passt/fwd_rule.c:281:2: 11.26. path: Condition "port <= new->last", taking true branch. /home/sbrivio/passt/fwd_rule.c:282:53: 11.27. path: Jumping back to the beginning of the loop. /home/sbrivio/passt/fwd_rule.c:281:2: 11.28. path: Condition "port <= new->last", taking false branch. /home/sbrivio/passt/fwd_rule.c:285:2: 11.29. parm_assign: Assigning: "fwd->sock_count" += "num", which taints "fwd->sock_count". /home/sbrivio/passt/conf.c:2024:4: 12. path: Condition "fwd_rule_add(fwd, &r) < 0", taking false branch. /home/sbrivio/passt/conf.c:2026:3: 13. path: Jumping back to the beginning of the loop. /home/sbrivio/passt/conf.c:2013:3: 14. path: Condition "i < count", taking true branch. /home/sbrivio/passt/conf.c:2014:4: 15. path: Condition "fwd_rule_read(fd, &r)", taking false branch. /home/sbrivio/passt/conf.c:2017:4: 16. path: Condition "r.ifname[15UL /* sizeof (r.ifname) - 1 */]", taking false branch. /home/sbrivio/passt/conf.c:2024:4: 17. tainted_data: Passing tainted expression "fwd->sock_count" to "fwd_rule_add", which uses it as an offset. /home/sbrivio/passt/fwd_rule.c:197:2: Tainted data flows to a taint sink 17.1. path: Condition "new->first > new->last", taking false branch. /home/sbrivio/passt/fwd_rule.c:202:2: 17.2. path: Condition "!new->first", taking false branch. /home/sbrivio/passt/fwd_rule.c:206:2: 17.3. path: Condition "!new->to", taking false branch. /home/sbrivio/passt/fwd_rule.c:206:2: 17.4. path: Condition "(in_port_t)(new->to + new->last - new->first) < new->to", taking false branch. /home/sbrivio/passt/fwd_rule.c:211:2: 17.5. path: Condition "new->flags & -8 /* ~allowed_flags */", taking false branch. /home/sbrivio/passt/fwd_rule.c:216:2: 17.6. path: Condition "new->flags & (1UL /* 1UL << 0 */)", taking true branch. /home/sbrivio/passt/fwd_rule.c:217:3: 17.7. path: Condition "!inany_equals(&new->addr, (union inany_addr const *)&in6addr_any)", taking false branch. /home/sbrivio/passt/fwd_rule.c:224:3: 17.8. path: Condition "!(fwd->caps & (1UL /* 1UL << 0 */))", taking false branch. /home/sbrivio/passt/fwd_rule.c:228:3: 17.9. path: Condition "!(fwd->caps & (2UL /* 1UL << 1 */))", taking false branch. /home/sbrivio/passt/fwd_rule.c:232:2: 17.10. path: Falling through to end of if statement. /home/sbrivio/passt/fwd_rule.c:242:2: 17.11. path: Condition "new->proto == IPPROTO_TCP", taking true branch. /home/sbrivio/passt/fwd_rule.c:243:3: 17.12. path: Condition "!(fwd->caps & (4UL /* 1UL << 2 */))", taking false branch. /home/sbrivio/passt/fwd_rule.c:247:2: 17.13. path: Falling through to end of if statement. /home/sbrivio/passt/fwd_rule.c:258:2: 17.14. path: Condition "i < fwd->count", taking true branch. /home/sbrivio/passt/fwd_rule.c:261:3: 17.15. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. /home/sbrivio/passt/fwd_rule.c:262:4: 17.16. path: Continuing loop. /home/sbrivio/passt/fwd_rule.c:258:2: 17.17. path: Condition "i < fwd->count", taking true branch. /home/sbrivio/passt/fwd_rule.c:261:3: 17.18. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. /home/sbrivio/passt/fwd_rule.c:262:4: 17.19. path: Continuing loop. /home/sbrivio/passt/fwd_rule.c:258:2: 17.20. path: Condition "i < fwd->count", taking true branch. /home/sbrivio/passt/fwd_rule.c:261:3: 17.21. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. /home/sbrivio/passt/fwd_rule.c:262:4: 17.22. path: Continuing loop. /home/sbrivio/passt/fwd_rule.c:258:2: 17.23. path: Condition "i < fwd->count", taking false branch. /home/sbrivio/passt/fwd_rule.c:270:2: 17.24. path: Condition "fwd->count >= 255U /* (int)(sizeof (fwd->rules) / sizeof (fwd->rules[0])) */", taking false branch. /home/sbrivio/passt/fwd_rule.c:274:2: 17.25. path: Condition "fwd->sock_count + num > 327680U /* (int)(sizeof (fwd->socks) / sizeof (fwd->socks[0])) */", taking false branch. /home/sbrivio/passt/fwd_rule.c:280:2: 17.26. data_index: Using tainted expression "fwd->sock_count" as an index to array "fwd->socks". /home/sbrivio/passt/conf.c:2024:4: 18. remediation: Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.

At the same time, Coverity Scan doesn't realise that 'num' is positive,

Uh.. what? num is unsigned, so it must know it's positive.

but we know that, so this will never trigger. As I wrote in the comment, this check is redundant and it won't trigger. It's just good to have to avoid noise from false positives.

Agreed, but sock_count < ARRAY_SIZE() is a fundamental invariant of the data structure, so that makes more sense to document/enforce with a check.

...

...

...

2) (fwd->sock_count + num) overflows

That's a closer-to-real concern. I'm pretty sure we can't hit it for real, because num is necessarily <= 65536, so as long as (1) is true this can't overflow. But that relies on the specific value of ARRAY_SIZE(fwd->socks), so it's kind of fragile.

I think an explicit check for this is a good idea, but it should actually check for this, not just side-effects of it, so: if (fwd->sock_count + num <= fwd->sock_count) { warn("Blah blah overflow"); return -EFAULT; /* or whatever */ }

...

fwd->rulesocks[fwd->count] = &fwd->socks[fwd->sock_count]; + + /* Redundant ('num' checked above), but not for static checkers */ + if (new->last > ARRAY_SIZE(fwd->socks) + new->first) + return -ENOSPC;

This way of organising the check is very confusing to me. I'm not really sure what it's trying to catch.

Same as above.

I'm not sure which "above" you mean.

Same here: the warning is pretty clear:

/home/sbrivio/passt/conf.c:2024:4: Type: Untrusted loop bound (TAINTED_SCALAR)

/home/sbrivio/passt/conf.c:1992:3: Tainted data flows to a taint sink 1. path: Condition "read_u8(fd, &pif)", taking false branch. /home/sbrivio/passt/conf.c:1995:3: 2. path: Condition "pif == 0", taking false branch. /home/sbrivio/passt/conf.c:1998:3: 3. path: Condition "pif >= 4 /* (int)(sizeof (c->fwd_pending) / sizeof (c->fwd_pending[0])) */", taking false branch. /home/sbrivio/passt/conf.c:1998:3: 4. path: Condition "!(fwd = c->fwd_pending[pif])", taking false branch. /home/sbrivio/passt/conf.c:2004:3: 5. path: Condition "read_u32(fd, &count)", taking false branch. /home/sbrivio/passt/conf.c:2007:3: 6. path: Condition "count > 255U /* (1U << 8) - 1 */", taking false branch. /home/sbrivio/passt/conf.c:2013:3: 7. path: Condition "i < count", taking true branch. /home/sbrivio/passt/conf.c:2014:4: 8. path: Condition "fwd_rule_read(fd, &r)", taking false branch. /home/sbrivio/passt/conf.c:2017:4: 9. path: Condition "r.ifname[15UL /* sizeof (r.ifname) - 1 */]", taking false branch. /home/sbrivio/passt/conf.c:2024:4: 10. path: Condition "fwd_rule_add(fwd, &r) < 0", taking false branch. /home/sbrivio/passt/conf.c:2026:3: 11. path: Jumping back to the beginning of the loop. /home/sbrivio/passt/conf.c:2013:3: 12. path: Condition "i < count", taking true branch. /home/sbrivio/passt/conf.c:2014:4: 13. tainted_argument: Calling function "fwd_rule_read" taints argument "r". /home/sbrivio/passt/fwd_rule.c:659:2: Tainted data flows to a taint sink 13.1. tainted_data_argument: Calling function "read_all_buf" taints parameter "*rule". /home/sbrivio/passt/serialise.c:39:2: Tainted data flows to a taint sink 13.1.1. var_assign_parm: Assigning: "p" = "buf". /home/sbrivio/passt/serialise.c:41:2: 13.1.2. path: Condition "left", taking true branch. /home/sbrivio/passt/serialise.c:44:3: 13.1.3. path: Condition "left <= len", taking true branch. /home/sbrivio/passt/serialise.c:47:4: 13.1.4. tainted_data_argument: Calling function "read" taints parameter "*p". [Note: The source code implementation of the function has been overridden by a builtin model.] /home/sbrivio/passt/serialise.c:48:37: 13.1.5. path: Condition "rc < 0", taking false branch. /home/sbrivio/passt/serialise.c:50:3: 13.1.6. path: Condition "rc < 0", taking false branch. /home/sbrivio/passt/serialise.c:53:3: 13.1.7. path: Condition "rc == 0", taking false branch. /home/sbrivio/passt/serialise.c:60:2: 13.1.8. path: Jumping back to the beginning of the loop. /home/sbrivio/passt/serialise.c:41:2: 13.1.9. path: Condition "left", taking true branch. /home/sbrivio/passt/serialise.c:44:3: 13.1.10. path: Condition "left <= len", taking true branch. /home/sbrivio/passt/serialise.c:48:37: 13.1.11. path: Condition "rc < 0", taking false branch. /home/sbrivio/passt/serialise.c:50:3: 13.1.12. path: Condition "rc < 0", taking false branch. /home/sbrivio/passt/serialise.c:53:3: 13.1.13. path: Condition "rc == 0", taking false branch. /home/sbrivio/passt/serialise.c:60:2: 13.1.14. path: Jumping back to the beginning of the loop. /home/sbrivio/passt/serialise.c:41:2: 13.1.15. path: Condition "left", taking false branch. /home/sbrivio/passt/fwd_rule.c:659:2: 13.2. path: Condition "read_all_buf(fd, rule, 40UL /* sizeof (*rule) */)", taking false branch. /home/sbrivio/passt/conf.c:2014:4: 14. path: Condition "fwd_rule_read(fd, &r)", taking false branch. /home/sbrivio/passt/conf.c:2017:4: 15. path: Condition "r.ifname[15UL /* sizeof (r.ifname) - 1 */]", taking false branch. /home/sbrivio/passt/conf.c:2024:4: 16. tainted_data: Passing tainted expression "r.last" to "fwd_rule_add", which uses it as a loop boundary. /home/sbrivio/passt/fwd_rule.c:197:2: Tainted data flows to a taint sink 16.1. lower_bounds: Casting narrower unsigned "new->last" to wider signed type "int" effectively tests its lower bound. /home/sbrivio/passt/fwd_rule.c:197:2: 16.2. path: Condition "new->first > new->last", taking false branch. /home/sbrivio/passt/fwd_rule.c:197:2: 16.3. lower_bounds: Checking lower bounds of unsigned scalar "new->last" by taking the false branch of "new->first > new->last". /home/sbrivio/passt/fwd_rule.c:202:2: 16.4. path: Condition "!new->first", taking false branch. /home/sbrivio/passt/fwd_rule.c:206:2: 16.5. path: Condition "!new->to", taking false branch. /home/sbrivio/passt/fwd_rule.c:206:2: 16.6. lower_bounds: Casting narrower unsigned "new->last" to wider signed type "int" effectively tests its lower bound. /home/sbrivio/passt/fwd_rule.c:206:2: 16.7. path: Condition "(in_port_t)(new->to + new->last - new->first) < new->to", taking false branch. /home/sbrivio/passt/fwd_rule.c:211:2: 16.8. path: Condition "new->flags & -8 /* ~allowed_flags */", taking false branch. /home/sbrivio/passt/fwd_rule.c:216:2: 16.9. path: Condition "new->flags & (1UL /* 1UL << 0 */)", taking true branch. /home/sbrivio/passt/fwd_rule.c:217:3: 16.10. path: Condition "!inany_equals(&new->addr, (union inany_addr const *)&in6addr_any)", taking false branch. /home/sbrivio/passt/fwd_rule.c:224:3: 16.11. path: Condition "!(fwd->caps & (1UL /* 1UL << 0 */))", taking false branch. /home/sbrivio/passt/fwd_rule.c:228:3: 16.12. path: Condition "!(fwd->caps & (2UL /* 1UL << 1 */))", taking false branch. /home/sbrivio/passt/fwd_rule.c:232:2: 16.13. path: Falling through to end of if statement. /home/sbrivio/passt/fwd_rule.c:242:2: 16.14. path: Condition "new->proto == IPPROTO_TCP", taking true branch. /home/sbrivio/passt/fwd_rule.c:243:3: 16.15. path: Condition "!(fwd->caps & (4UL /* 1UL << 2 */))", taking false branch. /home/sbrivio/passt/fwd_rule.c:247:2: 16.16. path: Falling through to end of if statement. /home/sbrivio/passt/fwd_rule.c:258:2: 16.17. path: Condition "i < fwd->count", taking true branch. /home/sbrivio/passt/fwd_rule.c:261:3: 16.18. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. /home/sbrivio/passt/fwd_rule.c:262:4: 16.19. path: Continuing loop. /home/sbrivio/passt/fwd_rule.c:258:2: 16.20. path: Condition "i < fwd->count", taking true branch. /home/sbrivio/passt/fwd_rule.c:261:3: 16.21. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. /home/sbrivio/passt/fwd_rule.c:262:4: 16.22. path: Continuing loop. /home/sbrivio/passt/fwd_rule.c:258:2: 16.23. path: Condition "i < fwd->count", taking false branch. /home/sbrivio/passt/fwd_rule.c:270:2: 16.24. path: Condition "fwd->count >= 255U /* (int)(sizeof (fwd->rules) / sizeof (fwd->rules[0])) */", taking false branch. /home/sbrivio/passt/fwd_rule.c:274:2: 16.25. path: Condition "fwd->sock_count + num > 327680U /* (int)(sizeof (fwd->socks) / sizeof (fwd->socks[0])) */", taking false branch. /home/sbrivio/passt/fwd_rule.c:281:2: 16.26. loop_bound_upper: Using tainted expression "new->last" as a loop boundary. /home/sbrivio/passt/conf.c:2024:4: 17. remediation: Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.

I'll expand the comment and remove the newline between this and the for loop.

...

...

...

We've already checked that last >= first, so using num is safer to deal with at this point than ARRAY_SIZE() + first, which could in principle overflow even if sock_count + num is perfectly ok.

Using 'num' won't work. It shouldn't overflow anyway because the addition happens in 'int'.

It shouldn't overflow, but proving that requires knowing that: a. sock_count is bounded by ARRAY_SIZE(socks) b. first, last and num are bounded by 2^16 c. ARRAY_SIZE(socks) + 2^16 won't overflow (in unsigned int)

I'm not sure which part coverity is missing. (c) at least requires knowledge which is not found in immediately adjacent code.

It's missing the fact that 'num' is already checked above (that is, a.), and due to that we know we're not overflowing fwd->rulesocks[x].

...

Oh... and... I just remembered that ARRAY_SIZE() is int, not unsigned (or size_t). I thought about changing that at some point, but it seemed to cause more trouble than it was worth. Does keep tripping me though, since it seems like it logically ought to be unsigned. Signed overflows are much nastier (UB) that unsigned overflows.

...

I'll try to change the rest if I find some time but it doesn't really look that critical to me.

...

...

for (port = new->first; port <= new->last; port++) fwd->rulesocks[fwd->count][port - new->first] = -1;

-- 2.43.0

-- Stefano

-- Stefano

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