From dd0f005b8a97f66144de29a2da2c2256d24b03f7 Mon Sep 17 00:00:00 2001 From: pavel-shirshov Date: Sun, 23 Jun 2019 15:26:02 -0700 Subject: [PATCH] [FRR]: Port some patches from sonic-quagga repo (#3017) * Update sonic-quagga submodule * Port some patches from sonic-quagga * Fix Makefile * Another patch * Uncomment bgp test * Downport Nikos's patch * Add a patch to alleviate the vendor issue * use patch instead of stg --- platform/vs/tests/bgp/test_invalid_nexthop.py | 1 - src/sonic-frr/Makefile | 4 + ...01-Add-support-of-bgp-tcp-DSCP-value.patch | 141 ++++++++++++++++ ...verity-of-Vty-connected-from-message.patch | 25 +++ ...xthop-attribute-when-NLRI-is-present.patch | 156 ++++++++++++++++++ ...EXT_HOP-to-be-0.0.0.0-due-to-allevia.patch | 27 +++ src/sonic-frr/patch/series | 4 + 7 files changed, 357 insertions(+), 1 deletion(-) create mode 100644 src/sonic-frr/patch/0001-Add-support-of-bgp-tcp-DSCP-value.patch create mode 100644 src/sonic-frr/patch/0002-Reduce-severity-of-Vty-connected-from-message.patch create mode 100644 src/sonic-frr/patch/0003-ignore-nexthop-attribute-when-NLRI-is-present.patch create mode 100644 src/sonic-frr/patch/0004-Allow-BGP-attr-NEXT_HOP-to-be-0.0.0.0-due-to-allevia.patch create mode 100644 src/sonic-frr/patch/series diff --git a/platform/vs/tests/bgp/test_invalid_nexthop.py b/platform/vs/tests/bgp/test_invalid_nexthop.py index 9458ecdeee..021e3bfea0 100644 --- a/platform/vs/tests/bgp/test_invalid_nexthop.py +++ b/platform/vs/tests/bgp/test_invalid_nexthop.py @@ -5,7 +5,6 @@ import time import json import pytest -@pytest.mark.skip(reason="not working for frr") def test_InvalidNexthop(dvs, testlog): dvs.copy_file("/etc/frr/", "bgp/files/invalid_nexthop/bgpd.conf") diff --git a/src/sonic-frr/Makefile b/src/sonic-frr/Makefile index 5ba70addf1..0d68706a89 100644 --- a/src/sonic-frr/Makefile +++ b/src/sonic-frr/Makefile @@ -8,6 +8,10 @@ DERIVED_TARGET = $(FRR_PYTHONTOOLS) $(FRR_DBG) $(FRR_SNMP) $(FRR_SNMP_DBG) $(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% : # Build the package pushd ./frr + patch -p1 < ../patch/0001-Add-support-of-bgp-tcp-DSCP-value.patch + patch -p1 < ../patch/0002-Reduce-severity-of-Vty-connected-from-message.patch + patch -p1 < ../patch/0003-ignore-nexthop-attribute-when-NLRI-is-present.patch + patch -p1 < ../patch/0004-Allow-BGP-attr-NEXT_HOP-to-be-0.0.0.0-due-to-allevia.patch tools/tarsource.sh -V -e '-sonic' dpkg-buildpackage -rfakeroot -b -us -uc -Ppkg.frr.nortrlib -j$(SONIC_CONFIG_MAKE_JOBS) popd diff --git a/src/sonic-frr/patch/0001-Add-support-of-bgp-tcp-DSCP-value.patch b/src/sonic-frr/patch/0001-Add-support-of-bgp-tcp-DSCP-value.patch new file mode 100644 index 0000000000..e9f496d7af --- /dev/null +++ b/src/sonic-frr/patch/0001-Add-support-of-bgp-tcp-DSCP-value.patch @@ -0,0 +1,141 @@ +From ef017a613691a40f32cdaa5396bbd4889b1cb647 Mon Sep 17 00:00:00 2001 +From: Pavel Shirshov +Date: Fri, 14 Jun 2019 17:45:31 -0700 +Subject: [PATCH] Add support of bgp tcp DSCP value + +--- + bgpd/bgp_network.c | 11 ++++------- + bgpd/bgp_vty.c | 40 ++++++++++++++++++++++++++++++++++++++++ + bgpd/bgpd.c | 5 ++++- + bgpd/bgpd.h | 3 +++ + 4 files changed, 51 insertions(+), 8 deletions(-) + +diff --git a/bgpd/bgp_network.c b/bgpd/bgp_network.c +index 4153da5a6..fed133eb7 100644 +--- a/bgpd/bgp_network.c ++++ b/bgpd/bgp_network.c +@@ -569,11 +569,9 @@ int bgp_connect(struct peer *peer) + #ifdef IPTOS_PREC_INTERNETCONTROL + frr_elevate_privs(&bgpd_privs) { + if (sockunion_family(&peer->su) == AF_INET) +- setsockopt_ipv4_tos(peer->fd, +- IPTOS_PREC_INTERNETCONTROL); ++ setsockopt_ipv4_tos(peer->fd, peer->bgp->tcp_dscp); + else if (sockunion_family(&peer->su) == AF_INET6) +- setsockopt_ipv6_tclass(peer->fd, +- IPTOS_PREC_INTERNETCONTROL); ++ setsockopt_ipv6_tclass(peer->fd, peer->bgp->tcp_dscp); + } + #endif + +@@ -643,10 +641,9 @@ static int bgp_listener(int sock, struct sockaddr *sa, socklen_t salen, + + #ifdef IPTOS_PREC_INTERNETCONTROL + if (sa->sa_family == AF_INET) +- setsockopt_ipv4_tos(sock, IPTOS_PREC_INTERNETCONTROL); ++ setsockopt_ipv4_tos(sock, bgp->tcp_dscp); + else if (sa->sa_family == AF_INET6) +- setsockopt_ipv6_tclass(sock, +- IPTOS_PREC_INTERNETCONTROL); ++ setsockopt_ipv6_tclass(sock, bgp->tcp_dscp); + #endif + + sockopt_v6only(sa->sa_family, sock); +diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c +index 7445df883..f91b908a4 100644 +--- a/bgpd/bgp_vty.c ++++ b/bgpd/bgp_vty.c +@@ -1113,6 +1113,42 @@ DEFUN (no_router_bgp, + return CMD_SUCCESS; + } + ++/* bgp session-dscp */ ++ ++DEFUN (bgp_session_dscp, ++ bgp_session_dscp_cmd, ++ "bgp session-dscp DSCP", ++ BGP_STR ++ "Override default (C0) bgp TCP session DSCP value\n" ++ "Manually configured dscp parameter\n") ++{ ++ struct bgp *bgp = VTY_GET_CONTEXT(bgp); ++ ++ uint8_t value = (uint8_t)strtol(argv[2]->arg, NULL, 16); ++ if ((value == 0 && errno == EINVAL) || (value > 0x3f)) ++ { ++ vty_out (vty, "%% Malformed bgp session-dscp parameter\n"); ++ return CMD_WARNING_CONFIG_FAILED; ++ } ++ ++ bgp->tcp_dscp = value << 2; ++ ++ return CMD_SUCCESS; ++} ++ ++DEFUN (no_bgp_session_dscp, ++ no_bgp_session_dscp_cmd, ++ "no bgp session-dscp", ++ NO_STR ++ BGP_STR ++ "Override default (C0) bgp tcp session ip dscp value\n") ++{ ++ struct bgp *bgp = VTY_GET_CONTEXT(bgp); ++ ++ bgp->tcp_dscp = IPTOS_PREC_INTERNETCONTROL; ++ ++ return CMD_SUCCESS; ++} + + /* BGP router-id. */ + +@@ -12798,6 +12834,10 @@ void bgp_vty_init(void) + /* "no router bgp" commands. */ + install_element(CONFIG_NODE, &no_router_bgp_cmd); + ++ /* "bgp session-dscp command */ ++ install_element (BGP_NODE, &bgp_session_dscp_cmd); ++ install_element (BGP_NODE, &no_bgp_session_dscp_cmd); ++ + /* "bgp router-id" commands. */ + install_element(BGP_NODE, &bgp_router_id_cmd); + install_element(BGP_NODE, &no_bgp_router_id_cmd); +diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c +index 6fb3a70c7..503e6b4ed 100644 +--- a/bgpd/bgpd.c ++++ b/bgpd/bgpd.c +@@ -2995,7 +2995,7 @@ static struct bgp *bgp_create(as_t *as, const char *name, + + bgp->evpn_info = XCALLOC(MTYPE_BGP_EVPN_INFO, + sizeof(struct bgp_evpn_info)); +- ++ bgp->tcp_dscp = IPTOS_PREC_INTERNETCONTROL; + bgp_evpn_init(bgp); + bgp_pbr_init(bgp); + return bgp; +@@ -7516,6 +7516,9 @@ int bgp_config_write(struct vty *vty) + if (CHECK_FLAG(bgp->flags, BGP_FLAG_NO_FAST_EXT_FAILOVER)) + vty_out(vty, " no bgp fast-external-failover\n"); + ++ if (bgp->tcp_dscp != IPTOS_PREC_INTERNETCONTROL) ++ vty_out(vty, " bgp session-dscp %02X\n", bgp->tcp_dscp >> 2); ++ + /* BGP router ID. */ + if (bgp->router_id_static.s_addr != 0) + vty_out(vty, " bgp router-id %s\n", +diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h +index df3fde840..a6546457c 100644 +--- a/bgpd/bgpd.h ++++ b/bgpd/bgpd.h +@@ -553,6 +553,9 @@ struct bgp { + /* Count of peers in established state */ + uint32_t established_peers; + ++ /* dscp value for tcp sessions */ ++ uint8_t tcp_dscp; ++ + QOBJ_FIELDS + }; + DECLARE_QOBJ_TYPE(bgp) +-- +2.17.1.windows.2 + diff --git a/src/sonic-frr/patch/0002-Reduce-severity-of-Vty-connected-from-message.patch b/src/sonic-frr/patch/0002-Reduce-severity-of-Vty-connected-from-message.patch new file mode 100644 index 0000000000..ae2b3ba912 --- /dev/null +++ b/src/sonic-frr/patch/0002-Reduce-severity-of-Vty-connected-from-message.patch @@ -0,0 +1,25 @@ +From 87760a6a04d6ffbcc7a1093549bfb76656002b61 Mon Sep 17 00:00:00 2001 +From: Pavel Shirshov +Date: Fri, 14 Jun 2019 17:48:50 -0700 +Subject: [PATCH] Reduce severity of 'Vty connected from' message + +--- + lib/vty.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/lib/vty.c b/lib/vty.c +index 8450922c2..f159d1b4b 100644 +--- a/lib/vty.c ++++ b/lib/vty.c +@@ -1875,7 +1875,7 @@ static int vty_accept(struct thread *thread) + zlog_info("can't set sockopt to vty_sock : %s", + safe_strerror(errno)); + +- zlog_info("Vty connection from %s", ++ zlog_debug("Vty connection from %s", + sockunion2str(&su, buf, SU_ADDRSTRLEN)); + + vty_create(vty_sock, &su); +-- +2.17.1.windows.2 + diff --git a/src/sonic-frr/patch/0003-ignore-nexthop-attribute-when-NLRI-is-present.patch b/src/sonic-frr/patch/0003-ignore-nexthop-attribute-when-NLRI-is-present.patch new file mode 100644 index 0000000000..5d55ce547f --- /dev/null +++ b/src/sonic-frr/patch/0003-ignore-nexthop-attribute-when-NLRI-is-present.patch @@ -0,0 +1,156 @@ +From 7c31178d8f1b8cc3a9627dc7c7bd1d2a51fe54ce Mon Sep 17 00:00:00 2001 +From: Pavel Shirshov +Date: Tue, 18 Jun 2019 15:27:19 -0700 +Subject: [PATCH] ignore nexthop attribute when NLRI is present + +Backport of https://github.com/FRRouting/frr/pull/4258 +--- + bgpd/bgp_attr.c | 73 ++++++++++++++++++++++++++++++----------------- + bgpd/bgp_attr.h | 3 ++ + bgpd/bgp_packet.c | 11 +++++++ + 3 files changed, 61 insertions(+), 26 deletions(-) + +diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c +index 05e103142..ea7f761ab 100644 +--- a/bgpd/bgp_attr.c ++++ b/bgpd/bgp_attr.c +@@ -1257,6 +1257,32 @@ static int bgp_attr_as4_path(struct bgp_attr_parser_args *args, + return BGP_ATTR_PARSE_PROCEED; + } + ++/* ++ * Check that the nexthop attribute is valid. ++ */ ++bgp_attr_parse_ret_t ++bgp_attr_nexthop_valid(struct peer *peer, struct attr *attr) ++{ ++ in_addr_t nexthop_h; ++ ++ nexthop_h = ntohl(attr->nexthop.s_addr); ++ if ((IPV4_NET0(nexthop_h) || IPV4_NET127(nexthop_h) ++ || IPV4_CLASS_DE(nexthop_h)) ++ && !BGP_DEBUG(allow_martians, ALLOW_MARTIANS)) { ++ char buf[INET_ADDRSTRLEN]; ++ ++ inet_ntop(AF_INET, &attr->nexthop.s_addr, buf, ++ INET_ADDRSTRLEN); ++ flog_err(EC_BGP_ATTR_MARTIAN_NH, "Martian nexthop %s", ++ buf); ++ bgp_notify_send(peer, BGP_NOTIFY_UPDATE_ERR, ++ BGP_NOTIFY_UPDATE_INVAL_NEXT_HOP); ++ return BGP_ATTR_PARSE_ERROR; ++ } ++ ++ return BGP_ATTR_PARSE_PROCEED; ++} ++ + /* Nexthop attribute. */ + static bgp_attr_parse_ret_t bgp_attr_nexthop(struct bgp_attr_parser_args *args) + { +@@ -1264,8 +1290,6 @@ static bgp_attr_parse_ret_t bgp_attr_nexthop(struct bgp_attr_parser_args *args) + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; + +- in_addr_t nexthop_h, nexthop_n; +- + /* Check nexthop attribute length. */ + if (length != 4) { + flog_err(EC_BGP_ATTR_LEN, +@@ -1275,30 +1299,7 @@ static bgp_attr_parse_ret_t bgp_attr_nexthop(struct bgp_attr_parser_args *args) + args->total); + } + +- /* According to section 6.3 of RFC4271, syntactically incorrect NEXT_HOP +- attribute must result in a NOTIFICATION message (this is implemented +- below). +- At the same time, semantically incorrect NEXT_HOP is more likely to +- be just +- logged locally (this is implemented somewhere else). The UPDATE +- message +- gets ignored in any of these cases. */ +- nexthop_n = stream_get_ipv4(peer->curr); +- nexthop_h = ntohl(nexthop_n); +- if ((IPV4_NET0(nexthop_h) || IPV4_NET127(nexthop_h) +- || IPV4_CLASS_DE(nexthop_h)) +- && !BGP_DEBUG( +- allow_martians, +- ALLOW_MARTIANS)) /* loopbacks may be used in testing */ +- { +- char buf[INET_ADDRSTRLEN]; +- inet_ntop(AF_INET, &nexthop_n, buf, INET_ADDRSTRLEN); +- flog_err(EC_BGP_ATTR_MARTIAN_NH, "Martian nexthop %s", buf); +- return bgp_attr_malformed( +- args, BGP_NOTIFY_UPDATE_INVAL_NEXT_HOP, args->total); +- } +- +- attr->nexthop.s_addr = nexthop_n; ++ attr->nexthop.s_addr = stream_get_ipv4(peer->curr); + attr->flag |= ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP); + + return BGP_ATTR_PARSE_PROCEED; +@@ -2669,6 +2670,26 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, + return BGP_ATTR_PARSE_ERROR; + } + ++ /* ++ * RFC4271: If the NEXT_HOP attribute field is syntactically incorrect, ++ * then the Error Subcode MUST be set to Invalid NEXT_HOP Attribute. ++ * This is implemented below and will result in a NOTIFICATION. If the ++ * NEXT_HOP attribute is semantically incorrect, the error SHOULD be ++ * logged, and the route SHOULD be ignored. In this case, a NOTIFICATION ++ * message SHOULD NOT be sent. This is implemented elsewhere. ++ * ++ * RFC4760: An UPDATE message that carries no NLRI, other than the one ++ * encoded in the MP_REACH_NLRI attribute, SHOULD NOT carry the NEXT_HOP ++ * attribute. If such a message contains the NEXT_HOP attribute, the BGP ++ * speaker that receives the message SHOULD ignore this attribute. ++ */ ++ if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP)) ++ && !CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_MP_REACH_NLRI))) { ++ if (bgp_attr_nexthop_valid(peer, attr) < 0) { ++ return BGP_ATTR_PARSE_ERROR; ++ } ++ } ++ + /* Check all mandatory well-known attributes are present */ + if ((ret = bgp_attr_check(peer, attr)) < 0) { + if (as4_path) +diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h +index 47a4182fe..a86684583 100644 +--- a/bgpd/bgp_attr.h ++++ b/bgpd/bgp_attr.h +@@ -350,6 +350,9 @@ extern void bgp_packet_mpunreach_prefix(struct stream *s, struct prefix *p, + uint32_t, int, uint32_t, struct attr *); + extern void bgp_packet_mpunreach_end(struct stream *s, size_t attrlen_pnt); + ++extern bgp_attr_parse_ret_t bgp_attr_nexthop_valid(struct peer *peer, ++ struct attr *attr); ++ + static inline int bgp_rmap_nhop_changed(uint32_t out_rmap_flags, + uint32_t in_rmap_flags) + { +diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c +index fe8a1a256..13f4cf436 100644 +--- a/bgpd/bgp_packet.c ++++ b/bgpd/bgp_packet.c +@@ -1533,6 +1533,17 @@ static int bgp_update_receive(struct peer *peer, bgp_size_t size) + nlris[NLRI_UPDATE].nlri = stream_pnt(s); + nlris[NLRI_UPDATE].length = update_len; + stream_forward_getp(s, update_len); ++ ++ if (CHECK_FLAG(attr.flag, ATTR_FLAG_BIT(BGP_ATTR_MP_REACH_NLRI))) { ++ /* ++ * We skipped nexthop attribute validation earlier so ++ * validate the nexthop now. ++ */ ++ if (bgp_attr_nexthop_valid(peer, &attr) < 0) { ++ bgp_attr_unintern_sub(&attr); ++ return BGP_Stop; ++ } ++ } + } + + if (BGP_DEBUG(update, UPDATE_IN)) +-- +2.17.1.windows.2 + diff --git a/src/sonic-frr/patch/0004-Allow-BGP-attr-NEXT_HOP-to-be-0.0.0.0-due-to-allevia.patch b/src/sonic-frr/patch/0004-Allow-BGP-attr-NEXT_HOP-to-be-0.0.0.0-due-to-allevia.patch new file mode 100644 index 0000000000..22cf08b82c --- /dev/null +++ b/src/sonic-frr/patch/0004-Allow-BGP-attr-NEXT_HOP-to-be-0.0.0.0-due-to-allevia.patch @@ -0,0 +1,27 @@ +From 4cd83e56e4f67fdc06325d92a82534fb4cb69952 Mon Sep 17 00:00:00 2001 +From: Pavel Shirshov +Date: Thu, 20 Jun 2019 15:35:50 -0700 +Subject: [PATCH] Allow BGP attr NEXT_HOP to be 0.0.0.0 due to alleviate the + vendor bug + +--- + bgpd/bgp_route.c | 3 +-- + 1 file changed, 1 insertion(+), 2 deletions(-) + +diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c +index 38f3cad5a..55240eab8 100644 +--- a/bgpd/bgp_route.c ++++ b/bgpd/bgp_route.c +@@ -2873,8 +2873,7 @@ static int bgp_update_martian_nexthop(struct bgp *bgp, afi_t afi, safi_t safi, + + /* If NEXT_HOP is present, validate it. */ + if (attr->flag & ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP)) { +- if (attr->nexthop.s_addr == 0 +- || IPV4_CLASS_DE(ntohl(attr->nexthop.s_addr)) ++ if (IPV4_CLASS_DE(ntohl(attr->nexthop.s_addr)) + || bgp_nexthop_self(bgp, attr->nexthop)) + return 1; + } +-- +2.17.1.windows.2 + diff --git a/src/sonic-frr/patch/series b/src/sonic-frr/patch/series new file mode 100644 index 0000000000..05df330b9b --- /dev/null +++ b/src/sonic-frr/patch/series @@ -0,0 +1,4 @@ +0001-Add-support-of-bgp-tcp-DSCP-value.patch +0002-Reduce-severity-of-Vty-connected-from-message.patch +0003-ignore-nexthop-attribute-when-NLRI-is-present.patch +0004-Allow-BGP-attr-NEXT_HOP-to-be-0.0.0.0-due-to-allevia.patch