WIP: AF_VSOCK support #8

avnik · 2024-09-09T11:48:53Z

Initial support of AF_VSOCK sockets (vsocks are sockets for virtual machines to host communication).

It basically works, so I like hear some feedback.

Some opinionated things done -- ~~I dropped unused derived traits on ListenerAddr (Eq, Ord and Hash) -- main reason was a VsockAddr,, which is not support Ord/Hash~~.

Some things still pending to do:

~~Solve import of VsockAddr -- importing it from nix cause version conflict with tonic, importing straight from tokio_vsock make it impossible to build without it.~~
Test build on platform not support vsock (I am not super expert in rust's conditional compilation)
Only tonic012 adapter is implemented (and tested, I test everything on tonic app) -- others need to be implemented
Clap integration not implemented

Feedback and suggestions welcomed

vi · 2024-09-09T14:35:07Z

WIP:

On Github, maybe draft pull requests can be used for this? I see "Still in progress? [Convert to draft]".

I dropped unused derived traits on ListenerAddr ...

What if somebody wants to manage many listener addresses in a Hash/BTreeSet?

What is a VSOCK address? Is it some byte buffer or a string or a number?

According to vsock(7), "A socket address is defined as a combination of a 32-bit Context Identifier (CID) and a 32-bit port number.", which looks comparable and sortable.

What is a best way to test that support locally? Is socat - VSOCK-CONNECT:<cid>:<port> enough or a real VM should be used?

vi · 2024-09-09T14:40:34Z

src/connection.rs

@@ -34,6 +37,8 @@ impl std::fmt::Debug for Connection {
            ConnectionImpl::Tcp(_) => f.write_str("Connection(tcp)"),
            #[cfg(all(feature = "unix", unix))]
            ConnectionImpl::Unix(_) => f.write_str("Connection(unix)"),
+            #[cfg(all(feature = "vsock", target_os = "linux"))]


Shall android be also added?

Custom kernels may enable AF_VSOCK.

#[cfg(any(linux_android, apple_targets))] used in nix for AF_VSOCK definition

And VsockAddr defined here https://github.com/nix-rust/nix/blob/master/src/sys/socket/addr.rs#L2049 (It have Hash, but not Ord).
If I need to have own VsockAddr struct, where better to have it?

#[cfg(any(linux_android, apple_targets))] used in nix for AF_VSOCK definition

That means something similar should be available in tokio-listener.

If vsock and tokio-vsock supports a platform, tokio-listener should as well.

nix have following aliases, https://github.com/nix-rust/nix/blob/master/build.rs#L21
And vsock exported on any(android_linux, apple_targets).

Should I bring similar aliases, or just use any(target_os = "android", target_os="linux", target_os="macos")?
(that set which I have in not-pushed-yet development state at the moment)

Bringing in cfg_aliases! (and build.rs) is probably too much for this. Let's wait when such feature appears in Rust proper.

The simple (latter) one looks better.

src/listener_address.rs

vi · 2024-09-09T15:03:36Z

Clap integration not implemented

Are there any unusual vsock-specific options that need a separate command line flags?

Primary way to users to access vsock listener is expected to be the FromStr implementaiton.

The only clap thing that comes to mind is a allowing overriding CIDs from VMADDR_CID_ANY using a separate option, like in socat, leaving main address specifier with just a port, i.e. vsock:1234 instead of vsock:0:1234.

That would somewhat deviate from TCP's obligatory address.

vi · 2024-09-09T15:33:18Z

And VsockAddr defined here https://github.com/nix-rust/nix/blob/master/src/sys/socket/addr.rs#L2049 (It have Hash, but not Ord).
If I need to have own VsockAddr struct, where better to have it?

ListenerAddress should represent what is typed by user, i.e. if user specifies a port number then the variant should carry a number; if user specifies a CID and a port number, then the variant should carry two numbers.

It is also expected to be portable, i.e. you should be able to specify a ListenerAddress on unsupported platform (e.g. on Windows), serialize it, deserialize it on Linux, then bind to it. Note the absence of #[cfg]s on the variants. tokio-listener should recognize user's attempt to use valid, but unsupported address and show the appropriate error message.

Less portable things may be in SomeSocketAddr[Clonable], where address of incoming client is expected to appear

avnik · 2024-09-10T11:25:10Z

@vi correct.
So it could be Vsock((u32, u32)) or

// where
#[derive(Debug, Clone, Eq, Ord, Hash)]
pub enum VSockAddr {
  pub cid: u32,
  pub port: u32,
}
// provide instances for From/Into<tokio_vsock::VsockAddr>

So I would personally prefer last one, because I like to have
ListenerAddress::Vsock(addr.into()) from tokio::VsockAddr.

Regarding parsing from string -- I have use following parser in my code https://github.com/avnik/ghaf-givc/blob/avnik/listeners/src/utils/vsock.rs (not claiming as universal solution)

avnik · 2024-09-11T13:52:24Z

Okay, I refactor ListenerAddress::Vsock as simple pair of u32, also implement parse of vsock:cid:port (like vsock:42:42).
Any other suggestions of improvements?

vi · 2024-09-11T15:05:10Z

following parser in my code https://github.com/avnik/ghaf-givc/blob/avnik/listeners/src/utils/vsock.rs

It maybe something like that, but those aliases (any, local, host) should probably be inlined into ListenerAddress::from_str itself, not be dependent on a third-party code.

So it could be Vsock((u32, u32)) or (big code block)

Maybe Vsock { port: u32, cid: u32 }, variant can be a middle ground between a dedicated type and a pair of easy to confuse numbers?

vi · 2024-09-11T15:12:33Z

Any other suggestions of improvements?

Documentation should be extended. Pair of CID and port is not enough - it should explain a bit what is it about and maybe give a link to vsock(7) manpage.

Also there should be a snippet in Clap documentation comment (duplicated for positional and -L variants) - unlike in the main doccomment, it can be short there. But keep in mind that it will be present everywhere, not just on Linux, so it should not be confusing to users of other platforms.

Speaking of the platform support: what about Android and Mac? As far as I understand, tokio-vsock support more than just Linux.

What about socket options? Are recv_buffer_size / send_buffer_size relevant for AF_VSOCK?

avnik · 2024-09-11T15:32:10Z

Any other suggestions of improvements?

Documentation should be extended. Pair of CID and port is not enough - it should explain a bit what is it about and maybe give a link to vsock(7) manpage.

Documentation would be expanded as well (with mention that it works on linux/android, and probably on darwin).

Also there should be a snippet in Clap documentation comment (duplicated for positional and -L variants) - unlike in the main doccomment, it can be short there. But keep in mind that it will be present everywhere, not just on Linux, so it should not be confusing to users of other platforms.

Speaking of the platform support: what about Android and Mac? As far as I understand, tokio-vsock support more than just Linux.

working on change conditions on all(any(linux_androd, apple_systems), feature="vsock")) (same as in nix for AF_VSOCK definitions).

But I test everything only on linux now, I haven't apple/windows machines here.

What about socket options? Are recv_buffer_size / send_buffer_size relevant for AF_VSOCK?

Would investigate if it possibe

Thanks for feedback

avnik · 2024-09-11T15:35:05Z

following parser in my code https://github.com/avnik/ghaf-givc/blob/avnik/listeners/src/utils/vsock.rs

It maybe something like that, but those aliases (any, local, host) should probably be inlined into ListenerAddress::from_str itself, not be dependent on a third-party code.

Constants are platform dependent (in nix or tokio_vsock), should we just use magic numbers for them?

So it could be Vsock((u32, u32)) or (big code block)

Maybe Vsock { port: u32, cid: u32 }, variant can be a middle ground between a dedicated type and a pair of easy to confuse numbers?

Looks good for me

vi · 2024-09-11T15:47:32Z

Constants are platform dependent (in nix or tokio_vsock), should we just use magic numbers for them?

Do they differ significantly (other numbers between Darwin and Linux) or just defined in a cfg-ed way?

If the latter then maybe just hard code them for now. If the former then maybe we should omit aliases, as they would change meaning if one serializes+deserializes ListenerAddress across platforms.

avnik · 2024-09-12T23:15:37Z

Constants are platform dependent (in nix or tokio_vsock), should we just use magic numbers for them?

Do they differ significantly (other numbers between Darwin and Linux) or just defined in a cfg-ed way?

cfg-ed for all 3 platforms, but equal (linux have little bit more comments inside)

libc/src/unix/linux_like/android/mod.rs
2976:pub const VMADDR_CID_ANY: ::c_uint = 0xFFFFFFFF;
2977:pub const VMADDR_CID_HYPERVISOR: ::c_uint = 0;
2978:pub const VMADDR_CID_LOCAL: ::c_uint = 1;
2979:pub const VMADDR_CID_HOST: ::c_uint = 2;
2980:pub const VMADDR_PORT_ANY: ::c_uint = 0xFFFFFFFF;

So we can borrow these values as pub (crate), and ensure in tests that they equal appropriate libc values

avnik · 2024-09-21T10:43:06Z

@vi I addressed most of issues. Please re-review

Last pending thing -- VsockConnectionInfo for tonic. But it require tokio-vsock with tonic-support feature, I didn't know how require it properly in cargo.toml in case if both tonic012 and vsock requested in combo. Have you any suggestion here?

avnik · 2024-09-23T18:12:49Z

Okay, I added tonic integration, but it require

[patch.crates-io]
tokio-vsock = { git = "https://github.com/rust-vsock/tokio-vsock" }

to be added into app's Cargo.toml.
(because tonic version in tokio-vsock bumped only on master)

I didn't touch tonic010, tonic011 because I could test only 0.12 (my app use 0.12)

vi · 2024-09-23T18:23:25Z

I'm not sure a [patch.crates-io] is a good long-term idea, especially on published crates.

I'm not even sure such crates are publishable on crates.io - they should only depend on another crates.io crates.

vi · 2024-09-23T18:30:37Z

But it require tokio-vsock with tonic-support feature

What Tonic has to do with Tokio support for VSOCK?

Why does tokio_vsock ~~non-optionally~~ depends on Tonic? ~~Something feels wrong in the whole architecture of dependencies.~~

Maybe if Tonic over VSOCK is a popular use case then providing a helper (and reusing it for tokio-listener) directly in tokio-vsock may be reasonable idea. If it is an exotic use case then tokio-vsock can be kept simpler, with only tokio-listener handing the tonic::transport::server::Connected impl.

avnik · 2024-09-23T18:38:09Z

But it require tokio-vsock with tonic-support feature

What Tonic has to do with Tokio support for VSOCK?

Why does tokio_vsock ~~non-optionally~~ depends on Tonic? Something feels wrong in the whole architecture of dependencies.

VsockConnectionInfo for tonic is also conditional in tonic-vsock
(https://github.com/rust-vsock/tokio-vsock/blob/master/src/tonic_support.rs -- it cfg-ed in main lib.rs)

vi · 2024-09-23T18:40:08Z

But it require tokio-vsock with tonic-support feature, I didn't know how require it properly in cargo.toml in case if both tonic012 and vsock requested in combo. Have you any suggestion here?

One of the way is just avoid relying on tokio_vsock::VsockConnectInfo and provide (e.g. duplicate) similar helper within tokio-listener. Do I correctly understand that it is a little part without any tricky logic, it just connects one thing to another?

avnik · 2024-09-23T18:40:41Z

I'm not sure a [patch.crates-io] is a good long-term idea, especially on published crates.

I'm not even sure such crates are publishable on crates.io - they should only depend on another crates.io crates.

Yep, I used it for make app buildable.
(probably nudge author to release new version with bumped tonic is better idea)

avnik · 2024-09-23T18:44:10Z

But it require tokio-vsock with tonic-support feature, I didn't know how require it properly in cargo.toml in case if both tonic012 and vsock requested in combo. Have you any suggestion here?

One of the way is just avoid relying on tokio_vsock::VsockConnectInfo and provide (e.g. duplicate) similar helper within tokio-listener. Do I correctly understand that it is a little part without any tricky logic, it just connects one thing to another?

I could borrow/inline this code if you with (adding FIXME note to remove duplication, when new version would be released)

vi · 2024-09-23T18:49:01Z

src/claptools.rs

@@ -29,6 +29,9 @@ pub struct ListenerAddressPositional {
    ///
    /// * UNIX socket path like /tmp/mysock or Linux abstract address like @abstract
    ///
+    /// * AF_VSOCK socket in format vsock:cid:port, like vsock:42:8080
+    ///   Refer vsock manual for details: https://www.man7.org/linux/man-pages/man7/vsock.7.html


Maybe you meant more spaces before Refer for it to render nicely.

Here how it currently looks:

I expect you intended it to look other way.

If you run cargo doc --all-features you also see some warning regarding the unformatted link. Note that there are currently a lot of other, unrelated doc warnings.

Note that in this case you should also care how it would look in console, so a balance between Markdown-ifying and just inhibiting relevant warnings may be needed.

Here probably should be a dot before "refer" word, but it easily could be mistaken as part of syntax

Maybe that part is too lengthy for a CLI help message anyway, so it may be worth to keep it like that:

AF_VSOCK socket in format vsock:cid:port. Example: vsock:any:8080

There is no reference to e.g. unix(7) or inet(7) anyway nearby.

src/listener.rs

vi · 2024-09-23T19:14:19Z

Note: currently it fails the cargo hack --feature-powerset check test.

You can use it (combined with e.g. --target=x86_64-pc-windows-gnu to also include a platform without AF_VSOCK support) to find you if all those #[cfg]s are OK.

vi · 2024-09-23T19:16:49Z

src/listener_address.rs

+    /// [vsock manual]: https://www.man7.org/linux/man-pages/man7/vsock.7.html
+    /// (Implemented only on Linux and Darwin)
+    Vsock {
+        cid: u32,


Documentation detailing what specifically cid and port mean can be moved here. Otherwise it triggers the #[warn(missing_docs)] lint.

avnik · 2024-09-23T19:18:59Z

Note: currently it fails the cargo hack --feature-powerset check test.

You can use it (combined with e.g. --target=x86_64-pc-windows-gnu to also include a platform without AF_VSOCK support) to find you if all those #[cfg]s are OK.

Would take a look. I really tested only on linux (because haven't other platforms, but I would ask someone to try build on darwin and windows as well)

vi · 2024-09-23T19:25:17Z

src/some_socket_addr.rs

@@ -27,6 +30,8 @@ impl Display for SomeSocketAddr {
            SomeSocketAddr::Unix(_x) => "unix".fmt(f),
            #[cfg(feature = "inetd")]
            SomeSocketAddr::Stdio => "stdio".fmt(f),
+            #[cfg(all(any(target_os = "linux", target_os = "android", target_os = "macos"), feature = "vsock"))]
+            SomeSocketAddr::Vsock(x) => x.fmt(f),


It would produce something like cid: {} port: {} (without any direct indication that it is VSOCK). Is it OK?

Done as "vsock:{cid}:{port}"

vi · 2024-09-23T19:27:56Z

src/tonic010.rs

@@ -31,6 +31,10 @@ impl Connected for Connection {
        if self.try_borrow_stdio().is_some() {
            return ListenerConnectInfo::Stdio;
        }
+        #[cfg(feature = "vsock")]
+        if let Some(vsock) = self.try_borrow_vsock() {
+            return ListenerConnectInfo::Vsock(vsock.connect_info())


ListenerConnectInfo::Vsock does not currently exist (and it fails compilation), also #[cfg] omits platform check.

Are you sure we need to also add a special support for legacy tonic 0.10? Maybe just let it fall back to Other below?

Yup, tonic010 was added by mistake. I plan to add support 0.12+ only (if you haven't mind). Otherwise it was too much copy-paste for inlining VsockConnectInfo

vi · 2024-09-23T19:30:09Z

src/tonic012.rs

-use tonic_012::transport::server::{Connected, TcpConnectInfo};
+
+#[cfg(all(any(target_os = "linux", target_os = "android", target_os = "macos"), feature = "vsock"))]
+use tokio_vsock::{VsockStream, VsockConnectInfo};


(see note about duplicating the VsockConnectInfo here in the main discussion)

Inlined that code as vsock_support module at end of file

vi · 2024-09-23T20:05:03Z

Please re-review.

Added the comments.

Also ensured cargo hack --feature-powerset check --exclude-features tonic010,tonic011,tonic012 runs without problems.

cargo check --target x86_64-pc-windows-gnu --features vsock fails, not being able to compile vsock.

Maybe the dependency needs to be specified like this:

[target."cfg(any(target_os = \"linux\", target_os = \"android\", target_os = \"macos\"))".dependencies]
tokio-vsock = {version = "0.5", optional=true}

vi · 2024-09-23T20:06:49Z

I really tested only on linux (because haven't other platforms, but I would ask someone to try build on darwin and windows as well)

You can check most of compilability without a tricky setup, just by installing libstd for that target. cargo check --target=... should just work. To make it actually produce executables, you'll also need to set up a linker.

vi · 2024-09-23T20:21:10Z

I could borrow/inline this code if you with [sic]

Assuming "wish".

(adding FIXME note to remove duplication, when new version would be released)

I don't understand what ecosystem fixes are expected here. Part of the reason is to avoid tricky conditional feature setup in Cargo.toml (if such setup is even possible).

The more I think about that (e.g. what would happen when new features will appear in tokio-vsock proper and when new Tonic versions would appear), the more I think that native tokio-vsock's Tonic support should be avoided. I.e. AF_VSOCK worlds and Tonic worlds should be kept apart and only meet within tokio-listener in a controllable way (providing e.g. support for legacy Tonic versions if needed).

Unfortunately, that means Tonic apps that really need to know what CID and port connects to them would need a bit deeper migration to/from tokio-vsock/tonic-conn from/to tokio-listener/vsock+tonic. But in return, if tokio-vsock would lag behind Tonic versions, it won't be a problem if tokio-listener is still getting updated.

avnik · 2024-09-24T10:15:48Z

I could borrow/inline this code if you with [sic]

Assuming "wish".

(adding FIXME note to remove duplication, when new version would be released)

I don't understand what ecosystem fixes are expected here. Part of the reason is to avoid tricky conditional feature setup in Cargo.toml (if such setup is even possible).

Unfortunately, that means Tonic apps that really need to know what CID and port connects to them would need a bit deeper migration to/from tokio-vsock/tonic-conn from/to tokio-listener/vsock+tonic. But in return, if tokio-vsock would lag behind Tonic versions, it won't be a problem if tokio-listener is still getting updated.

Well, in our app we plan use this feature exactly for this -- extra authentication, some requests should be allowed only from host, or dedicated guest vm.

vi · 2024-09-24T10:53:38Z

Do I correctly assume that a VSOCK address is just a pair of numbers without any tricky things, so any options should be easy enough for app developers - copy those numbers from one place to another and call it a day?

avnik · 2024-09-25T10:20:13Z

cargo hack --feature-powerset check --exclude-features tonic010,tonic011,tonic012

~~Is it newer version of cargo, or additional tool?~~
Nevermind, found it as additional tool

Signed-off-by: Alexander V. Nikolaev <[email protected]>

vi · 2024-09-26T16:23:47Z

Cargo.toml

@@ -35,6 +35,7 @@ tracing = "0.1.37"
 axum07 = { version="0.7", package="axum", optional=true }
 futures-util = {version="0.3", optional=true}
 tokio-util = {version = "0.7", optional=true, features=["net","codec"]}
+tokio-vsock = {version = "0.5", optional=true}


tokio-vsock does not seem to build on Windows, which breaks --all-features build.

Somewhere else in the comments there is a snippet about how to include it conditionally.

Strange, because diff started with suggested snippet in Cargo.toml

avnik · 2024-09-26T16:46:36Z

@vi updated a branch. cargo check/cargo hack check now pass for both linux and win32, vsock connection info code inlined (and slightly rewritten)

vi · 2024-12-25T01:36:07Z

@avnik , What is current status of the pull request?

Is it marked as a draft because of you are not yet sure it is complete enough to be mergeable?

avnik · 2025-01-12T16:17:15Z

@vi it finished, I waited until someone test it on windows. But looks like you merged it anyway, thanks

vi reviewed Sep 9, 2024

View reviewed changes

src/listener_address.rs Outdated Show resolved Hide resolved

avnik marked this pull request as draft September 9, 2024 14:47

avnik force-pushed the avnik/vsock branch from 569b673 to c9ca624 Compare September 20, 2024 13:45

vi reviewed Sep 23, 2024

View reviewed changes

src/listener.rs Outdated Show resolved Hide resolved

vi reviewed Sep 23, 2024

View reviewed changes

avnik added 5 commits September 26, 2024 16:45

AF_VSOCK support

734e721

Signed-off-by: Alexander V. Nikolaev <[email protected]>

Use field for vsock address, add docs

af0f297

Signed-off-by: Alexander V. Nikolaev <[email protected]>

Add documentation to clap structs

a03f400

Signed-off-by: Alexander V. Nikolaev <[email protected]>

Tonic support

512acd0

Signed-off-by: Alexander V. Nikolaev <[email protected]>

Inline vsock connection info code

34cb172

Signed-off-by: Alexander V. Nikolaev <[email protected]>

vi reviewed Sep 26, 2024

View reviewed changes

avnik force-pushed the avnik/vsock branch from d4b8ecc to 34cb172 Compare September 26, 2024 16:44

WIP: AF_VSOCK support #8

Are you sure you want to change the base?

WIP: AF_VSOCK support #8

Conversation

avnik commented Sep 9, 2024 • edited Loading

vi commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vi commented Sep 9, 2024 • edited Loading

vi commented Sep 9, 2024 • edited Loading

avnik commented Sep 10, 2024

avnik commented Sep 11, 2024

vi commented Sep 11, 2024

vi commented Sep 11, 2024 • edited Loading

avnik commented Sep 11, 2024

avnik commented Sep 11, 2024

vi commented Sep 11, 2024 • edited Loading

avnik commented Sep 12, 2024

avnik commented Sep 21, 2024

avnik commented Sep 23, 2024

vi commented Sep 23, 2024

vi commented Sep 23, 2024 • edited Loading

avnik commented Sep 23, 2024

vi commented Sep 23, 2024

avnik commented Sep 23, 2024

avnik commented Sep 23, 2024

vi Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vi Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

vi commented Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

avnik commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vi commented Sep 23, 2024

vi commented Sep 23, 2024

vi commented Sep 23, 2024

avnik commented Sep 24, 2024

vi commented Sep 24, 2024 • edited Loading

avnik commented Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avnik commented Sep 26, 2024

vi commented Dec 25, 2024 • edited Loading

avnik commented Jan 12, 2025

avnik commented Sep 9, 2024 •

edited

Loading

vi commented Sep 9, 2024 •

edited

Loading

vi commented Sep 9, 2024 •

edited

Loading

vi commented Sep 11, 2024 •

edited

Loading

vi commented Sep 11, 2024 •

edited

Loading

vi commented Sep 23, 2024 •

edited

Loading

vi Sep 23, 2024 •

edited

Loading

vi Sep 23, 2024 •

edited

Loading

vi commented Sep 23, 2024 •

edited

Loading

vi commented Sep 24, 2024 •

edited

Loading

avnik commented Sep 25, 2024 •

edited

Loading

vi commented Dec 25, 2024 •

edited

Loading