Correctly preserve inbound UFID on outbound TCP close #469
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
Outbound TCP packets which transitioned to close were double-removing the TCP flow entries allocated to them. This is not bad on its own due to the table lock. However, the inbound UFID used to invalidate both UFT entries was being sourced from the second remove, so always returned `None`. This fix plumbs the inbound UFT down correctly and performs the removal only once. Fixes #466.
This is an extra safeguard in the event that the TCP flow state's inbound UFID is never set, and hard-reconciles any case where we have a UFT for a TCP flow but no TCP flow.
rcgoodfellow
approved these changes
Mar 7, 2024
FelixMcFelix
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Mar 13, 2024
* a4c956e chore(deps): lock file maintenance (oxidecomputer/opte#474) * 6847b04 chore(deps): update rust crate nix to 0.28 (oxidecomputer/opte#473) * c30aa36 Benchmarks and instrumentation (oxidecomputer/opte#395) * 53201de Fix xtask non-package install for fresh systems. (oxidecomputer/opte#470) * 7e56634 Correctly preserve inbound UFID on outbound TCP close (oxidecomputer/opte#469)
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.
Outbound TCP packets which transitioned the state machine to
Closedweredouble-removing the TCP flow entries allocated to them. This is not bad on its
own due to the table lock. However, the inbound UFID was being always being
reported as
Nonein this case. Ultimately, we need both UFIDs to removeboth UFT entries.
The inbound UFID used to invalidate both UFT entries was being computed from a
second remove in e.g.
process_out_tcp_existing. Naturally this returnedNone,as the TCP flow entry has already been removed. This resulted in the inbound UFID
always being
None-- a later call toupdate_tcp_entrythus removes only theoutbound UFT entry and leaves a dangling inbound UFT entry.
This dangling entry prevents new inbound TCP flows on the same 5-tuple for a time
of 1 minute (i.e., until the UFT entry expires).
This fix plumbs the inbound UFID down correctly and performs the flow entry removal
only once. This ensures that both UFT entries will be torn down when a guest
packet transitions the state to
TcpState::Closed. Also, any packets droppeddue to a missing TCP flow entry will now remove the existing UFT entry they matched
against.
Fixes #466.