It was hard to catch those unless using higher values than uint32_t, but
already hit, it's time to fix completely.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
(cherry picked from commit d782e3ffa2)
Since we increased peer->af_flags from uint32_t to uint64_t,
peer_af_flag_check() was historically returning integer, and not bool
as should be.
The bug was that if we have af_flags higher than uint32_t it will never
returned a right value.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
(cherry picked from commit 2c722516c3)
The l2vpn afi was not being properly displayed
when a show run was being issued. Add a
afi2str_lower function and use it.
Fixes: #12867
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
(cherry picked from commit 0884f1644b)
According to the flowspec RFC 8955 a flowspec nlri is <length, <nlri data>>
Specifying 0 as a length makes BGP get all warm on the inside. Which
in this case is not a good thing at all. Prevent warmth, stay cold
on the inside.
Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
(cherry picked from commit 0b999c886e)
Before:
```
r1# sh ip bgp wide
BGP table version is 1, local router ID is 192.168.2.1, vrf id 0
Default local pref 100, local AS 65001
Status codes: s suppressed, d damped, h history, * valid, > best, = multipath,
i internal, r RIB-failure, S Stale, R Removed
Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self
Origin codes: i - IGP, e - EGP, ? - incomplete
RPKI validation codes: V valid, I invalid, N Not found
Network Next Hop Metric LocPrf Weight Path
* 172.16.255.254/32 192.168.2.2 0 0 (65003) i
*> 192.168.1.2 0 0 (65002) i
Displayed 1 routes and 2 total paths
r1#
```
After:
```
r1# sh ip bgp wide
BGP table version is 1, local router ID is 192.168.2.1, vrf id 0
Default local pref 100, local AS 65001
Status codes: s suppressed, d damped, h history, * valid, > best, = multipath,
i internal, r RIB-failure, S Stale, R Removed
Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self
Origin codes: i - IGP, e - EGP, ? - incomplete
RPKI validation codes: V valid, I invalid, N Not found
Network Next Hop Metric LocPrf Weight Path
* 172.16.255.254/32 192.168.2.2 0 0 (65003) i
*> 192.168.1.2 0 0 (65002) i
Displayed 1 routes and 2 total paths
r1#
```
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
(cherry picked from commit fccd7e53db)
Sometimes bgp connections can be rejected for a variety of reasons. Give
a bit more context about what is going wrong so that the operator can
make better decisions about their network.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
(cherry picked from commit 7c5062fb27)
An rfapi timer callback is responsible for the memory in a
context object, even in special-case exit paths. Always
free that object.
Signed-off-by: Mark Stapp <mjs@labn.net>
(cherry picked from commit 870d3d2cb8)
When we specify remote-as as external/internal, we need to set local_as to
bgp->as, instead of bgp->confed_id. Before this patch, (bgp->as != *as) is
always valid for such a case because *as is always 0.
Also, append peer->local_as as CONFED_SEQ to avoid other side withdrawing
the routes due to confederation own AS received and/or malformed as-path.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
(cherry picked from commit db5a5ee6e4)
Using a route-map to update the local ipv6 address has to be
better clarified. Actually, when a VPN SAFI is used, the nexthop
length must be changed to 48 bytes. Other cases, the length will
be 32 bytes.
Fixes: 9795e9f234 ("bgpd: fix when route-map changes the link local
nexthop for vpnv6")
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
(cherry picked from commit 5bbcc78202)
Because mp_nexthop_len attribute value stands for the length
to encode in the stream, simplify the way the nexthop is
forged.
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
(cherry picked from commit 558e8f5801)
Ensure that a multipath set is fully comprised of EVPN paths (i.e.,
paths imported into the VRF from EVPN address-family) or non-EVPN
paths. This is actually a condition that existed already in the code
but was not properly enforced.
This change, as a side effect, eliminates the known trigger condition
for bad or missing RMAC programming in an EVPN deployment, described
in tickets CM-29043 and CM-31222. Routes (actually, paths) in a VRF
routing table that require VXLAN tunneling to the next hop currently
need some special handling in zebra to deal with the nexthop (neigh)
and RMAC programming, and this is implemented for the entire route
(prefix), not per-path. This can lead to the bad or missing RMAC
situation, which is now eliminated by ensuring all paths in the route
are 'similar'.
The longer-term solution in CL 5.x will be to deal with the special
programming by means of explicit communication between bgpd and zebra.
This is already implemented for EVPN-MH via CM-31398. These changes
will be extended to non-MH also and the special code in zebra removed
or refined.
Signed-off-by: Vivek Venkatraman <vivek@nvidia.com>
Acked-by: Trey Aspelund <taspelund@nvidia.com>
Acked-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Acked-by: Chirag Shah <chirag@nvidia.com>
Ticket: CM-29043
Testing Done:
1. Manual testing
2. precommit on both MLX and BCM platforms
3. evpn-smoke - BCM and VX
Results described in the ticket
(cherry picked from commit d2d71b042e)
When performing deterministic MED processing, ensure that the peer
status is not checked when we encounter a stale path. Otherwise, this
path will be skipped from the DMED consideration leading to it potentially
not being installed.
Test scenario: Consider a prefix with 2 (multi)paths. The peer that
announces the path with the winning DMED undergoes a graceful-restart.
Before it comes back up, the other path goes away. Prior to the fix, a
third router that receives both these paths would have ended up not
having any path installed to the prefix after the above events.
Signed-off-by: Vivek Venkatraman <vivek@nvidia.com>
For internal use:
Ticket: CM-32032
Testing done: Multiple manual testing
(cherry picked from commit de692a4ebe)
When we receive a default route from a peer and we originate default route
using `neighbor default-originate`, we do not track of struct attr we use,
and when we do `no neighbor default-originate` we withdraw our generated
default route, but we announce default-route from the peer.
After we do this, we unintern aspath (which was used for default-originate),
BUT it was used also for peer's default route we received.
And here we have a use-after-free crash, because bgp_process_main_one()
reaps old paths that are marked as BGP_PATH_REMOVED with aspath->refcnt > 0,
but here it's 0.
```
0 0x55c24bbcd022 in aspath_key_make bgpd/bgp_aspath.c:2070
1 0x55c24b8f1140 in attrhash_key_make bgpd/bgp_attr.c:777
2 0x7f52322e66c9 in hash_release lib/hash.c:220
3 0x55c24b8f6017 in bgp_attr_unintern bgpd/bgp_attr.c:1271
4 0x55c24ba0acaa in bgp_path_info_free_with_caller bgpd/bgp_route.c:283
5 0x55c24ba0a7de in bgp_path_info_unlock bgpd/bgp_route.c:309
6 0x55c24ba0af6d in bgp_path_info_reap bgpd/bgp_route.c:426
7 0x55c24ba17b9a in bgp_process_main_one bgpd/bgp_route.c:3333
8 0x55c24ba18a1d in bgp_process_wq bgpd/bgp_route.c:3425
9 0x7f52323c2cd5 in work_queue_run lib/workqueue.c:282
10 0x7f52323aab92 in thread_call lib/thread.c:2006
11 0x7f5232300dc7 in frr_run lib/libfrr.c:1198
12 0x55c24b8ea792 in main bgpd/bgp_main.c:520
13 0x7f5231c3a082 in __libc_start_main ../csu/libc-start.c:308
14 0x55c24b8ef0bd in _start (/usr/lib/frr/bgpd+0x2c90bd)
```
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
(cherry picked from commit e9340ff429)
```
anlan(config-router-af)# vni 33
anlan(config-router-af-vni)# route-target both 44:55
anlan(config-router-af-vni)# no route-target both 44:55
vtysh: error reading from bgpd: Resource temporarily unavailable (11)Warning: closing connection to bgpd because of an I/O error!
```
When `bgp_evpn_vni_rt_cmd` deals with "both" type, it wrongly created
only one node ( should be two nodes ) for lists of both `vpn->import_rtl` and
`vpn->export_rtl`. At this time, the two lists are already wrong.
In `no route-target both RT`, it will free the single node from lists of both
`vpn->import_rtl` and `vpn->export_rtl`. After freed from `vpn->import_rtl`,
it is "use-after-free" at the time of freeing it from `vpn->export_rtl`.
It causes crash sometimes, or other unexpected behaviours.
This issue is introduced by commit `3b7e8d`, which have adjusted both
`bgp_evpn_vni_rt_cmd` and `bgp_evpn_vrf_rt_cmd`.
Since `bgp_evpn_vrf_rt_cmd/no_bgp_evpn_vrf_rt_cmd` works well again
unintentionally with commit `7022da`, only `bgp_evpn_vni_rt_cmd` needs to
modify - add two nodes for "both" type and some explicit comments for this
special case of "both" type.
Signed-off-by: anlan_cs <vic.lan@pica8.com>
(cherry picked from commit 432ff4b036)
BGP was modified in a0b937de42
to grab the peer->io_mtx before validating the header to ensure
that the input Queue was not being modified by anyone else at that
moment in time. Unfortunately validate_header can detect a problem
and attempt to relock the mutex, which deadlocks. This deadlock in
the bgp_io pthread is the lone deadlock at first, eventually though
bgp attempts to write another packet to the peer( say when the
it's time to send the next packet ) and the main pthread of bgpd
becomes deadlocked and then the whole bgpd process is stuck at that
point in time leaving us dead in the water.
The point of locking the mutex earlier was to ensure that the input
Queue wasn't being modified by anyone else, (Say reading off it )
as that we wanted to ensure that we don't hold more packets then necessary.
Let's grab the mutex long enough to look at the input Q size, this
ensure that we have room and then we can validate_header and do the right
thing from there. We'll need to lock the mutex when we actually move it
into the input Q as well.
Fixes: #12725
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Commit: 3cdb03fba7
changed the vty_json output to not be pretty printing.
The previous commit in the tree added vty_json_no_pretty
let's use that instead
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Initial commit: 23b2a7ef52
changed the json output of `show bgp <afi> <safi> json` to
not have pretty print because when under a situation where
there are a bunch of routes with a large scale ecmp show
output was taking forever and this commit cut 2 minutes out
of vtysh run time.
Subusequent commit: f4ec52f7cc
changed this back.
When upgrading to latest version the long run time was noticed
due to testing. Let's add back this functionality such that
FRR can have reduced run times with vtysh when it's really
needed.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Before this patch, we always passed `struct attr` for NLRI_UPDATE, but if we
have a situation with treat-as-withdraw (for example: malformed attribute, or
using a command like `neighbor path-attribute treat-as-withdraw`) the route
MUST be withdrawn form the BGP table.
Hence, we MUST pass attr as NULL, in this case we already have this check
under NLRI_ATTR_ARG() macro, just reuse it properly.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
The function ecommunity_str2com_internal appears to want to handle
the ecommunity_token_rt6 enum but skips over it. Commit
9a659715df tried to add this but I really
don't see how this is going to behave correctly. Add the
ecommunity_token_rt6 case to the switch statement so it is handled
appropriately?
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Before this, if the peer disables sending FQDN capability, the old hostname
still (STALE) exists and is misleading in the outputs of `show bgp ...`.
Especially when using with `bgp default show-hostname`, etc.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
These two functions always return 0. As such any and all
tests against this make no sense. Remove the return 0
to a void and follow the chain, logically, to remove all
the dead code.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Moves the old/new IP comparison into handle_tunnel_ip_change instead of
expecting the caller to do the check on their own.
Also changes handle_tunnel_ip_change to return void since it only ever
returned 0 in all cases.
Signed-off-by: Trey Aspelund <taspelund@nvidia.com>
When processing a new local VNI, we were always walking the global EVPN
table to look for routes that needed to be removed due to a martian
nexthop change (specifically a tunnel-ip change).
Since the martian TIP table is global (all VNIs) + the walk is also in
the global table (all VNIs), we can trust that any new TIP from any VNI
would result in routes getting removed from the global table and
unimported from all live (L2)VNIs.
i.e.
The only time this update is actionable is if we are adding/removing an
IP from the martian TIP table, and we do not need to walk the table for
normal refcount adjustments.
Signed-off-by: Trey Aspelund <taspelund@nvidia.com>
We do use non-constant/literal format strings in a few places for more
or less valid reasons; put `ignored "-Wformat-nonliteral"` around those
so we can have the warning enabled for everywhere else.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
This fix updates the nexthop length of a bgp update to be
transmitted to a remote peer. Before the previous commit,
the ipv6 nexthop length was internally set to 32 bytes which
was not correct, as it should be 48 bytes which is conform
to the vpnv6 encoding format.
However, without the previous match, even if internally, the
nexthop length was set to 32, the real nexthop length was set
to 48 bytes, and everything was operating ok.
Now, if we use the following route-map, and attach it to
outgoing for vpnv6 address family, then we have a malformed
packet detected, and the peering breaks.
> route-map rmap permit 1
> set ipv6 next-hop global 5:5::3:6
> set ipv6 next-hop local fe80:55::333:222
Maintain the mp_nexthop_len to 48 bytes if it was already set
to 48 previously.
Fixes: 35ac9b53f2 ("bgpd: fix vpnv6 nexthop encoding")
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
For BGP evpn route table detail json to use
non pretty form of display.
Problem:
In scaled evpn route table detail json dump
occupies high resources (CPU + memory) of the system.
In high scale evpn route dump using pretty form
hogs CPU for a while which can trigger watchfrr
to kill bgpd.
Solution:
Avoid pretty JSON print for detail version dump
Signed-off-by: Chirag Shah <chirag@nvidia.com>
This change updates the nexthop attribute length
accordingly to the safi used. Actually, with the
previous commit, the length calculated was not
aligned with the real nexthop length. Such packet
received by remote peer was malformed, and this
was resulting in breaking vpnv6 peering.
Fix this by updating appropriately the real
nexthop length.
Fixes: 35ac9b53f2 ("bgpd: fix vpnv6 nexthop encoding")
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
