Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen

[Stefano Garzarella <sgarzare@redhat.com>]

On Thu, Feb 13, 2020 at 09:51:36AM +0000, Boeuf, Sebastien wrote:
> Hi Stefano,
> 
> On Thu, 2020-02-13 at 10:41 +0100, Stefano Garzarella wrote:
> > Hi Sebastien,
> > 
> > On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, Sebastien wrote:
> > > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 Mon Sep 17 00:00:00
> > > 2001
> > > From: Sebastien Boeuf <sebastien.boeuf@intel.com>
> > > Date: Thu, 13 Feb 2020 08:50:38 +0100
> > > Subject: [PATCH] net: virtio_vsock: Fix race condition between bind
> > > and listen
> > > 
> > > Whenever the vsock backend on the host sends a packet through the
> > > RX
> > > queue, it expects an answer on the TX queue. Unfortunately, there
> > > is one
> > > case where the host side will hang waiting for the answer and will
> > > effectively never recover.
> > 
> > Do you have a test case?
> 
> Yes I do. This has been a bug we've been investigating on Kata
> Containers for quite some time now. This was happening when using Kata
> along with Cloud-Hypervisor (which rely on the hybrid vsock
> implementation from Firecracker). The thing is, this bug is very hard
> to reproduce and was happening for Kata because of the connection
> strategy. The kata-runtime tries to connect a million times after it
> started the VM, just hoping the kata-agent will start to listen from
> the guest side at some point.

Maybe is related to something else. I tried the following which should be
your case simplified (IIUC):

guest$ python
    import socket
    s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM)
    s.bind((socket.VMADDR_CID_ANY, 1234))

host$ python
    import socket
    s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM)
    s.connect((3, 1234))

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TimeoutError: [Errno 110] Connection timed out

> 
> > 
> > In the host, the af_vsock.c:vsock_stream_connect() set a timeout, so
> > if
> > the host try to connect before the guest starts listening, the
> > connect()
> > should return ETIMEDOUT if the guest does not answer anything.
> > 
> > Anyway, maybe the patch make sense anyway, changing a bit the
> > description
> > (if the host connect() receive the ETIMEDOUT).
> > I'm just concerned that this code is common between guest and host.
> > If a
> > malicious guest starts sending us wrong requests, we spend time
> > sending
> > a reset packet. But we already do that if we can't find the bound
> > socket,
> > so it might make sense.
> 
> Yes I don't think this is gonna cause more trouble, but at least we
> cannot end up in this weird situation I described.

Okay, but in the host, we can't trust the guest to answer, we should
handle this case properly.

> 
> I was just not sure if the function we should use to do the reset
> should be virtio_transport_reset_no_sock() or virtio_transport_reset()
> since at this point the socket is already bound.

I think you can safely use virtio_transport_reset() in this case.

Maybe we can add a test case to the vsock test suite (tools/testing/vsock)
to stress the connect.

Thanks,
Stefano