bgpd: rework BGP_MAX_PACKET_SIZE & friends

BGP_MAX_PACKET_SIZE no longer represented the absolute maximum BGP
packet size as it did before, instead it was defined as 4096 bytes,
which is the maximum unless extended message capability is negotiated,
in which case the maximum goes to 65k.

That introduced at least one bug - last_reset_cause was undersized for
extended messages, and when sending an extended message > 4096 bytes
back to a peer as part of NOTIFY data would trigger a bounds check
assert.

This patch redefines the macro to restore its previous meaning,
introduces a new macro - BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE - to
represent the 4096 byte size, and renames the extended size to
BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE for consistency. Code locations
that definitely should use the small size have been updated, locations
that semantically always need whatever the max is, no matter what that
is, use BGP_MAX_PACKET_SIZE.

BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE should only be used as a constant
when storing what the negotiated max size is for use at runtime and to
define BGP_MAX_PACKET_SIZE. Unless there is a future standard that
introduces a third valid size it should not be used for any other
purpose.

Signed-off-by: Quentin Young <qlyoung@nvidia.com>

diff --git a/bgpd/bgp_dump.c b/bgpd/bgp_dump.c
index 944a5848ec9335c63f0a35ef06a1177b9d88bd19..299ee305beafe17e429d860078f55349f93702d4 100644 (file)
--- a/bgpd/bgp_dump.c
+++ b/bgpd/bgp_dump.c
@@ -389,7 +389,8 @@ bgp_dump_route_node_record(int afi, struct bgp_dest *dest,
                bgp_dump_routes_attr(obuf, path->attr, p);
 
                cur_endp = stream_get_endp(obuf);
-               if (cur_endp > BGP_MAX_PACKET_SIZE + BGP_DUMP_MSG_HEADER
+               if (cur_endp > BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE
+                                      + BGP_DUMP_MSG_HEADER
                                       + BGP_DUMP_HEADER_SIZE) {
                        stream_set_endp(obuf, endp);
                        break;
@@ -868,8 +869,8 @@ void bgp_dump_init(void)
        memset(&bgp_dump_routes, 0, sizeof(struct bgp_dump));
 
        bgp_dump_obuf =
-               stream_new((BGP_MAX_PACKET_SIZE << 1) + BGP_DUMP_MSG_HEADER
-                          + BGP_DUMP_HEADER_SIZE);
+               stream_new((BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE * 2)
+                          + BGP_DUMP_MSG_HEADER + BGP_DUMP_HEADER_SIZE);
 
        install_node(&bgp_dump_node);
 

diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c
index 76426402184925a2e0f36e8c773f31bbe547ea95..94d905127dec84870c67710f816ebac00f41c0f8 100644 (file)
--- a/bgpd/bgp_open.c
+++ b/bgpd/bgp_open.c
@@ -1122,7 +1122,7 @@ int bgp_open_option_parse(struct peer *peer, uint8_t length, int *mp_capability)
 {
        int ret = 0;
        uint8_t *error;
-       uint8_t error_data[BGP_MAX_PACKET_SIZE];
+       uint8_t error_data[BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE];
        struct stream *s = BGP_INPUT(peer);
        size_t end = stream_get_getp(s) + length;
 
@@ -1217,8 +1217,8 @@ int bgp_open_option_parse(struct peer *peer, uint8_t length, int *mp_capability)
        /* Extended Message Support */
        peer->max_packet_size =
                CHECK_FLAG(peer->cap, PEER_CAP_EXTENDED_MESSAGE_RCV)
-                       ? BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE
-                       : BGP_MAX_PACKET_SIZE;
+                       ? BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE
+                       : BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE;
 
        /* Check there are no common AFI/SAFIs and send Unsupported Capability
           error. */

diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index d79621a74908c9dab3f94048d15f003f3d19af79..62982881d8011a5876c27e8a7ad17466ee96081f 100644 (file)
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -545,7 +545,7 @@ void bgp_keepalive_send(struct peer *peer)
 {
        struct stream *s;
 
-       s = stream_new(BGP_MAX_PACKET_SIZE);
+       s = stream_new(BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE);
 
        /* Make keepalive packet. */
        bgp_packet_set_marker(s, BGP_MSG_KEEPALIVE);
@@ -586,7 +586,7 @@ void bgp_open_send(struct peer *peer)
        else
                local_as = peer->local_as;
 
-       s = stream_new(BGP_MAX_PACKET_SIZE);
+       s = stream_new(BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE);
 
        /* Make open packet. */
        bgp_packet_set_marker(s, BGP_MSG_OPEN);
@@ -752,7 +752,7 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code,
         */
        if (peer->curr) {
                size_t packetsize = stream_get_endp(peer->curr);
-               assert(packetsize <= sizeof(peer->last_reset_cause));
+               assert(packetsize <= peer->max_packet_size);
                memcpy(peer->last_reset_cause, peer->curr->data, packetsize);
                peer->last_reset_cause_size = packetsize;
        }

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 73c1cbbbd9e096bbca63b0021a422618285967f2..21abfeb001a1abe51ac0979c120c6d8ce200d9cc 100644 (file)
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -1349,7 +1349,7 @@ struct peer *peer_new(struct bgp *bgp)
        peer->bgp = bgp_lock(bgp);
        peer = peer_lock(peer); /* initial reference */
        peer->password = NULL;
-       peer->max_packet_size = BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE;
+       peer->max_packet_size = BGP_MAX_PACKET_SIZE;
 
        /* Set default flags. */
        FOREACH_AFI_SAFI (afi, safi) {
@@ -1384,7 +1384,7 @@ struct peer *peer_new(struct bgp *bgp)
 
        /* We use a larger buffer for peer->obuf_work in the event that:
         * - We RX a BGP_UPDATE where the attributes alone are just
-        *   under BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE.
+        *   under BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE.
         * - The user configures an outbound route-map that does many as-path
         *   prepends or adds many communities. At most they can have
         *   CMD_ARGC_MAX args in a route-map so there is a finite limit on how
@@ -1394,12 +1394,12 @@ struct peer *peer_new(struct bgp *bgp)
         * bounds checking for every single attribute as we construct an
         * UPDATE.
         */
-       peer->obuf_work = stream_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE
-                                    + BGP_MAX_PACKET_SIZE_OVERFLOW);
-       peer->ibuf_work = ringbuf_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE
-                                     * BGP_READ_PACKET_MAX);
+       peer->obuf_work =
+               stream_new(BGP_MAX_PACKET_SIZE + BGP_MAX_PACKET_SIZE_OVERFLOW);
+       peer->ibuf_work =
+               ringbuf_new(BGP_MAX_PACKET_SIZE * BGP_READ_PACKET_MAX);
 
-       peer->scratch = stream_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE);
+       peer->scratch = stream_new(BGP_MAX_PACKET_SIZE);
 
        bgp_sync_init(peer);
 

diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 3343b332f4e5134d57317a77a666ebbc0af6f1da..38c6a70b8b780aa347184513a9acfd98569f65c9 100644 (file)
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -866,8 +866,9 @@ typedef enum {
 /* BGP message header and packet size.  */
 #define BGP_MARKER_SIZE                                16
 #define BGP_HEADER_SIZE                                19
-#define BGP_MAX_PACKET_SIZE                   4096
-#define BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE 65535
+#define BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE 4096
+#define BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE 65535
+#define BGP_MAX_PACKET_SIZE BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE
 #define BGP_MAX_PACKET_SIZE_OVERFLOW          1024
 
 /*
@@ -1049,7 +1050,7 @@ struct peer {
        struct stream_fifo *obuf; // packets waiting to be written
 
        /* used as a block to deposit raw wire data to */
-       uint8_t ibuf_scratch[BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE
+       uint8_t ibuf_scratch[BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE
                             * BGP_READ_PACKET_MAX];
        struct ringbuf *ibuf_work; // WiP buffer used by bgp_read() only
        struct stream *obuf_work;  // WiP buffer used to construct packets

diff --git a/tests/bgpd/test_aspath.c b/tests/bgpd/test_aspath.c
index 1a9183c472af50af3d93ffca063a813fa685e0fb..aaf3fd2aa4d4eef650153773edd0a170803869ec 100644 (file)
--- a/tests/bgpd/test_aspath.c
+++ b/tests/bgpd/test_aspath.c
@@ -892,7 +892,7 @@ static int validate(struct aspath *as, const struct test_spec *sp)
 
        /* Excercise AS4 parsing a bit, with a dogfood test */
        if (!s)
-               s = stream_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE);
+               s = stream_new(BGP_MAX_PACKET_SIZE);
        bytes4 = aspath_put(s, as, 1);
        as4 = make_aspath(STREAM_DATA(s), bytes4, 1);
 
@@ -1201,13 +1201,13 @@ static int handle_attr_test(struct aspath_tests *t)
 
        asp = make_aspath(t->segment->asdata, t->segment->len, 0);
 
-       peer.curr = stream_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE);
+       peer.curr = stream_new(BGP_MAX_PACKET_SIZE);
        peer.obuf = stream_fifo_new();
        peer.bgp = &bgp;
        peer.host = (char *)"none";
        peer.fd = -1;
        peer.cap = t->cap;
-       peer.max_packet_size = BGP_MAX_PACKET_SIZE;
+       peer.max_packet_size = BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE;
 
        stream_write(peer.curr, t->attrheader, t->len);
        datalen = aspath_put(peer.curr, asp, t->as4 == AS4_DATA);

diff --git a/tests/bgpd/test_capability.c b/tests/bgpd/test_capability.c
index 91c0cce80c5e2f2ffc46a060363bcf6c1227b24f..153b83897d988b3ed11a76656e43b356a1e8959c 100644 (file)
--- a/tests/bgpd/test_capability.c
+++ b/tests/bgpd/test_capability.c
@@ -935,7 +935,7 @@ int main(void)
                        peer->afc_adv[i][j] = 1;
                }
 
-       peer->curr = stream_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE);
+       peer->curr = stream_new(BGP_MAX_PACKET_SIZE);
 
        i = 0;
        while (mp_segments[i].name)

diff --git a/tests/bgpd/test_mp_attr.c b/tests/bgpd/test_mp_attr.c
index 8de0604c45185fb84c6765225e9590bf286e547a..f51076091357f83cd4083a658d6a14959ff0eab3 100644 (file)
--- a/tests/bgpd/test_mp_attr.c
+++ b/tests/bgpd/test_mp_attr.c
@@ -1100,7 +1100,7 @@ int main(void)
        peer = peer_create_accept(bgp);
        peer->host = (char *)"foo";
        peer->status = Established;
-       peer->curr = stream_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE);
+       peer->curr = stream_new(BGP_MAX_PACKET_SIZE);
 
        ifp.ifindex = 0;
        peer->nexthop.ifp = &ifp;