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.

Sign up for GitHub

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

Jump to bottom

zebra: evpn: not coerce VTEP IP to IPv4 in nh_list #16341

Merged
merged 2 commits into from
Jul 22, 2024
Merged

zebra: evpn: not coerce VTEP IP to IPv4 in nh_list #16341

merged 2 commits into from
Jul 22, 2024

Conversation

crosser
Copy link
Contributor

@crosser crosser commented Jul 3, 2024

In L3 BGP-EVPN, if there are both IPv4 and IPv6 routes in the VPN, zebra maintains two instances of struct zebra_neigh object: one with IPv4 address of the nexthop, and another with IPv6 address that is an IPv4 mapped to IPv6, but only one intance of struct zebra_mac object, that contains a list of nexthop addresses that use this mac.

The code in zebra_vxlan module uses the fact that the list is empty as the indication that the zebra_mac object is unused, and needs to be dropped. However, preexisting code used nexthop address converted to IPv4 notation for the element of this list. As a result, when two zebra_neigh objects, one IPv4 and one IPv6-mapped-IPv4 were linked to the zebra_mac object, only one element was added to the list. Consequently, when one of the two zebra_neigh objects was dropped, the only element in the list was removed, making it empty, and zebra_mac object was dropped, and neigbrour cache elements uninstalled from the kernel.

As a result, after the last route in one family was removed from a remote vtep, all remaining routes in the other family became unreachable, because RMAC of the vtep was removed.

This commit makes zebra_mac use uncoerced IP address of the zebra_neigh object for the entries in the nh_list. This way, zebra_mac object no longer loses track of zebra_neigh objects that need it.

Bug-ID: 16340

@frrbot frrbot bot added the zebra label Jul 3, 2024
@crosser crosser mentioned this pull request Jul 3, 2024
Open
2 tasks
ton31337
ton31337 reviewed Jul 4, 2024
View reviewed changes
zebra/zebra_vxlan.c Outdated Show resolved Hide resolved
zebra/zebra_vxlan.c Outdated Show resolved Hide resolved
@ton31337 ton31337 requested a review from chiragshah6 July 4, 2024 07:04
@crosser
Copy link
Contributor Author

crosser commented Jul 4, 2024

I don't understand what does this failure mean

 Jobs Failure: 1/49

    TopoTests Ubuntu 22.04 amd64 Part 9 -> https://ci1.netdef.org/browse/FRR-PULLREQ3-TOPO9U2204AMD64-4041
    🚫 ospf_metric_propagation.test_ospf_metric_propagation test_link_1_4_down

It is just on one architecture? Is it related to the change?...

@ton31337
Copy link
Member

ton31337 commented Jul 4, 2024

I don't understand what does this failure mean

 Jobs Failure: 1/49

    TopoTests Ubuntu 22.04 amd64 Part 9 -> https://ci1.netdef.org/browse/FRR-PULLREQ3-TOPO9U2204AMD64-4041
    🚫 ospf_metric_propagation.test_ospf_metric_propagation test_link_1_4_down

It is just on one architecture? Is it related to the change?...

Ignore it, I rerun failed tests only.

@ton31337
Copy link
Member

ton31337 commented Jul 4, 2024

But still, could you fix frrbot check (styling)?

@crosser
Copy link
Contributor Author

crosser commented Jul 4, 2024

But still, could you fix frrbot check (styling)?

Done.

I did not do it intially because it asked to change some adjacent code that my change did not touch. Did now.

ton31337
ton31337 approved these changes Jul 5, 2024
View reviewed changes
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

LGTM, but IMO this should also be covered by a topotest, what do you think?

@crosser
Copy link
Contributor Author

crosser commented Jul 5, 2024

LGTM, but IMO this should also be covered by a topotest, what do you think?

I agree that it would be useful, but as I am not acquainted with the testing framework that this project is using, I am not sure that I will be able to do it in reasonable time...

@leonshaw
Copy link
Contributor

leonshaw commented Jul 6, 2024

Is this the same case encountered in #14808?

@ton31337
Copy link
Member

ton31337 commented Jul 7, 2024

Is this the same case encountered in #14808?

Looks like, and there is a topotest already. Anyway, it would be great if @chiragshah6 could help here reviewing.

@ton31337 ton31337 added this to the 10.1 milestone Jul 7, 2024
@crosser
Copy link
Contributor Author

crosser commented Jul 7, 2024 •
edited
Loading

Is this the same case encountered in #14808?

Looks like so! And such a shame that I did not find it and had to redo all the work myself.

But even bigger shame that your PR was not merged back then. Would have saved us quite some trouble that we had in production...

edit: @leonshaw , do you think that my change looks a bit cleaner? But, I am no judge, you guys decide which to take! The main thing is to have the bug fixed.

@leonshaw
Copy link
Contributor

leonshaw commented Jul 8, 2024

edit: @leonshaw , do you think that my change looks a bit cleaner?

I think you are correct. It's cleaner to have both IPv4/IPv6 nexthops explicitly. BTW, you can cherry-pick the topotest part if you like.

leonshaw and others added 2 commits July 8, 2024 10:32
@leonshaw @crosser
tests: Add IPv6 network adv/withdraw case in bgp_evpn_rt5 topotest
66a84c7 
Note that withdrawing IPv6 route should not affect IPv4.

Signed-off-by: Xiao Liang <shaw.leon@gmail.com>
@crosser
zebra: evpn: not coerce VTEP IP to IPv4 in nh_list
f883c71 
In L3 BGP-EVPN, if there are both IPv4 and IPv6 routes in the VPN, zebra
maintains two instances of `struct zebra_neigh` object: one with IPv4
address of the nexthop, and another with IPv6 address that is an IPv4
mapped to IPv6, but only one intance of `struct zebra_mac` object, that
contains a list of nexthop addresses that use this mac.

The code in `zebra_vxlan` module uses the fact that the list is empty as
the indication that the `zebra_mac` object is unused, and needs to be
dropped. However, preexisting code used nexthop address converted to
IPv4 notation for the element of this list. As a result, when two
`zebra_neigh` objects, one IPv4 and one IPv6-mapped-IPv4 were linked to
the `zebra_mac` object, only one element was added to the list.
Consequently, when one of the two `zebra_neigh` objects was dropped, the
only element in the list was removed, making it empty, and `zebra_mac`
object was dropped, and neigbrour cache elements uninstalled from the
kernel.

As a result, after the last route in _one_ family was removed from a
remote vtep, all remaining routes in the _other_ family became
unreachable, because RMAC of the vtep was removed.

This commit makes `zebra_mac` use uncoerced IP address of the `zebra_neigh`
object for the entries in the `nh_list`. This way, `zebra_mac` object no
longer loses track of `zebra_neigh` objects that need it.

Bug-URL: FRRouting#16340
Signed-off-by: Eugene Crosser <crosser@average.org>
@frrbot frrbot bot added the tests Topotests, make check, etc label Jul 8, 2024
@github-actions github-actions bot added size/L and removed size/M labels Jul 8, 2024
@crosser
Copy link
Contributor Author

crosser commented Jul 8, 2024 •
edited
Loading

you can cherry-pick the topotest part if you like.

Thanks! Cherry-picked, and put in front of the fix commit (to make it easier to reproduce the problem with with old code in place).

(I could not make topotests run locally though, they require docker, I installed it, but it gives me some permission errors... Hopefully CI will be able to run them.)

edit:

I managed to make docker work on the worksation, but the test inside busyloops ☹️ Anyway, the test added in this PR succeeds in CI. The current failure is apparently the same false negative as before, ospf_metric_propagation.test_ospf_metric_propagation test_link_1_4_down.

@pbrisset pbrisset self-requested a review July 9, 2024 15:34
@ton31337 ton31337 merged commit 8eb78b2 into FRRouting:master Jul 22, 2024
13 checks passed
@ton31337 ton31337 mentioned this pull request Jul 22, 2024
Closed
@Cellebyte
Copy link

I miss the proposed backport of this PR in stable/10.1. I thought when it is assigned to a milestone it should also be in the released frr version ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ton31337 ton31337 ton31337 approved these changes

@chiragshah6 chiragshah6 Awaiting requested review from chiragshah6

@pbrisset pbrisset Awaiting requested review from pbrisset

Assignees
No one assigned
Labels
master size/L tests Topotests, make check, etc zebra
Projects
None yet
Milestone
10.1
Development

Successfully merging this pull request may close these issues.
4 participants
@crosser @ton31337 @leonshaw @Cellebyte