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 e4caffe1dc120e7c62b2fcab90d7c770f223bc52..14260d633d50079b233647169775049490bf8a3a 100644 (file)
--- 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 32f2c2e8ab88a35f9935438b3d75cfcee7b2167e..97a2ba4065ad18365fd565395c20385f6de77da7 100644 (file)
--- 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 e6e01e7ba06a697b5b21afb708649ac548b399e9..e17f47ae05ac7169b1efa277a40b62359ab12891 100644 (file)
--- 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);
+      }
     }
   }
 }