Skip to content

Commit

Permalink
[FRR]: Port some patches from sonic-quagga repo (#3017)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
pavel-shirshov authored Jun 23, 2019
1 parent 93582c7 commit dd0f005
Show file tree
Hide file tree
Showing 7 changed files with 357 additions and 1 deletion.
1 change: 0 additions & 1 deletion platform/vs/tests/bgp/test_invalid_nexthop.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
4 changes: 4 additions & 0 deletions src/sonic-frr/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
141 changes: 141 additions & 0 deletions src/sonic-frr/patch/0001-Add-support-of-bgp-tcp-DSCP-value.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
From ef017a613691a40f32cdaa5396bbd4889b1cb647 Mon Sep 17 00:00:00 2001
From: Pavel Shirshov <pavelsh@microsoft.com>
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

Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
From 87760a6a04d6ffbcc7a1093549bfb76656002b61 Mon Sep 17 00:00:00 2001
From: Pavel Shirshov <pavelsh@microsoft.com>
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

Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
From 7c31178d8f1b8cc3a9627dc7c7bd1d2a51fe54ce Mon Sep 17 00:00:00 2001
From: Pavel Shirshov <pavelsh@microsoft.com>
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

Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
From 4cd83e56e4f67fdc06325d92a82534fb4cb69952 Mon Sep 17 00:00:00 2001
From: Pavel Shirshov <pavelsh@microsoft.com>
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

4 changes: 4 additions & 0 deletions src/sonic-frr/patch/series
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit dd0f005

Please sign in to comment.