From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40147 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pn74P-0006lk-Iw for qemu-devel@nongnu.org; Wed, 09 Feb 2011 05:14:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pn74N-0002vh-Bn for qemu-devel@nongnu.org; Wed, 09 Feb 2011 05:14:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51848) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pn74N-0002uz-4o for qemu-devel@nongnu.org; Wed, 09 Feb 2011 05:14:43 -0500 Date: Wed, 9 Feb 2011 10:14:33 +0000 From: "Daniel P. Berrange" Subject: Re: [Qemu-devel] [V4 PATCH 2/8] Provide chroot environment server side interfaces Message-ID: <20110209101433.GB25870@redhat.com> References: <1296537693-16406-1-git-send-email-mohan@in.ibm.com> <1296537939-16649-1-git-send-email-mohan@in.ibm.com> <20110201103222.GC1725@redhat.com> <201102081747.42543.mohan@in.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <201102081747.42543.mohan@in.ibm.com> Reply-To: "Daniel P. Berrange" List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "M. Mohan Kumar" Cc: blauwirbel@gmail.com, stefanha@gmail.com, qemu-devel@nongnu.org, Anthony Liguori On Tue, Feb 08, 2011 at 05:47:42PM +0530, M. Mohan Kumar wrote: > Hi, > > Is it okay to fork the chroot process in the fsdev parameter parsing time? Can > we ask other initialization routines to not create threads till chroot process > is forked? > > Following code snippet forks the chroot process during fsdev parameter parsing > to avoid the problems associated with forking in multi thread environment. It is slightly fragile because a change elsewhere in QEMU might silently cause trouble. More critically though, it won't work if you start a VM without any fsdev configured on the command line, and then use the monitor to hotplug one on the fly once QEMU is up & running... > > diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c > index 0b33290..2dad688 100644 > --- a/fsdev/qemu-fsdev.c > +++ b/fsdev/qemu-fsdev.c > @@ -17,6 +17,8 @@ > #include "osdep.h" > #include "qemu-common.h" > #include "qemu-config.h" > +#include "qemu_socket.h" > +#include "hw/9pfs/virtio-9p-chroot.h" > > static QTAILQ_HEAD(FsTypeEntry_head, FsTypeListEntry) fstype_entries = > QTAILQ_HEAD_INITIALIZER(fstype_entries); > @@ -72,6 +74,34 @@ int qemu_fsdev_add(QemuOpts *opts) > fsle->fse.security_model = qemu_strdup(sec_model); > fsle->fse.ops = FsTypes[i].ops; > > + if (!strcmp(fsle->fse.security_model, "passthrough")) { > + uint64_t code; > + if (v9fs_chroot(&fsle->fse) < 0) { > + exit(1); > + } > + > + /* > + * Chroot process sends 0 to indicate chroot process creation is > + * successful > + */ > + if (read(fsle->fse.chroot_socket, &code, sizeof(code)) != > sizeof(code)) { > + fprintf(stderr, "chroot process creation failed"); > + exit(1); > + } > + if (code != 0) { > + switch (code >> 32) { > + case CHROOT_ERROR: > + fprintf(stderr, "chroot system call failed: %s", > + strerror(code & 0xFFFFFFFF)); > + break; > + case SETSID_ERROR: > + fprintf(stderr, "setsid failed: %s", strerror(code & > 0xFFFFFFFF)); > + break; > + } > + exit(1); > + } > + } > + > QTAILQ_INSERT_TAIL(&fstype_entries, fsle, next); > return 0; > > > > > On Tuesday 01 February 2011 4:02:22 pm Daniel P. Berrange wrote: > > On Tue, Feb 01, 2011 at 10:55:39AM +0530, M. Mohan Kumar wrote: > > > Implement chroot server side interfaces like sending the file > > > descriptor to qemu process, reading the object request from socket etc. > > > Also add chroot main function and other helper routines. > > > > > > Signed-off-by: M. Mohan Kumar > > > --- > > > > > > Makefile.objs | 1 + > > > hw/9pfs/virtio-9p-chroot.c | 212 > > > ++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/virtio-9p-chroot.h > > > | 41 +++++++++ > > > hw/9pfs/virtio-9p.c | 33 +++++++ > > > hw/file-op-9p.h | 3 + > > > 5 files changed, 290 insertions(+), 0 deletions(-) > > > create mode 100644 hw/9pfs/virtio-9p-chroot.c > > > create mode 100644 hw/9pfs/virtio-9p-chroot.h > > > > > > diff --git a/Makefile.objs b/Makefile.objs > > > index bc0344c..3007b6d 100644 > > > --- a/Makefile.objs > > > +++ b/Makefile.objs > > > +/* > > > + * Fork a process and chroot into the share path. Communication > > > + * between qemu process and chroot process happens via socket > > > + * All file descriptors (including stdout and stderr) are closed > > > + * except one socket descriptor (which is used for communicating > > > + * between qemu process and chroot process) > > > + */ > > > +int v9fs_chroot(FsContext *fs_ctx) > > > +{ > > > + int fd_pair[2], chroot_sock, error; > > > + V9fsFileObjectRequest request; > > > + pid_t pid; > > > + uint64_t code; > > > + FdInfo fd_info; > > > + > > > + if (socketpair(PF_UNIX, SOCK_STREAM, 0, fd_pair) < 0) { > > > + error_report("socketpair %s", strerror(errno)); > > > + return -1; > > > + } > > > + > > > + pid = fork(); > > > + if (pid < 0) { > > > + error_report("fork %s", strerror(errno)); > > > + return -1; > > > + } > > > + if (pid != 0) { > > > + fs_ctx->chroot_socket = fd_pair[0]; > > > + close(fd_pair[1]); > > > + return 0; > > > + } > > > + > > > + close(fd_pair[0]); > > > + chroot_sock = fd_pair[1]; > > > + if (chroot(fs_ctx->fs_root) < 0) { > > > + code = CHROOT_ERROR << 32 | errno; > > > + error = qemu_write_full(chroot_sock, &code, sizeof(code)); > > > + _exit(1); > > > + } > > > + > > > + error = chroot_daemonize(chroot_sock); > > > + if (error) { > > > + code = SETSID_ERROR << 32 | error; > > > + error = qemu_write_full(chroot_sock, &code, sizeof(code)); > > > + _exit(1); > > > + } > > > + > > > + /* > > > + * Write 0 to chroot socket to indicate chroot process creation is > > > + * successful > > > + */ > > > + code = 0; > > > + if (qemu_write_full(chroot_sock, &code, sizeof(code)) > > > + != sizeof(code)) { > > > + _exit(1); > > > + } > > > + /* get the request from the socket */ > > > + while (1) { > > > + memset(&fd_info, 0, sizeof(fd_info)); > > > + if (chroot_read_request(chroot_sock, &request) == EIO) { > > > + fd_info.fi_fd = 0; > > > + fd_info.fi_error = EIO; > > > + fd_info.fi_flags = FI_SOCKERR; > > > + chroot_sendfd(chroot_sock, &fd_info); > > > + continue; > > > + } > > > + qemu_free((void *)request.path.path); > > > + if (request.data.oldpath_len) { > > > + qemu_free((void *)request.path.old_path); > > > + } > > > + } > > > +} > > > > There is a subtle problem with using fork() in a multi-threaded > > program that I was recently made aware of in libvirt. In short > > if you have a multi-threaded program that calls fork(), then > > the child process must only use POSIX functions that are > > declared 'async signal safe', until the child calls exec() or > > exit(). In particular any malloc()/free() related functions > > are *not* async signal safe. > > > > http://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html > > > > "If a multi-threaded process calls fork(), the new process shall contain > > a replica of the calling thread and its entire address space, possibly > > including the states of mutexes and other resources. Consequently, to > > avoid errors, the child process may only execute async-signal-safe > > operations until such time as one of the exec functions is called." > > > > One example problem scenario. Thread 1 is currently doing a > > malloc() and the malloc() impl is holding a mutex. Thread 2 > > now does a fork(), and in the child process calls malloc(). > > The child process will deadlock / hang forever because there > > is nothing which will ever release the malloc() mutex that > > was originally held by Thread 1. See also this thread which > > brought the problem to my attention: > > > > http://lists.gnu.org/archive/html/coreutils/2011-01/msg00085.html > > > > Regards, > > Daniel > > ---- > M. Mohan Kumar 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 :|