[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/4] vhost-user: Convert slave channel to QIOChannelSocket
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH 2/4] vhost-user: Convert slave channel to QIOChannelSocket |
Date: |
Wed, 10 Mar 2021 13:48:46 +0000 |
User-agent: |
Mutt/2.0.5 (2021-01-21) |
On Wed, Mar 10, 2021 at 02:45:25PM +0100, Greg Kurz wrote:
> On Wed, 10 Mar 2021 11:43:58 +0000
> Daniel P. Berrangé <berrange@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 <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.
> >
>
> 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 ?
IIUC it shouldn't be able to return EAGAIN in that scenario for stream
sockets (TCP, UNIX), only a datagram socket (UDP) would be at risk of
EAGAIN
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 :|
- [PATCH 1/4] vhost-user: Introduce nested event loop in vhost_user_read(), (continued)
[PATCH 3/4] vhost-user: Monitor slave channel in vhost_user_read(), Greg Kurz, 2021/03/08
[PATCH 4/4] virtiofsd: Release vu_dispatch_lock when stopping queue, Greg Kurz, 2021/03/08