From bcf2327425d4dd96f381b87501cccf943bed606e Mon Sep 17 00:00:00 2001
From: Katerina Kubecova <katerina.kubecova@nic.cz>
Date: Fri, 27 Oct 2023 17:11:06 +0200
Subject: [PATCH] BGP: Send hold timer

Implement BGP Send hold timer according to draft-ietf-idr-bgp-sendholdtimer.
The Send hold timer drops the session if the neighbor is sending keepalives,
but does not receive our messages, causing the TCP connection to stall.
---
 doc/bird.sgml       | 17 +++++++++++++++++
 proto/bgp/bgp.c     | 35 ++++++++++++++++++++++++++++++++++-
 proto/bgp/bgp.h     |  6 +++++-
 proto/bgp/config.Y  |  5 ++++-
 proto/bgp/packets.c | 17 ++++++++++++++++-
 5 files changed, 76 insertions(+), 4 deletions(-)

diff --git a/doc/bird.sgml b/doc/bird.sgml
index 10c6f121..8834c1cb 100644
--- a/doc/bird.sgml
+++ b/doc/bird.sgml
@@ -3157,6 +3157,23 @@ using the following configuration parameters:
 	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-send-hold-time">send hold time <m/number/</tag>
+	Maximum time in seconds betweeen successfull transmissions of BGP messages.
+	Send hold timer drops the session if the neighbor is sending keepalives,
+	but does not receive our messages, causing the TCP connection to stall.
+	This may happen due to malfunctioning or overwhelmed neighbor. See
+	<HTMLURL URL="https://datatracker.ietf.org/doc/draft-ietf-idr-bgp-sendholdtimer/"
+	name="draft-ietf-idr-bgp-sendholdtimer"> for more details.
+
+	Like the option <cf/keepalive time/, the effective value depends on the
+	negotiated hold time, as it is scaled to maintain proportion between the
+	send hold time and the keepalive time. If it is set to zero, the timer
+	is disabled. Default: double of the hold timer limit.
+
+	The option <cf/disable rx/ is intended only for testing this feature and
+	should not be used anywhere else. It discards received messages and
+	disables the hold timer.
+
 	<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 b14df932..9d4671af 100644
--- a/proto/bgp/bgp.c
+++ b/proto/bgp/bgp.c
@@ -375,6 +375,8 @@ bgp_close_conn(struct bgp_conn *conn)
   conn->keepalive_timer = NULL;
   rfree(conn->hold_timer);
   conn->hold_timer = NULL;
+  rfree(conn->send_hold_timer);
+  conn->send_hold_timer = NULL;
   rfree(conn->tx_ev);
   conn->tx_ev = NULL;
   rfree(conn->sk);
@@ -673,6 +675,13 @@ bgp_conn_enter_established_state(struct bgp_conn *conn)
     p->channel_map[c->index] = c;
   }
 
+  /* Breaking rx_hook for simulating receive problem */
+  if (p->cf->disable_rx)
+  {
+    conn->sk->rx_hook = NULL;
+    tm_stop(conn->hold_timer);
+  }
+
   /* proto_notify_state() will likely call bgp_feed_begin(), setting c->feed_state */
 
   bgp_conn_set_state(conn, BS_ESTABLISHED);
@@ -1044,6 +1053,27 @@ bgp_keepalive_timeout(timer *t)
     ev_run(conn->tx_ev);
 }
 
+void
+bgp_send_hold_timeout(timer *t)
+{
+  struct bgp_conn *conn = t->data;
+  struct bgp_proto *p = conn->bgp;
+
+  if (conn->state == BS_CLOSE)
+    return;
+
+  /* Error codes not yet assigned by IANA */
+  uint code = 4;
+  uint subcode = 1;
+
+  /* Like bgp_error() but without NOTIFICATION */
+  bgp_log_error(p, BE_BGP_TX, "Error", code, subcode, NULL, 0);
+  bgp_store_error(p, conn, BE_BGP_TX, (code << 16) | subcode);
+  bgp_conn_enter_idle_state(conn);
+  bgp_update_startup_delay(p);
+  bgp_stop(p, 0, NULL, 0);
+}
+
 static void
 bgp_setup_conn(struct bgp_proto *p, struct bgp_conn *conn)
 {
@@ -1058,6 +1088,7 @@ bgp_setup_conn(struct bgp_proto *p, struct bgp_conn *conn)
   conn->connect_timer	= tm_new_init(p->p.pool, bgp_connect_timeout,	 conn, 0, 0);
   conn->hold_timer 	= tm_new_init(p->p.pool, bgp_hold_timeout,	 conn, 0, 0);
   conn->keepalive_timer	= tm_new_init(p->p.pool, bgp_keepalive_timeout, conn, 0, 0);
+  conn->send_hold_timer = tm_new_init(p->p.pool, bgp_send_hold_timeout, conn, 0, 0);
 
   conn->tx_ev = ev_new_init(p->p.pool, bgp_kick_tx, conn);
 }
@@ -2619,7 +2650,9 @@ bgp_show_proto_info(struct proto *P)
 	    tm_remains(p->conn->hold_timer), p->conn->hold_time);
     cli_msg(-1006, "    Keepalive timer:  %t/%u",
 	    tm_remains(p->conn->keepalive_timer), p->conn->keepalive_time);
-  }
+    cli_msg(-1006, "    Send hold timer:  %t/%u",
+	    tm_remains(p->conn->send_hold_timer), p->conn->send_hold_time);
+}
 
 #if 0
   struct bgp_stats *s = &p->stats;
diff --git a/proto/bgp/bgp.h b/proto/bgp/bgp.h
index 61127562..7127bc88 100644
--- a/proto/bgp/bgp.h
+++ b/proto/bgp/bgp.h
@@ -117,6 +117,8 @@ struct bgp_config {
   int setkey;				/* Set MD5 password to system SA/SP database */
   u8  local_role;			/* Set peering role with neighbor [RFC 9234] */
   int require_roles;			/* Require configured roles on both sides */
+  int send_hold_time;
+  int disable_rx;			/* Stop reading messages after handshake (for simulating error) */
   /* Times below are in seconds */
   unsigned gr_time;			/* Graceful restart timeout */
   unsigned llgr_time;			/* Long-lived graceful restart stale time */
@@ -307,6 +309,7 @@ struct bgp_conn {
   timer *connect_timer;
   timer *hold_timer;
   timer *keepalive_timer;
+  timer *send_hold_timer;
   event *tx_ev;
   u32 packets_to_send;			/* Bitmap of packet types to be sent */
   u32 channels_to_send;			/* Bitmap of channels with packets to be sent */
@@ -315,7 +318,7 @@ struct bgp_conn {
   int notify_code, notify_subcode, notify_size;
   byte *notify_data;
 
-  uint hold_time, keepalive_time;	/* Times calculated from my and neighbor's requirements */
+  uint hold_time, keepalive_time, send_hold_time;	/* Times calculated from my and neighbor's requirements */
 };
 
 struct bgp_proto {
@@ -558,6 +561,7 @@ void bgp_conn_enter_openconfirm_state(struct bgp_conn *conn);
 void bgp_conn_enter_established_state(struct bgp_conn *conn);
 void bgp_conn_enter_close_state(struct bgp_conn *conn);
 void bgp_conn_enter_idle_state(struct bgp_conn *conn);
+void broke_bgp_listening(struct channel *C);
 void bgp_handle_graceful_restart(struct bgp_proto *p);
 void bgp_graceful_restart_done(struct bgp_channel *c);
 void bgp_refresh_begin(struct bgp_channel *c);
diff --git a/proto/bgp/config.Y b/proto/bgp/config.Y
index 0fcbb276..1173ff06 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)
+	RS_SERVER, RS_CLIENT, REQUIRE, BGP_OTC, GLOBAL, SEND)
 
 %type <i> bgp_nh
 %type <i32> bgp_afi
@@ -77,6 +77,7 @@ bgp_proto_start: proto_start BGP {
      BGP_CFG->local_role = BGP_ROLE_UNDEFINED;
      BGP_CFG->dynamic_name = "dynbgp";
      BGP_CFG->check_link = -1;
+     BGP_CFG->send_hold_time = -1;
    }
  ;
 
@@ -182,6 +183,7 @@ bgp_proto:
  | bgp_proto CONNECT RETRY TIME expr ';' { BGP_CFG->connect_retry_time = $5; }
  | 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 SEND HOLD TIME expr';' { BGP_CFG->send_hold_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; }
@@ -222,6 +224,7 @@ bgp_proto:
  | bgp_proto ENFORCE FIRST AS bool ';' { BGP_CFG->enforce_first_as = $5; }
  | bgp_proto LOCAL ROLE bgp_role_name ';' { BGP_CFG->local_role = $4; }
  | bgp_proto REQUIRE ROLES bool ';' { BGP_CFG->require_roles = $4; }
+ | bgp_proto DISABLE RX bool ';' { BGP_CFG->disable_rx = $4; }
  ;
 
 bgp_afi:
diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c
index 1e5a226f..e8cc4718 100644
--- a/proto/bgp/packets.c
+++ b/proto/bgp/packets.c
@@ -926,6 +926,10 @@ bgp_rx_open(struct bgp_conn *conn, byte *pkt, uint len)
     (p->cf->keepalive_time * hold_time / p->cf->hold_time) :
     hold_time / 3;
 
+  uint send_hold_time = (p->cf->send_hold_time >= 0) ?
+    (p->cf->send_hold_time * hold_time / p->cf->hold_time) :
+    2 * hold_time;
+
   /* Keepalive time might be rounded down to zero */
   if (hold_time && !keepalive_time)
     keepalive_time = 1;
@@ -1034,6 +1038,7 @@ bgp_rx_open(struct bgp_conn *conn, byte *pkt, uint len)
   /* Update our local variables */
   conn->hold_time = hold_time;
   conn->keepalive_time = keepalive_time;
+  conn->send_hold_time = send_hold_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;
@@ -1043,6 +1048,7 @@ bgp_rx_open(struct bgp_conn *conn, byte *pkt, uint len)
 
   bgp_schedule_packet(conn, NULL, PKT_KEEPALIVE);
   bgp_start_timer(conn->hold_timer, conn->hold_time);
+  bgp_start_timer(conn->send_hold_timer, conn->send_hold_time);
   bgp_conn_enter_openconfirm_state(conn);
 }
 
@@ -3060,7 +3066,11 @@ bgp_send(struct bgp_conn *conn, uint type, uint len)
   put_u16(buf+16, len);
   buf[18] = type;
 
-  return sk_send(sk, len);
+  int success = sk_send(sk, len);
+  if (success && ((conn->state == BS_ESTABLISHED) || (conn->state == BS_OPENCONFIRM)))
+    bgp_start_timer(conn->send_hold_timer, conn->send_hold_time);
+
+  return success;
 }
 
 /**
@@ -3220,6 +3230,10 @@ bgp_tx(sock *sk)
 {
   struct bgp_conn *conn = sk->data;
 
+  /* Pending message was passed to kernel */
+  if ((conn->state == BS_ESTABLISHED) || (conn->state == BS_OPENCONFIRM))
+    bgp_start_timer(conn->send_hold_timer, conn->send_hold_time);
+
   DBG("BGP: TX hook\n");
   uint max = 1024;
   while (--max && (bgp_fire_tx(conn) > 0))
@@ -3261,6 +3275,7 @@ static struct {
   { 3, 10, "Invalid network field" },
   { 3, 11, "Malformed AS_PATH" },
   { 4, 0, "Hold timer expired" },
+  { 4, 1, "Send hold timer expired" }, /* Provisional [draft-ietf-idr-bgp-sendholdtimer] */
   { 5, 0, "Finite state machine error" }, /* Subcodes are according to [RFC6608] */
   { 5, 1, "Unexpected message in OpenSent state" },
   { 5, 2, "Unexpected message in OpenConfirm state" },