Donald Sharp [Sun, 4 Feb 2024 03:35:31 +0000 (22:35 -0500)]
bgpd: Prevent rpki from hooking multiple times into rcu code
As far as I can tell, the rpki code creates a pthread that
is used to handle the i/o associated with talking to the
remote rpki server. The problem that we are having is that
the rpki code in FRR wants to behave like FRR code and use
the zlog_XXX functions. These functions all depend on
the RCU code. Which is a bit picky( and rightly so!!! )
about being started up properly and shut down properly.
This commit is fixing the problem of shutdown. From
playing with the rpki code, I was able to experimentally
determine that the rpki_create_socket callback function
can be called multiple times per pthread. Additionally
I was able to clearly see multiple *different* pthreads
actually be created. This leaves the possiblity
that each time it is called it might be hooking into the
RCU code. Which makes the rcu code unhappy on shutdown.
Let's address the issue by checking to see if this pthread
has already hooked into the RCU code or not. If so
then don't do this again.
Donald Sharp [Sun, 4 Feb 2024 02:34:02 +0000 (21:34 -0500)]
lib: Actually create the rcu and save it before using it
In a non-controlled startup, the rcu data structures were
not being created until after logging could happen. This
is bad. Move it so that the rcu data structures are
created first, before logging( HA! ) can happen.
cisco routers are not dealing fairly whith unsupported capabilities.
When a cisco router receive an unsupported capabilities it reset the
negociation without notifying the unmatching capability as described in
RFC2842.
Cisco suggest the use of
neighbor x.x.x.x capability fqdn
to avoid the use of fqdn in open message.
this new command is to remove the use of fqdn capability in the
open message with the peer "x.x.x.x".
Louis Scalbert [Fri, 2 Feb 2024 09:35:10 +0000 (10:35 +0100)]
bgpd: fix logging from rpki_create_socket()
Fix the following crash when logging from rpki_create_socket():
> #0 raise (sig=<optimized out>) at ../sysdeps/unix/sysv/linux/raise.c:50
> #1 0x00007f6e21723798 in core_handler (signo=6, siginfo=0x7f6e1e502ef0, context=0x7f6e1e502dc0) at lib/sigevent.c:248
> #2 <signal handler called>
> #3 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
> #4 0x00007f6e2144e537 in __GI_abort () at abort.c:79
> #5 0x00007f6e2176348e in _zlog_assert_failed (xref=0x7f6e2180c920 <_xref.16>, extra=0x0) at lib/zlog.c:670
> #6 0x00007f6e216b1eda in rcu_read_lock () at lib/frrcu.c:294
> #7 0x00007f6e21762da8 in vzlog_notls (xref=0x0, prio=2, fmt=0x7f6e217afe50 "%s:%d: %s(): assertion (%s) failed", ap=0x7f6e1e504248) at lib/zlog.c:425
> #8 0x00007f6e217632fb in vzlogx (xref=0x0, prio=2, fmt=0x7f6e217afe50 "%s:%d: %s(): assertion (%s) failed", ap=0x7f6e1e504248) at lib/zlog.c:627
> #9 0x00007f6e217621f5 in zlog (prio=2, fmt=0x7f6e217afe50 "%s:%d: %s(): assertion (%s) failed") at lib/zlog.h:73
> #10 0x00007f6e21763596 in _zlog_assert_failed (xref=0x7f6e2180c920 <_xref.16>, extra=0x0) at lib/zlog.c:687
> #11 0x00007f6e216b1eda in rcu_read_lock () at lib/frrcu.c:294
> #12 0x00007f6e21762da8 in vzlog_notls (xref=0x7f6e21a50040 <_xref.68>, prio=4, fmt=0x7f6e21a4999f "getaddrinfo: debug", ap=0x7f6e1e504878) at lib/zlog.c:425
> #13 0x00007f6e217632fb in vzlogx (xref=0x7f6e21a50040 <_xref.68>, prio=4, fmt=0x7f6e21a4999f "getaddrinfo: debug", ap=0x7f6e1e504878) at lib/zlog.c:627
> #14 0x00007f6e21a3f774 in zlog_ref (xref=0x7f6e21a50040 <_xref.68>, fmt=0x7f6e21a4999f "getaddrinfo: debug") at ./lib/zlog.h:84
> #15 0x00007f6e21a451b2 in rpki_create_socket (_cache=0x55729149cc30) at bgpd/bgp_rpki.c:1337
> #16 0x00007f6e2120e7b7 in tr_tcp_open (tr_socket=0x5572914d1520) at rtrlib/rtrlib/transport/tcp/tcp_transport.c:111
> #17 0x00007f6e2120e212 in tr_open (socket=0x5572914b5e00) at rtrlib/rtrlib/transport/transport.c:16
> #18 0x00007f6e2120faa2 in rtr_fsm_start (rtr_socket=0x557290e17180) at rtrlib/rtrlib/rtr/rtr.c:130
> #19 0x00007f6e218b7ea7 in start_thread (arg=<optimized out>) at pthread_create.c:477
> #20 0x00007f6e21527a2f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
rpki_create_socket() is a hook function called from the rtrlib library.
The issue arises because rtrlib initiates its own separate pthread in which
it runs the hook, which does not establish an FRR RCU context. Consequently,
this leads to failures in the logging mechanism that relies on RCU.
Initialize a new FRR pthread context from the rtrlib pthread with a
valid RCU context to allow logging from the rpki_create_socket() and
dependent functions.
Louis Scalbert [Fri, 2 Feb 2024 09:25:05 +0000 (10:25 +0100)]
lib: add ability to log from external pthread
External libraries can re-enter the FRR code through a hook function. A
crash occurs when logging from this hook function if the library has
initiated a new pthread, as the FRR RCU context is not initialized for
this thread.
Add frr_pthread_non_controlled_startup() function to initialize a valid
RCU context within a FRR pthread context, originating from an external
pthread.
Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Igor Ryzhov [Thu, 1 Feb 2024 22:18:30 +0000 (00:18 +0200)]
lib: fix "no vrf" command
Remove operational data check from CLI command. It never works in mgmtd
and it is not needed in backend daemons because it's done in
`lib_vrf_destroy` callback.
Igor Ryzhov [Thu, 1 Feb 2024 22:57:59 +0000 (00:57 +0200)]
staticd: fix NB dependency hack
Currently, staticd configuration is tightly coupled with VRF existence.
Because of that, it has to use a hack in NB infrastructure to create a
VRF configuration when at least one static route is configured for this
VRF. This hack is incompatible with mgmtd, because mgmtd doesn't execute
configuration callbacks. Because of that, the configuration may become
out of sync between mgmtd and staticd. There are two main cases:
1. Create static route in a VRF. The VRF data node will be created
automatically in staticd by the NB hack, but not in mgmtd.
2. Delete VRF which has some static routes configured. The static route
configuration will be deleted from staticd by the NB hack, but not
from mgmtd.
To fix the problem, decouple configuration of static routes from VRF
configuration. Now it is possible to configure static routes even if the
VRF doesn't exist yet. Once the VRF is created, staticd applies all the
preconfigured routes.
This change also fixes the problem with static routes being preserved in
the system when staticd "control-plane-protocol" container is deleted
but the VRF is still configured.
Igor Ryzhov [Wed, 31 Jan 2024 12:10:09 +0000 (14:10 +0200)]
staticd: fix nexthops without interface
When interface is not set in "ip route" command, CLI passes "(null)" as
an interface name instead of an empty string. The actual code in turn
uses "nh->ifname[0] != 0" to check if the interface name was set. Fix
the problem by changing the "(null)" string into an empty string when
populating the nexthop structure.
This change is wrong for several reasons:
- it is backwards incompatible - previously it was always possible to
create blackhole/reject routes using shortened versions of the words
and it suddenly became impossible if there's an interface in the
system with the same name
- it uses operational data for validation which is prohibited
- it doesn't really solve the problem with inability to create routes
using interface names like `bla` or `rej`
Mark Stapp [Thu, 1 Feb 2024 16:40:02 +0000 (11:40 -0500)]
*: create a single registry of daemons' default port values
Create a single registry of default port values that daemons
are using. Most of these are vty ports, but there are some
others for features like ospfapi and zebra FPM.
Donald Sharp [Wed, 31 Jan 2024 18:37:25 +0000 (13:37 -0500)]
bgpd: Save memory when using bgp_path_info_extra and vnc
Structure size of bgp_path_info_extra when compiled
with vnc is 184 bytes. Reduce this size to 72 bytes
when compiled w/ vnc but not necessarily turned
on vnc.
With 2 full bgp feeds this saves aproximately 100mb
when compiling with vnc and not using vnc.
Igor Ryzhov [Sun, 28 Jan 2024 23:05:14 +0000 (01:05 +0200)]
mgmtd: add our own format definitions for frontend messages
It allows people not familiar with libyang and FRR internals to use
mgmtd FE API by looking only at `mgmt_msg_native.h` header. We still use
the same values to avoid a lot of mapping code, and ensure that any
change doesn't slip unnoticed by using static asserts.
Igor Ryzhov [Sun, 28 Jan 2024 00:35:14 +0000 (02:35 +0200)]
mgmtd: fix printing an empty data tree
We don't need to create an actual tree to print an empty tree, libyang
handles NULL just fine. The actual problem is that `yang_dnode_new`
creates a tree by validating it, and the validation creates all implicit
default nodes. Therefore, when called with "with-default" flags, instead
of getting an empty tree, we get a tree with all top-level default set.
Louis Scalbert [Tue, 30 Jan 2024 14:18:43 +0000 (15:18 +0100)]
bgpd: fix attr comparaison bgp_adj_in_set
In bgp_adj_in_set(), attr has not yet been interned. adj->attr is always
different from attr. adj->attr is always uninterned and interned even if
attr and adj->attr are identical.
Fix the comparison.
Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Donatas Abraitis [Tue, 30 Jan 2024 13:44:38 +0000 (15:44 +0200)]
bgpd: Reinstall aggregated routes if using route-maps and it was changed
Without this change when we change the route-map, we never reinstall the route
if the route-map has changed.
We checked only some attributes like aspath, communities, large-communities,
extended-communities, but ignoring the rest of attributes.
With this change, let's check if the route-map has changed.
bgp_route_map_process_update() is triggered on route-map change, and we set
`changed` to true, which treats aggregated route as not the same as it was before.
Mark Stapp [Thu, 9 Nov 2023 15:26:06 +0000 (10:26 -0500)]
tools: add config-write callbacks to nb templates
Add the cli_show (config write) callback when emitting the
create or modify callback in the northbound template. Split
the config-handling and config-output callbacks into two
structs/arrays; this seems to be helpful when doing mgmtd
conversion.
The previous commit has added a routing leak update when a nexthop
update is received from zebra. It indirectly calls
bgp_find_or_add_nexthop() in which a static route triggers a nexthop
cache entry registration that triggers a nexthop update from zebra.
Do not register again the nexthop cache entry if the BGP_STATIC_ROUTE is
already set.
Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Louis Scalbert [Fri, 29 Apr 2022 12:26:04 +0000 (14:26 +0200)]
bgpd: fix VRF leaking with 'network import-check' (3/4)
If 'bgp network import-check' is defined on the source BGP session,
prefixes that are defined with the network command cannot be leaked to
the other VRFs BGP table even if they are present in the origin VRF RIB
if the 'rt import' statement is defined after the 'network <prefix>'
ones.
When a prefix nexthop is updated, update the prefix route leaking. The
current state of nexthop validation is now stored in the attributes of
the bgp path info. Attributes are compared with the previous ones at
route leaking update so that a nexthop validation change now triggers
the update of destination VRF BGP table.
Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Louis Scalbert [Thu, 28 Apr 2022 16:32:20 +0000 (18:32 +0200)]
bgpd: fix VRF leaking with 'network import-check' (1/4)
If 'bgp network import-check' is defined on the source BGP session,
prefixes that are defined with the network command cannot be leaked to
the other VRFs BGP table even if they are present in the origin VRF RIB.
Always validate the nexthop of BGP static routes (i.e. defined with the
network statement) if 'network import-check' is defined on the source
BGP session and the prefix is present in source RIB.
Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Igor Ryzhov [Wed, 24 Jan 2024 03:11:07 +0000 (05:11 +0200)]
zebra, build: disable irdp by default
IRDP client (rdisc) was deleted from iputils more than 2 years ago. It's
time to drop IRDP, but first let's stop building and including it in the
packages by default to see if anyone will be complaining.
Igor Ryzhov [Sun, 28 Jan 2024 21:23:57 +0000 (23:23 +0200)]
lib, mgmtd, zebra: cleanup of zebra conversion to mgmtd
- use `apply_finish` callback when possible to avoid multiple applies per commit
- move table range working to the CLI handler
- remove unnecessary conditional compilation
- remove unnecessary boolean conversion
Igor Ryzhov [Sat, 27 Jan 2024 04:20:22 +0000 (06:20 +0200)]
build: remove mgmtd exception from xref2vtysh
We may actually need to send CLI commands to mgmtd and another daemon at
the same time, for example, if this daemon is not mgmtd-converted. The
only daemon this exception protects is staticd. But we don't actually
need any configuration commands in staticd, so just remove the exception
and don't install unnecessary commands to staticd.
Igor Ryzhov [Wed, 24 Jan 2024 20:26:47 +0000 (22:26 +0200)]
lib: fix yang_dnode_dup
When duplicating nodes, we should always keep flags, especially the
LYD_NEW flag that indicates not validated data. This allows to select a
new choice's case without the need to explicitly remove the existing one.
Igor Ryzhov [Sat, 27 Jan 2024 11:11:08 +0000 (13:11 +0200)]
zebra: make vrf netns commands do nothing
These commands don't really provide any functionality. VRF is associated
with netns automatically based on its name, and it's not possible to
associate VRF and netns with different names with these commands:
- When trying to assosiate a VRF with an already existing netns with a
different name:
`NS /run/netns/test is already configured with VRF 1(test)`
- When trying to assiciate a VRF with a non-existing netns, so they
become linked once the netns is created:
`Invalid pathname for /run/netns/test: No such file or directory`
- When doing "no netns" to unlink the netns and link it back to the same
VRF:
`VRF 1 is already configured with VRF test`
- When doing "no netns" to unlink the netns and link it to another VRF:
`Can not associate NS 4294967295 with NETNS /run/netns/test`
As shown above, not a single usecase is working. We can't remove them
completely to preserve backwards-compatibility, so just make them empty.
The main reason for this change is not to spend a lot of time trying to
figure out how to convert them to northbound.
projects / matthieu / frr.git / log