Re: [PATCH 10/18] fwd_rule: Move rule conflict checking from fwd_rule_add() to caller

David Gibson
2026-04-08T01:37:13Z

[-- Attachment #1: Type: text/plain, Size: 8342 bytes --]

On Wed, Apr 08, 2026 at 01:14:50AM +0200, Stefano Brivio wrote:
> On Tue, 7 Apr 2026 13:16:22 +1000
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > ---
> > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]