From: Marcel Röthke Date: Thu, 11 Jun 2020 14:11:09 +0000 (+0200) Subject: bgpd: fix rpki revalidation for invalid announcements X-Git-Tag: base_7.5~269^2 X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=refs%2Fpull%2F6577%2Fhead;p=mirror%2Ffrr.git 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 --- diff --git a/bgpd/bgp_rpki.c b/bgpd/bgp_rpki.c index be1df08ad2..3228b38990 100644 --- 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 dcf9852a67..8acd42ffc0 100644 --- 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 da2ca3181a..c35833e66b 100644 --- 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 79a8bb4408..7d81d42afb 100644 --- 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 4deaf08c22..53bd37233a 100644 --- 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')