From db8db5804d8425cb8e1cbb8af37160153d548891 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 17 Dec 2020 22:23:02 -0500 Subject: [PATCH 1/4] lib: arg can never be NULL Arg can never be null, get rid of an unneeded if statement Signed-off-by: Donald Sharp --- lib/routemap.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/lib/routemap.c b/lib/routemap.c index 004beb3628..f4c1648fea 100644 --- a/lib/routemap.c +++ b/lib/routemap.c @@ -2586,22 +2586,18 @@ static void route_map_clear_reference(struct hash_bucket *bucket, void *arg) struct route_map_dep *dep = bucket->data; struct route_map_dep_data *dep_data = NULL, tmp_dep_data; - if (arg) { - memset(&tmp_dep_data, 0, sizeof(struct route_map_dep_data)); - tmp_dep_data.rname = arg; - dep_data = hash_release(dep->dep_rmap_hash, - &tmp_dep_data); - if (dep_data) { - XFREE(MTYPE_ROUTE_MAP_NAME, dep_data->rname); - XFREE(MTYPE_ROUTE_MAP_DEP_DATA, dep_data); - } - if (!dep->dep_rmap_hash->count) { - dep = hash_release(dep->this_hash, - (void *)dep->dep_name); - hash_free(dep->dep_rmap_hash); - XFREE(MTYPE_ROUTE_MAP_NAME, dep->dep_name); - XFREE(MTYPE_ROUTE_MAP_DEP, dep); - } + memset(&tmp_dep_data, 0, sizeof(struct route_map_dep_data)); + tmp_dep_data.rname = arg; + dep_data = hash_release(dep->dep_rmap_hash, &tmp_dep_data); + if (dep_data) { + XFREE(MTYPE_ROUTE_MAP_NAME, dep_data->rname); + XFREE(MTYPE_ROUTE_MAP_DEP_DATA, dep_data); + } + if (!dep->dep_rmap_hash->count) { + dep = hash_release(dep->this_hash, (void *)dep->dep_name); + hash_free(dep->dep_rmap_hash); + XFREE(MTYPE_ROUTE_MAP_NAME, dep->dep_name); + XFREE(MTYPE_ROUTE_MAP_DEP, dep); } } From af87aff65d0c3d07622d2e8726a112fd5c899165 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 17 Dec 2020 22:25:27 -0500 Subject: [PATCH 2/4] lib: Add some useful debugs to understand what is going on Signed-off-by: Donald Sharp --- lib/routemap.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/routemap.c b/lib/routemap.c index f4c1648fea..bd97845564 100644 --- a/lib/routemap.c +++ b/lib/routemap.c @@ -2590,6 +2590,11 @@ static void route_map_clear_reference(struct hash_bucket *bucket, void *arg) tmp_dep_data.rname = arg; dep_data = hash_release(dep->dep_rmap_hash, &tmp_dep_data); if (dep_data) { + if (rmap_debug) + zlog_debug("Clearing reference for %s to %s count: %d", + dep->dep_name, tmp_dep_data.rname, + dep_data->refcnt); + XFREE(MTYPE_ROUTE_MAP_NAME, dep_data->rname); XFREE(MTYPE_ROUTE_MAP_DEP_DATA, dep_data); } @@ -2605,6 +2610,9 @@ static void route_map_clear_all_references(char *rmap_name) { int i; + if (rmap_debug) + zlog_debug("Clearing references for %s", rmap_name); + for (i = 1; i < ROUTE_MAP_DEP_MAX; i++) { hash_iterate(route_map_dep_hash[i], route_map_clear_reference, (void *)rmap_name); From 02e7a369b8356e79160d26c18798308c88b4d29d Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 18 Dec 2020 14:22:09 -0500 Subject: [PATCH 3/4] lib: Fix dependency of match types in route-map code Route-maps contain a hash of hash's that contain the container type name ( say community or access list or whatever ) and then it has a hash of route-maps that this maps too Suppose you have this: ! frr version 7.3.1 frr defaults traditional hostname eva log stdout ! debug route-map ! router bgp 239 neighbor 192.168.161.2 remote-as external ! address-family ipv4 unicast neighbor 192.168.161.2 route-map foo in exit-address-family ! bgp community-list standard 7000:40002 permit 7000:40002 bgp community-list standard 7000:40002 permit 7000:40003 ! route-map foo deny 20 match community 7000:40002 ! route-map foo permit 10 ! line vty ! end You have a community hash which has an 7000:40002 entry This entry has a hash of routemaps that are referencing it. In this above example it would have `foo` as the single entry. Given the above config if you do this: eva# conf eva(config)# route-map foo deny 20 eva(config-route-map)# match community 7000:4003 eva(config-route-map)# We would expect the `7000:40002` community hash to no longer have a reference to the `foo` routemap. Instead we see the code doing this: 2020/12/18 13:47:12 BGP: bgpd 7.3.1 starting: vty@2605, bgp@:179 2020/12/18 13:47:47 BGP: Add route-map foo 2020/12/18 13:47:47 BGP: Route-map foo add sequence 10, type: permit 2020/12/18 13:47:57 BGP: Route-map foo add sequence 20, type: deny 2020/12/18 13:48:05 BGP: Adding dependency for filter 7000:40002 in route-map foo 2020/12/18 13:48:05 BGP: route_map_print_dependency: Dependency for 7000:40002: foo 2020/12/18 13:48:41 BGP: bgp_update_receive: rcvd End-of-RIB for IPv4 Unicast from 192.168.161.2 in vrf default 2020/12/18 13:49:19 BGP: Deleting dependency for filter 7000:4003 in route-map foo 2020/12/18 13:49:19 BGP: Adding dependency for filter 7000:4003 in route-map foo 2020/12/18 13:49:19 BGP: route_map_print_dependency: Dependency for 7000:4003: foo Note how the code attempts to remove the dependency for `7000:4003` instead of the dependency for `7000:40002`. Then we create a new hash for `7000:4003` and then install the routemap name in it. This is wrong. We should remove the `7000:40002` dependency and then install a dependency for `7000:4003`. Fix the code to do the right thing. Signed-off-by: Donald Sharp --- lib/routemap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/routemap.c b/lib/routemap.c index bd97845564..5082a9fa9a 100644 --- a/lib/routemap.c +++ b/lib/routemap.c @@ -1347,7 +1347,7 @@ enum rmap_compile_rets route_map_add_match(struct route_map_index *index, get_route_map_delete_event(type); route_map_upd8_dependency( delete_rmap_event_type, - rule_key, + rule->rule_str, index->map->name); } From 9149c63517c41681a6cf07e11e6b85f8458dffdb Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 18 Dec 2020 14:38:40 -0500 Subject: [PATCH 4/4] lib: Add a warning for when we are not operating correctly There exists a possibilty that route map dependencies have gotten wrong. Prevent the crash and warn the user that we may be in trouble. Signed-off-by: Donald Sharp --- lib/routemap.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/routemap.c b/lib/routemap.c index 5082a9fa9a..1c2f43d968 100644 --- a/lib/routemap.c +++ b/lib/routemap.c @@ -2767,12 +2767,19 @@ static int route_map_dep_update(struct hash *dephash, const char *dep_name, memset(&tmp_dep_data, 0, sizeof(struct route_map_dep_data)); tmp_dep_data.rname = rname; dep_data = hash_lookup(dep->dep_rmap_hash, &tmp_dep_data); - - if (!dep_data) + /* + * If dep_data is NULL then something has gone seriously + * wrong in route-map handling. Note it and prevent + * the crash. + */ + if (!dep_data) { + zlog_warn( + "route-map dependency for route-map %s: %s is not correct", + rmap_name, dep_name); goto out; + } - if (dep_data->refcnt) - dep_data->refcnt--; + dep_data->refcnt--; if (!dep_data->refcnt) { ret_dep_data = hash_release(dep->dep_rmap_hash,