From eb6918e4db57b7e5fabc531963deb46313289d80 Mon Sep 17 00:00:00 2001
From: Maria Matejka <mq@ucw.cz>
Date: Sun, 10 Nov 2024 13:33:22 +0100
Subject: [PATCH] Neighbor cache: fixed neighbor referencing

---
 nest/iface.c          | 14 ++++++++------
 nest/iface.h          | 22 ++++++++++++++++++++++
 nest/neighbor.c       | 30 ++++++++++++++++++++++++++----
 nest/proto.c          |  8 ++++++--
 proto/bfd/bfd.c       |  7 ++++++-
 proto/bgp/bgp.c       |  7 ++++++-
 proto/rip/rip.c       |  4 ++++
 proto/static/static.c | 10 +++++++++-
 8 files changed, 87 insertions(+), 15 deletions(-)

diff --git a/nest/iface.c b/nest/iface.c
index 1059ae06..3d1483ac 100644
--- a/nest/iface.c
+++ b/nest/iface.c
@@ -262,7 +262,7 @@ if_enqueue_notify_to(struct iface_notification x, struct iface_subscription *s)
       break;
     case IFNOT_NEIGHBOR:
       if (!s->neigh_notify) return;
-      neigh_link(x.n);
+      neigh_link_locked(x.n);
       break;
     default:
       bug("Unknown interface notification type: %d", x.type);
@@ -583,9 +583,7 @@ iface_notify_hook(void *_s)
 	break;
       case IFNOT_NEIGHBOR:
 	s->neigh_notify(n->n);
-	IFACE_LOCK;
 	neigh_unlink(n->n);
-	IFACE_UNLOCK;
 	break;
       default:
 	bug("Bad interface notification type: %d", n->type);
@@ -658,8 +656,6 @@ iface_unsubscribe(struct iface_subscription *s)
   IFACE_LOCK;
 
   SKIP_BACK_DECLARE(struct proto, p, iface_sub, s);
-  WALK_TLIST_DELSAFE(proto_neigh, n, &p->neighbors)
-    neigh_unlink(n);
 
   ifsub_rem_node(&iface_sub_list, s);
   ev_postpone(&s->event);
@@ -676,7 +672,7 @@ iface_unsubscribe(struct iface_subscription *s)
 	if_unlink(n->i);
 	break;
       case IFNOT_NEIGHBOR:
-	neigh_unlink(n->n);
+	neigh_unlink_locked(n->n);
 	break;
       default:
 	bug("Bad interface notification type: %d", n->type);
@@ -686,6 +682,12 @@ iface_unsubscribe(struct iface_subscription *s)
     sl_free(n);
   }
 
+  WALK_TLIST_DELSAFE(proto_neigh, n, &p->neighbors)
+  {
+    log(L_WARN "%s: Unlinking forgotten neighbor %I", p->name, n->addr);
+    neigh_unlink_locked(n);
+  }
+
   ASSERT_DIE(EMPTY_TLIST(proto_neigh, &p->neighbors));
 
   IFACE_UNLOCK;
diff --git a/nest/iface.h b/nest/iface.h
index a98bdf37..699cffc2 100644
--- a/nest/iface.h
+++ b/nest/iface.h
@@ -10,6 +10,7 @@
 #define _BIRD_IFACE_H_
 
 #include "lib/locking.h"
+#include "lib/defer.h"
 #include "lib/event.h"
 #include "lib/lists.h"
 #include "lib/tlists.h"
@@ -178,6 +179,27 @@ void neigh_init(struct pool *);
 void neigh_link(neighbor *);
 void neigh_unlink(neighbor *);
 
+struct neigh_unlink_deferred {
+  struct deferred_call dc;
+  neighbor *n;
+};
+
+void neigh_unlink_deferred(struct deferred_call *dc);
+
+static inline void neigh_unlink_later(neighbor *n)
+{
+  struct neigh_unlink_deferred nud = {
+    .dc.hook = neigh_unlink_deferred,
+    .n = n,
+  };
+
+  defer_call(&nud.dc, sizeof nud);
+}
+
+/* For internal use */
+void neigh_link_locked(neighbor *);
+void neigh_unlink_locked(neighbor *);
+
 /*
  *	Notification mechanism
  */
diff --git a/nest/neighbor.c b/nest/neighbor.c
index 991ed0ea..257e61c4 100644
--- a/nest/neighbor.c
+++ b/nest/neighbor.c
@@ -283,7 +283,8 @@ neigh_find(struct proto *p, ip_addr a, struct iface *iface, uint flags)
   n->flags = flags;
   n->scope = scope;
 
-  neigh_link(n);
+  neigh_link_locked(n);
+  neigh_unlink_later(n);
 
   IFACE_UNLOCK;
   return n;
@@ -378,14 +379,14 @@ neigh_down(neighbor *n)
 }
 
 void
-neigh_link(neighbor *n)
+neigh_link_locked(neighbor *n)
 {
   IFACE_ASSERT_LOCKED;
   n->uc++;
 }
 
 void
-neigh_unlink(neighbor *n)
+neigh_unlink_locked(neighbor *n)
 {
   IFACE_ASSERT_LOCKED;
   if (--n->uc)
@@ -409,6 +410,27 @@ neigh_unlink(neighbor *n)
   sl_free(n);
 }
 
+void
+neigh_link(neighbor *n)
+{
+  IFACE_LOCK;
+  neigh_link_locked(n);
+  IFACE_UNLOCK;
+}
+
+void
+neigh_unlink(neighbor *n)
+{
+  IFACE_LOCK;
+  neigh_unlink_locked(n);
+  IFACE_UNLOCK;
+}
+
+void neigh_unlink_deferred(struct deferred_call *dc)
+{
+  neigh_unlink(SKIP_BACK(struct neigh_unlink_deferred, dc, dc)->n);
+}
+
 /**
  * neigh_update: update neighbor entry w.r.t. change on specific iface
  * @n: neighbor to update
@@ -472,7 +494,7 @@ neigh_update(neighbor *n, struct iface *iface)
 
   if ((n->scope < 0) && !(n->flags & NEF_STICKY))
   {
-    neigh_unlink(n);
+    neigh_unlink_locked(n);
     return;
   }
 
diff --git a/nest/proto.c b/nest/proto.c
index 19593533..dee72eb2 100644
--- a/nest/proto.c
+++ b/nest/proto.c
@@ -1291,13 +1291,17 @@ proto_event(void *ptr)
 
   if (p->do_stop)
   {
-    iface_unsubscribe(&p->iface_sub);
-
     p->do_stop = 0;
   }
 
   if (proto_is_done(p) && p->pool_inloop)  /* perusing pool_inloop to do this once only */
   {
+    /* Interface notification unsubscribe can't be done
+     * before the protocol is really done, as it also destroys
+     * the neighbors which may be needed (e.g. by BGP->MRT)
+     * during the STOP phase as well. */
+    iface_unsubscribe(&p->iface_sub);
+
     rp_free(p->pool_inloop);
     p->pool_inloop = NULL;
     if (p->loop != &main_birdloop)
diff --git a/proto/bfd/bfd.c b/proto/bfd/bfd.c
index e6bf3f39..923381e9 100644
--- a/proto/bfd/bfd.c
+++ b/proto/bfd/bfd.c
@@ -994,6 +994,8 @@ bfd_start_neighbor(struct bfd_proto *p, struct bfd_neighbor *n)
     return;
   }
 
+  neigh_link(nb);
+
   n->neigh = nb;
   nb->data = n;
 
@@ -1007,8 +1009,11 @@ static void
 bfd_stop_neighbor(struct bfd_proto *p UNUSED, struct bfd_neighbor *n)
 {
   if (n->neigh)
+  {
     n->neigh->data = NULL;
-  n->neigh = NULL;
+    neigh_unlink(n->neigh);
+    n->neigh = NULL;
+  }
 
   rfree(n->req);
   n->req = NULL;
diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c
index 1a284df8..6dd12149 100644
--- a/proto/bgp/bgp.c
+++ b/proto/bgp/bgp.c
@@ -588,7 +588,11 @@ bgp_down(struct bgp_proto *p)
     bgp_close(p);
   }
 
-  p->neigh = NULL;
+  if (p->neigh)
+  {
+    neigh_unlink(p->neigh);
+    p->neigh = NULL;
+  }
 
   BGP_TRACE(D_EVENTS, "Down");
   proto_notify_state(&p->p, PS_DOWN);
@@ -1749,6 +1753,7 @@ bgp_start_locked(void *_p)
   }
 
   p->neigh = n;
+  neigh_link(n);
 
   if (n->scope <= 0)
     BGP_TRACE(D_EVENTS, "Waiting for %I%J to become my neighbor", p->remote_ip, cf->iface);
diff --git a/proto/rip/rip.c b/proto/rip/rip.c
index 1d4e6cf5..f62ec667 100644
--- a/proto/rip/rip.c
+++ b/proto/rip/rip.c
@@ -454,6 +454,8 @@ rip_get_neighbor(struct rip_proto *p, ip_addr *a, struct rip_iface *ifa)
   struct rip_neighbor *n = mb_allocz(p->p.pool, sizeof(struct rip_neighbor));
   n->ifa = ifa;
   n->nbr = nbr;
+  neigh_link(nbr);
+
   nbr->data = n;
   n->csn = nbr->aux;
 
@@ -475,6 +477,8 @@ rip_remove_neighbor(struct rip_proto *p, struct rip_neighbor *n)
   nbr->data = NULL;
   nbr->aux = n->csn;
 
+  neigh_unlink(nbr);
+
   rfree(n->bfd_req);
   n->bfd_req = NULL;
   n->last_seen = 0;
diff --git a/proto/static/static.c b/proto/static/static.c
index c8fdcc37..d091924b 100644
--- a/proto/static/static.c
+++ b/proto/static/static.c
@@ -302,6 +302,8 @@ static_add_rte(struct static_proto *p, struct static_route *r)
 	continue;
       }
 
+      neigh_link(n);
+
       r2->neigh = n;
       r2->chain = n->data;
       n->data = r2;
@@ -321,7 +323,13 @@ static_reset_rte(struct static_proto *p UNUSED, struct static_route *r)
 
   for (r2 = r; r2; r2 = r2->mp_next)
   {
-    r2->neigh = NULL;
+    if (r2->neigh)
+    {
+      r2->neigh->data = NULL;
+      neigh_unlink(r2->neigh);
+      r2->neigh = NULL;
+    }
+
     r2->chain = NULL;
 
     r2->state = 0;