On Wed, Feb 10, 2016 at 01:53:49PM +0200, Michael S. Tsirkin wrote: > 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 <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? > > >> > > >>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.
Err, no it can't. tcp_chr_write() is the thing responsible for sending FDs, so we can't assert that no FDs are set, as that'll make it impossible for anything to send FDs. 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 :|
