From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48321) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTTLL-0007KH-GV for qemu-devel@nongnu.org; Wed, 10 Feb 2016 06:54:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTTLH-0006Nu-D1 for qemu-devel@nongnu.org; Wed, 10 Feb 2016 06:53:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43108) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTTLH-0006Nn-5Y for qemu-devel@nongnu.org; Wed, 10 Feb 2016 06:53:55 -0500 Date: Wed, 10 Feb 2016 13:53:49 +0200 From: "Michael S. Tsirkin" Message-ID: <20160210135104-mutt-send-email-mst@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> <20160209170421.GB17911@redhat.com> <56BB0451.7000901@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56BB0451.7000901@6wind.com> Subject: Re: [Qemu-devel] [PATCH 1/3] char: fix vhost-user socket full 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 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 > >>>>>>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 > > 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. -- MST