On Tue, Jan 13, 2026 at 12:26:10AM +0100, Stefano Brivio wrote: > On Thu, 8 Jan 2026 13:29:36 +1100 > David Gibson wrote: > > > At present, we set up forwarding as we parse the -t, and -u options, not > > keeping a persistent data structure with all the details. We do have > > some information in struct fwd_ports, but it doesn't capture all the nuance > > that the options do. > > > > As a first step to generalising our forwarding model, add a table of all > > the forwarding rules to struct fwd_ports. For now it covers only explicit > > forwards, not automatic, and we don't do anything with it other than print > > some additional debug information. We'll do more with it in future > > patches. > > > > Signed-off-by: David Gibson > > --- > > conf.c | 80 ++++++++++++++++++++++++++++++------------------- > > fwd.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > fwd.h | 35 +++++++++++++++++++++- > > 3 files changed, 179 insertions(+), 31 deletions(-) > > > > diff --git a/conf.c b/conf.c > > index c936664b..127e69f5 100644 > > --- a/conf.c > > +++ b/conf.c > > @@ -137,7 +137,7 @@ static int parse_port_range(const char *s, char **endptr, > > * @last: Last port to forward > > * @exclude: Bitmap of ports to exclude > > * @to: Port to translate @first to when forwarding > > - * @weak: Ignore errors, as long as at least one port is mapped > > + * @flags: Flags for forwarding entries > > */ > > static void conf_ports_range_except(const struct ctx *c, char optname, > > const char *optarg, struct fwd_ports *fwd, > > @@ -145,10 +145,11 @@ static void conf_ports_range_except(const struct ctx *c, char optname, > > const char *ifname, > > uint16_t first, uint16_t last, > > const uint8_t *exclude, uint16_t to, > > - bool weak) > > + uint8_t flags) > > { > > + unsigned delta = to - first; > > bool bound_one = false; > > - unsigned i; > > + unsigned base, i; > > int fd; > > > > if (first == 0) { > > @@ -172,37 +173,45 @@ static void conf_ports_range_except(const struct ctx *c, char optname, > > } > > } > > > > - for (i = first; i <= last; i++) { > > - if (bitmap_isset(exclude, i)) > > + for (base = first; base <= last; base++) { > > + if (bitmap_isset(exclude, base)) > > continue; > > > > - if (bitmap_isset(fwd->map, i)) { > > - warn( > > -"Altering mapping of already mapped port number: %s", optarg); > > - } > > + for (i = base; i <= last; i++) { > > This nested loop is a bit convoluted, but I don't have better ideas and > I suppose it goes away later (I just reached 10/14 so far). The nested loop remains, so far. A lot of the rest of the loop body does go away, so the overall complexity does reduce. > > + if (bitmap_isset(exclude, i)) > > + break; > > > > - bitmap_set(fwd->map, i); > > - fwd->delta[i] = to - first; > > + if (bitmap_isset(fwd->map, i)) { > > + warn( > > +"Altering mapping of already mapped port number: %s", optarg); > > + } > > > > - if (optname == 't') > > - fd = tcp_listen(c, PIF_HOST, addr, ifname, i); > > - else if (optname == 'u') > > - fd = udp_listen(c, PIF_HOST, addr, ifname, i); > > - else > > - /* No way to check in advance for -T and -U */ > > - fd = 0; > > + if (optname == 't') > > + fd = tcp_listen(c, PIF_HOST, addr, ifname, i); > > + else if (optname == 'u') > > + fd = udp_listen(c, PIF_HOST, addr, ifname, i); > > + else > > + /* No way to check in advance for -T and -U */ > > + fd = 0; > > + > > + if (fd == -ENFILE || fd == -EMFILE) { > > + die( > > +"Can't open enough sockets for port specifier: %s", > > + optarg); > > + } > > > > - if (fd == -ENFILE || fd == -EMFILE) { > > - die("Can't open enough sockets for port specifier: %s", > > - optarg); > > + if (fd >= 0) { > > + bound_one = true; > > + } else if (!(flags & FWD_WEAK)) { > > + die( > > +"Failed to bind port %u (%s) for option '-%c %s'", > > + i, strerror_(-fd), optname, optarg); > > + } > > } > > > > - if (fd >= 0) { > > - bound_one = true; > > - } else if (!weak) { > > - die("Failed to bind port %u (%s) for option '-%c %s'", > > - i, strerror_(-fd), optname, optarg); > > - } > > + fwd_rule_add(fwd, flags, addr, ifname, base, i - 1, > > + base + delta); > > + base = i - 1; > > } > > > > if (!bound_one) > > @@ -272,7 +281,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, > > conf_ports_range_except(c, optname, optarg, fwd, > > NULL, NULL, > > 1, NUM_PORTS - 1, exclude, > > - 1, true); > > + 1, FWD_WEAK); > > return; > > } > > > > @@ -357,7 +366,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, > > conf_ports_range_except(c, optname, optarg, fwd, > > addr, ifname, > > 1, NUM_PORTS - 1, exclude, > > - 1, true); > > + 1, FWD_WEAK); > > return; > > } > > > > @@ -390,7 +399,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, > > addr, ifname, > > orig_range.first, orig_range.last, > > exclude, > > - mapped_range.first, false); > > + mapped_range.first, 0); > > } while ((p = next_chunk(p, ','))); > > > > return; > > @@ -1225,6 +1234,17 @@ dns6: > > info(" %s", c->dns_search[i].n); > > } > > } > > + > > + info("Inbound TCP forwarding:"); > > + fwd_rules_print(&c->tcp.fwd_in); > > + info("Inbound UDP forwarding:"); > > + fwd_rules_print(&c->udp.fwd_in); > > + if (c->mode == MODE_PASTA) { > > + info("Outbound TCP forwarding:"); > > + fwd_rules_print(&c->tcp.fwd_out); > > + info("Outbound UDP forwarding:"); > > + fwd_rules_print(&c->udp.fwd_out); > > + } > > } > > > > /** > > diff --git a/fwd.c b/fwd.c > > index 961c533f..ad398e54 100644 > > --- a/fwd.c > > +++ b/fwd.c > > @@ -13,6 +13,7 @@ > > * Author: David Gibson > > */ > > > > +#include > > #include > > #include > > #include > > @@ -22,6 +23,8 @@ > > > > #include "util.h" > > #include "ip.h" > > +#include "siphash.h" > > +#include "inany.h" > > #include "fwd.h" > > #include "passt.h" > > #include "lineread.h" > > @@ -300,6 +303,19 @@ parse_err: > > warn("Unable to parse %s", PORT_RANGE_SYSCTL); > > } > > > > +/** > > + * fwd_rule_addr() - Return match address for a rule > > + * @rule: Forwarding rule > > + * > > + * Return: Rule's match address, or NULL, if it matches any address > > What about: > > matching address for rule, NULL if it matches all addresses > > + */ > > +static const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule) > > +{ > > + if (rule->flags & FWD_DUAL_STACK_ANY) > > + return NULL; > > Nit: usual newline here. Fixed. > > + return &rule->addr; > > +} > > + > > /** > > * fwd_port_is_ephemeral() - Is port number ephemeral? > > * @port: Port number > > @@ -313,6 +329,85 @@ bool fwd_port_is_ephemeral(in_port_t port) > > return (port >= fwd_ephemeral_min) && (port <= fwd_ephemeral_max); > > } > > > > +/** > > + * fwd_rule_add() - Add a rule to a forwarding table > > + * @fwd: Table to add to > > + * @flags: Flags for this entry > > + * @addr: Our address to forward (NULL for both 0.0.0.0 and ::) > > + * @ifname: Only forward from this interface name, if non-empty > > + * @first: First port number to forward > > + * @last: Last port number to forward > > + * @to: First port of target port range to map to > > + */ > > +void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags, > > + const union inany_addr *addr, const char *ifname, > > + in_port_t first, in_port_t last, in_port_t to) > > +{ > > + /* Flags which can be set from the caller */ > > + const uint8_t allowed_flags = FWD_WEAK; > > + struct fwd_rule *new; > > + unsigned port; > > + > > + ASSERT(!(flags & ~allowed_flags)); > > + > > + if (fwd->count >= ARRAY_SIZE(fwd->rules)) > > + die("Too many port forwarding ranges"); > > + > > + new = &fwd->rules[fwd->count++]; > > + new->flags = flags; > > + > > + if (addr) { > > + new->addr = *addr; > > + } else { > > + new->addr = inany_any6; > > + new->flags |= FWD_DUAL_STACK_ANY; > > + } > > + > > + memset(new->ifname, 0, sizeof(new->ifname)); > > + if (ifname) > > + strncpy(new->ifname, ifname, sizeof(new->ifname)); > > Coverity Scan complains because we copy exactly up to > sizeof(new->ifname) bytes, and the terminator might be missing: > > /home/sbrivio/passt/fwd.c:368:3: > Type: Buffer not null terminated (BUFFER_SIZE) > > /home/sbrivio/passt/fwd.c:351:2: > 1. path: Condition "!(flags & -3 /* ~allowed_flags */)", taking true branch. > /home/sbrivio/passt/fwd.c:353:2: > 2. path: Condition "fwd->count >= 128U /* (int)(sizeof (fwd->rules) / sizeof (fwd->rules[0])) */", taking false branch. > /home/sbrivio/passt/fwd.c:359:2: > 3. path: Condition "addr", taking true branch. > /home/sbrivio/passt/fwd.c:361:2: > 4. path: Falling through to end of if statement. > /home/sbrivio/passt/fwd.c:367:2: > 5. path: Condition "ifname", taking true branch. > /home/sbrivio/passt/fwd.c:368:3: > 6. buffer_size_warning: Calling "strncpy" with a maximum size argument of 16 bytes on destination array "new->ifname" of size 16 bytes might leave the destination string unterminated. > /home/sbrivio/passt/fwd.c:370:2: Ah, good point. I added a check. > > + > > + ASSERT(first <= last); > > + new->first = first; > > + new->last = last; > > + > > + new->to = to; > > + > > + for (port = new->first; port <= new->last; port++) { > > + /* Fill in the legacy data structures to match the table */ > > + bitmap_set(fwd->map, port); > > + fwd->delta[port] = new->to - new->first; > > + } > > +} > > + > > +/** > > + * fwd_rules_print() - Print forwarding rules for debugging > > + * @fwd: Table to print > > + */ > > +void fwd_rules_print(const struct fwd_ports *fwd) > > +{ > > + unsigned i; > > + > > + for (i = 0; i < fwd->count; i++) { > > + const struct fwd_rule *rule = &fwd->rules[i]; > > + const char *weak = rule->flags & FWD_WEAK ? " WEAK" : ""; > > Should we print " might fail" or " can fail" instead of " WEAK"? This > is for users. Good point. THough I'm not sure "might fail" or "can fail" is terribly clear in context either. I've gone with " (best effort)" for now. > > > + const char *percent = *rule->ifname ? "%" : ""; > > + char addr[INANY_ADDRSTRLEN]; > > + > > + inany_ntop(fwd_rule_addr(rule), addr, sizeof(addr)); > > + > > + if (rule->first == rule->last) { > > + info(" [%s]%s%s:%hu => %hu %s", > > + addr, percent, rule->ifname, > > + rule->first, rule->to, weak); > > + } else { > > + info(" [%s]%s%s:%hu-%hu => %hu-%hu %s", > > + addr, percent, rule->ifname, rule->first, rule->last, > > + rule->to, rule->last - rule->first + rule->to, weak); > > + } > > + } > > +} > > + > > /* 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 934bab33..3dfc7959 100644 > > --- a/fwd.h > > +++ b/fwd.h > > @@ -16,6 +16,30 @@ struct flowside; > > void fwd_probe_ephemeral(void); > > bool fwd_port_is_ephemeral(in_port_t port); > > > > +/** > > + * struct fwd_rule - Forwarding rule governing a range of ports > > + * @addr: Address to forward from > > + * @ifname: Interface to forward from > > + * @first: First port number to forward > > + * @last: Last port number to forward > > + * @to: Port number to forward port @first to. > > + * @flags: Flag mask > > + * FWD_DUAL_STACK_ANY - match any IPv4 or IPv6 address (@addr should be ::) > > + * FWD_WEAK - Don't give an error if binds fail for some forwards > > Usually we document bit flags inline as we define them (see struct > tcp_tap_conn), I guess it's a bit easier to follow, but this has the > advantage of looking less cramped (unless we add comment lines before > the #defines, which would also be reasonable I think). > > Not a strong preference either way. Keeping it as-is, for now, since some of the descriptions are quite long so they'd wrap if put on the same line as the #define. > > > + * > > + * FIXME: @addr and @ifname currently ignored for outbound tables > > + */ > > +struct fwd_rule { > > + union inany_addr addr; > > + char ifname[IFNAMSIZ]; > > + in_port_t first, last, to; > > Having one line for each helps (me at least) visualising the size of the > struct. Not a strong preference. Fair point, altered. > > +#define FWD_DUAL_STACK_ANY BIT(0) > > +#define FWD_WEAK BIT(1) > > + uint8_t flags; > > +}; > > + > > +#define MAX_FWD_RULES 128 > > + > > /** > > * union fwd_listen_ref - information about a single listening socket > > * @port: Bound port number of the socket > > @@ -44,6 +68,8 @@ enum fwd_ports_mode { > > * @mode: Overall forwarding mode (all, none, auto, specific ports) > > * @scan4: /proc/net fd to scan for IPv4 ports when in AUTO mode > > * @scan6: /proc/net fd to scan for IPv6 ports when in AUTO mode > > + * @count: Number of forwarding rules > > + * @rules: Array of forwarding rules > > * @map: Bitmap describing which ports are forwarded > > * @delta: Offset between the original destination and mapped port number > > */ > > @@ -51,14 +77,21 @@ struct fwd_ports { > > enum fwd_ports_mode mode; > > int scan4; > > int scan6; > > + unsigned count; > > + struct fwd_rule rules[MAX_FWD_RULES]; > > uint8_t map[PORT_BITMAP_SIZE]; > > in_port_t delta[NUM_PORTS]; > > }; > > > > #define FWD_PORT_SCAN_INTERVAL 1000 /* ms */ > > > > +void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags, > > + const union inany_addr *addr, const char *ifname, > > + in_port_t first, in_port_t last, in_port_t to); > > +void fwd_rules_print(const struct fwd_ports *fwd); > > + > > void fwd_scan_ports_init(struct ctx *c); > > -void fwd_scan_ports_timer(struct ctx *c, const struct timespec *now); > > +void fwd_scan_ports_timer(struct ctx * c, const struct timespec *now); > > > > bool nat_inbound(const struct ctx *c, const union inany_addr *addr, > > union inany_addr *translated); > > -- > 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