Donald Sharp [Tue, 24 May 2022 17:33:35 +0000 (13:33 -0400)]
pimd: Allow the LPM match work properly with prefix lists and normal rp's
All rp_info's are being saved in the pim->rp_list and the non prefix-list
rp's are supposed to be saved in table pim->rp_table. What was happening,
though, is that all the plists were being stored at the 224.0.0.0/4 node
of the tree(irrelevant to the fact that we should not be looking up the
non-prefix list rp's in the table) and since we can have multiple prefix lists
and only one rp_info stored at the 224.0.0.0/4 node of the tree, there can be situations
where the 224.0.0.0/4 node can be overwritten due to the order entered.
As such there exists situations where command enter ordering will cause
what we match to, change in pim_rp_find_match_group.
Fixes:
a) Do not store prefix list based rp_info's in the pim->rp_table
b) In pim_rp_find_match_group, ensure that the node lookup does
not correspond to a prefix list based node.
c) When in the situation there are both:
ip pim rp 4.5.6.7 224.0.0.0/16
ip pim rp 5.6.67.8 prefix-list FOO
ip prefix-list FOO permit 224.0.1.0/24
and we receive a group for 224.0.1.5, we were comparing the
224.0.0.0/16 to the 224.0.0.0/4 of the 5.6.67.8 group, when
FRR should have been comparing to entry that matched in the prefix-list
Donatas Abraitis [Thu, 26 May 2022 11:03:02 +0000 (14:03 +0300)]
bgpd: Fix memory leak for BGP community alias in CLI
Before:
```
root@spine1-debian-11:~/frr# vtysh -c 'show memory bgpd | include Large Community'
Large Community : 100 40 4000 100 4000
Large Community value : 100 12 2400 100 2400
root@spine1-debian-11:~/frr# for x in $(seq 1 100); do vtysh -c 'conf' -c 'bgp community alias 123:123:123 testas' > /dev/null; done
root@spine1-debian-11:~/frr# vtysh -c 'show memory bgpd | include Large Community'
Large Community : 200 40 8000 200 8000
Large Community value : 200 12 4800 200 4800
root@spine1-debian-11:~/frr# for x in $(seq 1 100); do vtysh -c 'conf' -c 'bgp community alias 123:123:123 testas' > /dev/null; done
root@spine1-debian-11:~/frr# vtysh -c 'show memory bgpd | include Large Community'
Large Community : 300 40 12000 300 12000
Large Community value : 300 12 7200 300 7200
root@spine1-debian-11:~/frr#
```
After:
```
root@spine1-debian-11:~/frr# vtysh -c 'show memory bgpd | include Large Community'
Large Community : 0 40 0 1 56
Large Community display string: 0 8192 0 1 8200
Large Community value : 0 12 0 1 24
root@spine1-debian-11:~/frr# for x in $(seq 1 100); do vtysh -c 'conf' -c 'bgp community alias 123:123:123 testas' > /dev/null; done
root@spine1-debian-11:~/frr# vtysh -c 'show memory bgpd | include Large Community'
Large Community : 0 40 0 1 56
Large Community display string: 0 8192 0 1 8200
Large Community value : 0 12 0 1 24
root@spine1-debian-11:~/frr#
```
After we call [l]community_str2com(), we should free the memory.
Donald Sharp [Thu, 12 May 2022 13:39:27 +0000 (09:39 -0400)]
bgpd: Prevent crash when issuing various forms of `bgp no-rib`
The `bgp no-rib` command cycles through all the bgp rib tables
and removes them from zebra. Modify the code so that FRR notices
that it is attempting to cycle through the safi's that are two level
tables. In addition these safi's cannot just blindly remove the routes
from the rib as that there are none explicitly.
This code just prevents the crash in bgpd. It does not properly cycle
through and remove the zebra changes made that are explicit to these afi's.
This should be handled as appropriate by the developers on these safi's when
it becomes important to them.
Fixes: #11178 Signed-off-by: Donald Sharp <sharpd@nvidia.com>
If duplicate value is entered, the whole plist/alist just dropped.
Before:
```
$ grep prefix-list /etc/frr/frr.conf
ip prefix-list test seq 5 permit 1.1.1.1/32
ip prefix-list test seq 10 permit 1.1.1.1/32
$ systemctl restart frr
$ vtysh -c 'show run | include prefix-list'
$
```
After:
```
$ grep prefix-list /etc/frr/frr.conf
ip prefix-list test seq 5 permit 1.1.1.1/32
ip prefix-list test seq 10 permit 1.1.1.1/32
$ systemctl restart frr
$ vtysh -c 'show run | include prefix-list'
ip prefix-list test seq 5 permit 1.1.1.1/32
```
Donald Sharp [Wed, 27 Apr 2022 12:16:50 +0000 (08:16 -0400)]
tests: Correctly align variable type in tests
New compilers are noticing that the tests are compiling with
a pointer for the bgpd_privs variable while the bgp library
that is being linked against is not a pointer. Since
these tests had the declaration just to make the compiler
happy, let's actually align the variable type to make the
compiler even happier.
Donald Sharp [Wed, 27 Apr 2022 11:57:41 +0000 (07:57 -0400)]
bgpd: Ensure pkt_afi and pkt_safi are not used uninited in some cases
The compiler is, rightly, pointing out that in some cases it is
possible that the pkt_afi and pkt_safi values are not properly
set and could result in a use before initialized. I do not
actually belive that this is possible, but let's make the compiler
happy.
Carl Baldwin [Wed, 29 Dec 2021 21:32:33 +0000 (14:32 -0700)]
bgpd: ensure that the node gets unlocked in all cases
The logic to unlock dest if iteration completed without iterating the
entire node was flawed. Specifically, if iteration terminated due to
`gr_deferred == 0` then the node would not get unlocked.
This change takes into account the fact that dest will be NULL only in
the case when the entire table was iterated and all nodes were already
unlocked. In any other case, it needs to be unlocked.
David Lamparter [Mon, 25 Apr 2022 12:07:41 +0000 (14:07 +0200)]
build: fix new gcc 11.2 warnings
Some recent improvement in GCC triggers 2 new warnings, and they're
actual bugs (reading beyond end of prefix_ipv6 by accessing it as
prefix, which is larger.) Luckily it's only in sharpd.
Modified BGP to pay more attention the prefix returned from
zebra to ensure that a LPM wasn't accidently causing BGP
import checks to think it had a match when it did not.
This unfortunately removed the check to handle the route
removal.
This sequence of config and events would leave BGP in a bad state:
ip route 100.100.100.0/24 Null0
router bgp 32932
bgp network import-check
address-family ipv4 uni
network 100.100.100.0/24
Then if you removed the static route the import check would
still think the route existed:
donatas-pc(config)# ip route 100.100.100.0/24 Null0
donatas-pc(config)# do sh ip bgp import-check-table
Current BGP import check cache:
100.100.100.0 valid [IGP metric 0], #paths 1
blackhole
Last update: Sat Apr 23 22:51:34 2022
donatas-pc(config)# do sh ip nht
100.100.100.0
resolved via static
is directly connected, Null0
Client list: bgp(fd 17)
donatas-pc(config)# do sh ip bgp neighbors 192.168.10.123 advertised-routes | include 100.100.100.0
*> 100.100.100.0/24 0.0.0.0 0 32768 i
donatas-pc(config)# no ip route 100.100.100.0/24 Null0
donatas-pc(config)# do sh ip nht
100.100.100.0
resolved via kernel
via 192.168.10.1, enp3s0
Client list: bgp(fd 17)
donatas-pc(config)# do sh ip bgp import-check-table
Current BGP import check cache:
100.100.100.0 valid [IGP metric 0], #paths 1
blackhole
Last update: Sat Apr 23 22:51:34 2022
donatas-pc(config)# do sh ip bgp neighbors 192.168.10.123 advertised-routes | include 100.100.100.0
*> 100.100.100.0/24 0.0.0.0 0 32768 i
donatas-pc(config)#
Fix this by moving the code to handle the prefix check to the
evaluation function and mark the bnc as not matching and actually
evaluate the bnc.
Donald Sharp [Thu, 21 Apr 2022 18:13:01 +0000 (14:13 -0400)]
lib: Ensure an empty string does not get printed for host/domain
End operator is showing:
!
frr version 8.0.1
frr defaults traditional
hostname test.example.com
domainname
domainname should not be printed in this case at all. I do not
see any mechanism in current code that this could happen, but
what do I know? Put some extra stupid insurance in place
to prevent bad config from being generated.
zebra: set ZEBRA_IFC_DOWN on connected routes for inactive interfaces
If you are in a situation where you have multiple addresses on an
interface, zebra creates one connected route for them.
The issue is that the rib entry is not created if addresses were
added before the interface was running.
We add the address to a running interface in a typical flow.
Therefore, we handle the route & rib creation within a single ADD event.
In the opposite case, we create the route entries without activating them.
These are considered to be active since ZEBRA_IFC_DOWN is not set.
On the following interface UP, we ignore the same ADDR_ADD as it overlaps
with the existing prefixes -> rib is never created.
The minimal reproducible setup:
-----------------------------------------
ip link add name dummy0 type dummy
ip addr flush dev dummy0
ip link set dummy0 down
ip addr add 192.168.1.7/24 dev dummy0
ip addr add 192.168.1.8/24 dev dummy0
ip link set dummy0 up
vtysh -c 'show ip route' | grep dummy0
Donald Sharp [Sat, 9 Apr 2022 17:12:28 +0000 (13:12 -0400)]
zebra: Allow system routes to recurse through themselves
Currently if a end user has something like this:
Routing entry for 192.168.212.1/32
Known via "kernel", distance 0, metric 100, best
Last update 00:07:50 ago
* directly connected, ens5
Codes: K - kernel route, C - connected, S - static, R - RIP,
O - OSPF, I - IS-IS, B - BGP, E - EIGRP, N - NHRP,
T - Table, v - VNC, V - VNC-Direct, A - Babel, F - PBR,
f - OpenFabric,
> - selected route, * - FIB route, q - queued, r - rejected, b - backup
t - trapped, o - offload failure
K>* 0.0.0.0/0 [0/100] via 192.168.212.1, ens5, src 192.168.212.19, 00:00:15
C>* 192.168.212.0/27 is directly connected, ens5, 00:07:50
K>* 192.168.212.1/32 [0/100] is directly connected, ens5, 00:07:50
And FRR does a link flap, it refigures the route and rejects the default
route:
2022/04/09 16:38:20 ZEBRA: [NZNZ4-7P54Y] default(0:254):0.0.0.0/0: Processing rn 0x56224dbb5b00
2022/04/09 16:38:20 ZEBRA: [ZJVZ4-XEGPF] default(0:254):0.0.0.0/0: Examine re 0x56224dbddc20 (kernel) status: Changed Installed flags: Selected dist 0 metric 100
2022/04/09 16:38:20 ZEBRA: [GG8QH-195KE] nexthop_active_update: re 0x56224dbddc20 nhe 0x56224dbdd950 (7), curr_nhe 0x56224dedb550
2022/04/09 16:38:20 ZEBRA: [T9JWA-N8HM5] nexthop_active_check: re 0x56224dbddc20, nexthop 192.168.212.1, via ens5
2022/04/09 16:38:20 ZEBRA: [M7EN1-55BTH] nexthop_active: Route Type kernel has not turned on recursion
2022/04/09 16:38:20 ZEBRA: [HJ48M-MB610] nexthop_active_check: Unable to find active nexthop
2022/04/09 16:38:20 ZEBRA: [JPJF4-TGCY5] default(0:254):0.0.0.0/0: After processing: old_selected 0x56224dbddc20 new_selected 0x0 old_fib 0x56224dbddc20 new_fib 0x0
So the 192.168.212.1 route is matched for the nexthop but it is not connected and
zebra treats it as a problem. Modify the code such that if a system route
matches through another system route, then it should work imo.
Optional recognized and unrecognized BGP attributes,
whether transitive or non-transitive, SHOULD NOT be updated by the
route server (unless enforced by local IXP operator configuration)
and SHOULD be passed on to other route server clients.
By default LB ext-community works with iBGP peers. When we receive a route
from eBGP peer, we can send LB ext-community to iBGP peers.
With this patch, allow sending LB ext-community to iBGP/eBGP peers if they
are set as RS clients.
FRR does not send non-transitive ext-communities to eBGP peers, but for
example GoBGP sends and if it's set as RS client, we should pass those attributes
towards another RS client.
bgpd: Do not forget to update conditional advertisements rmaps for peer-groups
When the peer is configured for the first time:
```
neighbor P1 peer-group
neighbor P1 remote-as external
neighbor P1 advertise-map ADV exist-map EXIST
neighbor 10.10.10.1 peer-group P1
```
Conditional advertisements route-maps are not updated and cond. advertisements
do not work until FRR restarted. BGP sessions clear does not help.
Or even changing peer-group for a peer, causes this bug to kick in.
```
no neighbor 10.10.10.1
neighbor 10.10.10.1 peer-group P2
```
With this fix, cond. advertisements start working immediatelly.
bgpd: Allow setting BGP [large]community in route-maps
Before:
```
spine1-debian-11(config-route-map)# bgp community alias 65001:65001 test1
spine1-debian-11(config)# route-map rm permit 10
spine1-debian-11(config-route-map)# set community 65001:65001
% Malformed communities attribute
```
After:
```
spine1-debian-11(config)# bgp community alias 65001:65001 test1
spine1-debian-11(config)# route-map rm permit 10
spine1-debian-11(config-route-map)# set community 65001:65001
spine1-debian-11(config-route-map)#
```
Donald Sharp [Thu, 31 Mar 2022 17:07:06 +0000 (13:07 -0400)]
pimd: Send immediate join( with possible SG RPT prune bit set
When pimd has this setup:
src ----- rtr ------ receiver
|
rp
And the receiver sends a *,G join to rtr. When the
src starts sending a S,G, rtr can wait up to one join/prune
interval before sending a S,G rpt prune. This interval
causes the pimreg device to be in the S,G OIL as that the
RP does not know to prune this leg off.
rtr receives a *,G igmp join, sends a *,G join towards the rp
rtr receives a S,G packet <WRVIFWHOLE>
creates the S,G upstream, sends the register packet to the rp
the rp sees that it still has downstream interest so it forwards the packet on
After (up to 60 seconds ) the rtr, sends the normally scheduled join for
the G and sends the S,GRPT prune as part of it.
What is being done to fix it:
In wrvifwhole handling, when pimd detects that this is the FHR
and is not the RP *and* the incoming interface for the *,G
is different than the incomding interface for the S,G immediately
send a single join packet for the G( which will have the S,G RPT
prune in it ). Only do this on the first time receiving the
WRVIFWHOLE.
Donald Sharp [Fri, 1 Apr 2022 12:47:38 +0000 (08:47 -0400)]
tests: Reduce some pim test timings to more manageable levels
a) Remove the retry mechanism to continue looking for 75%
of the time for pim code.
This alone saves a bunch of time in tests that use lib/pim.py
Effectively all the times given for retry are already long
enough. Additionally some tests are gathering data with
the expectation that they will not find data so the entire
time is being taken up in retry's. Extending the retry
mechanism makes this even worse. This is especially bad
for pim in that keep alive timers are counting down and
state can be removed due to excessive time waiting.
b) Reduce verify verify_multicast_traffic from 40 seconds
to 20 seconds to gather traffic data.
A bunch of tests are doing this:
a) gather pre test start traffic data( taking about 70
seconds to run, because a bunch of time it was looking
for data that does not exist yet)
b) run a change to introduce a different traffic flow
c) gather post test traffic data ( taking about 70
seconds to run )
Why does this matter? Tests were iterating through
all the different routers looking for traffic flow
as well as different mroute state. This is against
the keepalive timer of 210 seconds. It does not take
long before the stream can be removed and the test is
still looking for data that is no longer there due
to state timeout.
The multicast_pim_sm_topo3/test_multicast_pim_sm_topo3.py
test reduced run time from 398 seconds to 297 seconds.
Greatly reducing keepalive timeout problems.
Trey Aspelund [Thu, 31 Mar 2022 16:35:18 +0000 (16:35 +0000)]
zebra: don't send RAs w/o LLv6 or on bridge-ports
It's confusing for a user to see 'Tx RA failed' in the logs when
they've enabled RAs (either through interface config or BGP unnumbered)
on an interface that can't send them. Let's avoid sending RAs on
interfaces that are bridge_slaves or don't have a link-local address,
since they are the two of the most common reasons for RA Tx failures.
Donald Sharp [Thu, 31 Mar 2022 19:56:24 +0000 (15:56 -0400)]
isisd, lib, ospfd, pathd: Null out free'd pointer
The commands:
router isis 1
mpls-te on
no mpls-te on
mpls-te on
no mpls-te on
!
Will crash
Valgrind gives us this:
==652336== Invalid read of size 8
==652336== at 0x49AB25C: typed_rb_min (typerb.c:495)
==652336== by 0x4943B54: vertices_const_first (link_state.h:424)
==652336== by 0x493DCE4: vertices_first (link_state.h:424)
==652336== by 0x493DADC: ls_ted_del_all (link_state.c:1010)
==652336== by 0x47E77B: isis_instance_mpls_te_destroy (isis_nb_config.c:1871)
==652336== by 0x495BE20: nb_callback_destroy (northbound.c:1131)
==652336== by 0x495B5AC: nb_callback_configuration (northbound.c:1356)
==652336== by 0x4958127: nb_transaction_process (northbound.c:1473)
==652336== by 0x4958275: nb_candidate_commit_apply (northbound.c:906)
==652336== by 0x49585B8: nb_candidate_commit (northbound.c:938)
==652336== by 0x495CE4A: nb_cli_classic_commit (northbound_cli.c:64)
==652336== by 0x495D6C5: nb_cli_apply_changes_internal (northbound_cli.c:250)
==652336== Address 0x6f928e0 is 272 bytes inside a block of size 320 free'd
==652336== at 0x48399AB: free (vg_replace_malloc.c:538)
==652336== by 0x494BA30: qfree (memory.c:141)
==652336== by 0x493D99D: ls_ted_del (link_state.c:997)
==652336== by 0x493DC20: ls_ted_del_all (link_state.c:1018)
==652336== by 0x47E77B: isis_instance_mpls_te_destroy (isis_nb_config.c:1871)
==652336== by 0x495BE20: nb_callback_destroy (northbound.c:1131)
==652336== by 0x495B5AC: nb_callback_configuration (northbound.c:1356)
==652336== by 0x4958127: nb_transaction_process (northbound.c:1473)
==652336== by 0x4958275: nb_candidate_commit_apply (northbound.c:906)
==652336== by 0x49585B8: nb_candidate_commit (northbound.c:938)
==652336== by 0x495CE4A: nb_cli_classic_commit (northbound_cli.c:64)
==652336== by 0x495D6C5: nb_cli_apply_changes_internal (northbound_cli.c:250)
==652336== Block was alloc'd at
==652336== at 0x483AB65: calloc (vg_replace_malloc.c:760)
==652336== by 0x494B6F8: qcalloc (memory.c:116)
==652336== by 0x493D7D2: ls_ted_new (link_state.c:967)
==652336== by 0x47E4DD: isis_instance_mpls_te_create (isis_nb_config.c:1832)
==652336== by 0x495BB29: nb_callback_create (northbound.c:1034)
==652336== by 0x495B547: nb_callback_configuration (northbound.c:1348)
==652336== by 0x4958127: nb_transaction_process (northbound.c:1473)
==652336== by 0x4958275: nb_candidate_commit_apply (northbound.c:906)
==652336== by 0x49585B8: nb_candidate_commit (northbound.c:938)
==652336== by 0x495CE4A: nb_cli_classic_commit (northbound_cli.c:64)
==652336== by 0x495D6C5: nb_cli_apply_changes_internal (northbound_cli.c:250)
==652336== by 0x495D23E: nb_cli_apply_changes (northbound_cli.c:268)
Let's null out the pointer. After this change. Valgrind no longer reports issues
and isisd no longer crashes.
Donatas Abraitis [Mon, 28 Mar 2022 08:41:35 +0000 (11:41 +0300)]
bgpd: Stop LLGR timer when the connection is established
When the connection goes up, the timer is not stopped and if we have a
subsequent GR event we have an old timer which is not as we expect.
Before:
```
spine1-debian-11# sh ip bgp 192.168.100.1/32
BGP routing table entry for 192.168.100.1/32, version 95
Paths: (1 available, best #1, table default, mark routes to be retained for a longer time. Requires support for Long-lived BGP Graceful Restart)
Not advertised to any peer
65001 47583, (stale)
192.168.0.1 from 192.168.0.1 (100.100.200.100)
Origin incomplete, valid, external, best (First path received)
Community: llgr-stale
Last update: Mon Mar 28 08:27:53 2022
Time until Long-lived stale route deleted: 23 <<<<<<<<<<<<
spine1-debian-11# sh ip bgp 192.168.100.1/32
BGP routing table entry for 192.168.100.1/32, version 103
Paths: (1 available, best #1, table default)
Advertised to non peer-group peers:
192.168.0.1
65001 47583
192.168.0.1 from 192.168.0.1 (100.100.200.100)
Origin incomplete, valid, external, best (First path received)
Last update: Mon Mar 28 08:43:29 2022
spine1-debian-11# sh ip bgp 192.168.100.1/32
BGP routing table entry for 192.168.100.1/32, version 103
Paths: (1 available, best #1, table default, mark routes to be retained for a longer time. Requires support for Long-lived BGP Graceful Restart)
Not advertised to any peer
65001 47583, (stale)
192.168.0.1 from 192.168.0.1 (100.100.200.100)
Origin incomplete, valid, external, best (First path received)
Community: llgr-stale
Last update: Mon Mar 28 08:43:30 2022
Time until Long-lived stale route deleted: 17 <<<<<<<<<<<<<<<
```
After:
```
spine1-debian-11# sh ip bgp 192.168.100.1/32
BGP routing table entry for 192.168.100.1/32, version 79
Paths: (1 available, best #1, table default, mark routes to be retained for a longer time. Requires support for Long-lived BGP Graceful Restart)
Not advertised to any peer
65001 47583, (stale)
192.168.0.1 from 192.168.0.1 (0.0.0.0)
Origin incomplete, valid, external, best (First path received)
Community: llgr-stale
Last update: Mon Mar 28 09:05:18 2022
Time until Long-lived stale route deleted: 24 <<<<<<<<<<<<<<<
spine1-debian-11# sh ip bgp 192.168.100.1/32
BGP routing table entry for 192.168.100.1/32, version 87
Paths: (1 available, best #1, table default)
Advertised to non peer-group peers:
192.168.0.1
65001 47583
192.168.0.1 from 192.168.0.1 (100.100.200.100)
Origin incomplete, valid, external, best (First path received)
Last update: Mon Mar 28 09:05:25 2022
spine1-debian-11# sh ip bgp 192.168.100.1/32
BGP routing table entry for 192.168.100.1/32, version 87
Paths: (1 available, best #1, table default, mark routes to be retained for a longer time. Requires support for Long-lived BGP Graceful Restart)
Not advertised to any peer
65001 47583, (stale)
192.168.0.1 from 192.168.0.1 (100.100.200.100)
Origin incomplete, valid, external, best (First path received)
Community: llgr-stale
Last update: Mon Mar 28 09:05:29 2022
Time until Long-lived stale route deleted: 29 <<<<<<<<<<<<<<
```
rgirada [Mon, 21 Mar 2022 09:48:46 +0000 (02:48 -0700)]
ospfd: Default route becomes stale route in nbrs even after flush from originator.
Description:
Default route is not getting flushed from neighbours though originator
triggered flush and deleted LSA from its database. It become as stale
LSA in neighbours databse forever. This could seen in the following
sequence of configurations with less than a second interval b/w configs.
And this could happen only when originator shouldnt have default route
in its rib so it originates default route only when configure with 'always'
option.
In step-1, default route will be originated to AS.
In step-2, default route will be flushed to AS, but neighbours will be
discarding this update due to minlsainterval condition.
And it is expected that DUT need to keep send this update
until it receives the ack from neighbours by adding each
neighbour's retransmission list.
In Step-3: It is deleting the lsas from nbr's retransmission list
by assuming it initiated the flush. This is cuasing to not
send the lsa update anymore to neighbours which makes
stale lsa in nbrs forever.
Fix:
Allowed to delete the lsa from retransmission list only when lsa is
not in maxage during flushing procedure.
Donald Sharp [Fri, 25 Mar 2022 11:44:55 +0000 (07:44 -0400)]
bgpd: Fix possible insufficient stream data
When reading the BGP_PREFIX_SID_SRV6_L3_SERVICE_SID_STRUCTURE
it is possible that the length read in the packet is insufficiently
large enough to read a BGP_PREFIX_SID_SRV6_L3_SERVICE_SID_STRUCTURE.
Let's ensure that it is.
Fixes: #10860 Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Donald Sharp [Sat, 26 Mar 2022 20:20:53 +0000 (16:20 -0400)]
lib: Ensure order of operations is expected with SECONDS
These 3 values:
ONE_DAY_SECOND
ONE_WEEK_SECOND
ONE_YEAR_SECOND
Are defined based upon the number of seconds. Unfortunately doing math
on these values say something like:
days = t->tv_sec / ONE_DAY_SECOND;
Once you go over about a day causes the order of operations to cause the multiplication
to get messed up:
204 if (!t)
(gdb) n
207 w = d = h = m = ms = 0;
(gdb) set t->tv_sec = ONE_DAY_SECOND + 30
(gdb) n
208 memset(buf, 0, size);
(gdb)
210 us = t->tv_usec;
(gdb)
211 if (us >= 1000) {
(gdb)
212 ms = us / 1000;
(gdb)
213 us %= 1000;
(gdb)
217 if (ms >= 1000) {
(gdb)
222 if (t->tv_sec > ONE_WEEK_SECOND) {
(gdb)
227 if (t->tv_sec > ONE_DAY_SECOND) {
(gdb)
228 d = t->tv_sec / ONE_DAY_SECOND;
(gdb) n
229 t->tv_sec -= d * ONE_DAY_SECOND;
(gdb) n
232 if (t->tv_sec >= HOUR_IN_SECONDS) {
(gdb) p d
$6 = 2073600
(gdb) p t->tv_sec
$7 = -179158953570
(gdb)
Converting to adding paranthesis around around the ONE_DAY_SECOND causes
the order of operations to work as expected.
Donald Sharp [Fri, 25 Mar 2022 00:02:33 +0000 (20:02 -0400)]
zebra: Fix use after deletion event in freebsd
In the FreeBSD code if you delete the interface
and it has no configuration, the ifp pointer will
be deleted from the system *but* zebra continues
to dereference the just freed pointer.
==58624== Invalid read of size 1
==58624== at 0x48539F3: strlcpy (in /usr/local/libexec/valgrind/vgpreload_memcheck-amd64-freebsd.so)
==58624== by 0x2B0565: ifreq_set_name (ioctl.c:48)
==58624== by 0x2B0565: if_get_flags (ioctl.c:416)
==58624== by 0x2B2D9E: ifan_read (kernel_socket.c:455)
==58624== by 0x2B2D9E: kernel_read (kernel_socket.c:1403)
==58624== by 0x499F46E: thread_call (thread.c:2002)
==58624== by 0x495D2B7: frr_run (libfrr.c:1196)
==58624== by 0x2B40B8: main (main.c:471)
==58624== Address 0x6baa7f0 is 64 bytes inside a block of size 432 free'd
==58624== at 0x484ECDC: free (in /usr/local/libexec/valgrind/vgpreload_memcheck-amd64-freebsd.so)
==58624== by 0x4953A64: if_delete (if.c:283)
==58624== by 0x2A93C1: if_delete_update (interface.c:874)
==58624== by 0x2B2DF3: ifan_read (kernel_socket.c:453)
==58624== by 0x2B2DF3: kernel_read (kernel_socket.c:1403)
==58624== by 0x499F46E: thread_call (thread.c:2002)
==58624== by 0x495D2B7: frr_run (libfrr.c:1196)
==58624== by 0x2B40B8: main (main.c:471)
==58624== Block was alloc'd at
==58624== at 0x4851381: calloc (in /usr/local/libexec/valgrind/vgpreload_memcheck-amd64-freebsd.so)
==58624== by 0x496A022: qcalloc (memory.c:116)
==58624== by 0x49546BC: if_new (if.c:164)
==58624== by 0x49546BC: if_create_name (if.c:218)
==58624== by 0x49546BC: if_get_by_name (if.c:603)
==58624== by 0x2B1295: ifm_read (kernel_socket.c:628)
==58624== by 0x2A7FB6: interface_list (if_sysctl.c:129)
==58624== by 0x2E99C8: zebra_ns_enable (zebra_ns.c:127)
==58624== by 0x2E99C8: zebra_ns_init (zebra_ns.c:214)
==58624== by 0x2B3FF2: main (main.c:401)
==58624==
Zebra needs to pass back whether or not the ifp pointer
was freed when if_delete_update is called and it should
then check in ifan_read as well as ifm_read that the
ifp pointer is still valid for use.
Donatas Abraitis [Thu, 24 Mar 2022 10:00:57 +0000 (12:00 +0200)]
bgpd: Turn off thread when running `no bmp targets X`
Avoid use-after-free and prevent from crashing:
```
(gdb) bt
0 raise (sig=<optimized out>) at ../sysdeps/unix/sysv/linux/raise.c:50
1 0x00007f2a15c2c30d in core_handler (signo=11, siginfo=0x7fffb915e630, context=<optimized out>) at lib/sigevent.c:261
2 <signal handler called>
3 0x00007f2a156201e4 in bmp_stats (thread=<optimized out>) at bgpd/bgp_bmp.c:1330
4 0x00007f2a15c3d553 in thread_call (thread=thread@entry=0x7fffb915ebf0) at lib/thread.c:2001
5 0x00007f2a15bfa570 in frr_run (master=0x55c43a393ae0) at lib/libfrr.c:1196
6 0x000055c43930627c in main (argc=<optimized out>, argv=<optimized out>) at bgpd/bgp_main.c:519
(gdb)
```
projects / mirror / frr.git / log