lib: fix for incorrect nexthop lookup for recursive routes #16970
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ffc2919 to
4a1d3a0
Compare
|
Could you fix first https://ci1.netdef.org/browse/FRR-PULLREQ3-CHECKOUT-5250 checkpatch.pl issues? |
954a39b to
3be7e2c
Compare
The only test that failed is passing on my debian 12 setup - could I get a CI rerun? Thanks @ton31337 |
donaldsharp
reviewed
Oct 3, 2024
|
To answer earlier questions - for a debug session, I attached to zebra and created a breakpoint like this: Starting state of the environment:
And
Run continue until I run into a nexthop_same function call where, nh1->resolved had an ipv6 of ::ffff:a28:1 and nh2->resolved had an ipv6 of ::ffff:a28:3. NH1 - 2300:5800::1 via (resolved) ::ffff:10:40:0:1 nexthop_same returns true for those two. nexthop_same(nh1->resolved, nh2->resolved) returns false: That was the basic idea behind the patch, but after some more digging I found another way to provoke this bug - this reproduction makes it seem that nexthop caching for a recursive route is incorrect. Video: https://youtube.com/watch?v=GY57SUDj-z4 (Edit after a force push - as asked in the review commands, comparison of resolved nexthops was moved up to nhg_compare_nexthops, which is only called in 2 places, instead of nexthop_same which is called in way more places). Please take a look if this approach now is acceptable. This solves our issue, seemingly without breaking anything else. |
piotrsuchy
added a commit
to piotrsuchy/frr
that referenced
this pull request
Oct 10, 2024
For cases where the real (resolved) nexthop changes for a recursive route, FRR fails to update second level routes depending on this route. That is because the zebra_nhg_hash_equal() function uses nhg_compare_nexthops(), which returns true for nexthops that are the same, but have different resolved nexthops. This commit changes that comparison only if both nexthops have a resolved nexthop when comparing. Example of a reproduction described in PR: FRRouting#16970. Signed-off-by: Piotr Suchy <psuchy@akamai.com>
3be7e2c to
e9db249
Compare
piotrsuchy
added a commit
to piotrsuchy/frr
that referenced
this pull request
Oct 10, 2024
For cases where the real (resolved) nexthop changes for a recursive route, FRR fails to update second level routes depending on this route. That is because the zebra_nhg_hash_equal() function uses nhg_compare_nexthops(), which returns true for nexthops that are the same, but have different resolved nexthops. This commit changes that comparison only if both nexthops have a resolved nexthop when comparing. Example of a reproduction described in PR: FRRouting#16970. Signed-off-by: Piotr Suchy <psuchy@akamai.com>
e9db249 to
c210c5e
Compare
For cases where the resolved nexthop changes for a recursive route, FRR fails to update second level routes depending on this route. That is because the zebra_nhg_hash_equal() function uses nhg_compare_nexthops(), which returns true for nexthops that are the same, but have different resolved nexthops. This commit changes that comparison only if both nexthops have a resolved nexthop when comparing. Example of a reproduction described in PR: FRRouting#16970. Signed-off-by: Piotr Suchy <psuchy@akamai.com>
c210c5e to
44ba2c9
Compare
|
CI:rerun |
mjstapp
reviewed
Oct 23, 2024
| * is hashed to the same group as a new one, because this function returns true, | ||
| * even though their resolved nexthop is not the same. | ||
| */ | ||
| if (nh1->resolved && nh2->resolved) |
There was a problem hiding this comment.
is 'same' is 'false' at this point, let's not do extra work?
| * even though their resolved nexthop is not the same. | ||
| */ | ||
| if (nh1->resolved && nh2->resolved) | ||
| return same && nexthop_same(nh1->resolved, nh2->resolved); |
There was a problem hiding this comment.
this will only compare one of the nexthops in 'resolved' - which may be a list?
|
|
||
| return true; | ||
| /* | ||
| * In case of both nexthops having a resolved nexthop, check if both those |
There was a problem hiding this comment.
I'm still not clear about this change.
This function is used to compare the nexthops that are forming an nhg - the list of nexthops in a route via zapi, or a daemon-owned nhg (also via zapi). this is different from the complete walk of the fully-resolved set of nexthops used in nexthop_group_hash(), for example.
|
Where are we on this one? Anyone still working on it? |
|
Yes we will come back to this shortly, had other issues and couldn't properly respond here - sorry! |
This was referenced Feb 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently there are cases, where a recursive route has an update of a resolved nexthop, but it is not properly picked up by zebra, because zebra thinks their hashes of nhg are equal.
This PR has a fix for this issue.
Topology:
We have 3 hosts running FRR and a 4th host which runs gobgp and injects static routes into the FRR hosts. The host running gobgp acts like a route server and the hosts are peered with the route server (actually 3 more instances of the route server but that's redundant for this bug) using bgp evpn and the injected static routes are evpn routes:
The reproduction of this bug goes like this:
22.22.22.22.22 via 2300:5800::1 via
Which means it wasn't properly updated and FRR has a conflicting state, where in one place the nexthop of the 2300:5800::1 route is host3's address (because the kernel route to 2300:5800::1 is there) and in the recursive route it's host1's address (because it used to be there and it wasn't updated properly).
I also have two videos with more details, showing the reproduction of the bug using:
If you have any more question about the topology or reproduction let me know.