From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46184) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWh44-0002c0-RW for qemu-devel@nongnu.org; Fri, 19 Feb 2016 04:09:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aWh40-0001kR-33 for qemu-devel@nongnu.org; Fri, 19 Feb 2016 04:09:28 -0500 Received: from mail-wm0-x229.google.com ([2a00:1450:400c:c09::229]:33225) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWh3z-0001kG-Oy for qemu-devel@nongnu.org; Fri, 19 Feb 2016 04:09:24 -0500 Received: by mail-wm0-x229.google.com with SMTP id g62so59786647wme.0 for ; Fri, 19 Feb 2016 01:09:23 -0800 (PST) Message-ID: <56C6DBB3.7080306@6wind.com> Date: Fri, 19 Feb 2016 10:09:07 +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> <56BB0451.7000901@6wind.com> <20160210135104-mutt-send-email-mst@redhat.com> <20160210121520.GA18383@redhat.com> In-Reply-To: <20160210121520.GA18383@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" , "Michael S. Tsirkin" Cc: thibaut.collet@6wind.com, jmg@6wind.com, qemu-devel@nongnu.org On 02/10/2016 01:15 PM, Daniel P. Berrange wrote: > On Wed, Feb 10, 2016 at 01:53:49PM +0200, Michael S. Tsirkin wrote: >> 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. > Err, no it can't. tcp_chr_write() is the thing responsible for sending > FDs, so we can't assert that no FDs are set, as that'll make it impossible > for anything to send FDs. > > Regards, > Daniel I fully agree with daniel, i think it can't be done in tcp_chr_write as is. So finally, what should i do? Add a method that may return the number of pending msgfds to be sent? It may allow to check that no message fds are pending when using tcp_chr_write... thanks didier