From: Ondrej Zajicek <santiago@crfreenet.org>
Date: Thu, 6 Mar 2025 02:43:15 +0000 (+0100)
Subject: Nest: Fix locking of tables by channels
X-Git-Tag: v3.1.0~36^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2679de7a39761385411d0e3d21a5d2e33085bd41;p=thirdparty%2Fbird.git

Nest: Fix locking of tables by channels

Channels that are down keep ptr on routing tables but do not keep them
locked. It is safe because the existence of tables depend on being
configured. But when a table is removed during reconfiguration, the
channel kept a dangling pointer since it fell down until it was removed.
This could be triggered by 'show protocols all' and other similar.

Change locking so that a channel kept a table locked for its whole
existence. The same change is already in BIRD 3.

Note that this is somewhat conceptually problematic as downed channels
do not keep resources. Also, other objects in specialized channels
(igp_table, base_table in bgp_channel, mpls_domain / mpls_range in
mpls_channel) are still locked only when channel is active, but for
them it is easier to keep track that they are not accessed when
they are deconfigured.
---

diff --git a/nest/proto.c b/nest/proto.c
index e4caffe1d..14260d633 100644
--- a/nest/proto.c
+++ b/nest/proto.c
@@ -165,6 +165,7 @@ proto_add_channel(struct proto *p, struct channel_config *cf)
   c->channel = cf->channel;
   c->proto = p;
   c->table = cf->table->table;
+  rt_lock_table(c->table);
 
   c->in_filter = cf->in_filter;
   c->out_filter = cf->out_filter;
@@ -204,6 +205,7 @@ proto_remove_channel(struct proto *p UNUSED, struct channel *c)
 
   CD(c, "Removed", c->name);
 
+  rt_unlock_table(c->table);
   rem_node(&c->n);
   mb_free(c);
 }
@@ -550,7 +552,6 @@ channel_setup_out_table(struct channel *c)
 static void
 channel_do_start(struct channel *c)
 {
-  rt_lock_table(c->table);
   add_tail(&c->table->channels, &c->table_node);
   c->proto->active_channels++;
 
@@ -604,7 +605,6 @@ channel_do_down(struct channel *c)
   ASSERT(!c->feed_active && !c->reload_active);
 
   rem_node(&c->table_node);
-  rt_unlock_table(c->table);
   c->proto->active_channels--;
 
   if ((c->stats.imp_routes + c->stats.filt_routes) != 0)
diff --git a/nest/protocol.h b/nest/protocol.h
index 32f2c2e8a..97a2ba406 100644
--- a/nest/protocol.h
+++ b/nest/protocol.h
@@ -578,7 +578,7 @@ struct channel {
  *
  * CS_DOWN - The initial and the final state of a channel. There is no route
  * exchange between the protocol and the table. Channel is not counted as
- * active. Channel keeps a ptr to the table, but do not lock the table and is
+ * active. Channel keeps a ptr to the table, keeps the table locked, but is
  * not linked in the table. Generally, new closed channels are created in
  * protocols' init() hooks. The protocol is expected to explicitly activate its
  * channels (by calling channel_init() or channel_open()).
@@ -602,8 +602,8 @@ struct channel {
  * channel. The channel is still initialized, but no route exchange is allowed.
  * Instead, the associated table is running flush loop to remove routes imported
  * through the channel. After that, the channel changes state to CS_DOWN and
- * is detached from the table (the table is unlocked and the channel is unlinked
- * from it). Unlike other states, the CS_FLUSHING state is not explicitly
+ * is detached from the table (the channel is unlinked from it, but keeps the
+ * table locked). Unlike other states, the CS_FLUSHING state is not explicitly
  * entered or left by the protocol. A protocol may request to close a channel
  * (by calling channel_close()), which causes the channel to change state to
  * CS_FLUSHING and later to CS_DOWN. Also note that channels are closed
diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c
index e6e01e7ba..e17f47ae0 100644
--- a/proto/bgp/bgp.c
+++ b/proto/bgp/bgp.c
@@ -2733,14 +2733,18 @@ bgp_show_proto_info(struct proto *P)
 	  cli_msg(-1006, "    BGP Next hop:   %I %I", c->next_hop_addr, c->link_addr);
       }
 
-      if (c->igp_table_ip4)
-	cli_msg(-1006, "    IGP IPv4 table: %s", c->igp_table_ip4->name);
+      /* After channel is deconfigured, these pointers are no longer valid */
+      if (!p->p.reconfiguring || (c->c.channel_state != CS_DOWN))
+      {
+	if (c->igp_table_ip4)
+	  cli_msg(-1006, "    IGP IPv4 table: %s", c->igp_table_ip4->name);
 
-      if (c->igp_table_ip6)
-	cli_msg(-1006, "    IGP IPv6 table: %s", c->igp_table_ip6->name);
+	if (c->igp_table_ip6)
+	  cli_msg(-1006, "    IGP IPv6 table: %s", c->igp_table_ip6->name);
 
-      if (c->base_table)
-	cli_msg(-1006, "    Base table:     %s", c->base_table->name);
+	if (c->base_table)
+	  cli_msg(-1006, "    Base table:     %s", c->base_table->name);
+      }
     }
   }
 }