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

[Victor Kaplansky]

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

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