Merge commit '3859e4efc1597368df647323c5a3cc1771cb64ca' into thread-next

5 files changed, 58 insertions, 8 deletions

diff --git a/doc/bird.sgml b/doc/bird.sgml
index badfe649..b9ccf9f7 100644
--- a/doc/bird.sgml
+++ b/doc/bird.sgml
@@ -2783,9 +2783,16 @@ using the following configuration parameters:
 
 	<tag><label id="bgp-hold-time">hold time <m/number/</tag>
 	Time in seconds to wait for a Keepalive message from the other side
-	before considering the connection stale. Default: depends on agreement
-	with the neighboring router, we prefer 240 seconds if the other side is
-	willing to accept it.
+	before considering the connection stale. The effective value is
+	negotiated during session establishment and it is a minimum of this
+	configured value and the value proposed by the peer. The zero value has
+	a special meaning, signifying that no keepalives are used. Default: 240
+	seconds.
+
+	<tag><label id="bgp-min-hold-time">min hold time <m/number/</tag>
+	Minimum value of the hold time that is accepted during session negotiation.
+	If the peer proposes a lower value, the session is rejected with error.
+	Default: none.
 
 	<tag><label id="bgp-startup-hold-time">startup hold time <m/number/</tag>
 	Value of the hold timer used before the routers have a chance to exchange
@@ -2793,8 +2800,15 @@ using the following configuration parameters:
 
 	<tag><label id="bgp-keepalive-time">keepalive time <m/number/</tag>
 	Delay in seconds between sending of two consecutive Keepalive messages.
+	The effective value depends on the negotiated hold time, as it is scaled
+	to maintain proportion between the keepalive time and the hold time.
 	Default: One third of the hold time.
 
+	<tag><label id="bgp-min-keepalive-time">min keepalive time <m/number/</tag>
+	Minimum value of the keepalive time that is accepted during session
+	negotiation. If the proposed hold time would lead to a lower value of
+	the keepalive time, the session is rejected with error. Default: none.
+
 	<tag><label id="bgp-connect-delay-time">connect delay time <m/number/</tag>
 	Delay in seconds between protocol startup and the first attempt to
 	connect. Default: 5 seconds.
diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c
index 2f8da1af..e8e65ad7 100644
--- a/proto/bgp/bgp.c
+++ b/proto/bgp/bgp.c
@@ -2075,6 +2075,21 @@ bgp_postconfig(struct proto_config *CF)
   if (internal && cf->enforce_first_as)
     cf_error("Enforce first AS check is requires EBGP sessions");
 
+  if (cf->keepalive_time > cf->hold_time)
+    cf_error("Keepalive time must be at most hold time");
+
+  if (cf->keepalive_time > (cf->hold_time / 2))
+    log(L_WARN "Keepalive time should be at most 1/2 of hold time");
+
+  if (cf->min_hold_time > cf->hold_time)
+    cf_error("Min hold time (%u) exceeds hold time (%u)",
+	     cf->min_hold_time, cf->hold_time);
+
+  uint keepalive_time = cf->keepalive_time ?: cf->hold_time / 3;
+  if (cf->min_keepalive_time > keepalive_time)
+    cf_error("Min keepalive time (%u) exceeds keepalive time (%u)",
+	     cf->min_keepalive_time, keepalive_time);
+
 
   struct bgp_channel_config *cc;
   BGP_CF_WALK_CHANNELS(cf, cc)
diff --git a/proto/bgp/bgp.h b/proto/bgp/bgp.h
index b87e0fc4..5a680022 100644
--- a/proto/bgp/bgp.h
+++ b/proto/bgp/bgp.h
@@ -120,8 +120,11 @@ struct bgp_config {
   unsigned llgr_time;			/* Long-lived graceful restart stale time */
   unsigned connect_delay_time;		/* Minimum delay between connect attempts */
   unsigned connect_retry_time;		/* Timeout for connect attempts */
-  unsigned hold_time, initial_hold_time;
+  unsigned hold_time;
+  unsigned min_hold_time;		/* Minimum accepted hold time */
+  unsigned initial_hold_time;
   unsigned keepalive_time;
+  unsigned min_keepalive_time;		/* Minimum accepted keepalive time */
   unsigned error_amnesia_time;		/* Errors are forgotten after */
   unsigned error_delay_time_min;	/* Time to wait after an error is detected */
   unsigned error_delay_time_max;
diff --git a/proto/bgp/config.Y b/proto/bgp/config.Y
index b4edf2c5..0be50b5e 100644
--- a/proto/bgp/config.Y
+++ b/proto/bgp/config.Y
@@ -154,7 +154,8 @@ bgp_proto:
  | bgp_proto RS CLIENT bool ';' { BGP_CFG->rs_client = $4; }
  | bgp_proto CONFEDERATION expr ';' { BGP_CFG->confederation = $3; }
  | bgp_proto CONFEDERATION MEMBER bool ';' { BGP_CFG->confederation_member = $4; }
- | bgp_proto HOLD TIME expr ';' { BGP_CFG->hold_time = $4; }
+ | bgp_proto HOLD TIME expr ';' { BGP_CFG->hold_time = $4; if (($4 && $4<3) || ($4>65535)) cf_error("Hold time must be in range 3-65535 or zero"); }
+ | bgp_proto MIN HOLD TIME expr ';' { BGP_CFG->min_hold_time = $5; }
  | bgp_proto STARTUP HOLD TIME expr ';' { BGP_CFG->initial_hold_time = $5; }
  | bgp_proto DIRECT ';' { BGP_CFG->multihop = 0; }
  | bgp_proto MULTIHOP ';' { BGP_CFG->multihop = 64; }
@@ -178,7 +179,8 @@ bgp_proto:
  | bgp_proto START DELAY TIME expr ';' { BGP_CFG->connect_delay_time = $5; log(L_WARN "%s: Start delay time option is deprecated, use connect delay time", this_proto->name); }
  | bgp_proto CONNECT DELAY TIME expr ';' { BGP_CFG->connect_delay_time = $5; }
  | bgp_proto CONNECT RETRY TIME expr ';' { BGP_CFG->connect_retry_time = $5; }
- | bgp_proto KEEPALIVE TIME expr ';' { BGP_CFG->keepalive_time = $4; }
+ | bgp_proto KEEPALIVE TIME expr ';' { BGP_CFG->keepalive_time = $4; if (($4<1) || ($4>65535)) cf_error("Keepalive time must be in range 1-65535"); }
+ | bgp_proto MIN KEEPALIVE TIME expr ';' { BGP_CFG->min_keepalive_time = $5; }
  | bgp_proto ERROR FORGET TIME expr ';' { BGP_CFG->error_amnesia_time = $5; }
  | bgp_proto ERROR WAIT TIME expr ',' expr ';' { BGP_CFG->error_delay_time_min = $5; BGP_CFG->error_delay_time_max = $7; }
  | bgp_proto DISABLE AFTER ERROR bool ';' { BGP_CFG->disable_after_error = $5; }
diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c
index 506268c9..04e4505c 100644
--- a/proto/bgp/packets.c
+++ b/proto/bgp/packets.c
@@ -847,9 +847,25 @@ bgp_rx_open(struct bgp_conn *conn, byte *pkt, uint len)
   if (bgp_read_options(conn, pkt+29, pkt[28], len-29) < 0)
     return;
 
+  /* RFC 4271 4.2 - hold time must be either 0 or at least 3 */
   if (hold > 0 && hold < 3)
   { bgp_error(conn, 2, 6, pkt+22, 2); return; }
 
+  /* Compute effective hold and keepalive times */
+  uint hold_time = MIN(hold, p->cf->hold_time);
+  uint keepalive_time = p->cf->keepalive_time ?
+    (p->cf->keepalive_time * hold_time / p->cf->hold_time) :
+    hold_time / 3;
+
+  /* Keepalive time might be rounded down to zero */
+  if (hold_time && !keepalive_time)
+    keepalive_time = 1;
+
+  /* Check effective values against configured minimums */
+  if ((hold_time < p->cf->min_hold_time) ||
+      (keepalive_time < p->cf->min_keepalive_time))
+  { bgp_error(conn, 2, 6, pkt+22, 2); return; }
+
   /* RFC 6286 2.2 - router ID is nonzero and AS-wide unique */
   if (!id || (p->is_internal && id == p->local_id))
   { bgp_error(conn, 2, 3, pkt+24, -4); return; }
@@ -947,8 +963,8 @@ bgp_rx_open(struct bgp_conn *conn, byte *pkt, uint len)
   }
 
   /* Update our local variables */
-  conn->hold_time = MIN(hold, p->cf->hold_time);
-  conn->keepalive_time = p->cf->keepalive_time ? : conn->hold_time / 3;
+  conn->hold_time = hold_time;
+  conn->keepalive_time = keepalive_time;
   conn->as4_session = conn->local_caps->as4_support && caps->as4_support;
   conn->ext_messages = conn->local_caps->ext_messages && caps->ext_messages;
   p->remote_id = id;