Re: [PATCH nft v4 3/4] src: Add support for concatenated set ranges

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

Applied, thanks. See comments below though.

On Thu, Jan 30, 2020 at 01:16:57AM +0100, Stefano Brivio wrote:
> After exporting field lengths via NFTNL_SET_DESC_CONCAT attributes,
> we now need to adjust parsing of user input and generation of
> netlink key data to complete support for concatenation of set
> ranges.
> 
> Instead of using separate elements for start and end of a range,
> denoting the end element by the NFT_SET_ELEM_INTERVAL_END flag,
> as it's currently done for ranges without concatenation, we'll use
> the new attribute NFTNL_SET_ELEM_KEY_END as suggested by Pablo. It
> behaves in the same way as NFTNL_SET_ELEM_KEY, but it indicates
> that the included key represents the upper bound of a range.
> 
> For example, "packets with an IPv4 address between 192.0.2.0 and
> 192.0.2.42, with destination port between 22 and 25", needs to be
> expressed as a single element with two keys:
> 
>   NFTA_SET_ELEM_KEY:		192.0.2.0 . 22
>   NFTA_SET_ELEM_KEY_END:	192.0.2.42 . 25
> 
> To achieve this, we need to:
> 
> - adjust the lexer rules to allow multiton expressions as elements
>   of a concatenation. As wildcards are not allowed (semantics would
>   be ambiguous), exclude wildcards expressions from the set of
>   possible multiton expressions, and allow them directly where
>   needed. Concatenations now admit prefixes and ranges
> 
> - generate, for each element in a range concatenation, a second key
>   attribute, that includes the upper bound for the range
> 
> - also expand prefixes and non-ranged values in the concatenation
>   to ranges: given a set with interval and concatenation support,
>   the kernel has no way to tell which elements are ranged, so they
>   all need to be. For example, 192.0.2.0 . 192.0.2.9 : 1024 is
>   sent as:
> 
>   NFTA_SET_ELEM_KEY:		192.0.2.0 . 1024
>   NFTA_SET_ELEM_KEY_END:	192.0.2.9 . 1024
> 
> - aggregate ranges when elements received by the kernel represent
>   concatenated ranges, see concat_range_aggregate()

I think concat_range_aggregate() can be remove.

NFTA_SET_ELEM_KEY and the NFTA_SET_ELEM_KEY_END are now coming in the
same element. From the set element delinearization path this could
just build the range, correct?

[...]
> diff --git a/include/rule.h b/include/rule.h
> index a7f106f715cf..c232221e541b 100644
> --- a/include/rule.h
> +++ b/include/rule.h
> @@ -372,6 +372,11 @@ static inline bool set_is_interval(uint32_t set_flags)
>  	return set_flags & NFT_SET_INTERVAL;
>  }
>  
> +static inline bool set_is_non_concat_range(struct set *s)
> +{
> +	return (s->flags & NFT_SET_INTERVAL) && s->desc.field_count <= 1;
> +}

I might make a second pass to revisit this new helper.

Probably, we can pass struct set to all set_is_*() helpers instead,
and use set_is_interval() for the legacy interval representation
that is using the segtree infrastructure.