Merge pull request #12415 from donaldsharp/bgp_use_after_free

Bgp use after free

5 files changed, 236 insertions, 159 deletions

diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index fcb7275c34..65de35cbdb 100644
--- a/bgpd/bgp_fsm.c
+++ b/bgpd/bgp_fsm.c
@@ -61,6 +61,13 @@
 DEFINE_HOOK(peer_backward_transition, (struct peer * peer), (peer));
 DEFINE_HOOK(peer_status_changed, (struct peer * peer), (peer));
 
+enum bgp_fsm_state_progress {
+	BGP_FSM_FAILURE_AND_DELETE = -2,
+	BGP_FSM_FAILURE = -1,
+	BGP_FSM_SUCCESS = 0,
+	BGP_FSM_SUCCESS_STATE_TRANSFER = 1,
+};
+
 /* Definition of display strings corresponding to FSM events. This should be
  * kept consistent with the events defined in bgpd.h
  */
@@ -99,7 +106,7 @@ static void bgp_holdtime_timer(struct thread *);
 static void bgp_delayopen_timer(struct thread *);
 
 /* BGP FSM functions. */
-static int bgp_start(struct peer *);
+static enum bgp_fsm_state_progress bgp_start(struct peer *);
 
 /* Register peer with NHT */
 int bgp_peer_reg_with_nht(struct peer *peer)
@@ -1293,7 +1300,8 @@ void bgp_fsm_change_status(struct peer *peer, int status)
 		 * Clearing
 		 * (or Deleted).
 		 */
-		if (!work_queue_is_scheduled(peer->clear_node_queue))
+		if (!work_queue_is_scheduled(peer->clear_node_queue) &&
+		    status != Deleted)
 			BGP_EVENT_ADD(peer, Clearing_Completed);
 	}
 
@@ -1341,11 +1349,11 @@ void bgp_fsm_change_status(struct peer *peer, int status)
 }
 
 /* Flush the event queue and ensure the peer is shut down */
-static int bgp_clearing_completed(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_clearing_completed(struct peer *peer)
 {
-	int rc = bgp_stop(peer);
+	enum bgp_fsm_state_progress rc = bgp_stop(peer);
 
-	if (rc >= 0)
+	if (rc >= BGP_FSM_SUCCESS)
 		BGP_EVENT_FLUSH(peer);
 
 	return rc;
@@ -1353,12 +1361,12 @@ static int bgp_clearing_completed(struct peer *peer)
 
 /* Administrative BGP peer stop event. */
 /* May be called multiple times for the same peer */
-int bgp_stop(struct peer *peer)
+enum bgp_fsm_state_progress bgp_stop(struct peer *peer)
 {
 	afi_t afi;
 	safi_t safi;
 	char orf_name[BUFSIZ];
-	int ret = 0;
+	enum bgp_fsm_state_progress ret = BGP_FSM_SUCCESS;
 	struct bgp *bgp = peer->bgp;
 	struct graceful_restart_info *gr_info = NULL;
 
@@ -1375,7 +1383,7 @@ int bgp_stop(struct peer *peer)
 			zlog_debug("%s (dynamic neighbor) deleted (%s)",
 				   peer->host, __func__);
 		peer_delete(peer);
-		return -1;
+		return BGP_FSM_FAILURE_AND_DELETE;
 	}
 
 	/* Can't do this in Clearing; events are used for state transitions */
@@ -1584,7 +1592,7 @@ int bgp_stop(struct peer *peer)
 	if (!CHECK_FLAG(peer->flags, PEER_FLAG_CONFIG_NODE)
 	    && !(CHECK_FLAG(peer->flags, PEER_FLAG_DELETE))) {
 		peer_delete(peer);
-		ret = -1;
+		ret = BGP_FSM_FAILURE_AND_DELETE;
 	} else {
 		bgp_peer_conf_if_to_su_update(peer);
 	}
@@ -1592,7 +1600,7 @@ int bgp_stop(struct peer *peer)
 }
 
 /* BGP peer is stoped by the error. */
-static int bgp_stop_with_error(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_stop_with_error(struct peer *peer)
 {
 	/* Double start timer. */
 	peer->v_start *= 2;
@@ -1606,16 +1614,16 @@ static int bgp_stop_with_error(struct peer *peer)
 			zlog_debug("%s (dynamic neighbor) deleted (%s)",
 				   peer->host, __func__);
 		peer_delete(peer);
-		return -1;
+		return BGP_FSM_FAILURE;
 	}
 
-	return (bgp_stop(peer));
+	return bgp_stop(peer);
 }
 
 
 /* something went wrong, send notify and tear down */
-static int bgp_stop_with_notify(struct peer *peer, uint8_t code,
-				uint8_t sub_code)
+static enum bgp_fsm_state_progress
+bgp_stop_with_notify(struct peer *peer, uint8_t code, uint8_t sub_code)
 {
 	/* Send notify to remote peer */
 	bgp_notify_send(peer, code, sub_code);
@@ -1625,13 +1633,13 @@ static int bgp_stop_with_notify(struct peer *peer, uint8_t code,
 			zlog_debug("%s (dynamic neighbor) deleted (%s)",
 				   peer->host, __func__);
 		peer_delete(peer);
-		return -1;
+		return BGP_FSM_FAILURE;
 	}
 
 	/* Clear start timer value to default. */
 	peer->v_start = BGP_INIT_START_TIMER;
 
-	return (bgp_stop(peer));
+	return bgp_stop(peer);
 }
 
 /**
@@ -1696,13 +1704,12 @@ static void bgp_connect_check(struct thread *thread)
 
 /* TCP connection open.  Next we send open message to remote peer. And
    add read thread for reading open message. */
-static int bgp_connect_success(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_connect_success(struct peer *peer)
 {
 	if (peer->fd < 0) {
 		flog_err(EC_BGP_CONNECT, "%s peer's fd is negative value %d",
 			 __func__, peer->fd);
-		bgp_stop(peer);
-		return -1;
+		return bgp_stop(peer);
 	}
 
 	if (bgp_getsockname(peer) < 0) {
@@ -1712,7 +1719,7 @@ static int bgp_connect_success(struct peer *peer)
 		bgp_notify_send(peer, BGP_NOTIFY_FSM_ERR,
 				bgp_fsm_error_subcode(peer->status));
 		bgp_writes_on(peer);
-		return -1;
+		return BGP_FSM_FAILURE;
 	}
 
 	/*
@@ -1734,19 +1741,19 @@ static int bgp_connect_success(struct peer *peer)
 	/* Send an open message */
 	bgp_open_send(peer);
 
-	return 0;
+	return BGP_FSM_SUCCESS;
 }
 
 /* TCP connection open with RFC 4271 optional session attribute DelayOpen flag
  * set.
  */
-static int bgp_connect_success_w_delayopen(struct peer *peer)
+static enum bgp_fsm_state_progress
+bgp_connect_success_w_delayopen(struct peer *peer)
 {
 	if (peer->fd < 0) {
 		flog_err(EC_BGP_CONNECT, "%s: peer's fd is negative value %d",
 			 __func__, peer->fd);
-		bgp_stop(peer);
-		return -1;
+		return bgp_stop(peer);
 	}
 
 	if (bgp_getsockname(peer) < 0) {
@@ -1756,7 +1763,7 @@ static int bgp_connect_success_w_delayopen(struct peer *peer)
 		bgp_notify_send(peer, BGP_NOTIFY_FSM_ERR,
 				bgp_fsm_error_subcode(peer->status));
 		bgp_writes_on(peer);
-		return -1;
+		return BGP_FSM_FAILURE;
 	}
 
 	/*
@@ -1787,18 +1794,18 @@ static int bgp_connect_success_w_delayopen(struct peer *peer)
 		zlog_debug("%s [FSM] BGP OPEN message delayed for %d seconds",
 			   peer->host, peer->delayopen);
 
-	return 0;
+	return BGP_FSM_SUCCESS;
 }
 
 /* TCP connect fail */
-static int bgp_connect_fail(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_connect_fail(struct peer *peer)
 {
 	if (peer_dynamic_neighbor_no_nsf(peer)) {
 		if (bgp_debug_neighbor_events(peer))
 			zlog_debug("%s (dynamic neighbor) deleted (%s)",
 				   peer->host, __func__);
 		peer_delete(peer);
-		return -1;
+		return BGP_FSM_FAILURE_AND_DELETE;
 	}
 
 	/*
@@ -1807,13 +1814,13 @@ static int bgp_connect_fail(struct peer *peer)
 	 */
 	bgp_nht_interface_events(peer);
 
-	return (bgp_stop(peer));
+	return bgp_stop(peer);
 }
 
 /* This function is the first starting point of all BGP connection. It
  * try to connect to remote peer with non-blocking IO.
  */
-int bgp_start(struct peer *peer)
+enum bgp_fsm_state_progress bgp_start(struct peer *peer)
 {
 	int status;
 
@@ -1825,7 +1832,7 @@ int bgp_start(struct peer *peer)
 				"%s [FSM] Unable to get neighbor's IP address, waiting...",
 				peer->host);
 		peer->last_reset = PEER_DOWN_NBR_ADDR;
-		return -1;
+		return BGP_FSM_FAILURE;
 	}
 
 	if (BGP_PEER_START_SUPPRESSED(peer)) {
@@ -1841,7 +1848,7 @@ int bgp_start(struct peer *peer)
 			peer->last_reset = PEER_DOWN_USER_SHUTDOWN;
 		else if (CHECK_FLAG(peer->sflags, PEER_STATUS_PREFIX_OVERFLOW))
 			peer->last_reset = PEER_DOWN_PFX_COUNT;
-		return -1;
+		return BGP_FSM_FAILURE;
 	}
 
 	/* Scrub some information that might be left over from a previous,
@@ -1867,7 +1874,7 @@ int bgp_start(struct peer *peer)
 	/* If the peer is passive mode, force to move to Active mode. */
 	if (CHECK_FLAG(peer->flags, PEER_FLAG_PASSIVE)) {
 		BGP_EVENT_ADD(peer, TCP_connection_open_failed);
-		return 0;
+		return BGP_FSM_SUCCESS;
 	}
 
 	if (peer->bgp->vrf_id == VRF_UNKNOWN) {
@@ -1877,7 +1884,7 @@ int bgp_start(struct peer *peer)
 				"%s [FSM] In a VRF that is not initialised yet",
 				peer->host);
 		peer->last_reset = PEER_DOWN_VRF_UNINIT;
-		return -1;
+		return BGP_FSM_FAILURE;
 	}
 
 	/* Register peer for NHT. If next hop is already resolved, proceed
@@ -1891,7 +1898,7 @@ int bgp_start(struct peer *peer)
 					peer->host);
 			peer->last_reset = PEER_DOWN_WAITING_NHT;
 			BGP_EVENT_ADD(peer, TCP_connection_open_failed);
-			return 0;
+			return BGP_FSM_SUCCESS;
 		}
 	}
 
@@ -1926,7 +1933,7 @@ int bgp_start(struct peer *peer)
 			flog_err(EC_BGP_FSM,
 				 "%s peer's fd is negative value %d", __func__,
 				 peer->fd);
-			return -1;
+			return BGP_FSM_FAILURE;
 		}
 		/*
 		 * - when the socket becomes ready, poll() will signify POLLOUT
@@ -1943,24 +1950,26 @@ int bgp_start(struct peer *peer)
 				 &peer->t_connect_check_w);
 		break;
 	}
-	return 0;
+	return BGP_FSM_SUCCESS;
 }
 
 /* Connect retry timer is expired when the peer status is Connect. */
-static int bgp_reconnect(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_reconnect(struct peer *peer)
 {
-	if (bgp_stop(peer) < 0)
-		return -1;
+	enum bgp_fsm_state_progress ret;
+
+	ret = bgp_stop(peer);
+	if (ret < BGP_FSM_SUCCESS)
+		return ret;
 
 	/* Send graceful restart capabilty */
 	BGP_GR_ROUTER_DETECT_AND_SEND_CAPABILITY_TO_ZEBRA(peer->bgp,
 							  peer->bgp->peer);
 
-	bgp_start(peer);
-	return 0;
+	return bgp_start(peer);
 }
 
-static int bgp_fsm_open(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_fsm_open(struct peer *peer)
 {
 	/* If DelayOpen is active, we may still need to send an open message */
 	if ((peer->status == Connect) || (peer->status == Active))
@@ -1969,12 +1978,12 @@ static int bgp_fsm_open(struct peer *peer)
 	/* Send keepalive and make keepalive timer */
 	bgp_keepalive_send(peer);
 
-	return 0;
+	return BGP_FSM_SUCCESS;
 }
 
 /* FSM error, unexpected event.  This is error of BGP connection. So cut the
    peer and change to Idle status. */
-static int bgp_fsm_event_error(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_fsm_event_error(struct peer *peer)
 {
 	flog_err(EC_BGP_FSM, "%s [FSM] unexpected packet received in state %s",
 		 peer->host, lookup_msg(bgp_status_msg, peer->status, NULL));
@@ -1985,7 +1994,7 @@ static int bgp_fsm_event_error(struct peer *peer)
 
 /* Hold timer expire.  This is error of BGP connection. So cut the
    peer and change to Idle status. */
-static int bgp_fsm_holdtime_expire(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_fsm_holdtime_expire(struct peer *peer)
 {
 	if (bgp_debug_neighbor_events(peer))
 		zlog_debug("%s [FSM] Hold timer expire", peer->host);
@@ -2003,7 +2012,8 @@ static int bgp_fsm_holdtime_expire(struct peer *peer)
 }
 
 /* RFC 4271 DelayOpenTimer_Expires event */
-static int bgp_fsm_delayopen_timer_expire(struct peer *peer)
+static enum bgp_fsm_state_progress
+bgp_fsm_delayopen_timer_expire(struct peer *peer)
 {
 	/* Stop the DelayOpenTimer */
 	THREAD_OFF(peer->t_delayopen);
@@ -2014,7 +2024,7 @@ static int bgp_fsm_delayopen_timer_expire(struct peer *peer)
 	/* Set the HoldTimer to a large value (4 minutes) */
 	peer->v_holdtime = 245;
 
-	return 0;
+	return BGP_FSM_SUCCESS;
 }
 
 /* Start the selection deferral timer thread for the specified AFI, SAFI */
@@ -2095,25 +2105,28 @@ static int bgp_update_gr_info(struct peer *peer, afi_t afi, safi_t safi)
  * Convert peer from stub to full fledged peer, set some timers, and generate
  * initial updates.
  */
-static int bgp_establish(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_establish(struct peer *peer)
 {
 	afi_t afi;
 	safi_t safi;
 	int nsf_af_count = 0;
-	int ret = 0;
+	enum bgp_fsm_state_progress ret = BGP_FSM_SUCCESS;
 	struct peer *other;
 	int status;
 
 	other = peer->doppelganger;
+	hash_release(peer->bgp->peerhash, peer);
+	if (other)
+		hash_release(peer->bgp->peerhash, other);
+
 	peer = peer_xfer_conn(peer);
 	if (!peer) {
 		flog_err(EC_BGP_CONNECT, "%%Neighbor failed in xfer_conn");
-		return -1;
+		return BGP_FSM_FAILURE;
 	}
 
 	if (other == peer)
-		ret = 1; /* bgp_establish specific code when xfer_conn
-			    happens. */
+		ret = BGP_FSM_SUCCESS_STATE_TRANSFER;
 
 	/* Reset capability open status flag. */
 	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_CAPABILITY_OPEN))
@@ -2309,7 +2322,6 @@ static int bgp_establish(struct peer *peer)
 	 * the doppelgangers su and this peer's su are the same
 	 * so the hash_release is the same for either.
 	 */
-	hash_release(peer->bgp->peerhash, peer);
 	(void)hash_get(peer->bgp->peerhash, peer, hash_alloc_intern);
 
 	/* Start BFD peer if not already running. */
@@ -2320,21 +2332,21 @@ static int bgp_establish(struct peer *peer)
 }
 
 /* Keepalive packet is received. */
-static int bgp_fsm_keepalive(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_fsm_keepalive(struct peer *peer)
 {
 	THREAD_OFF(peer->t_holdtime);
-	return 0;
+	return BGP_FSM_SUCCESS;
 }
 
 /* Update packet is received. */
-static int bgp_fsm_update(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_fsm_update(struct peer *peer)
 {
 	THREAD_OFF(peer->t_holdtime);
-	return 0;
+	return BGP_FSM_SUCCESS;
 }
 
 /* This is empty event. */
-static int bgp_ignore(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_ignore(struct peer *peer)
 {
 	flog_err(
 		EC_BGP_FSM,
@@ -2343,11 +2355,11 @@ static int bgp_ignore(struct peer *peer)
 		lookup_msg(bgp_status_msg, peer->status, NULL),
 		bgp_event_str[peer->last_event],
 		bgp_event_str[peer->last_major_event], peer->fd);
-	return 0;
+	return BGP_FSM_SUCCESS;
 }
 
 /* This is to handle unexpected events.. */
-static int bgp_fsm_exeption(struct peer *peer)
+static enum bgp_fsm_state_progress bgp_fsm_exception(struct peer *peer)
 {
 	flog_err(
 		EC_BGP_FSM,
@@ -2356,7 +2368,7 @@ static int bgp_fsm_exeption(struct peer *peer)
 		lookup_msg(bgp_status_msg, peer->status, NULL),
 		bgp_event_str[peer->last_event],
 		bgp_event_str[peer->last_major_event], peer->fd);
-	return (bgp_stop(peer));
+	return bgp_stop(peer);
 }
 
 void bgp_fsm_nht_update(struct peer *peer, bool has_valid_nexthops)
@@ -2397,7 +2409,7 @@ void bgp_fsm_nht_update(struct peer *peer, bool has_valid_nexthops)
 
 /* Finite State Machine structure */
 static const struct {
-	int (*func)(struct peer *);
+	enum bgp_fsm_state_progress (*func)(struct peer *);
 	enum bgp_fsm_status next_state;
 } FSM[BGP_STATUS_MAX - 1][BGP_EVENTS_MAX - 1] = {
 	{
@@ -2428,19 +2440,19 @@ static const struct {
 		{bgp_connect_success, OpenSent}, /* TCP_connection_open */
 		{bgp_connect_success_w_delayopen,
 		 Connect},		    /* TCP_connection_open_w_delay */
-		{bgp_stop, Idle},	   /* TCP_connection_closed        */
+		{bgp_stop, Idle},	    /* TCP_connection_closed        */
 		{bgp_connect_fail, Active}, /* TCP_connection_open_failed   */
 		{bgp_connect_fail, Idle},   /* TCP_fatal_error              */
 		{bgp_reconnect, Connect},   /* ConnectRetry_timer_expired   */
-		{bgp_fsm_exeption, Idle},   /* Hold_Timer_expired           */
-		{bgp_fsm_exeption, Idle},   /* KeepAlive_timer_expired      */
+		{bgp_fsm_exception, Idle},  /* Hold_Timer_expired           */
+		{bgp_fsm_exception, Idle},  /* KeepAlive_timer_expired      */
 		{bgp_fsm_delayopen_timer_expire,
 		 OpenSent},		     /* DelayOpen_timer_expired */
 		{bgp_fsm_open, OpenConfirm}, /* Receive_OPEN_message         */
-		{bgp_fsm_exeption, Idle},    /* Receive_KEEPALIVE_message    */
-		{bgp_fsm_exeption, Idle},    /* Receive_UPDATE_message       */
-		{bgp_stop, Idle},	    /* Receive_NOTIFICATION_message */
-		{bgp_fsm_exeption, Idle},    /* Clearing_Completed           */
+		{bgp_fsm_exception, Idle},   /* Receive_KEEPALIVE_message    */
+		{bgp_fsm_exception, Idle},   /* Receive_UPDATE_message       */
+		{bgp_stop, Idle},	     /* Receive_NOTIFICATION_message */
+		{bgp_fsm_exception, Idle},   /* Clearing_Completed           */
 	},
 	{
 		/* Active, */
@@ -2448,97 +2460,97 @@ static const struct {
 		{bgp_stop, Idle},     /* BGP_Stop                     */
 		{bgp_connect_success, OpenSent}, /* TCP_connection_open */
 		{bgp_connect_success_w_delayopen,
-		 Active},		  /* TCP_connection_open_w_delay */
-		{bgp_stop, Idle},	 /* TCP_connection_closed        */
-		{bgp_ignore, Active},     /* TCP_connection_open_failed   */
-		{bgp_fsm_exeption, Idle}, /* TCP_fatal_error              */
-		{bgp_start, Connect},     /* ConnectRetry_timer_expired   */
-		{bgp_fsm_exeption, Idle}, /* Hold_Timer_expired           */
-		{bgp_fsm_exeption, Idle}, /* KeepAlive_timer_expired      */
+		 Active},		   /* TCP_connection_open_w_delay */
+		{bgp_stop, Idle},	   /* TCP_connection_closed        */
+		{bgp_ignore, Active},	   /* TCP_connection_open_failed   */
+		{bgp_fsm_exception, Idle}, /* TCP_fatal_error              */
+		{bgp_start, Connect},	   /* ConnectRetry_timer_expired   */
+		{bgp_fsm_exception, Idle}, /* Hold_Timer_expired           */
+		{bgp_fsm_exception, Idle}, /* KeepAlive_timer_expired      */
 		{bgp_fsm_delayopen_timer_expire,
 		 OpenSent},		     /* DelayOpen_timer_expired */
 		{bgp_fsm_open, OpenConfirm}, /* Receive_OPEN_message         */
-		{bgp_fsm_exeption, Idle},    /* Receive_KEEPALIVE_message    */
-		{bgp_fsm_exeption, Idle},    /* Receive_UPDATE_message       */
-		{bgp_fsm_exeption, Idle},    /* Receive_NOTIFICATION_message */
-		{bgp_fsm_exeption, Idle},    /* Clearing_Completed           */
+		{bgp_fsm_exception, Idle},   /* Receive_KEEPALIVE_message    */
+		{bgp_fsm_exception, Idle},   /* Receive_UPDATE_message       */
+		{bgp_fsm_exception, Idle},   /* Receive_NOTIFICATION_message */
+		{bgp_fsm_exception, Idle},   /* Clearing_Completed           */
 	},
 	{
 		/* OpenSent, */
-		{bgp_ignore, OpenSent},   /* BGP_Start                    */
-		{bgp_stop, Idle},	 /* BGP_Stop                     */
-		{bgp_stop, Active},       /* TCP_connection_open          */
-		{bgp_fsm_exeption, Idle}, /* TCP_connection_open_w_delay */
-		{bgp_stop, Active},       /* TCP_connection_closed        */
-		{bgp_stop, Active},       /* TCP_connection_open_failed   */
-		{bgp_stop, Active},       /* TCP_fatal_error              */
-		{bgp_fsm_exeption, Idle}, /* ConnectRetry_timer_expired   */
+		{bgp_ignore, OpenSent},	   /* BGP_Start                    */
+		{bgp_stop, Idle},	   /* BGP_Stop                     */
+		{bgp_stop, Active},	   /* TCP_connection_open          */
+		{bgp_fsm_exception, Idle}, /* TCP_connection_open_w_delay */
+		{bgp_stop, Active},	   /* TCP_connection_closed        */
+		{bgp_stop, Active},	   /* TCP_connection_open_failed   */
+		{bgp_stop, Active},	   /* TCP_fatal_error              */
+		{bgp_fsm_exception, Idle}, /* ConnectRetry_timer_expired   */
 		{bgp_fsm_holdtime_expire, Idle}, /* Hold_Timer_expired */
-		{bgp_fsm_exeption, Idle},    /* KeepAlive_timer_expired      */
-		{bgp_fsm_exeption, Idle},    /* DelayOpen_timer_expired */
+		{bgp_fsm_exception, Idle},   /* KeepAlive_timer_expired      */
+		{bgp_fsm_exception, Idle},   /* DelayOpen_timer_expired */
 		{bgp_fsm_open, OpenConfirm}, /* Receive_OPEN_message         */
 		{bgp_fsm_event_error, Idle}, /* Receive_KEEPALIVE_message    */
 		{bgp_fsm_event_error, Idle}, /* Receive_UPDATE_message       */
 		{bgp_fsm_event_error, Idle}, /* Receive_NOTIFICATION_message */
-		{bgp_fsm_exeption, Idle},    /* Clearing_Completed           */
+		{bgp_fsm_exception, Idle},   /* Clearing_Completed           */
 	},
 	{
 		/* OpenConfirm, */
 		{bgp_ignore, OpenConfirm}, /* BGP_Start                    */
-		{bgp_stop, Idle},	  /* BGP_Stop                     */
-		{bgp_stop, Idle},	  /* TCP_connection_open          */
-		{bgp_fsm_exeption, Idle},  /* TCP_connection_open_w_delay */
-		{bgp_stop, Idle},	  /* TCP_connection_closed        */
-		{bgp_stop, Idle},	  /* TCP_connection_open_failed   */
-		{bgp_stop, Idle},	  /* TCP_fatal_error              */
-		{bgp_fsm_exeption, Idle},  /* ConnectRetry_timer_expired   */
+		{bgp_stop, Idle},	   /* BGP_Stop                     */
+		{bgp_stop, Idle},	   /* TCP_connection_open          */
+		{bgp_fsm_exception, Idle}, /* TCP_connection_open_w_delay */
+		{bgp_stop, Idle},	   /* TCP_connection_closed        */
+		{bgp_stop, Idle},	   /* TCP_connection_open_failed   */
+		{bgp_stop, Idle},	   /* TCP_fatal_error              */
+		{bgp_fsm_exception, Idle}, /* ConnectRetry_timer_expired   */
 		{bgp_fsm_holdtime_expire, Idle}, /* Hold_Timer_expired */
 		{bgp_ignore, OpenConfirm},    /* KeepAlive_timer_expired      */
-		{bgp_fsm_exeption, Idle},     /* DelayOpen_timer_expired */
-		{bgp_fsm_exeption, Idle},     /* Receive_OPEN_message         */
+		{bgp_fsm_exception, Idle},    /* DelayOpen_timer_expired */
+		{bgp_fsm_exception, Idle},    /* Receive_OPEN_message         */
 		{bgp_establish, Established}, /* Receive_KEEPALIVE_message    */
-		{bgp_fsm_exeption, Idle},     /* Receive_UPDATE_message       */
+		{bgp_fsm_exception, Idle},    /* Receive_UPDATE_message       */
 		{bgp_stop_with_error, Idle},  /* Receive_NOTIFICATION_message */
-		{bgp_fsm_exeption, Idle},     /* Clearing_Completed           */
+		{bgp_fsm_exception, Idle},    /* Clearing_Completed           */
 	},
 	{
 		/* Established, */
 		{bgp_ignore, Established}, /* BGP_Start                    */
-		{bgp_stop, Clearing},      /* BGP_Stop                     */
-		{bgp_stop, Clearing},      /* TCP_connection_open          */
-		{bgp_fsm_exeption, Idle},  /* TCP_connection_open_w_delay */
-		{bgp_stop, Clearing},      /* TCP_connection_closed        */
-		{bgp_stop, Clearing},      /* TCP_connection_open_failed   */
-		{bgp_stop, Clearing},      /* TCP_fatal_error              */
-		{bgp_stop, Clearing},      /* ConnectRetry_timer_expired   */
+		{bgp_stop, Clearing},	   /* BGP_Stop                     */
+		{bgp_stop, Clearing},	   /* TCP_connection_open          */
+		{bgp_fsm_exception, Idle}, /* TCP_connection_open_w_delay */
+		{bgp_stop, Clearing},	   /* TCP_connection_closed        */
+		{bgp_stop, Clearing},	   /* TCP_connection_open_failed   */
+		{bgp_stop, Clearing},	   /* TCP_fatal_error              */
+		{bgp_stop, Clearing},	   /* ConnectRetry_timer_expired   */
 		{bgp_fsm_holdtime_expire, Clearing}, /* Hold_Timer_expired */
 		{bgp_ignore, Established}, /* KeepAlive_timer_expired      */
-		{bgp_fsm_exeption, Idle},  /* DelayOpen_timer_expired */
-		{bgp_stop, Clearing},      /* Receive_OPEN_message         */
+		{bgp_fsm_exception, Idle}, /* DelayOpen_timer_expired */
+		{bgp_stop, Clearing},	   /* Receive_OPEN_message         */
 		{bgp_fsm_keepalive,
 		 Established}, /* Receive_KEEPALIVE_message    */
 		{bgp_fsm_update, Established}, /* Receive_UPDATE_message */
 		{bgp_stop_with_error,
-		 Clearing},		  /* Receive_NOTIFICATION_message */
-		{bgp_fsm_exeption, Idle}, /* Clearing_Completed           */
+		 Clearing},		   /* Receive_NOTIFICATION_message */
+		{bgp_fsm_exception, Idle}, /* Clearing_Completed           */
 	},
 	{
 		/* Clearing, */
 		{bgp_ignore, Clearing}, /* BGP_Start                    */
-		{bgp_stop, Clearing},   /* BGP_Stop                     */
-		{bgp_stop, Clearing},   /* TCP_connection_open          */
-		{bgp_stop, Clearing},   /* TCP_connection_open_w_delay */
-		{bgp_stop, Clearing},   /* TCP_connection_closed        */
-		{bgp_stop, Clearing},   /* TCP_connection_open_failed   */
-		{bgp_stop, Clearing},   /* TCP_fatal_error              */
-		{bgp_stop, Clearing},   /* ConnectRetry_timer_expired   */
-		{bgp_stop, Clearing},   /* Hold_Timer_expired           */
-		{bgp_stop, Clearing},   /* KeepAlive_timer_expired      */
-		{bgp_stop, Clearing},   /* DelayOpen_timer_expired */
-		{bgp_stop, Clearing},   /* Receive_OPEN_message         */
-		{bgp_stop, Clearing},   /* Receive_KEEPALIVE_message    */
-		{bgp_stop, Clearing},   /* Receive_UPDATE_message       */
-		{bgp_stop, Clearing},   /* Receive_NOTIFICATION_message */
+		{bgp_stop, Clearing},	/* BGP_Stop                     */
+		{bgp_stop, Clearing},	/* TCP_connection_open          */
+		{bgp_stop, Clearing},	/* TCP_connection_open_w_delay */
+		{bgp_stop, Clearing},	/* TCP_connection_closed        */
+		{bgp_stop, Clearing},	/* TCP_connection_open_failed   */
+		{bgp_stop, Clearing},	/* TCP_fatal_error              */
+		{bgp_stop, Clearing},	/* ConnectRetry_timer_expired   */
+		{bgp_stop, Clearing},	/* Hold_Timer_expired           */
+		{bgp_stop, Clearing},	/* KeepAlive_timer_expired      */
+		{bgp_stop, Clearing},	/* DelayOpen_timer_expired */
+		{bgp_stop, Clearing},	/* Receive_OPEN_message         */
+		{bgp_stop, Clearing},	/* Receive_KEEPALIVE_message    */
+		{bgp_stop, Clearing},	/* Receive_UPDATE_message       */
+		{bgp_stop, Clearing},	/* Receive_NOTIFICATION_message */
 		{bgp_clearing_completed, Idle}, /* Clearing_Completed */
 	},
 	{
@@ -2571,13 +2583,15 @@ void bgp_event(struct thread *thread)
 	peer = THREAD_ARG(thread);
 	event = THREAD_VAL(thread);
 
+	peer_lock(peer);
 	bgp_event_update(peer, event);
+	peer_unlock(peer);
 }
 
 int bgp_event_update(struct peer *peer, enum bgp_fsm_events event)
 {
 	enum bgp_fsm_status next;
-	int ret = 0;
+	enum bgp_fsm_state_progress ret = 0;
 	struct peer *other;
 	int passive_conn = 0;
 	int dyn_nbr;
@@ -2606,8 +2620,9 @@ int bgp_event_update(struct peer *peer, enum bgp_fsm_events event)
 	if (FSM[peer->status - 1][event - 1].func)
 		ret = (*(FSM[peer->status - 1][event - 1].func))(peer);
 
-	if (ret >= 0) {
-		if (ret == 1 && next == Established) {
+	if (ret >= BGP_FSM_SUCCESS) {
+		if (ret == BGP_FSM_SUCCESS_STATE_TRANSFER &&
+		    next == Established) {
 			/* The case when doppelganger swap accurred in
 			   bgp_establish.
 			   Update the peer pointer accordingly */
@@ -2641,7 +2656,8 @@ int bgp_event_update(struct peer *peer, enum bgp_fsm_events event)
 		 * we need to indicate that the peer was stopped in the return
 		 * code.
 		 */
-		if (!dyn_nbr && !passive_conn && peer->bgp) {
+		if (!dyn_nbr && !passive_conn && peer->bgp &&
+		    ret != BGP_FSM_FAILURE_AND_DELETE) {
 			flog_err(
 				EC_BGP_FSM,
 				"%s [FSM] Failure handling event %s in state %s, prior events %s, %s, fd %d",
diff --git a/bgpd/bgp_network.c b/bgpd/bgp_network.c
index b43f8c8664..7186a50711 100644
--- a/bgpd/bgp_network.c
+++ b/bgpd/bgp_network.c
@@ -553,9 +553,7 @@ static void bgp_accept(struct thread *thread)
 				peer1->host);
 
 	peer = peer_create(&su, peer1->conf_if, peer1->bgp, peer1->local_as,
-			   peer1->as, peer1->as_type, NULL);
-	hash_release(peer->bgp->peerhash, peer);
-	(void)hash_get(peer->bgp->peerhash, peer, hash_alloc_intern);
+			   peer1->as, peer1->as_type, NULL, false);
 
 	peer_xfer_config(peer, peer1);
 	bgp_peer_gr_flags_update(peer);
@@ -572,8 +570,6 @@ static void bgp_accept(struct thread *thread)
 		}
 	}
 
-	UNSET_FLAG(peer->flags, PEER_FLAG_CONFIG_NODE);
-
 	peer->doppelganger = peer1;
 	peer1->doppelganger = peer;
 	peer->fd = bgp_sock;
diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index 862b5bdd1f..7b9400118b 100644
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -4678,7 +4678,7 @@ static int peer_conf_interface_get(struct vty *vty, const char *conf_if,
 			ret = peer_remote_as(bgp, NULL, conf_if, &as, as_type);
 	} else {
 		peer = peer_create(NULL, conf_if, bgp, bgp->as, as, as_type,
-				   NULL);
+				   NULL, true);
 
 		if (!peer) {
 			vty_out(vty, "%% BGP failed to create peer\n");
diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index e6a7a4cc43..734e44f252 100644
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -950,6 +950,7 @@ static bool peer_hash_same(const void *p1, const void *p2)
 {
 	const struct peer *peer1 = p1;
 	const struct peer *peer2 = p2;
+
 	return (sockunion_same(&peer1->su, &peer2->su)
 		&& CHECK_FLAG(peer1->flags, PEER_FLAG_CONFIG_NODE)
 			   == CHECK_FLAG(peer2->flags, PEER_FLAG_CONFIG_NODE));
@@ -1127,6 +1128,7 @@ static void peer_free(struct peer *peer)
 	bgp_timer_set(peer);
 	bgp_reads_off(peer);
 	bgp_writes_off(peer);
+	thread_cancel_event_ready(bm->master, peer);
 	FOREACH_AFI_SAFI (afi, safi)
 		THREAD_OFF(peer->t_revalidate_all[afi][safi]);
 	assert(!peer->t_write);
@@ -1457,7 +1459,10 @@ void peer_xfer_config(struct peer *peer_dst, struct peer *peer_src)
 
 	/* peer flags apply */
 	peer_dst->flags = peer_src->flags;
-
+	/*
+	 * The doppelganger *must* not have a config node stored
+	 */
+	UNSET_FLAG(peer_dst->flags, PEER_FLAG_CONFIG_NODE);
 	peer_dst->peer_gr_present_state = peer_src->peer_gr_present_state;
 	peer_dst->peer_gr_new_status_flag = peer_src->peer_gr_new_status_flag;
 
@@ -1613,15 +1618,18 @@ void bgp_peer_conf_if_to_su_update(struct peer *peer)
 	struct interface *ifp;
 	int prev_family;
 	int peer_addr_updated = 0;
+	struct listnode *node;
+	union sockunion old_su;
 
+	/*
+	 * This function is only ever needed when FRR an interface
+	 * based peering, so this simple test will tell us if
+	 * we are in an interface based configuration or not
+	 */
 	if (!peer->conf_if)
 		return;
 
-	/*
-	 * Our peer structure is stored in the bgp->peerhash
-	 * release it before we modify anything.
-	 */
-	hash_release(peer->bgp->peerhash, peer);
+	old_su = peer->su;
 
 	prev_family = peer->su.sa.sa_family;
 	if ((ifp = if_lookup_by_name(peer->conf_if, peer->bgp->vrf_id))) {
@@ -1664,9 +1672,48 @@ void bgp_peer_conf_if_to_su_update(struct peer *peer)
 	}
 
 	/*
-	 * Since our su changed we need to del/add peer to the peerhash
+	 * If they are the same, nothing to do here, move along
 	 */
-	(void)hash_get(peer->bgp->peerhash, peer, hash_alloc_intern);
+	if (!sockunion_same(&old_su, &peer->su)) {
+		union sockunion new_su = peer->su;
+		struct bgp *bgp = peer->bgp;
+
+		/*
+		 * Our peer structure is stored in the bgp->peerhash
+		 * release it before we modify anything in both the
+		 * hash and the list.  But *only* if the peer
+		 * is in the bgp->peerhash as that on deletion
+		 * we call bgp_stop which calls this function :(
+		 * so on deletion let's remove from the list first
+		 * and then do the deletion preventing this from
+		 * being added back on the list below when we
+		 * fail to remove it up here.
+		 */
+
+		/*
+		 * listnode_lookup just scans the list
+		 * for the peer structure so it's safe
+		 * to use without modifying the su
+		 */
+		node = listnode_lookup(bgp->peer, peer);
+		if (node) {
+			/*
+			 * Let's reset the peer->su release and
+			 * reset it and put it back.  We have to
+			 * do this because hash_release will
+			 * scan through looking for a matching
+			 * su if needed.
+			 */
+			peer->su = old_su;
+			hash_release(peer->bgp->peerhash, peer);
+			listnode_delete(peer->bgp->peer, peer);
+
+			peer->su = new_su;
+			(void)hash_get(peer->bgp->peerhash, peer,
+				       hash_alloc_intern);
+			listnode_add_sort(peer->bgp->peer, peer);
+		}
+	}
 }
 
 void bgp_recalculate_afi_safi_bestpaths(struct bgp *bgp, afi_t afi, safi_t safi)
@@ -1715,7 +1762,8 @@ void bgp_recalculate_all_bestpaths(struct bgp *bgp)
  */
 struct peer *peer_create(union sockunion *su, const char *conf_if,
 			 struct bgp *bgp, as_t local_as, as_t remote_as,
-			 int as_type, struct peer_group *group)
+			 int as_type, struct peer_group *group,
+			 bool config_node)
 {
 	int active;
 	struct peer *peer;
@@ -1753,6 +1801,10 @@ struct peer *peer_create(union sockunion *su, const char *conf_if,
 	peer = peer_lock(peer); /* bgp peer list reference */
 	peer->group = group;
 	listnode_add_sort(bgp->peer, peer);
+
+	if (config_node)
+		SET_FLAG(peer->flags, PEER_FLAG_CONFIG_NODE);
+
 	(void)hash_get(bgp->peerhash, peer, hash_alloc_intern);
 
 	/* Adjust update-group coalesce timer heuristics for # peers. */
@@ -1780,8 +1832,6 @@ struct peer *peer_create(union sockunion *su, const char *conf_if,
 	/* Default configured keepalives count for shutdown rtt command */
 	peer->rtt_keepalive_conf = 1;
 
-	SET_FLAG(peer->flags, PEER_FLAG_CONFIG_NODE);
-
 	/* If 'bgp default <afi>-<safi>' is configured, then activate the
 	 * neighbor for the corresponding address family. IPv4 Unicast is
 	 * the only address family enabled by default without expliict
@@ -1816,6 +1866,7 @@ struct peer *peer_create_accept(struct bgp *bgp)
 
 	peer = peer_lock(peer); /* bgp peer list reference */
 	listnode_add_sort(bgp->peer, peer);
+	(void)hash_get(bgp->peerhash, peer, hash_alloc_intern);
 
 	return peer;
 }
@@ -1987,7 +2038,8 @@ int peer_remote_as(struct bgp *bgp, union sockunion *su, const char *conf_if,
 		else
 			local_as = bgp->as;
 
-		peer_create(su, conf_if, bgp, local_as, *as, as_type, NULL);
+		peer_create(su, conf_if, bgp, local_as, *as, as_type, NULL,
+			    true);
 	}
 
 	return 0;
@@ -2446,6 +2498,7 @@ int peer_delete(struct peer *peer)
 	bgp_keepalives_off(peer);
 	bgp_reads_off(peer);
 	bgp_writes_off(peer);
+	thread_cancel_event_ready(bm->master, peer);
 	FOREACH_AFI_SAFI (afi, safi)
 		THREAD_OFF(peer->t_revalidate_all[afi][safi]);
 	assert(!CHECK_FLAG(peer->thread_flags, PEER_THREAD_WRITES_ON));
@@ -2509,9 +2562,16 @@ int peer_delete(struct peer *peer)
 	/* Delete from all peer list. */
 	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)
 	    && (pn = listnode_lookup(bgp->peer, peer))) {
-		peer_unlock(peer); /* bgp peer list reference */
+		/*
+		 * Removing from the list node first because
+		 * peer_unlock *can* call peer_delete( I know,
+		 * I know ).  So let's remove it and in
+		 * the su recalculate function we'll ensure
+		 * it's in there or not.
+		 */
 		list_delete_node(bgp->peer, pn);
 		hash_release(bgp->peerhash, peer);
+		peer_unlock(peer); /* bgp peer list reference */
 	}
 
 	/* Buffers.  */
@@ -2649,6 +2709,7 @@ static void peer_group2peer_config_copy(struct peer_group *group,
 {
 	uint32_t flags_tmp;
 	struct peer *conf;
+	bool config_node = !!CHECK_FLAG(peer->flags, PEER_FLAG_CONFIG_NODE);
 
 	conf = group->conf;
 
@@ -2676,6 +2737,9 @@ static void peer_group2peer_config_copy(struct peer_group *group,
 	SET_FLAG(peer->flags, flags_tmp);
 	SET_FLAG(peer->flags_invert, conf->flags_invert);
 
+	if (config_node)
+		SET_FLAG(peer->flags, PEER_FLAG_CONFIG_NODE);
+
 	/* peer timers apply */
 	if (!CHECK_FLAG(peer->flags_override, PEER_FLAG_TIMER)) {
 		PEER_ATTR_INHERIT(peer, group, holdtime);
@@ -3077,7 +3141,7 @@ int peer_group_bind(struct bgp *bgp, union sockunion *su, struct peer *peer,
 		}
 
 		peer = peer_create(su, NULL, bgp, bgp->as, group->conf->as,
-				   group->conf->as_type, group);
+				   group->conf->as_type, group, true);
 
 		peer = peer_lock(peer); /* group->peer list reference */
 		listnode_add(group->peer, peer);
@@ -3099,8 +3163,6 @@ int peer_group_bind(struct bgp *bgp, union sockunion *su, struct peer *peer,
 				peer_deactivate(peer, afi, safi);
 		}
 
-		SET_FLAG(peer->flags, PEER_FLAG_CONFIG_NODE);
-
 		/* Set up peer's events and timers. */
 		if (peer_active(peer))
 			bgp_timer_set(peer);
@@ -3723,8 +3785,10 @@ int bgp_delete(struct bgp *bgp)
 	for (ALL_LIST_ELEMENTS(bgp->group, node, next, group))
 		peer_group_delete(group);
 
-	for (ALL_LIST_ELEMENTS(bgp->peer, node, next, peer))
+	while (listcount(bgp->peer)) {
+		peer = listnode_head(bgp->peer);
 		peer_delete(peer);
+	}
 
 	if (bgp->peer_self) {
 		peer_delete(bgp->peer_self);
@@ -3971,7 +4035,7 @@ struct peer *peer_create_bind_dynamic_neighbor(struct bgp *bgp,
 
 	/* Create peer first; we've already checked group config is valid. */
 	peer = peer_create(su, NULL, bgp, bgp->as, group->conf->as,
-			   group->conf->as_type, group);
+			   group->conf->as_type, group, true);
 	if (!peer)
 		return NULL;
 
@@ -4000,7 +4064,6 @@ struct peer *peer_create_bind_dynamic_neighbor(struct bgp *bgp,
 	/* Mark as dynamic, but also as a "config node" for other things to
 	 * work. */
 	SET_FLAG(peer->flags, PEER_FLAG_DYNAMIC_NEIGHBOR);
-	SET_FLAG(peer->flags, PEER_FLAG_CONFIG_NODE);
 
 	return peer;
 }
diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index b7a329fdcd..a75bfdf746 100644
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -2177,8 +2177,10 @@ extern bool peer_active_nego(struct peer *);
 extern bool peer_afc_received(struct peer *peer);
 extern bool peer_afc_advertised(struct peer *peer);
 extern void bgp_recalculate_all_bestpaths(struct bgp *bgp);
-extern struct peer *peer_create(union sockunion *, const char *, struct bgp *,
-				as_t, as_t, int, struct peer_group *);
+extern struct peer *peer_create(union sockunion *su, const char *conf_if,
+				struct bgp *bgp, as_t local_as, as_t remote_as,
+				int as_type, struct peer_group *group,
+				bool config_node);
 extern struct peer *peer_create_accept(struct bgp *);
 extern void peer_xfer_config(struct peer *dst, struct peer *src);
 extern char *peer_uptime(time_t uptime2, char *buf, size_t len, bool use_json,