From 3fe2ab07d8d98bea9467a3fe5c29a1199d561119 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 23 May 2024 12:15:46 -0400 Subject: [PATCH] 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 --- bgpd/bgp_attr.c | 5 ++++- bgpd/bgp_fsm.c | 5 ++++- bgpd/bgp_io.c | 8 ++++++-- bgpd/bgp_open.c | 10 +++++----- bgpd/bgp_packet.c | 10 +++++----- bgpd/bgp_updgrp.c | 18 ++++++++++++------ bgpd/bgp_updgrp_packet.c | 6 +++--- bgpd/bgpd.c | 5 +++-- bgpd/bgpd.h | 2 +- 9 files changed, 43 insertions(+), 26 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 71f02a7f83a2..7e9736d906ca 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -3618,7 +3618,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 = diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index 15cc5dbe2ea0..075a6d102a08 100644 --- a/bgpd/bgp_fsm.c +++ b/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); diff --git a/bgpd/bgp_io.c b/bgpd/bgp_io.c index b07e69ac31bb..a6be738e24b6 100644 --- a/bgpd/bgp_io.c +++ b/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) diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c index e163df5954ee..0e959f033c02 100644 --- a/bgpd/bgp_open.c +++ b/bgpd/bgp_open.c @@ -1466,11 +1466,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)) diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 9c194eac1c2a..de5d43ba7390 100644 --- a/bgpd/bgp_packet.c +++ b/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); @@ -1112,7 +1112,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)) @@ -1232,7 +1232,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); diff --git a/bgpd/bgp_updgrp.c b/bgpd/bgp_updgrp.c index 124e7a388b82..ab4e0c0cc5b4 100644 --- a/bgpd/bgp_updgrp.c +++ b/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); @@ -356,7 +360,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); @@ -473,7 +478,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)"); @@ -924,7 +929,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) { diff --git a/bgpd/bgp_updgrp_packet.c b/bgpd/bgp_updgrp_packet.c index 1f691b6a9e19..4a29df78dc9f 100644 --- a/bgpd/bgp_updgrp_packet.c +++ b/bgpd/bgp_updgrp_packet.c @@ -899,7 +899,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); @@ -1136,7 +1136,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); @@ -1223,7 +1223,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); diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 09e64cf9ec31..baa6f7dd7283 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1528,7 +1528,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) { @@ -1623,7 +1624,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; diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 7da07605ea56..a344333554e9 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -1845,7 +1845,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];