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=202602 header.b=JnBaG7rI; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 6ECFA5A0271 for ; Wed, 08 Apr 2026 03:37:13 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1775612230; bh=NkStIIVS89YottfD8ty9+e2R3txT80UusS7WLVDSOP0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=JnBaG7rI6mXaUIjzBPXcpGfhU00UulULGLC8chbxurLvF7wWRt/zs7/V9u93Llc+M IPRBjtJ+ZHKictxKwPsPWeTDGMghDfYPIajzL4S2TaV6ZDFLnUg62ADDY6CVwW/Y2w qEDsvbVpdWr34VeSrksvJ52VSRncmNXZXhKD8b5nfMplMU1duw6cS8yQP6kO2wy+rf PLSjcBai2Q40jFMi1QfJMZ/pY6I9BZgV20SqJlJksxBoHW1rz8TgYhK4uDiKti7olv J/L+8EZBsZgetvsqvbpCigu5dmbjc8YV1jWZKFPUU/5f5IzY1uMlfYpct9JPtqq9Lu NUSFSaV7AfNaA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4fr5K20BQ0z4wV1; Wed, 08 Apr 2026 11:37:10 +1000 (AEST) Date: Wed, 8 Apr 2026 11:37:04 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 10/18] fwd_rule: Move rule conflict checking from fwd_rule_add() to caller Message-ID: References: <20260407031630.2457081-1-david@gibson.dropbear.id.au> <20260407031630.2457081-11-david@gibson.dropbear.id.au> <20260408011450.1ef3fd7c@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="ueCxGru64Sd8oUVh" Content-Disposition: inline In-Reply-To: <20260408011450.1ef3fd7c@elisabeth> Message-ID-Hash: S37R27ID5X43PH6APZVLNLM5B53ZOUU4 X-Message-ID-Hash: S37R27ID5X43PH6APZVLNLM5B53ZOUU4 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: --ueCxGru64Sd8oUVh Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Apr 08, 2026 at 01:14:50AM +0200, Stefano Brivio wrote: > On Tue, 7 Apr 2026 13:16:22 +1000 > David Gibson wrote: >=20 > > Amongst other checks, fwd_rule_add() checks that the newly added rule > > doesn't conflict with any existing rules. However, unlike the other th= ings > > we verify, this isn't really required for safe operation. Rule conflic= ts > > are a useful thing for the user to know about, but the forwarding logic > > is perfectly sound with conflicting rules (the first one will win). > >=20 > > In order to support dynamic rule updates, we want fwd_rule_add() to bec= ome > > a more low-level function, only checking the things it really needs to. > > So, move rule conflict checking to its caller via new helpers in > > fwd_rule.c. > >=20 > > Signed-off-by: David Gibson > > --- > > conf.c | 5 +++++ > > fwd.c | 26 +------------------------- > > fwd_rule.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > > fwd_rule.h | 2 ++ > > 4 files changed, 52 insertions(+), 25 deletions(-) > >=20 > > diff --git a/conf.c b/conf.c > > index c9ee8c59..a93837cc 100644 > > --- a/conf.c > > +++ b/conf.c > > @@ -205,13 +205,18 @@ static void conf_ports_range_except(const struct = ctx *c, char optname, > > =20 > > if (c->ifi4) { > > rulev.addr =3D inany_loopback4; > > + fwd_rule_conflict_check(&rulev, > > + fwd->rules, fwd->count); > > fwd_rule_add(fwd, &rulev); > > } > > if (c->ifi6) { > > rulev.addr =3D inany_loopback6; > > + fwd_rule_conflict_check(&rulev, > > + fwd->rules, fwd->count); > > fwd_rule_add(fwd, &rulev); > > } > > } else { > > + fwd_rule_conflict_check(&rule, fwd->rules, fwd->count); > > fwd_rule_add(fwd, &rule); > > } > > base =3D i - 1; > > diff --git a/fwd.c b/fwd.c > > index c05107d1..c9637525 100644 > > --- a/fwd.c > > +++ b/fwd.c > > @@ -341,7 +341,7 @@ void fwd_rule_add(struct fwd_table *fwd, const stru= ct fwd_rule *new) > > /* Flags which can be set from the caller */ > > const uint8_t allowed_flags =3D FWD_WEAK | FWD_SCAN | FWD_DUAL_STACK_= ANY; > > unsigned num =3D (unsigned)new->last - new->first + 1; > > - unsigned i, port; > > + unsigned port; > > =20 > > assert(!(new->flags & ~allowed_flags)); > > /* Passing a non-wildcard address with DUAL_STACK_ANY is a bug */ > > @@ -354,30 +354,6 @@ void fwd_rule_add(struct fwd_table *fwd, const str= uct fwd_rule *new) > > if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) > > die("Too many listening sockets"); > > =20 > > - /* Check for any conflicting entries */ > > - for (i =3D 0; i < fwd->count; i++) { > > - char newstr[INANY_ADDRSTRLEN], rulestr[INANY_ADDRSTRLEN]; > > - const struct fwd_rule *rule =3D &fwd->rules[i]; > > - > > - if (new->proto !=3D rule->proto) > > - /* Non-conflicting protocols */ > > - continue; > > - > > - if (!inany_matches(fwd_rule_addr(new), fwd_rule_addr(rule))) > > - /* Non-conflicting addresses */ > > - continue; > > - > > - if (new->last < rule->first || rule->last < new->first) > > - /* Port ranges don't overlap */ > > - continue; > > - > > - die("Forwarding configuration conflict: %s/%u-%u versus %s/%u-%u", > > - inany_ntop(fwd_rule_addr(new), newstr, sizeof(newstr)), > > - new->first, new->last, > > - inany_ntop(fwd_rule_addr(rule), rulestr, sizeof(rulestr)), > > - rule->first, rule->last); > > - } > > - > > fwd->rulesocks[fwd->count] =3D &fwd->socks[fwd->sock_count]; > > for (port =3D new->first; port <=3D new->last; port++) > > fwd->rulesocks[fwd->count][port - new->first] =3D -1; > > diff --git a/fwd_rule.c b/fwd_rule.c > > index abe9dfbf..4d5048f9 100644 > > --- a/fwd_rule.c > > +++ b/fwd_rule.c > > @@ -87,3 +87,47 @@ void fwd_rules_info(const struct fwd_rule *rules, si= ze_t count) > > 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 > > + */ > > +static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct = fwd_rule *b) > > +{ > > + if (a->proto !=3D b->proto) > > + /* Non-conflicting protocols */ > > + return false; > > + > > + if (!inany_matches(fwd_rule_addr(a), fwd_rule_addr(b))) > > + /* Non-conflicting addresses */ > > + return false; > > + > > + assert(a->first <=3D a->last && b->first <=3D b->last); > > + if (a->last < b->first || b->last < a->first) > > + /* Port ranges don't overlap */ > > + return false; > > + > > + return true; > > +} > > + > > +/* fwd_rule_conflict_check() - Die with errir if rule conflicts with a= ny in list >=20 > Nit: an errir happens only when Mimir (ask Jon) makes a mistake, which > is quite rare. :) Fixed. Also the incorrect formatting for the start of the comment block. > > + * @new: New 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) > > +{ > > + unsigned i; > > + > > + for (i =3D 0; i < count; i++) { > > + char newstr[FWD_RULE_STRLEN], rulestr[FWD_RULE_STRLEN]; > > + > > + if (!fwd_rule_conflicts(new, &rules[i])) > > + continue; > > + > > + die("Forwarding configuration conflict: %s versus %s", > > + fwd_rule_fmt(new, newstr, sizeof(newstr)), > > + fwd_rule_fmt(&rules[i], rulestr, sizeof(rulestr))); > > + } > > +} > > diff --git a/fwd_rule.h b/fwd_rule.h > > index e92efb6d..f852be39 100644 > > --- a/fwd_rule.h > > +++ b/fwd_rule.h > > @@ -52,5 +52,7 @@ struct fwd_rule { > > =20 > > const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule); > > 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); > > =20 > > #endif /* FWD_RULE_H */ >=20 > I reviewed only up to here so far, the rest will come in a bit. >=20 > I had a quick look at the whole series and it all looks good to me so > far but that wasn't quite a review. >=20 > Meanwhile, I noticed some warnings that strangely appear only during the > build of passt.avx2: >=20 > cc -Wall -Wextra -Wno-format-zero-length -Wformat-security -pedantic -std= =3Dc11 -D_XOPEN_SOURCE=3D700 -D_GNU_SOURCE -D_FORTIFY_SOURCE=3D2 -pie -fPIE= -DPAGE_SIZE=3D4096 -DVERSION=3D\"2026_01_20.386b5f5-84-ge87c74f\" -DDUAL_S= TACK_SOCKETS=3D1 -DHAS_GETRANDOM -fstack-protector-strong -Ofast -mavx2 -ft= ree-vectorize -funroll-loops \ > arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c epoll_ctl.c flow= =2Ec fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread= =2Ec log.c mld.c ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c = pif.c repair.c serialise.c tap.c tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.= c udp_flow.c udp_vu.c util.c vhost_user.c virtio.c vu_common.c -o passt.avx= 2=20 > In file included from util.h:22, > from ip.h:12, > from fwd_rule.h:16, > from fwd_rule.c:20: > fwd_rule.c: In function =E2=80=98fwd_rules_info=E2=80=99: > fwd_rule.c:86:22: warning: =E2=80=98%s=E2=80=99 directive argument is nul= l [-Wformat-overflow=3D] > 86 | info(" %s", fwd_rule_fmt(&rules[i], buf, sizeo= f(buf))); > | ^~~~~~~~ > log.h:31:66: note: in definition of macro =E2=80=98info=E2=80=99 > 31 | #define info(...) logmsg(true, false, LOG_INFO, = __VA_ARGS__) > | = ^~~~~~~~~~~ > fwd_rule.c:86:27: note: format string is defined here > 86 | info(" %s", fwd_rule_fmt(&rules[i], buf, sizeo= f(buf))); > | ^~ >=20 > ...but I don't think I looked at the code changes causing them, yet (and > didn't bisect the series either). This is with gcc 13.3.0. Huh. I don't see those those with gcc 15.2.1. I _think_ it's a false positive. Investigating... --=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 --ueCxGru64Sd8oUVh Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmnVsT8ACgkQzQJF27ox 2GcMaw//fmI6yhcqpsQT+CKxtLbfcZzUDG9QRsvOrnMFW//Nevl4JrFDlSmc4U19 EXtkGOgCV6ARcT3v+roEk65ZEL29sG+o/6uCazM/hSy5JwYJtSXiorBD9AIde+4g zqXz5tjVB3Ek85Qg2iYKXF6NBV71EOuDbVwq1Y566OiB9mxpFkH4zvTBMTZLhTgD EDKxyBggUqbyFf+UuwqSRbWooCu3jdx2gUBaoiW2JWpjsRO5tB0KyZ29hAJhg/cU YjX30zVU9XAihe1vvNlTAy9hmbUTcH4eNL2HZjKz6SojYEIf3y03lCkCOku4uOiv uL/fIOON8z2YUm9A6aRmyLLIh3yWh49f5NzkUVWS21zE2BzCnEhj9XMPFtEIJSw5 4kcOQWkvCpEyZ/WVyuNMg2ZjbhPwQpTebrWjWlkkqk9oUrxuDWklWe9Fx/j7BrSI ryaLgImTsIhCe5c9l2GmsoaBlx+9bwuucxrPFNGh8am01/plDldj7E/z+g0p+Y6u 95Fa2qaP74b2IRqqaSkxzTUZs/0z90zbT1cOvsHUASvtFFKarLffbywkTojNSo3b My03pKZ6TZleMWOurxixVkmRe3v3beFjZEsiGs6WVn/QB5FPkJc1oyBBA1XhXbyZ jARUWoMGWiF1ZaReky3JRGgyhsHda+uO8KKZVMDmSHTFoIHiiCk= =zFpS -----END PGP SIGNATURE----- --ueCxGru64Sd8oUVh--