Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

netfilter: nft_set_rbtree: Drop spurious condition for overlap detection on insertion

Case a1. for overlap detection in __nft_rbtree_insert() is not a valid
one: start-after-start is not needed to detect any type of interval
overlap and it actually results in a false positive if, while
descending the tree, this is the only step we hit after starting from
the root.

This introduced a regression, as reported by Pablo, in Python tests
cases ip/ip.t and ip/numgen.t:

ip/ip.t: ERROR: line 124: add rule ip test-ip4 input ip hdrlength vmap { 0-4 : drop, 5 : accept, 6 : continue } counter: This rule should not have failed.
ip/numgen.t: ERROR: line 7: add rule ip test-ip4 pre dnat to numgen inc mod 10 map { 0-5 : 192.168.10.100, 6-9 : 192.168.20.200}: This rule should not have failed.

Drop case a1. and renumber others, so that they are a bit clearer. In
order for these diagrams to be readily understandable, a bigger rework
is probably needed, such as an ASCII art of the actual rbtree (instead
of a flattened version).

Shell script test sets/0044interval_overlap_0 should cover all
possible cases for false negatives, so I consider that test case still
sufficient after this change.

v2: Fix comments for cases a3. and b3.

Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
Fixes: 7c84d41416d8 ("netfilter: nft_set_rbtree: Detect partial overlaps on insertion")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

authored by

Stefano Brivio and committed by
Pablo Neira Ayuso
72239f27 0452800f

+11 -12
+11 -12
net/netfilter/nft_set_rbtree.c
··· 218 218 219 219 /* Detect overlaps as we descend the tree. Set the flag in these cases: 220 220 * 221 - * a1. |__ _ _? >|__ _ _ (insert start after existing start) 222 - * a2. _ _ __>| ?_ _ __| (insert end before existing end) 223 - * a3. _ _ ___| ?_ _ _>| (insert end after existing end) 224 - * a4. >|__ _ _ _ _ __| (insert start before existing end) 221 + * a1. _ _ __>| ?_ _ __| (insert end before existing end) 222 + * a2. _ _ ___| ?_ _ _>| (insert end after existing end) 223 + * a3. _ _ ___? >|_ _ __| (insert start before existing end) 225 224 * 226 225 * and clear it later on, as we eventually reach the points indicated by 227 226 * '?' above, in the cases described below. We'll always meet these 228 227 * later, locally, due to tree ordering, and overlaps for the intervals 229 228 * that are the closest together are always evaluated last. 230 229 * 231 - * b1. |__ _ _! >|__ _ _ (insert start after existing end) 232 - * b2. _ _ __>| !_ _ __| (insert end before existing start) 233 - * b3. !_____>| (insert end after existing start) 230 + * b1. _ _ __>| !_ _ __| (insert end before existing start) 231 + * b2. _ _ ___| !_ _ _>| (insert end after existing start) 232 + * b3. _ _ ___! >|_ _ __| (insert start after existing end) 234 233 * 235 - * Case a4. resolves to b1.: 234 + * Case a3. resolves to b3.: 236 235 * - if the inserted start element is the leftmost, because the '0' 237 236 * element in the tree serves as end element 238 237 * - otherwise, if an existing end is found. Note that end elements are 239 238 * always inserted after corresponding start elements. 240 239 * 241 - * For a new, rightmost pair of elements, we'll hit cases b1. and b3., 240 + * For a new, rightmost pair of elements, we'll hit cases b3. and b2., 242 241 * in that order. 243 242 * 244 243 * The flag is also cleared in two special cases: ··· 261 262 p = &parent->rb_left; 262 263 263 264 if (nft_rbtree_interval_start(new)) { 264 - overlap = nft_rbtree_interval_start(rbe) && 265 - nft_set_elem_active(&rbe->ext, 266 - genmask); 265 + if (nft_rbtree_interval_end(rbe) && 266 + nft_set_elem_active(&rbe->ext, genmask)) 267 + overlap = false; 267 268 } else { 268 269 overlap = nft_rbtree_interval_end(rbe) && 269 270 nft_set_elem_active(&rbe->ext,