Fix mirrored mode port tracking for implicit binds resulting from accept() calls#40287
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Linux-side GnsPortTracker port refresh logic so that tracked port allocations are not deallocated while any active socket is still using the same (protocol, port), including sockets that implicitly “inherit” the local port via accept().
Changes:
- Adjust
OnRefreshAllocatedPortsto consider any active socket using the same protocol+port as a match (not requiring an exactPortAllocationmatch). - Add explanatory comments clarifying why implicit binds (e.g., accepted sockets) must keep allocations alive.
|
todo add automated tests for the scenario of opening listen socket, accepting a connection on that socket, closing the listen socket, verify the traffic still flows on the connection socket |
FetoiuCatalin
changed the title
OneBlue
left a comment
OneBlue
left a comment
There was a problem hiding this comment.
I think this is OK as a short-term solution, even though I don't love forking the port tracking behavior between networking modes.
Long term, I think we should look into something like using cgroup_sock bind & unbind notifications.
This should allow us track when sockets are bound without having to loop with a timeout, which should remove a lot of pain points from the current implementation
|
|
||
| // Wait > 60 seconds so that the port tracker's deallocation logic kicks in. | ||
| // See c_bind_timeout_seconds in GnsPortTracker.cpp | ||
| std::this_thread::sleep_for(std::chrono::seconds(90)); |
There was a problem hiding this comment.
I don't love the idea of waiting 1.5 minutes in the tests, since this will add a big delay in the execution.
I'm not sure what's the best way to fix this, maybe by making this configurable via wslconfig. Any thoughts on this @benhillis ?
There was a problem hiding this comment.
yeah waiting 1.5 minutes isn't great.
There was a problem hiding this comment.
I agree this waiting is not ideal. looking at the comments in GnsPortTracker::OnRefreshAllocatedPorts, the reasoning for this deallocation timeout is because we can't know for sure when a bind() finished and don't want to deallocate too early.
we could try lowering c_bind_timeout_seconds below 60 seconds as that is probably more than enough.
thoughts?
// Because there's no way to get notified when the bind() call actually completes, it' possible
// that this method is called before the bind() completion and so the port allocation may not be visible yet.
// To avoid deallocating ports that simply haven't been done allocating yet, m_allocatedPorts stores a timeout
// that prevents deallocating the port unless:
//
// - The port has been seen to be allocated (if so, then the timeout is empty)
// - The timeout has expired
Agreed, I really don't like diverging in behavior between the two modes. |
The 2 modes have clearly different requirements for port tracking. Thus there is required divergence to implement each set of requirements. e.g., the listen socket binds, but that can create N # of accept sockets, where the local addresses for the accepted socket can be different from the listen socket |
|
@FetoiuCatalin Thanks for the fix — the bug analysis is solid. One thing I'd like to explore before merging: rather than branching on networking mode, could we just always match by port+protocol in The allocation side ( The tradeoff is that ports might be held slightly longer in non-mirrored modes, but I think that's acceptable given the allocation side already works this way. What do you think? |
@benhillis sorry, I missed your comment before hitting merge. the allocation method you mentioned (OnPortAllocationRequest) is specific to mirrored mode. My understanding from the code and from chat with Pierre is that virtio mode uses a different path to handle port allocations (VirtioNetworking::HandlePortNotification, VirtioNetworking::ModifyOpenPorts) which does not appear to use refcounting, which suggests allocations and deallocations need to be done individually there instead of delaying until all Linux sockets that use the port have closed. |
Summary of the Pull Request
The scenario of opening a listen socket on port X, accepting a connection on the listen socket (which will generate a connection socket that also uses source port X, but with an implicit bind() done by the OS), then closing the listen socket would lead to port X to be incorrectly deallocated for the guest, even though the connection socket is still using it
The Linux port tracker already has logic that queries the existing active sockets and checks what source ports they are using. This logic already sees the connection socket resulting from accept(). Then the list of currently tracked bound ports is matched against the list of sockets and if a port is not used by any socket, it is deallocated. This logic was assuming the address on which the original bind was done needs to match exactly with the address used by the socket, which is not true here (e.g. the listen socket was bound to 0.0.0.0, while the connection socket will use a non-zero source address)
Fix: In mirrored mode, Only match on protocol and port, ignoring address and family
This is consistent with how port allocations are done in GuestNetworkService::OnPortAllocationRequest, which uses just the port and protocol and uses a reference count to bundle together multiple port allocation requests, even if they have a different address or family.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
Add automated test for the listen + accept scenario that is fixed
Add automated test for mirrored mode for multiple binds reusing the same port
Run existing NetworkTests, which include multiple port tracking testcases
Verified using log in GnsPortTracker constructor that the networking mode is correctly plumbed to it in both mirrored and virtio modes