Donald Sharp [Thu, 8 Feb 2024 16:35:31 +0000 (11:35 -0500)]
zebra: Installation success should not set NHG as valid
The nexthop group is marked as valid/invalid and then
installed. Not installed and then marked valid.
This is just a bit of code removed that might be covering
up other problems that need to be sorted.
Donald Sharp [Thu, 8 Feb 2024 16:30:30 +0000 (11:30 -0500)]
zebra: Use switch when handling return from dplane for nhgs
Convert the dplane results function for nhg's over to
using a switch for the result enum. Let's specifically
call out the unexpected state and also set the nexthop
group as not installed when installation fails.
When installing a NHG via dplane_nexthop_add, it can only return
REQUEST_QUEUED or REQUEST_FAILURE. There is no way SUCCESS can
be returned with the way the dplane works at this point in time.
Remove the code that attempts to set the NHE state appropriately
as it is impossible.
Igor Ryzhov [Fri, 2 Feb 2024 23:15:46 +0000 (01:15 +0200)]
vtysh: remove resync workaround when exiting to config node
When exiting from a level below the config node, like `router rip`,
vtysh executes a resync by sending "end" and "conf term [file-lock]"
commands to all the daemons. As statet in the description comment, it's
done "in case one of the daemons is somewhere else". I don't think this
actually ever happens, but even if it is, it is a bug in a daemon that
needs to be fixed. This resync was okay before the introduction of
mgmtd, but now it unlocks and locks back the datastores during the
configuration reading process, which can lead to a failure which is
explained in the previous commit.
Igor Ryzhov [Fri, 2 Feb 2024 22:42:58 +0000 (00:42 +0200)]
mgmtd, vtysh: fix possible conflict when reading the config
When FRR starts, after mgmtd is initialized, backend clients connect to
it and request their config. To supply the config, mgmtd creates a
configuration transaction. At the same time, `vtysh -b` tries to read
the startup config and configure mgmtd, which also creates a
configuration transaction. If these two actions happen at the exact same
time, there's a conflict between them, because only a single
configuration translaction is allowed. Because of that, vtysh fails and
the config is completely ignored.
When starting the config reading, vtysh locks candidate and running
datastores in mgmtd. This commit adds locking of running datastore when
initializing the backend client. It allows to retry locking on the vtysh
side and read the config only when the lock is aquired instead of
failing.
This change also prevents running datastore from being changed during
initialization of backend clients. This could lead to a desynchronized
state between mgmtd and backends.
Louis Scalbert [Wed, 7 Feb 2024 12:16:26 +0000 (13:16 +0100)]
lib: fix affinity map duplicate
Fix duplicate definition of frr_affinity_map_cli_info in libfrr.so.0 and
libmgmt_be_nb.so.0
> =================================================================
> ==3860488==ERROR: AddressSanitizer: odr-violation (0x7f12c98c4d20):
> [1] size=296 'frr_affinity_map_cli_info' lib/affinitymap_cli.c:77:35
> [2] size=296 'frr_affinity_map_cli_info' lib/affinitymap_cli.c:77:35
> These globals were registered at these points:
> [1]:
> #0 0x7f12c9a36f40 in __asan_register_globals ../../../../src/libsanitizer/asan/asan_globals.cpp:341
> #1 0x7f12c9585b7d in _sub_I_00099_1 (/lib/libfrr.so.0+0x185b7d)
> #2 0x7f12ca437fe1 in call_init elf/dl-init.c:72
>
> [2]:
> #0 0x7f12c9a36f40 in __asan_register_globals ../../../../src/libsanitizer/asan/asan_globals.cpp:341
> #1 0x7f12c93824ed in _sub_I_00099_1 (/lib/libmgmt_be_nb.so.0+0x6f4ed)
> #2 0x7f12ca437fe1 in call_init elf/dl-init.c:72
>
> ==3860488==HINT: if you don't care about these errors you may set ASAN_OPTIONS=detect_odr_violation=0
> SUMMARY: AddressSanitizer: odr-violation: global 'frr_affinity_map_cli_info' at lib/affinitymap_cli.c:77:35
> ==3860488==ABORTING
Fixes: dc6ff4c0de ("lib: convert affinity-map to mgmtd") Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Donald Sharp [Sat, 3 Feb 2024 13:52:38 +0000 (08:52 -0500)]
doc: Add some documentation around a new pthread call
Not necessarily the correct place for this but there
is no other place and it needs to be called out and I
would rather have some documentation in place. Long
term I would like to add a bunch of frr_pthread documentation
but at this point in time it's not there. We can
re-arrange when that happens.
Donald Sharp [Mon, 5 Feb 2024 19:15:29 +0000 (14:15 -0500)]
zebra: Reorg `struct route_entry` to have important bits first
The `struct route_entry` had items that were almost never used at
the front of the data structure resulting in items that would be
loaded first into memory that were never used. Let's reorg a
tiny bit and put all the frequently used items in the first cache
line. I'm sure people will notice .000000001 speedup
There was a question in regards to how the update-source
choose the ip address for the source when using the `update-source`
command in BGP. Upon looking at the code, I was a but surprised,
so I decided to document this behavior.
Igor Ryzhov [Sat, 3 Feb 2024 20:27:49 +0000 (22:27 +0200)]
lib: add ietf-yang-metadata to the list of built-in modules
We don't need to manually load built-in modules. This fixes the
following warning in mgmtd:
```
YANG model "ietf-yang-metadata@*" "*@*"not embedded, trying external file
```
Igor Ryzhov [Sat, 3 Feb 2024 20:43:58 +0000 (22:43 +0200)]
mgmtd: disable lib code for config reading
mgmtd reads config files on its own, it doesn't need libfrr to do that.
The code is already skipped, because mgmtd uses `di->read_in` thread for
config reading and libfrr doesn't reschedule the thread, so this commit
just removes the dead code.
Igor Ryzhov [Sun, 4 Feb 2024 19:52:47 +0000 (21:52 +0200)]
lib, mgmtd: don't register NB config callbacks in mgmtd
mgmtd is supposed to only register CLI callbacks. If configuration
callbacks are registered, they are getting called on startup when mgmtd
reads config files, and they can use infrastructure that is not
initialized on mgmtd, or allocate some memory that is never freed.
Igor Ryzhov [Sun, 4 Feb 2024 19:52:47 +0000 (21:52 +0200)]
lib, mgmtd: don't register NB config callbacks in mgmtd
mgmtd is supposed to only register CLI callbacks. If configuration
callbacks are registered, they are getting called on startup when mgmtd
reads config files, and they can use infrastructure that is not
initialized on mgmtd, or allocate some memory that is never freed.
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.
projects / matthieu / frr.git / log