Re: [Qemu-devel] [V4 PATCH 2/8] Provide chroot environment server side interfaces

[Daniel P. Berrange]

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 <address@hidden>
> > > ---
> > > 
> > >  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 :|