On Tue, Feb 09, 2016 at 05:17:16PM +0100, Didier Pallard wrote:
> On 02/09/2016 01:21 PM, Michael S. Tsirkin wrote:
> >On Tue, Feb 09, 2016 at 11:48:13AM +0000, Daniel P. Berrange wrote:
> >>On Thu, Feb 04, 2016 at 04:10:38PM +0200, 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 <didier.pall...@6wind.com>
> >>>>Reviewed-by: Thibaut Collet <thibaut.col...@6wind.com>
> >>>>---
> >>>> 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?
> >>NB, this TCP chardev code has completely changed since this patch
> >>was submitted. It now uses QIOChannel instead of raw sockets APIs.
> >>The same problem still exists though - when we get EAGAIN from
> >>the sendmsg, we're releasing the file descriptors even though
> >>they've not been sent.
> >>
> >>Adding an assert in qemu_chr_fe_write() won't solve it - the
> >>same problem still exists in qemu_chr_fe_write_all() - although
> >>that loops to re-try on EAGAIN, the FDs are free'd before it
> >>does the retry.
> >>
> >>We need to update tcp_chr_write() to not free the FDs when
> >>io_channel_send_full() returns with errno==EAGAIN, in the
> >>same way this patch was doing.
> >Absolutely. We need to fix qemu_chr_fe_write_all.
> >I am just worried that someone tries to use
> >qemu_chr_fe_write with fds and then we get
> >a memory leak if qemu_chr_fe_write is not retried.
> >
> >So I propose we teach qemu_chr_fe_write
> >that it can't send msg fds.
> Patch is easy to port on head version.
>
> My concern with following assert in qemu_chr_fe_write:
>
> assert(s->set_msgfds == NULL);
>
> Is that it may trigger on a tcp/linux socket that never wants
> to send message fds but uses qemu_chr_fe_write instead
> of qemu_chr_fe_write_all. Are we supposed to always use
> qemu_chr_fe_write_all on a tcp/linux socket?
No but why will set_msgfds be != NULL if you don't send fds? There's
probably something obvious I'm missing.
> I think that only vhost-user socket is using the ability to send
> message fds through socket, but i don't know all places were a linux/tcp
> socket can be used through qemu_chr_fe_write, without wanting
> to send any fd...
>
> anyway, i can put it in the code, but i don't know how to test that it does
> not break in some unexpected cases.
>
> thanks
>
> >>>>+ */
> >>>>+ if (r < 0 && errno == EAGAIN) {
> >>>>+ return r;
> >>>>+ }
> >>>>+
> >>>> /* free the written msgfds, no matter what */
> >>>> if (s->write_msgfds_num) {
> >>>> g_free(s->write_msgfds);
> >>Regards,
> >>Daniel
> >>--
> >>|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/
> >>:|
> >>|: http://libvirt.org -o- http://virt-manager.org
> >>:|
> >>|: http://autobuild.org -o- http://search.cpan.org/~danberr/
> >>:|
> >>|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc
> >>:|
