bgpd: fix race condition causing occasional assert

If a BGP message header fails validation we send a BGP NOTIFICATION from
the I/O thread. At this time we clear the output buffer, push a
NOTIFICATION and then call the manual write function for errors. But in
between the push and the write the main thread could have pushed some
other message. Thus we need to hold the lock for the duration of the
function. TOCTTOU.

Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>

1 files changed, 31 insertions, 21 deletions

diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index eed5fdc65..0ce2466f5 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -545,19 +545,26 @@ void bgp_open_send(struct peer *peer)
 	bgp_writes_on(peer);
 }
 
-/* This is only for sending NOTIFICATION message to neighbor. */
+/*
+ * Writes NOTIFICATION message directly to a peer socket without waiting for
+ * the I/O thread.
+ *
+ * There must be exactly one stream on the peer->obuf FIFO, and the data within
+ * this stream must match the format of a BGP NOTIFICATION message.
+ * Transmission is best-effort.
+ *
+ * @requires peer->io_mtx
+ * @param peer
+ * @return 0
+ */
 static int bgp_write_notify(struct peer *peer)
 {
 	int ret, val;
 	u_char type;
 	struct stream *s;
 
-	pthread_mutex_lock(&peer->io_mtx);
-	{
-		/* There should be at least one packet. */
-		s = stream_fifo_pop(peer->obuf);
-	}
-	pthread_mutex_unlock(&peer->io_mtx);
+	/* There should be at least one packet. */
+	s = stream_fifo_pop(peer->obuf);
 
 	if (!s)
 		return 0;
@@ -622,6 +629,14 @@ static int bgp_write_notify(struct peer *peer)
  * This function attempts to write the packet from the thread it is called
  * from, to ensure the packet gets out ASAP.
  *
+ * This function may be called from multiple threads. Since the function
+ * modifies I/O buffer(s) in the peer, these are locked for the duration of the
+ * call to prevent tampering from other threads.
+ *
+ * Delivery of the NOTIFICATION is attempted once and is best-effort. After
+ * return, the peer structure *must* be reset; no assumptions about session
+ * state are valid.
+ *
  * @param peer
  * @param code      BGP error code
  * @param sub_code  BGP error subcode
@@ -634,6 +649,10 @@ void bgp_notify_send_with_data(struct peer *peer, u_char code, u_char sub_code,
 	struct stream *s;
 	int length;
 
+	/* Lock I/O mutex to prevent other threads from pushing packets */
+	pthread_mutex_lock(&peer->io_mtx);
+	/* ============================================== */
+
 	/* Allocate new stream. */
 	s = stream_new(BGP_MAX_PACKET_SIZE);
 
@@ -651,20 +670,8 @@ void bgp_notify_send_with_data(struct peer *peer, u_char code, u_char sub_code,
 	/* Set BGP packet length. */
 	length = bgp_packet_set_size(s);
 
-	/*
-	 * Turn off keepalive generation for peer. This is necessary because
-	 * otherwise between the time we wipe the output buffer and the time we
-	 * push the NOTIFY onto it, the KA generation thread could have pushed
-	 * a KEEPALIVE in the middle.
-	 */
-	bgp_keepalives_off(peer);
-
 	/* wipe output buffer */
-	pthread_mutex_lock(&peer->io_mtx);
-	{
-		stream_fifo_clean(peer->obuf);
-	}
-	pthread_mutex_unlock(&peer->io_mtx);
+	stream_fifo_clean(peer->obuf);
 
 	/*
 	 * If possible, store last packet for debugging purposes. This check is
@@ -728,9 +735,12 @@ void bgp_notify_send_with_data(struct peer *peer, u_char code, u_char sub_code,
 		peer->last_reset = PEER_DOWN_NOTIFY_SEND;
 
 	/* Add packet to peer's output queue */
-	bgp_packet_add(peer, s);
+	stream_fifo_push(peer->obuf, s);
 
 	bgp_write_notify(peer);
+
+	/* ============================================== */
+	pthread_mutex_unlock(&peer->io_mtx);
 }
 
 /*