From 71ca5b09bc71e8cbe38177cf41e83fe164e52eee Mon Sep 17 00:00:00 2001 From: Mark Stapp Date: Thu, 8 Sep 2022 16:14:36 -0400 Subject: [PATCH] bgpd: avoid notify race between io and main pthreads The "bgp_notify_" apis in bgp_packet.c generate a notification to a peer, usually during error handling. The io pthread wants to send notifications in a couple of cases during early received-packet validation - but the existing api interacts with the peer struct itself, and that's not safe. Add a new api for use by the io pthread, and adjust the main notify api so that it can avoid touching the peer struct. Signed-off-by: Mark Stapp --- bgpd/bgp_io.c | 17 ++++++++--------- bgpd/bgp_packet.c | 32 ++++++++++++++++++++++++++++---- bgpd/bgp_packet.h | 2 ++ 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/bgpd/bgp_io.c b/bgpd/bgp_io.c index 7af1fae280..f9bb8d518d 100644 --- a/bgpd/bgp_io.c +++ b/bgpd/bgp_io.c @@ -37,7 +37,7 @@ #include "bgpd/bgp_debug.h" // for bgp_debug_neighbor_events, bgp_type_str #include "bgpd/bgp_errors.h" // for expanded error reference information #include "bgpd/bgp_fsm.h" // for BGP_EVENT_ADD, bgp_event -#include "bgpd/bgp_packet.h" // for bgp_notify_send_with_data, bgp_notify... +#include "bgpd/bgp_packet.h" // for bgp_notify_io_invalid... #include "bgpd/bgp_trace.h" // for frrtraces #include "bgpd/bgpd.h" // for peer, BGP_MARKER_SIZE, bgp_master, bm /* clang-format on */ @@ -526,8 +526,8 @@ static bool validate_header(struct peer *peer) return false; if (memcmp(m_correct, m_rx, BGP_MARKER_SIZE) != 0) { - bgp_notify_send(peer, BGP_NOTIFY_HEADER_ERR, - BGP_NOTIFY_HEADER_NOT_SYNC); + bgp_notify_io_invalid(peer, BGP_NOTIFY_HEADER_ERR, + BGP_NOTIFY_HEADER_NOT_SYNC, NULL, 0); return false; } @@ -547,9 +547,8 @@ static bool validate_header(struct peer *peer) zlog_debug("%s unknown message type 0x%02x", peer->host, type); - bgp_notify_send_with_data(peer, BGP_NOTIFY_HEADER_ERR, - BGP_NOTIFY_HEADER_BAD_MESTYPE, &type, - 1); + bgp_notify_io_invalid(peer, BGP_NOTIFY_HEADER_ERR, + BGP_NOTIFY_HEADER_BAD_MESTYPE, &type, 1); return false; } @@ -574,9 +573,9 @@ static bool validate_header(struct peer *peer) uint16_t nsize = htons(size); - bgp_notify_send_with_data(peer, BGP_NOTIFY_HEADER_ERR, - BGP_NOTIFY_HEADER_BAD_MESLEN, - (unsigned char *)&nsize, 2); + bgp_notify_io_invalid(peer, BGP_NOTIFY_HEADER_ERR, + BGP_NOTIFY_HEADER_BAD_MESLEN, + (unsigned char *)&nsize, 2); return false; } diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 7daac44946..90695219a7 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -871,8 +871,9 @@ bool bgp_notify_received_hard_reset(struct peer *peer, uint8_t code, * @param data Data portion * @param datalen length of data portion */ -void bgp_notify_send_with_data(struct peer *peer, uint8_t code, - uint8_t sub_code, uint8_t *data, size_t datalen) +static void bgp_notify_send_internal(struct peer *peer, uint8_t code, + uint8_t sub_code, uint8_t *data, + size_t datalen, bool use_curr) { struct stream *s; bool hard_reset = bgp_notify_send_hard_reset(peer, code, sub_code); @@ -917,8 +918,11 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code, * If possible, store last packet for debugging purposes. This check is * in place because we are sometimes called with a doppelganger peer, * who tends to have a plethora of fields nulled out. + * + * Some callers should not attempt this - the io pthread for example + * should not touch internals of the peer struct. */ - if (peer->curr) { + if (use_curr && peer->curr) { size_t packetsize = stream_get_endp(peer->curr); assert(packetsize <= peer->max_packet_size); memcpy(peer->last_reset_cause, peer->curr->data, packetsize); @@ -1001,7 +1005,27 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code, */ void bgp_notify_send(struct peer *peer, uint8_t code, uint8_t sub_code) { - bgp_notify_send_with_data(peer, code, sub_code, NULL, 0); + bgp_notify_send_internal(peer, code, sub_code, NULL, 0, true); +} + +/* + * Enqueue notification; called from the main pthread, peer object access is ok. + */ +void bgp_notify_send_with_data(struct peer *peer, uint8_t code, + uint8_t sub_code, uint8_t *data, size_t datalen) +{ + bgp_notify_send_internal(peer, code, sub_code, data, datalen, true); +} + +/* + * For use by the io pthread, queueing a notification but avoiding access to + * the peer object. + */ +void bgp_notify_io_invalid(struct peer *peer, uint8_t code, uint8_t sub_code, + uint8_t *data, size_t datalen) +{ + /* Avoid touching the peer object */ + bgp_notify_send_internal(peer, code, sub_code, data, datalen, false); } /* diff --git a/bgpd/bgp_packet.h b/bgpd/bgp_packet.h index a0eb579db7..9f6d772bc0 100644 --- a/bgpd/bgp_packet.h +++ b/bgpd/bgp_packet.h @@ -62,6 +62,8 @@ extern void bgp_open_send(struct peer *); extern void bgp_notify_send(struct peer *, uint8_t, uint8_t); extern void bgp_notify_send_with_data(struct peer *, uint8_t, uint8_t, uint8_t *, size_t); +void bgp_notify_io_invalid(struct peer *peer, uint8_t code, uint8_t sub_code, + uint8_t *data, size_t datalen); extern void bgp_route_refresh_send(struct peer *peer, afi_t afi, safi_t safi, uint8_t orf_type, uint8_t when_to_refresh, int remove, uint8_t subtype); -- 2.39.5