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

[Michael S. Tsirkin]

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?

No but why will set_msgfds be != NULL if you don't send fds? There's
probably something obvious I'm missing.

> 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...
> 
> anyway, i can put it in the code, but i don't know how to test that it does
> not break in some unexpected cases.
> 
> thanks
> 
> >>>>+     */
> >>>>+    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 
> >>:|