From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43017) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTBiH-0000RU-I7 for qemu-devel@nongnu.org; Tue, 09 Feb 2016 12:04:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTBiE-0004Yc-9W for qemu-devel@nongnu.org; Tue, 09 Feb 2016 12:04:29 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41978) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTBiE-0004YS-26 for qemu-devel@nongnu.org; Tue, 09 Feb 2016 12:04:26 -0500 Date: Tue, 9 Feb 2016 17:04:21 +0000 From: "Daniel P. Berrange" Message-ID: <20160209170421.GB17911@redhat.com> References: <1449136399-4158-1-git-send-email-didier.pallard@6wind.com> <1449136399-4158-2-git-send-email-didier.pallard@6wind.com> <20160204154232-mutt-send-email-mst@redhat.com> <20160209114813.GA3083@redhat.com> <20160209141859-mutt-send-email-mst@redhat.com> <56BA110C.10806@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <56BA110C.10806@6wind.com> Subject: Re: [Qemu-devel] [PATCH 1/3] char: fix vhost-user socket full Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Didier Pallard Cc: thibaut.collet@6wind.com, jmg@6wind.com, qemu-devel@nongnu.org, "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 > >>>>Reviewed-by: Thibaut Collet > >>>>--- > >>>> 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 -- |: 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 :|