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 d577820eec7a4569928525a2c77e0bbef274085a..a333786de04838314f888b95431a463f0bd550cc 100644 (file)
--- a/nest/protocol.h
+++ b/nest/protocol.h
@@ -571,7 +571,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()).
@@ -595,8 +595,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 90c565aea8e2c465cfa7852991616c44910e11e8..e2e1d7b1577f5d0db7c4aca231026ea31a638e6b 100644 (file)
--- a/proto/bgp/bgp.c
+++ b/proto/bgp/bgp.c
@@ -2700,14 +2700,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);
+      }
     }
   }
 }