Merge pull request #14436 from opensourcerouting/fix/set_mss_for_passive_nodes

bgpd: Set TCP MSS for the socket even if the session is set to passive

9 files changed, 191 insertions, 18 deletions

diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index a84ddae01..eef3b6440 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 3e252a06f..f322de768 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)
@@ -782,6 +829,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,
diff --git a/bgpd/bgp_network.h b/bgpd/bgp_network.h
index f26d64f1f..7a0b3cc67 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/bgp_vty.c b/bgpd/bgp_vty.c
index 5574afb0a..bbe74ef5a 100644
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -13715,13 +13715,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))
@@ -13801,11 +13798,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))
diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 895f36f99..585863954 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);
 }
 
 /*
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 000000000..e69de29bb
--- /dev/null
+++ b/tests/topotests/bgp_tcp_mss_passive/__init__.py
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 000000000..a0fcd52a1
--- /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 000000000..7213975cc
--- /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 000000000..cd405f7b2
--- /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 <donatas@opensourcerouting.org>
+#
+
+"""
+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))