Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a connectivity issue between the Windows host and VSCode's remote server running inside WSL (mirrored/virtio networking mode). When VSCode calls bind() with port 0, the kernel dynamically assigns an ephemeral port. Previously the port tracker ignored such bind calls since the port was unknown at intercept time. The fix adds a deferred port-resolution path that captures the socket fd (via pidfd_getfd) while the process is still stopped by seccomp, then polls getsockname() in a background thread after the bind completes to discover and register the assigned port with the host.
Changes:
- New
DeferredPortLookupstruct andRunDeferredResolvebackground thread to handle post-bind port resolution for port-0 binds m_portsMutexadded to protectm_allocatedPortsfrom concurrent access between the main loop and the new background thread, refactoredTrackPortandHandleRequestaccordinglyDuplicateSocketFd+ResolvePortZeroBindlogic to obtain and poll the duplicated socket fd after bind completion
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/linux/init/GnsPortTracker.h |
Adds DeferredPortLookup struct, deferred-queue members (m_deferredMutex, m_deferredCv, m_deferredQueue), m_portsMutex, and new method declarations |
src/linux/init/GnsPortTracker.cpp |
Implements DuplicateSocketFd, RunDeferredResolve, ResolvePortZeroBind, TrackPort; refactors Run() and OnRefreshAllocatedPorts to be thread-safe; handles port-0 binds by deferring resolution |
b46e0a7 to
aa53c4d
Compare
aa53c4d to
0092279
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
You can also share your feedback on Copilot code review. Take the survey.
keith-horton
left a comment
There was a problem hiding this comment.
just a couple of simple fixes, and this change is good!
@benhillis fyi.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
You can also share your feedback on Copilot code review. Take the survey.
| return {}; | ||
| } | ||
|
|
||
| wil::unique_fd dupFd(static_cast<int>(syscall(SYS_pidfd_getfd, pidFd.get(), SocketFd, 0u))); |
There was a problem hiding this comment.
It looks like we're duplicating a bound socket in our process here. Is there a risk of this creating invalid states ? For instance, if a process does:
socket = bind(..., port = <ephemeral>);
auto boundPort = getPortFromBoundSocket(socket);
close(socket);
reopenedSocket = bind(..., port = boundPort);
Is there a risk of us having a copy of socket opened after the process closes the original socket ? If so, that could lead to weird behaviors
There was a problem hiding this comment.
I guess maybe one way we could solve this would be to make sure that whenever we get a bind from a process, we should try to resolve all the pending ephemeral binds that we have for that process before continuing
|
|
||
| void GnsPortTracker::ResolvePortZeroBind(DeferredPortLookup lookup) | ||
| { | ||
| // The socket fd was already duplicated (via pidfd_getfd) while the target process |
There was a problem hiding this comment.
Given that we're retrying for up to 2.5 seconds, there's a risk that we run "behind" if a bunch of sockets are in the queue here.
Maybe one we could solve this would be by having a list of "in-progress" resolves, and keep track of how many attempts we made to resolve, and just fail them once it goes above 25 (and loop through all pending sockets at once). That would prevent a possible buildup of sockets there
There was a problem hiding this comment.
Probably not blocking for this PR, could be done in a followup though
There was a problem hiding this comment.
Actually, how about we trace to return of the bind() method via ebpf ? We can have one hook to decide whether to allow / reject the bind, keep track of the pending bind() and examine it when bind() returns
OneBlue
left a comment
There was a problem hiding this comment.
Approving since the current change is an improvement over the current behavior. Let's look into using a post bind() hook in a followup change. Thank you for doing this !
|
Agreed with your comments, @OneBlue - I'll look at doing some kind of post bind hook as a follow up, but merging this now to light this up for mirrored / virtioproxy. |
With mirrored and virtio networking, we observed an issue when trying to connect to VSCode remote instance inside a WSL environment. The issue is that VSCode will call bind with the localhost IP address with port 0 specified. This indicates to the kernel that it should assign any available ephemeral port to the requesting process.
In WSL's port tracker, we ignore bind syscalls where the port is 0 because the kernel will assign one when the syscall completes. When the port is not 0, we send a PortMappingRequest to the host, which will create a new host-side socket bound to the specified local address and port where incoming connections can be listened to.
To address the issue, we should still track the bind syscall when the port is 0, but follow-up after the syscall has been completed. We do this by duplicating the socket that was created and calling getsockname to check was port has been assigned. Then we can send the same PortMappingRequest to the host.