[BUG] crosvm vhost-user does not handle partial reads

[Alyssa Ross]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

I don't have a Google account, so am reporting this here on crosvm-dev.
Hope that's okay.

Recently, I'm frequently seeing error messages like the following when
using crosvm with vhost-user-gpu:

> [2023-03-13T13:59:01.796197257+00:00 ERROR devices::virtio::vhost::user::vmm::handler] failed to start vhost_user_virtio_gpu worker: backend request failure: failed to handle a vhost-user request: temporary socket error: Resource temporarily unavailable (os error 11)

Below I have annotated the relevant code to explain why this happens.
(from devices/src/virtio/vhost/user/vmm/handler/sys/unix.rs)

> pub async fn run_backend_request_handler(
> handler: Option<BackendReqHandler>,
> ex: &Executor,
> ) -> Result<()> {
> let mut handler = match handler {
> Some(h) => h,
> None => std::future::pending().await,
> };
>
> let h = SafeDescriptor::try_from(&handler as &dyn AsRawDescriptor)
> .map(AsyncWrapper::new)
> .context("failed to get safe descriptor for handler")?;
> let handler_source = ex
> .async_from(h)
> .context("failed to create an async source")?;

Here the backend request socket is set to non-blocking. This applies to
both file descriptors for it — the one owned by handler_source/h, and
the one owned by handler.

>
> loop {
> handler_source
> .wait_readable()
> .await
> .context("failed to wait for the handler to become readable")?;

This waits until there's something to read from the socket, but the
problem is that there being something to read does not guarantee that a
full request is available to read.

> match handler.handle_request() {

This returns a SocketRetry error if there isn't a full message available
to read.

> Ok(_) => (),
> Err(VhostError::ClientExit) => {
> info!("vhost-user connection closed");
> // Exit as the client closed the connection.
> return Ok(());
> }
> Err(e) => {
> bail!("failed to handle a vhost-user request: {}", e);
> }
> };
> }
> }

There are a couple of ways this could be fixed:

• Don't set the socket to be non-blocking. It would still be possible
to use poll() or similar to yield execution between messages
(although the crosvm codebase lacks a method for it), so blocking
would only happen during the very short time it takes to receive a
message.

• async-ify handle_request(), and the functions it uses to read from
the socket. Reading from the socket could be done in a loop that
would repeat on EAGAIN, with execution being yielded in between loop
iterations.

It might be tempting to try to split up reading the request's header and
body, like was done in https://crrev.com/5b456d31, but this would be
incorrect, because the backend request socket is a stream socket.
Stream sockets have no concept of packets, so even just reading the
header could result in a partial read. As a result, something like the
async read loop suggested above is needed to guarantee that messages are
read properly. I think the fix in the linked commit is probably wrong
for the same reason, because the vhost-user frontend request socket is
also a stream socket. It looks like the bug goes back to
https://crrev.com/3040cf02, when async vhost-user request handling was
first introduced.
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEH9wgcxqlHM/ARR3h+dvtSFmyccAFAmQPa/EACgkQ+dvtSFmy
ccBNNhAAkN7SvMzbUDXNJox3F8R9NNw2uecdRESILeQaD1rraeW6aJc6am9MhqQV
CYV/bW6mB7ddyOKEbGPXeJYoXDTbWFPctgh67DzcYyGjaDsBYY8l1dU5zYsJxLeT
/G13z7zMawst8AyaQtrsPO4wrHSi7C2irZow8Mf7q4y4yCX4kjppoWXjN/eC6EuH
hRojkVwAVbWBZrvYqrVN6/7kXPSG5VyeWYqKF7yi2O/nC0Q7WjVx3XDTPCsAF6BV
V4rx6Qez7kkx2JVHlQdn45i4yTbT7E+kznViS5t8CdOEH/rul83Bt5m7kYPwWkN2
6UcW2PfJXlUjJVRxJBYST43sQOrXMaxwCCHlvWgjhHnQl2+/LEiYc5bCgEuxQzXZ
K/+ZVdODJTa0oYSSQBPWz2RCXv17pAOVW+eFXxv01j4vNFgThYL6gQ35AW6STcn2
oJ+XJzHg1Kvr+KA9x4CvR7ladvmmzBnWb5lMxXdn8qQN3NsuYiWXMMZFH0jLBXGV
mYwxeuAOGCdnxH/wyTWPORghr4a3R26xMuiMTa7qHa7OVQZ3I/chdjZzjgu/3Hnq
ldWSZ7hMgEOIu9A2mqS/j25JnyIP+SPS06RSbLM6y17LbYmqq4RB1baaZ/yibWql
Ze+aaJILR/IoM8qIVbUht7D0iJukhqYMYE62JrUcX7phU6Q2uT4=
=p73j
-----END PGP SIGNATURE-----

[Frederick Mayle]

Perhaps a quick fix we could temporarily set the FD back to blocking mode for the scope of the call to `handle_request`. It is bad to do something "sync" in an "async" function, but as long as the other side of the socket is trusted, the latency between the header and body should be small. (ofc if the other side is untrusted, the current technique is even worse)

--
You received this message because you are subscribed to the Google Groups "crosvm-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to crosvm-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/crosvm-dev/87mt4gtsem.fsf%40alyssa.is.
For more options, visit https://groups.google.com/a/chromium.org/d/optout.

[Alyssa Ross]

On Mon, Mar 13, 2023 at 01:13:10PM -0700, Frederick Mayle wrote:
> Perhaps a quick fix we could temporarily set the FD back to blocking mode
> for the scope of the call to `handle_request`. It is bad to do something
> "sync" in an "async" function, but as long as the other side of the socket
> is trusted, the latency between the header and body should be small. (ofc
> if the other side is untrusted, the current technique is even worse)

I think that would work, yes. (Excepting some unforeseen problem caused
by breaking PollSource/UringSource's invariants.)

[Noah Gold]

On Mon, Mar 13, 2023 at 4:46 PM Alyssa Ross <h...@alyssa.is> wrote:
>
> On Mon, Mar 13, 2023 at 01:13:10PM -0700, Frederick Mayle wrote:
> > Perhaps a quick fix we could temporarily set the FD back to blocking mode
> > for the scope of the call to `handle_request`. It is bad to do something
> > "sync" in an "async" function, but as long as the other side of the socket
> > is trusted, the latency between the header and body should be small. (ofc
> > if the other side is untrusted, the current technique is even worse)
>
> I think that would work, yes. (Excepting some unforeseen problem caused
> by breaking PollSource/UringSource's invariants.)

The trouble with this is it'll work fine right up until it causes some
inexplicable stall (e.g. maybe due to a badly behaved or buggy device
backend). The right solution here is to have a proper async read on
the UDS. It's a reasonable amount of work as we'd have to get the
async socket implementation working in cros_async (we have one in
cros_asyncv2). In theory we could delegate the blocking read to the
thread pool as a short term workaround, but I suspect that'd be messy
and unergonomic (hard to say for sure without looking at a patch).

[Daniel Verkamp]

> > > On Mon, Mar 13, 2023 at 11:31 AM Alyssa Ross <h...@alyssa.is> wrote:
> > >
> > > > I don't have a Google account, so am reporting this here on crosvm-dev.
> > > > Hope that's okay.

Thanks for the report - I have filed a public bug for this issue here:
https://issuetracker.google.com/issues/273574299

On Mon, Mar 13, 2023 at 5:23 PM 'Noah Gold' via crosvm-dev
<crosv...@chromium.org> wrote:
>
> On Mon, Mar 13, 2023 at 4:46 PM Alyssa Ross <h...@alyssa.is> wrote:
> >
> > On Mon, Mar 13, 2023 at 01:13:10PM -0700, Frederick Mayle wrote:
> > > Perhaps a quick fix we could temporarily set the FD back to blocking mode
> > > for the scope of the call to `handle_request`. It is bad to do something
> > > "sync" in an "async" function, but as long as the other side of the socket
> > > is trusted, the latency between the header and body should be small. (ofc
> > > if the other side is untrusted, the current technique is even worse)
> >
> > I think that would work, yes. (Excepting some unforeseen problem caused
> > by breaking PollSource/UringSource's invariants.)
>
> The trouble with this is it'll work fine right up until it causes some
> inexplicable stall (e.g. maybe due to a badly behaved or buggy device
> backend). The right solution here is to have a proper async read on
> the UDS. It's a reasonable amount of work as we'd have to get the
> async socket implementation working in cros_async (we have one in
> cros_asyncv2). In theory we could delegate the blocking read to the
> thread pool as a short term workaround, but I suspect that'd be messy
> and unergonomic (hard to say for sure without looking at a patch).

I think full async would be the preferred option; however, this will
likely mean a lot of churn in the vhost-user code. I put together a
quick change that just propagates the async through the API, and it
touches a lot of files -- note that this does not actually implement
async for UnixStream sockets, it just tries to get the function
signatures/types right:
https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4335146

Another short-term option, similar to the blocking approach that
Frederick mentioned, would be to add a retry loop if we get less than
the expected amount of data, retrying on EAGAIN, with a poll() call
inserted so we don't busy-wait on data becoming available. I'm not
sure if this is any cleaner than switching the socket back and forth
between blocking and non-blocking, though.

Thanks,
-- Daniel

[Dmitry Torokhov]

FWIW I am concerned that toggling the socket back and forth between
blocking and non-blocking will lead to fragility and hard to track
bugs. Repeating the reads until enough data is accumulated is a common
pattern. We can't switch the socket type, can we?

Thanks,
Dmitry

[Noah Gold]

We probably can't change the socket type because the spec uses a byte
oriented socket (honestly I'm not sure why this is the case; the
vhost-user protocol under the hood seems to just be implementing
seqpacket on top of byte sockets). If we used seqpacket this problem
would go away (assuming poll doesn't signal until a full message is
available).

I'm also concerned about the workaround solutions, albeit for
different reasons (sync code in an async context is scary). Daniel's
patch does churn interfaces, but it looks like an evolution in the
right direction.

>
> Thanks,
> Dmitry

[Alyssa Ross]

On Tue, Mar 14, 2023 at 05:08:04PM -0700, Noah Gold wrote:
> We probably can't change the socket type because the spec uses a byte
> oriented socket (honestly I'm not sure why this is the case; the
> vhost-user protocol under the hood seems to just be implementing
> seqpacket on top of byte sockets). If we used seqpacket this problem
> would go away (assuming poll doesn't signal until a full message is
> available).

I suspect this is because seqpacket unix sockets are Linux-specific,
and there's otherwise nothing stopping vhost-user being portable to
other Unixes.