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

[Stefano Brivio <sbrivio@redhat.com>]

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.

> +	}
> +
>  	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?"

> + * @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