From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50828) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTAyv-0004aI-2G for qemu-devel@nongnu.org; Tue, 09 Feb 2016 11:17:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTAyr-0006cf-Jz for qemu-devel@nongnu.org; Tue, 09 Feb 2016 11:17:36 -0500 Received: from mail-wm0-x22b.google.com ([2a00:1450:400c:c09::22b]:37319) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTAyr-0006cZ-9A for qemu-devel@nongnu.org; Tue, 09 Feb 2016 11:17:33 -0500 Received: by mail-wm0-x22b.google.com with SMTP id g62so30330629wme.0 for ; Tue, 09 Feb 2016 08:17:32 -0800 (PST) Message-ID: <56BA110C.10806@6wind.com> Date: Tue, 09 Feb 2016 17:17:16 +0100 From: Didier Pallard MIME-Version: 1.0 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> In-Reply-To: <20160209141859-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" , "Daniel P. Berrange" Cc: thibaut.collet@6wind.com, jmg@6wind.com, qemu-devel@nongnu.org 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... 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 :|