Re: [PATCH RFC server v2 05/11] vfio-user: run vfio-user context

[John Levon]

On Wed, Sep 08, 2021 at 01:58:46PM +0100, Stefan Hajnoczi wrote:

> > +static void *vfu_object_attach_ctx(void *opaque)
> > +{
> > +    VfuObject *o = opaque;
> > +    int ret;
> > +
> > +retry_attach:
> > +    ret = vfu_attach_ctx(o->vfu_ctx);
> > +    if (ret < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) {
> 
> Does this loop consume 100% CPU since this is non-blocking?

Looks like it. Instead after vfu_create_ctx, there should be a vfu_get_poll_fd()
to get the listen socket, then a qemu_set_fd_handler(vfu_object_attach_ctx)
to handle the attach when the listen socket is ready, modulo the below part.

> libvfio-user has non-blocking listen_fd but conn_fd is always blocking.

It is, but in vfu_run_ctx(), we poll on it:

```
790     if (vfu_ctx->flags & LIBVFIO_USER_FLAG_ATTACH_NB) {                     
     
791         sock_flags = MSG_DONTWAIT | MSG_WAITALL;                            
     
792     }                                                                       
     
793     return get_msg(hdr, sizeof(*hdr), fds, nr_fds, ts->conn_fd, 
sock_flags);     
```

> This means ATTACH_NB is not useful because vfu_attach_ctx() is actually
> blocking.

You're correct that vfu_attach_ctx is in fact partially blocking: after
accepting the connection, we call negotiate(), which can indeed block waiting if
the client hasn't sent anything.

> I think this means vfu_run_ctx() is also blocking in some places

Correct. There's a presumption that if a message is ready, we can read it all
without blocking, and equally that we can write to the socket without blocking.

The library docs are not at all clear on this point.

> and QEMU's event loop might hang :(
> 
> Can you make libvfio-user non-blocking in order to solve these issues?

I presume you're concerned about the security aspect: a malicious client could
withhold a write, and hence hang the device server.

Problem is the libvfio-user API is synchronous: there's no way to return
half-way through a vfu_attach_ctx() (or a vfu_run_ctx() after we read the
header) then resume.

We'd have to have a whole separate API to do that, so a separate thread seems a
better approach?

regards
john