From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=d33clgsp; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id C6C885A0262 for ; Wed, 08 Apr 2026 01:16:20 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1775603779; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=QAET5yelBMQj9tUEKzS5yaLXhmU8UNnRpW3fiCblsXw=; b=d33clgspLqg8ph+bu2ibmn7WcAEAjw5YZjXmQ/oWfiYkq8DuvJYCiclLzl7Nkyo6S0cs8h FYzc2QZqtcAOe/Qi6I3EpphI7PoecgELXIfuorpBlKqBmnuRVkcbqCr4xNxEx1TM7Oj+UZ C7VPxg8+9lWQjRRo4oZRZs3/YYPO86M= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-443-SokrHN6CPR-dXXrvysJ71g-1; Tue, 07 Apr 2026 19:16:18 -0400 X-MC-Unique: SokrHN6CPR-dXXrvysJ71g-1 X-Mimecast-MFC-AGG-ID: SokrHN6CPR-dXXrvysJ71g_1775603777 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-488c74405ecso162845e9.1 for ; Tue, 07 Apr 2026 16:16:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1775603777; x=1776208577; h=date:content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=8GV9M53OUsXyWXE9jqX9BMidnneaj120OC19hC9cjbs=; b=Af+YTU90g6A1C5ednWd56THlB0ZmdNeqQ4ywQjpi34SV65rFPD4rhJQPDfQUIYrvo9 HTc9u67UfHZ7CVeVoxA91DYr0EgtDXg9lQ4SEPhsGVl89z2kfuqXaOGGr1Rq3qcfUsyL QzZIoR+gL6gUee87aemwsbXPYdy+SVNLoD0P068WTkttrK5Ptw9L+qsQpygjZEys6m0w MgWjWVPQbjo41BeQsMhxdx473r2lARhEYlLbrz9wD/5ANn+u2iPZPHV8LWbWgAEiWf1k bsEX6J7cQrGxTzxXmxGflTeriMSGFRuhRwiy3g7oLck9CoxKP6+fNaKo2N374o7xm7vj oJEw== X-Gm-Message-State: AOJu0YxUoVPOt7sE0WPYXO6ywSNvvXmUtQxY5OV0tU7QkMfLZBYTOc6/ aaS9mmafACMXFz74Sd2rVW+pdbnX20GoCcswsHyAeQnoYcV3yZfsQEJqnzvQKJiBpGBWvvxBujK TaYG8aydwpfAXDyIsmKIYpgSLLE2G7fcYn/uPfQRvcN8pidxBG2+p+Py5BqPRuA== X-Gm-Gg: AeBDiettgw/zzRpRDefyA6YI4yQAlSMQEZ1ghEsZawCfNwzKNy1E01gvOARwgPpLClY DGvlvQiPI8g/RboyDMs62tA38pdfhoHavNIGgG3TVYeWTrhh+vxrqcW57zNPXOUztzASxA2sMnC lahO1OfEX3V/VZ8Kon3+1cquPLYMXWy4/ZTFBuYFynb0bDuMAMUJBVPAL5JPOATee+np96KxzDJ hAN8Egu3TKsDikPfZsKnPyRtnB+R1u/VAdSnRT21iEnd9yOG0eKPYLmI7Vc0p1/2dK8nJdoHu+6 Fp5oj9T+kH/EuzpH3jTud/WXDD032YlLZXiC+O2L3AMEaM/M/rk0/fEvLUWg3wJjAUIZlCPrrq8 NlAFU7dhbbNummlvjJyOBM5eAKUBKwsLu X-Received: by 2002:a05:600c:628e:b0:480:20f1:7aa6 with SMTP id 5b1f17b1804b1-488997dd822mr241841195e9.21.1775603776896; Tue, 07 Apr 2026 16:16:16 -0700 (PDT) X-Received: by 2002:a05:600c:628e:b0:480:20f1:7aa6 with SMTP id 5b1f17b1804b1-488997dd822mr241840855e9.21.1775603776504; Tue, 07 Apr 2026 16:16:16 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-488be75215dsm87652975e9.6.2026.04.07.16.14.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Apr 2026 16:15:42 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 10/18] fwd_rule: Move rule conflict checking from fwd_rule_add() to caller Message-ID: <20260408011450.1ef3fd7c@elisabeth> In-Reply-To: <20260407031630.2457081-11-david@gibson.dropbear.id.au> References: <20260407031630.2457081-1-david@gibson.dropbear.id.au> <20260407031630.2457081-11-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Wed, 08 Apr 2026 01:14:50 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: TgGOaTT5jph1kil0eBBL4nwbREI1gCA_PJOVV7v9fHc_1775603777 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: YYN2GMUWSDZ6MHPP7FJOODGUDNARQJDF X-Message-ID-Hash: YYN2GMUWSDZ6MHPP7FJOODGUDNARQJDF X-MailFrom: sbrivio@redhat.com 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: On Tue, 7 Apr 2026 13:16:22 +1000 David Gibson wrote: > Amongst other checks, fwd_rule_add() checks that the newly added rule > doesn't conflict with any existing rules. However, unlike the other thin= gs > we verify, this isn't really required for safe operation. Rule conflicts > 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 becom= e > 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 ct= x *c, char optname, > =20 > =09=09=09if (c->ifi4) { > =09=09=09=09rulev.addr =3D inany_loopback4; > +=09=09=09=09fwd_rule_conflict_check(&rulev, > +=09=09=09=09=09=09=09fwd->rules, fwd->count); > =09=09=09=09fwd_rule_add(fwd, &rulev); > =09=09=09} > =09=09=09if (c->ifi6) { > =09=09=09=09rulev.addr =3D inany_loopback6; > +=09=09=09=09fwd_rule_conflict_check(&rulev, > +=09=09=09=09=09=09=09fwd->rules, fwd->count); > =09=09=09=09fwd_rule_add(fwd, &rulev); > =09=09=09} > =09=09} else { > +=09=09=09fwd_rule_conflict_check(&rule, fwd->rules, fwd->count); > =09=09=09fwd_rule_add(fwd, &rule); > =09=09} > =09=09base =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 struct= fwd_rule *new) > =09/* Flags which can be set from the caller */ > =09const uint8_t allowed_flags =3D FWD_WEAK | FWD_SCAN | FWD_DUAL_STACK_= ANY; > =09unsigned num =3D (unsigned)new->last - new->first + 1; > -=09unsigned i, port; > +=09unsigned port; > =20 > =09assert(!(new->flags & ~allowed_flags)); > =09/* 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 struc= t fwd_rule *new) > =09if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) > =09=09die("Too many listening sockets"); > =20 > -=09/* Check for any conflicting entries */ > -=09for (i =3D 0; i < fwd->count; i++) { > -=09=09char newstr[INANY_ADDRSTRLEN], rulestr[INANY_ADDRSTRLEN]; > -=09=09const struct fwd_rule *rule =3D &fwd->rules[i]; > - > -=09=09if (new->proto !=3D rule->proto) > -=09=09=09/* Non-conflicting protocols */ > -=09=09=09continue; > - > -=09=09if (!inany_matches(fwd_rule_addr(new), fwd_rule_addr(rule))) > -=09=09=09/* Non-conflicting addresses */ > -=09=09=09continue; > - > -=09=09if (new->last < rule->first || rule->last < new->first) > -=09=09=09/* Port ranges don't overlap */ > -=09=09=09continue; > - > -=09=09die("Forwarding configuration conflict: %s/%u-%u versus %s/%u-%u", > -=09=09 inany_ntop(fwd_rule_addr(new), newstr, sizeof(newstr)), > -=09=09 new->first, new->last, > -=09=09 inany_ntop(fwd_rule_addr(rule), rulestr, sizeof(rulestr)), > -=09=09 rule->first, rule->last); > -=09} > - > =09fwd->rulesocks[fwd->count] =3D &fwd->socks[fwd->sock_count]; > =09for (port =3D new->first; port <=3D new->last; port++) > =09=09fwd->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, size= _t count) > =09=09info(" %s", fwd_rule_fmt(&rules[i], buf, sizeof(buf))); > =09} > } > + > +/** > + * fwd_rule_conflicts() - Test if two rules conflict with each other > + * @a, @b:=09Rules to test > + */ > +static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fw= d_rule *b) > +{ > +=09if (a->proto !=3D b->proto) > +=09=09/* Non-conflicting protocols */ > +=09=09return false; > + > +=09if (!inany_matches(fwd_rule_addr(a), fwd_rule_addr(b))) > +=09=09/* Non-conflicting addresses */ > +=09=09return false; > + > +=09assert(a->first <=3D a->last && b->first <=3D b->last); > +=09if (a->last < b->first || b->last < a->first) > +=09=09/* Port ranges don't overlap */ > +=09=09return false; > + > +=09return true; > +} > + > +/* fwd_rule_conflict_check() - Die with errir if rule conflicts with any= in list Nit: an errir happens only when Mimir (ask Jon) makes a mistake, which is quite rare. :) > + * @new:=09New rule > + * @rules:=09Existing rules against which to test > + * @count:=09Number of rules in @rules > + */ > +void fwd_rule_conflict_check(const struct fwd_rule *new, > +=09=09=09 const struct fwd_rule *rules, size_t count) > +{ > +=09unsigned i; > + > +=09for (i =3D 0; i < count; i++) { > +=09=09char newstr[FWD_RULE_STRLEN], rulestr[FWD_RULE_STRLEN]; > + > +=09=09if (!fwd_rule_conflicts(new, &rules[i])) > +=09=09=09continue; > + > +=09=09die("Forwarding configuration conflict: %s versus %s", > +=09=09 fwd_rule_fmt(new, newstr, sizeof(newstr)), > +=09=09 fwd_rule_fmt(&rules[i], rulestr, sizeof(rulestr))); > +=09} > +} > 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, > +=09=09=09 const struct fwd_rule *rules, size_t count); > =20 > #endif /* FWD_RULE_H */ I reviewed only up to here so far, the rest will come in a bit. 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. Meanwhile, I noticed some warnings that strangely appear only during the build of passt.avx2: 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 \ =09arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c epoll_ctl.c flow= .c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c= 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 ud= p_flow.c udp_vu.c util.c vhost_user.c virtio.c vu_common.c -o passt.avx2=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 null = [-Wformat-overflow=3D] 86 | info(" %s", fwd_rule_fmt(&rules[i], buf, sizeof(= 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, sizeof(= buf))); | ^~ ...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. --=20 Stefano