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: > > > Amongst other checks, fwd_rule_add() checks that the newly added rule > > doesn't conflict with any existing rules. However, unlike the other things > > 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). > > > > In order to support dynamic rule updates, we want fwd_rule_add() to become > > 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. > > > > 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(-) > > > > 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, > > > > if (c->ifi4) { > > rulev.addr = inany_loopback4; > > + fwd_rule_conflict_check(&rulev, > > + fwd->rules, fwd->count); > > fwd_rule_add(fwd, &rulev); > > } > > if (c->ifi6) { > > rulev.addr = 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 = 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) > > /* Flags which can be set from the caller */ > > const uint8_t allowed_flags = FWD_WEAK | FWD_SCAN | FWD_DUAL_STACK_ANY; > > unsigned num = (unsigned)new->last - new->first + 1; > > - unsigned i, port; > > + unsigned port; > > > > 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 struct fwd_rule *new) > > if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) > > die("Too many listening sockets"); > > > > - /* Check for any conflicting entries */ > > - for (i = 0; i < fwd->count; i++) { > > - char newstr[INANY_ADDRSTRLEN], rulestr[INANY_ADDRSTRLEN]; > > - const struct fwd_rule *rule = &fwd->rules[i]; > > - > > - if (new->proto != 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] = &fwd->socks[fwd->sock_count]; > > for (port = new->first; port <= new->last; port++) > > fwd->rulesocks[fwd->count][port - new->first] = -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) > > 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 != 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 <= a->last && b->first <= 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 any in list > > 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 = 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 { > > > > 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); > > > > #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=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -pie -fPIE -DPAGE_SIZE=4096 -DVERSION=\"2026_01_20.386b5f5-84-ge87c74f\" -DDUAL_STACK_SOCKETS=1 -DHAS_GETRANDOM -fstack-protector-strong -Ofast -mavx2 -ftree-vectorize -funroll-loops \ > arch.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 udp_flow.c udp_vu.c util.c vhost_user.c virtio.c vu_common.c -o passt.avx2 > 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 ‘fwd_rules_info’: > fwd_rule.c:86:22: warning: ‘%s’ directive argument is null [-Wformat-overflow=] > 86 | info(" %s", fwd_rule_fmt(&rules[i], buf, sizeof(buf))); > | ^~~~~~~~ > log.h:31:66: note: in definition of macro ‘info’ > 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. Huh. I don't see those those with gcc 15.2.1. I _think_ it's a false positive. Investigating... -- 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