path: root/net/bird3/files/patch-04-BFD-Fix-session-locking-order

From 83495362789d961914c4bfaa590e31cb17370ed0 Mon Sep 17 00:00:00 2001
From: Maria Matejka <mq@ucw.cz>
Date: Sat, 21 Dec 2024 19:02:22 +0100
Subject: [PATCH] BFD: Fix session reconfiguration locking order

The sessions have to be updated asynchronously to avoid
cross-locking between protocols.

Testsuite: cf-ibgp-bfd-switch, cf-ibgp-multi-bfd-auth
Fixes: #139

Thanks to Daniel Suchy <danny@danysek.cz> for reporting:
https://trubka.network.cz/pipermail/bird-users/2024-December/017984.html
---
 nest/bfd.h          |   7 ++-
 proto/bfd/bfd.c     | 144 +++++++++++++++++++++++---------------------
 proto/bfd/bfd.h     |  21 +------
 proto/bfd/config.Y  |  42 +++++--------
 proto/bfd/packets.c |   4 +-
 5 files changed, 98 insertions(+), 120 deletions(-)

diff --git a/nest/bfd.h b/nest/bfd.h
index 5dacff5d7..c046152f8 100644
--- nest/bfd.h
+++ nest/bfd.h
@@ -18,8 +18,11 @@ struct bfd_options {
   u32 min_tx_int;
   u32 idle_tx_int;
   u8 multiplier;
-  u8 passive;
-  u8 passive_set;
+  PACKED enum bfd_opt_passive {
+    BFD_OPT_PASSIVE_UNKNOWN = 0,
+    BFD_OPT_PASSIVE,
+    BFD_OPT_NOT_PASSIVE,
+  } passive;
   u8 mode;
   u8 auth_type;				/* Authentication type (BFD_AUTH_*) */
   list *passwords;			/* Passwords for authentication */
diff --git a/proto/bfd/bfd.c b/proto/bfd/bfd.c
index 34f992b93..4997f803a 100644
--- proto/bfd/bfd.c
+++ proto/bfd/bfd.c
@@ -172,17 +172,17 @@ static void bfd_free_iface(struct bfd_iface *ifa);
  *	BFD sessions
  */
 
-static inline struct bfd_session_config
-bfd_merge_options(const struct bfd_iface_config *cf, const struct bfd_options *opts)
+static inline struct bfd_options
+bfd_merge_options(const struct bfd_options *bottom, const struct bfd_options *top)
 {
-  return (struct bfd_session_config) {
-    .min_rx_int = opts->min_rx_int ?: cf->min_rx_int,
-    .min_tx_int = opts->min_tx_int ?: cf->min_tx_int,
-    .idle_tx_int = opts->idle_tx_int ?: cf->idle_tx_int,
-    .multiplier = opts->multiplier ?: cf->multiplier,
-    .passive = opts->passive_set ? opts->passive : cf->passive,
-    .auth_type = opts->auth_type ?: cf->auth_type,
-    .passwords = opts->passwords ?: cf->passwords,
+  return (struct bfd_options) {
+    .min_rx_int = top->min_rx_int ?: bottom->min_rx_int,
+    .min_tx_int = top->min_tx_int ?: bottom->min_tx_int,
+    .idle_tx_int = top->idle_tx_int ?: bottom->idle_tx_int,
+    .multiplier = top->multiplier ?: bottom->multiplier,
+    .passive = top->passive ?: bottom->passive,
+    .auth_type = top->auth_type ?: bottom->auth_type,
+    .passwords = top->passwords ?: bottom->passwords,
   };
 }
 
@@ -478,7 +478,7 @@ bfd_add_session(struct bfd_proto *p, ip_addr addr, ip_addr local, struct iface *
   HASH_INSERT(p->session_hash_id, HASH_ID, s);
   HASH_INSERT(p->session_hash_ip, HASH_IP, s);
 
-  s->cf = bfd_merge_options(ifa->cf, opts);
+  s->cf = bfd_merge_options(&ifa->cf->opts, opts);
 
   /* Initialization of state variables - see RFC 5880 6.8.1 */
   s->loc_state = BFD_STATE_DOWN;
@@ -561,26 +561,58 @@ bfd_remove_session(struct bfd_proto *p, struct bfd_session *s)
   birdloop_leave(p->p.loop);
 }
 
+struct bfd_reconfigure_sessions_deferred_call {
+  struct deferred_call dc;
+  struct bfd_proto *p;
+  config_ref old_config;
+};
+
 static void
-bfd_reconfigure_session(struct bfd_proto *p, struct bfd_session *s)
+bfd_reconfigure_sessions(struct deferred_call *dc)
 {
-  if (EMPTY_LIST(s->request_list))
-    return;
+  SKIP_BACK_DECLARE(struct bfd_reconfigure_sessions_deferred_call,
+      brsdc, dc, dc);
 
-  ASSERT_DIE(birdloop_inside(p->p.loop));
+  struct bfd_proto *p = brsdc->p;
+  birdloop_enter(p->p.loop);
 
-  SKIP_BACK_DECLARE(struct bfd_request, req, n, HEAD(s->request_list));
-  s->cf = bfd_merge_options(s->ifa->cf, &req->opts);
+  HASH_WALK(p->session_hash_id, next_id, s)
+  {
+    if (!EMPTY_LIST(s->request_list))
+    {
+      SKIP_BACK_DECLARE(struct bfd_request, req, n, HEAD(s->request_list));
+      struct bfd_options opts = bfd_merge_options(&s->ifa->cf->opts, &req->opts);
 
-  u32 tx = (s->loc_state == BFD_STATE_UP) ? s->cf.min_tx_int : s->cf.idle_tx_int;
-  bfd_session_set_min_tx(s, tx);
-  bfd_session_set_min_rx(s, s->cf.min_rx_int);
-  s->detect_mult = s->cf.multiplier;
-  s->passive = s->cf.passive;
+#define CHK(x)	(opts.x != s->cf.x) ||
+      bool reload = MACRO_FOREACH(CHK,
+	  min_rx_int,
+	  min_tx_int,
+	  idle_tx_int,
+	  multiplier,
+	  passive) false; /* terminating the || chain */
+#undef CHK
 
-  bfd_session_control_tx_timer(s, 0);
+      s->cf = opts;
+
+      if (reload)
+      {
+	u32 tx = (s->loc_state == BFD_STATE_UP) ? s->cf.min_tx_int : s->cf.idle_tx_int;
+	bfd_session_set_min_tx(s, tx);
+	bfd_session_set_min_rx(s, s->cf.min_rx_int);
+	s->detect_mult = s->cf.multiplier;
+	s->passive = s->cf.passive;
+
+	bfd_session_control_tx_timer(s, 0);
+
+	TRACE(D_EVENTS, "Session to %I reconfigured", s->addr);
+      }
+    }
+  }
+  HASH_WALK_END;
+  birdloop_leave(p->p.loop);
 
-  TRACE(D_EVENTS, "Session to %I reconfigured", s->addr);
+  /* Now the config is clean */
+  OBSREF_CLEAR(brsdc->old_config);
 }
 
 
@@ -589,10 +621,12 @@ bfd_reconfigure_session(struct bfd_proto *p, struct bfd_session *s)
  */
 
 static struct bfd_iface_config bfd_default_iface = {
-  .min_rx_int = BFD_DEFAULT_MIN_RX_INT,
-  .min_tx_int = BFD_DEFAULT_MIN_TX_INT,
-  .idle_tx_int = BFD_DEFAULT_IDLE_TX_INT,
-  .multiplier = BFD_DEFAULT_MULTIPLIER,
+  .opts = {
+    .min_rx_int = BFD_DEFAULT_MIN_RX_INT,
+    .min_tx_int = BFD_DEFAULT_MIN_TX_INT,
+    .idle_tx_int = BFD_DEFAULT_IDLE_TX_INT,
+    .multiplier = BFD_DEFAULT_MULTIPLIER,
+  },
 };
 
 static inline struct bfd_iface_config *
@@ -650,24 +684,6 @@ bfd_free_iface(struct bfd_iface *ifa)
   mb_free(ifa);
 }
 
-static void
-bfd_reconfigure_iface(struct bfd_proto *p UNUSED, struct bfd_iface *ifa, struct bfd_config *nc)
-{
-  struct bfd_iface_config *new = bfd_find_iface_config(nc, ifa->iface);
-  struct bfd_iface_config *old = ifa->cf;
-
-  /* Check options that are handled in bfd_reconfigure_session() */
-  ifa->changed =
-    (new->min_rx_int != old->min_rx_int) ||
-    (new->min_tx_int != old->min_tx_int) ||
-    (new->idle_tx_int != old->idle_tx_int) ||
-    (new->multiplier != old->multiplier) ||
-    (new->passive != old->passive);
-
-  /* This should be probably changed to not access ifa->cf from the BFD thread */
-  ifa->cf = new;
-}
-
 
 /*
  *	BFD requests
@@ -900,20 +916,7 @@ bfd_request_session(pool *p, ip_addr addr, ip_addr local,
 void
 bfd_update_request(struct bfd_request *req, const struct bfd_options *opts)
 {
-  struct bfd_session *s = req->session;
-
-  if (!memcmp(opts, &req->opts, sizeof(const struct bfd_options)))
-    return;
-
   req->opts = *opts;
-
-  if (s)
-  {
-    struct bfd_proto *p = s->ifa->bfd;
-    birdloop_enter(p->p.loop);
-    bfd_reconfigure_session(p, s);
-    birdloop_leave(p->p.loop);
-  }
 }
 
 static void
@@ -1193,21 +1196,22 @@ bfd_reconfigure(struct proto *P, struct proto_config *c)
       (new->zero_udp6_checksum_rx != old->zero_udp6_checksum_rx))
     return 0;
 
-  birdloop_mask_wakeups(p->p.loop);
-
   WALK_LIST(ifa, p->iface_list)
-    bfd_reconfigure_iface(p, ifa, new);
-
-  HASH_WALK(p->session_hash_id, next_id, s)
-  {
-    if (s->ifa->changed)
-      bfd_reconfigure_session(p, s);
-  }
-  HASH_WALK_END;
+    ifa->cf = bfd_find_iface_config(new, ifa->iface);
 
   bfd_reconfigure_neighbors(p, new);
 
-  birdloop_unmask_wakeups(p->p.loop);
+  /* Sessions get reconfigured after all the config is applied */
+  struct bfd_reconfigure_sessions_deferred_call brsdc = {
+    .dc.hook = bfd_reconfigure_sessions,
+    .p = p,
+  };
+  SKIP_BACK_DECLARE(struct bfd_reconfigure_sessions_deferred_call,
+      brsdcp, dc, defer_call(&brsdc.dc, sizeof brsdc));
+
+  /* We need to keep the old config alive until all the sessions get
+   * reconfigured */
+  OBSREF_SET(brsdcp->old_config, P->cf->global);
 
   return 1;
 }
diff --git a/proto/bfd/bfd.h b/proto/bfd/bfd.h
index 578ce8755..107829b72 100644
--- proto/bfd/bfd.h
+++ proto/bfd/bfd.h
@@ -54,24 +54,7 @@ struct bfd_config
 struct bfd_iface_config
 {
   struct iface_patt i;
-  u32 min_rx_int;
-  u32 min_tx_int;
-  u32 idle_tx_int;
-  u8 multiplier;
-  u8 passive;
-  u8 auth_type;				/* Authentication type (BFD_AUTH_*) */
-  list *passwords;			/* Passwords for authentication */
-};
-
-struct bfd_session_config
-{
-  u32 min_rx_int;
-  u32 min_tx_int;
-  u32 idle_tx_int;
-  u8 multiplier;
-  u8 passive;
-  u8 auth_type;				/* Authentication type (BFD_AUTH_*) */
-  list *passwords;			/* Passwords for authentication */
+  struct bfd_options opts;
 };
 
 struct bfd_neighbor
@@ -146,7 +129,7 @@ struct bfd_session
   u32 loc_id;				/* Local session ID (local discriminator) */
   u32 rem_id;				/* Remote session ID (remote discriminator) */
 
-  struct bfd_session_config cf;		/* Static configuration parameters */
+  struct bfd_options cf;		/* Static configuration parameters */
 
   u32 des_min_tx_int;			/* Desired min rx interval, local option */
   u32 des_min_tx_new;			/* Used for des_min_tx_int change */
diff --git a/proto/bfd/config.Y b/proto/bfd/config.Y
index 9e9919c4e..56d1ffac4 100644
--- proto/bfd/config.Y
+++ proto/bfd/config.Y
@@ -86,44 +86,37 @@ bfd_iface_start:
   add_tail(&BFD_CFG->patt_list, NODE this_ipatt);
   init_list(&this_ipatt->ipn_list);
 
-  BFD_IFACE->min_rx_int = BFD_DEFAULT_MIN_RX_INT;
-  BFD_IFACE->min_tx_int = BFD_DEFAULT_MIN_TX_INT;
-  BFD_IFACE->idle_tx_int = BFD_DEFAULT_IDLE_TX_INT;
-  BFD_IFACE->multiplier = BFD_DEFAULT_MULTIPLIER;
+  this_bfd_opts = &BFD_IFACE->opts;
+
+  this_bfd_opts->min_rx_int = BFD_DEFAULT_MIN_RX_INT;
+  this_bfd_opts->min_tx_int = BFD_DEFAULT_MIN_TX_INT;
+  this_bfd_opts->idle_tx_int = BFD_DEFAULT_IDLE_TX_INT;
+  this_bfd_opts->multiplier = BFD_DEFAULT_MULTIPLIER;
 
   reset_passwords();
 };
 
 bfd_iface_finish:
 {
-  BFD_IFACE->passwords = get_passwords();
+  this_bfd_opts->passwords = get_passwords();
 
-  if (!BFD_IFACE->auth_type != !BFD_IFACE->passwords)
+  if (!this_bfd_opts->auth_type != !this_bfd_opts->passwords)
     cf_warn("Authentication and password options should be used together");
 
-  if (BFD_IFACE->passwords)
+  if (this_bfd_opts->passwords)
   {
     struct password_item *pass;
-    WALK_LIST(pass, *BFD_IFACE->passwords)
+    WALK_LIST(pass, *this_bfd_opts->passwords)
     {
       if (pass->alg)
         cf_error("Password algorithm option not available in BFD protocol");
 
-      pass->alg = bfd_auth_type_to_hash_alg[BFD_IFACE->auth_type];
+      pass->alg = bfd_auth_type_to_hash_alg[this_bfd_opts->auth_type];
     }
   }
-};
 
-bfd_iface_item:
-   INTERVAL expr_us { BFD_IFACE->min_rx_int = BFD_IFACE->min_tx_int = $2; }
- | MIN RX INTERVAL expr_us { BFD_IFACE->min_rx_int = $4; }
- | MIN TX INTERVAL expr_us { BFD_IFACE->min_tx_int = $4; }
- | IDLE TX INTERVAL expr_us { BFD_IFACE->idle_tx_int = $4; }
- | MULTIPLIER expr { BFD_IFACE->multiplier = $2; }
- | PASSIVE bool { BFD_IFACE->passive = $2; }
- | AUTHENTICATION bfd_auth_type { BFD_IFACE->auth_type = $2; }
- | password_list {}
- ;
+  this_bfd_opts = NULL;
+};
 
 bfd_auth_type:
    NONE			 { $$ = BFD_AUTH_NONE; }
@@ -134,14 +127,9 @@ bfd_auth_type:
  | METICULOUS KEYED SHA1 { $$ = BFD_AUTH_METICULOUS_KEYED_SHA1; }
  ;
 
-bfd_iface_opts:
-   /* empty */
- | bfd_iface_opts bfd_iface_item ';'
- ;
-
 bfd_iface_opt_list:
    /* empty */
- | '{' bfd_iface_opts '}'
+ | '{' bfd_items '}'
  ;
 
 bfd_iface:
@@ -194,7 +182,7 @@ bfd_item:
  | MIN TX INTERVAL expr_us { this_bfd_opts->min_tx_int = $4; }
  | IDLE TX INTERVAL expr_us { this_bfd_opts->idle_tx_int = $4; }
  | MULTIPLIER expr { this_bfd_opts->multiplier = $2; }
- | PASSIVE bool { this_bfd_opts->passive = $2; this_bfd_opts->passive_set = 1; }
+ | PASSIVE bool { this_bfd_opts->passive = $2 ? BFD_OPT_PASSIVE : BFD_OPT_NOT_PASSIVE; }
  | GRACEFUL { this_bfd_opts->mode = BGP_BFD_GRACEFUL; }
  | AUTHENTICATION bfd_auth_type { this_bfd_opts->auth_type = $2; }
  | password_list {}
diff --git a/proto/bfd/packets.c b/proto/bfd/packets.c
index 1ceb470c1..f8bd63d73 100644
--- proto/bfd/packets.c
+++ proto/bfd/packets.c
@@ -109,7 +109,7 @@ const u8 bfd_auth_type_to_hash_alg[] = {
 static void
 bfd_fill_authentication(struct bfd_proto *p, struct bfd_session *s, struct bfd_ctl_packet *pkt)
 {
-  struct bfd_session_config *cf = &s->cf;
+  struct bfd_options *cf = &s->cf;
   struct password_item *pass = password_find(cf->passwords, 0);
   uint meticulous = 0;
 
@@ -179,7 +179,7 @@ bfd_fill_authentication(struct bfd_proto *p, struct bfd_session *s, struct bfd_c
 static int
 bfd_check_authentication(struct bfd_proto *p, struct bfd_session *s, struct bfd_ctl_packet *pkt)
 {
-  struct bfd_session_config *cf = &s->cf;
+  struct bfd_options *cf = &s->cf;
   const char *err_dsc = NULL;
   uint err_val = 0;
   uint auth_type = 0;
-- 
GitLab