Re: [PATCH nft] evaluate: prevent merge of sets with incompatible keys

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Thu, Jun 26, 2025 at 02:52:48AM +0200, Florian Westphal wrote:
> Its not enough to check for interval flag, this would assert in interval
> code due to concat being passed to the interval code:
> BUG: unhandled key type 13
> 
> After fix:
> same_set_name_but_different_keys_assert:8:6-7: Error: set already exists with
> different datatype (concatenation of (IPv4 address, network interface index) vs
> network interface index)
>         set s4 {
>             ^^
> 
> This also improves error verbosity when mixing datamap and objref maps:
> 
> invalid_transcation_merge_map_and_objref_map:9:13-13:
> Error: map already exists with different datatype (IPv4 address vs string)
> 
> .. instead of 'Cannot merge map with incompatible existing map of same name'.
> The 'Cannot merge map with incompatible existing map of same name' check
> is kept in place to catch when ruleset contains a set and map with same name
> and same key definition.

LGTM, thanks.

> Signed-off-by: Florian Westphal <fw@strlen.de>

Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

> ---
>  Followup to previous
>  '[nft,v2] evaluate: check that set type is identical before merging',
>   it has the welcome side effect to improve error reporting as well.
> 
>  src/evaluate.c                                      | 12 ++++++++++++
>  src/intervals.c                                     |  2 +-
>  .../nft-f/same_set_name_but_different_keys_assert   | 13 +++++++++++++
>  3 files changed, 26 insertions(+), 1 deletion(-)
>  create mode 100644 tests/shell/testcases/bogons/nft-f/same_set_name_but_different_keys_assert
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index fc9d82f73b68..a2d5d7c29514 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -5304,6 +5304,18 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
>  		if (set_is_interval(set->flags) && !set_is_interval(existing_set->flags))
>  			return set_error(ctx, set,
>  					 "existing %s lacks interval flag", type);
> +		if (set->data && existing_set->data &&
> +		    !datatype_equal(existing_set->data->dtype, set->data->dtype))
> +			return set_error(ctx, set,
> +					 "%s already exists with different datatype (%s vs %s)",
> +					 type, existing_set->data->dtype->desc,
> +					 set->data->dtype->desc);
> +		if (!datatype_equal(existing_set->key->dtype, set->key->dtype))
> +			return set_error(ctx, set,
> +					 "%s already exists with different datatype (%s vs %s)",
> +					 type, existing_set->key->dtype->desc,
> +					 set->key->dtype->desc);
> +		/* Catch attempt to merge set and map */
>  		if (!set_type_compatible(set, existing_set))
>  			return set_error(ctx, set, "Cannot merge %s with incompatible existing %s of same name",
>  					type,
> diff --git a/src/intervals.c b/src/intervals.c
> index bf125a0c59d3..e5bbb0384964 100644
> --- a/src/intervals.c
> +++ b/src/intervals.c
> @@ -70,7 +70,7 @@ static void setelem_expr_to_range(struct expr *expr)
>  		expr->key = key;
>  		break;
>  	default:
> -		BUG("unhandled key type %d\n", expr->key->etype);
> +		BUG("unhandled key type %s\n", expr_name(expr->key));
>  	}
>  }
>  
> diff --git a/tests/shell/testcases/bogons/nft-f/same_set_name_but_different_keys_assert b/tests/shell/testcases/bogons/nft-f/same_set_name_but_different_keys_assert
> new file mode 100644
> index 000000000000..8fcfdf5cba03
> --- /dev/null
> +++ b/tests/shell/testcases/bogons/nft-f/same_set_name_but_different_keys_assert
> @@ -0,0 +1,13 @@
> +table ip t {
> +	set s4 {
> +		type ipv4_addr . iface_index
> +		flags interval
> +		elements = { 127.0.0.1 . "lo" }
> +	}
> +
> +	set s4 {
> +		type iface_index
> +		flags interval
> +		elements = { "lo" }
> +	}
> +}
> -- 
> 2.49.0
> 
>