qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/3] char: fix vhost-user socket full


From: Michael S. Tsirkin
Subject: Re: [Qemu-devel] [PATCH 1/3] char: fix vhost-user socket full
Date: Tue, 9 Feb 2016 14:21:10 +0200

On Tue, Feb 09, 2016 at 11:48:13AM +0000, Daniel P. Berrange wrote:
> On Thu, Feb 04, 2016 at 04:10:38PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Dec 03, 2015 at 10:53:17AM +0100, Didier Pallard wrote:
> > > unix_send_msgfds is used by vhost-user control socket. 
> > > qemu_chr_fe_write_all
> > > is used to send a message and retries as long as EAGAIN errno is set,
> > > but write_msgfds buffer is freed after first EAGAIN failure, causing
> > > message to be sent without proper fds attachment.
> > > 
> > > In case unix_send_msgfds is called through qemu_chr_fe_write, it will be
> > > user responsability to resend message as is or to free write_msgfds
> > > using set_msgfds(0)
> > > 
> > > Signed-off-by: Didier Pallard <address@hidden>
> > > Reviewed-by: Thibaut Collet <address@hidden>
> > > ---
> > >  qemu-char.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/qemu-char.c b/qemu-char.c
> > > index 5448b0f..26d5f2e 100644
> > > --- a/qemu-char.c
> > > +++ b/qemu-char.c
> > > @@ -2614,6 +2614,16 @@ static int unix_send_msgfds(CharDriverState *chr, 
> > > const uint8_t *buf, int len)
> > >          r = sendmsg(s->fd, &msgh, 0);
> > >      } while (r < 0 && errno == EINTR);
> > >  
> > > +    /* Ancillary data are not sent if no byte is written
> > > +     * so don't free msgfds buffer if return value is EAGAIN
> > > +     * If called from qemu_chr_fe_write_all retry will come soon
> > > +     * If called from qemu_chr_fe_write, it is the user responsibility
> > > +     * to resend message or free fds using set_msgfds(0)
> > 
> > Problem is, if ever anyone tries to send messages
> > with qemu_chr_fe_write and does not retry, there will
> > be a memory leak.
> > 
> > Rather than this, how about adding an assert in qemu_chr_fe_write
> > to make sure its not used with msgfds?
> 
> NB, this TCP chardev code has completely changed since this patch
> was submitted. It now uses QIOChannel instead of raw sockets APIs.
> The same problem still exists though - when we get EAGAIN from
> the sendmsg, we're releasing the file descriptors even though
> they've not been sent.
> 
> Adding an assert in qemu_chr_fe_write() won't solve it - the
> same problem still exists in qemu_chr_fe_write_all() - although
> that loops to re-try on EAGAIN, the FDs are free'd before it
> does the retry.
> 
> We need to update tcp_chr_write() to not free the FDs when
> io_channel_send_full() returns with errno==EAGAIN, in the
> same way this patch was doing.

Absolutely. We need to fix qemu_chr_fe_write_all.
I am just worried that someone tries to use
qemu_chr_fe_write with fds and then we get
a memory leak if qemu_chr_fe_write is not retried.

So I propose we teach qemu_chr_fe_write
that it can't send msg fds.

> 
> > > +     */
> > > +    if (r < 0 && errno == EAGAIN) {
> > > +        return r;
> > > +    }
> > > +
> > >      /* free the written msgfds, no matter what */
> > >      if (s->write_msgfds_num) {
> > >          g_free(s->write_msgfds);
> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|



reply via email to

[Prev in Thread] Current Thread [Next in Thread]