From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57728) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a70lz-0002nA-Bm for qemu-devel@nongnu.org; Thu, 10 Dec 2015 07:56:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a70lv-0007jG-8A for qemu-devel@nongnu.org; Thu, 10 Dec 2015 07:56:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39787) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a70lv-0007j7-1s for qemu-devel@nongnu.org; Thu, 10 Dec 2015 07:56:35 -0500 Date: Thu, 10 Dec 2015 14:56:30 +0200 From: Victor Kaplansky Message-ID: <20151210143219-mutt-send-email-victork@redhat.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <56685F7E.6020807@6wind.com> Content-Transfer-Encoding: quoted-printable 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: Didier Pallard Cc: "Michael S. Tsirkin" , Thibaut Collet , Jean-Mickael Guerin , QEMU , =?iso-8859-1?Q?Marc-Andr=E9?= Lureau , pbonzini@redhat.com 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=E9 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_w= rite_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 wil= l 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 =3D sendmsg(s->fd, &msgh, 0); > >>> } while (r < 0 && errno =3D=3D 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 responsibil= ity > >>>+ * to resend message or free fds using set_msgfds(0) > >>>+ */ > >>>+ if (r < 0 && errno =3D=3D 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 lon= g > >>as a few bytes have been sent, the ancillary data is sent. So it look= s > >>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 > > >=20 > 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, s= ince > the problem corrected by this patch is a message arriving from qemu wit= hout > 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 =3D=3D EAGAIN.=20 >=20 > thanks > didier >=20 > >> > >>Reviewed-by: Marc-Andr=E9 Lureau > >> > >>> /* free the written msgfds, no matter what */ > >>> if (s->write_msgfds_num) { > >>> g_free(s->write_msgfds); > >>>-- > >>>2.1.4 > >>> > >>> > >> > >> > >> > >>-- > >>Marc-Andr=E9 Lureau > >> >=20 >=20 >=20