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