Graceful-Restart restart time is exchanged using OPEN messages. In order to
reduce restart time before doing an actual graceful restart, it might be useful
to increase the time, but this is not possible without resetting the session.
With this change, it's possible to send dynamic capability with a new value, and
GR will respect a new reset time value.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Capability was unset, but forgot to unset the role.
Fixes: 5ad080d37a ("bgpd: Handle Role capability via dynamic capabilities for SET/UNSET properly")
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Before this path we used session reset method, which is discouraged by rfc7606.
Handle this as rfc requires.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
```
donatas-pc(config-router)# timers bgp 8 12
% keeplive value 8 is larger than 1/3 of the holdtime, setting to 4
donatas-pc(config-router)# do sh run | include timers bgp
timers bgp 4 12
donatas-pc(config-router)#
```
Closes https://github.com/FRRouting/frr/issues/14287
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
It was missed to handle UNSET Role capability using dynamic capabilities.
Also move length check before actually handling Role capability.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
When bgp_stop finishes and it deletes the peer it is sending
back a return code stating that the peer was deleted, but
the code was operating like it was not deleted and continued
to access the data structure. Fix.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
The return code from a event handling perspective
is an enum. Let's intentionally make it a switch
so that all cases are ensured to be covered now
and in the future.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Straighten out the code to not mix the two. Especially
since bgp was assigning non enum values to the enum.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Shouldn't validate the label after 'decode_label'. If we validate
the label after 'decode_label', even the 'MPLS_INVALID_LABEL' will
be valid then.
Signed-off-by: Valerian_He <1826906282@qq.com>
```
3 0x00007f423aa42476 in __GI_raise (sig=sig@entry=11) at ../sysdeps/posix/raise.c:26
4 0x00007f423aef9740 in core_handler (signo=11, siginfo=0x7fffc414deb0, context=<optimized out>) at lib/sigevent.c:246
5 <signal handler called>
6 0x0000564dea2fc71e in route_set_aspath_prepend (rule=0x564debd66d50, prefix=0x7fffc414ea30, object=0x7fffc414e400)
at bgpd/bgp_routemap.c:2258
7 0x00007f423aeec7e0 in route_map_apply_ext (map=<optimized out>, prefix=prefix@entry=0x7fffc414ea30,
match_object=match_object@entry=0x7fffc414e400, set_object=set_object@entry=0x7fffc414e400, pref=pref@entry=0x0) at lib/routemap.c:2690
8 0x0000564dea2d277e in bgp_input_modifier (peer=peer@entry=0x7f4238f59010, p=p@entry=0x7fffc414ea30, attr=attr@entry=0x7fffc414e770,
afi=afi@entry=AFI_IP, safi=safi@entry=SAFI_UNICAST, rmap_name=rmap_name@entry=0x0, label=0x0, num_labels=0, dest=0x564debdd5130)
at bgpd/bgp_route.c:1772
9 0x0000564dea2df762 in bgp_update (peer=peer@entry=0x7f4238f59010, p=p@entry=0x7fffc414ea30, addpath_id=addpath_id@entry=0,
attr=0x7fffc414eb50, afi=afi@entry=AFI_IP, safi=<optimized out>, safi@entry=SAFI_UNICAST, type=9, sub_type=0, prd=0x0, label=0x0,
num_labels=0, soft_reconfig=0, evpn=0x0) at bgpd/bgp_route.c:4374
10 0x0000564dea2e2047 in bgp_nlri_parse_ip (peer=0x7f4238f59010, attr=attr@entry=0x7fffc414eb50, packet=0x7fffc414eaf0)
at bgpd/bgp_route.c:6249
11 0x0000564dea2c5a58 in bgp_nlri_parse (peer=peer@entry=0x7f4238f59010, attr=attr@entry=0x7fffc414eb50,
packet=packet@entry=0x7fffc414eaf0, mp_withdraw=mp_withdraw@entry=false) at bgpd/bgp_packet.c:339
12 0x0000564dea2c5d66 in bgp_update_receive (peer=peer@entry=0x7f4238f59010, size=size@entry=109) at bgpd/bgp_packet.c:2024
13 0x0000564dea2c901d in bgp_process_packet (thread=<optimized out>) at bgpd/bgp_packet.c:2933
14 0x00007f423af0bf71 in event_call (thread=thread@entry=0x7fffc414ee40) at lib/event.c:1995
15 0x00007f423aebb198 in frr_run (master=0x564deb73c670) at lib/libfrr.c:1213
16 0x0000564dea261b83 in main (argc=<optimized out>, argv=<optimized out>) at bgpd/bgp_main.c:505
```
With the configuration:
```
frr version 9.1-dev-MyOwnFRRVersion
frr defaults traditional
hostname ip-172-31-13-140
log file /tmp/debug.log
log syslog
service integrated-vtysh-config
!
debug bgp keepalives
debug bgp neighbor-events
debug bgp updates in
debug bgp updates out
!
router bgp 100
bgp router-id 9.9.9.9
no bgp ebgp-requires-policy
bgp bestpath aigp
neighbor 172.31.2.47 remote-as 200
!
address-family ipv4 unicast
neighbor 172.31.2.47 default-originate
neighbor 172.31.2.47 route-map RM_IN in
exit-address-family
exit
!
route-map RM_IN permit 10
set as-path prepend 200
exit
!
```
The issue is that we try to process NLRIs even if the attribute length is 0.
Later bgp_update() will handle route-maps and a crash occurs because all the
attributes are NULL, including aspath, where we dereference.
According to the RFC 4271:
A value of 0 indicates that neither the Network Layer
Reachability Information field nor the Path Attribute field is
present in this UPDATE message.
But with a fuzzed UPDATE message this can be faked. I think it's reasonable
to skip processing NLRIs if both update_len and attribute_len are 0.
Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Make sure we don't exceed the maximum of BGP_MAX_SOFT_VERSION.
The Capability Length SHOULD be no greater than 64.
Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Found when fuzzing:
```
==3470861==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xffff77801ef7 at pc 0xaaaaba7b3dbc bp 0xffffcff0e760 sp 0xffffcff0df50
READ of size 2 at 0xffff77801ef7 thread T0
0 0xaaaaba7b3db8 in __asan_memcpy (/home/ubuntu/frr_8_5_2/frr_8_5_2_fuzz_clang/bgpd/bgpd+0x363db8) (BuildId: cc710a2356e31c7f4e4a17595b54de82145a6e21)
1 0xaaaaba81a8ac in ptr_get_be16 /home/ubuntu/frr_8_5_2/frr_8_5_2_fuzz_clang/./lib/stream.h:399:2
2 0xaaaaba819f2c in bgp_attr_aigp_valid /home/ubuntu/frr_8_5_2/frr_8_5_2_fuzz_clang/bgpd/bgp_attr.c:504:3
3 0xaaaaba808c20 in bgp_attr_aigp /home/ubuntu/frr_8_5_2/frr_8_5_2_fuzz_clang/bgpd/bgp_attr.c:3275:7
4 0xaaaaba7ff4e0 in bgp_attr_parse /home/ubuntu/frr_8_5_2/frr_8_5_2_fuzz_clang/bgpd/bgp_attr.c:3678:10
```
Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Properly free the dynamically allocated memory held by `str` after its use.
The change also maintains the return value of `nb_cli_apply_changes` by using `ret` variable.
The ASan leak log for reference:
```
Direct leak of 55 byte(s) in 2 object(s) allocated from:
#0 0x7f16f285f867 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
#1 0x7f16f23fda11 in qmalloc ../lib/memory.c:100
#2 0x7f16f23a01a0 in frrstr_join ../lib/frrstr.c:89
#3 0x7f16f23418c7 in argv_concat ../lib/command.c:183
#4 0x55aba24731f2 in set_aspath_exclude_access_list_magic ../bgpd/bgp_routemap.c:6327
#5 0x55aba2455cf4 in set_aspath_exclude_access_list bgpd/bgp_routemap_clippy.c:836
#6 0x7f16f2345d61 in cmd_execute_command_real ../lib/command.c:993
#7 0x7f16f23460ee in cmd_execute_command ../lib/command.c:1052
#8 0x7f16f2346dc0 in cmd_execute ../lib/command.c:1218
#9 0x7f16f24f7197 in vty_command ../lib/vty.c:591
#10 0x7f16f24fc07c in vty_execute ../lib/vty.c:1354
#11 0x7f16f250247a in vtysh_read ../lib/vty.c:2362
#12 0x7f16f24e72f4 in event_call ../lib/event.c:1979
#13 0x7f16f23d1828 in frr_run ../lib/libfrr.c:1213
#14 0x55aba2269e52 in main ../bgpd/bgp_main.c:510
#15 0x7f16f1dbfd8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
```
Signed-off-by: Keelan Cannoo <keelan.cannoo@icloud.com>
The BGP FSM was using the peer as the unit of work
but the FSM is connection focused. So let's switch
it over to using that.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
As part of the conversion to a `struct peer_connection` it will
be desirable to have 2 pointers one for when we open a connection
and one for when we receive a connection. Start this actual
conversion over to this in `struct peer`. If this sounds confusing
take a look at the bgp state machine for connections and how
it resolves the processing of this router opening -vs- this
router receiving an open. At some point in time the state
machine decides that we are keeping one of the two connections.
Future commits will allow us to untangle the peer/doppelganger
duality with this abstraction.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
The status and ostatus are a function of the `struct peer_connection`
move it into that data structure.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Move PEER_THREAD_WRITES_ON and PEER_THREAD_READS_ON to
be a part of the `struct peer_connection` since this is
a connection oriented bit of data.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Move the peer->t_write and peer->t_read into `struct peer_connection`
as that these are properties of the connection.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
P# Please enter the commit message for your changes. Lines starting
BGP tracks connections based upon the peer. But the problem
with this is that the doppelganger structure for it is being
created. This has introduced a bunch of fragileness in that
the peer exists independently of the connections to it.
The whole point of the doppelganger structure was to allow
BGP to both accept and initiate tcp connections and then
when we get one to a `good` state we collapse into the
appropriate one. The problem with this is that having
2 peer structures for this creates a situation where
we have to make sure we are configing the `right` one
and also make sure that we collapse the two independent
peer structures into 1 acting peer. This makes no sense
let's abstract out the peer into having 2 connection
one for incoming connections and one for outgoing connections
then we can easily collapse down without having to do crazy
stuff. In addition people adding new features don't need
to have to go touch a million places in the code.
This is the start of this abstraction. In this commit
we'll just pull out the fd and input/output buffers
into a connection data structure. Future commits
will abstract further.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Properly free the dynamically allocated memory held by `str` after its use.
The change also maintains the return value of `nb_cli_apply_changes` by using 'ret' variable.
The ASan leak log for reference:
```
***********************************************************************************
Address Sanitizer Error detected in bgp_set_aspath_replace.test_bgp_set_aspath_replace/r1.asan.bgpd.11586
=================================================================
==11586==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 92 byte(s) in 3 object(s) allocated from:
#0 0x7f4e2951db40 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb40)
#1 0x7f4e28f19ea2 in qmalloc lib/memory.c:100
#2 0x7f4e28edbb08 in frrstr_join lib/frrstr.c:89
#3 0x7f4e28e9a601 in argv_concat lib/command.c:183
#4 0x56519adf8413 in set_aspath_replace_access_list_magic bgpd/bgp_routemap.c:6174
#5 0x56519adf8942 in set_aspath_replace_access_list bgpd/bgp_routemap_clippy.c:683
#6 0x7f4e28e9d548 in cmd_execute_command_real lib/command.c:993
#7 0x7f4e28e9da0c in cmd_execute_command lib/command.c:1051
#8 0x7f4e28e9de8b in cmd_execute lib/command.c:1218
#9 0x7f4e28fc4f1c in vty_command lib/vty.c:591
#10 0x7f4e28fc53c7 in vty_execute lib/vty.c:1354
#11 0x7f4e28fcdc8d in vtysh_read lib/vty.c:2362
#12 0x7f4e28fb8c8b in event_call lib/event.c:1979
#13 0x7f4e28efd445 in frr_run lib/libfrr.c:1213
#14 0x56519ac85d81 in main bgpd/bgp_main.c:510
#15 0x7f4e27f40c86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)
SUMMARY: AddressSanitizer: 92 byte(s) leaked in 3 allocation(s).
***********************************************************************************
***********************************************************************************
Address Sanitizer Error detected in bgp_set_aspath_exclude.test_bgp_set_aspath_exclude/r1.asan.bgpd.10385
=================================================================
==10385==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 55 byte(s) in 2 object(s) allocated from:
#0 0x7f6814fdab40 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb40)
#1 0x7f68149d6ea2 in qmalloc lib/memory.c:100
#2 0x7f6814998b08 in frrstr_join lib/frrstr.c:89
#3 0x7f6814957601 in argv_concat lib/command.c:183
#4 0x5570e05117a1 in set_aspath_exclude_access_list_magic bgpd/bgp_routemap.c:6327
#5 0x5570e05119da in set_aspath_exclude_access_list bgpd/bgp_routemap_clippy.c:836
#6 0x7f681495a548 in cmd_execute_command_real lib/command.c:993
#7 0x7f681495aa0c in cmd_execute_command lib/command.c:1051
#8 0x7f681495ae8b in cmd_execute lib/command.c:1218
#9 0x7f6814a81f1c in vty_command lib/vty.c:591
#10 0x7f6814a823c7 in vty_execute lib/vty.c:1354
#11 0x7f6814a8ac8d in vtysh_read lib/vty.c:2362
#12 0x7f6814a75c8b in event_call lib/event.c:1979
#13 0x7f68149ba445 in frr_run lib/libfrr.c:1213
#14 0x5570e03a0d81 in main bgpd/bgp_main.c:510
#15 0x7f68139fdc86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)
SUMMARY: AddressSanitizer: 55 byte(s) leaked in 2 allocation(s).
***********************************************************************************
```
Signed-off-by: Keelan Cannoo <keelan.cannoo@icloud.com>
Problem:
On GR helper, paths learnt from an interface based peer were linked
to bnc with ifindex=0. During restart of GR peer, BGP (unnumbered)
session (with GR restarter peer) goes down on GR helper but the routes
are retained. Later, when BGP receives an interface up event, it
will process all the paths associated with BNC whose ifindex matches the
ifindex of the interface for which UP event is received. However, paths
associated with bnc that has ifindex=0 were not being reinstalled since
ifindex=0 doesn't match ifindex of any interfaces. This results in
BGP routes not being reinstalled in zebra and kernel.
Fix:
For paths learnt from an interface based peer, set the
ifindex to peer's interface ifindex so that correct
peer based nexthop can be found and linked to the path.
Signed-off-by: Donald Sharp sharpd@nvidia.com
Signed-off-by: Pooja Jagadeesh Doijode <pdoijode@nvidia.com>
Relax this handling (RFC 7606) only for eBGP peers.
More details: https://datatracker.ietf.org/doc/html/rfc7606#section-4
There are two error cases in which the Total Attribute Length value
can be in conflict with the enclosed path attributes, which
themselves carry length values:
* In the first case, the length of the last encountered path
attribute would cause the Total Attribute Length to be exceeded
when parsing the enclosed path attributes.
* In the second case, fewer than three octets remain (or fewer than
four octets, if the Attribute Flags field has the Extended Length
bit set) when beginning to parse the attribute. That is, this
case exists if there remains unconsumed data in the path
attributes but yet insufficient data to encode a single minimum-
sized path attribute. <<<< HANDLING THIS CASE IN THIS COMMIT >>>>
In either of these cases, an error condition exists and the "treat-
as-withdraw" approach MUST be used (unless some other, more severe
error is encountered dictating a stronger approach), and the Total
Attribute Length MUST be relied upon to enable the beginning of the
NLRI field to be located.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Before the patch:
```
donatas-laptop(config-router)# bgp confederation
identifier AS number in plain <1-4294967295> or dotted <0-65535>.<0-65535> format
peers Peer ASs in BGP confederation
```
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
