BFD notifications respect protocol loop settings

6 files changed, 58 insertions, 18 deletions

diff --git a/nest/bfd.h b/nest/bfd.h
index 37561266..e8977dbd 100644
--- a/nest/bfd.h
+++ b/nest/bfd.h
@@ -35,6 +35,7 @@ struct bfd_request {
 
   void (*hook)(struct bfd_request *);
   void *data;
+  struct birdloop *target;
 
   struct bfd_session *session;
 
@@ -57,14 +58,14 @@ static inline struct bfd_options * bfd_new_options(void)
 
 #ifdef CONFIG_BFD
 
-struct bfd_request * bfd_request_session(pool *p, ip_addr addr, ip_addr local, struct iface *iface, struct iface *vrf, void (*hook)(struct bfd_request *), void *data, const struct bfd_options *opts);
+struct bfd_request * bfd_request_session(pool *p, ip_addr addr, ip_addr local, struct iface *iface, struct iface *vrf, void (*hook)(struct bfd_request *), void *data, struct birdloop *target, const struct bfd_options *opts);
 void bfd_update_request(struct bfd_request *req, const struct bfd_options *opts);
 
 static inline void cf_check_bfd(int use UNUSED) { }
 
 #else
 
-static inline struct bfd_request * bfd_request_session(pool *p UNUSED, ip_addr addr UNUSED, ip_addr local UNUSED, struct iface *iface UNUSED, struct iface *vrf UNUSED, void (*hook)(struct bfd_request *) UNUSED, void *data UNUSED, const struct bfd_options *opts UNUSED) { return NULL; }
+static inline struct bfd_request * bfd_request_session(pool *p UNUSED, ip_addr addr UNUSED, ip_addr local UNUSED, struct iface *iface UNUSED, struct iface *vrf UNUSED, void (*hook)(struct bfd_request *) UNUSED, void *data UNUSED, struct birdloop *target UNUSED, const struct bfd_options *opts UNUSED) { return NULL; }
 static inline void bfd_update_request(struct bfd_request *req UNUSED, const struct bfd_options *opts UNUSED) { };
 
 static inline void cf_check_bfd(int use) { if (use) cf_error("BFD not available"); }
diff --git a/proto/bfd/bfd.c b/proto/bfd/bfd.c
index 575ebc3c..59134a22 100644
--- a/proto/bfd/bfd.c
+++ b/proto/bfd/bfd.c
@@ -657,7 +657,17 @@ bfd_request_notify(struct bfd_request *req, u8 state, u8 diag)
   req->down = (old_state == BFD_STATE_UP) && (state == BFD_STATE_DOWN);
 
   if (req->hook)
+  {
+    struct birdloop *target = !birdloop_inside(req->target) ? req->target : NULL;
+
+    if (target)
+      birdloop_enter(target);
+
     req->hook(req);
+
+    if (target)
+      birdloop_leave(target);
+  }
 }
 
 static int
@@ -676,7 +686,6 @@ bfd_add_request(struct bfd_proto *p, struct bfd_request *req)
 
   uint ifindex = req->iface ? req->iface->index : 0;
   struct bfd_session *s = bfd_find_session_by_addr(p, req->addr, ifindex);
-  u8 state, diag;
 
   if (!s)
     s = bfd_add_session(p, req->addr, req->local, req->iface, &req->opts);
@@ -686,11 +695,15 @@ bfd_add_request(struct bfd_proto *p, struct bfd_request *req)
   req->session = s;
 
   bfd_lock_sessions(p);
-  state = s->loc_state;
-  diag = s->loc_diag;
+
+  int notify = !NODE_VALID(&s->n);
+  if (notify)
+    add_tail(&p->notify_list, &s->n);
+
   bfd_unlock_sessions(p);
 
-  bfd_request_notify(req, state, diag);
+  if (notify)
+    ev_send(&global_event_list, &p->notify_event);
 
   return 1;
 }
@@ -698,6 +711,26 @@ bfd_add_request(struct bfd_proto *p, struct bfd_request *req)
 static void
 bfd_pickup_requests(void *_data UNUSED)
 {
+  /* NOTE TO MY FUTURE SELF
+   *
+   * Functions bfd_take_requests() and bfd_drop_requests() need to have
+   * consistent &bfd_global.wait_list and this is ensured only by having these
+   * functions called from bfd_start() and bfd_shutdown() which are both called
+   * in PROTO_LOCKED_FROM_MAIN context, i.e. always from &main_birdloop.
+   *
+   * This pickup event is also called in &main_birdloop, therefore we can
+   * freely do BFD_LOCK/BFD_UNLOCK while processing all the requests. All BFD
+   * protocols capable of bfd_add_request() are either started before this code
+   * happens or after that.
+   *
+   * If BFD protocols could start in parallel with this routine, they might
+   * miss some of the waiting requests, thus if anybody tries to start
+   * protocols or run this pickup event outside &main_birdloop in future, they
+   * shall ensure that this race condition is mitigated somehow.
+   *
+   * Thank you, my future self, for understanding. Have a nice day!
+   */
+
   node *n;
   WALK_LIST(n, bfd_global.proto_list)
   {
@@ -714,12 +747,16 @@ bfd_pickup_requests(void *_data UNUSED)
   }
 
   BFD_LOCK;
-  node *rn, *rnxt;
-  WALK_LIST_DELSAFE(rn, rnxt, bfd_global.pickup_list)
+  while (!EMPTY_LIST(bfd_global.pickup_list))
   {
-    rem_node(rn);
-    add_tail(&bfd_global.wait_list, rn);
-    bfd_request_notify(SKIP_BACK(struct bfd_request, n, rn), BFD_STATE_ADMIN_DOWN, 0);
+    struct bfd_request *req = SKIP_BACK(struct bfd_request, n, HEAD(bfd_global.pickup_list));
+    rem_node(&req->n);
+    BFD_UNLOCK;
+
+    bfd_request_notify(req, BFD_STATE_ADMIN_DOWN, 0);
+
+    BFD_LOCK;
+    add_tail(&bfd_global.wait_list, &req->n);
   }
   BFD_UNLOCK;
 }
@@ -749,7 +786,6 @@ bfd_drop_requests(struct bfd_proto *p)
       rem_node(&req->n);
       add_tail(&bfd_global.pickup_list, &req->n);
       req->session = NULL;
-      bfd_request_notify(req, BFD_STATE_ADMIN_DOWN, 0);
     }
 
     ev_send(&global_event_list, &bfd_pickup_event);
@@ -766,6 +802,7 @@ struct bfd_request *
 bfd_request_session(pool *p, ip_addr addr, ip_addr local,
 		    struct iface *iface, struct iface *vrf,
 		    void (*hook)(struct bfd_request *), void *data,
+		    struct birdloop *target,
 		    const struct bfd_options *opts)
 {
   struct bfd_request *req = ralloc(p, &bfd_request_class);
@@ -778,8 +815,10 @@ bfd_request_session(pool *p, ip_addr addr, ip_addr local,
   if (opts)
     req->opts = *opts;
 
+  ASSERT_DIE(target || !hook);
   req->hook = hook;
   req->data = data;
+  req->target = target;
 
   req->session = NULL;
 
@@ -854,7 +893,7 @@ bfd_neigh_notify(struct neighbor *nb)
   if ((nb->scope > 0) && !n->req)
   {
     ip_addr local = ipa_nonzero(n->local) ? n->local : nb->ifa->ip;
-    n->req = bfd_request_session(p->p.pool, n->addr, local, nb->iface, p->p.vrf, NULL, NULL, NULL);
+    n->req = bfd_request_session(p->p.pool, n->addr, local, nb->iface, p->p.vrf, NULL, NULL, NULL, NULL);
   }
 
   if ((nb->scope <= 0) && n->req)
@@ -871,7 +910,7 @@ bfd_start_neighbor(struct bfd_proto *p, struct bfd_neighbor *n)
 
   if (n->multihop)
   {
-    n->req = bfd_request_session(p->p.pool, n->addr, n->local, NULL, p->p.vrf, NULL, NULL, NULL);
+    n->req = bfd_request_session(p->p.pool, n->addr, n->local, NULL, p->p.vrf, NULL, NULL, NULL, NULL);
     return;
   }
 
diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c
index f350c8ca..104e5862 100644
--- a/proto/bgp/bgp.c
+++ b/proto/bgp/bgp.c
@@ -1469,7 +1469,7 @@ bgp_update_bfd(struct bgp_proto *p, const struct bfd_options *bfd)
   if (bfd && !p->bfd_req && !bgp_is_dynamic(p))
     p->bfd_req = bfd_request_session(p->p.pool, p->remote_ip, p->local_ip,
 				     p->cf->multihop ? NULL : p->neigh->iface,
-				     p->p.vrf, bgp_bfd_notify, p, bfd);
+				     p->p.vrf, bgp_bfd_notify, p, p->p.loop, bfd);
 
   if (!bfd && p->bfd_req)
   {
diff --git a/proto/ospf/neighbor.c b/proto/ospf/neighbor.c
index ca369819..b0fdc42f 100644
--- a/proto/ospf/neighbor.c
+++ b/proto/ospf/neighbor.c
@@ -777,7 +777,7 @@ ospf_neigh_update_bfd(struct ospf_neighbor *n, int use_bfd)
   if (use_bfd && !n->bfd_req)
     n->bfd_req = bfd_request_session(n->pool, n->ip, n->ifa->addr->ip,
 				     n->ifa->iface, p->p.vrf,
-				     ospf_neigh_bfd_hook, n, NULL);
+				     ospf_neigh_bfd_hook, n, p->p.loop, NULL);
 
   if (!use_bfd && n->bfd_req)
   {
diff --git a/proto/rip/rip.c b/proto/rip/rip.c
index 97d1dd80..2c504112 100644
--- a/proto/rip/rip.c
+++ b/proto/rip/rip.c
@@ -543,7 +543,7 @@ rip_update_bfd(struct rip_proto *p, struct rip_neighbor *n)
     ip_addr saddr = rip_is_v2(p) ? n->ifa->sk->saddr : n->nbr->ifa->ip;
     n->bfd_req = bfd_request_session(p->p.pool, n->nbr->addr, saddr,
 				     n->nbr->iface, p->p.vrf,
-				     rip_bfd_notify, n, NULL);
+				     rip_bfd_notify, n, p->p.loop, NULL);
   }
 
   if (!use_bfd && n->bfd_req)
diff --git a/proto/static/static.c b/proto/static/static.c
index 82fbfe7a..86142790 100644
--- a/proto/static/static.c
+++ b/proto/static/static.c
@@ -213,7 +213,7 @@ static_update_bfd(struct static_proto *p, struct static_route *r)
     // ip_addr local = ipa_nonzero(r->local) ? r->local : nb->ifa->ip;
     r->bfd_req = bfd_request_session(p->p.pool, r->via, nb->ifa->ip,
 				     nb->iface, p->p.vrf,
-				     static_bfd_notify, r, NULL);
+				     static_bfd_notify, r, p->p.loop, NULL);
   }
 
   if (!bfd_up && r->bfd_req)