Re: [PATCH v3 09/14] conf, fwd: Check forwarding table for conflicting rules

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

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

On Tue, Jan 13, 2026 at 12:26:46AM +0100, Stefano Brivio wrote:
> On Thu,  8 Jan 2026 13:29:43 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > It's possible for a user to supply conflicting forwarding parameters, e.g.
> >     $ pasta -t 80:8080 -t 127.0.0.1/80:8888
> > 
> > We give a warning in this case, but it's based on the legacy
> > forwarding bitmaps.  This is too strict, because it will also warn on
> > cases that shouldn't conflict because they use different addresses,
> > e.g.
> >     $ pasta -t 192.0.2.1/80:8080 127.0.0.1/80:8888
> > 
> > Theoretically, it's also too loose because it won't take into account
> > auto-scan forwarding rules.  We can't hit that in practice now,
> > because we only ever have one auto-scan rule and nothing else, but we
> > want to remove that restriction in future.
> > 
> > Replace the bitmap based check with a check based on actually scanning
> > the forwarding rules for conflicts.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  conf.c  |  5 -----
> >  fwd.c   | 21 ++++++++++++++++++++-
> >  inany.c | 19 +++++++++++++++++++
> >  inany.h |  1 +
> >  4 files changed, 40 insertions(+), 6 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index 725cf88b..e8d6d5d9 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -172,11 +172,6 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
> >  		for (i = base; i <= last; i++) {
> >  			if (exclude && bitmap_isset(exclude, i))
> >  				break;
> > -
> > -			if (bitmap_isset(fwd->map, i)) {
> > -				warn(
> > -"Altering mapping of already mapped port number: %s", optarg);
> > -			}
> >  		}
> >  
> >  		if ((optname == 'T' || optname == 'U') && c->no_bindtodevice) {
> > diff --git a/fwd.c b/fwd.c
> > index 70ef73a3..5208155b 100644
> > --- a/fwd.c
> > +++ b/fwd.c
> > @@ -348,7 +348,7 @@ void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags,
> >  	const uint8_t allowed_flags = FWD_WEAK | FWD_SCAN;
> >  	unsigned num = (unsigned)last - first + 1;
> >  	struct fwd_rule *new;
> > -	unsigned port;
> > +	unsigned i, port;
> >  
> >  	ASSERT(!(flags & ~allowed_flags));
> >  
> > @@ -357,6 +357,25 @@ void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags,
> >  	if ((fwd->listen_sock_count + num) > ARRAY_SIZE(fwd->listen_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];
> > +		struct fwd_rule *rule = &fwd->rules[i];
> > +
> > +		if (!inany_matches(addr, fwd_rule_addr(rule)))
> > +			/* Non-conflicting addresses */
> > +			continue;
> > +
> > +		if (last < rule->first || rule->last < first)
> > +			/* Port ranges don't overlap */
> > +			continue;
> > +
> > +		die("Forwarding configuration conflict: %s/%u-%u versus %s/%u-%u",
> > +		    inany_ntop(addr, newstr, sizeof(newstr)), first, last,
> > +		    inany_ntop(fwd_rule_addr(rule), rulestr, sizeof(rulestr)),
> > +		    rule->first, rule->last);
> 
> Same as comments to earlier places in fwd_rule_add(): we'll eventually
> trigger this from a client so we should eventually report failure
> rather than quitting.

Right, and in the same way I'm planning to change all the die()s to
return errors in a later patch.

> 
> > +	}
> > +
> >  	new = &fwd->rules[fwd->count++];
> >  	new->flags = flags;
> >  
> > diff --git a/inany.c b/inany.c
> > index 87a4d8b6..a8c44237 100644
> > --- a/inany.c
> > +++ b/inany.c
> > @@ -21,6 +21,25 @@
> >  const union inany_addr inany_loopback4 = INANY_INIT4(IN4ADDR_LOOPBACK_INIT);
> >  const union inany_addr inany_any4 = INANY_INIT4(IN4ADDR_ANY_INIT);
> >  
> > +/** inany_matches - Do two addresses match
> 
> Nit: "Do [...] match?"

Fixed.

> > + * @a, @b:	IPv[46] addresses (NULL for 0.0.0.0 & ::)
> > + *
> > + * Return: true if they match, false otherwise
> > + *
> > + * Addresses match themselves, but also with unspecified addresses of the same
> > + * family.
> > + */
> > +bool inany_matches(const union inany_addr *a, const union inany_addr *b)
> > +{
> > +	if (!a || !b)
> > +		return true;
> > +
> > +	if (inany_is_unspecified(a) || inany_is_unspecified(b))
> > +		return !!inany_v4(a) == !!inany_v4(b);
> > +
> > +	return inany_equals(a, b);
> > +}
> > +
> >  /** inany_ntop - Convert an IPv[46] address to text format
> >   * @src:	IPv[46] address (NULL for unspecified)
> >   * @dst:	output buffer, minimum INANY_ADDRSTRLEN bytes
> > diff --git a/inany.h b/inany.h
> > index 61b36fb4..b02c2891 100644
> > --- a/inany.h
> > +++ b/inany.h
> > @@ -293,6 +293,7 @@ static inline void inany_siphash_feed(struct siphash_state *state,
> >  
> >  #define INANY_ADDRSTRLEN	MAX(INET_ADDRSTRLEN, INET6_ADDRSTRLEN)
> >  
> > +bool inany_matches(const union inany_addr *a, const union inany_addr *b);
> >  const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size);
> >  int inany_pton(const char *src, union inany_addr *dst);
> >  
> 
> I reviewed up to 10/14 only (I have no comments on that one), I still
> need a bit of time for the remaining four patches.
> 
> -- 
> Stefano
> 

-- 
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 --]