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

On Thu, Apr 16, 2026 at 12:04:28AM +0200, Stefano Brivio wrote:

On Fri, 10 Apr 2026 11:02:56 +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 | 45 +++++++++++++++++++++++++++++++++++++++++++++ fwd_rule.h | 2 ++ 4 files changed, 53 insertions(+), 25 deletions(-)

diff --git a/conf.c b/conf.c index 027bbac9..b871646f 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 a034d5d1..5bc94efe 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -93,3 +93,48 @@ 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);

I expected this assert() to be gone by the end of the series, like the ones dropped in 11/23, but it's still there. Is this something that the client can't specifically trigger for some reason?

Oops, yeah, that's a mistake. What I had in mind is that the rule is first checked for internal consistency (including first <= last), and only then do we conflict check. But then I changed things so that's no longer the case. I'll fix it with followup patches in the next spin. -- 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