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

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 7 Feb 2020 11:33:06 +0100
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

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

Correct, with two caveats:

- building ranges isn't that straightforward. Some complexity currently
  in concat_range_aggregate() would go away if we embed that logic in  
  netlink_delinearize_setelem(), but most of it would remain, and that
  logic doesn't seem to belong to "netlink" functions. I guess this is
  quite subjective though

- if we keep a mechanism that can build ranges this way, the day we
  want to switch to NFTA_SET_ELEM_KEY_END for ranges in general, also
  for other set types (or without concatenation anyway), maintaining
  compatibility with older kernels, it should be easier to let
  concat_range_aggregate() handle all cases. I'm not sure, I haven't
  really thought it through

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

Ah, yes, I also think that would make sense.

By the way, while I didn't switch other helpers to take 'struct set' in
this series (because it didn't fit the scope), I'm quite convinced that
functions called set_is_*() should really take a 'set' as argument. :)

-- 
Stefano