Spectrum Development

Download

devel@spectrum-os.org

July 2020

2 participants
12 discussions

[PATCH crosvm 1/2] sys_util: make UnixSeqpacket::recv use libc::recv
by Alyssa Ross 08 Jul '20

08 Jul '20

It's very surprising that a method called "recv" would end up calling read(). When UnixSeqpacket was introduced in 1d44223f9, the method was called "read", so this behaviour made more sense. It was renamed to "recv" in b7196e2a1, but the implementation was not changed to call libc::recv. In most cases, according to recv(2), the difference between read() and recv() doesn't matter: > The only difference between recv() and read(2) is the presence of > flags. With a zero flags argument, recv() is generally equivalent > to read(2) (but see NOTES). But the NOTES section explains that > If a zero-length datagram is pending, read(2) and recv() with a > flags argument of zero provide different behavior. In this > circumstance, read(2) has no effect (the datagram remains pending), > while recv() consumes the pending datagram. This means that, while we might expect UnixSeqpacket::recv(&mut []) to mirror the behaviour of recv() and discard a message despite the empty buffer, it would actually leave it there. This became relevant when I implemented the startup message queue in 5bd2e7f60, because I used an empty recv to drop the ready message from the socket, since it was only used for polling and I didn't care about the content. But, it turns out that my UnixSeqpacket::recv call did nothing, and left the message on the socket. This wasn't immediately noticeable, but it meant that when a message came through on the control socket later, it would actually get the ready message from UnixSeqpacket::recv, rather than the expected message on the control socket. For example, trying to use crosvm disk resize would print a message like the following: > [ERROR:vm_control/src/lib.rs:649] unexpected disk socket result: Ready > [ERROR:devices/src/virtio/block.rs:334] Attempted to resize read-only block device We can see that it does try to process the received message, but first it tries to do something with the Ready message, which should have been discarded by now. With this fix applied, we can see that it now receives and tries to process the correct message (but then fails because the VM I tested didn't support disk resizing!): > [ERROR:devices/src/virtio/block.rs:334] Attempted to resize read-only block device Fixes: b7196e2a1c1eb7123e7eace5418b7eb4a3e24dbe Fixes: 5bd2e7f606b726b598fdc26fe26a9b4029fab6d3 Cc: Cole Helbling <cole.e.helbling(a)outlook.com> --- This is the fix I described in This Week in Spectrum[1]. [1]: https://spectrum-os.org/lists/archives/spectrum-discuss/8736654ij2.fsf@alys… sys_util/src/net.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sys_util/src/net.rs b/sys_util/src/net.rs index 71ab3eeb5..82b4f5bb0 100644 --- a/sys_util/src/net.rs +++ b/sys_util/src/net.rs @@ -197,7 +197,7 @@ impl UnixSeqpacket { } } - /// Read data from the socket fd to a given buffer + /// Receive data from the socket fd to a given buffer. /// /// # Arguments /// * `buf` - A mut reference to the data buffer. @@ -206,11 +206,11 @@ impl UnixSeqpacket { /// * `usize` - The size of bytes read to the buffer. /// /// # Errors - /// Returns error when `libc::read` failed. + /// Returns error when `libc::recv` failed. pub fn recv(&self, buf: &mut [u8]) -> io::Result<usize> { // Safe since we make sure the input `count` == `buf.len()` and handle the returned error. unsafe { - let ret = libc::read(self.fd, buf.as_mut_ptr() as *mut _, buf.len()); + let ret = libc::recv(self.fd, buf.as_mut_ptr() as *mut _, buf.len(), 0); if ret < 0 { Err(io::Error::last_os_error()) } else { -- 2.26.2

2 6

This Week in Spectrum, 2020-W27
by Alyssa Ross 06 Jul '20

06 Jul '20

I really didn't want this to be another week where I posted about how I was still trying to patch Wayland to do virtio_wl, and I am delighted to have just discovered it's not going to be! crosvm ------ I realised that emulating accept(2) for the Wayland compositor socket in the way I'd planned would require some crosvm rework. I want to have a host proxy program that accepts the connection, then connects the connection socket to crosvm. I had made it possible to dynamically add sockets to the crosvm Wl device through the control socket, but this turned out not be enough, because crosvm would store virtio_wl sockets in a BTreeMap<String, PathBuf>, and then use connect(2) to connect to the socket when asked to by the guest kernel. This works fine for e.g. connecting to a host Wayland compositor, which is what crosvm was designed for, but it wouldn't work for opening a connection socket from accept(2), because you can only connect to a listening socket. So instead, I modified the `crosvm wl add' command to take a file descriptor pointing to the connection socket. I made crosvm store sockets as an enum that looks like this: enum WaylandSocket { Listening(PathBuf), NonListening(UnixStream), } This way, when it gets asked by the VM to connect to a socket, it can either connect to a listening socket at its path using connect(2), or just use the existing file descriptor if it's a non-listening socket. A NonListening socket will be consumed by a connection, so when the VM close(2)s it, it'll go away, and on the host side the connection will finish as expected. Listening sockets can be connected to repeatedly, as before. I also added support to `crosvm add wl' for dynamic socket names. So it's possible to do `crosvm add wl wl-conn-%d', and connections will be added with names like `wl-conn-0', `wl-conn-1', etc. So it's easy to get unique names for connection sockets. The chosen name is printed by the command, so the caller knows what name to tell the VM to connect to. I also found and fixed a bug with the previous crosvm deadlock fix[1]. I had assumed that device_sock.recv(&mut []) would drop a message from the (SOCK_SEQPACKET) socket, without having to read any of it. But UnixSeqpacket::recv calls libc::read, and read(2) tells us that: > In the absence of any errors, or if read() does not check for errors, > a read() with a count of 0 returns zero and has no other effects. So this was in fact doing nothing at all. I don't know why crosvm's UnixSeqpacket::recv calls read() instead of recv(), but it's always been like that and I'm guessing this sort of thing (from recv(2)) might have something to do with it: > The only difference between recv() and read(2) is the presence of > flags. With a zero flags argument, recv() is generally equivalent to > read(2) (but see NOTES). So probably read() just looked like a nicer way to recv() when no flags were needed. But, unfortunately, zero-byte reads are when the aforementioned NOTES section becomes relevant: > If a zero-length datagram is pending, read(2) and recv() with a flags > argument of zero provide different behavior. In this circumstance, > read(2) has no effect (the datagram remains pending), while recv() > consumes the pending datagram. So, my assumption that UnixSeqpacket::recv(&mut []) would consume a message turned out to be quite reasonable -- the surprising thing was that a method called `recv' would call read() rather than recv(). I think the best fix here will be to just make it call recv() instead, rather than modifying my code to do UnixSeqpacket::recv(&mut [0]) or something, to prevent further nasty surprises with this in future. [1]: https://spectrum-os.org/lists/archives/spectrum-devel/20200614114344.22642-… Wayland ------- I created API-compatible implementations of the libc sendmsg(2) and recvmsg(2) functions for virtio_wl sockets. This was quite an achievement, because the API (which allows you to send and receive data and file descriptors, as well as other things I don't intend to support) is rather arcane (see the example in cmsg(3) if you're not familiar with them). I wrote unit tests for them, and it took a long time before they worked reliably. Once I had these, though, I could find the places where Wayland called sendmsg() and recvmsg() and fall back to the virtio_wl-based implementations if the standard functions failed with ENOTSOCK. I stubbed out some stuff that isn't going to work over virtio_wl, like looking up the pid of the Wayland client through getsockopt(2). I also had to resort to a few hacks, like faking support for MSG_DONTWAIT by using fcntl(2) to set O_NONBLOCK on the socket, recv()ing from it, and then removing O_NONBLOCK again, or faking mremap(2) by munmap()-ing and mmap()-ing. We will want to clean these up later by implementing the required missing functionality in the virtio_wl kernel module. In the first case, at least, this should be pretty straightforward, because it supports non-blocking operations if the socket is O_NONBLOCK -- it just needs to accept a MSG_DONTWAIT option as well. The VIRTWL_IOCTL_{SEND,RECV} syscalls don't currently have a flags argument, so that'll need to be added. I implemented this bit by bit, at every step trying to run Alacritty on my host system, connected to the virtio_wl Wayland server socket through the accept() proxy, and using strace and some printf()-debugging to see where the Wayland compositor in the VM would get stuck, and about an hour ago, it finally worked! For the first time, a Wayland compositor running in a VM can display an application running outside of it. (Obviously we'll want the application to be running in another VM rather than on the host, but that's similar enough that it probably works already -- I just haven't tested it yet.) This feels like a huge achievement. I've been working towards it for so long. Next week, I'll be cleaning up this code and posting patches for all of it. Then I'll probably move on to other sorts of device virtualization, like running a virtual network device in a VM. I'm feeling so much more positive about the direction of the project than I was before. It's been difficult to make myself keep going making little progress for the last couple of weeks, and it's great that I've managed to pick things up again so much. I hope that the level of detail in this email is enough to make up for the brevity of last week's! I'm sending late again, too, but only by a couple of minutes -- I didn't expect this email to take over an hour to write, but there we go. Thanks for reading! I hope you're looking forward to seeing where things go from here as much as I am.

1 0