bgpd: Enforce self-next-hop check in next-hop update. #6191
Conversation
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/a7fd4b2934bb7b6d6fa018d51b3988ac/raw/58bfc1fd06fdbb575c17e2de6bab548d873dc49e/cr_6191_1586418055.diff | git apply
diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c
index a4dfc0c8e..6ed2b63b5 100644
--- a/bgpd/bgp_nht.c
+++ b/bgpd/bgp_nht.c
@@ -745,12 +745,12 @@ static void evaluate_paths(struct bgp_nexthop_cache *bnc)
bnc_is_valid_nexthop =
bgp_isvalid_labeled_nexthop(bnc) ? 1 : 0;
} else {
- if (bgp_update_martian_nexthop(bnc->bgp, afi, safi,
- path->type,
- path->sub_type,
- path->attr, rn)) {
- zlog_debug("Prefix %pRN, ignoring path due to martian or self-next-hop",
- rn);
+ if (bgp_update_martian_nexthop(
+ bnc->bgp, afi, safi, path->type,
+ path->sub_type, path->attr, rn)) {
+ zlog_debug(
+ "Prefix %pRN, ignoring path due to martian or self-next-hop",
+ rn);
} else {
bnc_is_valid_nexthop =
bgp_isvalid_nexthop(bnc) ? 1 : 0;
If you are a new contributor to FRR, please see our contributing guidelines.
NaveenThanikachalam
force-pushed
the
ibgp_connected
branch
from
c290173 to
611042a
Compare
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11749/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11750/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
|
Why is the commit message so radically different than the comment that came with the commit above? Why wouldn't we want this discussion in the commit message? |
NaveenThanikachalam
force-pushed
the
ibgp_connected
branch
from
611042a to
233fae5
Compare
There was a problem hiding this comment.
Thanks for your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-byline; we can't accept your contribution until all of your commits have one
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/79943933562db58198d0d61c2a1f8926/raw/a6142e1c0dd7f57852934d3d45033feab61e32ba/cr_6191_1586487385.diff | git apply
diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c
index ab6596b29..baf4e5796 100644
--- a/bgpd/bgp_nht.c
+++ b/bgpd/bgp_nht.c
@@ -750,8 +750,8 @@ static void evaluate_paths(struct bgp_nexthop_cache *bnc)
path->sub_type, path->attr, rn)) {
if (BGP_DEBUG(nht, NHT))
zlog_debug(
- "%s: Prefix %pRN (vrf %s), ignoring path due to martian or self-next-hop",
- __func__, rn, bgp_path->name);
+ "%s: Prefix %pRN (vrf %s), ignoring path due to martian or self-next-hop",
+ __func__, rn, bgp_path->name);
} else {
bnc_is_valid_nexthop =
bgp_isvalid_nexthop(bnc) ? 1 : 0;
If you are a new contributor to FRR, please see our contributing guidelines.
NaveenThanikachalam
force-pushed
the
ibgp_connected
branch
from
233fae5 to
431d7c3
Compare
There was a problem hiding this comment.
Thanks for your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-byline; we can't accept your contribution until all of your commits have one
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/fc576456fb5c9bfaec071e24e7979bbc/raw/a6142e1c0dd7f57852934d3d45033feab61e32ba/cr_6191_1586487460.diff | git apply
diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c
index ab6596b29..baf4e5796 100644
--- a/bgpd/bgp_nht.c
+++ b/bgpd/bgp_nht.c
@@ -750,8 +750,8 @@ static void evaluate_paths(struct bgp_nexthop_cache *bnc)
path->sub_type, path->attr, rn)) {
if (BGP_DEBUG(nht, NHT))
zlog_debug(
- "%s: Prefix %pRN (vrf %s), ignoring path due to martian or self-next-hop",
- __func__, rn, bgp_path->name);
+ "%s: Prefix %pRN (vrf %s), ignoring path due to martian or self-next-hop",
+ __func__, rn, bgp_path->name);
} else {
bnc_is_valid_nexthop =
bgp_isvalid_nexthop(bnc) ? 1 : 0;
If you are a new contributor to FRR, please see our contributing guidelines.
NaveenThanikachalam
force-pushed
the
ibgp_connected
branch
from
431d7c3 to
be18a0f
Compare
There was a problem hiding this comment.
Thanks for your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-byline; we can't accept your contribution until all of your commits have one
If you are a new contributor to FRR, please see our contributing guidelines.
NaveenThanikachalam
force-pushed
the
ibgp_connected
branch
from
be18a0f to
d995b65
Compare
Thanks, @donaldsharp. I thought I'd keep the commit message crisp. |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11771/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11770/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11772/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11773/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
NaveenThanikachalam
force-pushed
the
ibgp_connected
branch
from
d995b65 to
1e4b59c
Compare
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11774/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
|
ci:rerun |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11775/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
|
ci:rerun |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11782/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
Problem Description: ===================== +--+ +--+ |R1|-(192.201.202.1)----iBGP----(192.201.202.2)-|R2| +--+ +--+ Routes on R2: ============= S>* 202.202.202.202/32 [1/0] via 192.201.78.1, ens256, 00:40:48 Where, the next-hop network, 192.201.78.0/24, is a directly connected network address. C>* 192.201.78.0/24 is directly connected, ens256, 00:40:48 Configurations on R1: ===================== ! router bgp 201 bgp router-id 192.168.0.1 neighbor 192.201.202.2 remote-as 201 ! Configurations on R2: ===================== ! ip route 202.202.202.202/32 192.201.78.1 ! router bgp 201 bgp router-id 192.168.0.2 neighbor 192.201.202.1 remote-as 201 ! address-family ipv4 unicast redistribute static exit-address-family ! Step-1: ======= R1 receives the route 202.202.202.202/32 from R2. R1 installs the route in its BGP RIB. Step-2: ======= On R1, a connected interface address is added. The address is the same as the next-hop of the BGP route received from R2 (192.201.78.1). Point of Failure: ================= R1 resolves the BGP route even though the route's next-hop is its own connected address. Even though this appears to be a misconfiguration it would still be better to safeguard the code against it. Fix: ==== When BGP receives a connected route from Zebra, it processes the routes for the next-hop update. While doing so, BGP must ignore routes whose next-hop address matches the address of the connected route for which Zebra sent the next-hop update message. Signed-off-by: NaveenThanikachalam <nthanikachal@vmware.com>
NaveenThanikachalam
force-pushed
the
ibgp_connected
branch
from
1e4b59c to
e7cbe5e
Compare
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11791/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
There was a problem hiding this comment.
@NaveenThanikachalam can we have this behavior covered in topotests?
Thanks, @ton31337. Sure, I'll write up a topotest for this. |
Problem Description:
Routes on R2:
S>* 202.202.202.202/32 [1/0] via 192.201.78.1, ens256, 00:40:48Where, the next-hop network, 192.201.78.0/24, is a directly connected network address.
C>* 192.201.78.0/24 is directly connected, ens256, 00:40:48Configurations on R1:
Configurations on R2:
Step-1:
R1 receives the route 202.202.202.202/32 from R2.
R1 installs the route in its BGP RIB.
Step-2:
On R1, a connected interface address is added. The address is the same as the next-hop of the BGP route received from R2 (192.201.78.1).
Point of Failure:
R1 resolves the BGP route even though the route's next-hop is its own connected address.
Even though this appears to be a misconfiguration it would still be better to safeguard the code against it.
Fix:
When BGP receives a connected route from Zebra, it processes the
routes for the next-hop update.
While doing so, BGP must ignore routes whose next-hop address matches
the address of the connected route for which Zebra sent the next-hop update
message.
Signed-off-by: NaveenThanikachalam nthanikachal@vmware.com