Re: [PATCH 2/4] vhost-user: Convert slave channel to QIOChannelSocket

[Daniel P . Berrangé]

On Tue, Mar 09, 2021 at 09:23:22PM +0100, Greg Kurz wrote:
> On Tue, 9 Mar 2021 15:17:21 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > On Mon, Mar 08, 2021 at 01:31:39PM +0100, Greg Kurz wrote:
> > > +    g_autofree int *fd = NULL;
> > > +    size_t fdsize = 0;
> > > +    int i;
> > >  
> > >      /* Read header */
> > >      iov.iov_base = &hdr;
> > >      iov.iov_len = VHOST_USER_HDR_SIZE;
> > >  
> > >      do {
> > > -        size = recvmsg(u->slave_fd, &msgh, 0);
> > > -    } while (size < 0 && (errno == EINTR || errno == EAGAIN));
> > > +        size = qio_channel_readv_full(ioc, &iov, 1, &fd, &fdsize, NULL);
> > > +    } while (size == QIO_CHANNEL_ERR_BLOCK);
> > 
> > Is it possible to leak file descriptors and fd[] memory if we receive a
> > short message and then loop around? qio_channel_readv_full() will
> > overwrite &fd and that's how the leak occurs.
> > 
> 
> qio_channel_readv_full() only returns QIO_CHANNEL_ERR_BLOCK when the
> channel is non-blocking mode and no data is available. Under the hood,
> this translates to this code in qio_channel_socket_readv():
> 
>  retry:
>     ret = recvmsg(sioc->fd, &msg, sflags);
>     if (ret < 0) {
>         if (errno == EAGAIN) {
>             return QIO_CHANNEL_ERR_BLOCK;
>         }
>         if (errno == EINTR) {
>             goto retry;
>         }
> 
>         error_setg_errno(errp, errno,
>                          "Unable to read from socket");
>         return -1;
>     }
> 
> This is strictly equivalent to what we currently have. This cannot
> leak file descriptors because we would only loop until the first
> byte of real data is available and ancillary data cannot fly without
> real data, i.e. no file descriptors when recvmsg() returns EAGAIN.

Yep, EAGAIN will only happen if you have no data AND no FDs.

> 
> > On the other hand, it looks like ioc is in blocking mode. I'm not sure
> > QIO_CHANNEL_ERR_BLOCK can occur?
> > 
> 
> You're right but the existing code is ready to handle the non-blocking
> case, so I just kept this behavior.

The existing code is *not* handling the non-blocking case in any
useful way IMHO. It will block execution of this QEMU thread, and
sit in a tight loop burning CPU in the EAGAIN case.

Handling non-blocking means using an I/O watch with the event loop
to be notified when something becomes ready.

I notice the code that follows is also doing


    if (size != VHOST_USER_HDR_SIZE) {
        error_report("Failed to read from slave.");
        goto err;
    }

which is also dubious because it assumes the previous recvmsg is
guaranteed to return VHOST_USER_HDR_SIZE bytes of data in a single
call.

It does at least look like the code is intentionally blocking
execution fo the QEMU thread until the expected amount of I/O
is receieved.

Given this, you should remove the while loop entirely and
just call

   qio_channel_readv_full_all()

this will block execution until *all* the requestd data bytes
are read. It will re-try the recvmsg in the event of a partial
data read, and will intelligently wait in poll() on EAGAIN.

> 
> > > @@ -1500,8 +1479,8 @@ static void slave_read(void *opaque)
> > >  
> > >      /* Read payload */
> > >      do {
> > > -        size = read(u->slave_fd, &payload, hdr.size);
> > > -    } while (size < 0 && (errno == EINTR || errno == EAGAIN));
> > > +        size = qio_channel_read(ioc, (char *) &payload, hdr.size, NULL);
> > > +    } while (size == QIO_CHANNEL_ERR_BLOCK);
> > 
> > Same question here.
> 
> This also ends up in qio_channel_socket_readv().



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|