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

[Michael S. Tsirkin]

On Mon, Feb 08, 2016 at 02:12:14PM +0100, Didier Pallard wrote:
> On 02/04/2016 03:10 PM, Michael S. Tsirkin wrote:
> >On Thu, Dec 03, 2015 at 10:53:17AM +0100, 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 <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)
> >Problem is, if ever anyone tries to send messages
> >with qemu_chr_fe_write and does not retry, there will
> >be a memory leak.
> >
> >Rather than this, how about adding an assert in qemu_chr_fe_write
> >to make sure its not used with msgfds?
> 
> Well, in qemu_chr_fe_write, all i have is a CharDriverState.
> Information on number of fds to pass are located in TCPCharDriver structure.
> 
> should i assert as soon as a set_msgfds method is set:
> assert(s->set_msgfds == NULL);
> but it may trigger on unix sockets were message fds are never used.

Why? Because this field is never initialized?
Let's initialize it unconditionally then?

> or try to detect the implementation and really check the number of fds
> passed:
> assert((s->chr_write != tcp_chr_write) && ((TCPCharDriver
> *)s->opaque)->write_msgfds_num == 0))
> but i didn't find better than checking a method to detect type of underlying
> driver and I don't like this kind of code.
> 
> Another solution should be to add a new method to get pending fds number,
> assert would become:
> assert((s->get_write_msgfds_num == NULL) || (s->get_write_msgfds_num(s) ==
> 0))
> 
> s->get_write_msgfds_num being set only for tcp_ socket, with a simple
> function:
> 
> tcp_chr_get_write_msgfds_num(CharDriverState *chr)
> {
>     TCPCharDriver *s = chr->opaque;
>     return s->write_msgfds_num;
> }
> 
> ...
> s->get_write_msgfds_num = tcp_chr_get_write_msgfds_num;
> ...
> 
> ?
> 
> >
> >>+     */
> >>+    if (r < 0 && errno == EAGAIN) {
> >>+        return r;
> >>+    }
> >>+
> >>      /* free the written msgfds, no matter what */
> >>      if (s->write_msgfds_num) {
> >>          g_free(s->write_msgfds);
> >>-- 
> >>2.1.4
> >>
> >
>