Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove precondition on result of IOCTL_VM_SOCKETS_GET_LOCAL_CID #2588

Merged
merged 2 commits into from Nov 24, 2023

Conversation

simonjbeaumont
Copy link
Contributor

Motivation:

When using the IOCTL_VM_SOCKETS_GET_LOCAL_CID call to get the local Context ID for a VSOCK socket, we precondition that the resulting Context ID is not VMADDR_CID_ANY. This probably was an assumption too far and we've seen an issue1 where this led to a crash in the tests.

Digging a little more on this and it looks like we've been too strict:

  • Based on the tests in XNU it looks like it expects the result of this call to not be VMADDR_CID_ANY2.
  • The HyperV transport in the Linux kernel always returns VMADDR_CID_ANY3.
  • Loopback transport always returns VMADDR_CID_LOCAL4.
  • Virtio transport seems to return VMADDR_CID_ANY in the error path5.
  • The VMCI transport sends a control message datagram to the host6.

All that to say... we probably shouldn't make any precondition on the result of IOCTL_VM_SOCKETS_GET_LOCAL_CID since it appears to tightly coupled to the VSOCK transport, and I don't think we can confidently say anything about what the value should be.

Additionally, while investigating this I found that the tests actually crash when we don't have /dev/vsock available, which was an oversight:

docker run --privileged --rm -v $PWD:/code -w /code -it swift:5.9.1-jammy ls /dev/vsock
/dev/vsockdocker run --privileged --rm -v $PWD:/code -w /code -it swift:5.9.1-jammy swift test --filter VsockAddressTest.testGetLocalCID
Building for debugging...
Build complete! (6.90s)
Test Suite 'Selected tests' started at 2023-11-06 13:18:07.422
Test Suite 'VsockAddressTest' started at 2023-11-06 13:18:07.424
Test Case 'VsockAddressTest.testGetLocalCID' started at 2023-11-06 13:18:07.424
Test Case 'VsockAddressTest.testGetLocalCID' passed (0.004 seconds)
Test Suite 'VsockAddressTest' passed at 2023-11-06 13:18:07.427
         Executed 1 test, with 0 failures (0 unexpected) in 0.004 (0.004) seconds
Test Suite 'Selected tests' passed at 2023-11-06 13:18:07.427
         Executed 1 test, with 0 failures (0 unexpected) in 0.004 (0.004) secondsdocker run --rm -v $PWD:/code -w /code -it swift:5.9.1-jammy ls /dev/vsock
ls: cannot access '/dev/vsock': No such file or directorydocker run --rm -v $PWD:/code -w /code -it swift:5.9.1-jammy swift test --filter VsockAddressTest.testGetLocalCID
Building for debugging...
Build complete! (6.90s)
Test Suite 'Selected tests' started at 2023-11-06 13:18:43.133
Test Suite 'VsockAddressTest' started at 2023-11-06 13:18:43.134
Test Case 'VsockAddressTest.testGetLocalCID' started at 2023-11-06 13:18:43.134
NIOPosix/VsockAddress.swift:223: Fatal error: 'try!' expression unexpectedly raised an error: open(file:oFlag:): No such file or directory (errno: 2)
...[crash dump]...

These tests should be skipped by virtue of try XCTSkipUnless(System.supportsVsock), which checks that we can create a socket with AF_VSOCK. However, on Linux, this is not enough of a guard and we should probably also check for the presence of /dev/vsock.

Modifications:

  1. Remove the precondition that checks that the local CID as returned by the ioctl() is not VMADDR_CID_ANY.
  2. Don't force-try to open /dev/vsock, instead let this fail, which will propagate up to through the Channel.getOption path.
  3. Update the hasVsockSupport guard that conditionally runs the test to include checking for the presence of /dev/vsock when on Linux.

Result:

The tests no longer crash when /dev/vsock is not available on Linux. Note that we don't currently exercise these tests in CI because the CI runner doesn't have a new enough Linux kernel, but I've included some logs below from some manual runs.

Linux, with /dev/vsock (still runs and passes)

docker run --privileged --rm -v $PWD:/code -w /code -it swift:5.9.0-jammy swift test --filter VsockAddressTest.testGetLocalCID
...
Test Suite 'Selected tests' started at 2023-11-07 18:11:09.259
Test Suite 'VsockAddressTest' started at 2023-11-07 18:11:09.262
Test Case 'VsockAddressTest.testGetLocalCID' started at 2023-11-07 18:11:09.262
Test Case 'VsockAddressTest.testGetLocalCID' passed (0.004 seconds)
Test Suite 'VsockAddressTest' passed at 2023-11-07 18:11:09.266
         Executed 1 test, with 0 failures (0 unexpected) in 0.004 (0.004) seconds
Test Suite 'Selected tests' passed at 2023-11-07 18:11:09.266
         Executed 1 test, with 0 failures (0 unexpected) in 0.004 (0.004) seconds

Linux, without /dev/vsock (skips, no longer crashes)

docker run --rm -v $PWD:/code -w /code -it swift:5.9.0-jammy swift test --filter VsockAddressTest.testGetLocalCID
Test Suite 'Selected tests' started at 2023-11-07 18:12:00.285
Test Suite 'VsockAddressTest' started at 2023-11-07 18:12:00.287
Test Case 'VsockAddressTest.testGetLocalCID' started at 2023-11-07 18:12:00.287
/code/Tests/NIOPosixTests/VsockAddressTest.swift:57: VsockAddressTest.testGetLocalCID : Test skipped: required false value but got true
Test Case 'VsockAddressTest.testGetLocalCID' skipped (0.002 seconds)
Test Suite 'VsockAddressTest' passed at 2023-11-07 18:12:00.289
         Executed 1 test, with 1 test skipped and 0 failures (0 unexpected) in 0.002 (0.002) seconds
Test Suite 'Selected tests' passed at 2023-11-07 18:12:00.289
         Executed 1 test, with 1 test skipped and 0 failures (0 unexpected) in 0.002 (0.002) seconds

macOS (skips, as before)

uname
Darwinswift test --filter VsockAddressTest.testGetLocalCID
Test Suite 'Selected tests' started at 2023-11-07 18:14:46.468.
Test Suite 'swift-nioPackageTests.xctest' started at 2023-11-07 18:14:46.469.
Test Suite 'VsockAddressTest' started at 2023-11-07 18:14:46.469.
Test Case '-[NIOPosixTests.VsockAddressTest testGetLocalCID]' started.
/Users/Si/work/code/swift-nio/Tests/NIOPosixTests/VsockAddressTest.swift:57: -[NIOPosixTests.VsockAddressTest testGetLocalCID] : Test skipped
Test Case '-[NIOPosixTests.VsockAddressTest testGetLocalCID]' skipped (0.001 seconds).
Test Suite 'VsockAddressTest' passed at 2023-11-07 18:14:46.471.
         Executed 1 test, with 1 test skipped and 0 failures (0 unexpected) in 0.001 (0.001) seconds
Test Suite 'swift-nioPackageTests.xctest' passed at 2023-11-07 18:14:46.471.
         Executed 1 test, with 1 test skipped and 0 failures (0 unexpected) in 0.001 (0.001) seconds
Test Suite 'Selected tests' passed at 2023-11-07 18:14:46.471.
         Executed 1 test, with 1 test skipped and 0 failures (0 unexpected) in 0.001 (0.002) seconds

Related issues

Footnotes

  1. VsockAddressTest.testGetLocalCID crashes in jammy with Swift 5.9.1 #2585

  2. https://opensource.apple.com/source/xnu/xnu-7195.50.7.100.1/tests/vsock.c.auto.html

  3. https://github.com/torvalds/linux/blob/d2f51b3516dade79269ff45eae2a7668ae711b25/net/vmw_vsock/hyperv_transport.c#L437-L440

  4. https://github.com/torvalds/linux/blob/d2f51b3516dade79269ff45eae2a7668ae711b25/net/vmw_vsock/vsock_loopback.c#L24-L27

  5. https://github.com/torvalds/linux/blob/d2f51b3516dade79269ff45eae2a7668ae711b25/net/vmw_vsock/virtio_transport.c#L79-L95

  6. https://github.com/torvalds/linux/blob/d2f51b3516dade79269ff45eae2a7668ae711b25/drivers/misc/vmw_vmci/vmci_guest.c#L88-L100

Copy link
Contributor

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catches and thanks for the detailed explanation. Those changes make sense to me!

@simonjbeaumont simonjbeaumont marked this pull request as ready for review November 24, 2023 14:24
@simonjbeaumont simonjbeaumont enabled auto-merge (squash) November 24, 2023 14:24
@simonjbeaumont simonjbeaumont merged commit 703ee91 into apple:main Nov 24, 2023
6 of 8 checks passed
@glbrntt glbrntt added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VsockAddressTest.testGetLocalCID crashes in jammy with Swift 5.9.1
4 participants