From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45782) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a72qo-0006nR-Fi for qemu-devel@nongnu.org; Thu, 10 Dec 2015 10:09:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a72qj-0000Pp-Om for qemu-devel@nongnu.org; Thu, 10 Dec 2015 10:09:46 -0500 Received: from mail-wm0-x22d.google.com ([2a00:1450:400c:c09::22d]:32962) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a72qj-0000Pg-Fs for qemu-devel@nongnu.org; Thu, 10 Dec 2015 10:09:41 -0500 Received: by mail-wm0-x22d.google.com with SMTP id c201so37700711wme.0 for ; Thu, 10 Dec 2015 07:09:41 -0800 (PST) Message-ID: <566995A3.1040603@6wind.com> Date: Thu, 10 Dec 2015 16:09:23 +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> <56685F7E.6020807@6wind.com> <20151210143219-mutt-send-email-victork@redhat.com> In-Reply-To: <20151210143219-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 Cc: "Michael S. Tsirkin" , Thibaut Collet , Jean-Mickael Guerin , QEMU , =?windows-1252?Q?Marc-Andr=E9_Lureau?= , pbonzini@redhat.com On 12/10/2015 01:56 PM, Victor Kaplansky wrote: > On Wed, Dec 09, 2015 at 06:06:06PM +0100, Didier Pallard wrote: >> 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. > > That's good. > >> 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. > > I'm talking about the problem that supposed to be fixed by the > first patch. It is not clear to me how the patch fixes the > partial send. sendmsg() is called in qemu-char.c:unix_send_msgfds > with zero flags, which means a blocking operation, so I'm > surprised that sendmsg can return with errno == EAGAIN. > Well, vhost-user socket is started with following chardev: -chardev socket,id=vhostuserchr0,path=/tmp/vhost_sock0,server and according to code in tcp_chr_add_client: static int tcp_chr_add_client(CharDriverState *chr, int fd) { ... qemu_set_nonblock(fd); So fd is set in non blocking mode. This is enough to have an EAGAIN returned value on socket buffer full, whatever flags used in sendmsg, i think. Perhaps changing the blocking mode here may also correct the first problem, but I am not able to measure the impact that may have such a modification... >> >> 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 >>>> >> >> >>