From: Maria Matejka <mq@ucw.cz>
Date: Wed, 20 Aug 2025 13:35:32 +0000 (+0200)
Subject: BGP: Fix dynamic instance reconfiguration
X-Git-Tag: v2.18~16
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=abc1eec433c9d639870c01e035b099a583857699;p=thirdparty%2Fbird.git

BGP: Fix dynamic instance reconfiguration

Every dynamic BGP was torn down on reconfig because the inherited
configuration is a little bit different than the parent one. Fixed this
by applying the same changes before the reconfiguration's memcmp().

This bug was introduced in 56af56a5ab8929bf211bdbe8fef28cb37c2cb7fa.

Also fixed interface pattern reconfiguration which always restarted.
Added not only comparison but also actual reconfiguration of the pattern
itself so that one can update the pattern without restarting a running
BGP session.

Finally, extended documentation a bit to cover dynamic BGP scenarios a
little bit better. Yet, it probably deserves a separate section on
dynamic BGP.
---

diff --git a/doc/bird.sgml b/doc/bird.sgml
index d75640c6b..ca039ef87 100644
--- a/doc/bird.sgml
+++ b/doc/bird.sgml
@@ -3059,7 +3059,14 @@ protocol bgp [<name>] {
 	instances are spawned for incoming BGP connections (if source address
 	matches the network prefix). It is possible to mix regular BGP instances
 	with dynamic BGP instances and have multiple dynamic BGP instances with
-	different ranges.
+        different ranges. These spawned dynamic BGP instances share the parent
+        configuration, therefore by reconfiguring the parent protocol forces
+        reconfiguration of the spawned protocols.
+
+        When the neighbor range is changed, all the spawned dynamic instances
+        shut down. Reconfiguration clears all dynamic instances which were
+        previously disabled by the <cf/disable/ CLI command. This may re-enable
+	connection of some clients otherwise blocked by the disabled instance.
 
 	<tag><label id="bgp-iface">interface "<m/text/"</tag>
 	Define interface we should use for link-local BGP IPv6 sessions.
@@ -3996,6 +4003,7 @@ direction (re-export of routes to the BGP neighbor):
 	<item><cf/require hostname/
 	<item><cf/require graceful restart/
 	<item><cf/require long lived graceful restart/
+	<item><cf/interface range/ if the already existing option is updated to another value.
 </itemize>
 
 <p>Channel options that cause a restart when changed:
@@ -4023,6 +4031,10 @@ incompatible with the current protocol state):
 	<item><cf/max long lived stale time/
 </itemize>
 
+<p>All these configuration changes apply also to the spawned dynamic BGP sessions
+and therefore reconfiguring the parent protocol may lead to shutdown of some or all
+of the dynamic BGP sessions.
+
 <sect1>Attributes
 <label id="bgp-attr">
 
diff --git a/nest/iface.c b/nest/iface.c
index 499822b64..a91f4ecf3 100644
--- a/nest/iface.c
+++ b/nest/iface.c
@@ -771,7 +771,7 @@ iface_patt_find(list *l, struct iface *i, struct ifa *a)
   return NULL;
 }
 
-static int
+int
 iface_plists_equal(struct iface_patt *pa, struct iface_patt *pb)
 {
   struct iface_patt_node *x, *y;
diff --git a/nest/iface.h b/nest/iface.h
index 1e4225eed..66af3d918 100644
--- a/nest/iface.h
+++ b/nest/iface.h
@@ -178,6 +178,7 @@ struct iface_patt {
 
 int iface_patt_match(struct iface_patt *ifp, struct iface *i, struct ifa *a);
 struct iface_patt *iface_patt_find(list *l, struct iface *i, struct ifa *a);
+int iface_plists_equal(struct iface_patt *pa, struct iface_patt *pb);
 int iface_patts_equal(list *, list *, int (*)(struct iface_patt *, struct iface_patt *));
 
 
diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c
index 862756f2a..d623eb4a8 100644
--- a/proto/bgp/bgp.c
+++ b/proto/bgp/bgp.c
@@ -1100,7 +1100,6 @@ bgp_spawn(struct bgp_proto *pp, sock *sk)
   cf->remote_ip = sk->daddr;
   cf->local_ip = sk->saddr;
   cf->iface = sk->iface;
-  cf->ipatt = NULL;
 
   struct bgp_proto *p = SKIP_BACK(struct bgp_proto, p, proto_spawn(sym->proto, 0));
   p->postponed_sk = sk;
@@ -1979,8 +1978,8 @@ bgp_start_neighbor(struct bgp_proto *p)
   bgp_initiate(p);
 }
 
-static void
-bgp_iface_update(struct bgp_proto *p, uint flags, struct iface *i)
+static bool
+bgp_iface_match(struct bgp_proto *p, struct iface *i)
 {
   int ps = p->p.proto_state;
 
@@ -1988,11 +1987,17 @@ bgp_iface_update(struct bgp_proto *p, uint flags, struct iface *i)
   ASSERT_DIE(p->cf->strict_bind);
 
   if ((ps == PS_DOWN) || (ps == PS_STOP))
-    return;
+    return false;
 
   if (!iface_patt_match(p->cf->ipatt, i, NULL))
-    return;
+    return false;
+
+  return true;
+}
 
+static void
+bgp_iface_update(struct bgp_proto *p, uint flags, struct iface *i)
+{
   struct bgp_socket_params params = {
     .iface = i,
     .vrf = p->p.vrf,
@@ -2026,7 +2031,8 @@ bgp_if_notify(struct proto *P, uint flags, struct iface *i)
 {
   struct bgp_proto *p = (struct bgp_proto *) P;
   ASSERT_DIE(ipa_zero(p->cf->local_ip));
-  bgp_iface_update(p, flags, i);
+  if (bgp_iface_match(p, i))
+    bgp_iface_update(p, flags, i);
 }
 
 static void
@@ -2035,10 +2041,33 @@ bgp_ifa_notify(struct proto *P, uint flags, struct ifa *i)
   struct bgp_proto *p = (struct bgp_proto *) P;
   ASSERT_DIE(!ipa_zero(p->cf->local_ip));
 
-  if (ipa_equal(i->ip, p->cf->local_ip))
+  if (ipa_equal(i->ip, p->cf->local_ip) && bgp_iface_match(p, i->iface))
     bgp_iface_update(p, flags, i->iface);
 }
 
+static void
+bgp_if_reload(struct bgp_proto *p, struct iface_patt *patt)
+{
+  struct iface *iface;
+  struct ifa *a;
+
+  WALK_LIST(iface, iface_list)
+  {
+    bool old = iface_patt_match(p->cf->ipatt, iface, NULL);
+    bool new = iface_patt_match(patt, iface, NULL);
+
+    if (old == new)
+      continue;
+
+    if (ipa_zero(p->cf->local_ip))
+      bgp_iface_update(p, old ? IF_CHANGE_DOWN : IF_CHANGE_UP, iface);
+    else
+      WALK_LIST(a, iface->addrs)
+	if (ipa_equal(a->ip, p->cf->local_ip))
+	  bgp_iface_update(p, old ? IF_CHANGE_DOWN : IF_CHANGE_UP, iface);
+  }
+}
+
 static void
 bgp_neigh_notify(neighbor *n)
 {
@@ -2317,6 +2346,10 @@ bgp_start(struct proto *P)
   /* Initialize listening socket list */
   init_list(&p->listen);
 
+  /* Setup interface notification hooks */
+  P->if_notify = (cf->ipatt && ipa_zero(cf->local_ip)) ? bgp_if_notify : NULL;
+  P->ifa_notify = (cf->ipatt && !ipa_zero(cf->local_ip)) ? bgp_ifa_notify : NULL;
+
   /* Initialize TCP-AO keys */
   init_list(&p->ao.keys);
   if (cf->auth_type == BGP_AUTH_AO)
@@ -2476,9 +2509,6 @@ bgp_init(struct proto_config *CF)
   p->remote_ip = cf->remote_ip;
   p->remote_as = cf->remote_as;
 
-  P->if_notify = (cf->ipatt && ipa_zero(cf->local_ip)) ? bgp_if_notify : NULL;
-  P->ifa_notify = (cf->ipatt && !ipa_zero(cf->local_ip)) ? bgp_ifa_notify : NULL;
-
   /* Hack: We use cf->remote_ip just to pass remote_ip from bgp_spawn() */
   if (cf->c.parent)
     cf->remote_ip = IPA_NONE;
@@ -2959,15 +2989,40 @@ static int
 bgp_reconfigure(struct proto *P, struct proto_config *CF)
 {
   struct bgp_proto *p = (void *) P;
-  const struct bgp_config *new = (void *) CF;
+  struct bgp_config *new = (void *) CF;
   const struct bgp_config *old = p->cf;
 
+  /* XXX: There is a section in documentation describing which configuration
+   * changes force BGP restart. When changing this function, you have to update
+   * also that part of documentation. */
+
   if (proto_get_router_id(CF) != p->local_id)
     return 0;
 
   if (bstrcmp(proto_get_hostname(CF), p->hostname))
     return 0;
 
+  /* Fix the virtual configuration so that memcpy does not fail */
+  if (old->c.parent)
+  {
+    new->remote_ip = old->remote_ip;
+    new->local_ip = old->local_ip;
+
+    /* Pre-check interfaces */
+    if (new->ipatt)
+    {
+      if (!old->iface || !iface_patt_match(new->ipatt, old->iface, NULL))
+	return 0;
+    }
+    else if (new->iface)
+    {
+      if (old->iface != new->iface)
+	return 0;
+    }
+
+    new->iface = old->iface;
+  }
+
   int same = !memcmp(((byte *) old) + sizeof(struct proto_config),
 		     ((byte *) new) + sizeof(struct proto_config),
 		     // password item is last and must be checked separately
@@ -2978,7 +3033,19 @@ bgp_reconfigure(struct proto *P, struct proto_config *CF)
     && !bstrcmp(old->dynamic_name, new->dynamic_name)
     && (old->dynamic_name_digits == new->dynamic_name_digits);
 
-  /* Reconfigure TCP-AP */
+  /* Reconfigure interface notification hooks */
+  same = same && (!P->if_notify == !(new->ipatt && ipa_zero(new->local_ip)));
+  same = same && (!P->ifa_notify == !(new->ipatt && !ipa_zero(new->local_ip)));
+
+  /* Differing pattern lists cause an update of the listening sockets
+   * and also if the connection is up, then active sockets. */
+  bool need_if_reload = same && new->ipatt && old->ipatt && !iface_plists_equal(new->ipatt, old->ipatt);
+  if (need_if_reload && !bgp_is_dynamic(p) && (
+	p->incoming_conn.sk && !iface_patt_match(new->ipatt, p->incoming_conn.sk->iface, NULL) ||
+	p->outgoing_conn.sk && !iface_patt_match(new->ipatt, p->outgoing_conn.sk->iface, NULL)))
+    same = 0;
+
+  /* Reconfigure TCP-AO */
   same = same && bgp_reconfigure_ao_keys(p, new);
 
   /* FIXME: Move channel reconfiguration to generic protocol code ? */
@@ -3026,6 +3093,9 @@ bgp_reconfigure(struct proto *P, struct proto_config *CF)
   if (p->start_state > BSS_PREPARE)
     bgp_update_bfd(p, new->bfd);
 
+  if (need_if_reload)
+    bgp_if_reload(p, new->ipatt);
+
   return 1;
 }
 
diff --git a/proto/bgp/bgp.h b/proto/bgp/bgp.h
index 7e70d745b..9a206cfe8 100644
--- a/proto/bgp/bgp.h
+++ b/proto/bgp/bgp.h
@@ -76,11 +76,12 @@ struct bgp_af_desc {
 
 struct bgp_config {
   struct proto_config c;
+  /* Beginning here, all options must be comparable by memcmp().
+   * Do not add anything above unless you want this comment to become futile. */
   u32 local_as, remote_as;
   ip_addr local_ip;			/* Source address to use */
   ip_addr remote_ip;
   struct iface *iface;			/* Interface for link-local addresses */
-  struct iface_patt *ipatt;		/* Interface pattern for dynamic strict bind */
   u16 local_port;			/* Local listening port */
   u16 remote_port; 			/* Neighbor destination port */
   int peer_type;			/* Internal or external BGP (BGP_PT_*, optional) */
@@ -147,6 +148,7 @@ struct bgp_config {
   const char *password;			/* Password used for MD5 authentication */
   struct ao_config *ao_keys;		/* Keys for TCP-AO authentication */
   net_addr *remote_range;		/* Allowed neighbor range for dynamic BGP */
+  struct iface_patt *ipatt;		/* Interface patterns for dynamic strict bind */
   const char *dynamic_name;		/* Name pattern for dynamic BGP */
   int dynamic_name_digits;		/* Minimum number of digits for dynamic names */
   int check_link;			/* Use iface link state for liveness detection */
diff --git a/proto/bgp/config.Y b/proto/bgp/config.Y
index c87295445..da28de596 100644
--- a/proto/bgp/config.Y
+++ b/proto/bgp/config.Y
@@ -168,7 +168,7 @@ bgp_proto:
      BGP_CFG->remote_range = n;
    }
  | bgp_proto INTERFACE text ';' { BGP_CFG->iface = if_get_by_name($3); }
- | bgp_proto INTERFACE RANGE iface_patt ';' { BGP_CFG->ipatt = this_ipatt; }
+ | bgp_proto INTERFACE RANGE iface_patt ';' { BGP_CFG->ipatt =  this_ipatt; }
  | bgp_proto ONLINK bool ';' { BGP_CFG->onlink = $3; }
  | bgp_proto RR CLUSTER ID idval ';' { BGP_CFG->rr_cluster_id = $5; }
  | bgp_proto RR CLIENT bool ';' { BGP_CFG->rr_client = $4; }