On Mon, Jan 10, 2022 at 05:56:25PM +0000, John Levon wrote:
> On Thu, Jan 06, 2022 at 01:35:32PM +0000, Stefan Hajnoczi wrote:
>
> > > > >> +static void vfu_object_attach_ctx(void *opaque)
> > > > >> +{
> > > > >> + VfuObject *o = opaque;
> > > > >> + GPollFD pfds[1];
> > > > >> + int ret;
> > > > >> +
> > > > >> + qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL);
> > > > >> +
> > > > >> + pfds[0].fd = o->vfu_poll_fd;
> > > > >> + pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
> > > > >> +
> > > > >> +retry_attach:
> > > > >> + ret = vfu_attach_ctx(o->vfu_ctx);
> > > > >> + if (ret < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) {
> > > > >> + qemu_poll_ns(pfds, 1, 500 * (int64_t)SCALE_MS);
> > > > >> + goto retry_attach;
> > > > >
> > > > > This can block the thread indefinitely. Other events like monitor
> > > > > commands are not handled in this loop. Please make this asynchronous
> > > > > (set an fd handler and return from this function so we can try again
> > > > > later).
> > > > >
> > > > > The vfu_attach_ctx() implementation synchronously negotiates the
> > > > > vfio-user connection :(. That's a shame because even if accept(2) is
> > > > > handled asynchronously, the negotiation can still block. It would be
> > > > > cleanest to have a fully async libvfio-user's vfu_attach_ctx() API to
> > > > > avoid blocking. Is that possible?
> > > >
> > > > Thanos / John,
> > > >
> > > > Any thoughts on this?
> > >
> > > I'm discussing this with John and FYI there are other places where
> > > libvfio-user can block, e.g. sending a response or receiving a command.
> > > Is it just the negotiation you want it to be asynchronous or _all_
> > > libvfio-user operations? Making libvfio-user fully asynchronous might
> > > require a substantial API rewrite.
> >
> > I see at least two reasons for a fully async API:
> >
> > 1. The program wants to handle other events (e.g. a management REST API)
> > from the same event loop thread that invokes libvfio-user. If
> > libvfio-user blocks then the other events cannot be handled within a
> > reasonable time frame.
> >
> > The workaround for this is to use multi-threading and ignore the
> > event-driven architecture implied by vfu_get_poll_fd().
> >
> > 2. The program handles multiple clients that do not trust each other.
> > This could be a software-defined network switch or storage appliance.
> > A malicious client can cause a denial-of-service by making a
> > libvfio-user call block.
> >
> > Again, the program needs separate threads instead of an event loop to
> > work around this.
>
> Hi Stefan, we're certainly aware of the rationale. Ultimately we just haven't
> yet found resources to fix this: in practice, we don't really hit problems
> from
> the parts that are still synchronous. Of course, that's not a good argument
> when
> it comes to your second issue, and indeed the library is not currently
> suitable
> for multiple untrusted clients.
>
> For Jag but: patches are welcome. But it's not just negotiate(): all sorts of
> things are potentially synchronous - for example, it's expected that the
> region
> read/write callbacks are done synchronously. Any other client reply, or
> server->client message, is also synchronous.
>
> It's partly why we haven't yet stabilised the API.>From the QEMU side it's okay to have blocking code in this experimental feature if users are aware of the limitation (e.g. the monitor is unresponsive if vfio-user blocks) and multiple untrusted clients are not supported. The QEMU code should also make its limitations clear so readers are aware of them and know the author chose this approach intentionally rather than due to an oversight. Jag: Please mention this in the documentation and add a comment to vfu_object_attach_ctx() explaining that this code currently blocks. I think in the long run it will be necessary to address it, e.g. in the multi-client case. That can either be done with threads or by making libvfio-user fully async. Stefan
signature.asc
Description: PGP signature
