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=Fa8hlcnr; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 7BA7B5A0626 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=2cyTPVAeUijkKDKUszPBWvlf9fh9SCQsaX5yLvbOKgc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Fa8hlcnrA0+lv8maX9o0NaguQgmg7oZD4CvTgg+O/kIMRTyT41ODM8RiFpf5uyvv/ CgifOSWD2i4uNcnt4Qdet4gsyfZiU94sewT2kFuwV/j4vb0e/2PN3M2Txc7FBu/Fb6 SzsdW70XLg/1BBByx6aXzoC9dAJzwJYxGlLaVHdV0orQKstBDDKIiV1AeS4UTgzMAF g0VsM/eYuNm1JpoCNZOwA3LRXsAlKcftXv3Eq5zIBym2BQuIUFmH/9yRGFw0N7l8uU Nh7S0w2DrvZuyqypLhZ4ol4jBxewh/2ybtBYvI8wmE9EdzX1xaYmvOlR0iDIE/mAwP LRyTnkbJC5FJw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4drS4737dNz4wRH; Wed, 14 Jan 2026 11:41:59 +1100 (AEDT) Date: Wed, 14 Jan 2026 11:09:06 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 11/14] flow, fwd: Consult rules table when forwarding a new flow from socket Message-ID: References: <20260108022948.2657573-1-david@gibson.dropbear.id.au> <20260108022948.2657573-12-david@gibson.dropbear.id.au> <20260113231201.09f44967@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="dyc6SrwZ8ruA68kO" Content-Disposition: inline In-Reply-To: <20260113231201.09f44967@elisabeth> Message-ID-Hash: 4J2LHJCIV277HY24YL4JZKLEJUSXD3AR X-Message-ID-Hash: 4J2LHJCIV277HY24YL4JZKLEJUSXD3AR 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: --dyc6SrwZ8ruA68kO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jan 13, 2026 at 11:12:01PM +0100, Stefano Brivio wrote: > Silly nits only and a couple of remarks that will probably be clarified > at a later point: >=20 > On Thu, 8 Jan 2026 13:29:45 +1100 > David Gibson wrote: >=20 > > We now have a formal array of forwarding rules. However, we don't actu= ally > > consult it when we forward a new flow. Instead we rely on (a) implicit > > information (we wouldn't be here if there wasn't a listening socket for= the > > rule) and (b) the legacy delta[] data structure. > >=20 > > Start addressing this, by searching for a matching forwarding rule when > > attempting to forward a new flow. For now this is incomplete: > > * We only do this for socket-initiated flows > > * We make a potentially costly linear lookup > > * We don't actually use the matching rule for anything yet > >=20 > > We'll address each of those in later patches. > >=20 > > Signed-off-by: David Gibson > > --- > > flow.c | 43 ++++++++++++++++++++++++++++++++++--------- > > fwd.c | 33 +++++++++++++++++++++++++++++++++ > > fwd.h | 2 ++ > > 3 files changed, 69 insertions(+), 9 deletions(-) > >=20 > > diff --git a/flow.c b/flow.c > > index 4f534865..045e9712 100644 > > --- a/flow.c > > +++ b/flow.c > > @@ -489,7 +489,7 @@ struct flowside *flow_initiate_sa(union flow *flow,= uint8_t pif, > > struct flowside *flow_target(const struct ctx *c, union flow *flow, > > uint8_t proto) > > { > > - char estr[INANY_ADDRSTRLEN], fstr[INANY_ADDRSTRLEN]; > > + char estr[INANY_ADDRSTRLEN], ostr[INANY_ADDRSTRLEN]; > > struct flow_common *f =3D &flow->f; > > const struct flowside *ini =3D &f->side[INISIDE]; > > struct flowside *tgt =3D &f->side[TGTSIDE]; > > @@ -500,6 +500,30 @@ struct flowside *flow_target(const struct ctx *c, = union flow *flow, > > ASSERT(f->pif[INISIDE] !=3D PIF_NONE && f->pif[TGTSIDE] =3D=3D PIF_NO= NE); > > ASSERT(flow->f.state =3D=3D FLOW_STATE_INI); > > =20 > > + if (pif_is_socket(f->pif[INISIDE])) { > > + const struct fwd_ports *fwd; > > + > > + if (f->pif[INISIDE] =3D=3D PIF_HOST && proto =3D=3D IPPROTO_TCP) > > + fwd =3D &c->tcp.fwd_in; > > + else if (f->pif[INISIDE] =3D=3D PIF_HOST && proto =3D=3D IPPROTO_UDP) > > + fwd =3D &c->udp.fwd_in; > > + else if (f->pif[INISIDE] =3D=3D PIF_SPLICE && proto =3D=3D IPPROTO_T= CP) > > + fwd =3D &c->tcp.fwd_out; > > + else if (f->pif[INISIDE] =3D=3D PIF_SPLICE && proto =3D=3D IPPROTO_U= DP) > > + fwd =3D &c->udp.fwd_out; > > + else > > + goto nofwd; > > + > > + if (!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 > > + */ > > + flow_dbg(flow, "Unexpected missing forward rule"); > > + goto nofwd; > > + } > > + } > > + > > switch (f->pif[INISIDE]) { > > case PIF_TAP: > > memcpy(f->tap_omac, MAC_UNDEF, ETH_ALEN); > > @@ -514,22 +538,23 @@ struct flowside *flow_target(const struct ctx *c,= union flow *flow, > > tgtpif =3D fwd_nat_from_host(c, proto, ini, tgt); > > fwd_neigh_mac_get(c, &tgt->oaddr, f->tap_omac); > > break; > > - > > default: > > - flow_err(flow, "No rules to forward %s [%s]:%hu -> [%s]:%hu", > > - pif_name(f->pif[INISIDE]), > > - inany_ntop(&ini->eaddr, estr, sizeof(estr)), > > - ini->eport, > > - inany_ntop(&ini->oaddr, fstr, sizeof(fstr)), > > - ini->oport); > > + goto nofwd; > > } > > =20 > > if (tgtpif =3D=3D PIF_NONE) > > - return NULL; > > + goto nofwd; > > =20 > > f->pif[TGTSIDE] =3D tgtpif; > > flow_set_state(f, FLOW_STATE_TGT); > > return tgt; > > + > > +nofwd: > > + flow_err(flow, "No rules to forward %s %s [%s]:%hu -> [%s]:%hu", > > + pif_name(f->pif[INISIDE]), ipproto_name(proto), > > + inany_ntop(&ini->eaddr, estr, sizeof(estr)), ini->eport, > > + inany_ntop(&ini->oaddr, ostr, sizeof(ostr)), ini->oport); >=20 > This assumes we're still using this function for inbound forwards only > (eaddr / eport -> oaddr / oport), perhaps we'll want a macro once it > starts being used for the other way around as well (if at all). No, that's correct for outbound as well. Both the addresses are from the initiating side (there is no target side, because we can't forward). It's always src -> dst for the exchange initiating the flow. > By the way, for rules, earlier in this series, you used "=3D>" to separate > source and target of the forward, here it's still "->", I guess we > should settle on one version (it just occurred to me while testing > stuff: it might be useful to grep in logs). As above, this is actually consistent. =3D> separates sides, both addresses are on the initiating side here. >=20 > > + return NULL; > > } > > =20 > > /** > > diff --git a/fwd.c b/fwd.c > > index 3f088fd2..633ba5db 100644 > > --- a/fwd.c > > +++ b/fwd.c > > @@ -409,6 +409,39 @@ void fwd_rule_add(struct fwd_ports *fwd, uint8_t f= lags, > > } > > } > > =20 > > +/** > > + * fwd_rule_match() - Does a prospective flow match a given forwarding= rule >=20 > Does [...]? Fixed. > > + * @rule: Forwarding rule > > + * @ini: Initiating side flow information > > + * > > + * Returns: true if the rule applies to the flow, false otherwise > > + */ > > +static bool fwd_rule_match(const struct fwd_rule *rule, > > + const struct flowside *ini) > > +{ > > + return inany_matches(&ini->oaddr, fwd_rule_addr(rule)) && > > + ini->oport >=3D rule->first && ini->oport <=3D rule->last; >=20 > The usual alignment is: >=20 > return inany_matches(&ini->oaddr, fwd_rule_addr(rule)) && > ini->oport >=3D rule->first && ini->oport <=3D rule->last; Huh. My emacs config almost always matches the passt style, but not in this case (I think it treats bare expressions like this differently =66rom function parameters). > > +} > > + > > +/** > > + * fwd_rule_search() - Find a rule which matches a prospective flow > > + * @fwd: Forwarding table > > + * @ini: Initiating side flow information > > + * > > + * Returns: first matching rule, or NULL if there is none >=20 > I guess that this will eventually need to become a function matching > the most specific rule first, tie breakers could be: Maybe, but I'm hoping not. The current model I have in mind is always first rule matches - if you want most specific first, then you need to order them like that. Potentially re-ordering by specificity is something the update client could do. In any case, I think that's something we want to avoid doing at the point we actually look up the table. > 1. specific address given vs. wildcard > 2. specific interface given vs. no interface > 3. the day we support/need it: specific port/range vs. no port > 4. smallest port range > 5. the day we support/need something like this: longest prefix length I'm not sure specificity rules will always give a total order. Having an explicit order lets is disambiguate such cases. > and after this we should actually have an error on insertion (already > guaranteed I think). That's a good idea. Even while it's first-rule-wins, we could give an error/warning if a rule is added that's unreachable because an earlier one will always win. I think that's slightly more complicated than the conflicting rules checking I already implemented, but not dramatically so. > > + */ > > +const struct fwd_rule *fwd_rule_search(const struct fwd_ports *fwd, > > + const struct flowside *ini) > > +{ > > + unsigned i; > > + > > + for (i =3D 0; i < fwd->count; i++) { > > + if (fwd_rule_match(&fwd->rules[i], ini)) > > + return &fwd->rules[i]; > > + } >=20 > Extra newline here to clearly separate the two outcomes. Done. > > + return NULL; > > +} > > + > > /** > > * fwd_rules_print() - Print forwarding rules for debugging > > * @fwd: Table to print > > diff --git a/fwd.h b/fwd.h > > index f84e7c01..a10bdbb4 100644 > > --- a/fwd.h > > +++ b/fwd.h > > @@ -103,6 +103,8 @@ struct fwd_ports { > > 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); > > +const struct fwd_rule *fwd_rule_search(const struct fwd_ports *fwd, > > + const struct flowside *ini); > > void fwd_rules_print(const struct fwd_ports *fwd); > > =20 > > void fwd_scan_ports_init(struct ctx *c); >=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 --dyc6SrwZ8ruA68kO Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmlm3qEACgkQzQJF27ox 2Gfpiw/8DUl13wf2lpDMalxkAchj3/i4qnfNmCOxJzKD32s4/EDjVEN353LZjVBu /IK0NV9zh4SUixJ2cgaG9Um+jKp10qOjkOmKjcsMxv0rhnXQs55JZFah+y+XoiQk nGXbDcD5fT+tQYX1zr4UEXHBlP69jdhXYq/H+u7GQMWeG9NDAB3KBNNsB7tCqTgS SFMUZ6DV+bQeiXAB1v4zM5LhpuJ7w1iKWciBmwh9DwqJjF3NjLsRKtZl0CbMxzi+ cSMmkJJGC9w2fcYtGt/3yR3HEc1GTsP/kQTVnyLifhtBAEkxt6Z9m6LUWpCDbtOv E9AO2BtrbltzMwjwk4LQIz7ukw3JWbSrkKBD9665/uFP5afOgEnMQiTDUyK7mOXf MYCfqdsk7SMAa4yVl66xF7ena/fZKkPFLqTdC2odZTn/VvlqfPAK8Rw+rmFzPAEO DUTb4fTtl/utiPZ24Scj1kV0NkD7luKvMRDcAxqteA3olnID8r5wz6aMxnumOFa+ RsVdaps4IBBG6hppDU4YW/AtyCg8O7rBa3xGvE85FdcLk+BAH4n94FrjDI59x6pS xjSb873uENGeDVgTEE9VXxXys0/XE4lfRimDRSiN1wNujKg8F1NX3WyuZfXv3pB2 B9vsii+T1u+YL2IuuRKe8K+JQEDvWjjuO/10npHF8z7WOExcyQs= =/Odm -----END PGP SIGNATURE----- --dyc6SrwZ8ruA68kO--