[bgpd] reference count the BGP instance

When a BGP instance is deleted with lots of routes and neighbors
it is possible for the peer rsclient queue to run after
bgp_delete has been called. This would lead to bgpd crashing,
see https://bugzilla.vyatta.com/show_bug.cgi?id=3436

The fix is to add reference counting to the BGP instance and defer
actual freeing until all references are gone.

This patch also fixes a memory leak where the self-reference
peer instance was being created but never freed.

The check in bgp_clear_route is no longer valid because it is possible
for it to be called when peer is in Deleted state during cleanup.

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 01b3898a05327a130b1ddb127ca81d19d8d6fe36..a44d47abaadfd6cdd7a38758908390fbe25c99c8 100644 (file)
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -1562,6 +1562,7 @@ bgp_processq_del (struct work_queue *wq, void *data)
 {
   struct bgp_process_queue *pq = data;
   
+  bgp_unlock(pq->bgp);
   bgp_unlock_node (pq->rn);
   XFREE (MTYPE_BGP_PROCESS_QUEUE, pq);
 }
@@ -1611,6 +1612,7 @@ bgp_process (struct bgp *bgp, struct bgp_node *rn, afi_t afi, safi_t safi)
   
   pqnode->rn = bgp_lock_node (rn); /* unlocked by bgp_processq_del */
   pqnode->bgp = bgp;
+  bgp_lock(bgp);
   pqnode->afi = afi;
   pqnode->safi = safi;
   
@@ -2843,19 +2845,6 @@ bgp_clear_route (struct peer *peer, afi_t afi, safi_t safi)
    */
   if (!peer->clear_node_queue->thread)
     bgp_clear_node_complete (peer->clear_node_queue);
-  else
-    {
-      /* clearing queue scheduled. Normal if in Established state
-       * (and about to transition out of it), but otherwise...
-       */
-      if (peer->status != Established)
-        {
-          plog_err (peer->log, "%s [Error] State %s is not Established,"
-                    " but routes were cleared - bug!",
-                    peer->host, LOOKUP (bgp_status_msg, peer->status));
-          assert (peer->status == Established);
-        }
-    }
 }
   
 void

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 3adede820e28778faf931c534094d6bb6ec93099..1fefbd3f5d1b2dd607b4ffd624ee399d16be84d2 100644 (file)
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -688,7 +688,9 @@ static inline void
 peer_free (struct peer *peer)
 {
   assert (peer->status == Deleted);
-  
+
+  bgp_unlock(peer->bgp);
+
   /* this /ought/ to have been done already through bgp_stop earlier,
    * but just to be sure.. 
    */
@@ -791,6 +793,7 @@ peer_new (struct bgp *bgp)
   peer->password = NULL;
   peer->bgp = bgp;
   peer = peer_lock (peer); /* initial reference */
+  bgp_lock (bgp);
 
   /* Set default flags.  */
   for (afi = AFI_IP; afi < AFI_MAX; afi++)
@@ -1909,6 +1912,7 @@ bgp_create (as_t *as, const char *name)
   if ( (bgp = XCALLOC (MTYPE_BGP, sizeof (struct bgp))) == NULL)
     return NULL;
   
+  bgp_lock (bgp);
   bgp->peer_self = peer_new (bgp);
   bgp->peer_self->host = strdup ("Static announcement");
 
@@ -2045,7 +2049,6 @@ bgp_delete (struct bgp *bgp)
   struct listnode *node;
   struct listnode *next;
   afi_t afi;
-  safi_t safi;
   int i;
 
   /* Delete static route. */
@@ -2059,14 +2062,46 @@ bgp_delete (struct bgp *bgp)
 
   for (ALL_LIST_ELEMENTS (bgp->group, node, next, group))
     peer_group_delete (group);
-  list_delete (bgp->group);
 
   for (ALL_LIST_ELEMENTS (bgp->peer, node, next, peer))
     peer_delete (peer);
-  list_delete (bgp->peer);
 
   for (ALL_LIST_ELEMENTS (bgp->rsclient, node, next, peer))
     peer_delete (peer);
+
+  if (bgp->peer_self) {
+    peer_delete(bgp->peer_self);
+    bgp->peer_self = NULL;
+  }
+
+  bgp_unlock(bgp);  /* initial reference */
+
+  return 0;
+}
+
+static void bgp_free (struct bgp *);
+
+void
+bgp_lock (struct bgp *bgp)
+{
+  ++bgp->lock;
+}
+
+void
+bgp_unlock(struct bgp *bgp)
+{
+  if (--bgp->lock == 0)
+    bgp_free (bgp);
+}
+
+static void
+bgp_free (struct bgp *bgp)
+{
+  afi_t afi;
+  safi_t safi;
+
+  list_delete (bgp->group);
+  list_delete (bgp->peer);
   list_delete (bgp->rsclient);
 
   listnode_delete (bm->bgp, bgp);
@@ -2085,8 +2120,6 @@ bgp_delete (struct bgp *bgp)
          XFREE (MTYPE_ROUTE_TABLE,bgp->rib[afi][safi]);
       }
   XFREE (MTYPE_BGP, bgp);
-
-  return 0;
 }
 \f
 struct peer *

diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index afe0663571d6e77993d2feeef930042a8c0014d6..e8b8ef5a7df2ac60280e548ceaafbd872c669503 100644 (file)
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -70,6 +70,9 @@ struct bgp
   /* Name of this BGP instance.  */
   char *name;
   
+  /* Reference count to allow peer_delete to finish after bgp_delete */
+  int lock;
+
   /* Self peer.  */
   struct peer *peer_self;
 
@@ -843,6 +846,9 @@ extern int bgp_flag_set (struct bgp *, int);
 extern int bgp_flag_unset (struct bgp *, int);
 extern int bgp_flag_check (struct bgp *, int);
 
+extern void bgp_lock (struct bgp *);
+extern void bgp_unlock (struct bgp *);
+
 extern int bgp_router_id_set (struct bgp *, struct in_addr *);
 
 extern int bgp_cluster_id_set (struct bgp *, struct in_addr *);