Firewall: keep the TCP hole open for the final ACK - #111
Open
nding0405 wants to merge 3 commits into
Open
Conversation
During an active TCP close, FreeRTOS calls `on_tcp_connect` before sending the final ACK. At that point, the callback removed the TCP firewall hole, so the firewall dropped the ACK and the peer could not finish closing. Waiting for the TCP state change is not enough because the state changes before the ACK passes through egress. Keep the firewall hole open until the final ACK passes through egress. `on_tcp_connect` marks the hole as closing, egress marks it as safe to remove, and `network_socket_close()` removes it afterward. Key changes: - Add a state field to each TCP firewall hole: `InUse` for an open connection, `InTermination` while waiting for the final ACK, and `CanBeRemoved` after that ACK passes egress. `NotFound` means that no matching hole exists. - Change the `SmallTableBase` helpers to compare only each table's key fields. The endpoint key remains the address and ports, so existing helper calls keep their old behavior and ignore the new state field. Lookup remains `O(log n)`; only the close helpers inspect the state. - Change the hole from `InUse` to `InTermination` in `on_tcp_connect`. - Change it from `InTermination` to `CanBeRemoved` after the next matching packet passes egress. - Make `network_socket_close()` wait for `CanBeRemoved` before removing the hole. A successful return guarantees that the hole was removed.
| * - `CanBeRemoved`: Egress passed the final packet; the hole may be removed. | ||
| * - `NotFound`: No hole matches the address and ports. | ||
| */ | ||
| enum class TCPFirewallState : uint32_t |
Contributor
There was a problem hiding this comment.
Can we make the base type smaller than this (e.g. uint8_t?) so that it fits better in the small table?
| * Get a TCP hole's state without changing it. | ||
| * | ||
| * Returns the state, or `TCPFirewallState::NotFound` if no hole matches. | ||
| */ |
Contributor
There was a problem hiding this comment.
Note that this can also fail for normal reasons so the return value might be a negative errno value.
Change firewall_get_tcpipv4_endpoint_state() to return int so it can return negative error codes. Change TCPFirewallState from uint32_t to uint8_t.
nding0405
force-pushed
the
firewall-block-last-ack
branch
from
August 8, 2026 03:44
ebf53a8 to
c4c4eb5
Compare
Contributor
|
Why the change to remove the comments on the values? This makes IDE integration worse. |
Updated comments for TCPFirewallState enum values for clarity.
Contributor
Author
Fixed. I restored the per-value comments. I had moved them out since I thought it might look a bit cleaner, but I see why per-value is better. Could you please take another look? Thanks! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
During an active TCP close, FreeRTOS calls
on_tcp_connectbefore sending the final ACK. At that point, the callback removed the TCP firewall hole, so the firewall dropped the ACK and the peer could not finish closing. Waiting for the TCP state change is not enough because the state changes before the ACK passes through egress.Keep the firewall hole open until the final ACK passes through egress.
on_tcp_connectmarks the hole as closing, egress marks it as safe to remove, andnetwork_socket_close()removes it afterward.Key changes:
InUsefor an open connection,InTerminationwhile waiting for the final ACK, andCanBeRemovedafter that ACK passes egress.NotFoundmeans that no matching hole exists.SmallTableBasehelpers to compare only each table's key fields. The endpoint key remains the address and ports, so existing helper calls keep their old behavior and ignore the new state field. Lookup remainsO(log n); only the close helpers inspect the state.InUsetoInTerminationinon_tcp_connect.InTerminationtoCanBeRemovedafter the next matching packet passes egress.network_socket_close()wait forCanBeRemovedbefore removing the hole. A successful return guarantees that the hole was removed.Note:
The polling here can be removed for ipv4 path, but I keep it here for ipv6. Since the firewall endpoint delayed removal and polling for firewall state logics are currently only applied to ipv4 path. The polling here still make sure we are at a safe point to free the socket, but it is not a signal of we can safely remove the firewall endpoint.