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 <mstapp@nvidia.com>

diff --git a/bgpd/bgp_io.c b/bgpd/bgp_io.c
index 7af1fae280b3e3e52294b16a4bb8ee41f4082468..f9bb8d518d960404f5765007a1aea056b4ef9db4 100644 (file)
--- 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 7daac44946cc945dd6346d55ce227147bafcb290..90695219a7ea37c518c325b914182400e64a1c54 100644 (file)
--- 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 a0eb579db7a80a0fb08f7b1f20fed0609815322a..9f6d772bc0a625354f7ce4bc6c2efdf38d841e94 100644 (file)
--- 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);