fix: port 0 tracking not working for threads - #41051
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes WSL’s port-0 bind tracking for binds originating from non–thread-group-leader threads by using the kernel 6.9+ PIDFD_THREAD flag with pidfd_open(), and adds coverage to ensure port-0 tracking works in both Mirrored and Consomme networking modes.
Changes:
- Update
GnsPortTracker::DuplicateSocketFd()to preferpidfd_open(..., PIDFD_THREAD)with a fallback topidfd_open(..., 0)when unsupported. - Extend Linux unit tests with a new
sockettest entry and a threaded port-0 listener variation. - Extend Windows networking tests to build Linux unit tests and validate threaded port-0 bind tracking in both Mirrored and Consomme modes.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/linux/init/GnsPortTracker.cpp |
Uses PIDFD_THREAD for pidfd_open() to support thread TIDs from seccomp notifications. |
test/linux/unit_tests/socket.c |
Adds a new threaded port-0 server variation and exposes SocketTestEntry for the unified test runner. |
test/linux/unit_tests/unittests.h |
Registers the new socket test entry prototype and test name macro. |
test/linux/unit_tests/unittests.c |
Adds socket to the unified unit-test dispatch table. |
test/linux/unit_tests/Makefile |
Ensures socket.o is built into wsl_unit_tests. |
test/windows/NetworkTests.cpp |
Adds a Windows-side test that runs the threaded port-0 listener variation and verifies host connectivity, and builds Linux unit tests in Mirrored/Consomme class setup. |
test/windows/Common.h |
Centralizes LXSST_TESTS_INSTALL_COMMAND_LINE for reuse. |
test/windows/UnitTests.cpp |
Removes duplicate macro definition (uses the centralized one). |
test/windows/DrvFsTests.cpp |
Removes duplicate macro definition (uses the centralized one). |
|
Updated SDK here: #41071 |
|
Hi @benhillis. The PR is updated with the new SDK. Please help take a look. Thanks. |
There was a problem hiding this comment.
🟡 Not ready to approve
PIDFD_THREAD is used without any in-repo definition or <linux/pidfd.h> include, which is likely to break builds depending on header availability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
src/linux/init/GnsPortTracker.cpp:637
- This function is now explicitly operating on a thread ID (seccomp reports the TID), but the log messages still say "pid". That’s misleading when diagnosing failures (especially since the process-only fallback is expected to fail for non-group-leader threads). Update the log text to say "tid" (or "pid/tid").
wil::unique_fd pidFd(pidFdResult);
if (!pidFd)
{
GNS_LOG_INFO("Port-0 bind: pidfd_open failed for pid {} (errno {})", Pid, errno);
return {};
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
I added back the macro as musl is not providing <sys/pidfd.h> and including <linux/pidfd.h> directly conflicts with musl's header files. |
There was a problem hiding this comment.
🟡 Not ready to approve
The new Linux unit test build step is run unconditionally in large networking test class setups, increasing runtime and making unrelated tests depend on the Linux unit test build succeeding.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
test/windows/NetworkTests.cpp:5250
build_tests.shis invoked unconditionally for the entireConsommeTestsclass. This adds avoidable setup work and also makes unrelated Consomme networking tests depend on the Linux unit test build succeeding. Gate the build behindLxsstuVmMode()so it only runs in WSL2/VM mode where the port-tracking tests apply.
VERIFY_ARE_EQUAL(LxsstuInitialize(false), TRUE);
// Build the Linux unit tests used by the port tracking tests.
VERIFY_ARE_EQUAL(LxsstuLaunchWsl(LXSST_TESTS_INSTALL_COMMAND_LINE), (DWORD)0);
test/windows/NetworkTests.cpp:3944
build_tests.shis invoked unconditionally for the entireMirroredTestsclass. This makes every mirrored networking test depend on the Linux unit test toolchain and does extra work even when running non-port-tracking tests. Since this build is only needed for the new port-tracking coverage, gate it behindLxsstuVmMode()so it only runs when WSL2/VM mode is active (and to avoid failing setup in non-VM environments).
This issue also appears on line 5246 of the same file.
VERIFY_ARE_EQUAL(LxsstuInitialize(false), TRUE);
// Build the Linux unit tests used by the port tracking tests.
VERIFY_ARE_EQUAL(LxsstuLaunchWsl(LXSST_TESTS_INSTALL_COMMAND_LINE), (DWORD)0);
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
OneBlue
left a comment
There was a problem hiding this comment.
LGTM. One minor test comment. Feel free to address in a followup or in this PR
Summary of the Pull Request
In the port 0 tracking logic. The seccomp filter provides the tid instead of the pid. But the pidfd_open call by default requires the first parameter to be a pid. This causes bind calls in threads not tracked by the tracker. Which further causes the local host forwarding to not work for consomme.
This PR applies the PIDFD_THREAD flag introduced in kernel 6.9 to the pidfd_open call. So, it works with non group leader threads as well.
Please note:
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
Updated these tests:
NetworkTests::MirroredTests::PortZeroBindIsTracked
NetworkTests::ConsommeTests::PortZeroBindIsTracked