BGP uses KEEPALIVE packets and the HOLD timer to detect stalled sessions.

The problem is that this timer only looks at the receive side of the TCP
session. If for some reason the send side stalls the system fully depends
on the remote BGP peer to reset the session. As seen in an ever growing
OutQ and as a result important changes can get stalled and cause routing
troubles.

This change introduces a SEND HOLD timer. The timer is reset whenever the
session engine was able to write data to the TCP socket. If the send hold
timer expires bgpd was not able to send any data to that neighbor for at
least 90 seconds and therefor the session is forcefully closed with a hold
timer expired notification.

The send hold timer acts as a last resort to detect faulty peers. On an
idle session it can take a long time until this timer triggers but the
main goal here is to reset a stuck session at some point which did not
happen before.

With and OK job@

3 files changed, 20 insertions, 3 deletions

diff --git a/usr.sbin/bgpd/bgpd.h b/usr.sbin/bgpd/bgpd.h
index 5e1e615335d..e0d5f61b6d9 100644
--- a/usr.sbin/bgpd/bgpd.h
+++ b/usr.sbin/bgpd/bgpd.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: bgpd.h,v 1.405 2020/11/05 11:52:59 claudio Exp $ */
+/*	$OpenBSD: bgpd.h,v 1.406 2020/12/23 13:20:47 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -1377,6 +1377,7 @@ static const char * const eventnames[] = {
 	"ConnectRetryTimer expired",
 	"HoldTimer expired",
 	"KeepaliveTimer expired",
+	"SendHoldTimer expired",
 	"OPEN message received",
 	"KEEPALIVE message received",
 	"UPDATE message received",
@@ -1467,6 +1468,7 @@ static const char * const timernames[] = {
 	"ConnectRetryTimer",
 	"KeepaliveTimer",
 	"HoldTimer",
+	"SendHoldTimer",
 	"IdleHoldTimer",
 	"IdleHoldResetTimer",
 	"CarpUndemoteTimer",
diff --git a/usr.sbin/bgpd/session.c b/usr.sbin/bgpd/session.c
index 23361aedc82..44c92912c65 100644
--- a/usr.sbin/bgpd/session.c
+++ b/usr.sbin/bgpd/session.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: session.c,v 1.406 2020/12/11 12:00:01 claudio Exp $ */
+/*	$OpenBSD: session.c,v 1.407 2020/12/23 13:20:47 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004, 2005 Henning Brauer <henning@openbsd.org>
@@ -375,6 +375,9 @@ session_main(int debug, int verbose)
 				case Timer_Hold:
 					bgp_fsm(p, EVNT_TIMER_HOLDTIME);
 					break;
+				case Timer_SendHold:
+					bgp_fsm(p, EVNT_TIMER_SENDHOLD);
+					break;
 				case Timer_ConnectRetry:
 					bgp_fsm(p, EVNT_TIMER_CONNRETRY);
 					break;
@@ -597,6 +600,7 @@ bgp_fsm(struct peer *peer, enum session_events event)
 		switch (event) {
 		case EVNT_START:
 			timer_stop(&peer->timers, Timer_Hold);
+			timer_stop(&peer->timers, Timer_SendHold);
 			timer_stop(&peer->timers, Timer_Keepalive);
 			timer_stop(&peer->timers, Timer_IdleHold);
 
@@ -709,6 +713,7 @@ bgp_fsm(struct peer *peer, enum session_events event)
 			change_state(peer, STATE_IDLE, event);
 			break;
 		case EVNT_TIMER_HOLDTIME:
+		case EVNT_TIMER_SENDHOLD:
 			session_notification(peer, ERR_HOLDTIMEREXPIRED,
 			    0, NULL, 0);
 			change_state(peer, STATE_IDLE, event);
@@ -749,6 +754,7 @@ bgp_fsm(struct peer *peer, enum session_events event)
 			change_state(peer, STATE_IDLE, event);
 			break;
 		case EVNT_TIMER_HOLDTIME:
+		case EVNT_TIMER_SENDHOLD:
 			session_notification(peer, ERR_HOLDTIMEREXPIRED,
 			    0, NULL, 0);
 			change_state(peer, STATE_IDLE, event);
@@ -784,6 +790,7 @@ bgp_fsm(struct peer *peer, enum session_events event)
 			change_state(peer, STATE_IDLE, event);
 			break;
 		case EVNT_TIMER_HOLDTIME:
+		case EVNT_TIMER_SENDHOLD:
 			session_notification(peer, ERR_HOLDTIMEREXPIRED,
 			    0, NULL, 0);
 			change_state(peer, STATE_IDLE, event);
@@ -875,6 +882,7 @@ change_state(struct peer *peer, enum session_state state,
 		timer_stop(&peer->timers, Timer_ConnectRetry);
 		timer_stop(&peer->timers, Timer_Keepalive);
 		timer_stop(&peer->timers, Timer_Hold);
+		timer_stop(&peer->timers, Timer_SendHold);
 		timer_stop(&peer->timers, Timer_IdleHold);
 		timer_stop(&peer->timers, Timer_IdleHoldReset);
 		session_close_connection(peer);
@@ -923,6 +931,7 @@ change_state(struct peer *peer, enum session_state state,
 			timer_stop(&peer->timers, Timer_ConnectRetry);
 			timer_stop(&peer->timers, Timer_Keepalive);
 			timer_stop(&peer->timers, Timer_Hold);
+			timer_stop(&peer->timers, Timer_SendHold);
 			timer_stop(&peer->timers, Timer_IdleHold);
 			timer_stop(&peer->timers, Timer_IdleHoldReset);
 			session_close_connection(peer);
@@ -1780,6 +1789,10 @@ session_dispatch_msg(struct pollfd *pfd, struct peer *p)
 			return (1);
 		}
 		p->stats.last_write = getmonotime();
+		if (p->holdtime > 0)
+			timer_set(&p->timers, Timer_SendHold,
+			    p->holdtime < INTERVAL_HOLD ? INTERVAL_HOLD :
+			    p->holdtime);
 		if (p->throttled && p->wbuf.queued < SESS_MSG_LOW_MARK) {
 			if (imsg_rde(IMSG_XON, p->conf.id, NULL, 0) == -1)
 				log_peer_warn(&p->conf, "imsg_compose XON");
diff --git a/usr.sbin/bgpd/session.h b/usr.sbin/bgpd/session.h
index d927ff816e6..c3c85307ce6 100644
--- a/usr.sbin/bgpd/session.h
+++ b/usr.sbin/bgpd/session.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: session.h,v 1.148 2020/12/11 12:00:01 claudio Exp $ */
+/*	$OpenBSD: session.h,v 1.149 2020/12/23 13:20:48 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -59,6 +59,7 @@ enum session_events {
 	EVNT_TIMER_CONNRETRY,
 	EVNT_TIMER_HOLDTIME,
 	EVNT_TIMER_KEEPALIVE,
+	EVNT_TIMER_SENDHOLD,
 	EVNT_RCVD_OPEN,
 	EVNT_RCVD_KEEPALIVE,
 	EVNT_RCVD_UPDATE,
@@ -180,6 +181,7 @@ enum Timer {
 	Timer_ConnectRetry,
 	Timer_Keepalive,
 	Timer_Hold,
+	Timer_SendHold,
 	Timer_IdleHold,
 	Timer_IdleHoldReset,
 	Timer_CarpUndemote,