| Age | Commit message (Collapse) | Author |
|---|
|
Fixes warning logs:
```
2023/05/29 20:11:50 BFD: [ZKB8W-3S2Q4][EC 100663330] unneeded 'destroy' callback for '/frr-bfdd:bfdd/bfd/profile/minimum-ttl'
2023/05/29 20:11:50 BFD: [ZKB8W-3S2Q4][EC 100663330] unneeded 'destroy' callback for '/frr-bfdd:bfdd/bfd/sessions/multi-hop/minimum-ttl'
```
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
(cherry picked from commit f7884aedf7a1249e3aae71b6a66c9a0f0915c4ef)
|
|
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>
(cherry picked from commit b17c179664da7331a4669a1cf548e4e9c48a5477)
|
|
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>
(cherry picked from commit f68114c1c397a939fd758af072c37d535f1de92f)
|
|
these changes are for improving the code maintainability
Signed-off-by: sri-mohan1 <sri.mohan@samsung.com>
|
|
BFDD: Add RTT to BFD IPV4 Echo packet processing
|
|
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>
|
|
bfdd: allow l3vrf bfd sessions without udp leaking
|
|
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>
|
|
Signed-off-by: Lynne Morrison <lynne.morrison@ibm.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>
|
|
Close the descriptor if something fails and we don't return it.
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>
|
|
*: standardize interface name maximum length
|
|
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>
|
|
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
|
|
The int return value is never used. Modify the code
base to just return a void instead.
Signed-off-by: Donald Sharp <sharpd@nvidia.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>
|
|
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
|
|
bfdd: fix broken FSM in passive mode
|
|
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>
|
|
Signed-off-by: anlan_cs <vic.lan@pica8.com>
|
|
bfdd: fix the possibly wrong counter of control packets
|
|
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>
|
|
*: rework renaming the default VRF
|
|
Found some extra spaces during code inspection. Let's
get them cleaned up.
Signed-off-by: Donald Sharp <sharpd@nvidia.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>
|
|
bfdd: remove unnecessary receive timer restart
|
|
bfdd: fix detection timeout update
|
|
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>
|
|
Signed-off-by: Donatas Abraitis <donatas.abraitis@gmail.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>
|
|
Naming functions/data structures more appropriately for
the project we are actually in.
Signed-off-by: Donald Sharp <sharpd@nvidia.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>
|
|
*: fix usage of if_lookup_by_index_all_vrf
|
|
*: fix usage of if_lookup_by_name_all_vrf
|
|
*: convert zclient callbacks to table
|
|
lib: northbound cli show/cmd functions must not modify data nodes
|
|
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>
|
|
*: `frr-format` with unmodified GCC
|
|
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>
|
