nl_bridge: properly handle L2 neighbors moving between ports #415
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
nl_cache_search() uses nl_object_identical()[1] to lookup the target object. This limits the check to unique attributes, which does not include the ifindex [2]. Therefore nl_cache_search() may return an object for a different port, in case it moved, which we then may wrongly remove from the cache. So add a check for the ifindex of the hit before attempting to delete any neighbors or cache entries. We could have also alternatively used nl_cache_find() which uses all attributes, but this function iterates over the whole cache intead of using the hashtable if possible, so would be much less performant. [1] https://github.com/thom311/libnl/blob/libnl3_7_0/lib/cache.c#L1113-L1114 [2] https://github.com/thom311/libnl/blob/libnl3_7_0/lib/route/neigh.c#L323 Fixes: 291591d ("l2 aging added") Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
Calling nl_cache_add() for an object cache that supports hashing will refuse to add an object if it already contains an "identical" object, where identical is considered having the same master, vid and mac for AF_BRIDGE neighbors. Therefore we need to remove the old entry first before we can add an updated one when we handle neighbors moving between ports. Without it, we may fail to update the flow if the port moves back again to its original port, as the outdated entry makes us think we already have a flow for it. Fixes: de9b16e ("nl_bridge: take over fdb entries instead of creating them") Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
rubensfig
approved these changes
Sep 21, 2023
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.
Properly handle L2 neighbors moving between ports without corrupting the cache.
Description
Due to bad assumptions, the code handling L2 neighbors moving between ports corrupts the local neighbor cache, poentially breaking additional moves or maybe even timouts.
nl_bridge::fdb_timeout()does not check the port of the returned entry from the cache, potentially deleting the wrong neighbor.Fix both issues to allow proper updating of flows when L2 neighbors move between ports.
How Has This Been Tested?
Extending the neighbor moving test to move twice, which then fails.