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

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

[Stefano Brivio]

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.

Reported-by: Mike Dillinger <miked@softtalker.com>
Cc: <stable@vger.kernel.org> # 5.6.x
Fixes: 8d8540c4f5e0 ("netfilter: nft_set_rbtree: add timeout support")
Fixes: 7c84d41416d8 ("netfilter: nft_set_rbtree: Detect partial overlaps on insertion")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 net/netfilter/nft_set_rbtree.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nft_set_rbtree.c b/net/netfilter/nft_set_rbtree.c
index 62f416bc0579..b6aad3fc46c3 100644
--- a/net/netfilter/nft_set_rbtree.c
+++ b/net/netfilter/nft_set_rbtree.c
@@ -271,12 +271,14 @@ static int __nft_rbtree_insert(const struct net *net, const struct nft_set *set,
 
 			if (nft_rbtree_interval_start(new)) {
 				if (nft_rbtree_interval_end(rbe) &&
-				    nft_set_elem_active(&rbe->ext, genmask))
+				    nft_set_elem_active(&rbe->ext, genmask) &&
+				    !nft_set_elem_expired(&rbe->ext))
 					overlap = false;
 			} else {
 				overlap = nft_rbtree_interval_end(rbe) &&
 					  nft_set_elem_active(&rbe->ext,
-							      genmask);
+							      genmask) &&
+					  !nft_set_elem_expired(&rbe->ext);
 			}
 		} else if (d > 0) {
 			p = &parent->rb_right;
@@ -284,9 +286,11 @@ static int __nft_rbtree_insert(const struct net *net, const struct nft_set *set,
 			if (nft_rbtree_interval_end(new)) {
 				overlap = nft_rbtree_interval_end(rbe) &&
 					  nft_set_elem_active(&rbe->ext,
-							      genmask);
+							      genmask) &&
+					  !nft_set_elem_expired(&rbe->ext);
 			} else if (nft_rbtree_interval_end(rbe) &&
-				   nft_set_elem_active(&rbe->ext, genmask)) {
+				   nft_set_elem_active(&rbe->ext, genmask) &&
+				   !nft_set_elem_expired(&rbe->ext)) {
 				overlap = true;
 			}
 		} else {
@@ -294,15 +298,18 @@ static int __nft_rbtree_insert(const struct net *net, const struct nft_set *set,
 			    nft_rbtree_interval_start(new)) {
 				p = &parent->rb_left;
 
-				if (nft_set_elem_active(&rbe->ext, genmask))
+				if (nft_set_elem_active(&rbe->ext, genmask) &&
+				    !nft_set_elem_expired(&rbe->ext))
 					overlap = false;
 			} else if (nft_rbtree_interval_start(rbe) &&
 				   nft_rbtree_interval_end(new)) {
 				p = &parent->rb_right;
 
-				if (nft_set_elem_active(&rbe->ext, genmask))
+				if (nft_set_elem_active(&rbe->ext, genmask) &&
+				    !nft_set_elem_expired(&rbe->ext))
 					overlap = false;
-			} else if (nft_set_elem_active(&rbe->ext, genmask)) {
+			} else if (nft_set_elem_active(&rbe->ext, genmask) &&
+				   !nft_set_elem_expired(&rbe->ext)) {
 				*ext = &rbe->ext;
 				return -EEXIST;
 			} else {
-- 
2.26.2

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

[Phil Sutter]

Hi,

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.

Yes, it seems I missed insert path entirely when adding
nft_set_elem_expired() checks. Assuming that it is fine that expired
elements block insertions until gc-interval has passed, I missed the
chance for one end of an interval to be accepted while the other is not.

Thanks for clearing up my mess!

[...]

> Reported-by: Mike Dillinger <miked@softtalker.com>
> Cc: <stable@vger.kernel.org> # 5.6.x
> Fixes: 8d8540c4f5e0 ("netfilter: nft_set_rbtree: add timeout support")
> Fixes: 7c84d41416d8 ("netfilter: nft_set_rbtree: Detect partial overlaps on insertion")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Acked-by: Phil Sutter <phil@nwl.cc>

Cheers, Phil

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

[Mike Dillinger]

> Signed-off-by: Stefano Brivio<sbrivio@redhat.com>
> ---
>   .../testcases/sets/0044interval_overlap_0     | 81 ++++++++++++++-----
>   1 file changed, 61 insertions(+), 20 deletions(-)
>
> diff --git a/tests/shell/testcases/sets/0044interval_overlap_0 b/tests/shell/testcases/sets/0044interval_overlap_0
> index fad92ddcf356..16f661a00116 100755
> --- a/tests/shell/testcases/sets/0044interval_overlap_0
> +++ b/tests/shell/testcases/sets/0044interval_overlap_0
> @@ -7,6 +7,13 @@
>   #   existing one
>   # - for concatenated ranges, the new element is less specific than any existing
>   #   overlapping element, as elements are evaluated in order of insertion
> +#
> +# Then, repeat the test with a set configured for 1s timeout, checking that:
> +# - we can insert all the elements as described above
> +# - once the timeout has expired, we can insert all the elements again, and old
> +#   elements are not present
> +# - before the timeout expires again, we can re-add elements that are not
> +#   expected to fail, but old elements might be present
>   
>   #	Accept	Interval	List
>   intervals_simple="
> @@ -39,28 +46,62 @@ intervals_concat="
>   	y	15-20 . 49-61	0-2 . 0-3, 10-20 . 30-40, 15-20 . 50-60, 3-9 . 4-29, 15-20 . 49-61
>   "
>   
> -$NFT add table t
> -$NFT add set t s '{ type inet_service ; flags interval ; }'
> -$NFT add set t c '{ type inet_service . inet_service ; flags interval ; }'
> +match_elements() {
> +	skip=0
> +	n=0
> +	out=
> +	for a in $($NFT list set t ${1})}; do
> +		[ ${n} -eq 0 ] && [ "${a}" = "elements" ] && n=1
> +		[ ${n} -eq 1 ] && [ "${a}" = "=" ]	  && n=2
> +		[ ${n} -eq 2 ] && [ "${a}" = "{" ]	  && n=3 && continue
> +		[ ${n} -lt 3 ] 					 && continue
> +
> +		[ "${a}" = "}" ]				 && break
> +
> +		[ ${skip} -eq 1 ] && skip=0 && out="${out},"	 && continue
> +		[ "${a}" = "expires" ] && skip=1		 && continue
> +
> +		[ -n "${out}" ] && out="${out} ${a}" || out="${a}"
> +	done
> +	[ "${out%,}" = "${2}" ]
> +}
>   
> -IFS='	
> +add_elements() {
> +	set="s"
> +	IFS='	
>   '
> -set="s"
> -for t in ${intervals_simple} switch ${intervals_concat}; do
> -	[ "${t}" = "switch" ] && set="c"         && continue
> -	[ -z "${pass}" ]      && pass="${t}"     && continue
> -	[ -z "${interval}" ]  && interval="${t}" && continue
> +	for t in ${intervals_simple} switch ${intervals_concat}; do
> +		unset IFS
> +		[ "${t}" = "switch" ] && set="c"         && continue
> +		[ -z "${pass}" ]      && pass="${t}"     && continue
> +		[ -z "${interval}" ]  && interval="${t}" && continue
>   
> -	if [ "${pass}" = "y" ]; then
> -		$NFT add element t ${set} "{ ${interval} }"
> -	else
> -		! $NFT add element t ${set} "{ ${interval} }" 2>/dev/null
> -	fi
> -	$NFT list set t ${set} | tr -d '\n\t' | tr -s ' ' | \
> -		grep -q "elements = { ${t} }"
> +		if [ "${pass}" = "y" ]; then
> +			$NFT add element t ${set} "{ ${interval} }"
> +		else
> +			! $NFT add element t ${set} "{ ${interval} }" 2>/dev/null
> +		fi
>   
> -	pass=
> -	interval=
> -done
> +		[ "${1}" != "nomatch" ] && match_elements "${set}" "${t}"
>   
> -unset IFS
> +		pass=
> +		interval=
> +		IFS='	
> +'
> +	done
> +	unset IFS
> +}
> +
> +$NFT add table t
> +$NFT add set t s '{ type inet_service ; flags interval ; }'
> +$NFT add set t c '{ type inet_service . inet_service ; flags interval ; }'
> +add_elements
> +
> +$NFT flush ruleset
> +$NFT add table t
> +$NFT add set t s '{ type inet_service ; flags interval,timeout; timeout 1s; gc-interval 1s; }'
> +$NFT add set t c '{ type inet_service . inet_service ; flags interval,timeout ; timeout 1s; gc-interval 1s; }'
> +add_elements
> +sleep 1
> +add_elements
> +add_elements nomatch

Hello All,

Is there any way I can track this change so I know what kernel version to expect it in?  Pardon my ignorance, but I'm new to Linux kernel changes.  I have familiarity with change requests, so if I can follow this on GitHub or some other tracking system, that would be great.

Thanks!
-MikeD

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

[Stefano Brivio]

Mike,

On Thu, 4 Jun 2020 11:16:09 -0700
Mike Dillinger <miked@softtalker.com> wrote:

>  [...]  
> 
> Hello All,
> 
> Is there any way I can track this change so I know what kernel
> version to expect it in?  Pardon my ignorance, but I'm new to Linux
> kernel changes.  I have familiarity with change requests, so if I
> can follow this on GitHub or some other tracking system, that would
> be great.

Pablo, the maintainer, will answer to the original patch email once
(and if) it gets applied to the netfilter (nf.git) tree. For that part,
you can also track it here:
	http://patchwork.ozlabs.org/project/netfilter-devel/list/

That should be applied to the main tree, though, before the stable team
picks it, there are several ways to track that... but you're using
Debian kernels, so I guess you're rather interested in:
	https://tracker.debian.org/pkg/linux
	https://tracker.debian.org/pkg/linux/rss

Changelog links are available from there too.

-- 
Stefano

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

[Pablo Neira Ayuso]

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.