From 6a55359b1eeba099dddf410bb37137370fe19b86 Mon Sep 17 00:00:00 2001
From: Ondrej Zajicek <santiago@crfreenet.org>
Date: Fri, 6 Dec 2024 04:06:16 +0100
Subject: [PATCH] BGP: Graceful restart timer overrides

Implement several options (min/max graceful restart time, min/max long
lived stale time) to override graceful restart and long-lived graceful
restart timer values, as suggested by RFC 9494.
---
 doc/bird.sgml      | 32 ++++++++++++++++++++++++++++++++
 lib/birdlib.h      |  1 +
 proto/bgp/bgp.c    | 31 +++++++++++++++++++++++++++++--
 proto/bgp/bgp.h    |  6 ++++++
 proto/bgp/config.Y | 20 ++++++++++++++++----
 5 files changed, 84 insertions(+), 6 deletions(-)

diff --git a/doc/bird.sgml b/doc/bird.sgml
index ab48565f4..6e1085273 100644
--- a/doc/bird.sgml
+++ b/doc/bird.sgml
@@ -3155,6 +3155,16 @@ using the following configuration parameters:
 	re-establish after a restart before deleting stale routes. Default:
 	120 seconds.
 
+	<tag><label id="bgp-min-graceful-restart-time">min graceful restart time <m/number/</tag>
+	The lower bound for the graceful restart time to override the value
+	received in the BGP Graceful Restart capability announced by the
+	neighbor. Default: no lower bound.
+
+	<tag><label id="bgp-max-graceful-restart-time">max graceful restart time <m/number/</tag>
+	The upper bound for the graceful restart time to override the value
+	received in the BGP Graceful Restart capability announced by the
+	neighbor. Default: no upper bound.
+
 	<tag><label id="bgp-require-graceful-restart">require graceful restart <m/switch/</tag>
 	If enabled, the BGP Graceful Restart capability (<rfc id="4724">)
 	must be announced by the BGP neighbor, otherwise the BGP session
@@ -3179,6 +3189,16 @@ using the following configuration parameters:
 	session is re-stablished and synchronized or the stale time expires and
 	routes are removed. Default: 3600 seconds.
 
+	<tag><label id="bgp-min-long-lived-stale-time">min long lived stale time <m/number/</tag>
+	The lower bound for the long-lived stale time to override the value
+	received in the BGP Long-lived Graceful Restart capability announced
+	by the neighbor. Default: no lower bound.
+
+	<tag><label id="bgp-max-long-lived-stale-time">max long lived stale time <m/number/</tag>
+	The upper bound for the long-lived stale time to override the value
+	received in the BGP Long-lived Graceful Restart capability announced
+	by the neighbor. Default: no upper bound.
+
 	<tag><label id="bgp-require-long-lived-graceful-restart">require long lived graceful restart <m/switch/</tag>
 	If enabled, the BGP Long-lived Graceful Restart capability (<rfc id="9494">)
 	must be announced by the BGP neighbor, otherwise the BGP session
@@ -3646,6 +3666,18 @@ be used in explicit configuration.
 	set <ref id="bgp-long-lived-stale-time" name="long lived stale time">
 	per AFI/SAFI pair instead of per protocol. Default: set by protocol-wide
 	option.
+
+	<tag><label id="bgp-min-long-lived-stale-time-c">min long lived stale time <m/number/</tag>
+	Like previous graceful restart channel options, this option allows to
+	set <ref id="bgp-min-long-lived-stale-time" name="min long lived stale time">
+	per AFI/SAFI pair instead of per protocol. Default: set by protocol-wide
+	option.
+
+	<tag><label id="bgp-max-long-lived-stale-time-c">max long lived stale time <m/number/</tag>
+	Like previous graceful restart channel options, this option allows to
+	set <ref id="bgp-max-long-lived-stale-time" name="max long lived stale time">
+	per AFI/SAFI pair instead of per protocol. Default: set by protocol-wide
+	option.
 </descrip>
 
 <sect1>Attributes
diff --git a/lib/birdlib.h b/lib/birdlib.h
index 762c5fa42..0779655e1 100644
--- a/lib/birdlib.h
+++ b/lib/birdlib.h
@@ -25,6 +25,7 @@ struct align_probe { char x; long int y; };
 
 #define MIN_(a,b) (((a)<(b))?(a):(b))
 #define MAX_(a,b) (((a)>(b))?(a):(b))
+#define CLAMP(a,l,h) (((a)<(l)) ? (l) : (((a)>(h)) ? (h) : (a)))
 
 #ifndef PARSER
 #undef MIN
diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c
index f0660e623..c18a73feb 100644
--- a/proto/bgp/bgp.c
+++ b/proto/bgp/bgp.c
@@ -622,7 +622,8 @@ bgp_conn_enter_established_state(struct bgp_conn *conn)
     p->llgr_ready = p->llgr_ready || llgr_ready;
 
     /* Remember last LLGR stale time */
-    c->stale_time = local->llgr_aware ? rem->llgr_time : 0;
+    c->stale_time = local->llgr_aware ?
+      CLAMP(rem->llgr_time, c->cf->min_llgr_time, c->cf->max_llgr_time) : 0;
 
     /* Channels not able to recover gracefully */
     if (p->p.gr_recovery && (!active || !peer_gr_ready))
@@ -805,8 +806,11 @@ bgp_handle_graceful_restart(struct bgp_proto *p)
   /* p->gr_ready -> at least one active channel is c->gr_ready */
   ASSERT(p->gr_active_num > 0);
 
+  uint gr_time = CLAMP(p->conn->remote_caps->gr_time,
+		       p->cf->min_gr_time, p->cf->max_gr_time);
+
   proto_notify_state(&p->p, PS_START);
-  tm_start(p->gr_timer, p->conn->remote_caps->gr_time S);
+  tm_start(p->gr_timer, gr_time S);
 }
 
 /**
@@ -2089,6 +2093,14 @@ bgp_postconfig(struct proto_config *CF)
     cf_error("Min keepalive time (%u) exceeds keepalive time (%u)",
 	     cf->min_keepalive_time, keepalive_time);
 
+  if (cf->min_gr_time > cf->max_gr_time)
+    cf_error("Min graceful restart time (%u) exceeds max graceful restart time (%u)",
+	     cf->min_gr_time, cf->max_gr_time);
+
+  if (cf->min_llgr_time > cf->max_llgr_time)
+    cf_error("Min long-lived stale time (%u) exceeds max long-lived stale time (%u)",
+	     cf->min_llgr_time, cf->max_llgr_time);
+
 
   struct bgp_channel_config *cc;
   BGP_CF_WALK_CHANNELS(cf, cc)
@@ -2133,6 +2145,12 @@ bgp_postconfig(struct proto_config *CF)
     if (cc->llgr_time == ~0U)
       cc->llgr_time = cf->llgr_time;
 
+    if (cc->min_llgr_time == ~0U)
+      cc->min_llgr_time = cf->min_llgr_time;
+
+    if (cc->max_llgr_time == ~0U)
+      cc->max_llgr_time = cf->max_llgr_time;
+
     /* AIGP enabled by default on interior sessions */
     if (cc->aigp == 0xff)
       cc->aigp = interior;
@@ -2178,6 +2196,11 @@ bgp_postconfig(struct proto_config *CF)
 
     if (cc->require_add_path && !cc->add_path)
       cf_warn("ADD-PATH required but not enabled");
+
+    if (cc->min_llgr_time > cc->max_llgr_time)
+      cf_error("Min long-lived stale time (%u) exceeds max long-lived stale time (%u)",
+	       cc->min_llgr_time, cc->max_llgr_time);
+
   }
 }
 
@@ -2272,6 +2295,10 @@ bgp_channel_reconfigure(struct channel *C, struct channel_config *CC, int *impor
       (TABLE(new, base_table) != TABLE(old, base_table)))
     return 0;
 
+  if (c->stale_time && ((new->min_llgr_time > c->stale_time) ||
+			(new->max_llgr_time < c->stale_time)))
+    return 0;
+
   if (new->mandatory && !old->mandatory && (C->channel_state != CS_UP))
     return 0;
 
diff --git a/proto/bgp/bgp.h b/proto/bgp/bgp.h
index 320786687..bd6fe2de4 100644
--- a/proto/bgp/bgp.h
+++ b/proto/bgp/bgp.h
@@ -122,6 +122,10 @@ struct bgp_config {
   /* Times below are in seconds */
   unsigned gr_time;			/* Graceful restart timeout */
   unsigned llgr_time;			/* Long-lived graceful restart stale time */
+  unsigned min_gr_time;			/* Minimum GR timeout */
+  unsigned max_gr_time;			/* Maximum GR timeout */
+  unsigned min_llgr_time;		/* Minimum LLGR stale time */
+  unsigned max_llgr_time;		/* Maximum LLGR stale time */
   unsigned connect_delay_time;		/* Minimum delay between connect attempts */
   unsigned connect_retry_time;		/* Timeout for connect attempts */
   unsigned hold_time;
@@ -167,6 +171,8 @@ struct bgp_channel_config {
   u8 gr_able;				/* Allow full graceful restart for the channel */
   u8 llgr_able;				/* Allow full long-lived GR for the channel */
   uint llgr_time;			/* Long-lived graceful restart stale time */
+  uint min_llgr_time;			/* Minimum LLGR stale time */
+  uint max_llgr_time;			/* Maximum LLGR stale time */
   u8 ext_next_hop;			/* Allow both IPv4 and IPv6 next hops */
   u8 require_ext_next_hop;		/* Require remote support of IPv4 NLRI with IPv6 next hops [RFC 8950] */
   u8 add_path;				/* Use ADD-PATH extension [RFC 7911] */
diff --git a/proto/bgp/config.Y b/proto/bgp/config.Y
index cf04ce232..222879e94 100644
--- a/proto/bgp/config.Y
+++ b/proto/bgp/config.Y
@@ -32,7 +32,7 @@ CF_KEYWORDS(BGP, LOCAL, NEIGHBOR, AS, HOLD, TIME, CONNECT, RETRY, KEEPALIVE,
 	LIVED, STALE, IMPORT, IBGP, EBGP, MANDATORY, INTERNAL, EXTERNAL, SETS,
 	DYNAMIC, RANGE, NAME, DIGITS, BGP_AIGP, AIGP, ORIGINATE, COST, ENFORCE,
 	FIRST, FREE, VALIDATE, BASE, ROLE, ROLES, PEER, PROVIDER, CUSTOMER,
-	RS_SERVER, RS_CLIENT, REQUIRE, BGP_OTC, GLOBAL, SEND)
+	RS_SERVER, RS_CLIENT, REQUIRE, BGP_OTC, GLOBAL, SEND, MIN, MAX)
 
 %type <i> bgp_nh
 %type <i32> bgp_afi
@@ -71,8 +71,12 @@ bgp_proto_start: proto_start BGP {
      BGP_CFG->default_local_pref = 100;
      BGP_CFG->gr_mode = BGP_GR_AWARE;
      BGP_CFG->gr_time = 120;
+     BGP_CFG->min_gr_time = 0;
+     BGP_CFG->max_gr_time = ~0U;
      BGP_CFG->llgr_mode = -1;
      BGP_CFG->llgr_time = 3600;
+     BGP_CFG->min_llgr_time = 0;
+     BGP_CFG->max_llgr_time = ~0U;
      BGP_CFG->setkey = 1;
      BGP_CFG->local_role = BGP_ROLE_UNDEFINED;
      BGP_CFG->dynamic_name = "dynbgp";
@@ -212,10 +216,14 @@ bgp_proto:
  | bgp_proto ALLOW AS SETS bool ';' { BGP_CFG->allow_as_sets = $5; }
  | bgp_proto GRACEFUL RESTART bool ';' { BGP_CFG->gr_mode = $4; }
  | bgp_proto GRACEFUL RESTART AWARE ';' { BGP_CFG->gr_mode = BGP_GR_AWARE; }
- | bgp_proto GRACEFUL RESTART TIME expr ';' { BGP_CFG->gr_time = $5; }
+ | bgp_proto GRACEFUL RESTART TIME expr ';' { BGP_CFG->gr_time = $5; if ($5 >= (1 << 12)) cf_error("Graceful restart time must be less than 4096"); }
+ | bgp_proto MIN GRACEFUL RESTART TIME expr ';' { BGP_CFG->min_gr_time = $6; if ($6 >= (1 << 12)) cf_error("Min graceful restart time must be less than 4096"); }
+ | bgp_proto MAX GRACEFUL RESTART TIME expr ';' { BGP_CFG->max_gr_time = $6; if ($6 >= (1 << 12)) cf_error("Max graceful restart time must be less than 4096"); }
  | bgp_proto LONG LIVED GRACEFUL RESTART bool ';' { BGP_CFG->llgr_mode = $6; }
  | bgp_proto LONG LIVED GRACEFUL RESTART AWARE ';' { BGP_CFG->llgr_mode = BGP_LLGR_AWARE; }
- | bgp_proto LONG LIVED STALE TIME expr ';' { BGP_CFG->llgr_time = $6; }
+ | bgp_proto LONG LIVED STALE TIME expr ';' { BGP_CFG->llgr_time = $6; if ($6 >= (1 << 24)) cf_error("Long-lived stale time must be less than 2^24"); }
+ | bgp_proto MIN LONG LIVED STALE TIME expr ';' { BGP_CFG->min_llgr_time = $7; if ($7 >= (1 << 24)) cf_error("Min long-lived stale time must be less than 2^24"); }
+ | bgp_proto MAX LONG LIVED STALE TIME expr ';' { BGP_CFG->max_llgr_time = $7; if ($7 >= (1 << 24)) cf_error("Max long-lived stale time must be less than 2^24"); }
  | bgp_proto TTL SECURITY bool ';' { BGP_CFG->ttl_security = $4; }
  | bgp_proto CHECK LINK bool ';' { BGP_CFG->check_link = $4; }
  | bgp_proto BFD bool ';' { if ($3) init_bfd_opts(&BGP_CFG->bfd); else BGP_CFG->bfd = NULL; }
@@ -263,6 +271,8 @@ bgp_channel_start: bgp_afi
     BGP_CC->gr_able = 0xff;	/* undefined */
     BGP_CC->llgr_able = 0xff;	/* undefined */
     BGP_CC->llgr_time = ~0U;	/* undefined */
+    BGP_CC->min_llgr_time = ~0U; /* undefined */
+    BGP_CC->max_llgr_time = ~0U; /* undefined */
     BGP_CC->aigp = 0xff;	/* undefined */
   }
 };
@@ -292,7 +302,9 @@ bgp_channel_item:
    }
  | GRACEFUL RESTART bool { BGP_CC->gr_able = $3; }
  | LONG LIVED GRACEFUL RESTART bool { BGP_CC->llgr_able = $5; }
- | LONG LIVED STALE TIME expr { BGP_CC->llgr_time = $5; }
+ | LONG LIVED STALE TIME expr { BGP_CC->llgr_time = $5; if ($5 >= (1 << 24)) cf_error("Long-lived stale time must be less than 2^24"); }
+ | MIN LONG LIVED STALE TIME expr { BGP_CC->min_llgr_time = $6; if ($6 >= (1 << 24)) cf_error("Min long-lived stale time must be less than 2^24"); }
+ | MAX LONG LIVED STALE TIME expr { BGP_CC->max_llgr_time = $6; if ($6 >= (1 << 24)) cf_error("Max long-lived stale time must be less than 2^24"); }
  | EXTENDED NEXT HOP bool { BGP_CC->ext_next_hop = $4; }
  | REQUIRE EXTENDED NEXT HOP bool { BGP_CC->require_ext_next_hop = $5;  if (BGP_AFI(BGP_CC->afi) != BGP_AFI_IPV4) cf_warn("Require extended next hop option ignored for non-IPv4 channels"); }
  | ADD PATHS RX { BGP_CC->add_path = BGP_ADD_PATH_RX; }
-- 
2.47.2