bgpd: Make peer->max_packet_size atomic

This value is being set and read at the same time according
to the thread sanitizer
WARNING: ThreadSanitizer: data race (pid=2914253)
  Read of size 2 at 0x7ba800011b10 by thread T2:
    #0 validate_header bgpd/bgp_io.c:601 (bgpd+0x60c5e0)
    #1 read_ibuf_work bgpd/bgp_io.c:177 (bgpd+0x608ffe)
    #2 bgp_process_reads bgpd/bgp_io.c:261 (bgpd+0x609880)
    #3 event_call lib/event.c:2011 (libfrr.so.0+0x59168d)
    #4 fpt_run lib/frr_pthread.c:369 (libfrr.so.0+0x35154e)
    #5 frr_pthread_inner lib/frr_pthread.c:178 (libfrr.so.0+0x34fef6)

  Previous write of size 2 at 0x7ba800011b10 by main thread:
    #0 bgp_open_option_parse bgpd/bgp_open.c:1469 (bgpd+0xb5006f)
    #1 bgp_open_receive bgpd/bgp_packet.c:2100 (bgpd+0x6b3f5c)
    #2 bgp_process_packet bgpd/bgp_packet.c:4019 (bgpd+0x6c9549)
    #3 event_call lib/event.c:2011 (libfrr.so.0+0x59168d)
    #4 frr_run lib/libfrr.c:1217 (libfrr.so.0+0x3b04a9)
    #5 main bgpd/bgp_main.c:548 (bgpd+0x49aa3d)

  Location is heap block of size 24328 at 0x7ba80000c000 allocated by main thread:
    #0 calloc ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:667 (libtsan.so.2+0x3fdd2)
    #1 qcalloc lib/memory.c:105 (libfrr.so.0+0x3f2784)
    #2 peer_new bgpd/bgpd.c:1517 (bgpd+0x955024)
    #3 peer_create bgpd/bgpd.c:1941 (bgpd+0x95c908)
    #4 peer_remote_as bgpd/bgpd.c:2211 (bgpd+0x9614a6)
    #5 peer_remote_as_vty bgpd/bgp_vty.c:4788 (bgpd+0x881239)
    #6 neighbor_remote_as bgpd/bgp_vty.c:4869 (bgpd+0x881a28)
    #7 cmd_execute_command_real lib/command.c:1002 (libfrr.so.0+0x2b53a2)
    #8 cmd_execute_command_strict lib/command.c:1111 (libfrr.so.0+0x2b5e0b)
    #9 command_config_read_one_line lib/command.c:1271 (libfrr.so.0+0x2b6972)
    #10 config_from_file lib/command.c:1324 (libfrr.so.0+0x2b7035)
    #11 vty_read_file lib/vty.c:2607 (libfrr.so.0+0x5c0d19)
    #12 vty_read_config lib/vty.c:2853 (libfrr.so.0+0x5c1f37)
    #13 frr_config_read_in lib/libfrr.c:981 (libfrr.so.0+0x3ae76a)
    #14 event_call lib/event.c:2011 (libfrr.so.0+0x59168d)
    #15 frr_run lib/libfrr.c:1217 (libfrr.so.0+0x3b04a9)
    #16 main bgpd/bgp_main.c:548 (bgpd+0x49aa3d)

  Thread T2 'bgpd_io' (tid=2914257, running) created by main thread at:
    #0 pthread_create ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:1001 (libtsan.so.2+0x63a59)
    #1 frr_pthread_run lib/frr_pthread.c:197 (libfrr.so.0+0x3500da)
    #2 bgp_pthreads_run bgpd/bgpd.c:8490 (bgpd+0x9d7716)
    #3 main bgpd/bgp_main.c:547 (bgpd+0x49a9c8)

Fix this.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>

bgpd/bgp_attr.c

@@ -3627,7 +3627,10 @@ enum bgp_attr_parse_ret bgp_attr_parse(struct peer *peer, struct attr *attr,
 				 * a stack buffer, since they perform bounds checking
 				 * and we are working with untrusted data.
 				 */
-				unsigned char ndata[peer->max_packet_size];
+				uint32_t max_packet_size =
+					atomic_load_explicit(&peer->max_packet_size,
+							     memory_order_relaxed);
+				unsigned char ndata[max_packet_size];
 
 				memset(ndata, 0x00, sizeof(ndata));
 				size_t lfl =

bgpd/bgp_fsm.c

@@ -211,7 +211,10 @@ static struct peer *peer_xfer_conn(struct peer *from_peer)
 	from_peer->last_major_event = last_maj_evt;
 	peer->remote_id = from_peer->remote_id;
 	peer->last_reset = from_peer->last_reset;
-	peer->max_packet_size = from_peer->max_packet_size;
+	atomic_store_explicit(&peer->max_packet_size,
+			      atomic_load_explicit(&from_peer->max_packet_size,
+						   memory_order_relaxed),
+			      memory_order_relaxed);
 
 	BGP_GR_ROUTER_DETECT_AND_SEND_CAPABILITY_TO_ZEBRA(peer->bgp,
 							  peer->bgp->peer);

bgpd/bgp_io.c

@@ -183,7 +183,8 @@ static int read_ibuf_work(struct peer_connection *connection)
 	pktsize = ntohs(pktsize);
 
 	/* if this fails we are seriously screwed */
-	assert(pktsize <= connection->peer->max_packet_size);
+	assert(pktsize <= atomic_load_explicit(&connection->peer->max_packet_size,
+					       memory_order_relaxed));
 
 	/*
 	 * If we have that much data, chuck it into its own
@@ -561,6 +562,7 @@ static bool validate_header(struct peer_connection *connection)
 	uint16_t size;
 	uint8_t type;
 	struct ringbuf *pkt = connection->ibuf_work;
+	uint32_t max_packet_size;
 
 	static const uint8_t m_correct[BGP_MARKER_SIZE] = {
 		0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
@@ -598,7 +600,9 @@ static bool validate_header(struct peer_connection *connection)
 	}
 
 	/* Minimum packet length check. */
-	if ((size < BGP_HEADER_SIZE) || (size > peer->max_packet_size)
+	max_packet_size = atomic_load_explicit(&peer->max_packet_size,
+					       memory_order_relaxed);
+	if ((size < BGP_HEADER_SIZE) || (size > max_packet_size)
 	    || (type == BGP_MSG_OPEN && size < BGP_MSG_OPEN_MIN_SIZE)
 	    || (type == BGP_MSG_UPDATE && size < BGP_MSG_UPDATE_MIN_SIZE)
 	    || (type == BGP_MSG_NOTIFY && size < BGP_MSG_NOTIFY_MIN_SIZE)

bgpd/bgp_open.c

@@ -1461,11 +1461,11 @@ int bgp_open_option_parse(struct peer *peer, uint16_t length,
 	}
 
 	/* Extended Message Support */
-	peer->max_packet_size =
-		(CHECK_FLAG(peer->cap, PEER_CAP_EXTENDED_MESSAGE_RCV)
-		 && CHECK_FLAG(peer->cap, PEER_CAP_EXTENDED_MESSAGE_ADV))
-			? BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE
-			: BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE;
+	atomic_store_explicit(&peer->max_packet_size,
+			      (CHECK_FLAG(peer->cap, PEER_CAP_EXTENDED_MESSAGE_RCV)
+			       && CHECK_FLAG(peer->cap, PEER_CAP_EXTENDED_MESSAGE_ADV))
+			      ? BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE
+			      : BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE, memory_order_relaxed);
 
 	/* Check that roles are corresponding to each other */
 	if (bgp_role_violation(peer))

bgpd/bgp_packet.c

@@ -175,7 +175,7 @@ static struct stream *bgp_update_packet_eor(struct peer *peer, afi_t afi,
 		zlog_debug("send End-of-RIB for %s to %s",
 			   get_afi_safi_str(afi, safi, false), peer->host);
 
-	s = stream_new(peer->max_packet_size);
+	s = stream_new(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed));
 
 	/* Make BGP update packet. */
 	bgp_packet_set_marker(s, BGP_MSG_UPDATE);
@@ -922,7 +922,7 @@ static void bgp_notify_send_internal(struct peer_connection *connection,
 	/* ============================================== */
 
 	/* Allocate new stream. */
-	s = stream_new(peer->max_packet_size);
+	s = stream_new(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed));
 
 	/* Make notify packet. */
 	bgp_packet_set_marker(s, BGP_MSG_NOTIFY);
@@ -963,7 +963,7 @@ static void bgp_notify_send_internal(struct peer_connection *connection,
 	 */
 	if (use_curr && peer->curr) {
 		size_t packetsize = stream_get_endp(peer->curr);
-		assert(packetsize <= peer->max_packet_size);
+		assert(packetsize <= atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed));
 		if (peer->last_reset_cause)
 			stream_free(peer->last_reset_cause);
 		peer->last_reset_cause = stream_dup(peer->curr);
@@ -1113,7 +1113,7 @@ void bgp_route_refresh_send(struct peer *peer, afi_t afi, safi_t safi,
 	/* Convert AFI, SAFI to values for packet. */
 	bgp_map_afi_safi_int2iana(afi, safi, &pkt_afi, &pkt_safi);
 
-	s = stream_new(peer->max_packet_size);
+	s = stream_new(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed));
 
 	/* Make BGP update packet. */
 	if (CHECK_FLAG(peer->cap, PEER_CAP_REFRESH_RCV))
@@ -1233,7 +1233,7 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi,
 	/* Convert AFI, SAFI to values for packet. */
 	bgp_map_afi_safi_int2iana(afi, safi, &pkt_afi, &pkt_safi);
 
-	s = stream_new(peer->max_packet_size);
+	s = stream_new(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed));
 
 	/* Make BGP update packet. */
 	bgp_packet_set_marker(s, BGP_MSG_CAPABILITY);

bgpd/bgp_updgrp.c

@@ -69,6 +69,8 @@ static void sync_init(struct update_subgroup *subgrp,
 		      struct update_group *updgrp)
 {
 	struct peer *peer = UPDGRP_PEER(updgrp);
+	uint16_t max_packet_size = atomic_load_explicit(&peer->max_packet_size,
+						       memory_order_relaxed);
 
 	subgrp->sync =
 		XCALLOC(MTYPE_BGP_SYNCHRONISE, sizeof(struct bgp_synchronize));
@@ -93,9 +95,9 @@ static void sync_init(struct update_subgroup *subgrp,
 	 * bounds
 	 * checking for every single attribute as we construct an UPDATE.
 	 */
-	subgrp->work = stream_new(peer->max_packet_size
+	subgrp->work = stream_new(max_packet_size
 				  + BGP_MAX_PACKET_SIZE_OVERFLOW);
-	subgrp->scratch = stream_new(peer->max_packet_size);
+	subgrp->scratch = stream_new(max_packet_size);
 }
 
 static void sync_delete(struct update_subgroup *subgrp)
@@ -134,7 +136,9 @@ static void conf_copy(struct peer *dst, struct peer *src, afi_t afi,
 	dst->flags = src->flags;
 	dst->af_flags[afi][safi] = src->af_flags[afi][safi];
 	dst->pmax_out[afi][safi] = src->pmax_out[afi][safi];
-	dst->max_packet_size = src->max_packet_size;
+	atomic_store_explicit(&dst->max_packet_size,
+			    atomic_load_explicit(&src->max_packet_size, memory_order_relaxed),
+			    memory_order_relaxed);
 	XFREE(MTYPE_BGP_PEER_HOST, dst->host);
 
 	dst->host = XSTRDUP(MTYPE_BGP_PEER_HOST, src->host);
@@ -361,7 +365,8 @@ static unsigned int updgrp_hash_key_make(const void *p)
 			  key);
 	key = jhash_1word(peer->v_routeadv, key);
 	key = jhash_1word(peer->change_local_as, key);
-	key = jhash_1word(peer->max_packet_size, key);
+	key = jhash_1word(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed),
+			  key);
 	key = jhash_1word(peer->pmax_out[afi][safi], key);
 
 
@@ -478,7 +483,7 @@ static unsigned int updgrp_hash_key_make(const void *p)
 			   peer->addpath_paths_limit[afi][safi].receive);
 		zlog_debug(
 			"%pBP Update Group Hash: max packet size: %u pmax_out: %u Peer Group: %s rmap out: %s",
-			peer, peer->max_packet_size, peer->pmax_out[afi][safi],
+			peer, atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed), peer->pmax_out[afi][safi],
 			peer->group ? peer->group->name : "(NONE)",
 			ROUTE_MAP_OUT_NAME(filter) ? ROUTE_MAP_OUT_NAME(filter)
 						   : "(NONE)");
@@ -929,7 +934,8 @@ static int update_group_show_walkcb(struct update_group *updgrp, void *arg)
 					: "");
 			if (peer)
 				vty_out(vty, "    Max packet size: %d\n",
-					peer->max_packet_size);
+					atomic_load_explicit(&peer->max_packet_size,
+							     memory_order_relaxed));
 		}
 		if (subgrp->peer_count > 0) {
 			if (ctx->uj) {

bgpd/bgp_updgrp_packet.c

@@ -902,7 +902,7 @@ struct bpacket *subgroup_update_packet(struct update_subgroup *subgrp)
 				subgrp->update_group->id, subgrp->id,
 				(stream_get_endp(packet)
 				 - stream_get_getp(packet)),
-				peer->max_packet_size, num_pfx);
+				atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed), num_pfx);
 		pkt = bpacket_queue_add(SUBGRP_PKTQ(subgrp), packet, &vecarr);
 		stream_reset(s);
 		stream_reset(snlri);
@@ -1139,7 +1139,7 @@ void subgroup_default_update_packet(struct update_subgroup *subgrp,
 			   tx_id_buf, attrstr);
 	}
 
-	s = stream_new(peer->max_packet_size);
+	s = stream_new(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed));
 
 	/* Make BGP update packet. */
 	bgp_packet_set_marker(s, BGP_MSG_UPDATE);
@@ -1226,7 +1226,7 @@ void subgroup_default_withdraw_packet(struct update_subgroup *subgrp)
 			   tx_id_buf);
 	}
 
-	s = stream_new(peer->max_packet_size);
+	s = stream_new(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed));
 
 	/* Make BGP update packet. */
 	bgp_packet_set_marker(s, BGP_MSG_UPDATE);

bgpd/bgpd.c

@@ -1551,7 +1551,8 @@ struct peer *peer_new(struct bgp *bgp)
 	peer->local_role = ROLE_UNDEFINED;
 	peer->remote_role = ROLE_UNDEFINED;
 	peer->password = NULL;
-	peer->max_packet_size = BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE;
+	atomic_store_explicit(&peer->max_packet_size, BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE,
+			    memory_order_relaxed);
 
 	/* Set default flags. */
 	FOREACH_AFI_SAFI (afi, safi) {
@@ -1646,7 +1647,7 @@ void peer_xfer_config(struct peer *peer_dst, struct peer *peer_src)
 	peer_dst->rmap_type = peer_src->rmap_type;
 	peer_dst->local_role = peer_src->local_role;
 
-	peer_dst->max_packet_size = peer_src->max_packet_size;
+	atomic_store_explicit(&peer_dst->max_packet_size, atomic_load_explicit(&peer_src->max_packet_size, memory_order_relaxed), memory_order_relaxed);
 
 	/* Timers */
 	peer_dst->holdtime = peer_src->holdtime;

bgpd/bgpd.h

@@ -1868,7 +1868,7 @@ struct peer {
 	char *domainname;
 
 	/* Extended Message Support */
-	uint16_t max_packet_size;
+	_Atomic uint16_t max_packet_size;
 
 	/* Conditional advertisement */
 	bool advmap_config_change[AFI_MAX][SAFI_MAX];