Re: [Qemu-devel] [PATCH 1/3] char: fix vhost-user socket full

[Victor Kaplansky]

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
> >><address@hidden> 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 <address@hidden>
> >>>Reviewed-by: Thibaut Collet <address@hidden>
> >>>---
> >>>  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. 

> 
> thanks
> didier
> 
> >>
> >>Reviewed-by: Marc-André Lureau <address@hidden>
> >>
> >>>      /* free the written msgfds, no matter what */
> >>>      if (s->write_msgfds_num) {
> >>>          g_free(s->write_msgfds);
> >>>--
> >>>2.1.4
> >>>
> >>>
> >>
> >>
> >>
> >>--
> >>Marc-André Lureau
> >>
> 
> 
>