From ee4613c449f11b4e6100248b151479e15a153ec5 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Fri, 7 Jun 2019 18:44:45 -0700 Subject: [PATCH 1/8] Update sonic-quagga submodule --- src/sonic-quagga | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic-quagga b/src/sonic-quagga index 2e192c06b8f5..6397c2c9b588 160000 --- a/src/sonic-quagga +++ b/src/sonic-quagga @@ -1 +1 @@ -Subproject commit 2e192c06b8f526cab6fce710ab5da0223b0ba2b1 +Subproject commit 6397c2c9b588a2271aaf3f4d7383111db16d090a From 8aaee9a7cd382931bc4e6d71b988eacff03f4c01 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Fri, 14 Jun 2019 19:12:23 -0700 Subject: [PATCH 2/8] Port some patches from sonic-quagga --- src/sonic-frr/Makefile | 5 + ...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 | 98 ++++++++++++ src/sonic-frr/patch/series | 3 + 5 files changed, 272 insertions(+) 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/series diff --git a/src/sonic-frr/Makefile b/src/sonic-frr/Makefile index 48da9c72116e..21ec3c75e118 100644 --- a/src/sonic-frr/Makefile +++ b/src/sonic-frr/Makefile @@ -8,6 +8,11 @@ DERIVED_TARGET = $(FRR_PYTHONTOOLS) $(FRR_DBG) $(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% : # Build the package pushd ./frr + git reset --hard + git clean -xfdf + stg init + git checkout -b work frr/7.0 + stg import -s ../patch/series 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 000000000000..e9f496d7af62 --- /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 000000000000..ae2b3ba91223 --- /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 000000000000..ef04c280fe87 --- /dev/null +++ b/src/sonic-frr/patch/0003-ignore-nexthop-attribute-when-NLRI-is-present.patch @@ -0,0 +1,98 @@ +From fcaec3a5712a26d3d3e03f76e52fecbe21c4eff6 Mon Sep 17 00:00:00 2001 +From: Pavel Shirshov +Date: Fri, 14 Jun 2019 18:08:26 -0700 +Subject: [PATCH] ignore nexthop attribute when NLRI is present + +According to section 1.3 of RFC2858, 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. +--- + bgpd/bgp_attr.c | 56 +++++++++++++++++++++++++++++-------------------- + 1 file changed, 33 insertions(+), 23 deletions(-) + +diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c +index 167ad89a5..ca88a8cb5 100644 +--- a/bgpd/bgp_attr.c ++++ b/bgpd/bgp_attr.c +@@ -1263,7 +1263,7 @@ 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; ++ in_addr_t nexthop_n; + + /* Check nexthop attribute length. */ + if (length != 4) { +@@ -1274,29 +1274,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->flag |= ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP); + +@@ -2688,6 +2666,38 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, + return ret; + } + ++ /* ++ * 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. ++ * ++ * According to section 1.3 of RFC2858, 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))) ++ { ++ 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; ++ } ++ } ++ + /* + * At this place we can see whether we got AS4_PATH and/or + * AS4_AGGREGATOR from a 16Bit peer and act accordingly. +-- +2.17.1.windows.2 + diff --git a/src/sonic-frr/patch/series b/src/sonic-frr/patch/series new file mode 100644 index 000000000000..896cf9d2680c --- /dev/null +++ b/src/sonic-frr/patch/series @@ -0,0 +1,3 @@ +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 From a82060f800018e18fe934db7b87685f2dc332830 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Fri, 14 Jun 2019 22:36:05 -0700 Subject: [PATCH 3/8] Fix Makefile --- src/sonic-frr/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic-frr/Makefile b/src/sonic-frr/Makefile index 21ec3c75e118..d7713cd1ffaf 100644 --- a/src/sonic-frr/Makefile +++ b/src/sonic-frr/Makefile @@ -10,8 +10,8 @@ $(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% : pushd ./frr git reset --hard git clean -xfdf - stg init git checkout -b work frr/7.0 + stg init stg import -s ../patch/series tools/tarsource.sh -V -e '-sonic' dpkg-buildpackage -rfakeroot -b -us -uc -Ppkg.frr.nortrlib -j$(SONIC_CONFIG_MAKE_JOBS) From b734609d2d8b17e248b10058576ca4f238ac425c Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Sat, 15 Jun 2019 00:24:48 -0700 Subject: [PATCH 4/8] Another patch --- src/sonic-frr/Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sonic-frr/Makefile b/src/sonic-frr/Makefile index d7713cd1ffaf..06685a463720 100644 --- a/src/sonic-frr/Makefile +++ b/src/sonic-frr/Makefile @@ -10,7 +10,8 @@ $(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% : pushd ./frr git reset --hard git clean -xfdf - git checkout -b work frr/7.0 + git branch work origin/frr/7.0 + git checkout work stg init stg import -s ../patch/series tools/tarsource.sh -V -e '-sonic' From eb434dbf94fe2ac56fab1378b441a8c37e360636 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Mon, 17 Jun 2019 09:42:12 -0700 Subject: [PATCH 5/8] Uncomment bgp test --- platform/vs/tests/bgp/test_invalid_nexthop.py | 1 - 1 file changed, 1 deletion(-) diff --git a/platform/vs/tests/bgp/test_invalid_nexthop.py b/platform/vs/tests/bgp/test_invalid_nexthop.py index 9458ecdeee5c..021e3bfea002 100644 --- a/platform/vs/tests/bgp/test_invalid_nexthop.py +++ b/platform/vs/tests/bgp/test_invalid_nexthop.py @@ -5,7 +5,6 @@ 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") From 69f81fc09fb46bbea86585fec0e95fadc2bbe79f Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Tue, 18 Jun 2019 15:42:16 -0700 Subject: [PATCH 6/8] Downport Nikos's patch --- ...xthop-attribute-when-NLRI-is-present.patch | 148 ++++++++++++------ 1 file changed, 103 insertions(+), 45 deletions(-) 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 index ef04c280fe87..5d55ce547fd1 100644 --- 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 @@ -1,30 +1,62 @@ -From fcaec3a5712a26d3d3e03f76e52fecbe21c4eff6 Mon Sep 17 00:00:00 2001 +From 7c31178d8f1b8cc3a9627dc7c7bd1d2a51fe54ce Mon Sep 17 00:00:00 2001 From: Pavel Shirshov -Date: Fri, 14 Jun 2019 18:08:26 -0700 +Date: Tue, 18 Jun 2019 15:27:19 -0700 Subject: [PATCH] ignore nexthop attribute when NLRI is present -According to section 1.3 of RFC2858, 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. +Backport of https://github.com/FRRouting/frr/pull/4258 --- - bgpd/bgp_attr.c | 56 +++++++++++++++++++++++++++++-------------------- - 1 file changed, 33 insertions(+), 23 deletions(-) + 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 167ad89a5..ca88a8cb5 100644 +index 05e103142..ea7f761ab 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c -@@ -1263,7 +1263,7 @@ static bgp_attr_parse_ret_t bgp_attr_nexthop(struct bgp_attr_parser_args *args) +@@ -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; -+ in_addr_t nexthop_n; - +- /* Check nexthop attribute length. */ if (length != 4) { -@@ -1274,29 +1274,7 @@ static bgp_attr_parse_ret_t bgp_attr_nexthop(struct bgp_attr_parser_args *args) + 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); } @@ -36,7 +68,7 @@ index 167ad89a5..ca88a8cb5 100644 - 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_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)) @@ -51,48 +83,74 @@ index 167ad89a5..ca88a8cb5 100644 - args, BGP_NOTIFY_UPDATE_INVAL_NEXT_HOP, args->total); - } - - attr->nexthop.s_addr = nexthop_n; +- attr->nexthop.s_addr = nexthop_n; ++ attr->nexthop.s_addr = stream_get_ipv4(peer->curr); attr->flag |= ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP); -@@ -2688,6 +2666,38 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, - return ret; + 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; } + /* -+ * 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. ++ * 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. + * -+ * According to section 1.3 of RFC2858, 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. ++ * 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))) -+ { -+ 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); ++ 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; + } + } + - /* - * At this place we can see whether we got AS4_PATH and/or - * AS4_AGGREGATOR from a 16Bit peer and act accordingly. + /* 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 From 80e6322bb1f635d6f374b62f95b6cbeac2669d37 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Thu, 20 Jun 2019 16:18:51 -0700 Subject: [PATCH 7/8] Add a patch to alleviate the vendor issue --- ...EXT_HOP-to-be-0.0.0.0-due-to-allevia.patch | 27 +++++++++++++++++++ src/sonic-frr/patch/series | 1 + 2 files changed, 28 insertions(+) create mode 100644 src/sonic-frr/patch/0004-Allow-BGP-attr-NEXT_HOP-to-be-0.0.0.0-due-to-allevia.patch 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 000000000000..22cf08b82cd4 --- /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 index 896cf9d2680c..05df330b9bae 100644 --- a/src/sonic-frr/patch/series +++ b/src/sonic-frr/patch/series @@ -1,3 +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 From d62e0ff69fdad0f501c6cb5fb04e9849d7c86a17 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Sun, 23 Jun 2019 12:48:22 -0700 Subject: [PATCH 8/8] use patch instead of stg --- src/sonic-frr/Makefile | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/sonic-frr/Makefile b/src/sonic-frr/Makefile index 06685a463720..80fb92165909 100644 --- a/src/sonic-frr/Makefile +++ b/src/sonic-frr/Makefile @@ -8,12 +8,10 @@ DERIVED_TARGET = $(FRR_PYTHONTOOLS) $(FRR_DBG) $(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% : # Build the package pushd ./frr - git reset --hard - git clean -xfdf - git branch work origin/frr/7.0 - git checkout work - stg init - stg import -s ../patch/series + 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