From 232470f3b7361b63d99f95796814ae81e1db34ab Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 15 Sep 2023 14:05:41 +0300 Subject: [PATCH 1/4] bgpd: Set TCP MSS for the socket even if the session is set to passive Signed-off-by: Donatas Abraitis --- bgpd/bgp_fsm.c | 6 ------ bgpd/bgp_network.c | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index a84ddae0184b..eef3b644080a 100644 --- a/bgpd/bgp_fsm.c +++ b/bgpd/bgp_fsm.c @@ -1832,12 +1832,6 @@ static enum bgp_fsm_state_progress bgp_start(struct peer_connection *connection) /* Clear peer capability flag. */ peer->cap = 0; - /* If the peer is passive mode, force to move to Active mode. */ - if (CHECK_FLAG(peer->flags, PEER_FLAG_PASSIVE)) { - BGP_EVENT_ADD(connection, TCP_connection_open_failed); - return BGP_FSM_SUCCESS; - } - if (peer->bgp->vrf_id == VRF_UNKNOWN) { if (bgp_debug_neighbor_events(peer)) flog_err( diff --git a/bgpd/bgp_network.c b/bgpd/bgp_network.c index 3e252a06f594..3bfdeb17710a 100644 --- a/bgpd/bgp_network.c +++ b/bgpd/bgp_network.c @@ -782,6 +782,12 @@ int bgp_connect(struct peer_connection *connection) return connect_error; } + /* If the peer is passive mode, force to move to Active mode. */ + if (CHECK_FLAG(peer->flags, PEER_FLAG_PASSIVE)) { + BGP_EVENT_ADD(connection, TCP_connection_open_failed); + return BGP_FSM_SUCCESS; + } + if (peer->conf_if || peer->ifname) ifindex = ifname2ifindex(peer->conf_if ? peer->conf_if : peer->ifname, From 84e14c14dc769f9544fe0f09473183f2fb787b4d Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Mon, 18 Sep 2023 15:54:43 +0300 Subject: [PATCH 2/4] bgpd: Show TCP MSS per neighbor always, despite if it's configured or not To show the TCP MSS value per neighbor you have to configure it, otherwise you don't see the actual value. Signed-off-by: Donatas Abraitis --- bgpd/bgp_vty.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index e76968cdb020..9949ce818724 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -13693,13 +13693,10 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, bool use_json, } /* Configured and Synced tcp-mss value for peer */ - if (CHECK_FLAG(p->flags, PEER_FLAG_TCP_MSS)) { - sync_tcp_mss = sockopt_tcp_mss_get(p->connection->fd); - json_object_int_add(json_neigh, "bgpTcpMssConfigured", - p->tcp_mss); - json_object_int_add(json_neigh, "bgpTcpMssSynced", - sync_tcp_mss); - } + sync_tcp_mss = sockopt_tcp_mss_get(p->connection->fd); + json_object_int_add(json_neigh, "bgpTcpMssConfigured", + p->tcp_mss); + json_object_int_add(json_neigh, "bgpTcpMssSynced", sync_tcp_mss); /* Extended Optional Parameters Length for BGP OPEN Message */ if (BGP_OPEN_EXT_OPT_PARAMS_CAPABLE(p)) @@ -13779,11 +13776,9 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, bool use_json, p->delayopen); /* Configured and synced tcp-mss value for peer */ - if (CHECK_FLAG(p->flags, PEER_FLAG_TCP_MSS)) { - sync_tcp_mss = sockopt_tcp_mss_get(p->connection->fd); - vty_out(vty, " Configured tcp-mss is %d", p->tcp_mss); - vty_out(vty, ", synced tcp-mss is %d\n", sync_tcp_mss); - } + sync_tcp_mss = sockopt_tcp_mss_get(p->connection->fd); + vty_out(vty, " Configured tcp-mss is %d", p->tcp_mss); + vty_out(vty, ", synced tcp-mss is %d\n", sync_tcp_mss); /* Extended Optional Parameters Length for BGP OPEN Message */ if (BGP_OPEN_EXT_OPT_PARAMS_CAPABLE(p)) From 81ece63e3e63fc43b496beafdd3b46c4cc52839b Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Mon, 18 Sep 2023 22:34:45 +0300 Subject: [PATCH 3/4] bgpd: Set TCP min MSS per listener Set only if at least one peer is in passive mode. Signed-off-by: Donatas Abraitis --- bgpd/bgp_network.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++ bgpd/bgp_network.h | 1 + bgpd/bgpd.c | 2 ++ 3 files changed, 50 insertions(+) diff --git a/bgpd/bgp_network.c b/bgpd/bgp_network.c index 3bfdeb17710a..f322de76833f 100644 --- a/bgpd/bgp_network.c +++ b/bgpd/bgp_network.c @@ -334,6 +334,53 @@ static int bgp_get_instance_for_inc_conn(int sock, struct bgp **bgp_inst) #endif } +int bgp_tcp_mss_set(struct peer *peer) +{ + struct listnode *node; + int ret = 0; + struct bgp_listener *listener; + uint32_t min_mss = 0; + struct peer *p; + + for (ALL_LIST_ELEMENTS_RO(peer->bgp->peer, node, p)) { + if (!CHECK_FLAG(p->flags, PEER_FLAG_TCP_MSS)) + continue; + + if (!p->tcp_mss) + continue; + + if (!min_mss) + min_mss = p->tcp_mss; + + min_mss = MIN(min_mss, p->tcp_mss); + } + + frr_with_privs(&bgpd_privs) { + for (ALL_LIST_ELEMENTS_RO(bm->listen_sockets, node, listener)) { + if (listener->su.sa.sa_family != + peer->connection->su.sa.sa_family) + continue; + + if (!listener->bgp) { + if (peer->bgp->vrf_id != VRF_DEFAULT) + continue; + } else if (listener->bgp != peer->bgp) + continue; + + /* Set TCP MSS per listener only if there is at least + * one peer that is in passive mode. Otherwise, TCP MSS + * is set per socket via bgp_connect(). + */ + if (CHECK_FLAG(peer->flags, PEER_FLAG_PASSIVE)) + sockopt_tcp_mss_set(listener->fd, min_mss); + + break; + } + } + + return ret; +} + static void bgp_socket_set_buffer_size(const int fd) { if (getsockopt_so_sendbuf(fd) < (int)bm->socket_buffer) diff --git a/bgpd/bgp_network.h b/bgpd/bgp_network.h index f26d64f1f805..7a0b3cc67d7e 100644 --- a/bgpd/bgp_network.h +++ b/bgpd/bgp_network.h @@ -30,6 +30,7 @@ extern int bgp_md5_unset_prefix(struct bgp *bgp, struct prefix *p); extern int bgp_md5_set(struct peer_connection *connection); extern int bgp_md5_unset(struct peer_connection *connection); extern int bgp_set_socket_ttl(struct peer_connection *connection); +extern int bgp_tcp_mss_set(struct peer *peer); extern int bgp_update_address(struct interface *ifp, const union sockunion *dst, union sockunion *addr); diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 895f36f992e0..585863954cc0 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -5779,6 +5779,7 @@ void peer_tcp_mss_set(struct peer *peer, uint32_t tcp_mss) { peer->tcp_mss = tcp_mss; SET_FLAG(peer->flags, PEER_FLAG_TCP_MSS); + bgp_tcp_mss_set(peer); } /* Reset the TCP-MSS value in the peer structure, @@ -5789,6 +5790,7 @@ void peer_tcp_mss_unset(struct peer *peer) { UNSET_FLAG(peer->flags, PEER_FLAG_TCP_MSS); peer->tcp_mss = 0; + bgp_tcp_mss_set(peer); } /* From 6cd8f13fac4502505c1905da0b5865cde062ab72 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Mon, 18 Sep 2023 22:54:53 +0300 Subject: [PATCH 4/4] tests: Check if TCP MSS is synced if using a passive neighbor Signed-off-by: Donatas Abraitis --- .../topotests/bgp_tcp_mss_passive/__init__.py | 0 .../topotests/bgp_tcp_mss_passive/r1/frr.conf | 12 ++ .../topotests/bgp_tcp_mss_passive/r2/frr.conf | 10 ++ .../test_bgp_tcp_mss_passive.py | 106 ++++++++++++++++++ 4 files changed, 128 insertions(+) create mode 100644 tests/topotests/bgp_tcp_mss_passive/__init__.py create mode 100644 tests/topotests/bgp_tcp_mss_passive/r1/frr.conf create mode 100644 tests/topotests/bgp_tcp_mss_passive/r2/frr.conf create mode 100644 tests/topotests/bgp_tcp_mss_passive/test_bgp_tcp_mss_passive.py diff --git a/tests/topotests/bgp_tcp_mss_passive/__init__.py b/tests/topotests/bgp_tcp_mss_passive/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/topotests/bgp_tcp_mss_passive/r1/frr.conf b/tests/topotests/bgp_tcp_mss_passive/r1/frr.conf new file mode 100644 index 000000000000..a0fcd52a1824 --- /dev/null +++ b/tests/topotests/bgp_tcp_mss_passive/r1/frr.conf @@ -0,0 +1,12 @@ +! +interface r1-eth0 + ip address 192.168.1.1/24 +! +router bgp 65001 + no bgp ebgp-requires-policy + neighbor 192.168.1.2 remote-as external + neighbor 192.168.1.2 timers 1 3 + neighbor 192.168.1.2 timers connect 1 + neighbor 192.168.1.2 passive + neighbor 192.168.1.2 tcp-mss 300 +! diff --git a/tests/topotests/bgp_tcp_mss_passive/r2/frr.conf b/tests/topotests/bgp_tcp_mss_passive/r2/frr.conf new file mode 100644 index 000000000000..7213975cc03a --- /dev/null +++ b/tests/topotests/bgp_tcp_mss_passive/r2/frr.conf @@ -0,0 +1,10 @@ +! +interface r2-eth0 + ip address 192.168.1.2/24 +! +router bgp 65002 + no bgp ebgp-requires-policy + neighbor 192.168.1.1 remote-as external + neighbor 192.168.1.1 timers 1 3 + neighbor 192.168.1.1 timers connect 1 +! diff --git a/tests/topotests/bgp_tcp_mss_passive/test_bgp_tcp_mss_passive.py b/tests/topotests/bgp_tcp_mss_passive/test_bgp_tcp_mss_passive.py new file mode 100644 index 000000000000..cd405f7b2289 --- /dev/null +++ b/tests/topotests/bgp_tcp_mss_passive/test_bgp_tcp_mss_passive.py @@ -0,0 +1,106 @@ +#!/usr/bin/env python +# SPDX-License-Identifier: ISC + +# +# Copyright (c) 2023 by +# Donatas Abraitis +# + +""" +Test if TCP MSS is synced with passive neighbor. +""" + +import os +import sys +import json +import pytest +import functools + +CWD = os.path.dirname(os.path.realpath(__file__)) +sys.path.append(os.path.join(CWD, "../")) + +# pylint: disable=C0413 +from lib import topotest +from lib.topogen import Topogen, TopoRouter, get_topogen + +pytestmark = [pytest.mark.bgpd] + + +def build_topo(tgen): + for routern in range(1, 3): + tgen.add_router("r{}".format(routern)) + + switch = tgen.add_switch("s1") + switch.add_link(tgen.gears["r1"]) + switch.add_link(tgen.gears["r2"]) + + +def setup_module(mod): + tgen = Topogen(build_topo, mod.__name__) + tgen.start_topology() + + router_list = tgen.routers() + + for i, (rname, router) in enumerate(router_list.items(), 1): + router.load_frr_config(os.path.join(CWD, "{}/frr.conf".format(rname))) + + tgen.start_router() + + +def teardown_module(mod): + tgen = get_topogen() + tgen.stop_topology() + + +def test_bgp_tcp_mss_passive(): + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + def _bgp_check_tcp_mss_configured(router, neighbor, mss): + output = json.loads(router.vtysh_cmd("show bgp neighbors json")) + expected = { + neighbor: { + "bgpTcpMssConfigured": mss, + } + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_tcp_mss_configured, tgen.gears["r1"], "192.168.1.2", 300 + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "r1 is not configured with TCP MSS 300" + + test_func = functools.partial( + _bgp_check_tcp_mss_configured, tgen.gears["r2"], "192.168.1.1", 0 + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "r2 is not configured with the default TCP MSS (1500)" + + def _bgp_check_tcp_mss_synced(router, neighbor, mss): + output = json.loads(router.vtysh_cmd("show bgp neighbors json")) + expected = { + neighbor: { + "bgpTcpMssSynced": mss, + } + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_tcp_mss_synced, tgen.gears["r1"], "192.168.1.2", 288 + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "r1 is not synced with TCP MSS 300" + + test_func = functools.partial( + _bgp_check_tcp_mss_synced, tgen.gears["r2"], "192.168.1.1", 288 + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "r2 is not synced with the default TCP MSS (1488)" + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args))