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.

diff --git a/bgpd/bgp_main.c b/bgpd/bgp_main.c
index 9d14683caf3780242d615267d02976acdc0a8886..1a460c6bbecd7d6814e27c91851df6967de354e3 100644 (file)
--- 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 0cde665eb380e4d2585d3d8595d7a41b249d30ca..719cb966cd61d55075a20e142b3c3eafc1476a39 100644 (file)
--- 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 60e9610ef18a749addf9dbcec272673928d6af8e..ed98ac0afb89dc54e3ab77881d10b17278f9f4f0 100644 (file)
--- 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 05d932252724b3ff34fcb6faf790621f61530de9..5902067182dc7aa7126f5c3a3cd657f5ddea1123 100644 (file)
--- 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"                       },