| Message ID | 924e80c7b563cc6522a241b123c955c18983edb1.1591141588.git.sbrivio@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf] nft_set_rbtree: Don't account for expired elements on insertion | expand |
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
> 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
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.
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.
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 {
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(-)