BGP: Improve handling of hold and keepalive timers

The effective keepalive time now scales relative to the negotiated
hold time, to maintain proportion between the keepalive time and the
hold time. This avoids issues when both keepalive and hold times
were configured, the hold time was negotiated to a smaller value,
but the keepalive time stayed the same.

Add new options 'min hold time' and 'min keepalive time', which reject
session attempts with too small hold time.

Improve validation of config options an their documentation.

Thanks to Alexander Zubkov and Sergei Goriunov for suggestions.

4 files changed, 41 insertions, 5 deletions

diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c
index ad78d5e5..f4098e04 100644
--- a/proto/bgp/bgp.c
+++ b/proto/bgp/bgp.c
@@ -2007,6 +2007,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 eda5d229..ec802fde 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 2294119e..4e83147e 100644
--- a/proto/bgp/config.Y
+++ b/proto/bgp/config.Y
@@ -153,7 +153,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; }
@@ -177,7 +178,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 c464e9c7..afcacffa 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;