Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refreshement of BGP multi ASNs #16838

Merged
merged 6 commits into from
Sep 24, 2024

Conversation

ton31337
Copy link
Member

@ton31337 ton31337 force-pushed the fix/refresh_pr_9079 branch 2 times, most recently from 7d1a3c8 to 3c0b7c1 Compare September 16, 2024 13:31
@ton31337 ton31337 marked this pull request as draft September 16, 2024 20:56
@ton31337 ton31337 marked this pull request as ready for review September 17, 2024 06:16
@riw777 riw777 self-requested a review September 17, 2024 15:13
dslicenc and others added 5 commits September 18, 2024 18:03
1. bgp coredump is observed when we delete default bgp instance
   when we have multi-vrf; and route-leaking is enabled between
   default, non-default vrfs.
Removing default router bgp when routes leaked between non-default vrfs.
- Routes are leaked from VRF-A to VRF-B
- VPN table is created with auto RD/RT in default instance.
- Default instance is deleted, we try to unimport the routes from all VRFs
- non-default VRF schedules a work-queue to process deleted routes.
- Meanwhile default bgp instance clears VPN tables and free the route
  entries as well, which are still referenced by non-default VRFs which
  have imported routes.
- When work queue process starts to delete imported route in VRF-A it cores
  as it accesses freed memory.

- Whenever we delete bgp in default vrf, we skip deleting routes in the vpn
  table, import and export lists.
- The default hidden bgp instance will not be listed in any of the show
  commands.
- Whenever we create new default instance, handle it with AS number change
  i.e. old hidden default bgp's AS number is updated and also changing
  local_as for all peers.

2. A default instance is created with ASN of the vrf with the import
  statement.
  This may not be the ASN desired for the default table
- First problem with current behavior.
  Define two vrfs with different ASNs and then add import between.
  starting without any bgp config (no default instance)
  A default instance is created with ASN of the vrf with the import
  statement.
  This may not be the ASN desired for the default table
- Second related problem.  Start with a default instance and a vrf in a
  different ASN. Do an import statement in the vrf for a bgp vrf instance
  not yet defined and it auto-creates that bgp/vrf instance and it inherits
  the ASN of the importing vrf
- Handle bgp instances with different ASNs and handle ASN for auto created
  BGP instance

Signed-off-by: Kantesh Mundaragi <[email protected]>
While it's okay to allow overwriting the ASN of a bgp vrf/instance
that is either hidden or automatically created, it's dangerous to
allow it on explicitly defined instances.  If that were allowed,
a typo entering the bgp config could take down existing peering,
which would be a bad thing.

Signed-off-by: Don Slice <[email protected]>
When we leak routes and are using a different ASN in the
source vrf from the target vrf, it's possible we could
create loops because of an incomplete as-path (missing
the source vrf ASN). This fix adds the source vrf ASN and
stops the importing of a BGP prefix that has the target
ASN in the as-path in the source vrf.

Signed-off-by: Don Slice <[email protected]>
When a prefix in a vrf is imported into evpn as a type5,
copy the asn of the source to make sure it is reflected
in the target vrf.

Ticket: cumuluslinux-2554562
Signed-off-by: Don Slice <[email protected]>
When we get a update on a route that we already have information on
from another router and that route has been leaked ensure that
we do not crash when trying to releak the code when we may want
to modify the as path.

Signed-off-by: Donald Sharp <[email protected]>
```
==5445==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x7ff4c6bedb19 bp 0x7ffc95f2e400 sp 0x7ffc95f2e3c0 T0)
==5445==The signal is caused by a READ memory access.
==5445==Hint: address points to the zero page.
    #0 0x7ff4c6bedb19 in hash_iterate lib/hash.c:246
    #1 0x5618f41f5f59 in bgp_evpn_nh_finish bgpd/bgp_evpn_mh.c:4663
    #2 0x5618f41dcbe8 in bgp_evpn_vrf_delete bgpd/bgp_evpn.c:7336
    #3 0x5618f43bdd35 in bgp_delete bgpd/bgpd.c:4098
    #4 0x5618f417ef6e in bgp_exit bgpd/bgp_main.c:206
    #5 0x5618f417ef6e in sigint bgpd/bgp_main.c:164
    #6 0x7ff4c6cac6c4 in frr_sigevent_process lib/sigevent.c:117
    #7 0x7ff4c6cd8258 in event_fetch lib/event.c:1767
    #8 0x7ff4c6c0dcbc in frr_run lib/libfrr.c:1230
    #9 0x5618f418080d in main bgpd/bgp_main.c:555
    #10 0x7ff4c670c249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #11 0x7ff4c670c304 in __libc_start_main_impl ../csu/libc-start.c:360
    #12 0x5618f417ea20 in _start (/usr/lib/frr/bgpd+0x2e4a20)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV lib/hash.c:246 in hash_iterate
```

Signed-off-by: Donatas Abraitis <[email protected]>
@github-actions github-actions bot added the rebase PR needs rebase label Sep 23, 2024
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@riw777 riw777 merged commit 1a2eaba into FRRouting:master Sep 24, 2024
11 checks passed
@ton31337 ton31337 deleted the fix/refresh_pr_9079 branch September 24, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants