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

[Victor Kaplansky]

On Thu, Dec 10, 2015 at 04:09:23PM +0100, Didier Pallard wrote:
> 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
> >>>><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 fil
> >>>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 for the clarification. I was able to reproduce both issues
- with send_msgfds() partial send and lost interrupts using code
based on vhost-user-bridge test application. 
I'm working on a fix for the lost interrupts. Will send an RFC
patch by the Sunday.

-- Victor
> 
> 
> >>
> >>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
> >>>>
> >>
> >>
> >>
> 
>