From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36800) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTRBN-0007IS-RU for qemu-devel@nongnu.org; Wed, 10 Feb 2016 04:35:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTRBK-0005i1-AZ for qemu-devel@nongnu.org; Wed, 10 Feb 2016 04:35:33 -0500 Received: from mail-wm0-x22a.google.com ([2a00:1450:400c:c09::22a]:36901) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTRBJ-0005hv-Ve for qemu-devel@nongnu.org; Wed, 10 Feb 2016 04:35:30 -0500 Received: by mail-wm0-x22a.google.com with SMTP id g62so18299166wme.0 for ; Wed, 10 Feb 2016 01:35:29 -0800 (PST) Message-ID: <56BB0451.7000901@6wind.com> Date: Wed, 10 Feb 2016 10:35:13 +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> <56BA110C.10806@6wind.com> <20160209170421.GB17911@redhat.com> In-Reply-To: <20160209170421.GB17911@redhat.com> Content-Type: text/plain; charset=utf-8; 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: "Daniel P. Berrange" Cc: thibaut.collet@6wind.com, jmg@6wind.com, qemu-devel@nongnu.org, "Michael S. Tsirkin" 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