lib: actually hash all 16 bytes of IPv6 addresses, not just 4 (backport #17901) #18085
Conversation
This file contains 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
![mergify[bot]](https://avatars.githubusercontent.com/in/10562?s=60&v=4){kind=small}
While the loop is currently exited in all cases after using nexthop, it is a footgun to have "nh" around to be reused in another iteration of the loop. This would leave nexthop with partial data from the previous use. Make it local where needed instead. Signed-off-by: David Lamparter <equinox@opensourcerouting.org> (cherry picked from commit ce7f5b2)
Zero out the 12 unused bytes (for the IPv6 address) when reading in an IPv4 address. Signed-off-by: David Lamparter <equinox@opensourcerouting.org> (cherry picked from commit 95cf0b2)
Doesn't seem to break anything but really poor style to pass potentially uninitialized data to hash_lookup. Signed-off-by: David Lamparter <equinox@opensourcerouting.org> (cherry picked from commit c88589f)
rmap_src wasn't initialized, so for IPv4 the unused 12 bytes would contain whatever junk is on the stack on function entry. Also move the IPv4 parse before the IPv6 parse so if it's successful we can be sure the other bytes haven't been touched. Signed-off-by: David Lamparter <equinox@opensourcerouting.org> (cherry picked from commit b666ee5)
When reading in a nexthop from ZAPI, only set the fields that actually have meaning. While it shouldn't happen to begin with, we can otherwise carry padding garbage into the unused leftover union bytes. Signed-off-by: David Lamparter <equinox@opensourcerouting.org> (cherry picked from commit 4a0e141)
We were hashing 4 bytes of the address. Even for IPv6 addresses. Oops. The reason this was done was to try to make it faster, but made a complex maze out of everything. Time for a refactor. Signed-off-by: David Lamparter <equinox@opensourcerouting.org> (cherry picked from commit 001fcfa)
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.
Core change: We were hashing 4 bytes of the address. Even for IPv6 addresses. Oops.
Related changes:
gatesrcandrmap_srcto check that we're not feeding garbage into the 12 unused bytes and hash thatrmap_srcuse is just… wrong… this does a best-effort fixup but it's only a bandaid.nexthop_hashas a whole. It just feeds a contiguous block ofstruct nexthopintojhashnow, this should perform better than thejhash_wordshenanigans we've gone to before.this PR is incomplete,
nhg_compare_nexthopsneeds to descend into->resolved(cf. #16970, but that PR is also incomplete)Unfortunately the test should only go red if both the compare and the hash are broken at the same time, so I'd prefer to also fix NHG comparison to see if fixing that (and having the hash broken) makes the test go green to validate that. But fixing the compare needs a bit more work (I'll keep updating this PR)
This is an automatic backport of pull request #17901 done by Mergify.