From ec8a02af45d038976f8423c17079a74db9e1fc63 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 08:53:36 -0400 Subject: [PATCH 01/24] bgpd: bgp_clear_adj_in|remove dest may be freed dest will not be freed due to lock but coverity does not know that. Give it a hint. This change includes modifying bgp_dest_unlock_node to return the dest pointer so that we can determine if we should continue working on dest or not with an assert. Since this is lock based we should be ok. Signed-off-by: Donald Sharp --- bgpd/bgp_advertise.c | 10 ++++++---- bgpd/bgp_advertise.h | 2 +- bgpd/bgp_route.c | 8 ++++++-- bgpd/bgp_table.c | 5 ++++- bgpd/bgp_table.h | 2 +- 5 files changed, 18 insertions(+), 9 deletions(-) diff --git a/bgpd/bgp_advertise.c b/bgpd/bgp_advertise.c index da7b496d65..c215611edf 100644 --- a/bgpd/bgp_advertise.c +++ b/bgpd/bgp_advertise.c @@ -188,11 +188,11 @@ void bgp_adj_in_set(struct bgp_dest *dest, struct peer *peer, struct attr *attr, bgp_dest_lock_node(dest); } -void bgp_adj_in_remove(struct bgp_dest *dest, struct bgp_adj_in *bai) +void bgp_adj_in_remove(struct bgp_dest **dest, struct bgp_adj_in *bai) { bgp_attr_unintern(&bai->attr); - BGP_ADJ_IN_DEL(dest, bai); - bgp_dest_unlock_node(dest); + BGP_ADJ_IN_DEL(*dest, bai); + *dest = bgp_dest_unlock_node(*dest); peer_unlock(bai->peer); /* adj_in peer reference */ XFREE(MTYPE_BGP_ADJ_IN, bai); } @@ -212,9 +212,11 @@ bool bgp_adj_in_unset(struct bgp_dest *dest, struct peer *peer, adj_next = adj->next; if (adj->peer == peer && adj->addpath_rx_id == addpath_id) - bgp_adj_in_remove(dest, adj); + bgp_adj_in_remove(&dest, adj); adj = adj_next; + + assert(dest); } return true; diff --git a/bgpd/bgp_advertise.h b/bgpd/bgp_advertise.h index a063208b6d..e597c56d13 100644 --- a/bgpd/bgp_advertise.h +++ b/bgpd/bgp_advertise.h @@ -132,7 +132,7 @@ extern void bgp_adj_in_set(struct bgp_dest *dest, struct peer *peer, struct attr *attr, uint32_t addpath_id); extern bool bgp_adj_in_unset(struct bgp_dest *dest, struct peer *peer, uint32_t addpath_id); -extern void bgp_adj_in_remove(struct bgp_dest *dest, struct bgp_adj_in *bai); +extern void bgp_adj_in_remove(struct bgp_dest **dest, struct bgp_adj_in *bai); extern unsigned int bgp_advertise_attr_hash_key(const void *p); extern bool bgp_advertise_attr_hash_cmp(const void *p1, const void *p2); diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index c8271bf5ce..1832d55ec0 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -5671,9 +5671,11 @@ static void bgp_clear_route_table(struct peer *peer, afi_t afi, safi_t safi, ain_next = ain->next; if (ain->peer == peer) - bgp_adj_in_remove(dest, ain); + bgp_adj_in_remove(&dest, ain); ain = ain_next; + + assert(dest); } for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = next) { @@ -5778,9 +5780,11 @@ void bgp_clear_adj_in(struct peer *peer, afi_t afi, safi_t safi) ain_next = ain->next; if (ain->peer == peer) - bgp_adj_in_remove(dest, ain); + bgp_adj_in_remove(&dest, ain); ain = ain_next; + + assert(dest); } } } diff --git a/bgpd/bgp_table.c b/bgpd/bgp_table.c index 149a5b9142..8465ada996 100644 --- a/bgpd/bgp_table.c +++ b/bgpd/bgp_table.c @@ -75,7 +75,7 @@ const char *bgp_dest_get_prefix_str(struct bgp_dest *dest) /* * bgp_dest_unlock_node */ -inline void bgp_dest_unlock_node(struct bgp_dest *dest) +inline struct bgp_dest *bgp_dest_unlock_node(struct bgp_dest *dest) { frrtrace(1, frr_bgp, bgp_dest_unlock, dest); bgp_delete_listnode(dest); @@ -89,9 +89,12 @@ inline void bgp_dest_unlock_node(struct bgp_dest *dest) rt->safi); } XFREE(MTYPE_BGP_NODE, dest); + dest = NULL; rn->info = NULL; } route_unlock_node(rn); + + return dest; } /* diff --git a/bgpd/bgp_table.h b/bgpd/bgp_table.h index 1f28624c16..5b4c3be212 100644 --- a/bgpd/bgp_table.h +++ b/bgpd/bgp_table.h @@ -112,7 +112,7 @@ extern struct bgp_table *bgp_table_init(struct bgp *bgp, afi_t, safi_t); extern void bgp_table_lock(struct bgp_table *); extern void bgp_table_unlock(struct bgp_table *); extern void bgp_table_finish(struct bgp_table **); -extern void bgp_dest_unlock_node(struct bgp_dest *dest); +extern struct bgp_dest *bgp_dest_unlock_node(struct bgp_dest *dest); extern struct bgp_dest *bgp_dest_lock_node(struct bgp_dest *dest); extern const char *bgp_dest_get_prefix_str(struct bgp_dest *dest); From b45925ad100c85404d15bce19a3e584ecb60f920 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:09:08 -0400 Subject: [PATCH 02/24] bgpd: evpn_cleanup_local_non_best_route could free dest But never really does due to locking, but since it can we need to treat it like it does and ensure that FRR is not making a mistake, by using memory after it has been freed. Signed-off-by: Donald Sharp --- bgpd/bgp_evpn.c | 17 ++++++++++------- bgpd/bgp_route.c | 6 ++++-- bgpd/bgp_route.h | 3 ++- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index e5d33e6d59..ff931e1f07 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -2035,10 +2035,10 @@ static void evpn_zebra_reinstall_best_route(struct bgp *bgp, * additional handling to prevent bgp from injecting and holding on to a * non-best local path. */ -static void evpn_cleanup_local_non_best_route(struct bgp *bgp, - struct bgpevpn *vpn, - struct bgp_dest *dest, - struct bgp_path_info *local_pi) +static struct bgp_dest * +evpn_cleanup_local_non_best_route(struct bgp *bgp, struct bgpevpn *vpn, + struct bgp_dest *dest, + struct bgp_path_info *local_pi) { /* local path was not picked as the winner; kick it out */ if (bgp_debug_zebra(NULL)) @@ -2046,10 +2046,11 @@ static void evpn_cleanup_local_non_best_route(struct bgp *bgp, dest); evpn_delete_old_local_route(bgp, vpn, dest, local_pi, NULL); - bgp_path_info_reap(dest, local_pi); /* tell zebra to re-add the best remote path */ evpn_zebra_reinstall_best_route(bgp, vpn, dest); + + return bgp_path_info_reap(dest, local_pi); } static inline bool bgp_evpn_route_add_l3_ecomm_ok(struct bgpevpn *vpn, @@ -2186,7 +2187,8 @@ static int update_evpn_route(struct bgp *bgp, struct bgpevpn *vpn, } else { if (!CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) { route_change = 0; - evpn_cleanup_local_non_best_route(bgp, vpn, dest, pi); + dest = evpn_cleanup_local_non_best_route(bgp, vpn, dest, + pi); } else { bool new_is_sync; @@ -2203,7 +2205,8 @@ static int update_evpn_route(struct bgp *bgp, struct bgpevpn *vpn, } bgp_path_info_unlock(pi); - bgp_dest_unlock_node(dest); + if (dest) + bgp_dest_unlock_node(dest); /* If this is a new route or some attribute has changed, export the * route to the global table. The route will be advertised to peers diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 1832d55ec0..f769071185 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -438,7 +438,8 @@ void bgp_path_info_add_with_caller(const char *name, struct bgp_dest *dest, /* Do the actual removal of info from RIB, for use by bgp_process completion callback *only* */ -void bgp_path_info_reap(struct bgp_dest *dest, struct bgp_path_info *pi) +struct bgp_dest *bgp_path_info_reap(struct bgp_dest *dest, + struct bgp_path_info *pi) { if (pi->next) pi->next->prev = pi->prev; @@ -450,7 +451,8 @@ void bgp_path_info_reap(struct bgp_dest *dest, struct bgp_path_info *pi) bgp_path_info_mpath_dequeue(pi); bgp_path_info_unlock(pi); hook_call(bgp_snmp_update_stats, dest, pi, false); - bgp_dest_unlock_node(dest); + + return bgp_dest_unlock_node(dest); } void bgp_path_info_delete(struct bgp_dest *dest, struct bgp_path_info *pi) diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index e001bf4f07..cc13500fa1 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -727,7 +727,8 @@ extern struct bgp_path_info * bgp_get_imported_bpi_ultimate(struct bgp_path_info *info); extern void bgp_path_info_add(struct bgp_dest *dest, struct bgp_path_info *pi); extern void bgp_path_info_extra_free(struct bgp_path_info_extra **extra); -extern void bgp_path_info_reap(struct bgp_dest *dest, struct bgp_path_info *pi); +extern struct bgp_dest *bgp_path_info_reap(struct bgp_dest *dest, + struct bgp_path_info *pi); extern void bgp_path_info_delete(struct bgp_dest *dest, struct bgp_path_info *pi); extern struct bgp_path_info_extra * From f491b540791dd48340f139f16381a1cfbe262049 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:12:26 -0400 Subject: [PATCH 03/24] bgpd: delete_evpn_route ensure that dest is not freed before usage There exist two spots in this function where the dest could be freed, but is not due to locking, but coverity thinks it might so let's make the function happy. Signed-off-by: Donald Sharp --- bgpd/bgp_evpn.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index ff931e1f07..6cc1dd710d 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -2330,9 +2330,13 @@ static int delete_evpn_route(struct bgp *bgp, struct bgpevpn *vpn, */ delete_evpn_route_entry(bgp, afi, safi, dest, &pi); if (pi) { - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + assert(dest); evpn_route_select_install(bgp, vpn, dest); } + + /* dest should still exist due to locking make coverity happy */ + assert(dest); bgp_dest_unlock_node(dest); return 0; From 8d39c8c9272b1ca876467af87dfc0cdbbd5ae7e4 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:15:41 -0400 Subject: [PATCH 04/24] bgpd: delete_vin_type2_route may free dest The dest pointer may be freed( but should not be due to locking ). Let's ensure that this assumption is true and make coverity happy. Signed-off-by: Donald Sharp --- bgpd/bgp_evpn.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index 6cc1dd710d..9bc6c508dc 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -2578,7 +2578,8 @@ static void delete_global_type2_routes(struct bgp *bgp, struct bgpevpn *vpn) } } -static void delete_vni_type2_route(struct bgp *bgp, struct bgp_dest *dest) +static struct bgp_dest *delete_vni_type2_route(struct bgp *bgp, + struct bgp_dest *dest) { struct bgp_path_info *pi; afi_t afi = AFI_L2VPN; @@ -2588,13 +2589,15 @@ static void delete_vni_type2_route(struct bgp *bgp, struct bgp_dest *dest) (const struct prefix_evpn *)bgp_dest_get_prefix(dest); if (evp->prefix.route_type != BGP_EVPN_MAC_IP_ROUTE) - return; + return dest; delete_evpn_route_entry(bgp, afi, safi, dest, &pi); /* Route entry in local table gets deleted immediately. */ if (pi) - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + + return dest; } static void delete_vni_type2_routes(struct bgp *bgp, struct bgpevpn *vpn) @@ -2605,12 +2608,16 @@ static void delete_vni_type2_routes(struct bgp *bgp, struct bgpevpn *vpn) * routes. */ for (dest = bgp_table_top(vpn->mac_table); dest; - dest = bgp_route_next(dest)) - delete_vni_type2_route(bgp, dest); + dest = bgp_route_next(dest)) { + dest = delete_vni_type2_route(bgp, dest); + assert(dest); + } for (dest = bgp_table_top(vpn->ip_table); dest; - dest = bgp_route_next(dest)) - delete_vni_type2_route(bgp, dest); + dest = bgp_route_next(dest)) { + dest = delete_vni_type2_route(bgp, dest); + assert(dest); + } } /* From dade8dfdd69add9a051723ea26547cfcddb4ac14 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:17:20 -0400 Subject: [PATCH 05/24] bgpd: bgp_evpn_mh_route_delete should ensure dest is still usable Again coverity believes that dest may be freed but it should not be because of how locking is done. Make coverity happy. Signed-off-by: Donald Sharp --- bgpd/bgp_evpn_mh.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_evpn_mh.c b/bgpd/bgp_evpn_mh.c index 91db9a061a..7700281cd2 100644 --- a/bgpd/bgp_evpn_mh.c +++ b/bgpd/bgp_evpn_mh.c @@ -513,8 +513,11 @@ static int bgp_evpn_mh_route_delete(struct bgp *bgp, struct bgp_evpn_es *es, */ delete_evpn_route_entry(bgp, afi, safi, dest, &pi); if (pi) - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + + assert(dest); bgp_dest_unlock_node(dest); + return 0; } From 35f352c457488d9ae49dc5e1d1ea59c46b56c3a4 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:19:05 -0400 Subject: [PATCH 06/24] bgpd: bgp_evpn_es_route_del_all should not free dest until after looping Again the dest pointer should be still locked by the table walk. Ensure that coverity is happy that this is not happening. Signed-off-by: Donald Sharp --- bgpd/bgp_evpn_mh.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_evpn_mh.c b/bgpd/bgp_evpn_mh.c index 7700281cd2..ffb1b17ec7 100644 --- a/bgpd/bgp_evpn_mh.c +++ b/bgpd/bgp_evpn_mh.c @@ -323,7 +323,9 @@ static void bgp_evpn_es_route_del_all(struct bgp *bgp, struct bgp_evpn_es *es) for (pi = bgp_dest_get_bgp_path_info(dest); (pi != NULL) && (nextpi = pi->next, 1); pi = nextpi) { bgp_path_info_delete(dest, pi); - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + + assert(dest); } } } From ed74c8b5553300f22355d9efa7f0b317174ce71b Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:20:48 -0400 Subject: [PATCH 07/24] bgpd: bgp_cleanup_routes ensure dest is not freed The bgp_cleanup_routes function holds the lock for dest while walking it. Ensure that coverity understands this proposition. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index f769071185..e72ebe3ae7 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -6033,7 +6033,9 @@ void bgp_cleanup_routes(struct bgp *bgp) bgp_cleanup_table(bgp, table, safi); bgp_table_finish(&table); bgp_dest_set_bgp_table_info(dest, NULL); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + + assert(dest); } } safi = SAFI_ENCAP; @@ -6044,7 +6046,9 @@ void bgp_cleanup_routes(struct bgp *bgp) bgp_cleanup_table(bgp, table, safi); bgp_table_finish(&table); bgp_dest_set_bgp_table_info(dest, NULL); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + + assert(dest); } } } @@ -6056,7 +6060,9 @@ void bgp_cleanup_routes(struct bgp *bgp) bgp_cleanup_table(bgp, table, SAFI_EVPN); bgp_table_finish(&table); bgp_dest_set_bgp_table_info(dest, NULL); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + + assert(dest); } } } From 493075d25b1686661b89e2e6bc68cec55ed5054b Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:22:47 -0400 Subject: [PATCH 08/24] bgpd: bgp_connected_delete needs to ensure dest is still there Again coverity believes that dest could be freed by a call into bgp_dest_unlock_node, and it can if the lock count is wrong. Let's fix that assumption for coverity Signed-off-by: Donald Sharp --- bgpd/bgp_nexthop.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c index 6cb6d65c64..367c7056fd 100644 --- a/bgpd/bgp_nexthop.c +++ b/bgpd/bgp_nexthop.c @@ -483,7 +483,9 @@ void bgp_connected_delete(struct bgp *bgp, struct connected *ifc) XFREE(MTYPE_BGP_CONN, bc); bgp_dest_set_bgp_connected_ref_info(dest, NULL); } - bgp_dest_unlock_node(dest); + + dest = bgp_dest_unlock_node(dest); + assert(dest); bgp_dest_unlock_node(dest); } From e6458d36b7f9347815ccd2aa2d4af8107f84fce5 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:27:51 -0400 Subject: [PATCH 09/24] bgpd: bgp_adj_in_unset needs to return the dest pointer This is incase it has been freed ( it wont due to locking ) and then we need to ensure that we can continue to use the pointer. Signed-off-by: Donald Sharp --- bgpd/bgp_advertise.c | 8 ++++---- bgpd/bgp_advertise.h | 2 +- bgpd/bgp_route.c | 4 +++- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/bgpd/bgp_advertise.c b/bgpd/bgp_advertise.c index c215611edf..6f4916b3c3 100644 --- a/bgpd/bgp_advertise.c +++ b/bgpd/bgp_advertise.c @@ -197,13 +197,13 @@ void bgp_adj_in_remove(struct bgp_dest **dest, struct bgp_adj_in *bai) XFREE(MTYPE_BGP_ADJ_IN, bai); } -bool bgp_adj_in_unset(struct bgp_dest *dest, struct peer *peer, +bool bgp_adj_in_unset(struct bgp_dest **dest, struct peer *peer, uint32_t addpath_id) { struct bgp_adj_in *adj; struct bgp_adj_in *adj_next; - adj = dest->adj_in; + adj = (*dest)->adj_in; if (!adj) return false; @@ -212,11 +212,11 @@ bool bgp_adj_in_unset(struct bgp_dest *dest, struct peer *peer, adj_next = adj->next; if (adj->peer == peer && adj->addpath_rx_id == addpath_id) - bgp_adj_in_remove(&dest, adj); + bgp_adj_in_remove(dest, adj); adj = adj_next; - assert(dest); + assert(*dest); } return true; diff --git a/bgpd/bgp_advertise.h b/bgpd/bgp_advertise.h index e597c56d13..94168e2fd7 100644 --- a/bgpd/bgp_advertise.h +++ b/bgpd/bgp_advertise.h @@ -130,7 +130,7 @@ extern bool bgp_adj_out_lookup(struct peer *peer, struct bgp_dest *dest, uint32_t addpath_tx_id); extern void bgp_adj_in_set(struct bgp_dest *dest, struct peer *peer, struct attr *attr, uint32_t addpath_id); -extern bool bgp_adj_in_unset(struct bgp_dest *dest, struct peer *peer, +extern bool bgp_adj_in_unset(struct bgp_dest **dest, struct peer *peer, uint32_t addpath_id); extern void bgp_adj_in_remove(struct bgp_dest **dest, struct bgp_adj_in *bai); diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index e72ebe3ae7..34d36a6375 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -5083,7 +5083,8 @@ void bgp_withdraw(struct peer *peer, const struct prefix *p, */ if (CHECK_FLAG(peer->af_flags[afi][safi], PEER_FLAG_SOFT_RECONFIG) && peer != bgp->peer_self) - if (!bgp_adj_in_unset(dest, peer, addpath_id)) { + if (!bgp_adj_in_unset(&dest, peer, addpath_id)) { + assert(dest); peer->stat_pfx_dup_withdraw++; if (bgp_debug_update(peer, p, NULL, 1)) { @@ -5100,6 +5101,7 @@ void bgp_withdraw(struct peer *peer, const struct prefix *p, } /* Lookup withdrawn route. */ + assert(dest); for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) if (pi->peer == peer && pi->type == type && pi->sub_type == sub_type From b7dd15242cc5e616e140369f598bed4527a6e838 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:32:29 -0400 Subject: [PATCH 10/24] bgpd: ensure delete_all_vni_routes does not free dest dest is locked by the table walk. ensure that coverity understands this. Signed-off-by: Donald Sharp --- bgpd/bgp_evpn.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index 9bc6c508dc..c97a53bf13 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -2649,7 +2649,9 @@ static void delete_all_vni_routes(struct bgp *bgp, struct bgpevpn *vpn) (pi != NULL) && (nextpi = pi->next, 1); pi = nextpi) { bgp_evpn_remote_ip_hash_del(vpn, pi); bgp_path_info_delete(dest, pi); - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + + assert(dest); } } @@ -2658,7 +2660,9 @@ static void delete_all_vni_routes(struct bgp *bgp, struct bgpevpn *vpn) for (pi = bgp_dest_get_bgp_path_info(dest); (pi != NULL) && (nextpi = pi->next, 1); pi = nextpi) { bgp_path_info_delete(dest, pi); - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + + assert(dest); } } } From 70f6103afd129853cf5a611e28f8002ed5f9ff79 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:33:55 -0400 Subject: [PATCH 11/24] bgpd: bgp_process_main_one should ensure dest exists Unsetting a flag after the dest has been possibly been freed is not a good thing to do. Ensure that this is not possible. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 34d36a6375..7d0cc377bd 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -3504,11 +3504,12 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest, /* Clear any route change flags. */ bgp_zebra_clear_route_change_flags(dest); + UNSET_FLAG(dest->flags, BGP_NODE_PROCESS_SCHEDULED); + /* Reap old select bgp_path_info, if it has been removed */ if (old_select && CHECK_FLAG(old_select->flags, BGP_PATH_REMOVED)) bgp_path_info_reap(dest, old_select); - UNSET_FLAG(dest->flags, BGP_NODE_PROCESS_SCHEDULED); return; } From 271c00074fba9c04a4fe0b47a8c7e9a4cb0f2143 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:35:38 -0400 Subject: [PATCH 12/24] bgpd: bgp_distance_unset ensure dest exists Coverity doesn't understand our locking scheme make sure it does a bit better. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 7d0cc377bd..4d4a2a5c35 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -15049,7 +15049,8 @@ static int bgp_distance_unset(struct vty *vty, const char *distance_str, bgp_distance_free(bdistance); bgp_dest_set_bgp_path_info(dest, NULL); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + assert(dest); bgp_dest_unlock_node(dest); return CMD_SUCCESS; From 6c61eba7731145cad280f3afde61225d7b78fdc9 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:38:56 -0400 Subject: [PATCH 13/24] bgpd: bgp_show_route_in_table ensure rm exists The rm exists because it is locked while we are walking it, so this should be safe. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 4d4a2a5c35..5189b3643c 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -12146,7 +12146,9 @@ static int bgp_show_route_in_table(struct vty *vty, struct bgp *bgp, rm_p); if (type5_pfxlen == match.prefixlen) { is_exact_pfxlen_match = true; - bgp_dest_unlock_node(rm); + rm = bgp_dest_unlock_node(rm); + + assert(rm); break; } } From aa3755bf4c9b40ac426d9a689e1ea7c11d09d2ba Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:40:29 -0400 Subject: [PATCH 14/24] bgpd: bgp_reg_for_label_callback ensure dest exist More dest may be freed so let's ensure it is not. Signed-off-by: Donald Sharp --- bgpd/bgp_label.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_label.c b/bgpd/bgp_label.c index 30090e0590..b8ce1ae467 100644 --- a/bgpd/bgp_label.c +++ b/bgpd/bgp_label.c @@ -195,7 +195,8 @@ int bgp_reg_for_label_callback(mpls_label_t new_label, void *labelid, return -1; } - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + assert(dest); if (BGP_DEBUG(labelpool, LABELPOOL)) zlog_debug("%s: FEC %pRN label=%u, allocated=%d", __func__, From 5486383c853d2e0deb6071e94d83439b0836319c Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:42:06 -0400 Subject: [PATCH 15/24] bgpd: bgp_static_delete ensure rm and dest exist Ensure that the rm and dest exist since the code has them locked to loop over them safely. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 5189b3643c..80498bc0d7 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -6891,7 +6891,8 @@ void bgp_static_delete(struct bgp *bgp) bgp_static_free(bgp_static); bgp_dest_set_bgp_static_info(rm, NULL); - bgp_dest_unlock_node(rm); + rm = bgp_dest_unlock_node(rm); + assert(rm); } } else { bgp_static = bgp_dest_get_bgp_static_info(dest); @@ -6900,7 +6901,8 @@ void bgp_static_delete(struct bgp *bgp) afi, safi, NULL); bgp_static_free(bgp_static); bgp_dest_set_bgp_static_info(dest, NULL); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + assert(dest); } } } From fce574212256d32ffaef386c54c8568518201f7d Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:43:33 -0400 Subject: [PATCH 16/24] bgpd: bgp_cleanup_table ensure dest is still usable. Make coverity happy Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 80498bc0d7..b84abf262e 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -6007,7 +6007,8 @@ static void bgp_cleanup_table(struct bgp *bgp, struct bgp_table *table, bgp_zebra_withdraw(p, pi, bgp, safi); } - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + assert(dest); } } From 8c9e7835ae93e2f3712a4ed48b1c2387d7aa617a Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:45:15 -0400 Subject: [PATCH 17/24] bgpd: bgp_static_set ensure dest is still usable. Again coverity thinks dest may be freed on the first call but it should not be. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index b84abf262e..38324681fe 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -6715,7 +6715,8 @@ int bgp_static_set(struct vty *vty, bool negate, const char *ip_str, } bgp_dest_set_bgp_static_info(dest, NULL); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + assert(dest); bgp_dest_unlock_node(dest); } else { dest = bgp_node_get(table, &p); From 3abbc2340a00041c970d8a4a4b4a766b1c1ed5f1 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:46:25 -0400 Subject: [PATCH 18/24] bgpd: Ensure debug is printed before possible dest freed in install_evpn_route_entry_in_vrf Signed-off-by: Donald Sharp --- bgpd/bgp_evpn.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index c97a53bf13..aa23f06762 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -3029,14 +3029,14 @@ static int install_evpn_route_entry_in_vrf(struct bgp *bgp_vrf, /* Process for route leaking. */ vpn_leak_from_vrf_update(bgp_get_default(), bgp_vrf, pi); - bgp_dest_unlock_node(dest); - if (bgp_debug_zebra(NULL)) zlog_debug("... %s pi dest %p (l %d) pi %p (l %d, f 0x%x)", new_pi ? "new" : "update", dest, bgp_dest_get_lock_count(dest), pi, pi->lock, pi->flags); + bgp_dest_unlock_node(dest); + return ret; } From 842c5259b613ae8ca843ca99abb97923bcc39817 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:47:44 -0400 Subject: [PATCH 19/24] bgpd: Ensure bgp_redistribute_withdraw dest is usable still Same story dest is locked during table walk. ensure coverity understands this. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 38324681fe..8320118a4e 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -8757,8 +8757,10 @@ void bgp_redistribute_withdraw(struct bgp *bgp, afi_t afi, int type, if (!CHECK_FLAG(bgp->flags, BGP_FLAG_DELETE_IN_PROGRESS)) bgp_process(bgp, dest, afi, SAFI_UNICAST); - else - bgp_path_info_reap(dest, pi); + else { + dest = bgp_path_info_reap(dest, pi); + assert(dest); + } } } } From dc01a8ba034e9483ac850e53125c6417714482ab Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:49:34 -0400 Subject: [PATCH 20/24] bgpd: Ensure bgp_aggregate_unset does dest good dest could be freed by the first unlock, but should not be due to our locking structure. Ensure coverity understands this. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 8320118a4e..d745f8cb78 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -8235,7 +8235,8 @@ static int bgp_aggregate_unset(struct vty *vty, const char *prefix_str, bgp_dest_set_bgp_aggregate_info(dest, NULL); bgp_free_aggregate_info(aggregate); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + assert(dest); bgp_dest_unlock_node(dest); return CMD_SUCCESS; From c955a3cbeced0539b3dbb12bb1c235b264f0d6b6 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:51:34 -0400 Subject: [PATCH 21/24] bgpd: bgp_best_selection ensure dest still exists When reaping the dest ensure that it still exists as that it should be locked by the calling function. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index d745f8cb78..367398e5fd 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -2815,9 +2815,11 @@ void bgp_best_selection(struct bgp *bgp, struct bgp_dest *dest, /* reap REMOVED routes, if needs be * selected route must stay for a while longer though */ - if (CHECK_FLAG(pi->flags, BGP_PATH_REMOVED) - && (pi != old_select)) - bgp_path_info_reap(dest, pi); + if (CHECK_FLAG(pi->flags, BGP_PATH_REMOVED) && + (pi != old_select)) { + dest = bgp_path_info_reap(dest, pi); + assert(dest); + } if (debug) zlog_debug( From 1195c44f4bad73fd5e02fa9013efc2388e79b46d Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:53:54 -0400 Subject: [PATCH 22/24] bgpd: In bgp_clear_route_table ensure dest is still usable. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 367398e5fd..3ef495cd5c 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -5690,9 +5690,10 @@ static void bgp_clear_route_table(struct peer *peer, afi_t afi, safi_t safi, if (pi->peer != peer) continue; - if (force) - bgp_path_info_reap(dest, pi); - else { + if (force) { + dest = bgp_path_info_reap(dest, pi); + assert(dest); + } else { struct bgp_clear_node_queue *cnq; /* both unlocked in bgp_clear_node_queue_del */ From ecb846048238b2e7442331af9aebdbfeb0ab37a0 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:55:24 -0400 Subject: [PATCH 23/24] bgpd: bgp_afi_node_get teach coverity about unlocking The pdest pointer is locked by the bgp_node_get so unlocking it should be fine and it should still exist. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 3ef495cd5c..8c8087a2f5 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -138,7 +138,9 @@ struct bgp_dest *bgp_afi_node_get(struct bgp_table *table, afi_t afi, bgp_dest_set_bgp_table_info( pdest, bgp_table_init(table->bgp, afi, safi)); else - bgp_dest_unlock_node(pdest); + pdest = bgp_dest_unlock_node(pdest); + + assert(pdest); table = bgp_dest_get_bgp_table_info(pdest); } From 53a95715355a0ae6a8d6c681b57581ebc29c2121 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 10 Sep 2023 09:56:53 -0400 Subject: [PATCH 24/24] bgpd: Ensure that leak_update does not free memory before it is being used The unlock may cause the bgp_process to use dest. Ensure that this does not happen. Signed-off-by: Donald Sharp --- bgpd/bgp_mplsvpn.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index 3ecb72b4ef..9b5d568636 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -1238,13 +1238,14 @@ leak_update(struct bgp *to_bgp, struct bgp_dest *bn, bgp_aggregate_increment(to_bgp, p, new, afi, safi); bgp_path_info_add(bn, new); - bgp_dest_unlock_node(bn); bgp_process(to_bgp, bn, afi, safi); if (debug) zlog_debug("%s: ->%s: %pBD: Added new route", __func__, to_bgp->name_pretty, bn); + bgp_dest_unlock_node(bn); + return new; }