On Wed, 10 Mar 2021 11:43:58 +0000
Daniel P. Berrangé <berra...@redhat.com> wrote:
> On Tue, Mar 09, 2021 at 09:23:22PM +0100, Greg Kurz wrote:
> > On Tue, 9 Mar 2021 15:17:21 +0000
> > Stefan Hajnoczi <stefa...@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.
>
Hmm... slave_read() is a handler registered with qemu_set_fd_handler().
Isn't it already the case then ? Can the first call to recvmsg() even
return EAGAIN actually ?
> 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.
>
Yeah this is broken since recvmsg() doesn't guarantee full reads.
> 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.
>
Thanks for the suggestion !
> >
> > > > @@ -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
