On Fri, 7 Feb 2020 11:33:06 +0100
Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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
