From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57114) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6iCF-0003Cq-8S for qemu-devel@nongnu.org; Wed, 09 Dec 2015 12:06:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a6iCC-0000Z1-I6 for qemu-devel@nongnu.org; Wed, 09 Dec 2015 12:06:31 -0500 Received: from mail-wm0-x232.google.com ([2a00:1450:400c:c09::232]:38629) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6iCC-0000Yg-6f for qemu-devel@nongnu.org; Wed, 09 Dec 2015 12:06:28 -0500 Received: by wmec201 with SMTP id c201so83128594wme.1 for ; Wed, 09 Dec 2015 09:06:27 -0800 (PST) Message-ID: <56685F7E.6020807@6wind.com> Date: Wed, 09 Dec 2015 18:06:06 +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> <20151209173600-mutt-send-email-victork@redhat.com> In-Reply-To: <20151209173600-mutt-send-email-victork@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit 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: Victor Kaplansky , =?windows-1252?Q?Marc-Andr=E9_?= =?windows-1252?Q?Lureau?= Cc: Thibaut Collet , Jean-Mickael Guerin , pbonzini@redhat.com, QEMU , "Michael S. Tsirkin" On 12/09/2015 04:59 PM, Victor Kaplansky wrote: > On Mon, Dec 07, 2015 at 02:31:36PM +0100, Marc-André Lureau wrote: >> Hi >> >> On Thu, Dec 3, 2015 at 10:53 AM, 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) >>> + */ >>> + if (r < 0 && errno == EAGAIN) { >>> + return r; >>> + } >>> + >> >> This looks reasonable to me. However, I don't know what happens with >> partial write of ancillary data. Hopefully it's all or nothing. >> Apparently, reading unix_stream_sendmsg() in kernel shows that as long >> as a few bytes have been sent, the ancillary data is sent. So it looks >> like it still does the right thing in case of a partial write. > > If I may put my two cents in, it looks to me very similar to an > fd leakage on back-end side. When a new set_call_fd request > arrives, it is very easy to forget closing the previous file > descriptor. As result, if interrupts are actively maksed/unmasked > by the guest, the back-end can easily reach maximum fds, which > will cause receiving side silently drop new fds in aux data. > --Victor > Hi victor, This is not a problem of fd exausted. This was my first axe of investigation, but fd management is correct in our vhost-user backend, there is no fd leakage. And i guess you are refering to the problem fixed by patches 2 and 3, since the problem corrected by this patch is a message arriving from qemu without ancillary data, whatever the state of the fds in the vhost-user backend. thanks didier >> >> Reviewed-by: Marc-André Lureau >> >>> /* free the written msgfds, no matter what */ >>> if (s->write_msgfds_num) { >>> g_free(s->write_msgfds); >>> -- >>> 2.1.4 >>> >>> >> >> >> >> -- >> Marc-André Lureau >>