bgpd: Fix NHT race with Connect leading to test tool issues
* The NHT change:
"bgpd, zebra: Use next hop tracking for connected routes too"
introduces a race where bgp_connect_check can be called on a peer in
Connect state before the TCP handshake has completed. If this happens,
then the SO_ERROR sockopt to check the state of the socket is undefined
or at least does not return a useful result - it returns 0, as with
a connected socket. SO_ERROR should only be called on non-block sockets
after the socket has been ready for writing.
The net result is that bgpd can then incorrectly advance the peer FSM for
the socket (also the main 'peer'), to OpenSent. As part of which, any
incoming connection from the peer will pass through collision_detect and
may be (incorrectly) closed, depending on the RIDs.
This race is reliably hit with testing tools which wait to listen for
incoming BGP connections from the RUT to know it is in Connect/Active, and
which ignore the TCP connection (no SYN|ACK, no RST), and then launch their
own connection.
The fix is to better integrate the BGP FSM and the NHT update, to ensure
connect_check is not called on peers in Connect state.
Note: There may be no need at all for NHT to tickle FSM.
* bgpd.h: Add NHT_Update FSM event for NHT valid.
* bgp_fsm.c: (bgp_fsm_nht_update) There is no need to have a separate
switch based FSM with its own event via an exported function. Have
NHT raise the NHT_Update even on the peer, instead of calling a
side-channel function into a sub-FSM in the FSM. No need to have
code for BGP_Start, FSM can call that. Actions for Connect and
Active are the same and just lead to ConnectRetry_timer_expired
event - so FSM can just call same transition func as that.
No need to call bgp_connect_check on Connect, as Connect implies no
connection.
(FSM) Handle the NHT_Update event, replacing bgp_fsm_nht_update.
Idle -> bgp_start, Connect and Active were doing the same as
ConnectRetry_timer_expired so replicate those. Rest are No-Ops.
* bgp_nht.c: (evaluate_paths) Raise NHT_Update FSM event. Always valid.
* bgp_packet.{c,h}: (bgp_connect_check) NHT change now unnecessary, revert.
diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index d84a865..2e07d93 100644
--- a/bgpd/bgp_fsm.c
+++ b/bgpd/bgp_fsm.c
@@ -947,45 +947,6 @@
return 0;
}
-void
-bgp_fsm_nht_update(struct peer *peer, int valid)
-{
- int ret = 0;
-
- if (!peer)
- return;
-
- switch (peer->status)
- {
- case Idle:
- if (valid)
- BGP_EVENT_ADD(peer, BGP_Start);
- break;
- case Connect:
- ret = bgp_connect_check(peer, 0);
- if (!ret && valid)
- {
- BGP_TIMER_OFF(peer->t_connect);
- BGP_EVENT_ADD(peer, ConnectRetry_timer_expired);
- }
- break;
- case Active:
- if (valid)
- {
- BGP_TIMER_OFF(peer->t_connect);
- BGP_EVENT_ADD(peer, ConnectRetry_timer_expired);
- }
- case OpenSent:
- case OpenConfirm:
- case Established:
- case Clearing:
- case Deleted:
- default:
- break;
- }
-}
-
-
/* Finite State Machine structure */
static const struct {
int (*func) (struct peer *);
@@ -1010,6 +971,7 @@
{bgp_ignore, Idle}, /* Receive_UPDATE_message */
{bgp_ignore, Idle}, /* Receive_NOTIFICATION_message */
{bgp_ignore, Idle}, /* Clearing_Completed */
+ {bgp_start, Connect}, /* NHT_Update */
},
{
/* Connect */
@@ -1027,6 +989,7 @@
{bgp_ignore, Idle}, /* Receive_UPDATE_message */
{bgp_stop, Idle}, /* Receive_NOTIFICATION_message */
{bgp_ignore, Idle}, /* Clearing_Completed */
+ {bgp_reconnect, Connect},/* NHT_Update */
},
{
/* Active, */
@@ -1044,6 +1007,7 @@
{bgp_ignore, Idle}, /* Receive_UPDATE_message */
{bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */
{bgp_ignore, Idle}, /* Clearing_Completed */
+ {bgp_start, Connect}, /* NHT_Update */
},
{
/* OpenSent, */
@@ -1061,6 +1025,7 @@
{bgp_fsm_event_error, Idle}, /* Receive_UPDATE_message */
{bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */
{bgp_ignore, Idle}, /* Clearing_Completed */
+ {bgp_ignore, OpenSent}, /* NHT_Update */
},
{
/* OpenConfirm, */
@@ -1078,6 +1043,7 @@
{bgp_ignore, Idle}, /* Receive_UPDATE_message */
{bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */
{bgp_ignore, Idle}, /* Clearing_Completed */
+ {bgp_ignore, OpenConfirm}, /* NHT_Update */
},
{
/* Established, */
@@ -1095,6 +1061,7 @@
{bgp_fsm_update, Established}, /* Receive_UPDATE_message */
{bgp_stop_with_error, Clearing}, /* Receive_NOTIFICATION_message */
{bgp_ignore, Idle}, /* Clearing_Completed */
+ {bgp_ignore, Established}, /* NHT_Update */
},
{
/* Clearing, */
@@ -1112,6 +1079,7 @@
{bgp_stop, Clearing}, /* Receive_UPDATE_message */
{bgp_stop, Clearing}, /* Receive_NOTIFICATION_message */
{bgp_clearing_completed, Idle}, /* Clearing_Completed */
+ {bgp_ignore, Clearing}, /* NHT_Update */
},
{
/* Deleted, */
@@ -1129,6 +1097,7 @@
{bgp_ignore, Deleted}, /* Receive_UPDATE_message */
{bgp_ignore, Deleted}, /* Receive_NOTIFICATION_message */
{bgp_ignore, Deleted}, /* Clearing_Completed */
+ {bgp_ignore, Deleted}, /* NHT_Update */
},
};
@@ -1149,6 +1118,7 @@
"Receive_UPDATE_message",
"Receive_NOTIFICATION_message",
"Clearing_Completed",
+ "NHT_Update",
};
/* Execute event process. */
diff --git a/bgpd/bgp_fsm.h b/bgpd/bgp_fsm.h
index b38e64c..752d6e2 100644
--- a/bgpd/bgp_fsm.h
+++ b/bgpd/bgp_fsm.h
@@ -72,7 +72,6 @@
} while (0)
/* Prototypes. */
-extern void bgp_fsm_nht_update(struct peer *, int valid);
extern int bgp_event (struct thread *);
extern int bgp_stop (struct peer *peer);
extern void bgp_timer_set (struct peer *);
diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c
index 591fbf9..50f14ce 100644
--- a/bgpd/bgp_nht.c
+++ b/bgpd/bgp_nht.c
@@ -528,7 +528,7 @@
{
if (BGP_DEBUG(nht, NHT))
zlog_debug("%s: Updating peer (%s) status with NHT", __FUNCTION__, peer->host);
- bgp_fsm_nht_update(peer, CHECK_FLAG(bnc->flags, BGP_NEXTHOP_VALID));
+ BGP_EVENT_ADD (peer, NHT_Update);
SET_FLAG(bnc->flags, BGP_NEXTHOP_PEER_NOTIFIED);
}
diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index 2ec595f..f42e544 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -103,8 +103,8 @@
}
/* Check file descriptor whether connect is established. */
-int
-bgp_connect_check (struct peer *peer, int change_state)
+static void
+bgp_connect_check (struct peer *peer)
{
int status;
socklen_t slen;
@@ -123,23 +123,20 @@
{
zlog (peer->log, LOG_INFO, "can't get sockopt for nonblocking connect");
BGP_EVENT_ADD (peer, TCP_fatal_error);
- return -1;
+ return;
}
/* When status is 0 then TCP connection is established. */
if (status == 0)
{
BGP_EVENT_ADD (peer, TCP_connection_open);
- return 1;
}
else
{
if (BGP_DEBUG (events, EVENTS))
plog_debug (peer->log, "%s [Event] Connect failed (%s)",
peer->host, safe_strerror (errno));
- if (change_state)
- BGP_EVENT_ADD (peer, TCP_connection_open_failed);
- return 0;
+ BGP_EVENT_ADD (peer, TCP_connection_open_failed);
}
}
@@ -718,7 +715,7 @@
/* For non-blocking IO check. */
if (peer->status == Connect)
{
- bgp_connect_check (peer, 1);
+ bgp_connect_check (peer);
return 0;
}
@@ -2495,7 +2492,7 @@
/* For non-blocking IO check. */
if (peer->status == Connect)
{
- bgp_connect_check (peer, 1);
+ bgp_connect_check (peer);
goto done;
}
else
diff --git a/bgpd/bgp_packet.h b/bgpd/bgp_packet.h
index 74c62c0..6b0b7f4 100644
--- a/bgpd/bgp_packet.h
+++ b/bgpd/bgp_packet.h
@@ -40,7 +40,6 @@
/* Packet send and receive function prototypes. */
extern int bgp_read (struct thread *);
extern int bgp_write (struct thread *);
-extern int bgp_connect_check (struct peer *, int change_state);
extern void bgp_keepalive_send (struct peer *);
extern void bgp_open_send (struct peer *);
diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 87d2fff..0058b58 100644
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -747,7 +747,8 @@
#define Receive_UPDATE_message 12
#define Receive_NOTIFICATION_message 13
#define Clearing_Completed 14
-#define BGP_EVENTS_MAX 15
+#define NHT_Update 15
+#define BGP_EVENTS_MAX 16
/* BGP timers default value. */
#define BGP_INIT_START_TIMER 1