bgpd: fix rpki revalidation for invalid announcements

Announcements that are marked as invalid were previously not revalidated.
This was fixed by replacing the range lookup with a subtree lookup.

Signed-off-by: Marcel Röthke <marcel.roethke@haw-hamburg.de>

diff --git a/bgpd/bgp_rpki.c b/bgpd/bgp_rpki.c
index be1df08ad2e1d97d7b1448218aaf132bf49c37e3..3228b38990b23204377c8152e7df2e9df73e998f 100644 (file)
--- a/bgpd/bgp_rpki.c
+++ b/bgpd/bgp_rpki.c
@@ -402,25 +402,23 @@ static int bgpd_sync_callback(struct thread *thread)
                                if (!peer->bgp->rib[afi][safi])
                                        continue;
 
-                               struct list *matches = list_new();
-
-                               matches->del =
-                                       (void (*)(void *))bgp_unlock_node;
-
-                               bgp_table_range_lookup(
-                                       peer->bgp->rib[afi][safi], prefix,
-                                       rec.max_len, matches);
-
-
-                               struct bgp_node *bgp_node;
-                               struct listnode *bgp_listnode;
-
-                               for (ALL_LIST_ELEMENTS_RO(matches, bgp_listnode,
-                                                         bgp_node))
-                                       revalidate_bgp_node(bgp_node, afi,
-                                                           safi);
-
-                               list_delete(&matches);
+                               struct bgp_node *match;
+                               struct bgp_node *node;
+
+                               match = bgp_table_subtree_lookup(
+                                       peer->bgp->rib[afi][safi], prefix);
+                               node = match;
+
+                               while (node) {
+                                       if (bgp_node_has_bgp_path_info_data(
+                                                   node)) {
+                                               revalidate_bgp_node(node, afi,
+                                                                   safi);
+                                       }
+
+                                       node = bgp_route_next_until(node,
+                                                                   match);
+                               }
                        }
                }
        }

diff --git a/bgpd/bgp_table.c b/bgpd/bgp_table.c
index dcf9852a675e648347509446584003446aa4e13c..8acd42ffc034d0b4dda3e762986a2dd89eda9f95 100644 (file)
--- a/bgpd/bgp_table.c
+++ b/bgpd/bgp_table.c
@@ -164,90 +164,42 @@ void bgp_delete_listnode(struct bgp_node *node)
        }
 }
 
-static struct bgp_node *
-bgp_route_next_until_maxlen(struct bgp_node *node, const struct bgp_node *limit,
-                           const uint8_t maxlen)
+struct bgp_node *bgp_table_subtree_lookup(const struct bgp_table *table,
+                                         const struct prefix *p)
 {
-       const struct prefix *p = bgp_node_get_prefix(node);
+       struct bgp_node *node = bgp_node_from_rnode(table->route_table->top);
+       struct bgp_node *matched = NULL;
 
-       if (node->l_left) {
-               const struct prefix *left_p =
-                       bgp_node_get_prefix(bgp_node_from_rnode(node->l_left));
+       if (node == NULL)
+               return NULL;
 
-               if (p->prefixlen < maxlen && left_p->prefixlen <= maxlen)
-                       return bgp_node_from_rnode(node->l_left);
-       }
 
-       if (node->l_right) {
-               const struct prefix *right_p =
-                       bgp_node_get_prefix(bgp_node_from_rnode(node->l_right));
+       while (node) {
+               const struct prefix *node_p = bgp_node_get_prefix(node);
 
-               if (p->prefixlen < maxlen && right_p->prefixlen <= maxlen)
-                       return bgp_node_from_rnode(node->l_right);
-       }
+               if (node_p->prefixlen >= p->prefixlen) {
+                       if (!prefix_match(p, node_p))
+                               return NULL;
 
-       while (node->parent && node != limit) {
-               if (bgp_node_from_rnode(node->parent->l_left) == node
-                   && node->parent->l_right) {
-                       return bgp_node_from_rnode(node->parent->l_right);
+                       matched = node;
+                       break;
                }
-               node = bgp_node_from_rnode(node->parent);
-       }
-       return NULL;
-}
 
-void bgp_table_range_lookup(const struct bgp_table *table,
-                           const struct prefix *p,
-                           uint8_t maxlen, struct list *matches)
-{
-       struct bgp_node *node = bgp_node_from_rnode(table->route_table->top);
-       struct bgp_node *matched = NULL;
+               if (!prefix_match(node_p, p))
+                       return NULL;
 
-       if (node == NULL)
-               return;
-
-       const struct prefix *node_p = bgp_node_get_prefix(node);
-
-       while (node && node_p->prefixlen <= p->prefixlen
-              && prefix_match(node_p, p)) {
-               if (bgp_node_has_bgp_path_info_data(node)
-                   && node_p->prefixlen == p->prefixlen) {
+               if (node_p->prefixlen == p->prefixlen) {
                        matched = node;
                        break;
                }
+
                node = bgp_node_from_rnode(node->link[prefix_bit(
                        &p->u.prefix, node_p->prefixlen)]);
-               node_p = bgp_node_get_prefix(node);
        }
 
-       if (!node)
-               return;
-
-       node_p = bgp_node_get_prefix(node);
-       if (matched == NULL && node_p->prefixlen <= maxlen
-           && prefix_match(p, node_p) && node->parent == NULL)
-               matched = node;
-       else if ((matched == NULL && node_p->prefixlen > maxlen)
-                || !node->parent)
-               return;
-       else if (matched == NULL && node->parent)
-               matched = node = bgp_node_from_rnode(node->parent);
-
        if (!matched)
-               return;
-
-       if (bgp_node_has_bgp_path_info_data(matched)) {
-               bgp_lock_node(matched);
-               listnode_add(matches, matched);
-       }
+               return NULL;
 
-       while ((node = bgp_route_next_until_maxlen(node, matched, maxlen))) {
-               node_p = bgp_node_get_prefix(node);
-               if (prefix_match(p, node_p)) {
-                       if (bgp_node_has_bgp_path_info_data(node)) {
-                               bgp_lock_node(node);
-                               listnode_add(matches, node);
-                       }
-               }
-       }
+       bgp_lock_node(matched);
+       return matched;
 }

diff --git a/bgpd/bgp_table.h b/bgpd/bgp_table.h
index da2ca3181a7f09abcc64823f7b828774232c9e2e..c35833e66b01f68d04df81f8fa6f19a392443486 100644 (file)
--- a/bgpd/bgp_table.h
+++ b/bgpd/bgp_table.h
@@ -347,9 +347,15 @@ static inline uint64_t bgp_table_version(struct bgp_table *table)
        return table->version;
 }
 
-void bgp_table_range_lookup(const struct bgp_table *table,
-                           const struct prefix *p,
-                           uint8_t maxlen, struct list *matches);
+/* Find the subtree of the prefix p
+ *
+ * This will return the first node that belongs the the subtree of p. Including
+ * p itself, if it is in the tree.
+ *
+ * If the subtree is not present in the table, NULL is returned.
+ */
+struct bgp_node *bgp_table_subtree_lookup(const struct bgp_table *table,
+                                         const struct prefix *p);
 
 
 static inline struct bgp_aggregate *

diff --git a/tests/bgpd/test_bgp_table.c b/tests/bgpd/test_bgp_table.c
index 79a8bb4408a1b0968202cbf4e4f010df75344f22..7d81d42afb42dacf80d2460a3f5698fa5a12ab92 100644 (file)
--- a/tests/bgpd/test_bgp_table.c
+++ b/tests/bgpd/test_bgp_table.c
@@ -73,70 +73,64 @@ static void add_node(struct bgp_table *table, const char *prefix_str)
        rn->info = node;
 }
 
-static void print_range_result(struct list *list)
+static bool prefix_in_array(const struct prefix *p, struct prefix *prefix_array,
+                           size_t prefix_array_size)
 {
-
-       struct listnode *listnode;
-       struct bgp_node *bnode;
-
-       for (ALL_LIST_ELEMENTS_RO(list, listnode, bnode)) {
-               char buf[PREFIX2STR_BUFFER];
-
-               prefix2str(bgp_node_get_prefix(bnode), buf, PREFIX2STR_BUFFER);
-               printf("%s\n", buf);
+       for (size_t i = 0; i < prefix_array_size; ++i) {
+               if (prefix_same(p, &prefix_array[i]))
+                       return true;
        }
+       return false;
 }
 
-static void check_lookup_result(struct list *list, va_list arglist)
+static void check_lookup_result(struct bgp_node *match, va_list arglist)
 {
        char *prefix_str;
-       unsigned int prefix_count = 0;
+       struct prefix *prefixes = NULL;
+       size_t prefix_count = 0;
 
-       printf("Searching results\n");
        while ((prefix_str = va_arg(arglist, char *))) {
-               struct listnode *listnode;
-               struct bgp_node *bnode;
-               struct prefix p;
-               bool found = false;
+               ++prefix_count;
+               prefixes = realloc(prefixes, sizeof(*prefixes) * prefix_count);
 
-               prefix_count++;
-               printf("Searching for %s\n", prefix_str);
-
-               if (str2prefix(prefix_str, &p) <= 0)
+               if (str2prefix(prefix_str, &prefixes[prefix_count - 1]) <= 0)
                        assert(0);
+       }
 
-               for (ALL_LIST_ELEMENTS_RO(list, listnode, bnode)) {
-                       if (prefix_same(bgp_node_get_prefix(bnode), &p))
-                               found = true;
-               }
+       /* check if the result is empty and if it is allowd to be empty */
+       assert((prefix_count == 0 && !match) || prefix_count > 0);
+       if (!match)
+               return;
 
-               assert(found);
-       }
+       struct bgp_node *node = match;
+
+       while ((node = bgp_route_next_until(node, match))) {
+               const struct prefix *node_p = bgp_node_get_prefix(node);
 
-       printf("Checking for unexpected result items\n");
-       printf("Expecting %d found %d\n", prefix_count, listcount(list));
-       assert(prefix_count == listcount(list));
+               if (bgp_node_has_bgp_path_info_data(node)
+                   && !prefix_in_array(node_p, prefixes, prefix_count)) {
+                       char buf[PREFIX2STR_BUFFER];
+
+                       prefix2str(node_p, buf, PREFIX2STR_BUFFER);
+                       printf("prefix %s was not expected!\n", buf);
+                       assert(0);
+               }
+       }
 }
 
-static void do_test(struct bgp_table *table, const char *prefix,
-                   uint32_t maxlen, ...)
+static void do_test(struct bgp_table *table, const char *prefix, ...)
 {
        va_list arglist;
-       struct list *list = list_new();
        struct prefix p;
 
-       list->del = (void (*)(void *))bgp_unlock_node;
 
-       va_start(arglist, maxlen);
-       printf("\nDoing lookup for %s-%d\n", prefix, maxlen);
+       va_start(arglist, prefix);
+       printf("\nDoing lookup for %s\n", prefix);
        if (str2prefix(prefix, &p) <= 0)
                assert(0);
-       bgp_table_range_lookup(table, &p, maxlen, list);
-       print_range_result(list);
-
-       check_lookup_result(list, arglist);
+       struct bgp_node *node = bgp_table_subtree_lookup(table, &p);
 
-       list_delete(&list);
+       check_lookup_result(node, arglist);
 
        va_end(arglist);
 
@@ -163,27 +157,22 @@ static void test_range_lookup(void)
        for (int i = 0; i < num_prefixes; i++)
                add_node(table, prefixes[i]);
 
-       do_test(table, "1.16.0.0/17", 20, "1.16.64.0/19", "1.16.32.0/20", NULL);
-       do_test(table, "1.16.128.0/17", 20, "1.16.128.0/18", "1.16.192.0/18",
-               "1.16.160.0/19", NULL);
-
-       do_test(table, "1.16.128.0/17", 20, "1.16.128.0/18", "1.16.192.0/18",
+       do_test(table, "1.16.0.0/17", "1.16.64.0/19", "1.16.32.0/20",
+               "1.16.32.0/20", "1.16.32.0/21", NULL);
+       do_test(table, "1.16.128.0/17", "1.16.128.0/18", "1.16.192.0/18",
                "1.16.160.0/19", NULL);
 
-       do_test(table, "1.16.0.0/16", 18, "1.16.0.0/16", "1.16.128.0/18",
-               "1.16.192.0/18", NULL);
-
-       do_test(table, "1.16.0.0/16", 21, "1.16.0.0/16", "1.16.128.0/18",
+       do_test(table, "1.16.0.0/16", "1.16.0.0/16", "1.16.128.0/18",
                "1.16.192.0/18", "1.16.64.0/19", "1.16.160.0/19",
                "1.16.32.0/20", "1.16.32.0/21", NULL);
 
-       do_test(table, "1.17.0.0/16", 20, NULL);
+       do_test(table, "1.17.0.0/16", NULL);
 
-       do_test(table, "128.0.0.0/8", 16, NULL);
+       do_test(table, "128.0.0.0/8", NULL);
 
-       do_test(table, "16.0.0.0/8", 16, "16.0.0.0/16", NULL);
+       do_test(table, "16.0.0.0/8", "16.0.0.0/16", NULL);
 
-       do_test(table, "0.0.0.0/2", 21, "1.16.0.0/16", "1.16.128.0/18",
+       do_test(table, "0.0.0.0/2", "1.16.0.0/16", "1.16.128.0/18",
                "1.16.192.0/18", "1.16.64.0/19", "1.16.160.0/19",
                "1.16.32.0/20", "1.16.32.0/21", "16.0.0.0/16", NULL);
 }

diff --git a/tests/bgpd/test_bgp_table.py b/tests/bgpd/test_bgp_table.py
index 4deaf08c22e16dda9890cdc9fce53c65c57d25bd..53bd37233a0718238ea6a28d527b4d7ca7dcda5e 100644 (file)
--- a/tests/bgpd/test_bgp_table.py
+++ b/tests/bgpd/test_bgp_table.py
@@ -3,5 +3,5 @@ import frrtest
 class TestTable(frrtest.TestMultiOut):
     program = './test_bgp_table'
 
-for i in range(9):
+for i in range(7):
     TestTable.onesimple('Checks successfull')