Browse Source

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.
Paul Jakma 3 years ago
parent
commit
743dd42b3f
6 changed files with 18 additions and 52 deletions
  1. 9 39
      bgpd/bgp_fsm.c
  2. 0 1
      bgpd/bgp_fsm.h
  3. 1 1
      bgpd/bgp_nht.c
  4. 6 9
      bgpd/bgp_packet.c
  5. 0 1
      bgpd/bgp_packet.h
  6. 2 1
      bgpd/bgpd.h

+ 9 - 39
bgpd/bgp_fsm.c

@@ -947,45 +947,6 @@ bgp_ignore (struct peer *peer)
   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 @@ static const struct {
     {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 @@ static const struct {
     {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 @@ static const struct {
     {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 @@ static const struct {
     {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 @@ static const struct {
     {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 @@ static const struct {
     {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 @@ static const struct {
     {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 @@ static const struct {
     {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 @@ static const char *bgp_event_str[] =
   "Receive_UPDATE_message",
   "Receive_NOTIFICATION_message",
   "Clearing_Completed",
+  "NHT_Update",
 };
 
 /* Execute event process. */

+ 0 - 1
bgpd/bgp_fsm.h

@@ -72,7 +72,6 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
   } 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 *);

+ 1 - 1
bgpd/bgp_nht.c

@@ -528,7 +528,7 @@ evaluate_paths (struct bgp_nexthop_cache *bnc)
     {
       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);
     }
 

+ 6 - 9
bgpd/bgp_packet.c

@@ -103,8 +103,8 @@ bgp_packet_delete (struct peer *peer)
 }
 
 /* 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 @@ bgp_connect_check (struct peer *peer, int change_state)
     {
       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 @@ bgp_write (struct thread *thread)
   /* For non-blocking IO check. */
   if (peer->status == Connect)
     {
-      bgp_connect_check (peer, 1);
+      bgp_connect_check (peer);
       return 0;
     }
 
@@ -2495,7 +2492,7 @@ bgp_read (struct thread *thread)
   /* For non-blocking IO check. */
   if (peer->status == Connect)
     {
-      bgp_connect_check (peer, 1);
+      bgp_connect_check (peer);
       goto done;
     }
   else

+ 0 - 1
bgpd/bgp_packet.h

@@ -40,7 +40,6 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
 /* 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 *);

+ 2 - 1
bgpd/bgpd.h

@@ -747,7 +747,8 @@ struct bgp_nlri
 #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