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=eF8q/84X; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 6BEB55A0623 for ; Wed, 14 Jan 2026 01:42:03 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202512; t=1768351319; bh=6ptmBA82G5aArlWrJYZb/t+756O+XfjB+qagJOigfZA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=eF8q/84XPYw3FaFUJfUytcw+osYya/CGvL7j3OoDYN3cF6e/8wOpruzt7+FlPvQDs Efg/xFS45xkOZN7XO9bUeCAHKHTLlfzM3ojRi3IxieyaKRpY4kIOcibzGXtsvlU6iR tllkSVD3a5Nu6xtXMs7ED0ksfNvveSwHPJnB3M8NeYuPCeRRcPYKtTBVQhaFNNP5xZ 7BHJIO8ieW41saY1nFMHac06XGur4VdlNR/tHUkPJ/H4rH675pwDhdjJNmAgZD2fL6 YJ7PNV6lqsAE/Jw/kA21n6L8gsNIvWw3fYLHhL0iN75BxBmUrUC9WKIQOJ2QoBpOs4 YzQd7IRq2+JTA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4drS473HzMz4wRN; Wed, 14 Jan 2026 11:41:59 +1100 (AEDT) Date: Wed, 14 Jan 2026 11:24:21 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 12/14] fwd: Remap ports based directly on forwarding rule Message-ID: References: <20260108022948.2657573-1-david@gibson.dropbear.id.au> <20260108022948.2657573-13-david@gibson.dropbear.id.au> <20260113231226.3b7fba24@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="pI/76tR6sWXQa9ES" Content-Disposition: inline In-Reply-To: <20260113231226.3b7fba24@elisabeth> Message-ID-Hash: 6LTF33XQIINVKUGE3KZWIPJ2WX7EDQ74 X-Message-ID-Hash: 6LTF33XQIINVKUGE3KZWIPJ2WX7EDQ74 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: --pI/76tR6sWXQa9ES Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jan 13, 2026 at 11:12:26PM +0100, Stefano Brivio wrote: > On Thu, 8 Jan 2026 13:29:46 +1100 > David Gibson wrote: >=20 > > Currently we remap port numbers based on the legacy delta[] array, which > > is indexed only by original port number, not the listening address. Now > > that we look up a forwarding rule entry in flow_target(), we can use th= is > > entry to directly determine the correct remapped port. Implement this, > > and remove the old delta[] array. > >=20 > > Link: https://bugs.passt.top/show_bug.cgi?id=3D187 > >=20 > > Signed-off-by: David Gibson > > --- > > flow.c | 7 ++++--- > > fwd.c | 21 +++++++-------------- > > fwd.h | 7 +++---- > > 3 files changed, 14 insertions(+), 21 deletions(-) > >=20 > > diff --git a/flow.c b/flow.c > > index 045e9712..38f88732 100644 > > --- a/flow.c > > +++ b/flow.c > > @@ -493,6 +493,7 @@ struct flowside *flow_target(const struct ctx *c, u= nion flow *flow, > > struct flow_common *f =3D &flow->f; > > const struct flowside *ini =3D &f->side[INISIDE]; > > struct flowside *tgt =3D &f->side[TGTSIDE]; > > + const struct fwd_rule *rule =3D NULL; >=20 > Coverity Scan complains about two cases where this NULL rule would be > passed to NAT functions dereferencing it. They should be both false > positives because the rule is always set on pif_is_socket(...), but I > wonder how fragile it is. Ah, yeah. I'm not terribly surprised. I'll see what I can do. > Regardless, it would be nice to avoid adding further warnings, if it's > cheap (ASSERT() or check on rule in fwd_nat_from*() functions?). Anyway, > here you go: >=20 > /home/sbrivio/passt/flow.c:535:3: > Type: Explicit null dereferenced (FORWARD_NULL) >=20 > /home/sbrivio/passt/flow.c:496:2: > 1. assign_zero: Assigning: "rule" =3D "NULL". > /home/sbrivio/passt/flow.c:499:2: > 2. path: Condition "flow_new_entry =3D=3D flow", taking true branch. > /home/sbrivio/passt/flow.c:499:2: > 3. path: Condition "f->state =3D=3D FLOW_STATE_INI", taking true branch. > /home/sbrivio/passt/flow.c:500:2: > 4. path: Condition "f->type =3D=3D FLOW_TYPE_NONE", taking true branch. > /home/sbrivio/passt/flow.c:501:2: > 5. path: Condition "f->pif[0] !=3D PIF_NONE", taking true branch. > /home/sbrivio/passt/flow.c:501:2: > 6. path: Condition "f->pif[1] =3D=3D PIF_NONE", taking true branch. > /home/sbrivio/passt/flow.c:502:2: > 7. path: Condition "flow->f.state =3D=3D FLOW_STATE_INI", taking true b= ranch. > /home/sbrivio/passt/flow.c:504:2: > 8. path: Condition "pif_is_socket(f->pif[0])", taking false branch. > /home/sbrivio/passt/flow.c:528:2: > 9. path: Switch case value "PIF_SPLICE". > /home/sbrivio/passt/flow.c:535:3: > 10. var_deref_model: Passing null pointer "rule" to "fwd_nat_from_splic= e", which dereferences it. > /home/sbrivio/passt/fwd.c:991:2: > 10.1. path: Condition "!inany_is_loopback(&ini->eaddr)", taking false b= ranch. > /home/sbrivio/passt/fwd.c:991:2: > 10.2. path: Condition "!inany_is_loopback(&ini->oaddr)", taking false b= ranch. > /home/sbrivio/passt/fwd.c:1008:2: > 10.3. path: Condition "proto =3D=3D IPPROTO_UDP", taking true branch. > /home/sbrivio/passt/fwd.c:1012:2: > 10.4. dereference: Dereferencing pointer "rule". >=20 > /home/sbrivio/passt/flow.c:539:3: > Type: Explicit null dereferenced (FORWARD_NULL) >=20 > /home/sbrivio/passt/flow.c:496:2: > 1. assign_zero: Assigning: "rule" =3D "NULL". > /home/sbrivio/passt/flow.c:499:2: > 2. path: Condition "flow_new_entry =3D=3D flow", taking true branch. > /home/sbrivio/passt/flow.c:499:2: > 3. path: Condition "f->state =3D=3D FLOW_STATE_INI", taking true branch. > /home/sbrivio/passt/flow.c:500:2: > 4. path: Condition "f->type =3D=3D FLOW_TYPE_NONE", taking true branch. > /home/sbrivio/passt/flow.c:501:2: > 5. path: Condition "f->pif[0] !=3D PIF_NONE", taking true branch. > /home/sbrivio/passt/flow.c:501:2: > 6. path: Condition "f->pif[1] =3D=3D PIF_NONE", taking true branch. > /home/sbrivio/passt/flow.c:502:2: > 7. path: Condition "flow->f.state =3D=3D FLOW_STATE_INI", taking true b= ranch. > /home/sbrivio/passt/flow.c:504:2: > 8. path: Condition "pif_is_socket(f->pif[0])", taking false branch. > /home/sbrivio/passt/flow.c:528:2: > 9. path: Switch case value "PIF_HOST". > /home/sbrivio/passt/flow.c:539:3: > 10. var_deref_model: Passing null pointer "rule" to "fwd_nat_from_host"= , which dereferences it. > /home/sbrivio/passt/fwd.c:1069:2: > 10.1. dereference: Dereferencing pointer "rule". >=20 > > uint8_t tgtpif =3D PIF_NONE; > > =20 > > ASSERT(flow_new_entry =3D=3D flow && f->state =3D=3D FLOW_STATE_INI); > > @@ -514,7 +515,7 @@ struct flowside *flow_target(const struct ctx *c, u= nion flow *flow, > > else > > goto nofwd; > > =20 > > - if (!fwd_rule_search(fwd, ini)) { > > + if (!(rule =3D fwd_rule_search(fwd, ini))) { > > /* This shouldn't happen, because if there's no rule for > > * it we should have no listening socket that would let > > * us get here > > @@ -531,11 +532,11 @@ struct flowside *flow_target(const struct ctx *c,= union flow *flow, > > break; > > =20 > > case PIF_SPLICE: > > - tgtpif =3D fwd_nat_from_splice(c, proto, ini, tgt); > > + tgtpif =3D fwd_nat_from_splice(rule, proto, ini, tgt); > > break; > > =20 > > case PIF_HOST: > > - tgtpif =3D fwd_nat_from_host(c, proto, ini, tgt); > > + tgtpif =3D fwd_nat_from_host(c, rule, proto, ini, tgt); > > fwd_neigh_mac_get(c, &tgt->oaddr, f->tap_omac); > > break; > > default: > > diff --git a/fwd.c b/fwd.c > > index 633ba5db..7c4575ff 100644 > > --- a/fwd.c > > +++ b/fwd.c > > @@ -405,7 +405,6 @@ void fwd_rule_add(struct fwd_ports *fwd, uint8_t fl= ags, > > /* Fill in the legacy forwarding data structures to match the table = */ > > if (!(new->flags & FWD_SCAN)) > > bitmap_set(fwd->map, port); > > - fwd->delta[port] =3D new->to - new->first; > > } > > } > > =20 > > @@ -978,7 +977,7 @@ uint8_t fwd_nat_from_tap(const struct ctx *c, uint8= _t proto, > > =20 > > /** > > * fwd_nat_from_splice() - Determine to forward a flow from the splice= interface > > - * @c: Execution context > > + * @rule: Forwarding rule to apply > > * @proto: Protocol (IP L4 protocol number) > > * @ini: Flow address information of the initiating side > > * @tgt: Flow address information on the target side (updated) > > @@ -986,7 +985,7 @@ uint8_t fwd_nat_from_tap(const struct ctx *c, uint8= _t proto, > > * Return: pif of the target interface to forward the flow to, PIF_NON= E if the > > * flow cannot or should not be forwarded at all. > > */ > > -uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto, > > +uint8_t fwd_nat_from_splice(const struct fwd_rule *rule, uint8_t proto, > > const struct flowside *ini, struct flowside *tgt) > > { > > if (!inany_is_loopback(&ini->eaddr) || > > @@ -1010,11 +1009,7 @@ uint8_t fwd_nat_from_splice(const struct ctx *c,= uint8_t proto, > > /* But for UDP preserve the source port */ > > tgt->oport =3D ini->eport; > > =20 > > - tgt->eport =3D ini->oport; > > - if (proto =3D=3D IPPROTO_TCP) > > - tgt->eport +=3D c->tcp.fwd_out.delta[tgt->eport]; > > - else if (proto =3D=3D IPPROTO_UDP) > > - tgt->eport +=3D c->udp.fwd_out.delta[tgt->eport]; > > + tgt->eport =3D ini->oport - rule->first + rule->to; >=20 > This made me notice that the current documentation to struct fwd_rule > doesn't really make it clear that n : 1 port mapping rules are not a > thing, Not yet, but that's a pretty obvious extension for later. > so, maybe, back to 2/14, instead of: >=20 > * @to: Port number to forward port @first to. >=20 > we could have perhaps: >=20 > * @to: Target port for @first, port n maps to @to + (n - @first) >=20 > ? Seems reasonable, done. > By the way, I find this notation a bit complicated to figure out, I > think that: >=20 > rule->to + (ini->oport - rule->first) Good idea, done. > (redundant parentheses included) is actually clearer. Or maybe even > with a temporary 'delta' variable. >=20 > In both cases: the subtraction now happens in in_port_t, so I guess we > should explicitly cast rule->first to int to be strictly correct. No, it should be correct to do it within a u16. At this point we know that @first <=3D port <=3D @last, so the subtraction can't over or underflow. I guess the addition could, strictly speaking. But 16-bit unsigned overflow has the correct semantics for this. I guess that does allow remapping to port 0, which is problematic so maybe we should enforce that (@to + @last - @first) < NUM_PORTS when we add rules. > > return PIF_HOST; > > } > > @@ -1058,6 +1053,7 @@ bool nat_inbound(const struct ctx *c, const union= inany_addr *addr, > > /** > > * fwd_nat_from_host() - Determine to forward a flow from the host int= erface > > * @c: Execution context > > + * @rule: Forwarding rule to apply > > * @proto: Protocol (IP L4 protocol number) > > * @ini: Flow address information of the initiating side > > * @tgt: Flow address information on the target side (updated) > > @@ -1065,15 +1061,12 @@ bool nat_inbound(const struct ctx *c, const uni= on inany_addr *addr, > > * Return: pif of the target interface to forward the flow to, PIF_NON= E if the > > * flow cannot or should not be forwarded at all. > > */ > > -uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto, > > +uint8_t fwd_nat_from_host(const struct ctx *c, > > + const struct fwd_rule *rule, uint8_t proto, > > const struct flowside *ini, struct flowside *tgt) > > { > > /* Common for spliced and non-spliced cases */ > > - tgt->eport =3D ini->oport; > > - if (proto =3D=3D IPPROTO_TCP) > > - tgt->eport +=3D c->tcp.fwd_in.delta[tgt->eport]; > > - else if (proto =3D=3D IPPROTO_UDP) > > - tgt->eport +=3D c->udp.fwd_in.delta[tgt->eport]; > > + tgt->eport =3D ini->oport - rule->first + rule->to; >=20 > Same as above. Done. > > if (!c->no_splice && inany_is_loopback(&ini->eaddr) && > > (proto =3D=3D IPPROTO_TCP || proto =3D=3D IPPROTO_UDP)) { > > diff --git a/fwd.h b/fwd.h > > index a10bdbb4..cfe9ed46 100644 > > --- a/fwd.h > > +++ b/fwd.h > > @@ -82,7 +82,6 @@ enum fwd_ports_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 > > * @listen_sock_count: Number of entries used in @listen_socks > > * @listen_socks: Listening sockets for forwarding > > */ > > @@ -93,7 +92,6 @@ struct fwd_ports { > > unsigned count; > > struct fwd_rule rules[MAX_FWD_RULES]; > > uint8_t map[PORT_BITMAP_SIZE]; > > - in_port_t delta[NUM_PORTS]; > > unsigned listen_sock_count; > > int listen_socks[MAX_LISTEN_SOCKS]; > > }; > > @@ -117,9 +115,10 @@ bool nat_inbound(const struct ctx *c, const union = inany_addr *addr, > > union inany_addr *translated); > > uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto, > > const struct flowside *ini, struct flowside *tgt); > > -uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto, > > +uint8_t fwd_nat_from_splice(const struct fwd_rule *rule, uint8_t proto, > > const struct flowside *ini, struct flowside *tgt); > > -uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto, > > +uint8_t fwd_nat_from_host(const struct ctx *c, > > + const struct fwd_rule *rule, uint8_t proto, > > const struct flowside *ini, struct flowside *tgt); > > void fwd_neigh_table_update(const struct ctx *c, const union inany_add= r *addr, > > const uint8_t *mac, bool permanent); >=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 --pI/76tR6sWXQa9ES Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmlm4jQACgkQzQJF27ox 2GeKFw//Z77xQr7WzpDzi+y2rTWVP7u74i2JfWrkcI/kHxdmNvZBNdViXijSBsbZ OGBhDA3ge6ht/ED1EifZtr4qGAHY6jCdMP7s4R4BQ1hON751vyhASoyVm13xaX/U sXym3NS7z7Wc1mENYuxUzdelq7xOiFPIaJDPw7SwE5zVrR3yRBvOjhUp4yIxexHE sANNAlUn0r3y48h81Yu+ekUniSuPqLT3AnAV8lcVVeJjRXqiex3FWLYQ+7Rc79ti LrFsBYn5t29y3Dqle0V87+dIShBWInaaYcoySBG3vyBM6VzgtmM0I387n0RRAp+X meZkRAVFYXilwB5LVq+hyBTD9BsSYR/1UIXSFfkuog882DwBMlvfkU9uW06HAEgZ GOgyalkDglEAnRFOGAmZuedZ+YcXSu7UPaGmBXyK62FW1GZ/+T/BOAcO5GLSnyQ4 wa3r35MzKNkJTv0r9CjiL7Kd0uhEShFMQ7OXiLn43iM1wcfG5GdoyuBjChBUJSGs gN/QZTQCHxFWfDarhVoV0JavkDqoSETPoudKRHt7/XZs7y0ZchGmLIwI5m4EHrXp 1eSJcIn+0Zd+xhLF7wAuNbjJxCNL6NnqdsihTwLf+Pb2aOW/WUYk2pQrmY3Nqkxk OJR2Tjq2MM6cOWvMgBnprn0EhITKFRXIST/wWJ3JwcxmTRUPvA8= =n0CV -----END PGP SIGNATURE----- --pI/76tR6sWXQa9ES--