Re: [PATCH nf] nft_set_rbtree: Don't account for expired elements on insertion

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

On Wed, Jun 03, 2020 at 01:50:11AM +0200, Stefano Brivio wrote:
> While checking the validity of insertion in __nft_rbtree_insert(),
> we currently ignore conflicting elements and intervals only if they
> are not active within the next generation.
> 
> However, if we consider expired elements and intervals as
> potentially conflicting and overlapping, we'll return error for
> entries that should be added instead. This is particularly visible
> with garbage collection intervals that are comparable with the
> element timeout itself, as reported by Mike Dillinger.
> 
> Other than the simple issue of denying insertion of valid entries,
> this might also result in insertion of a single element (opening or
> closing) out of a given interval. With single entries (that are
> inserted as intervals of size 1), this leads in turn to the creation
> of new intervals. For example:
> 
>   # nft add element t s { 192.0.2.1 }
>   # nft list ruleset
>   [...]
>      elements = { 192.0.2.1-255.255.255.255 }
> 
> Always ignore expired elements active in the next generation, while
> checking for conflicts.
> 
> It might be more convenient to introduce a new macro that covers
> both inactive and expired items, as this type of check also appears
> quite frequently in other set back-ends. This is however beyond the
> scope of this fix and can be deferred to a separate patch.
> 
> Other than the overlap detection cases introduced by commit
> 7c84d41416d8 ("netfilter: nft_set_rbtree: Detect partial overlaps
> on insertion"), we also have to cover the original conflict check
> dealing with conflicts between two intervals of size 1, which was
> introduced before support for timeout was introduced. This won't
> return an error to the user as -EEXIST is masked by nft if
> NLM_F_EXCL is not given, but would result in a silent failure
> adding the entry.

Applied, thanks.