feat: Add Seqpacket support#5595
Open
aerosouund wants to merge 18 commits intofirecracker-microvm:mainfrom
Open
feat: Add Seqpacket support#5595aerosouund wants to merge 18 commits intofirecracker-microvm:mainfrom
aerosouund wants to merge 18 commits intofirecracker-microvm:mainfrom
Conversation
aerosouund
force-pushed
the
vsock
branch
22 times, most recently
from
9cb7f64 to
4988a5e
Compare
aerosouund
requested review from
Manciukic,
kalyazin,
pb8o and
xmarcalx
as code owners
Contributor
|
Hi @aerosouund, I'm in the process of checking through the full PR again, since it's been a while since I've looked at it. Just a note, I checked out your PR and the tests did not run. It seems like the 'vsock_type' field you introduced is missing from one of the constructors in a test. Could you please fix this in the meantime? Thanks |
Contributor
Author
|
@JamesC1305 done |
aerosouund
force-pushed
the
vsock
branch
3 times, most recently
from
0f323e3 to
c162215
Compare
Contributor
Author
|
@JamesC1305 fixed the merge conflicts |
JamesC1305
reviewed
Apr 24, 2026
Contributor
JamesC1305
left a comment
JamesC1305
left a comment
There was a problem hiding this comment.
Apologies for the delay in getting back to you, it has been a busy few weeks for us. I've given an initial pass to your PR. I suggest you go through commit-by-commit to read my comments, as that is how I wrote them.
For future revisions, could you please restructure the commits? This PR is very hard to follow at 31 commits. There are some points where you introduce bugs and fix them in a later commit. We want to avoid that scenario entirely, by fixing the bug at the source. Additionally, there are sometimes changes in unrelated commits, which makes it hard to see the final state of the code. If you could try to make everything as self-contained as possible that would be great, because it's quite difficult to follow as is.
Regardless, I've left some feedback on the lower-level implementation details. Once the low-hanging fruit is resolved, we can discuss the architectural flow.
The module has two types. SeqpacketConn and SeqpacketListener. Calling accept() on the listener yields a conn. The types wrap libc calls that open, bind to, read from seqpacket sockets and implements traits required by connection consumers. Signed-off-by: aerosouund <aerosound161@gmail.com>
The enum forwards any calls made from the functions defined in the VsockConnectionBackend trait to the backing connection. the types are stream and seqpacket based connections. Signed-off-by: aerosouund <aerosound161@gmail.com>
Can be a seqpacket or stream. Refactor vsock.rs methods to expect it in function calls and use it in building structs. add a VSOCK_TYPE_SEQPACKET constant to denote seqpacket connections Signed-off-by: aerosouund <aerosound161@gmail.com>
- Make the multiplexer use a VsockConnection<ConnBackend> for its conn_map values to forward calls to stream or seqpacket types. - Make the host sock a boxed implementation of the Socket trait which exposes the accept() method with different implementations for stream and seqpacket connections. - Globally replace the MuxerConnection type with a VsockConnection generic over a ConnBackend. - Make the init_pkt_hdr method add different type constants based on the vsock type. - pass vsock type to read_local_stream_port. seqpacket sockets necessitate that you read the message in one go. Signed-off-by: aerosouund <aerosound161@gmail.com>
Add a new field to the vsock api and modify functions that interact with a VsockDeviceConfig or a VsockUnixBackend. Signed-off-by: aerosouund <aerosound161@gmail.com>
- incoming length is a trait with a default implementation to read the size of an incoming message from a connection fd - read result stores the count of bytes read and whether or no another receive should be triggered - change the VsockChannel trait to return a ReadResult instead of null Signed-off-by: aerosouund <aerosound161@gmail.com>
- create a set_msg_eom function to set eom in the packet header - create a set_msg_eor function to set eor in the packet header Signed-off-by: aerosouund <aerosound161@gmail.com>
- make it start another loop iteration if it received a result with should_retrigger Signed-off-by: aerosouund <aerosound161@gmail.com>
- add connection_buffer and conn_buffer_wrote_bytes fields to the connection - when there's a seqpacket connection with incoming data check the length of the incoming message - if its small enough to fit in the packet buffer (1 memory page size) receive directly from the af_unix to the buffer - if its bigger, receive into a buffer then copy whatever you can fit into the buffer and mark that we should retrigger - if the buffer has data, don't read from the stream. iterate until you finish the buffer - don't use intermediate rx buffering for stream connections - if the incoming message is larger than the connection buffer we return VsockError::MessageTooLong Signed-off-by: aerosouund <aerosound161@gmail.com>
- propagate conn_buffer_size to connections on creation to determine how big the rx intermediate buffer should be - if the result coming from the connection has should_retrigger push to the rx queue to mark that there's still data to be read from the buffer Signed-off-by: aerosouund <aerosound161@gmail.com>
- the setting should be persisted across snapshot and restore so the new vsock buffers have the same size - prevent the caller from passing too large buffers. 256 is also the limit set by the linux kernel - prevent the caller from passing too small buffers (less than 4096) bytes. - make the field default to none and add default serialization Signed-off-by: aerosouund <aerosound161@gmail.com>
- seqpacket sockets necessitate a different flag set than whats currently in the seccomp rules. we need to add seqpacket with SOCK_CLOEXEC for testing to work Signed-off-by: aerosouund <aerosound161@gmail.com>
- the functional tests use an echo server which is just a socat command. socat doesn't support seqpacket. - the program listens on a port and handles a connection in a separate thread Signed-off-by: aerosouund <aerosound161@gmail.com>
- can be stream or seqpacket. to facilitate seqpacket testing Signed-off-by: aerosouund <aerosound161@gmail.com>
- create a fixture to compile the vsock_seq_server program - create start_seqpacket_echo_server to run the program in the vm - create the check_guest_connections_seqpacket function to start a host side seqpacket server and connect to it from the guestn and check data hash - create h2g, g2h and overflow functional tests - run seqpacket tests on kernel 6.x only because seqpacket was introduced in linux 5.14. any previous version would fail Signed-off-by: aerosouund <aerosound161@gmail.com>
- so that the api anticipates it Signed-off-by: aerosouund <aerosound161@gmail.com>
add changelog entry for the modified field Signed-off-by: aerosouund <aerosound161@gmail.com>
- Added to the unreleased section Signed-off-by: aerosouund <aerosound161@gmail.com>
Contributor
Author
|
@JamesC1305 |
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.
Changes
There's currently no support for seqpacket based vsock connections in firecracker.
This PR adds it by creating an enum
ConnBackendthat implementsVsockConnectionBackendand make its variants a unix stream or a seqpacket connection. The actual connection is handled by a module for seqpacket based connection that leverages libc calls.It adds
vsock_typeas an api parameter to the vsock type.Reason
Fixes: #4822
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.