From 6c88b44dcb26b60bb1f93e5c387aa102019ed849 Mon Sep 17 00:00:00 2001 From: Chris Caputo Date: Tue, 27 Jul 2010 16:28:55 +0000 Subject: bgpd: fix bgp_node locking issues * bgpd: Connected table locks were being locked but not unlocked, such that eventually a lock would exceed 2^31 and become negative, thus triggering an assert later on. * bgp_main.c: (bgp_exit) delete connected elements along with ifp's. * bgp_nexthop.c: (bgp_nexthop_lookup{,_ipv6}) add missing unlocks (bgp_multiaccess_check_v4) ditto (bgp_connected_{add,delete}) Use a distinct memtype for bgp_connected_ref. (bgp_scan_finish) reset the nexthop cache to clean it up when bgpd exits * bgp_route.c: fix missing bgp_node unlocks * lib/memtype.c: (memory_list_bgp) add MTYPE_BGP_CONN * testing: has been tested for almost 2 months now. --- bgpd/bgp_main.c | 10 ++++++- bgpd/bgp_nexthop.c | 22 +++++++++++++--- bgpd/bgp_route.c | 77 ++++++++++++++++++++++++++++++++---------------------- lib/memtypes.c | 1 + 4 files changed, 74 insertions(+), 36 deletions(-) diff --git a/bgpd/bgp_main.c b/bgpd/bgp_main.c index 9d14683c..1a460c6b 100644 --- a/bgpd/bgp_main.c +++ b/bgpd/bgp_main.c @@ -243,7 +243,15 @@ bgp_exit (int status) if (retain_mode) if_add_hook (IF_DELETE_HOOK, NULL); for (ALL_LIST_ELEMENTS (iflist, node, nnode, ifp)) - if_delete (ifp); + { + struct listnode *c_node, *c_nnode; + struct connected *c; + + for (ALL_LIST_ELEMENTS (ifp->connected, c_node, c_nnode, c)) + bgp_connected_delete (c); + + if_delete (ifp); + } list_free (iflist); /* reverse bgp_attr_init */ diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c index 0cde665e..719cb966 100644 --- a/bgpd/bgp_nexthop.c +++ b/bgpd/bgp_nexthop.c @@ -276,6 +276,8 @@ bgp_nexthop_lookup_ipv6 (struct peer *peer, struct bgp_info *ri, int *changed, if (bnc->metric != oldbnc->metric) bnc->metricchanged = 1; + + bgp_unlock_node (oldrn); } } } @@ -365,6 +367,8 @@ bgp_nexthop_lookup (afi_t afi, struct peer *peer, struct bgp_info *ri, if (bnc->metric != oldbnc->metric) bnc->metricchanged = 1; + + bgp_unlock_node (oldrn); } } } @@ -571,7 +575,7 @@ bgp_connected_add (struct connected *ifc) } else { - bc = XCALLOC (0, sizeof (struct bgp_connected_ref)); + bc = XCALLOC (MTYPE_BGP_CONN, sizeof (struct bgp_connected_ref)); bc->refcnt = 1; rn->info = bc; } @@ -596,7 +600,7 @@ bgp_connected_add (struct connected *ifc) } else { - bc = XCALLOC (0, sizeof (struct bgp_connected_ref)); + bc = XCALLOC (MTYPE_BGP_CONN, sizeof (struct bgp_connected_ref)); bc->refcnt = 1; rn->info = bc; } @@ -636,7 +640,7 @@ bgp_connected_delete (struct connected *ifc) bc->refcnt--; if (bc->refcnt == 0) { - XFREE (0, bc); + XFREE (MTYPE_BGP_CONN, bc); rn->info = NULL; } bgp_unlock_node (rn); @@ -662,7 +666,7 @@ bgp_connected_delete (struct connected *ifc) bc->refcnt--; if (bc->refcnt == 0) { - XFREE (0, bc); + XFREE (MTYPE_BGP_CONN, bc); rn->info = NULL; } bgp_unlock_node (rn); @@ -1136,11 +1140,15 @@ bgp_multiaccess_check_v4 (struct in_addr nexthop, char *peer) rn1 = bgp_node_match (bgp_connected_table[AFI_IP], &p1); if (! rn1) return 0; + bgp_unlock_node (rn1); rn2 = bgp_node_match (bgp_connected_table[AFI_IP], &p2); if (! rn2) return 0; + bgp_unlock_node (rn2); + /* This is safe, even with above unlocks, since we are just + comparing pointers to the objects, not the objects themselves. */ if (rn1 == rn2) return 1; @@ -1316,6 +1324,9 @@ bgp_scan_init (void) void bgp_scan_finish (void) { + /* Only the current one needs to be reset. */ + bgp_nexthop_cache_reset (bgp_nexthop_cache_table[AFI_IP]); + bgp_table_unlock (cache1_table[AFI_IP]); cache1_table[AFI_IP] = NULL; @@ -1326,6 +1337,9 @@ bgp_scan_finish (void) bgp_connected_table[AFI_IP] = NULL; #ifdef HAVE_IPV6 + /* Only the current one needs to be reset. */ + bgp_nexthop_cache_reset (bgp_nexthop_cache_table[AFI_IP6]); + bgp_table_unlock (cache1_table[AFI_IP6]); cache1_table[AFI_IP6] = NULL; diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 60e9610e..ed98ac0a 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -6561,7 +6561,10 @@ bgp_show_route_in_table (struct vty *vty, struct bgp *bgp, if ((rm = bgp_node_match (table, &match)) != NULL) { if (prefix_check && rm->p.prefixlen != match.prefixlen) - continue; + { + bgp_unlock_node (rm); + continue; + } for (ri = rm->info; ri; ri = ri->next) { @@ -6575,6 +6578,8 @@ bgp_show_route_in_table (struct vty *vty, struct bgp *bgp, display++; route_vty_out_detail (vty, bgp, &rm->p, ri, AFI_IP, SAFI_MPLS_VPN); } + + bgp_unlock_node (rm); } } } @@ -6598,6 +6603,8 @@ bgp_show_route_in_table (struct vty *vty, struct bgp *bgp, route_vty_out_detail (vty, bgp, &rn->p, ri, afi, safi); } } + + bgp_unlock_node (rn); } } @@ -11355,41 +11362,49 @@ bgp_clear_damp_route (struct vty *vty, const char *view_name, if ((table = rn->info) != NULL) if ((rm = bgp_node_match (table, &match)) != NULL) - if (! prefix_check || rm->p.prefixlen == match.prefixlen) - { - ri = rm->info; - while (ri) - { - if (ri->extra && ri->extra->damp_info) - { - ri_temp = ri->next; - bgp_damp_info_free (ri->extra->damp_info, 1); - ri = ri_temp; - } - else - ri = ri->next; - } - } + { + if (! prefix_check || rm->p.prefixlen == match.prefixlen) + { + ri = rm->info; + while (ri) + { + if (ri->extra && ri->extra->damp_info) + { + ri_temp = ri->next; + bgp_damp_info_free (ri->extra->damp_info, 1); + ri = ri_temp; + } + else + ri = ri->next; + } + } + + bgp_unlock_node (rm); + } } } else { if ((rn = bgp_node_match (bgp->rib[afi][safi], &match)) != NULL) - if (! prefix_check || rn->p.prefixlen == match.prefixlen) - { - ri = rn->info; - while (ri) - { - if (ri->extra && ri->extra->damp_info) - { - ri_temp = ri->next; - bgp_damp_info_free (ri->extra->damp_info, 1); - ri = ri_temp; - } - else - ri = ri->next; - } - } + { + if (! prefix_check || rn->p.prefixlen == match.prefixlen) + { + ri = rn->info; + while (ri) + { + if (ri->extra && ri->extra->damp_info) + { + ri_temp = ri->next; + bgp_damp_info_free (ri->extra->damp_info, 1); + ri = ri_temp; + } + else + ri = ri->next; + } + } + + bgp_unlock_node (rn); + } } return CMD_SUCCESS; diff --git a/lib/memtypes.c b/lib/memtypes.c index 05d93225..59020671 100644 --- a/lib/memtypes.c +++ b/lib/memtypes.c @@ -108,6 +108,7 @@ struct memory_list memory_list_bgp[] = { MTYPE_BGP_NODE, "BGP node" }, { MTYPE_BGP_ROUTE, "BGP route" }, { MTYPE_BGP_ROUTE_EXTRA, "BGP ancillary route info" }, + { MTYPE_BGP_CONN, "BGP connected" }, { MTYPE_BGP_STATIC, "BGP static" }, { MTYPE_BGP_ADVERTISE_ATTR, "BGP adv attr" }, { MTYPE_BGP_ADVERTISE, "BGP adv" }, -- cgit v1.2.1