segtree: don't remove nul-root element from interval set

Check from the delinearize set element path if the nul-root element
already exists in the interval set. Hence, the element insertion path
skips the implicit nul-root interval insertion.

Under some circunstances, nft bogusly fails to delete the last element
of the interval set and to create an element in an existing empty
internal set. This patch includes a test that reproduces the issue.

Fixes: 4935a0d561b5 ("segtree: special handling for the first non-matching segment")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/include/netlink.h b/include/netlink.h
index e6941714d5b920f879951cbbde6d188185787dc0..53a55b61e4de6be6ff4d6dabf4c171a14c807df0 100644 (file)
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -133,7 +133,7 @@ extern int netlink_get_setelem(struct netlink_ctx *ctx, const struct handle *h,
                               const struct location *loc, struct table *table,
                               struct set *set, struct expr *init);
 extern int netlink_delinearize_setelem(struct nftnl_set_elem *nlse,
-                                      const struct set *set,
+                                      struct set *set,
                                       struct nft_cache *cache);
 
 extern int netlink_list_objs(struct netlink_ctx *ctx, const struct handle *h);

diff --git a/include/rule.h b/include/rule.h
index 0b2eba37934b1ddfcf05403b33600390323fbbd6..dadeb4b941dad81d736f7bfc46d377468bb61a61 100644 (file)
--- a/include/rule.h
+++ b/include/rule.h
@@ -307,6 +307,7 @@ struct set {
        struct expr             *init;
        struct expr             *rg_cache;
        uint32_t                policy;
+       bool                    root;
        bool                    automerge;
        struct {
                uint32_t        size;

diff --git a/src/netlink.c b/src/netlink.c
index 486e124737261abcbaa2ba1dcc257c956c552a2f..9fc0b17194a07ee8e78f99ab5c82592a668ebffa 100644 (file)
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -790,7 +790,7 @@ static void set_elem_parse_udata(struct nftnl_set_elem *nlse,
 }
 
 int netlink_delinearize_setelem(struct nftnl_set_elem *nlse,
-                               const struct set *set, struct nft_cache *cache)
+                               struct set *set, struct nft_cache *cache)
 {
        struct nft_data_delinearize nld;
        struct expr *expr, *key, *data;
@@ -828,8 +828,11 @@ int netlink_delinearize_setelem(struct nftnl_set_elem *nlse,
                nle = nftnl_set_elem_get(nlse, NFTNL_SET_ELEM_EXPR, NULL);
                expr->stmt = netlink_parse_set_expr(set, cache, nle);
        }
-       if (flags & NFT_SET_ELEM_INTERVAL_END)
+       if (flags & NFT_SET_ELEM_INTERVAL_END) {
                expr->flags |= EXPR_F_INTERVAL_END;
+               if (mpz_cmp_ui(set->key->value, 0) == 0)
+                       set->root = true;
+       }
 
        if (set_is_datamap(set->flags)) {
                if (nftnl_set_elem_is_set(nlse, NFTNL_SET_ELEM_DATA)) {

diff --git a/src/segtree.c b/src/segtree.c
index 7217dbca08e10be5ac3725d6f49c04b1fa704a34..d1dbe10c81a7de566767a4a413342b99188a0353 100644 (file)
--- a/src/segtree.c
+++ b/src/segtree.c
@@ -451,7 +451,7 @@ static int set_to_segtree(struct list_head *msgs, struct set *set,
 static bool segtree_needs_first_segment(const struct set *set,
                                        const struct expr *init, bool add)
 {
-       if (add) {
+       if (add && !set->root) {
                /* Add the first segment in four situations:
                 *
                 * 1) This is an anonymous set.
@@ -465,12 +465,6 @@ static bool segtree_needs_first_segment(const struct set *set,
                    (set->init == init)) {
                        return true;
                }
-       } else {
-               /* If the set is empty after the removal, we have to
-                * remove the first non-matching segment too.
-                */
-               if (set->init && set->init->size - init->size == 0)
-                       return true;
        }
        /* This is an update for a set that already contains elements, so don't
         * add the first non-matching elements otherwise we hit EEXIST.

diff --git a/tests/shell/testcases/sets/0041interval_0 b/tests/shell/testcases/sets/0041interval_0
new file mode 100755 (executable)
index 0000000..42fc6cc
--- /dev/null
+++ b/tests/shell/testcases/sets/0041interval_0
@@ -0,0 +1,25 @@
+#!/bin/bash
+
+set -e
+
+RULESET="
+table ip t {
+        set s {
+                type ipv4_addr
+                flags interval
+                elements = { 192.168.2.195, 192.168.2.196,
+                             192.168.2.197, 192.168.2.198 }
+        }
+}"
+
+$NFT -f - <<< "$RULESET"
+
+$NFT 'delete element t s { 192.168.2.195, 192.168.2.196 }; add element t s { 192.168.2.196 }' 2>/dev/null
+$NFT get element t s { 192.168.2.196, 192.168.2.197, 192.168.2.198 } 1>/dev/null
+$NFT 'delete element t s { 192.168.2.196, 192.168.2.197 }; add element t s { 192.168.2.197 }' 2>/dev/null
+$NFT get element t s { 192.168.2.197, 192.168.2.198 } 1>/dev/null
+$NFT 'delete element t s { 192.168.2.198, 192.168.2.197 }; add element t s { 192.168.2.196, 192.168.2.197, 192.168.2.195 }' 1>/dev/null
+$NFT get element t s { 192.168.2.196, 192.168.2.197, 192.168.2.195 } 1>/dev/null
+$NFT delete element t s { 192.168.2.196, 192.168.2.197, 192.168.2.195 } 2>/dev/null
+$NFT create element t s { 192.168.2.196} 2>/dev/null
+$NFT get element t s { 192.168.2.196 } 1>/dev/null