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

[Daniel P. Berrange]

On Wed, Feb 10, 2016 at 01:53:49PM +0200, Michael S. Tsirkin wrote:
> On Wed, Feb 10, 2016 at 10:35:13AM +0100, Didier Pallard wrote:
> > On 02/09/2016 06:04 PM, Daniel P. Berrange wrote:
> > >On Tue, Feb 09, 2016 at 05:17:16PM +0100, Didier Pallard wrote:
> > >>On 02/09/2016 01:21 PM, Michael S. Tsirkin wrote:
> > >>>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.
> > >>Patch is easy to port on head version.
> > >>
> > >>My concern with following assert in qemu_chr_fe_write:
> > >>
> > >>assert(s->set_msgfds == NULL);
> > >>
> > >>Is that it may trigger on a tcp/linux socket that never wants
> > >>to send message fds but uses qemu_chr_fe_write instead
> > >>of qemu_chr_fe_write_all. Are we supposed to always use
> > >>qemu_chr_fe_write_all on a tcp/linux socket?
> > >>
> > >>I think that only vhost-user socket is using the ability to send
> > >>message fds through socket, but i don't know all places were a linux/tcp
> > >>socket can be used through qemu_chr_fe_write, without wanting
> > >>to send any fd...
> > >The qemu_chr_fe_set_msgfds() function is only called from one
> > >place in QEMU:
> > >
> > >$ git grep qemu_chr_fe_set_msgfds
> > >hw/virtio/vhost-user.c:        qemu_chr_fe_set_msgfds(chr, fds, fd_num);
> > >include/sysemu/char.h: * @qemu_chr_fe_set_msgfds:
> > >include/sysemu/char.h:int qemu_chr_fe_set_msgfds(CharDriverState *s, int 
> > >*fds, int num);
> > >qemu-char.c:int qemu_chr_fe_set_msgfds(CharDriverState *s, int *fds, int 
> > >num)
> > >
> > >so if vhost-user.c does the write thing calling write_all(),
> > >then you're fine.
> > >
> > >Regards,
> > >Daniel
> > 
> > I agree that it will work as expected for vhost-user, but set_msgfds will be
> > set
> > to tcp_set_msgfds for all "socket" type chardev. If such chardev is
> > configured
> > to be used by some part of code using qemu_chr_fe_write instead of
> > qemu_chr_fe_write_all,
> > it will trigger the assert.
> > And i just don't know if this case can happen.
> > 
> > I will write a new patchset with the assert in a separate commit.
> > thanks
> 
> Oh, I see. I really meant
> 
> assert(!s->write_msgfds && !s->write_msgfds_num);
> 
> this assert can go into tcp_chr_write.

Err, no it can't. tcp_chr_write() is the thing responsible for sending
FDs, so we can't assert that no FDs are set, as that'll make it impossible
for anything to send FDs.

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 :|