Want virtio-vsock support

[papertigers]

Fixes: #969

[github-actions]

license-eye has checked 350 files.

Valid	Invalid	Ignored	Fixed
262	1	87	0

Click to see the invalid file list

• lib/propolis/src/vsock/poller_stub.rs

Use this command to fix any missing license headers

```bash

docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix

</details>

[papertigers]

There will be a few follow up commits but I wanted to get this up for some review time while that stuff is in progress.

cc @dancrossnyc if you have review cycles.

[hawkw]

isn't that what this PR does?

Suggested change

// XXX MTZ: add the vsock device

[hawkw]

is this a fix for a previous existing bug?

[hawkw]

nit, take it or leave it: it seems like Data doesn't need to be an owned Vec here, since we're just copy_from_sliceing its contents; i think this could be:

Suggested change

	pub fn push(&mut self, data: Vec<u8>) -> Result<(), VsockBufError> {
	pub fn push(&mut self, data: impl AsRef<[u8]>) -> Result<(), VsockBufError> {
	let data = data.as_ref();

that way, callers can call it with an owned Vec or an &Vec<u8> or a slice into a vec or anything you want.

[hawkw]

nit: a cute little trick @cbiffle showed me for slicing something to offset..len is that you can alternatively write:

Suggested change

	self.buf[head_offset..head_offset + data.len()]
	self.buf[head_offset..][...data.len()]

[dancrossnyc]

Oh that's cute. I assume that compiles down to the same code as the first version?

[hawkw]

Sadly, it's actually slightly worse (per this Godbolt), as the version with two slice indexing expressions generates two unique panic sites (as they have slightly different source locations).

Sigh.

[hawkw]

teensy, unimportant nit:

Suggested change

	// If the data can fit in the remaining space of the ring buffer copy it
	// in one go.
	// If the data can fit in the remaining space of the ring buffer, copy it
	// in one go.

[hawkw]

nit, not too important: perhaps we ought to link to someplace like https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-39000010 as a reference for the layout of these headers?

[hawkw]

this is somewhat coming out of left field and it's not super important, but it occurs to me that we might consider using the zerocopy crate for these structures. obviously, the biggest reason to reach for it is that it would allow us to write directly into a byte buffer or read fields out of that buffer in a structured way without having to copy all the bytes around (which may not really have a huge performance impact here) but it also has a rather neat zerocopy::byteorder module for defining structures of integers with explicit endiannesses which might make some of this code a bit simpler. on the other hand, it might just be a bunch of effort that isn't really worth it here. i dunno.

[dancrossnyc]

I like this suggestion. The need for packed here at the moment is unfortunately, but Zerocopy handles the padding issue. My count is that this struct is 44 bytes long, but aligned to an 8-byte boundary, which means space at the end. Ugh.

[hawkw]

nit, take it or leave it: perhaps we ought to explicitly log the unknown port here too?

[hawkw]

since this is zero-initialized and we never expect to grow it (i think) we might consider

Suggested change

	let mut read_buf = vec![0u8; 1024 * 64];
	let mut read_buf: Box<[u8]> = vec![0u8; 1024 * 64].into();

as it is often very slightly faster to index into. for proof, try running this playground in release mode a few times:¹

     alloc::vec::Vec<u8>: 90ns
            [u8; 1000000]: 50ns
  alloc::boxed::Box<[u8]>: 60ns

Footnotes

1. the timings are a bit variable but the vec is pretty consistently 30-50ns slower... ↩

[hawkw]

maybe worth commentary on some of these explaining why they're wrapping?

[dancrossnyc]

Did you intend to leave this in, but commented-out?

[dancrossnyc]

I know this is commented out, but I think this'd be fine as #[repr(transparent)].

[dancrossnyc]

In general, #[repr(C)] (without packed) ought to be ok for these headers. We know the target this runs on, and its ABI, unless you're specifically worried about alignment on a single byte boundary?

[dancrossnyc]

Why not make this a real enum?

#[derive(Clone, Copy, Debug)]
#[repr(u16)]
pub enum VsockPacketOp {
    Request = 1,
    Response = 2,
    Reset = 3,
    Shutdown = 4,
    ReadWrite = 5,
    UpdateCredit = 6,
    RequestCredit = 7,
}

impl TryFrom<u16> for VsockPacketOp {
    type Error = u16;
    fn try_from(raw: u16) -> Result<Self, Self::Error> {
        match raw {
            1 => Ok(Self::Request),
            2 => Ok(Self::Response),
            3 => Ok(Self::Reset),
            4 => Ok(Self::Shutdown),
            5 => Ok(Self::ReadWrite),
            6 => Ok(Self::UpdateCredit),
            7 => Ok(Self::RequestCredit),
            _ => Err(raw),
        }
    }
}

impl From<VsockPacketOp> for u16 {
    fn from(op: VsockPacketOp) -> u16 {
        op as u16
    }
}

[dancrossnyc]

For these, consider using bitflags::bitflags! or something similar?

[dancrossnyc]

Similarly, this could also just be an enum:

Suggested change

	type VsockSocketType = u16;
	#[derive(Clone, Copy, Debug)]
	#[repr(u16)]
	pub(crate) enum SocketType {
	Stream = 1,
	SeqPacket = 2,
	}
	impl TryFrom<u16> for SocketType {
	type Error = u16;
	fn try_from(raw: u16) -> Result<Self, Self::Error> {
	match raw {
	1 => Ok(Self::Stream),
	2 => Ok(Self::SeqPacket),
	_ => Err(raw),
	}
	}
	}
	impl From<SocketType> for u16 {
	fn from(sock_type: SocketType) -> u16 {
	sock_type as u16
	}
	}

[dancrossnyc]

There's a crate called bit_field that can help with this: https://docs.rs/bit_field/latest/bit_field/

You can write something like:

use bit_field::BitField;

    pub fn src_cid(&self) -> u64 {
        u64::from_le(self.src_cid).get_bits(0..32)
    }

[dancrossnyc]

Does this actually need to own a Vec? Or could it be a Box<[u8]> or something?

[dancrossnyc]

You appear to set every field in the struct, so using ::default() to create a mut struct and then a change of .set_... feels like it could just be:

    pub fn new_reset(guest_cid: u32, src_port: u32, dst_port: u32) -> Self {
        VsockPacketHeader {
            src_cid: VSOCK_HOST_CID as u32,
            dst_cid: guest_cid,
            src_port,
            dst_port,
            len: 0,
            socket_type: VIRTIO_VSOCK_TYPE_STREAM,
            op: VIRTIO_VSOCK_OP_RST,
            buf_alloc: 0,
            fwd_cnt: 0,
        }
    }

Or, given the enums above, this could perhaps be:

    pub fn new_reset(guest_cid: u32, src_port: u32, dst_port: u32) -> Self {
        let dst_cid = guest_cid.into();
        let src_cid = HOST_CID.into();
        let socket_type = SocketType::Stream as u16;
        let op = PacketOp::Reset as u16;
        Self {
            dst_cid,
            src_cid,
            src_port,
            dst_port,
            socket_type,
            op,
            ..Default::default()
        }
    }

For that matter, you could have a (private) "generic" new that takes the op, and delegate the rest to specializations of that:

    pub fn new_reset(guest_cid: u32, src_port: u32, dst_port: u32) -> Self {
        let dst_cid = guest_cid.into();
        let src_cid = HOST_CID.into();
        let socket_type = SocketType::Stream as u16;
        let op = VsockPacketOp::Reset as u16;
        Self {
            dst_cid,
            src_cid,
            src_port,
            dst_port,
            socket_type,
            op,
            ..Default::default()
        }
    }

And similarly with the other constructors, elsewhere.

Or, you could write a "common" private new and implement everything else in terms of that:

    fn new(guest_cid: u32, src_port: u32, dst_port: u32, op: PacketOp) -> Self {
        let dst_cid = guest_cid.into();
        let src_cid = HOST_CID.into();
        let socket_type = SocketType::Stream as u16;
        let op = op as u16;
        Self {
            dst_cid,
            src_cid,
            src_port,
            dst_port,
            socket_type,
            op,
            ..Default::default()
        }
    }

    pub fn new_reset(guest_cid: u32, src_port: u32, dst_port: u32) -> Self {
        Self::new(guest_cid, src_port, dst_port, PacketOp::Reset)
    }

    pub fn new_response(guest_cid: u32, src_port: u32, dst_port: u32, buf_alloc: u32) -> Self {
        Self {
            buf_alloc,
            ..Self::new(guest_cid, src_port, dst_port, PacketOp::Response)
        }
    }

    pub fn new_rw(
        guest_cid: u32,
        src_port: u32,
        dst_port: u32,
        buf_alloc: u32,
        fwd_cnt: u32,
        data: &[u8],
    ) -> Self {
        let len = data.len() as u32;
        Self {
            len,
            buf_alloc,
            fwd_cnt,
            ..Self::new(guest_cid, src_port, dst_port, PacketOp::ReadWrite)
        }
    }

    pub fn new_credit_udpate(
        guest_cid: u32,
        src_port: u32,
        dst_port: u32,
        buf_alloc: u32,
        fwd_cnt: u32,
    ) -> Self {
        Self {
            buf_alloc,
            fwd_cnt,
            ..Self::new(guest_cid, src_port, dst_port, PacketOp::UpdateCredit)
        }
    }

    pub fn new_shutdown(guest_cid: u32, src_port: u32, dst_port: u32, flags: u32) -> Self {
        Self {
            flags,
            ..Self::new(guest_cid, src_port, dst_port, PacketOp::Shutdown)
        }
    }

Or, finally, if new is implemented in terms of explicit initialization of all members, then you can make it const, and make all of the associated constructors similarly const:

Suggested change

	pub fn new_reset(guest_cid: u32, src_port: u32, dst_port: u32) -> Self {
	const fn new(guest_cid: u32, src_port: u32, dst_port: u32, op: PacketOp) -> Self {
	Self {
	dst_cid: guest_cid as u64,
	src_cid: HOST_CID as u64,
	src_port,
	dst_port,
	len: 0,
	socket_type: SocketType::Stream as u16,
	op: op as u16,
	flags: 0,
	buf_alloc: 0,
	fwd_cnt: 0,
	}
	}
	pub const fn new_reset(guest_cid: u32, src_port: u32, dst_port: u32) -> Self {
	Self::new(guest_cid, src_port, dst_port, PacketOp::Reset)
	}
	pub const fn new_response(
	guest_cid: u32,
	src_port: u32,
	dst_port: u32,
	buf_alloc: u32,
	) -> Self {
	Self {
	buf_alloc,
	..Self::new(guest_cid, src_port, dst_port, PacketOp::Response)
	}
	}
	pub const fn new_rw(
	guest_cid: u32,
	src_port: u32,
	dst_port: u32,
	buf_alloc: u32,
	fwd_cnt: u32,
	data: &[u8],
	) -> Self {
	let len = data.len() as u32;
	Self {
	len,
	buf_alloc,
	fwd_cnt,
	..Self::new(guest_cid, src_port, dst_port, PacketOp::ReadWrite)
	}
	}
	pub const fn new_credit_udpate(
	guest_cid: u32,
	src_port: u32,
	dst_port: u32,
	buf_alloc: u32,
	fwd_cnt: u32,
	) -> Self {
	Self {
	buf_alloc,
	fwd_cnt,
	..Self::new(guest_cid, src_port, dst_port, PacketOp::UpdateCredit)
	}
	}
	pub const fn new_shutdown(guest_cid: u32, src_port: u32, dst_port: u32, flags: u32) -> Self {
	Self {
	flags,
	..Self::new(guest_cid, src_port, dst_port, PacketOp::Shutdown)
	}
	}

[dancrossnyc]

To whatever extent possible, I encourage avoiding direct use of libc. In this case, something form nix might be helpful: https://docs.rs/nix/latest/nix/poll/struct.PollFlags.html

@iximeow and I talked about this, and I of course defer to them; but I'll note that nix is already a dependency of propolis, though indirectly.

[iximeow]

my hesitations were (are?) mostly about nix's permissiveness about actual libc calls, I think using the enums from there make a lot of sense either way (they're just a slightly souped up form of this macro though)

[jordanhendricks]

throw in an error message here if no bdf is specified?

[jordanhendricks]

"last_os_error"

[dancrossnyc]

Instead of doing something like this, I'd consider implementing an is_readable method on PollEvents.

impl PollEvents {
    pub const fn is_readable(self, event: Self) -> bool {
        const READABLE: Self = PollEvents::IN | PollEvents::HUP | PollEvents::ERR | PollEvents::PRI;
        READABLE.intersects(event)
    }
}

[iximeow]

(READABLE.intersects(self)?)

[dancrossnyc]

(Similarly here.)

[dancrossnyc]

It's probably not necessary, but I would perhaps consider using an IdOrdMap here to get in on some of that B-Tree cache locality goodness.

[dancrossnyc]

I haven't gone through poller.rs in detail, but that seems like it's where the bulk of the logic lives?

[iximeow]

I was just gonna mention the acc_mem thing because I happened to spot it, and then I started reading more, sorry 😁

I very slightly skimmed the rest but mostly focused on the places we're sending data between guest and host, doing unsafe {}, etc.

"changes requested" because of the acc_mem.access() None handling, the packet validation-before-use, and guest-provided cid truncation bits. the rest are generally suggestions that I think would help but I'm not as pressed about one way or the other.

[iximeow]

if acc_mem.access() returns None then either the memory maps have been taken away or we've gotten to implementing bus mastering and this device has been blocked from accessing memory (which probably shouldn't panic propolis)

the former shouldn't happen since we require devices and vcpus to be quiesced before tearing things down, and the latter won't currently happen because we just don't do that yet. but at the least ought to be "TODO: cannot access memory?". the expect() is fine to me, just don't want to lose track of where we'll need future work.

it'd be worth taking a look at other acc_mem.access() in this change to make sure error handling makes sense w.r.t above.

(you'll find similar todo's all over in other device code 🙂🙁)

[iximeow]

looks like write_rw is an old name, this since became fn write() below? (write_rw is referenced below as well)

[iximeow]

since the expectation is that self.rx_chain is Some by the time we're making RxPermit, would it make sense to actually hold rx_chain in RxPermit and get there like...

Suggested change

	if self.rx_chain.is_none() {
	let mem = self.acc_mem.access()?;
	let vq = self.queues.get(VSOCK_RX_QUEUE as usize)?;
	let mut chain = Chain::with_capacity(10);
	vq.pop_avail(&mut chain, &mem)?;
	self.rx_chain = Some(chain);
	}
	let header_size = std::mem::size_of::<VsockPacketHeader>();
	let available_data_space = self
	.rx_chain
	.as_ref()
	.unwrap()
	.remain_write_bytes()
	.saturating_sub(header_size);
	Some(RxPermit { available_data_space, vq: self })
	let chain = if let Some(chain) = self.rx_chain.take() {
	chain
	} else { /* self.rx_chain.is_none() */
	let mem = self.acc_mem.access()?;
	let vq = self.queues.get(VSOCK_RX_QUEUE as usize)?;
	let mut chain = Chain::with_capacity(10);
	vq.pop_avail(&mut chain, &mem)?;
	chain
	};
	Some(RxPermit { vq: self, chain })

?

(and available_data_space can be computed as-needed from the provided chain over on RxPermit)

this lets you still keep a &mut VsockVq to know there's at most one outstanding chain, and on RxPermit could stuff the chain back on VsockVq in a drop impl.

barring that, I think you could have a helper on VsockVq that directly returns (&Arc<VirtQueue>, &mut Chain) for RxPermit to hold, so the permit can operate directly on the chain instead of having to expect it can reach through an option

[iximeow]

since the VirtIO spec describes this as a u64 and as you've pointed out the upper half is reserved, i'm super fine with making this cid: u64 and asserting the upper half is zeroed in PciVirtioSock::new with a nod to the spec in that assert.

[iximeow]

we should at the very least warn if these reserved bits are set. ideally if there's a way to tell the guest that this packet is invalid and respond with an error, we should do that instead of masking off the upper bits. as-is if a guest addresses a packet to 0x00000001_00000002 we'll treat it the same as a packet addressed to the host when it really is not.

I see Linux does not accept 64-bit CID in sockaddr_vm, but if someone did and their kernel checked for CID == 2 as a way to disallow access to the host, we'd be undermining that check!

[iximeow]

my read of port_get(3c) is that we are saying here that we want to block for at least one event (nget = 1 above), and that we are willing to receive up to 32 (MAX_EVENTS) events. and then nget is set to however many we actually got once there was at least one event to read. is that right? (I consider this "me learning port_getn" not like, needing a comment or anything)

then we std::ptr::null_mut() (! not const ! because this unfortunate wrinkle in libc) to wait without a timeout. and that's fine because this is in a thread we spawn for the poller, where there's no runtime to be driving or other work to do? (having a remark that we don't wait with timeout and that's fine for such and such reasons seems helpful)

[iximeow]

i'd say this unsafe needs a // Safety: port_getn has promised up to nget events are initialized, but a different way to spell this might be...

Suggested change

	for i in 0..nget as usize {
	let event = PortEvent::from_raw(unsafe {
	events[i].assume_init_read()
	});
	// Safety: port_getn has promised ... etc
	let events = unsafe {
	std::slice::from_parts(
	events.as_ptr() as *const libc::port_event,
	nget
	)
	};

... additionally, this is my own caution, but until we see a perf need I'd also consider zero-filling the whole thing instead of MaybeUninit. likewise, assert!(nget <= events.len()) so that we have a check here that we have not suggested to the OS we might want more events than we should be reading, and that the OS has not suggested we should read more data than we can?

[iximeow]

why? I don't think we'd ever legitimately exec from propolis..

[iximeow]

I realize this is a bit pedantic but I assume fcntl has to return some error if fd was a totally bogus value. fcntl(2) doesn't say this explicitly and I assume could use a few words about what if The fildes argument is _not_ an open file descriptor?.

but if we do actually need CLOEXEC set for some reason, please check that fcntl(F_GETFD) is not negative, consistent with the promise about in ~RETURN VALUES` there 😁

[iximeow]

if there wasn't a connection and we'd tried to associate_fd, that's pretty weird right? I was going to suggest that we should warn in the else here, but maybe it makes more sense to just return the error upwards and let the caller decide to remove/reset the connection? I think that would be something like trying to get the connection in handle_fd_event, then passing the get'd connection for the read/write handling.

that leaves all the self.connections management at that top level instead of trying to clean up from under ourselves and trying to account for that with re-getting the connection for reads and then writes.