Skip to content

Commit

Permalink
avoid notify race between io and main pthreads (sonic-net#16288)
Browse files Browse the repository at this point in the history 
Why I did it
Cherry-Pick sonic-net#11926 into 202012

Work item tracking
Microsoft ADO (17850717):
How I did it
Create patch file to avoid notify race between io and main pthreads.

How to verify it
PR test, physical DUT for bgp related regression tests.
  • Loading branch information
@jcaiMR
jcaiMR committed Aug 29, 2023
1 parent a1ad1cf commit 5b6a935
Show file tree
Hide file tree
Showing 2 changed files with 136 additions and 1 deletion.
134 changes: 134 additions & 0 deletions src/sonic-frr/patch/0011-bgpd-avoid-notify-race-between-io-and-main-pthreads.patch
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
From 45185cffa86fac1532ba14d280d91be6cfb8cc78 Mon Sep 17 00:00:00 2001
From: jcaiMR <jcai@microsoft.com>
Date: Fri, 25 Aug 2023 16:07:55 +0000
Subject: [PATCH] bgpd: avoid notify race between io and main pthreads

---
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 4190b1851..9fd0dc548 100644
--- a/bgpd/bgp_io.c
+++ b/bgpd/bgp_io.c
@@ -38,7 +38,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/bgpd.h" // for peer, BGP_MARKER_SIZE, bgp_master, bm
/* clang-format on */

@@ -516,8 +516,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;
}

@@ -537,9 +537,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;
}

@@ -564,9 +563,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 2c8a375a5..da1cd9370 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -677,8 +677,9 @@ static void bgp_write_notify(struct peer *peer)
* @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;

@@ -710,8 +711,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 <= sizeof(peer->last_reset_cause));
memcpy(peer->last_reset_cause, peer->curr->data, packetsize);
@@ -794,7 +798,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 48de9d9cb..79210567f 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 *, afi_t, safi_t, uint8_t,
uint8_t, int);
extern void bgp_capability_send(struct peer *, afi_t, safi_t, int, int);
--
2.25.1

3 changes: 2 additions & 1 deletion src/sonic-frr/patch/series
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@
0007-frr-remove-frr-log-outchannel-to-var-log-frr.log.patch
0008-Add-support-of-bgp-l3vni-evpn.patch
0009-ignore-route-from-default-table.patch
0010-bgpd-change-log-level-of-graceful-restart-events-to-info.patch
0010-bgpd-change-log-level-of-graceful-restart-events-to-info.patch
0011-bgpd-avoid-notify-race-between-io-and-main-pthreads.patch

0 comments on commit 5b6a935

Please sign in to comment.