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

[Michael S. Tsirkin]

On Wed, Feb 10, 2016 at 10:35:13AM +0100, Didier Pallard wrote:
> On 02/09/2016 06:04 PM, Daniel P. Berrange wrote:
> >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 <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?
> >>>>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?
> >>
> >>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...
> >The qemu_chr_fe_set_msgfds() function is only called from one
> >place in QEMU:
> >
> >$ git grep qemu_chr_fe_set_msgfds
> >hw/virtio/vhost-user.c:        qemu_chr_fe_set_msgfds(chr, fds, fd_num);
> >include/sysemu/char.h: * @qemu_chr_fe_set_msgfds:
> >include/sysemu/char.h:int qemu_chr_fe_set_msgfds(CharDriverState *s, int 
> >*fds, int num);
> >qemu-char.c:int qemu_chr_fe_set_msgfds(CharDriverState *s, int *fds, int num)
> >
> >so if vhost-user.c does the write thing calling write_all(),
> >then you're fine.
> >
> >Regards,
> >Daniel
> 
> I agree that it will work as expected for vhost-user, but set_msgfds will be
> set
> to tcp_set_msgfds for all "socket" type chardev. If such chardev is
> configured
> to be used by some part of code using qemu_chr_fe_write instead of
> qemu_chr_fe_write_all,
> it will trigger the assert.
> And i just don't know if this case can happen.
> 
> I will write a new patchset with the assert in a separate commit.
> thanks

Oh, I see. I really meant

assert(!s->write_msgfds && !s->write_msgfds_num);

this assert can go into tcp_chr_write.

-- 
MST