pimd: Fix for data packet loss when FHR is LHR and RP
Topology:
A single router is acting as the First Hop Router (FHR), Last Hop Router (LHR), and RP.
RC and Issue:
When an upstream S,G is in join state, it sends a register message to the RP.
If the RP has the receiver, it sends a register stop message and switches to the shortest path.
When the register stop message is processed, it removes pimreg, moves to prune,
and starts the reg stop timer.
When the reg stop timer expires, PIM changes S,G state to Join Pending and sends out a NULL
register message to RP. RP receives it and fails to send Reg stop because SPT is not set at that point.
The problem is when the register stop timer pops and state is in Join Pending.
According to https://www.rfc-editor.org/rfc/rfc4601#section-4.4.1,
we need to put back the pimreg reg tunnel into the S,G mroute.
This causes data to be sent to the control plane and subsequently interrupts the line rate.
Fix:
If the router is FHR and RP to the group,
ignore SPT status and send out a register stop message back to the DR (in this context, the same router).
Christian Hopps [Tue, 11 Feb 2025 07:12:06 +0000 (07:12 +0000)]
lib: nb: call child destroy CBs when YANG container is deleted
Previously the code was only calling the child destroy callbacks if the target
deleted node was a non-presence container. We now add a flag to the callback
structure to instruct northbound to perform the rescursive delete for code that
wishes for this to happen.
- Fix wrong relative path lookup in keychain destroy callback
isisd: Request SRv6 locator after zebra connection
When SRv6 is enabled and an SRv6 locator is specified in the IS-IS
configuration, IS-IS may attempt to request SRv6 locator information from
zebra before the connection is fully established. If this occurs, the
request fails with the following error:
Donald Sharp [Fri, 14 Feb 2025 12:55:09 +0000 (07:55 -0500)]
bgpd: When removing the prefix list drop the pointer
We are very very rarely seeing this crash:
0 0x7f36ba48e389 in prefix_list_apply_ext lib/plist.c:789
1 0x55eff3fa4126 in subgroup_announce_check bgpd/bgp_route.c:2334
2 0x55eff3fa858e in subgroup_process_announce_selected bgpd/bgp_route.c:3440
3 0x55eff4016488 in subgroup_announce_table bgpd/bgp_updgrp_adv.c:808
4 0x55eff401664e in subgroup_announce_route bgpd/bgp_updgrp_adv.c:861
5 0x55eff40111df in peer_af_announce_route bgpd/bgp_updgrp.c:2223
6 0x55eff3f884cb in bgp_announce_route_timer_expired bgpd/bgp_route.c:5892
7 0x7f36ba4ec239 in event_call lib/event.c:2019
8 0x7f36ba41a22a in frr_run lib/libfrr.c:1295
9 0x55eff3e668b7 in main bgpd/bgp_main.c:557
10 0x7f36b9e2d249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
11 0x7f36b9e2d304 in __libc_start_main_impl ../csu/libc-start.c:360
12 0x55eff3e64a30 in _start (/home/ci/cibuild.1407/frr-source/bgpd/.libs/bgpd+0x2fda30)
0x608000037038 is located 24 bytes inside of 88-byte region [0x608000037020,0x608000037078)
freed by thread T0 here:
0 0x7f36ba8b76a8 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52
1 0x7f36ba439bd7 in qfree lib/memory.c:131
2 0x7f36ba48d3a3 in prefix_list_free lib/plist.c:156
3 0x7f36ba48d3a3 in prefix_list_delete lib/plist.c:247
4 0x7f36ba48fbef in prefix_bgp_orf_remove_all lib/plist.c:1516
5 0x55eff3f679c4 in bgp_route_refresh_receive bgpd/bgp_packet.c:2841
6 0x55eff3f70bab in bgp_process_packet bgpd/bgp_packet.c:4069
7 0x7f36ba4ec239 in event_call lib/event.c:2019
8 0x7f36ba41a22a in frr_run lib/libfrr.c:1295
9 0x55eff3e668b7 in main bgpd/bgp_main.c:557
10 0x7f36b9e2d249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
previously allocated by thread T0 here:
0 0x7f36ba8b83b7 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:77
1 0x7f36ba4392e4 in qcalloc lib/memory.c:106
2 0x7f36ba48d0de in prefix_list_new lib/plist.c:150
3 0x7f36ba48d0de in prefix_list_insert lib/plist.c:186
4 0x7f36ba48d0de in prefix_list_get lib/plist.c:204
5 0x7f36ba48f9df in prefix_bgp_orf_set lib/plist.c:1479
6 0x55eff3f67ba6 in bgp_route_refresh_receive bgpd/bgp_packet.c:2920
7 0x55eff3f70bab in bgp_process_packet bgpd/bgp_packet.c:4069
8 0x7f36ba4ec239 in event_call lib/event.c:2019
9 0x7f36ba41a22a in frr_run lib/libfrr.c:1295
10 0x55eff3e668b7 in main bgpd/bgp_main.c:557
11 0x7f36b9e2d249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
Let's just stop trying to save the pointer around in the peer->orf_plist
data structure. There are other design problems but at least lets
stop the crash from possibly happening.
Louis Scalbert [Fri, 14 Feb 2025 10:58:24 +0000 (11:58 +0100)]
tests: check as number in show run
Creates the default VRF instance after the other VRF instances. The
default VRF instance is created in hidden state. Check that AS number
in show run is correctly written.
Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
'import vrf VRF' could define a hidden bgp instance with
the default AS_UNSPECIFIED (i.e. = 1) value.
When a
router bgp AS vrf VRF
gets configured later on, replace this AS_UNSPECIFIED setting
with a requested value.
Fixes: 9680831518 ("bgpd: fix as_pretty mem leaks when un-hiding") Signed-off-by: Alexander Skorichenko <askorichenko@netgate.com> Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Upon reconfiguration of the default instance, the prefixes are never set
into a meta queue by mq_add_handler(). They are never processed for
zebra RIB installation and announcements of update/withdraw.
Louis Scalbert [Wed, 12 Feb 2025 11:56:49 +0000 (12:56 +0100)]
bgpd: fix default instance name when un-hiding
When unconfiguring a default BGP instance with VPN SAFI configurations,
the default BGP structure remains but enters a hidden state. Upon
reconfiguration, the instance name incorrectly appears as "VIEW ?"
instead of "VRF default". And the name_pretty pointer
The name_pretty pointer is replaced by another one with the incorrect
name. This also leads to a memory leak as the previous pointer is not
properly freed.
Louis Scalbert [Wed, 12 Feb 2025 12:49:50 +0000 (13:49 +0100)]
bgpd: release manual vpn label on instance deletion
When a BGP instance with a manually assigned VPN label is deleted, the
label is not released from the Zebra label registry. As a result,
reapplying a configuration with the same manual label leads to VPN
prefix export failures.
Fixes: d162d5f6f5 ("bgpd: fix hardset l3vpn label available in mpls pool") Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
(cherry picked from commit d6363625c35a99933bf60c9cf0b79627b468c9f7)
tests: Extend SRv6 static SIDs topotest to verify SID structure
The `static_srv6_sids` topotest verifies that staticd correctly
programs the SIDs in the zebra RIB. Currently, the topotest only
validates the programmed behavior and SID attributes.
This commit extends the topotest to also validate the SID structure.
The `show ipv6 route json` command displays the IPv6 routing table in
JSON format, including SRv6 SIDs. For each SRv6 SID, it provides
behavior and SID attributes. However, it does not include the SID
structure.
This commit adds the SID structure to the SRv6 SID JSON output.
The seg6local route dumped by 'show ipv6 route' makes think that the USP
flavor is supported, whereas it is not the case. This information is a
context information, and for End, the context information should be
empty.
> # show ipv6 route
> [..]
> I>* fc00:0:4::/128 [115/0] is directly connected, sr0, seg6local End USP, weight 1, 00:49:01
Fix this by suppressing the USP information from the output.
Fixes: e496b4203055 ("bgpd: prefix-sid srv6 l3vpn service tlv") Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
(cherry picked from commit 658bf0281d99461849453628ddc792ec424d0bd4)
Donald Sharp [Mon, 10 Feb 2025 17:02:00 +0000 (12:02 -0500)]
bgpd: Fix crash in bgp_labelpool
The bgp labelpool code is grabbing the vpn policy data structure.
This vpn_policy has a pointer to the bgp data structure. If
a item placed on the bgp label pool workqueue happens to sit
there for the microsecond or so and the operator issues a
`no router bgp...` command that corresponds to the vpn_policy
bgp pointer, when the workqueue is run it will crash because
the bgp pointer is now freed and something else owns it.
Modify the labelpool code to store the vrf id associated
with the request on the workqueue. When you wake up
if the vrf id still has a bgp pointer allow the request
to continue, else drop it.
Louis Scalbert [Wed, 12 Feb 2025 11:50:42 +0000 (12:50 +0100)]
bgpd: fix incorrect json in bgp_show_table_rd
In bgp_show_table_rd(), the is_last argument is determined using the
expression "next == NULL" to check if the RD table is the last one. This
helps ensure proper JSON formatting.
However, if next is not NULL but is no longer associated with a BGP
table, the JSON output becomes malformed.
Updates the condition to also verify the existence of the next bgp_dest
table.
Fixes: 1ae44dfcba ("bgpd: unify 'show bgp' with RD with normal unicast bgp show") Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
(cherry picked from commit cf0269649cdd09b8d3f2dd8815caf6ecf9cdeef9)
Donald Sharp [Wed, 5 Feb 2025 13:42:00 +0000 (08:42 -0500)]
bfdd: Use pass by reference instead of pass by value for a struct
The function bfd_key_lookup is currently sending by value for
a now very large structure. Let's convert this over to pass
by reference. This is noticed by coverity.
When SRv6 is enabled and an SRv6 locator is specified in the BGP
configuration, BGP may attempt to request SRv6 locator information from
zebra before the connection is fully established. If this occurs, the
request fails with the following error:
Philippe Guibert [Mon, 10 Feb 2025 15:15:44 +0000 (16:15 +0100)]
nhrpd: fix dont consider incomplete L2 entry
Sometimes, NHRP receives L2 information on a cache entry with the
0.0.0.0 IP address. NHRP considers it as valid and updates the binding
with the new IP address.
> Feb 09 20:09:54 aws-sin-vpn01 nhrpd[2695]: [QQ0NK-1H449] Netlink: new-neigh 10.2.114.238 dev dmvpn1 lladdr 162.251.180.10 nud 0x2 cache used 0 type 4
> Feb 09 20:10:35 aws-sin-vpn01 nhrpd[2695]: [QQ0NK-1H449] Netlink: new-neigh 10.2.114.238 dev dmvpn1 lladdr 162.251.180.10 nud 0x4 cache used 1 type 4
> Feb 09 20:10:48 aws-sin-vpn01 nhrpd[2695]: [QQ0NK-1H449] Netlink: del-neigh 10.2.114.238 dev dmvpn1 lladdr 162.251.180.10 nud 0x4 cache used 1 type 4
> Feb 09 20:10:49 aws-sin-vpn01 nhrpd[2695]: [QQ0NK-1H449] Netlink: who-has 10.2.114.238 dev dmvpn1 lladdr (unspec) nud 0x1 cache used 1 type 4
> Feb 09 20:10:49 aws-sin-vpn01 nhrpd[2695]: [QVXNM-NVHEQ] Netlink: update binding for 10.2.114.238 dev dmvpn1 from c 162.251.180.10 peer.vc.nbma 162.251.180.10 to lladdr (unspec)
> Feb 09 20:10:49 aws-sin-vpn01 nhrpd[2695]: [QQ0NK-1H449] Netlink: new-neigh 10.2.114.238 dev dmvpn1 lladdr 0.0.0.0 nud 0x2 cache used 1 type 4
> Feb 09 20:11:30 aws-sin-vpn01 nhrpd[2695]: [QQ0NK-1H449] Netlink: new-neigh 10.2.114.238 dev dmvpn1 lladdr 0.0.0.0 nud 0x4 cache used 1 type 4
Actually, the 0.0.0.0 IP addressed mentiones in the 'who-has' message is
wrong because the nud state value means that value is incomplete and
should not be handled as a valid entry. Instead of considering it, fix
this by by invalidating the current binding. This step is necessary in
order to permit NHRP to trigger resolution requests again.
David Lamparter [Fri, 7 Feb 2025 12:22:25 +0000 (13:22 +0100)]
lib: crash handlers must be allowed on threads
Blocking all signals on non-main threads is not the way to go, at least
the handlers for SIGSEGV, SIGBUS, SIGILL, SIGABRT and SIGFPE need to run
so we get backtraces. Otherwise the process just exits.
Chirag Shah [Tue, 11 Feb 2025 02:56:15 +0000 (18:56 -0800)]
bgpd: fix bgp vrf instance creation from implicit
In bgp route leak, when import vrf x is executed,
it creates bgp instance as hidden with asn value as unspecified.
When router bgp x is configured ensure the correct as,
asnotation is applied otherwise running config shows asn value as 0.
This can lead to frr-reload failure when any FRR config change.
Fix:
Move asn and asnotiation, as_pretty value in common done section,
so when bgp_create gets existing instance but before returning
update asn and required fields in common section.
In bgp_create(): when returning for hidden at least update asn
and required when bgp instance created implicitly due to vrf leak.
if (hidden) {
bgp = bgp_old;
goto peer_init; <<<
}
David Lamparter [Wed, 22 Jan 2025 10:19:04 +0000 (11:19 +0100)]
lib: guard against padding garbage in ZAPI read
When reading in a nexthop from ZAPI, only set the fields that actually
have meaning. While it shouldn't happen to begin with, we can otherwise
carry padding garbage into the unused leftover union bytes.
David Lamparter [Wed, 22 Jan 2025 10:17:21 +0000 (11:17 +0100)]
zebra: guard against junk in nexthop->rmap_src
rmap_src wasn't initialized, so for IPv4 the unused 12 bytes would
contain whatever junk is on the stack on function entry. Also move
the IPv4 parse before the IPv6 parse so if it's successful we can be
sure the other bytes haven't been touched.
David Lamparter [Wed, 22 Jan 2025 10:13:21 +0000 (11:13 +0100)]
bgpd: don't reuse nexthop variable in loop/switch
While the loop is currently exited in all cases after using nexthop, it
is a footgun to have "nh" around to be reused in another iteration of
the loop. This would leave nexthop with partial data from the previous
use. Make it local where needed instead.
Issue:
When there is no traffic for a group, the LHR and RP take the default KAT+Join timer expiry of
a maximum of 480 seconds to clear the S,G . However, in the FHR, we update the state from JOINED
to NOT Joined, downstream state from PPto NOINFO. This restarts the ET timer, causing S,G on FHR to
take more than 10 minutes to age out.
In other words,
Consider a case where (S,G) is in Join state. When the traffic stops and the KAT (210) expires,
the Join expiry timer restarts. At this time, if we receive a prune, the expectation is to set
PPT to 0 (RFC 4601 sec 4.5.2).
When the PPT expires, we move to the noinfo state and restart the expiry timer one more time. We remove the
(S,G) entry only after ~10 minutes when there is no active traffic.
Summary:
KAT Join ET 210 + PP ET 210 + NOINFO ET 210.
Solution:
Delete the ifchannel when in noinfo state, and KAT is not running.
Donald Sharp [Thu, 16 Jan 2025 16:17:11 +0000 (11:17 -0500)]
zebra: Ensure that changes to dg_update_list are protected by mutex
The dg_update_list access is controlled by the dg_mutex in all
other locations. Let's just add a mutex usage around the initialization
of the dg_update_list even if it's part of the startup, just to keep
things consistent.
msg_new takes a uint16_t, the length passed
down variable is a unsigned int, thus 32 bit.
It's possible, but highly unlikely, that the
msglen could be greater than 16 bit.
Let's just add some checks to ensure that
this could not happen.
Donald Sharp [Tue, 4 Feb 2025 15:56:59 +0000 (10:56 -0500)]
bgpd: Fix up memory leak in processing eoiu marker
Memory is being leaked when processing the eoiu marker.
BGP is creating a dummy dest to contain the data but
it was never freed. As well as the eoiu info was
not being freed either.
Chirag Shah [Mon, 3 Feb 2025 20:00:41 +0000 (12:00 -0800)]
bgpd: fix route-distinguisher in vrf leak json cmd
For auto configured value RD value comes as NULL,
switching back to original change will ensure to cover
for both auto and user configured RD value in JSON.
Chirag Shah [Fri, 31 Jan 2025 01:26:46 +0000 (17:26 -0800)]
zebra: evpn svd hash avoid double free
Upon zebra shutdown hash_clean_and_free is called
where user free function is passed,
The free function should not call hash_release
which lead to double free of hash bucket.
Fix:
The fix is to avoid calling hash_release from
free function if its called from hash_clean_and_free
path.
10 0x00007f0422b7df1f in free () from /lib/x86_64-linux-gnu/libc.so.6
11 0x00007f0422edd779 in qfree (mt=0x7f0423047ca0 <MTYPE_HASH_BUCKET>,
ptr=0x55fc8bc81980) at ../lib/memory.c:130
12 0x00007f0422eb97e2 in hash_clean (hash=0x55fc8b979a60,
free_func=0x55fc8a529478 <svd_nh_del_terminate>) at
../lib/hash.c:290
13 0x00007f0422eb98a1 in hash_clean_and_free (hash=0x55fc8a675920
<svd_nh_table>, free_func=0x55fc8a529478 <svd_nh_del_terminate>) at
../lib/hash.c:305
14 0x000055fc8a5323a5 in zebra_vxlan_terminate () at
../zebra/zebra_vxlan.c:6099
15 0x000055fc8a4c9227 in zebra_router_terminate () at
../zebra/zebra_router.c:276
16 0x000055fc8a4413b3 in zebra_finalize (dummy=0x7fffb881c1d0) at
../zebra/main.c:269
17 0x00007f0422f44387 in event_call (thread=0x7fffb881c1d0) at
../lib/event.c:2011
18 0x00007f0422ecb6fa in frr_run (master=0x55fc8b733cb0) at
../lib/libfrr.c:1243
19 0x000055fc8a441987 in main (argc=14, argv=0x7fffb881c4a8) at
../zebra/main.c:584
When a user wants to delete a specific SRv6 SID, he executes the
`no sid X:X::X:X/M` command.
However, by mistake, in addition to deleting the SID requested by the
user, this command also removes all other SIDs.
This happens because `no sid X:X::X:X/M` triggers a destroy operation
on the wrong xpath `frr-staticd:staticd/segment-routing/srv6`.
This commit fixes the issue by replacing the wrong xpath
`frr-staticd:staticd/segment-routing/srv6` with the correct xpath
`frr-staticd:staticd/segment-routing/srv6/static-sids/sid[sid='%s']`.
This ensures that the `no sid X:X::X:X/M` command only deletes the SID
that was requested by the user.
When staticd receives a `ZAPI_SRV6_SID_RELEASED` notification from SRv6
SID Manager, it tries to unset the validity flag of `sid`. But since
the `sid` variable is NULL, we get a NULL pointer dereference.
```
=================================================================
==13815==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000060 (pc 0xc14b813d9eac bp 0xffffcb135a40 sp 0xffffcb135a40 T0)
==13815==The signal is caused by a READ memory access.
==13815==Hint: address points to the zero page.
#0 0xc14b813d9eac in static_zebra_srv6_sid_notify staticd/static_zebra.c:1172
#1 0xe44e7aa2c194 in zclient_read lib/zclient.c:4746
#2 0xe44e7a9b69d8 in event_call lib/event.c:1984
#3 0xe44e7a85ac28 in frr_run lib/libfrr.c:1246
#4 0xc14b813ccf98 in main staticd/static_main.c:193
#5 0xe44e7a4773f8 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#6 0xe44e7a4774c8 in __libc_start_main_impl ../csu/libc-start.c:392
#7 0xc14b813cc92c in _start (/usr/lib/frr/staticd+0x1c92c)
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV staticd/static_zebra.c:1172 in static_zebra_srv6_sid_notify
==13815==ABORTING
```
This commit fixes the problem by doing a SID lookup first. If the SID
can't be found, we log an error and return. If the SID is found, we go
ahead and unset the validity flag.
Donald Sharp [Fri, 31 Jan 2025 23:53:30 +0000 (18:53 -0500)]
bgpd: With suppress-fib-pending ensure withdrawal is sent
When you have suppress-fib-pending turned on it is possible
to end up in a situation where the prefix is not withdrawn
from downstream peers.
Here is the timing that I believe is happening:
a) have 2 paths to a peer.
b) receive a withdrawal from 1 path, set BGP_NODE_FIB_INSTALL_PENDING
and send the route install to zebra.
c) receive a withdrawal from the other path.
d) At this point we have a dest->flags set BGP_NODE_FIB_INSTALL_PENDING
old_select the path_info going away, new_select is NULL
e) A bit further down we call group_announce_route() which calls
the code to see if we should advertise the path. It sees the
BGP_NODE_FIB_INSTALL_PENDING flag and says, nope.
f) the route is sent to zebra to withdraw, which unsets the
BGP_NODE_FIB_INSTALL_PENDING.
g) This function winds up and deletes the path_info. Dest now
has no path infos.
h) BGP receives the route install(from step b) and unsets the
BGP_NODE_FIB_INSTALL_PENDING flag
i) BGP receives the route removed from zebra (from step f) and
unsets the flag again.
We know if there is no new_select, let's go ahead and just
unset the PENDING flag to allow the withdrawal to go out
at the time when the second withdrawal is received.
Mark Stapp [Fri, 31 Jan 2025 18:13:48 +0000 (13:13 -0500)]
libs: return from change_caps if no caps
When called without caps/privs, just return from "change_caps"
instead of exiting - it's possible that a process may not need
privs, but a lib (for example) may use the api.
Donald Sharp [Fri, 31 Jan 2025 17:38:20 +0000 (12:38 -0500)]
zebra: Ensure dplane does not send work back to master at wrong time
When looping through the dplane providers, the worklist was
being populated with items from the last provider and then
the event system was checked to see if we should stop processing.
If the event system says `yes` then the dplane code would stop
and send the worklist to the master zebra pthread for collection.
This obviously skipped the next dplane provider on the list
which is double plus not good.
projects / matthieu / frr.git / log