From 33b10b9112f2f51df049315438204efec7a5434c Mon Sep 17 00:00:00 2001 From: Eric Garver Date: Tue, 25 Jan 2022 09:29:32 -0500 Subject: [PATCH 51/51] fix(ipset): further reduce cost of entry overlap detection This makes the complexity linear by sorting the networks ahead of time. Fixes: #881 Fixes: rhbz2043289 (cherry picked from commit 36c170db265265e838a089858be4b20dbbd582eb) --- src/firewall/core/ipset.py | 59 ++++++++++++++++++++++++++++++++--- src/tests/regression/gh881.at | 42 ++++++++++++++++++++++--- 2 files changed, 92 insertions(+), 9 deletions(-) diff --git a/src/firewall/core/ipset.py b/src/firewall/core/ipset.py index 66ea4335536d..b160d8345669 100644 --- a/src/firewall/core/ipset.py +++ b/src/firewall/core/ipset.py @@ -327,8 +327,57 @@ def check_for_overlapping_entries(entries): # at least one entry can not be parsed return - while entries: - entry = entries.pop() - for itr in entries: - if entry.overlaps(itr): - raise FirewallError(errors.INVALID_ENTRY, "Entry '{}' overlaps entry '{}'".format(entry, itr)) + # We can take advantage of some facts of IPv4Network/IPv6Network and + # how Python sorts the networks to quickly detect overlaps. + # + # Facts: + # + # 1. IPv{4,6}Network are normalized to remove host bits, e.g. + # 10.1.1.0/16 will become 10.1.0.0/16. + # + # 2. IPv{4,6}Network objects are sorted by: + # a. IP address (network bits) + # then + # b. netmask (significant bits count) + # + # Because of the above we have these properties: + # + # 1. big networks (netA) are sorted before smaller networks (netB) + # that overlap the big network (netA) + # - e.g. 10.1.128.0/17 (netA) sorts before 10.1.129.0/24 (netB) + # 2. same value addresses (network bits) are grouped together even + # if the number of network bits vary. e.g. /16 vs /24 + # - recall that address are normalized to remove host bits + # - e.g. 10.1.128.0/17 (netA) sorts before 10.1.128.0/24 (netC) + # 3. non-overlapping networks (netD, netE) are always sorted before or + # after networks that overlap (netB, netC) the current one (netA) + # - e.g. 10.1.128.0/17 (netA) sorts before 10.2.128.0/16 (netD) + # - e.g. 10.1.128.0/17 (netA) sorts after 9.1.128.0/17 (netE) + # - e.g. 9.1.128.0/17 (netE) sorts before 10.1.129.0/24 (netB) + # + # With this we know the sorted list looks like: + # + # list: [ netE, netA, netB, netC, netD ] + # + # netE = non-overlapping network + # netA = big network + # netB = smaller network that overlaps netA (subnet) + # netC = smaller network that overlaps netA (subnet) + # netD = non-overlapping network + # + # If networks netB and netC exist in the list, they overlap and are + # adjacent to netA. + # + # Checking for overlaps on a sorted list is thus: + # + # 1. compare adjacent elements in the list for overlaps + # + # Recall that we only need to detect a single overlap. We do not need to + # detect them all. + # + entries.sort() + prev_network = entries.pop(0) + for current_network in entries: + if prev_network.overlaps(current_network): + raise FirewallError(errors.INVALID_ENTRY, "Entry '{}' overlaps entry '{}'".format(prev_network, current_network)) + prev_network = current_network diff --git a/src/tests/regression/gh881.at b/src/tests/regression/gh881.at index c7326805b555..a5cf7e4eb912 100644 --- a/src/tests/regression/gh881.at +++ b/src/tests/regression/gh881.at @@ -5,21 +5,55 @@ dnl build a large ipset dnl AT_DATA([./deny_cidr], []) NS_CHECK([sh -c ' -for I in $(seq 10); do +for I in $(seq 250); do for J in $(seq 250); do echo "10.${I}.${J}.0/24" >> ./deny_cidr done done ']) +NS_CHECK([echo "10.254.0.0/16" >> ./deny_cidr]) dnl verify non-overlapping does not error dnl FWD_CHECK([--permanent --new-ipset=deny_set --type=hash:net --option=family=inet --option=hashsize=16384 --option=maxelem=20000], 0, [ignore]) -NS_CHECK([time timeout 300 firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 0, [ignore], [ignore]) +NS_CHECK([time firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 0, [ignore], [ignore]) + +dnl still no overlap +dnl +AT_DATA([./deny_cidr], [ +9.0.0.0/8 +11.1.0.0/16 +]) +NS_CHECK([time firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 0, [ignore], [ignore]) dnl verify overlap detection actually detects an overlap dnl -NS_CHECK([echo "10.1.0.0/16" >> ./deny_cidr]) -NS_CHECK([time timeout 300 firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 136, [ignore], [ignore]) +AT_DATA([./deny_cidr], [ +10.1.0.0/16 +10.2.0.0/16 +10.250.0.0/16 +]) +NS_CHECK([time firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 136, [ignore], [ignore]) + +AT_DATA([./deny_cidr], [ +10.253.0.0/16 +10.253.128.0/17 +]) +NS_CHECK([time firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 136, [ignore], [ignore]) + +AT_DATA([./deny_cidr], [ +10.1.1.1/32 +]) +NS_CHECK([time firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 136, [ignore], [ignore]) + +AT_DATA([./deny_cidr], [ +10.0.0.0/8 +10.0.0.0/25 +]) +NS_CHECK([time firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 136, [ignore], [ignore]) + +dnl empty file, no additions, but previous ones will remain +AT_DATA([./deny_cidr], []) +FWD_CHECK([--permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 0, [ignore], [ignore]) FWD_END_TEST() -- 2.31.1