Re: [PATCH v3 10/11] fwd_rule: Move conflict checking back within fwd_rule_add()

[Laurent Vivier <lvivier@redhat.com>]

On 4/17/26 07:05, David Gibson wrote:
> 2bffb631d31e ("fwd_rule: Move rule conflict checking from fwd_rule_add()
> to caller") moved rule conflict checking out of fwd_rule_add().  This
> seemed like a good idea at the time, but turns out to be kind of awkward:
> it means we're now checking for conflicts *before* we've checked the rule
> for internal consistency (including first <= last), which leaves an awkward
> assert() which might fire in unexpected places.
> 
> While it's true that it's not really necessary to include this in order to
> safely add a rule, the benefits from skipping it are pretty marginal.  So,
> for simplicity, fold this check back into fwd_rule_add(), making it
> non-fatal.  If we ever have cases with enough rules that the O(n^2) nature
> of the check matters, we might need to revisit.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>   fwd_rule.c | 38 +++++++++++++-------------------------
>   1 file changed, 13 insertions(+), 25 deletions(-)
> 
> diff --git a/fwd_rule.c b/fwd_rule.c
> index 7bba2602..cb462401 100644
> --- a/fwd_rule.c
> +++ b/fwd_rule.c
> @@ -195,29 +195,6 @@ static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fwd_rule *
>   	return true;
>   }
>   
> -/**
> - * fwd_rule_conflict_check() - Die if given rule conflicts with any in list
> - * @new:	New rule
> - * @rules:	Existing rules against which to test
> - * @count:	Number of rules in @rules
> - */
> -static 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)));
> -	}
> -}
> -
>   /**
>    * fwd_rule_add() - Validate and add a rule to a forwarding table
>    * @fwd:	Table to add to
> @@ -230,7 +207,7 @@ static int 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 port;
> +	unsigned port, i;
>   
>   	if (new->first > new->last) {
>   		warn("Rule has invalid port range %u-%u",
> @@ -292,6 +269,18 @@ static int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
>   		return -EINVAL;
>   	}
>   
> +	for (i = 0; i < fwd->count; i++) {
> +		char newstr[FWD_RULE_STRLEN], rulestr[FWD_RULE_STRLEN];
> +
> +		if (!fwd_rule_conflicts(new, &fwd->rules[i]))
> +			continue;
> +
> +		warn("Forwarding configuration conflict: %s versus %s",
> +		     fwd_rule_fmt(new, newstr, sizeof(newstr)),
> +		     fwd_rule_fmt(&fwd->rules[i], rulestr, sizeof(rulestr)));
> +		return -EEXIST;
> +	}
> +
>   	if (fwd->count >= ARRAY_SIZE(fwd->rules)) {
>   		warn("Too many rules (maximum %u)", ARRAY_SIZE(fwd->rules));
>   		return -ENOSPC;
> @@ -436,7 +425,6 @@ static void fwd_rule_range_except(struct fwd_table *fwd, uint8_t proto,
>   		rule.last = i - 1;
>   		rule.to = base + delta;
>   
> -		fwd_rule_conflict_check(&rule, fwd->rules, fwd->count);
>   		if (fwd_rule_add(fwd, &rule) < 0)
>   			goto fail;
>