Add parameters to the "show bfd peers json" command to
display interface and type of BFD session.
Signed-off-by: Lynne Morrison <lynne.morrison@ibm.com>
With this configuration:
```
bfd
peer 33:33::66 local-address 33:33::88 vrf vrf8 interface enp1s0
exit
!
exit
```
The bfd session can't be established with error:
```
bfdd[18663]: [YA0Q5-C0BPV] control-packet: wrong vrfid. [mhop:no peer:33:33::66 local:33:33::88 port:2 vrf:61]
```
The vrf check should use the carefully adjusted `vrfid`, which is
based on globally/reliable interface. We can't believe the
`bvrf->vrf->vrf_id` because the `/proc/sys/net/ipv4/udp_l3mdev_accept`
maybe is set "1" in VRF-lite backend even with security drawback.
Just correct the vrf check.
Signed-off-by: anlan_cs <vic.lan@pica8.com>
Effectively a massive search and replace of
`struct thread` to `struct event`. Using the
term `thread` gives people the thought that
this event system is a pthread when it is not
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
This is a first in a series of commits, whose goal is to rename
the thread system in FRR to an event system. There is a continual
problem where people are confusing `struct thread` with a true
pthread. In reality, our entire thread.c is an event system.
In this commit rename the thread.[ch] files to event.[ch].
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
The check with `ip->daddr == ip->saddr` in `bfd_recv_ipv4_fp()` is
useless, instead of it the ECHO packets should simply exit with
TTL checking failure regardless of this condition check.
Just remove the check.
Signed-off-by: anlan_cs <vic.lan@pica8.com>
The files converted in this commit either had some random misspelling or
formatting weirdness that made them escape automated replacement, or
have a particularly "weird" licensing setup (e.g. dual-licensed.)
This also marks a bunch of "public domain" files as SPDX License "NONE".
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
The imported BFD code had some logic to ignore the source address when
using single hop IPv4. The BFD peer socket function should allow the
source to be selected so we can:
1. Select the source address in the outgoing packets
2. Only receive packets from that specific source
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
Rather than running selected source files through the preprocessor and a
bunch of perl regex'ing to get the list of all DEFUNs, use the data
collected in frr.xref.
This not only eliminates issues we've been having with preprocessor
failures due to nonexistent header files, but is also much faster.
Where extract.pl would take 5s, this now finishes in 0.2s. And since
this is a non-parallelizable build step towards the end of the build
(dependent on a lot of other things being done already), the speedup is
actually noticeable.
Also files containing CLI no longer need to be listed in `vtysh_scan`
since the .xref data covers everything. `#ifndef VTYSH_EXTRACT_PL`
checks are equally obsolete.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
There are lib debugs being set but never show up in
`show debug` commands because there was no way to show
that they were being used. Add a bit of infrastructure
to allow this and then use it for `debug route-map`
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
When an interface does not have a mac address, don't
try to retrieve the mac address ( for it to just fail ).
Example interface:
sharpd@eva [2]> ip link show tun100
21: tun100@NONE: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1480 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
link/ipip 192.168.119.224 peer 192.168.119.120
Let's just notice that there is a NOARP flag and abort the call.
Fixes: #11733
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Add a send time into the BFD Echo packet. When the BFD Echo
packet is received back store time it took in usec. When
user issues a show bfd peer(s) command calculate and display
minimum, average, and max time it took for the BFD Echo packet
to be looped back.
Signed-off-by: Lynne Morrison <lynne.morrison@ibm.com>
Until now, when in vrf-lite mode, the BFD implementation
creates a single UDP socket and relies on the following
sysctl value to 1:
echo 1 > /proc/sys/net/ipv4/udp_l3mdev_accept
With this setting, the incoming BFD packets from a given
vrf, would leak to the default vrf, and would match the
UDP socket.
The drawback of this solution is that udp packets received
on a given vrf may leak to an other vrf. This may be a
security concern.
The commit addresses this issue by avoiding this leak
mechanism. An UDP socket is created for each vrf, and each
socket uses new setsockopt option: SO_REUSEADDR + SO_REUSEPORT.
With this option, the incoming UDP packets are distributed on
the available sockets. The impact of those options with l3mdev
devices is unknown. It has been observed that this option is not
needed, until the default vrf sockets are created.
To ensure the BFD packets are correctly routed to the appropriate
socket, a BPF filter has been put in place and attached to the
sockets : SO_ATTACH_REUSEPORT_CBPF. This option adds a criterium
to force the packet to choose a given socket. If initial criteria
from the default distribution algorithm were not good, at least
two sockets would be available, and the CBPF would force the
selection to the same socket. This would come to the situation
where an incoming packet would be processed on a different vrf.
The bpf code is the following one:
struct sock_filter code[] = {
{ BPF_RET | BPF_K, 0, 0, 0 },
};
struct sock_fprog p = {
.len = sizeof(code)/sizeof(struct sock_filter),
.filter = code,
};
if (setsockopt(sd, SOL_SOCKET, SO_ATTACH_REUSEPORT_CBPF, &p, sizeof(p))) {
zlog_warn("unable to set SO_ATTACH_REUSEPORT_CBPF on socket: %s",
strerror(errno));
return -1;
}
Some tests have been done with by creating vrf contexts, and by using
the below vtysh configuration:
ip route 2.2.2.2/32 10.126.0.2
vrf vrf2
ip route 2.2.2.2/32 10.126.0.2
!
interface ntfp2
ip address 10.126.0.1/24
!
interface ntfp3 vrf vrf4
ip address 10.126.0.1/24
!
interface ntfp2 vrf vrf1
ip address 10.126.0.1/24
!
interface ntfp2.100 vrf vrf2
ip address 10.126.0.1/24
!
interface ntfp2.200 vrf vrf3
ip address 10.126.0.1/24
!
line vty
!
bfd
peer 10.126.0.2 vrf vrf2
!
peer 10.126.0.2 vrf vrf3
!
peer 10.126.0.2
!
peer 10.126.0.2 vrf vrf4
!
peer 2.2.2.2 multihop local-address 1.1.1.1
!
peer 2.2.2.2 multihop local-address 1.1.1.1 vrf vrf2
transmit-interval 1500
receive-interval 1500
!
The results showed no issue related to packets received by
the wrong vrf. Even changing the udp_l3mdev_accept flag to
1 did not change the test results.
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
Use the destination for the operator `sizeof()` instead of the source
which could (and is) be bigger than destination.
We are not truncating any data here it just happens that the zebra
interface data structure hardware address can be bigger due to different
types of interface.
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
Modify the existing BFD Echo code to send an Echo message that will
be looped in the peers forwarding plane. The existing Echo code
only works with other FRR implementations because the Echo packet
must go up to BFD to be turned around and forwarded back to the
local router. The new BFD Echo code sets the src/dst IP of the
packet to be the local router's IP and sets the dest MAC to be the
peers MAC address. The peer receives the packet and because it
is not it's IP address it forwards it back to the local router.
Signed-off-by: Lynne Morrison <lynne.morrison@ibm.com>
Coverity is claiming that bfdd is able got have bglobal.bg_use_dplane
can be true, while dplane_addr can be uninitialized. Not really
possible since global variables are initialized to all 0's. In
any event. Force it to think it can't go there.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
After two single-hop sessions (*no local address are configured*) on two
interfaces are UP, remove one address of one interface, both of them
(actually, quite independent sessions) come to be DOWN, not just one.
Consider two boxes: A with `a1` and `a2` adddress on two interfaces,
and B with `b1` and `b2`.
Two sessions are set up and ok: `s1` with <a1,b1> and `s2` with <a2,b2>.
After `a1` of A is removed, there is an unhappy coincidence:
1) On A: `s1` changes local address, and sends <a2,b1> packets with help
of route.
2) On B: wrongly regarded <a2,b1> packets with non-zero remote descriminator
as part of `s2`, and are dropped for mismatched remote remote descriminator.
3) On A: `s1` sends <a2,b1> packets with zero remote descriminator to
initialize this session.
4) On B: wrongly regarded <a2,b1> packets with zero remote descriminator as
part of `s2`. Then `s2` will vibrate.
So the good sessions are overridden.
In this case, the <a2,b1> packets with zero remote descriminator won't take
effect until the current good sessions become bad.
Since single-hop sessions are allowed to be set without bound inteface in
current code, this commit adds one check in `bfd_recv_cb()` to avoid wrong
override.
Signed-off-by: anlan_cs <vic.lan@pica8.com>
Don't rely on the OS interface name length definition and use the FRR
definition instead.
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
The "local_address" of bfd is only used in `show bfd peers brief`
for single hop sessions which are configured without "local address".
Since it is set by destination address of received packet, not
completely correct, so remove it.
Signed-off-by: ewlumpkin <ewlumpkin@gmail.com>
Signed-off-by: anlan_cs <vic.lan@pica8.com>
With the simple BFD configuration -
(active mode, single hop, without other parameters)
```
!
bfd
peer 11.11.11.11
exit
!
```
The interface with 11.11.11.0/24 is a *virtual* interface,
which can be deleted.
After BFD FSM is created and session is ok, do these things:
1) delete this interface
2) create this interface
3) set same ip address in this interface
Now, everything seems completely restored because all configuration
is same. But bad thing happens, BFD session hang on "down" status -
```
root# show bfd peer 11.11.11.11
BFD Peer:
peer 11.11.11.11 vrf default
ID: 638815827
Remote ID: 0
Active mode
Status: down
Downtime: 3 second(s)
Diagnostics: path down <- caused by destroyed interface
Remote diagnostics: ok
```
With the interface creating, `bfdd_sessions_enable_interface()`
wrongly compares added interface with the created, even key of
this `bfd_session` isn't binded with any interface. So this
`bfd_session` will hang on "down" status for ever.
So skip the compare in this case (no interface in key) to wake up
this `bfd_session`.
Signed-off-by: anlan_cs <vic.lan@pica8.com>
If time ( a uint64_t ) is large enough doing division
and subtraction can still lead to situations where
the resulting number is greater than a uint32_t.
Just use uint32_t as an intermediate storage spot.
This is unlikely to every occur in a time frame
I could possibly care about but makes Coverity happy.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Problem:
One is with active mode, the other is with passive mode. Sometimes
the one with active mode is in `Down` stauts, but the other one
with passive mode is unluckily stuck in `Init` status:
It doesn't answer its peer with any packets, even receiving continuous
`Down` packets.
Root Cause:
bfdd with passive mode answers its peer only *one* packet in `Down` status,
then it enters into `Init` status and ignores subsequent `Down` packets.
Unluckily that *one* answered packet is lost, at that moment its peer
with active mode can only have to send `Down` packets.
Fix:
1) With passive mode, bfdd should start xmittimer after received `Down` packet.
Refer to RFC5880:
"A system taking the Passive role MUST NOT begin sending BFD packets for
a particular session until it has received a BFD packet for that session, and
thus has learned the remote system's discriminator value."
2) Currently this added xmittimer for passive mode can be safely removed
except receiving `AdminDown` packet:
- `bfd_session_enable/bfd_set_passive_mode` doesn't start xmittimer
- `ptm_bfd_sess_dn/bfd_set_shutdown` can remove xmittimer
Per RFC5880, receiving `AdminDown` packet should be also regarded as `Down`,
so just call `ptm_bfd_sess_dn`, which will safely remove the added xmittimer
for passive mode. In summary, call `ptm_bfd_sess_dn` for two status changes
on receiving `AdminDown`: `Init`->`Down` and `Up`->`Down`.
Signed-off-by: anlan_cs <vic.lan@pica8.com>
A few optimizations for bfd NB:
- Remove unuseful checks for parameters with the same values
- Replace checking values of bfd parameters with YANG's "range"
- Append "required-echo-receive-interval" with 0 for it can be disabled
Signed-off-by: anlan_cs <vic.lan@pica8.com>
Since control packets may be dropped by ttl check, the counter
operation should be put after all check including ttl check.
Signed-off-by: anlan_cs <vic.lan@pica8.com>
Currently, it is possible to rename the default VRF either by passing
`-o` option to zebra or by creating a file in `/var/run/netns` and
binding it to `/proc/self/ns/net`.
In both cases, only zebra knows about the rename and other daemons learn
about it only after they connect to zebra. This is a problem, because
daemons may read their config before they connect to zebra. To handle
this rename after the config is read, we have some special code in every
single daemon, which is not very bad but not desirable in my opinion.
But things are getting worse when we need to handle this in northbound
layer as we have to manually rewrite the config nodes. This approach is
already hacky, but still works as every daemon handles its own NB
structures. But it is completely incompatible with the central
management daemon architecture we are aiming for, as mgmtd doesn't even
have a connection with zebra to learn from it. And it shouldn't have it,
because operational state changes should never affect configuration.
To solve the problem and simplify the code, I propose to expand the `-o`
option to all daemons. By using the startup option, we let daemons know
about the rename before they read their configs so we don't need any
special code to deal with it. There's an easy way to pass the option to
all daemons by using `frr_global_options` variable.
Unfortunately, the second way of renaming by creating a file in
`/var/run/netns` is incompatible with the new mgmtd architecture.
Theoretically, we could force daemons to read their configs only after
they connect to zebra, but it means adding even more code to handle a
very specific use-case. And anyway this won't work for mgmtd as it
doesn't have a connection with zebra. So I had to remove this option.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
When the detection time expires, we put the session down and restart the
timer. As the comment in the code says, it's needed to zero the remote
discriminator after the second expiration.
But the RFC clearly says that this must be done on the first expiration:
bfd.RemoteDiscr
The remote discriminator for this BFD session. This is the
discriminator chosen by the remote system, and is totally opaque
to the local system. This MUST be initialized to zero. If a
period of a Detection Time passes without the receipt of a valid,
authenticated BFD packet from the remote system, this variable
MUST be set to zero.
And we actually already do it in `ptm_bfd_sess_dn`, so there's no need
to reset the timer and wait for it twice.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Per RFC 5880 section 6.8.12, the use of a Poll Sequence is not necessary
when the Detect Multiplier is changed. Currently, we update the Detection
Timeout only when a Poll Sequence is terminated, therefore we ignore the
Detect Multiplier change if it's not accompanied with RX/TX timer change.
To fix the problem, we should update the Detection Timeout on every
received packet.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Since f60a1188 we store a pointer to the VRF in the interface structure.
There's no need anymore to store a separate vrf_id field.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
show/clear DEFUNs always require either peer label or IP address to be
specified, so if `label` is NULL then `peer_str` is definitely not NULL.
But Coverity doesn't know about that, so it complains about possible
NULL dereference of `peer_str`. This commit should make Coverity happy.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
This removes a giant `switch { }` block from lib/zclient.c and
harmonizes all zclient callback function types to be the same (some had
a subset of the args, some had a void return, now they all have
ZAPI_CALLBACK_ARGS and int return.)
Apart from getting rid of the giant switch, this is a minor security
benefit since the function pointers are now in a `const` array, so they
can't be overwritten by e.g. heap overflows for code execution anymore.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Well, there are some weird and duplicated checks there...
All we need is two simple checks:
- VRF existence. We must have it to enable the session.
- Interface existence. If it's configured for the session, we have to
bind the session to the interface.
This commit implements these checks and removes unnecessary duplication.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
We get the pointer to the interface on which the packet was received
right at the beginning of bfd_recv_cb. So let's use this pointer and
don't perform additional interface lookups.
Also explain in more detail how we process VRF id with different
backends.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
To ensure this, add a const modifier to functions' arguments. Would be
great do this initially and avoid this large code change, but better
late than never.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
FRR should only ever use the appropriate THREAD_ON/THREAD_OFF
semantics. This is espacially true for the functions we
end up calling the thread for.
Signed-off-by: Donatas Abraitis <donatas.abraitis@gmail.com>
Since there's very few locations where the `frr-format` actually prints
false positive warnings, consensus seems to be to just work around the
false positives even if the code is correct.
In fact, there is only one pattern of false positives currently, in
`bfdd/dplane.c` which does `vty_out("%"PRIu64, (uint64_t)be64toh(...))`.
The workaround/fix for this is a replacement `be64toh` whose type is
always `uint64_t` regardless of what OS we're on, making the cast
unnecessary.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
There is a possibility that the same line can be matched as a command in
some node and its parent node. In this case, when reading the config,
this line is always executed as a command of the child node.
For example, with the following config:
```
router ospf
network 193.168.0.0/16 area 0
!
mpls ldp
discovery hello interval 111
!
```
Line `mpls ldp` is processed as command `mpls ldp-sync` inside the
`router ospf` node. This leads to a complete loss of `mpls ldp` node
configuration.
To eliminate this issue and all possible similar issues, let's print an
explicit "exit" at the end of every node config.
This commit also changes indentation for a couple of existing exit
commands so that all existing commands are on the same level as their
corresponding node-entering commands.
Fixes#9206.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
There's a padding byte between "mhop" and "peer" fields in this structure.
This structure is sometimes passed by value to functions and used in
assignments. The standard doesn't guarantee that the padding bytes are
copied on assignments. As this structure is used as a hash key, having
this padding byte with unspecified value can lead to unwanted behavior.
Fix the possible issue by making the "mhop" field to be 2 bytes. Also
make the struct packed as a precaution for future changes.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Never send an interface name/index for multihop sessions. It breaks
"neighbor A.B.C.D update-source" config in BGP.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Compile with v2.0.0 tag of `libyang2` branch of:
https://github.com/CESNET/libyang
staticd init load time of 10k routes now 6s vs ly1 time of 150s
Signed-off-by: Christian Hopps <chopps@labn.net>
The idea of the "with-defaults" flag is to show the default values for
parameters that were not configured by the user. But bfdd incorrectly
shows the default values for all parameters, including the
user-configured ones.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Most of these are many, many years out of date. All of them vary
randomly in quality. They show up by default in packages where they
aren't really useful now that we use integrated config. Remove them.
The useful ones have been moved to the docs.
Signed-off-by: Quentin Young <qlyoung@nvidia.com>
When bfd node is removed, we must clear all NB entries set by its
children - sessions and profiles. Let's store some fake data as an entry
for the bfd node to be able to unset it later.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Back when I put this together in 2015, ISO C11 was still reasonably new
and we couldn't require it just yet. Without ISO C11, there is no
"good" way (only bad hacks) to require a semicolon after a macro that
ends with a function definition. And if you added one anyway, you'd get
"spurious semicolon" warnings on some compilers...
With C11, `_Static_assert()` at the end of a macro will make it so that
the semicolon is properly required, consumed, and not warned about.
Consistently requiring semicolons after "file-level" macros matches
Linux kernel coding style and helps some editors against mis-syntax'ing
these macros.
Signed-off-by: David Lamparter <equinox@diac24.net>
Currently there is a single interval for both RX and TX echo functions.
This commit introduces separate RX and TX timers for echo packets.
The main advantage is to be able to set the receive interval to zero
when we don't want to receive echo packets from the remote system.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Echo-mode implementation is currently broken. Instead of sending packets
to it's own address, bfdd is sending echo packets to the peer's address.
It may seem to work when testing between two FRR instances, because FRR
loops back such packets, but no other implementation is supposed to do
that.
Let's warn users that the current implementation works only between two
FRR instances.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Currently this timer is only started when we receive the first echo
packet. If we never receive the packet, the timer is never started and
the user falsely assumes that echo function is working.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Current behavior is inconsistent. When the session is created by another
daemon, it is up by default. When we later configure peer in bfdd, the
session is still up, but the NB layer thinks that it is down.
More than that, even when the session is created in bfdd using peer
command, it is created in DOWN state, not ADM_DOWN. And it actually
starts sending and receiving packets. The sessions is marked with
SHUTDOWN flag only when we try to reconfigure some parameter. This
behavior is also very unexpected.
Fixes#7780.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
RFC 5880 Section 6.8.4:
In Asynchronous mode, the Detection Time calculated in the local
system is equal to the value of Detect Mult received from the remote
system, multiplied by the agreed transmit interval of the remote
system (the greater of bfd.RequiredMinRxInterval and the last
received Desired Min TX Interval).
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
It's not currently possible to configure echo mode in profile node:
```
(config)# bfd
(config-bfd)# profile test
(config-bfd-profile)# echo-mode
% Echo mode is only available for single hop sessions.
(config-bfd-profile)# echo-interval 20
% Echo mode is only available for single hop sessions.
```
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
