From: Maria Matejka <mq@ucw.cz>
Date: Wed, 21 May 2025 09:06:01 +0000 (+0200)
Subject: BGP: fix warnings for roa_check() with no import table and route refresh on
X-Git-Tag: v3.0.4~6
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c324949b20d3a562f61adcdbb7a6b7ac768312bc;p=thirdparty%2Fbird.git

BGP: fix warnings for roa_check() with no import table and route refresh on

When roa_check() appears in import filters, the autoreload is switched
on but prohibited for BGP if no import table is available. By fixing the
route refresh feature in 9edc421148fe9c5e7d6038b667ba8fafb587a1eb, we
inadvertently exposed another bug where Nest wasn't distinguishing
between locally and remotely available reload.

Whereas on manual reconfiguration, the route refresh is expected to be
invoked, the autoreload should not trigger any remote actions. Therefore
now the channels actualy indicate whether their reload hook triggers
remote actions or not. This information can be then used to decide
whether to allow autoreload or not.
---

diff --git a/nest/proto.c b/nest/proto.c
index 3f5cf0d25..253304415 100644
--- a/nest/proto.c
+++ b/nest/proto.c
@@ -55,6 +55,22 @@ static inline void channel_reimport(struct channel *c, struct rt_feeding_request
   ev_send(proto_event_list(c->proto), &c->reimport_event);
 }
 
+static inline bool channel_reload(struct channel *c, struct rt_feeding_request *rfr)
+{
+  if ((c->in_keep & RIK_PREFILTER) == RIK_PREFILTER)
+  {
+    channel_reimport(c, rfr);
+    return true;
+  }
+  else
+    return c->proto->reload_routes(c, rfr);
+}
+
+static inline void channel_reload_roa(struct channel *c, struct rt_feeding_request *rfr)
+{
+  ASSERT_DIE(channel_reload(c, rfr));
+}
+
 static inline void channel_refeed(struct channel *c, struct rt_feeding_request *rfr)
 {
   CALL(c->proto->refeed_begin, c, rfr);
@@ -67,10 +83,18 @@ static inline int proto_is_done(struct proto *p)
 static inline int channel_is_active(struct channel *c)
 { return (c->channel_state != CS_DOWN); }
 
-static inline int channel_reloadable(struct channel *c)
+static inline enum channel_reloadable channel_reloadable(struct channel *c)
 {
-  return c->reloadable && c->proto->reload_routes
-      || ((c->in_keep & RIK_PREFILTER) == RIK_PREFILTER);
+  /* Import table always reloads locally */
+  if ((c->in_keep & RIK_PREFILTER) == RIK_PREFILTER)
+    return CHANNEL_RELOADABLE_LOCALLY;
+
+  /* The protocol should indicate how the reload actually works */
+  if (c->proto->reload_routes)
+    return c->reloadable;
+
+  /* No reload possible */
+  return CHANNEL_RELOADABLE_NEVER;
 }
 
 static inline void
@@ -232,7 +256,7 @@ proto_add_channel(struct proto *p, struct channel_config *cf)
 
   c->channel_state = CS_DOWN;
   c->last_state_change = current_time();
-  c->reloadable = 1;
+  c->reloadable = CHANNEL_RELOADABLE_LOCALLY;
 
   init_list(&c->roa_subscriptions);
 
@@ -514,7 +538,7 @@ channel_roa_changed(void *_s)
 
 static inline void (*channel_roa_reload_hook(int dir))(struct channel *, struct rt_feeding_request *)
 {
-  return dir ? channel_reimport : channel_refeed;
+  return dir ? channel_reload_roa : channel_refeed;
 }
 
 static int
@@ -587,7 +611,7 @@ channel_roa_subscribe_filter(struct channel *c, int dir)
     return;
 
   /* No automatic reload for non-reloadable channels */
-  if (dir && !channel_reloadable(c))
+  if (dir && (channel_reloadable(c) != CHANNEL_RELOADABLE_LOCALLY))
     valid = 0;
 
   struct filter_iterator fit;
@@ -1040,10 +1064,11 @@ channel_request_reload(struct channel *c, struct rt_feeding_request *cir)
   else
     CD(c, "Full import reload requested");
 
-  if ((c->in_keep & RIK_PREFILTER) == RIK_PREFILTER)
-    channel_reimport(c, cir);
-  else if (! c->proto->reload_routes(c, cir))
-    cli_msg(-15, "%s.%s: partial reload refused, please run full reload instead", c->proto->name, c->name);
+  if (!channel_reload(c, cir))
+    if (cir && cir->prefilter.mode)
+      cli_msg(-15, "%s.%s: partial reload refused, please run full reload instead", c->proto->name, c->name);
+    else
+      bug("%s.%s: full reload refused");
 }
 
 const struct channel_class channel_basic = {
diff --git a/nest/protocol.h b/nest/protocol.h
index 71b839168..d6f69a31e 100644
--- a/nest/protocol.h
+++ b/nest/protocol.h
@@ -659,7 +659,12 @@ struct channel {
   u8 stale;				/* Used in reconfiguration */
 
   u8 channel_state;
-  u8 reloadable;			/* Hook reload_routes() is allowed on the channel */
+  PACKED enum channel_reloadable {
+    CHANNEL_RELOADABLE_NEVER = 0,	/* Never reload */
+    CHANNEL_RELOADABLE_REMOTELY = 1,	/* Hook reload_routes() causes remote actions */
+    CHANNEL_RELOADABLE_LOCALLY = 2,	/* Hook reload_routes() reloads routes locally */
+  } reloadable;
+
   OBSREF(struct graceful_recovery_context) gr_lock;	/* Graceful restart mechanism should wait for this channel */
   u8 gr_wait;				/* Route export to channel is postponed until graceful restart */
 
diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c
index a5ec4e6ac..aaf8c96c3 100644
--- a/proto/bgp/bgp.c
+++ b/proto/bgp/bgp.c
@@ -756,7 +756,11 @@ bgp_conn_enter_established_state(struct bgp_conn *conn)
 
     int active = loc->ready && rem->ready;
     c->c.disabled = !active;
-    c->c.reloadable = p->route_refresh;
+
+    if (p->route_refresh)
+      c->c.reloadable = CHANNEL_RELOADABLE_REMOTELY;
+    else
+      c->c.reloadable = CHANNEL_RELOADABLE_NEVER;
 
     c->index = active ? num++ : 0;