From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202512 header.b=p9XzITc3; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id AB7325A0624 for ; Tue, 13 Jan 2026 06:29:00 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202512; t=1768282136; bh=aYUmuQvAbU25PFA9JSQ3pe2uX5cHVAt0cMOzAW/byCg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=p9XzITc381/ne1spaqkHYWCE1M7DaDDYgJM1FWrAxAhEdnudMyGaB8/cyksiFFBF8 cm6EQxeMf9BSTwpEFXZI+IQ5SB1MOVm1A5A0d5NluKZgUwR6VKgngtp2eO5GtN9nH0 rnFCFgutpR6zYCzD9BrITeUXqrCbQC1UslPy8csN6evScn1duiJSFY4FcOY7eNrKE/ gZ/q9fxIfNHb18nJpyb5P/XmTAHSuO9kheLtn5RKXCXRnNohJWKkwiehX2bKxueUtQ 73Z9KlPU52NHbQyWdJs1tNYnH+5qrzdY/JgR7WFe5TbwyXZH0RqB750MRdqSBI2QE6 z+7aCXufe+oxw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4dqyTh0BTDz4wGs; Tue, 13 Jan 2026 16:28:56 +1100 (AEDT) Date: Tue, 13 Jan 2026 16:12:02 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 02/14] conf, fwd: Keep a table of our port forwarding configuration Message-ID: References: <20260108022948.2657573-1-david@gibson.dropbear.id.au> <20260108022948.2657573-3-david@gibson.dropbear.id.au> <20260113002610.5168fd6a@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="vRqr1WqoGKCreJkv" Content-Disposition: inline In-Reply-To: <20260113002610.5168fd6a@elisabeth> Message-ID-Hash: K7OFAAPT5OW4CESQTMX5V45XTDVQD3HR X-Message-ID-Hash: K7OFAAPT5OW4CESQTMX5V45XTDVQD3HR X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --vRqr1WqoGKCreJkv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: >=20 > > 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 nu= ance > > that the options do. > >=20 > > 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 expli= cit > > forwards, not automatic, and we don't do anything with it other than pr= int > > some additional debug information. We'll do more with it in future > > patches. > >=20 > > Signed-off-by: David Gibson > > --- > > conf.c | 80 ++++++++++++++++++++++++++++++------------------- > > fwd.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > fwd.h | 35 +++++++++++++++++++++- > > 3 files changed, 179 insertions(+), 31 deletions(-) > >=20 > > 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 **e= ndptr, > > * @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 =3D to - first; > > bool bound_one =3D false; > > - unsigned i; > > + unsigned base, i; > > int fd; > > =20 > > if (first =3D=3D 0) { > > @@ -172,37 +173,45 @@ static void conf_ports_range_except(const struct = ctx *c, char optname, > > } > > } > > =20 > > - for (i =3D first; i <=3D last; i++) { > > - if (bitmap_isset(exclude, i)) > > + for (base =3D first; base <=3D last; base++) { > > + if (bitmap_isset(exclude, base)) > > continue; > > =20 > > - if (bitmap_isset(fwd->map, i)) { > > - warn( > > -"Altering mapping of already mapped port number: %s", optarg); > > - } > > + for (i =3D base; i <=3D last; i++) { >=20 > 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; > > =20 > > - bitmap_set(fwd->map, i); > > - fwd->delta[i] =3D to - first; > > + if (bitmap_isset(fwd->map, i)) { > > + warn( > > +"Altering mapping of already mapped port number: %s", optarg); > > + } > > =20 > > - if (optname =3D=3D 't') > > - fd =3D tcp_listen(c, PIF_HOST, addr, ifname, i); > > - else if (optname =3D=3D 'u') > > - fd =3D udp_listen(c, PIF_HOST, addr, ifname, i); > > - else > > - /* No way to check in advance for -T and -U */ > > - fd =3D 0; > > + if (optname =3D=3D 't') > > + fd =3D tcp_listen(c, PIF_HOST, addr, ifname, i); > > + else if (optname =3D=3D 'u') > > + fd =3D udp_listen(c, PIF_HOST, addr, ifname, i); > > + else > > + /* No way to check in advance for -T and -U */ > > + fd =3D 0; > > + > > + if (fd =3D=3D -ENFILE || fd =3D=3D -EMFILE) { > > + die( > > +"Can't open enough sockets for port specifier: %s", > > + optarg); > > + } > > =20 > > - if (fd =3D=3D -ENFILE || fd =3D=3D -EMFILE) { > > - die("Can't open enough sockets for port specifier: %s", > > - optarg); > > + if (fd >=3D 0) { > > + bound_one =3D true; > > + } else if (!(flags & FWD_WEAK)) { > > + die( > > +"Failed to bind port %u (%s) for option '-%c %s'", > > + i, strerror_(-fd), optname, optarg); > > + } > > } > > =20 > > - if (fd >=3D 0) { > > - bound_one =3D 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 =3D i - 1; > > } > > =20 > > if (!bound_one) > > @@ -272,7 +281,7 @@ static void conf_ports(const struct ctx *c, char op= tname, const char *optarg, > > conf_ports_range_except(c, optname, optarg, fwd, > > NULL, NULL, > > 1, NUM_PORTS - 1, exclude, > > - 1, true); > > + 1, FWD_WEAK); > > return; > > } > > =20 > > @@ -357,7 +366,7 @@ static void conf_ports(const struct ctx *c, char op= tname, const char *optarg, > > conf_ports_range_except(c, optname, optarg, fwd, > > addr, ifname, > > 1, NUM_PORTS - 1, exclude, > > - 1, true); > > + 1, FWD_WEAK); > > return; > > } > > =20 > > @@ -390,7 +399,7 @@ static void conf_ports(const struct ctx *c, char op= tname, const char *optarg, > > addr, ifname, > > orig_range.first, orig_range.last, > > exclude, > > - mapped_range.first, false); > > + mapped_range.first, 0); > > } while ((p =3D next_chunk(p, ','))); > > =20 > > 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 =3D=3D MODE_PASTA) { > > + info("Outbound TCP forwarding:"); > > + fwd_rules_print(&c->tcp.fwd_out); > > + info("Outbound UDP forwarding:"); > > + fwd_rules_print(&c->udp.fwd_out); > > + } > > } > > =20 > > /** > > 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 > > */ > > =20 > > +#include > > #include > > #include > > #include > > @@ -22,6 +23,8 @@ > > =20 > > #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); > > } > > =20 > > +/** > > + * fwd_rule_addr() - Return match address for a rule > > + * @rule: Forwarding rule > > + * > > + * Return: Rule's match address, or NULL, if it matches any address >=20 > What about: >=20 > matching address for rule, NULL if it matches all addresses > > + */ > > +static const union inany_addr *fwd_rule_addr(const struct fwd_rule *ru= le) > > +{ > > + if (rule->flags & FWD_DUAL_STACK_ANY) > > + return NULL; >=20 > 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 >=3D fwd_ephemeral_min) && (port <=3D fwd_ephemeral_max); > > } > > =20 > > +/** > > + * 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 =3D FWD_WEAK; > > + struct fwd_rule *new; > > + unsigned port; > > + > > + ASSERT(!(flags & ~allowed_flags)); > > + > > + if (fwd->count >=3D ARRAY_SIZE(fwd->rules)) > > + die("Too many port forwarding ranges"); > > + > > + new =3D &fwd->rules[fwd->count++]; > > + new->flags =3D flags; > > + > > + if (addr) { > > + new->addr =3D *addr; > > + } else { > > + new->addr =3D inany_any6; > > + new->flags |=3D FWD_DUAL_STACK_ANY; > > + } > > + > > + memset(new->ifname, 0, sizeof(new->ifname)); > > + if (ifname) > > + strncpy(new->ifname, ifname, sizeof(new->ifname)); >=20 > Coverity Scan complains because we copy exactly up to > sizeof(new->ifname) bytes, and the terminator might be missing: >=20 > /home/sbrivio/passt/fwd.c:368:3: > Type: Buffer not null terminated (BUFFER_SIZE) >=20 > /home/sbrivio/passt/fwd.c:351:2: > 1. path: Condition "!(flags & -3 /* ~allowed_flags */)", taking true br= anch. > /home/sbrivio/passt/fwd.c:353:2: > 2. path: Condition "fwd->count >=3D 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 <=3D last); > > + new->first =3D first; > > + new->last =3D last; > > + > > + new->to =3D to; > > + > > + for (port =3D new->first; port <=3D new->last; port++) { > > + /* Fill in the legacy data structures to match the table */ > > + bitmap_set(fwd->map, port); > > + fwd->delta[port] =3D 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 =3D 0; i < fwd->count; i++) { > > + const struct fwd_rule *rule =3D &fwd->rules[i]; > > + const char *weak =3D rule->flags & FWD_WEAK ? " WEAK" : ""; >=20 > 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. >=20 > > + const char *percent =3D *rule->ifname ? "%" : ""; > > + char addr[INANY_ADDRSTRLEN]; > > + > > + inany_ntop(fwd_rule_addr(rule), addr, sizeof(addr)); > > + > > + if (rule->first =3D=3D rule->last) { > > + info(" [%s]%s%s:%hu =3D> %hu %s", > > + addr, percent, rule->ifname, > > + rule->first, rule->to, weak); > > + } else { > > + info(" [%s]%s%s:%hu-%hu =3D> %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); > > =20 > > +/** > > + * 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 >=20 > 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). >=20 > 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. >=20 > > + * > > + * 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; >=20 > 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 num= ber > > */ > > @@ -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]; > > }; > > =20 > > #define FWD_PORT_SCAN_INTERVAL 1000 /* ms */ > > =20 > > +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); > > =20 > > bool nat_inbound(const struct ctx *c, const union inany_addr *addr, > > union inany_addr *translated); >=20 > --=20 > Stefano >=20 --=20 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 --vRqr1WqoGKCreJkv Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmll1CEACgkQzQJF27ox 2Gf0SA/+NHJlXXFRxexeFMk2GsGGCdldvTNmXl9DERXMj3Uq2UpntJO4izRU2Lz0 riDumipFGQG/OqAmTz7DcWHE24HMK68r6jJZUm+EuVksqzatDnUmAoOYYEkj3UPM bKcgWAiT9SXovuigdrJ/m4EBgPW5Din4PkmXwaCLmAxmMpH81bM4KYXwLU36skCY 6NjWvMnjrf5+CcMc3ocnFzDjas91wGlx6zmRbJfgMqkLz4wrvF8ZzkH+GPU38Q46 zAd0t1d1klAV9GQCrfGI25QRVu0hS0RnaM2RDWethJQTC4ZGf8+8G/4Aolh7ygwN NE5Wf3Y3VtfzpDiQYeE/pyhTcgfJ3/NjwxXO7EU1eBd3O8o17Jt8GvpgyXH2ypbu +i8t36tDgR715EbiPn8AdUnhRRASafuG5ZN+khEILkWNSTyqF0t4ycuATxkK+7mv KkRrS3FjyLulX9n8y0uiqL20mA3nsShrkK3c/piF8ZlqydhxWvuPF9OUZ6PEKsxG DMyjmY1HlGDjRd1X5jjHjbN4L0qHYpmSJmufiZwYu6sjzmLMIgwXEoEL8XnfcvRa rLdcO7ZWjZY58pI9lUUQYdgDD+/BJegqb2zIPJ4gROPN22hYzBBFUzzo160ne1/T pk0fKDlY8ABfTgVsJmjWkGaxMoU0NmaLrpd4eSj5d3x28jsCro0= =01yL -----END PGP SIGNATURE----- --vRqr1WqoGKCreJkv--