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

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Apr 16, 2026 at 12:04:28AM +0200, Stefano Brivio wrote:
> On Fri, 10 Apr 2026 11:02:56 +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 | 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

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