[Qemu-devel] [V4 PATCH 0/8] virtio-9p: Use chroot to safely access files in passthrough security model

[Qemu-devel] [V4 PATCH 0/8] virtio-9p: Use chroot to safely access files in passthrough security model

[M. Mohan Kumar]

In passthrough security model, following symbolic links in the server
side could result in TOCTTOU vulnerabilities.

This patchset resolves this issue by creating a dedicated process which
chroots into the share path and all file object access is done in the
chroot environment.

This patchset implements chroot enviroment, provides necessary functions
that can be used by the passthrough function calls.

Changes from version V3
* Return EIO incase of socket read/write fail instead of exiting
* Changed data types as suggested by Blue Swirl
* Chroot process reports error through qemu process when possible

Changes from version V2
* Treat socket IO errors as fatal, ie qemu will exit
* Split patchset based on chroot side (server) and qemu side(client)
  functionalities

M. Mohan Kumar (8):
  Implement qemu_read_full
  Provide chroot environment server side interfaces
  Add client side interfaces for chroot environment
  Add support to open a file in chroot environment
  Create support in chroot environment
  Support for creating special files
  Move file post creation changes to none security model
  Chroot environment for other functions

 Makefile.objs              |    1 +
 hw/9pfs/virtio-9p-chroot.c |  463 ++++++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/virtio-9p-chroot.h |   44 +++++
 hw/9pfs/virtio-9p-local.c  |  455 +++++++++++++++++++++++++++++++++++--------
 hw/9pfs/virtio-9p.c        |   33 +++
 hw/file-op-9p.h            |    3 +
 osdep.c                    |   32 +++
 qemu-common.h              |    2 +
 8 files changed, 953 insertions(+), 80 deletions(-)
 create mode 100644 hw/9pfs/virtio-9p-chroot.c
 create mode 100644 hw/9pfs/virtio-9p-chroot.h

-- 
1.7.3.4

[Qemu-devel] [V4 PATCH 1/8] virtio-9p: Implement qemu_read_full

[M. Mohan Kumar]

Add qemu_read_full function

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 osdep.c       |   32 ++++++++++++++++++++++++++++++++
 qemu-common.h |    2 ++
 2 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/osdep.c b/osdep.c
index 327583b..8d84a88 100644
--- a/osdep.c
+++ b/osdep.c
@@ -127,6 +127,38 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t count)
 }
 
 /*
+ * A variant of read(2) which handles interrupted read.
+ * Simlar to qemu_write_full function
+ *
+ * Return the number of bytes read.
+ *
+ * This function does not work with non-blocking fd's.
+ * errno is set if fewer than `count' bytes are read because of any
+ * error
+ */
+ssize_t qemu_read_full(int fd, void *buf, size_t count)
+{
+    ssize_t ret = 0;
+    ssize_t total = 0;
+
+    while (count) {
+        ret = read(fd, buf, count);
+        if (ret <= 0) {
+            if (errno == EINTR) {
+                continue;
+            }
+            break;
+        }
+
+        count -= ret;
+        buf += ret;
+        total += ret;
+    }
+
+    return total;
+}
+
+/*
  * Opens a socket with FD_CLOEXEC set
  */
 int qemu_socket(int domain, int type, int protocol)
diff --git a/qemu-common.h b/qemu-common.h
index b3957f1..0f60e08 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -190,6 +190,8 @@ void qemu_mutex_unlock_iothread(void);
 int qemu_open(const char *name, int flags, ...);
 ssize_t qemu_write_full(int fd, const void *buf, size_t count)
     QEMU_WARN_UNUSED_RESULT;
+ssize_t qemu_read_full(int fd, void *buf, size_t count)
+    QEMU_WARN_UNUSED_RESULT;
 void qemu_set_cloexec(int fd);
 
 #ifndef _WIN32
-- 
1.7.3.4

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

[M. Mohan Kumar]

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 <mohan@in.ibm.com>
---
 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
@@ -273,6 +273,7 @@ hw-obj-$(CONFIG_SOUND) += $(sound-obj-y)
 9pfs-nested-$(CONFIG_VIRTFS) = virtio-9p-debug.o
 9pfs-nested-$(CONFIG_VIRTFS) +=  virtio-9p-local.o virtio-9p-xattr.o
 9pfs-nested-$(CONFIG_VIRTFS) +=   virtio-9p-xattr-user.o virtio-9p-posix-acl.o
+9pfs-nested-$(CONFIG_VIRTFS) +=   virtio-9p-chroot.o
 
 hw-obj-$(CONFIG_REALLY_VIRTFS) += $(addprefix 9pfs/, $(9pfs-nested-y))
 $(addprefix 9pfs/, $(9pfs-nested-y)): CFLAGS +=  -I$(SRC_PATH)/hw/
diff --git a/hw/9pfs/virtio-9p-chroot.c b/hw/9pfs/virtio-9p-chroot.c
new file mode 100644
index 0000000..5150ff0
--- /dev/null
+++ b/hw/9pfs/virtio-9p-chroot.c
@@ -0,0 +1,212 @@
+/*
+ * Virtio 9p chroot environment for contained access to exported path
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ * M. Mohan Kumar <mohan@in.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the copying file in the top-level directory
+ *
+ */
+
+#include <sys/fsuid.h>
+#include <sys/resource.h>
+#include <signal.h>
+#include "virtio.h"
+#include "qemu_socket.h"
+#include "qemu-thread.h"
+#include "qerror.h"
+#include "virtio-9p.h"
+#include "virtio-9p-chroot.h"
+
+/*
+ * Structure used by chroot functions to transmit file descriptor and
+ * error info
+ */
+typedef struct {
+    int fi_fd;
+#define FI_FDVALID 1
+#define FI_SOCKERR 2
+    int fi_flags;
+    int fi_error;
+} FdInfo;
+
+union MsgControl {
+    struct cmsghdr cmsg;
+    char control[CMSG_SPACE(sizeof(int))];
+};
+
+/* Send file descriptor and error status to qemu process */
+static void chroot_sendfd(int sockfd, const FdInfo *fd_info)
+{
+    struct msghdr msg = { };
+    struct iovec iov;
+    struct cmsghdr *cmsg;
+    int retval;
+    union MsgControl msg_control;
+
+    iov.iov_base = (void *)fd_info;
+    iov.iov_len = sizeof(*fd_info);
+
+    memset(&msg, 0, sizeof(msg));
+    msg.msg_iov = &iov;
+    msg.msg_iovlen = 1;
+    /* No ancillary data on error/fd invalid flag */
+    if (fd_info->fi_flags & FI_FDVALID) {
+        msg.msg_control = &msg_control;
+        msg.msg_controllen = sizeof(msg_control);
+
+        cmsg = &msg_control.cmsg;
+        cmsg->cmsg_len = CMSG_LEN(sizeof(fd_info->fi_fd));
+        cmsg->cmsg_level = SOL_SOCKET;
+        cmsg->cmsg_type = SCM_RIGHTS;
+        memcpy(CMSG_DATA(cmsg), &fd_info->fi_fd, sizeof(fd_info->fi_fd));
+    }
+    retval = sendmsg(sockfd, &msg, 0);
+    if (retval == EPIPE || retval == EBADF) {
+        _exit(1);
+    }
+    if (fd_info->fi_flags & FI_FDVALID) {
+        close(fd_info->fi_fd);
+    }
+}
+
+/* Read V9fsFileObjectRequest written by QEMU process */
+static int chroot_read_request(int sockfd, V9fsFileObjectRequest *request)
+{
+    int retval;
+    retval = qemu_read_full(sockfd, request, sizeof(request->data));
+    if (retval != sizeof(request->data)) {
+        if (errno == EBADF) {
+            _exit(1);
+        }
+        return EIO;
+    }
+    request->path.path = qemu_mallocz(request->data.path_len + 1);
+    retval = qemu_read_full(sockfd, (void *)request->path.path,
+                        request->data.path_len);
+    if (retval != request->data.path_len) {
+        qemu_free((void *)request->path.path);
+        if (errno == EBADF) {
+            _exit(1);
+        }
+        return EIO;
+    }
+    if (request->data.oldpath_len > 0) {
+        request->path.old_path =
+                qemu_mallocz(request->data.oldpath_len + 1);
+        retval = qemu_read_full(sockfd, (void *)request->path.old_path,
+                            request->data.oldpath_len);
+        if (retval != request->data.oldpath_len) {
+            qemu_free((void *)request->path.path);
+            qemu_free((void *)request->path.old_path);
+            if (errno == EBADF) {
+                _exit(1);
+            }
+            return EIO;
+        }
+    }
+    return 0;
+}
+
+static int chroot_daemonize(int chroot_sock)
+{
+    sigset_t sigset;
+    struct rlimit nr_fd;
+    int fd;
+
+    /* Block all signals for this process */
+    sigprocmask(SIG_SETMASK, &sigset, NULL);
+
+    /* Daemonize */
+    if (setsid() < 0) {
+        return errno;
+    }
+
+    /* Close other file descriptors */
+    getrlimit(RLIMIT_NOFILE, &nr_fd);
+    for (fd = 0; fd < nr_fd.rlim_cur; fd++) {
+        if (fd != chroot_sock) {
+            close(fd);
+        }
+    }
+    chdir("/");
+
+    /* Create files with mode as requested by client */
+    umask(0);
+    return 0;
+}
+
+/*
+ * 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);
+        }
+    }
+}
diff --git a/hw/9pfs/virtio-9p-chroot.h b/hw/9pfs/virtio-9p-chroot.h
new file mode 100644
index 0000000..6f3fd14
--- /dev/null
+++ b/hw/9pfs/virtio-9p-chroot.h
@@ -0,0 +1,41 @@
+#ifndef _QEMU_VIRTIO_9P_CHROOT_H
+#define _QEMU_VIRTIO_9P_CHROOT_H
+
+/* types for V9fsFileObjectRequest */
+#define T_OPEN      1
+#define T_CREATE    2
+#define T_MKDIR     3
+#define T_MKNOD     4
+#define T_SYMLINK   5
+#define T_LINK      6
+#define CHROOT_ERROR 1ULL
+#define SETSID_ERROR 2ULL
+
+struct V9fsFileObjectData
+{
+    int flags;
+    int mode;
+    uid_t uid;
+    gid_t gid;
+    dev_t dev;
+    int path_len;
+    int oldpath_len;
+    int type;
+};
+
+struct V9fsFileObjectPath
+{
+    const char *path;
+    const char *old_path;
+};
+
+typedef struct V9fsFileObjectRequest
+{
+    struct V9fsFileObjectData data;
+    struct V9fsFileObjectPath path;
+} V9fsFileObjectRequest;
+
+
+int v9fs_chroot(FsContext *fs_ctx);
+
+#endif /* _QEMU_VIRTIO_9P_CHROOT_H */
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 27e7750..fc006a9 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -14,10 +14,13 @@
 #include "virtio.h"
 #include "pc.h"
 #include "qemu_socket.h"
+#include "qerror.h"
 #include "virtio-9p.h"
 #include "fsdev/qemu-fsdev.h"
 #include "virtio-9p-debug.h"
 #include "virtio-9p-xattr.h"
+#include "virtio-9p-chroot.h"
+#include <pthread.h>
 
 int debug_9p_pdu;
 
@@ -3743,5 +3746,35 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
                         s->tag_len;
     s->vdev.get_config = virtio_9p_get_config;
 
+    if (s->ctx.fs_sm == SM_PASSTHROUGH) {
+        uint64_t code;
+        pthread_mutex_init(&s->ctx.chroot_mutex, 0);
+        s->ctx.chroot_ioerror = 0;
+        if (v9fs_chroot(&s->ctx) < 0) {
+            exit(1);
+        }
+
+        /*
+         * Chroot process sends 0 to indicate chroot process creation is
+         * successful
+         */
+        if (read(s->ctx.chroot_socket, &code, sizeof(code)) != sizeof(code)) {
+            error_report("chroot process creation failed");
+            exit(1);
+        }
+        if (code != 0) {
+            switch (code >> 32) {
+            case CHROOT_ERROR:
+                error_report("chroot system call failed: %s",
+                                strerror(code & 0xFFFFFFFF));
+                break;
+            case SETSID_ERROR:
+                error_report("setsid failed: %s", strerror(code & 0xFFFFFFFF));
+                break;
+            }
+            exit(1);
+        }
+    }
+
     return &s->vdev;
 }
diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h
index c7731c2..0cd7728 100644
--- a/hw/file-op-9p.h
+++ b/hw/file-op-9p.h
@@ -55,6 +55,9 @@ typedef struct FsContext
     SecModel fs_sm;
     uid_t uid;
     struct xattr_operations **xops;
+    pthread_mutex_t chroot_mutex;
+    int chroot_socket;
+    int chroot_ioerror;
 } FsContext;
 
 extern void cred_init(FsCred *);
-- 
1.7.3.4

[Qemu-devel] [V4 PATCH 3/8] Add client side interfaces for chroot environment

[M. Mohan Kumar]

Define QEMU side interfaces used for chroot environment.

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 hw/9pfs/virtio-9p-chroot.c |   87 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/hw/9pfs/virtio-9p-chroot.c b/hw/9pfs/virtio-9p-chroot.c
index 5150ff0..b466d9a 100644
--- a/hw/9pfs/virtio-9p-chroot.c
+++ b/hw/9pfs/virtio-9p-chroot.c
@@ -111,6 +111,86 @@ static int chroot_read_request(int sockfd, V9fsFileObjectRequest *request)
     return 0;
 }
 
+/* Receive file descriptor and error status from chroot process */
+static int v9fs_receivefd(int sockfd, int *error)
+{
+    struct msghdr msg = { };
+    struct iovec iov;
+    union MsgControl msg_control;
+    struct cmsghdr *cmsg;
+    int retval, fd;
+    FdInfo fd_info;
+
+    iov.iov_base = &fd_info;
+    iov.iov_len = sizeof(fd_info);
+
+    memset(&msg, 0, sizeof(msg));
+    msg.msg_iov = &iov;
+    msg.msg_iovlen = 1;
+    msg.msg_control = &msg_control;
+    msg.msg_controllen = sizeof(msg_control);
+
+    retval = recvmsg(sockfd, &msg, 0);
+    if (retval < 0) {
+        *error = EIO;
+        return -EIO;
+    }
+
+    if (fd_info.fi_flags & FI_SOCKERR) {
+        return -EIO;
+    }
+
+    /* If error is set, ancillary data is not present */
+    if (fd_info.fi_error) {
+        *error = fd_info.fi_error;
+        return -1;
+    }
+
+    if (!(fd_info.fi_flags & FI_FDVALID)) {
+        return 0;
+    }
+
+    for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
+        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
+                cmsg->cmsg_level != SOL_SOCKET ||
+                cmsg->cmsg_type != SCM_RIGHTS) {
+            continue;
+        }
+        fd = *((int *)CMSG_DATA(cmsg));
+        return fd;
+    }
+
+    *error = EAGAIN;
+    return -1;
+}
+
+/*
+ * V9fsFileObjectRequest is written into the socket by QEMU process.
+ * Then this request is read by chroot process using read_request function
+ */
+static int v9fs_write_request(int sockfd, V9fsFileObjectRequest *request)
+{
+    int retval, length;
+    char *buff, *buffp;
+
+    length = sizeof(request->data) + request->data.path_len +
+                    request->data.oldpath_len;
+
+    buff = qemu_malloc(length);
+    buffp = buff;
+    memcpy(buffp, &request->data, sizeof(request->data));
+    buffp += sizeof(request->data);
+    memcpy(buffp, request->path.path, request->data.path_len);
+    buffp += request->data.path_len;
+    memcpy(buffp, request->path.old_path, request->data.oldpath_len);
+
+    retval = qemu_write_full(sockfd, buff, length);
+    if (retval != length) {
+        return EIO;
+    }
+    return 0;
+}
+
 static int chroot_daemonize(int chroot_sock)
 {
     sigset_t sigset;
@@ -139,6 +219,12 @@ static int chroot_daemonize(int chroot_sock)
     return 0;
 }
 
+static void chroot_dummy(void)
+{
+    (void)v9fs_receivefd;
+    (void)v9fs_write_request;
+}
+
 /*
  * Fork a process and chroot into the share path. Communication
  * between qemu process and chroot process happens via socket
@@ -184,6 +270,7 @@ int v9fs_chroot(FsContext *fs_ctx)
         error = qemu_write_full(chroot_sock, &code, sizeof(code));
         _exit(1);
     }
+    chroot_dummy();
 
     /*
      * Write 0 to chroot socket to indicate chroot process creation is
-- 
1.7.3.4

[Qemu-devel] [V4 PATCH 4/8] Add support to open a file in chroot environment

[M. Mohan Kumar]

This patch adds both server and client side support to open a file
(directory) in the chroot environment

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 hw/9pfs/virtio-9p-chroot.c |   53 ++++++++++++++++++++++++++++++++++++++-----
 hw/9pfs/virtio-9p-chroot.h |    1 +
 hw/9pfs/virtio-9p-local.c  |   52 +++++++++++++++++++++++++++++++++++++++---
 3 files changed, 95 insertions(+), 11 deletions(-)

diff --git a/hw/9pfs/virtio-9p-chroot.c b/hw/9pfs/virtio-9p-chroot.c
index b466d9a..7da0702 100644
--- a/hw/9pfs/virtio-9p-chroot.c
+++ b/hw/9pfs/virtio-9p-chroot.c
@@ -191,6 +191,42 @@ static int v9fs_write_request(int sockfd, V9fsFileObjectRequest *request)
     return 0;
 }
 
+/* Return opened file descriptor on success or -1 on error */
+int v9fs_request(FsContext *fs_ctx, V9fsFileObjectRequest *request,
+                        int *error)
+{
+    int fd;
+    pthread_mutex_lock(&fs_ctx->chroot_mutex);
+    if (fs_ctx->chroot_ioerror) {
+        *error = EIO;
+        fd = -1;
+        goto unlock;
+    }
+    v9fs_write_request(fs_ctx->chroot_socket, request);
+    fd = v9fs_receivefd(fs_ctx->chroot_socket, error);
+    if (fd == -EIO) {
+        fs_ctx->chroot_ioerror = 1;
+    }
+unlock:
+    pthread_mutex_unlock(&fs_ctx->chroot_mutex);
+    return fd;
+}
+
+/*
+ * Helper routine to open a file and return fd and error status in
+ * FdInfo structure
+ */
+static void chroot_do_open(V9fsFileObjectRequest *request, FdInfo *fd_info)
+{
+    fd_info->fi_fd = open(request->path.path, request->data.flags);
+    if (fd_info->fi_fd < 0) {
+        fd_info->fi_error = errno;
+    } else {
+        fd_info->fi_error = 0;
+        fd_info->fi_flags = FI_FDVALID;
+    }
+}
+
 static int chroot_daemonize(int chroot_sock)
 {
     sigset_t sigset;
@@ -219,12 +255,6 @@ static int chroot_daemonize(int chroot_sock)
     return 0;
 }
 
-static void chroot_dummy(void)
-{
-    (void)v9fs_receivefd;
-    (void)v9fs_write_request;
-}
-
 /*
  * Fork a process and chroot into the share path. Communication
  * between qemu process and chroot process happens via socket
@@ -270,7 +300,6 @@ int v9fs_chroot(FsContext *fs_ctx)
         error = qemu_write_full(chroot_sock, &code, sizeof(code));
         _exit(1);
     }
-    chroot_dummy();
 
     /*
      * Write 0 to chroot socket to indicate chroot process creation is
@@ -291,6 +320,16 @@ int v9fs_chroot(FsContext *fs_ctx)
             chroot_sendfd(chroot_sock, &fd_info);
             continue;
         }
+        switch (request.data.type) {
+        case T_OPEN:
+            chroot_do_open(&request, &fd_info);
+            break;
+        default:
+            fd_info.fi_fd = 0;
+            fd_info.fi_error = EIO;
+            break;
+        }
+        chroot_sendfd(chroot_sock, &fd_info);
         qemu_free((void *)request.path.path);
         if (request.data.oldpath_len) {
             qemu_free((void *)request.path.old_path);
diff --git a/hw/9pfs/virtio-9p-chroot.h b/hw/9pfs/virtio-9p-chroot.h
index 6f3fd14..ffeeddc 100644
--- a/hw/9pfs/virtio-9p-chroot.h
+++ b/hw/9pfs/virtio-9p-chroot.h
@@ -37,5 +37,6 @@ typedef struct V9fsFileObjectRequest
 
 
 int v9fs_chroot(FsContext *fs_ctx);
+int v9fs_request(FsContext *fs_ctx, V9fsFileObjectRequest *or, int *error);
 
 #endif /* _QEMU_VIRTIO_9P_CHROOT_H */
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 0a015de..96e8181 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -13,6 +13,7 @@
 #include "virtio.h"
 #include "virtio-9p.h"
 #include "virtio-9p-xattr.h"
+#include "virtio-9p-chroot.h"
 #include <arpa/inet.h>
 #include <pwd.h>
 #include <grp.h>
@@ -20,6 +21,36 @@
 #include <sys/un.h>
 #include <attr/xattr.h>
 
+/* Helper routine to fill V9fsFileObjectRequest structure */
+static void fill_request(V9fsFileObjectRequest *request, const char *path,
+                FsCred *credp)
+{
+    memset(request, 0, sizeof(*request));
+    request->data.path_len = strlen(path);
+    request->path.path = qemu_strdup(path);
+    if (credp) {
+        request->data.mode = credp->fc_mode;
+        request->data.uid = credp->fc_uid;
+        request->data.gid = credp->fc_gid;
+        request->data.dev = credp->fc_rdev;
+    }
+}
+
+static int passthrough_open(FsContext *fs_ctx, const char *path, int flags)
+{
+    V9fsFileObjectRequest request;
+    int fd, error = 0;
+
+    fill_request(&request, path, NULL);
+    request.data.flags = flags;
+    request.data.type = T_OPEN;
+    fd = v9fs_request(fs_ctx, &request, &error);
+    if (fd < 0) {
+        errno = error;
+    }
+    qemu_free((void *)request.path.path);
+    return fd;
+}
 
 static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
 {
@@ -138,14 +169,27 @@ static int local_closedir(FsContext *ctx, DIR *dir)
     return closedir(dir);
 }
 
-static int local_open(FsContext *ctx, const char *path, int flags)
+static int local_open(FsContext *fs_ctx, const char *path, int flags)
 {
-    return open(rpath(ctx, path), flags);
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        return passthrough_open(fs_ctx, path, flags);
+    } else {
+        return open(rpath(fs_ctx, path), flags);
+    }
 }
 
-static DIR *local_opendir(FsContext *ctx, const char *path)
+static DIR *local_opendir(FsContext *fs_ctx, const char *path)
 {
-    return opendir(rpath(ctx, path));
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        int fd;
+        fd = passthrough_open(fs_ctx, path, O_DIRECTORY);
+        if (fd < 0) {
+            return NULL;
+        }
+        return fdopendir(fd);
+    } else {
+        return opendir(rpath(fs_ctx, path));
+    }
 }
 
 static void local_rewinddir(FsContext *ctx, DIR *dir)
-- 
1.7.3.4

[Qemu-devel] [V4 PATCH 5/8] Create support in chroot environment

[M. Mohan Kumar]

Add both server & client side interfaces to create regular files in
chroot environment

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 hw/9pfs/virtio-9p-chroot.c |   41 +++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/virtio-9p-local.c  |   22 ++++++++++++++++++++--
 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/virtio-9p-chroot.c b/hw/9pfs/virtio-9p-chroot.c
index 7da0702..890dd78 100644
--- a/hw/9pfs/virtio-9p-chroot.c
+++ b/hw/9pfs/virtio-9p-chroot.c
@@ -227,6 +227,44 @@ static void chroot_do_open(V9fsFileObjectRequest *request, FdInfo *fd_info)
     }
 }
 
+/*
+ * Helper routine to create a file and return the file descriptor and
+ * error status in FdInfo structure.
+ */
+static void chroot_do_create(V9fsFileObjectRequest *request, FdInfo *fd_info)
+{
+    uid_t cur_uid;
+    gid_t cur_gid;
+
+    cur_uid = geteuid();
+    cur_gid = getegid();
+
+    fd_info->fi_fd = -1;
+
+    if (setfsuid(request->data.uid) < 0) {
+        fd_info->fi_error = errno;
+        return;
+    }
+    if (setfsgid(request->data.gid) < 0) {
+        fd_info->fi_error = errno;
+        goto unset_uid;
+    }
+
+    fd_info->fi_fd = open(request->path.path, request->data.flags,
+                        request->data.mode);
+
+    if (fd_info->fi_fd < 0) {
+        fd_info->fi_error = errno;
+    } else {
+        fd_info->fi_error = 0;
+        fd_info->fi_flags = FI_FDVALID;
+    }
+
+    setfsgid(cur_gid);
+unset_uid:
+    setfsuid(cur_uid);
+}
+
 static int chroot_daemonize(int chroot_sock)
 {
     sigset_t sigset;
@@ -324,6 +362,9 @@ int v9fs_chroot(FsContext *fs_ctx)
         case T_OPEN:
             chroot_do_open(&request, &fd_info);
             break;
+        case T_CREATE:
+            chroot_do_create(&request, &fd_info);
+            break;
         default:
             fd_info.fi_fd = 0;
             fd_info.fi_error = EIO;
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 96e8181..afb088a 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -52,6 +52,23 @@ static int passthrough_open(FsContext *fs_ctx, const char *path, int flags)
     return fd;
 }
 
+static int passthrough_create(FsContext *fs_ctx, const char *path, int flags,
+                    FsCred *credp)
+{
+    V9fsFileObjectRequest request;
+    int fd, error = 0;
+
+    fill_request(&request, path, credp);
+    request.data.flags = flags;
+    request.data.type = T_CREATE;
+    fd = v9fs_request(fs_ctx, &request, &error);
+    if (fd < 0) {
+        errno = error;
+    }
+    qemu_free((void *)request.path.path);
+    return fd;
+}
+
 static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
 {
     int err;
@@ -376,8 +393,7 @@ static int local_open2(FsContext *fs_ctx, const char *path, int flags,
             serrno = errno;
             goto err_end;
         }
-    } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
-               (fs_ctx->fs_sm == SM_NONE)) {
+    } else if (fs_ctx->fs_sm == SM_NONE) {
         fd = open(rpath(fs_ctx, path), flags, credp->fc_mode);
         if (fd == -1) {
             return fd;
@@ -387,6 +403,8 @@ static int local_open2(FsContext *fs_ctx, const char *path, int flags,
             serrno = errno;
             goto err_end;
         }
+    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        fd = passthrough_create(fs_ctx, path, flags, credp);
     }
     return fd;
 
-- 
1.7.3.4

[Qemu-devel] [V4 PATCH 6/8] Support for creating special files

[M. Mohan Kumar]

Add both server and client side interfaces to create special files
(directory, device nodes, links and symbolic links)

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 hw/9pfs/virtio-9p-chroot.c |   84 ++++++++++++++++++++++++++
 hw/9pfs/virtio-9p-chroot.h |    2 +
 hw/9pfs/virtio-9p-local.c  |  141 ++++++++++++++++++++++++++++++++++----------
 3 files changed, 196 insertions(+), 31 deletions(-)

diff --git a/hw/9pfs/virtio-9p-chroot.c b/hw/9pfs/virtio-9p-chroot.c
index 890dd78..85a42e6 100644
--- a/hw/9pfs/virtio-9p-chroot.c
+++ b/hw/9pfs/virtio-9p-chroot.c
@@ -227,6 +227,31 @@ static void chroot_do_open(V9fsFileObjectRequest *request, FdInfo *fd_info)
     }
 }
 
+int v9fs_create_special(FsContext *fs_ctx,
+                V9fsFileObjectRequest *request, int *error)
+{
+    int fd;
+    pthread_mutex_lock(&fs_ctx->chroot_mutex);
+
+    if (fs_ctx->chroot_ioerror) {
+        *error = EIO;
+        goto unlock;
+    }
+    *error = 0;
+    v9fs_write_request(fs_ctx->chroot_socket, request);
+    fd = v9fs_receivefd(fs_ctx->chroot_socket, error);
+    if (fd == -EIO) {
+        fs_ctx->chroot_ioerror = 1;
+    }
+unlock:
+    pthread_mutex_unlock(&fs_ctx->chroot_mutex);
+    if (*error) {
+        return -1;
+    } else {
+        return 0;
+    }
+}
+
 /*
  * Helper routine to create a file and return the file descriptor and
  * error status in FdInfo structure.
@@ -265,6 +290,59 @@ unset_uid:
     setfsuid(cur_uid);
 }
 
+/*
+ * Create directory, symbolic link, link, device node and regular files
+ * Similar to create, but it does not return the fd of created object
+ * Returns 0 on success, returns errno on failure
+ */
+static void chroot_do_create_special(V9fsFileObjectRequest *request,
+                FdInfo *fd_info)
+{
+    int cur_uid, cur_gid;
+
+    cur_uid = geteuid();
+    cur_gid = getegid();
+
+    fd_info->fi_fd = -1;
+    fd_info->fi_error = 0;
+
+    if (setfsuid(request->data.uid) < 0) {
+        fd_info->fi_error = errno;
+        return;
+    }
+    if (setfsgid(request->data.gid) < 0) {
+        fd_info->fi_error = errno;
+        goto unset_uid;
+    }
+
+    switch (request->data.type) {
+    case T_MKDIR:
+        fd_info->fi_fd = mkdir(request->path.path, request->data.mode);
+        break;
+    case T_SYMLINK:
+        fd_info->fi_fd = symlink(request->path.old_path, request->path.path);
+        break;
+    case T_LINK:
+        fd_info->fi_fd = link(request->path.old_path, request->path.path);
+        break;
+    default:
+        fd_info->fi_fd = mknod(request->path.path, request->data.mode,
+                        request->data.dev);
+        break;
+    }
+
+    if (fd_info->fi_fd < 0) {
+        fd_info->fi_error = errno;
+    } else {
+        fd_info->fi_error = 0;
+        fd_info->fi_fd = -1;
+    }
+
+    setfsgid(cur_gid);
+unset_uid:
+    setfsuid(cur_uid);
+}
+
 static int chroot_daemonize(int chroot_sock)
 {
     sigset_t sigset;
@@ -365,6 +443,12 @@ int v9fs_chroot(FsContext *fs_ctx)
         case T_CREATE:
             chroot_do_create(&request, &fd_info);
             break;
+        case T_MKDIR:
+        case T_SYMLINK:
+        case T_LINK:
+        case T_MKNOD:
+            chroot_do_create_special(&request, &fd_info);
+            break;
         default:
             fd_info.fi_fd = 0;
             fd_info.fi_error = EIO;
diff --git a/hw/9pfs/virtio-9p-chroot.h b/hw/9pfs/virtio-9p-chroot.h
index ffeeddc..c7ebb47 100644
--- a/hw/9pfs/virtio-9p-chroot.h
+++ b/hw/9pfs/virtio-9p-chroot.h
@@ -38,5 +38,7 @@ typedef struct V9fsFileObjectRequest
 
 int v9fs_chroot(FsContext *fs_ctx);
 int v9fs_request(FsContext *fs_ctx, V9fsFileObjectRequest *or, int *error);
+int v9fs_create_special(FsContext *fs_ctx,
+                V9fsFileObjectRequest *request, int *error);
 
 #endif /* _QEMU_VIRTIO_9P_CHROOT_H */
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index afb088a..c9ece6b 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -69,6 +69,78 @@ static int passthrough_create(FsContext *fs_ctx, const char *path, int flags,
     return fd;
 }
 
+static int passthrough_mknod(FsContext *fs_ctx, const char *path, FsCred *credp)
+{
+    V9fsFileObjectRequest request;
+    int retval, error = 0;
+
+    fill_request(&request, path, credp);
+    request.data.type = T_MKNOD;
+
+    retval = v9fs_create_special(fs_ctx, &request, &error);
+    if (retval < 0) {
+        errno = error;
+    }
+    return retval;
+}
+
+static int passthrough_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp)
+{
+    V9fsFileObjectRequest request;
+    int retval, error = 0;
+
+    fill_request(&request, path, credp);
+    request.data.type = T_MKDIR;
+
+    retval = v9fs_create_special(fs_ctx, &request, &error);
+    if (retval < 0) {
+        errno = error;
+    }
+    qemu_free((void *)request.path.path);
+    return retval;
+}
+
+static int passthrough_symlink(FsContext *fs_ctx, const char *oldpath,
+                const char *newpath, FsCred *credp)
+{
+    V9fsFileObjectRequest request;
+    int retval, error = 0;
+
+    fill_request(&request, newpath, credp);
+    request.data.type = T_SYMLINK;
+    request.data.oldpath_len = strlen(oldpath);
+    request.path.old_path = qemu_strdup(oldpath);
+
+    retval = v9fs_create_special(fs_ctx, &request, &error);
+    if (retval > 0) {
+        errno = error;
+        return 0;
+    }
+    qemu_free((void *)request.path.old_path);
+    qemu_free((void *)request.path.path);
+    return retval;
+}
+
+static int passthrough_link(FsContext *fs_ctx, const char *oldpath,
+                const char *newpath)
+{
+    V9fsFileObjectRequest request;
+    int retval, error = 0;
+
+    fill_request(&request, newpath, NULL);
+    request.data.type = T_LINK;
+    request.data.oldpath_len = strlen(oldpath);
+    request.path.old_path = qemu_strdup(oldpath);
+    retval = v9fs_create_special(fs_ctx, &request, &error);
+    if (retval > 0) {
+        errno = error;
+        return 0;
+    }
+    qemu_free((void *)request.path.old_path);
+    qemu_free((void *)request.path.path);
+    return retval;
+}
+
 static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
 {
     int err;
@@ -286,8 +358,7 @@ static int local_mknod(FsContext *fs_ctx, const char *path, FsCred *credp)
             serrno = errno;
             goto err_end;
         }
-    } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
-               (fs_ctx->fs_sm == SM_NONE)) {
+    } else if (fs_ctx->fs_sm == SM_NONE) {
         err = mknod(rpath(fs_ctx, path), credp->fc_mode, credp->fc_rdev);
         if (err == -1) {
             return err;
@@ -297,6 +368,12 @@ static int local_mknod(FsContext *fs_ctx, const char *path, FsCred *credp)
             serrno = errno;
             goto err_end;
         }
+    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        err = passthrough_mknod(fs_ctx, path, credp);
+        if (err < 0) {
+            serrno = errno;
+            goto err_end;
+        }
     }
     return err;
 
@@ -323,8 +400,7 @@ static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp)
             serrno = errno;
             goto err_end;
         }
-    } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
-               (fs_ctx->fs_sm == SM_NONE)) {
+    } else if (fs_ctx->fs_sm == SM_NONE) {
         err = mkdir(rpath(fs_ctx, path), credp->fc_mode);
         if (err == -1) {
             return err;
@@ -334,6 +410,12 @@ static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp)
             serrno = errno;
             goto err_end;
         }
+    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        err = passthrough_mkdir(fs_ctx, path, credp);
+        if (err < 0) {
+            serrno = errno;
+            goto err_end;
+        }
     }
     return err;
 
@@ -451,23 +533,18 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath,
             serrno = errno;
             goto err_end;
         }
-    } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
-               (fs_ctx->fs_sm == SM_NONE)) {
+    } else if (fs_ctx->fs_sm == SM_NONE) {
         err = symlink(oldpath, rpath(fs_ctx, newpath));
         if (err) {
             return err;
         }
         err = lchown(rpath(fs_ctx, newpath), credp->fc_uid, credp->fc_gid);
-        if (err == -1) {
-            /*
-             * If we fail to change ownership and if we are
-             * using security model none. Ignore the error
-             */
-            if (fs_ctx->fs_sm != SM_NONE) {
-                serrno = errno;
-                goto err_end;
-            } else
-                err = 0;
+        err = 0;
+    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        err = passthrough_symlink(fs_ctx, oldpath, newpath, credp);
+        if (err < 0) {
+            serrno = errno;
+            goto err_end;
         }
     }
     return err;
@@ -478,24 +555,26 @@ err_end:
     return err;
 }
 
-static int local_link(FsContext *ctx, const char *oldpath, const char *newpath)
+static int local_link(FsContext *fs_ctx, const char *oldpath,
+                const char *newpath)
 {
-    char *tmp = qemu_strdup(rpath(ctx, oldpath));
     int err, serrno = 0;
 
-    if (tmp == NULL) {
-        return -ENOMEM;
-    }
-
-    err = link(tmp, rpath(ctx, newpath));
-    if (err == -1) {
-        serrno = errno;
-    }
-
-    qemu_free(tmp);
-
-    if (err == -1) {
-        errno = serrno;
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        err = passthrough_link(fs_ctx, oldpath, newpath);
+        if (err < 0) {
+            serrno = errno;
+        }
+    } else {
+        char *tmp = qemu_strdup(rpath(fs_ctx, oldpath));
+        if (tmp == NULL) {
+            return -ENOMEM;
+        }
+        err = link(tmp, rpath(fs_ctx, newpath));
+        if (err == -1) {
+            serrno = errno;
+        }
+        qemu_free(tmp);
     }
 
     return err;
-- 
1.7.3.4

[Qemu-devel] [V4 PATCH 7/8] Move file post creation changes to none security model

[M. Mohan Kumar]

After creating a file object, its permission and ownership details are updated
as per client's request for both passthrough and none security model. But with
chrooted environment its not required for passthrough security model. Move all
post file creation changes to none security model

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 hw/9pfs/virtio-9p-local.c |   19 ++++++-------------
 1 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index c9ece6b..37a2510 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -208,21 +208,14 @@ static int local_set_xattr(const char *path, FsCred *credp)
     return 0;
 }
 
-static int local_post_create_passthrough(FsContext *fs_ctx, const char *path,
+static int local_post_create_none(FsContext *fs_ctx, const char *path,
         FsCred *credp)
 {
+    int retval;
     if (chmod(rpath(fs_ctx, path), credp->fc_mode & 07777) < 0) {
         return -1;
     }
-    if (lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid) < 0) {
-        /*
-         * If we fail to change ownership and if we are
-         * using security model none. Ignore the error
-         */
-        if (fs_ctx->fs_sm != SM_NONE) {
-            return -1;
-        }
-    }
+    retval = lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
     return 0;
 }
 
@@ -363,7 +356,7 @@ static int local_mknod(FsContext *fs_ctx, const char *path, FsCred *credp)
         if (err == -1) {
             return err;
         }
-        err = local_post_create_passthrough(fs_ctx, path, credp);
+        err = local_post_create_none(fs_ctx, path, credp);
         if (err == -1) {
             serrno = errno;
             goto err_end;
@@ -405,7 +398,7 @@ static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp)
         if (err == -1) {
             return err;
         }
-        err = local_post_create_passthrough(fs_ctx, path, credp);
+        err = local_post_create_none(fs_ctx, path, credp);
         if (err == -1) {
             serrno = errno;
             goto err_end;
@@ -480,7 +473,7 @@ static int local_open2(FsContext *fs_ctx, const char *path, int flags,
         if (fd == -1) {
             return fd;
         }
-        err = local_post_create_passthrough(fs_ctx, path, credp);
+        err = local_post_create_none(fs_ctx, path, credp);
         if (err == -1) {
             serrno = errno;
             goto err_end;
-- 
1.7.3.4

[Qemu-devel] [V4 PATCH 8/8] Chroot environment for other functions

[M. Mohan Kumar]

Add chroot functionality for systemcalls that can operate on a file
using relative directory file descriptor.

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 hw/9pfs/virtio-9p-local.c |  221 +++++++++++++++++++++++++++++++++++++++------
 1 files changed, 191 insertions(+), 30 deletions(-)

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 37a2510..b6471dd 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -20,6 +20,7 @@
 #include <sys/socket.h>
 #include <sys/un.h>
 #include <attr/xattr.h>
+#include <libgen.h>
 
 /* Helper routine to fill V9fsFileObjectRequest structure */
 static void fill_request(V9fsFileObjectRequest *request, const char *path,
@@ -141,14 +142,36 @@ static int passthrough_link(FsContext *fs_ctx, const char *oldpath,
     return retval;
 }
 
+/*
+ * Returns file descriptor of dirname(path)
+ * This fd can be used by *at functions
+ */
+static int get_dirfd(FsContext *fs_ctx, const char *path)
+{
+    V9fsFileObjectRequest request;
+    int fd, error = 0;
+    char *dpath = qemu_strdup(path);
+
+    fill_request(&request, dirname(dpath), NULL);
+    request.data.type = T_OPEN;
+    fd = v9fs_request(fs_ctx, &request, &error);
+    if (fd < 0) {
+        errno = error;
+    }
+    qemu_free(dpath);
+    qemu_free((void *)request.path.path);
+    return fd;
+}
+
 static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
 {
     int err;
-    err =  lstat(rpath(fs_ctx, path), stbuf);
-    if (err) {
-        return err;
-    }
+
     if (fs_ctx->fs_sm == SM_MAPPED) {
+        err =  lstat(rpath(fs_ctx, path), stbuf);
+        if (err) {
+            return err;
+        }
         /* Actual credentials are part of extended attrs */
         uid_t tmp_uid;
         gid_t tmp_gid;
@@ -170,6 +193,27 @@ static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
                         sizeof(dev_t)) > 0) {
                 stbuf->st_rdev = tmp_dev;
         }
+    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        int pfd, serrno = 0;
+        char *tmp_path;
+
+        pfd = get_dirfd(fs_ctx, path);
+        if (pfd < 0) {
+            return -1;
+        }
+        tmp_path = qemu_strdup(path);
+        err = fstatat(pfd, basename(tmp_path), stbuf, AT_SYMLINK_NOFOLLOW);
+        if (err < 0) {
+            serrno = errno;
+        }
+        close(pfd);
+        qemu_free(tmp_path);
+        errno = serrno;
+    } else {
+        err =  lstat(rpath(fs_ctx, path), stbuf);
+        if (err) {
+            return err;
+        }
     }
     return err;
 }
@@ -234,9 +278,23 @@ static ssize_t local_readlink(FsContext *fs_ctx, const char *path,
         } while (tsize == -1 && errno == EINTR);
         close(fd);
         return tsize;
-    } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
-               (fs_ctx->fs_sm == SM_NONE)) {
+    } else if (fs_ctx->fs_sm == SM_NONE) {
         tsize = readlink(rpath(fs_ctx, path), buf, bufsz);
+    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        int pfd, serrno = 0;
+        char *tmp_path;
+        pfd = get_dirfd(fs_ctx, path);
+        if (pfd < 0) {
+            return -1;
+        }
+        tmp_path = qemu_strdup(path);
+        tsize = readlinkat(pfd, basename(tmp_path), buf, bufsz);
+        if (tsize < 0) {
+            serrno = 0;
+        }
+        close(pfd);
+        qemu_free(tmp_path);
+        errno = serrno;
     }
     return tsize;
 }
@@ -328,8 +386,23 @@ static int local_chmod(FsContext *fs_ctx, const char *path, FsCred *credp)
 {
     if (fs_ctx->fs_sm == SM_MAPPED) {
         return local_set_xattr(rpath(fs_ctx, path), credp);
-    } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
-               (fs_ctx->fs_sm == SM_NONE)) {
+    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        int pfd, err, serrno = 0;
+        char *tmp_path;
+        pfd = get_dirfd(fs_ctx, path);
+        if (pfd < 0) {
+            return -1;
+        }
+        tmp_path = qemu_strdup(path);
+        err = fchmodat(pfd, basename(tmp_path), credp->fc_mode, 0);
+        if (err == -1) {
+            serrno = errno;
+        }
+        qemu_free(tmp_path);
+        close(pfd);
+        errno = serrno;
+        return err;
+    } else if (fs_ctx->fs_sm == SM_NONE) {
         return chmod(rpath(fs_ctx, path), credp->fc_mode);
     }
     return -1;
@@ -575,53 +648,141 @@ static int local_link(FsContext *fs_ctx, const char *oldpath,
 
 static int local_truncate(FsContext *ctx, const char *path, off_t size)
 {
-    return truncate(rpath(ctx, path), size);
+    if (ctx->fs_sm == SM_PASSTHROUGH) {
+        int fd, retval;
+        fd = passthrough_open(ctx, path, O_RDWR);
+        if (fd < 0) {
+            return fd;
+        }
+        retval = ftruncate(fd, size);
+        close(fd);
+        return retval;
+    } else {
+        return truncate(rpath(ctx, path), size);
+    }
 }
 
 static int local_rename(FsContext *ctx, const char *oldpath,
                         const char *newpath)
 {
-    char *tmp;
-    int err;
-
-    tmp = qemu_strdup(rpath(ctx, oldpath));
+    int err, serrno = 0;
 
-    err = rename(tmp, rpath(ctx, newpath));
-    if (err == -1) {
-        int serrno = errno;
-        qemu_free(tmp);
+    if (ctx->fs_sm == SM_PASSTHROUGH) {
+        int opfd, npfd;
+        char *old_tmppath, *new_tmppath;
+        opfd = get_dirfd(ctx, oldpath);
+        if (opfd < 0) {
+            return -1;
+        }
+        npfd = get_dirfd(ctx, newpath);
+        if (npfd < 0) {
+            close(opfd);
+            return -1;
+        }
+        old_tmppath = qemu_strdup(oldpath);
+        new_tmppath = qemu_strdup(newpath);
+        err = renameat(opfd, basename(old_tmppath),
+                        npfd, basename(new_tmppath));
+        if (err == -1) {
+            serrno = errno;
+        }
+        close(npfd);
+        close(opfd);
+        qemu_free(old_tmppath);
+        qemu_free(new_tmppath);
         errno = serrno;
     } else {
-        qemu_free(tmp);
+        char *tmp;
+        tmp = qemu_strdup(rpath(ctx, oldpath));
+
+        err = rename(tmp, rpath(ctx, newpath));
+        if (err == -1) {
+            int serrno = errno;
+            qemu_free(tmp);
+            errno = serrno;
+        } else {
+            qemu_free(tmp);
+        }
     }
 
     return err;
-
 }
 
 static int local_chown(FsContext *fs_ctx, const char *path, FsCred *credp)
 {
-    if ((credp->fc_uid == -1 && credp->fc_gid == -1) ||
-            (fs_ctx->fs_sm == SM_PASSTHROUGH)) {
+    if (fs_ctx->fs_sm == SM_NONE ||
+                    (credp->fc_uid == -1 && credp->fc_gid == -1)) {
         return lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
     } else if (fs_ctx->fs_sm == SM_MAPPED) {
         return local_set_xattr(rpath(fs_ctx, path), credp);
-    } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
-               (fs_ctx->fs_sm == SM_NONE)) {
-        return lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
+    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        int pfd, err, serrno = 0;
+        char *old_path;
+        pfd = get_dirfd(fs_ctx, path);
+        if (pfd < 0) {
+            return -1;
+        }
+        old_path = qemu_strdup(path);
+        err = fchownat(pfd, basename(old_path), credp->fc_uid, credp->fc_gid,
+                        AT_SYMLINK_NOFOLLOW);
+        if (err == -1) {
+            serrno = errno;
+        }
+        qemu_free(old_path);
+        close(pfd);
+        errno = serrno;
+        return err;
     }
     return -1;
 }
 
-static int local_utimensat(FsContext *s, const char *path,
-                           const struct timespec *buf)
+static int local_utimensat(FsContext *fs_ctx, const char *path,
+                const struct timespec *buf)
 {
-    return qemu_utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        int fd, retval;
+        fd = passthrough_open(fs_ctx, path, O_RDONLY);
+        retval = futimens(fd, buf);
+        close(fd);
+        return retval;
+    } else {
+        return utimensat(AT_FDCWD, rpath(fs_ctx, path), buf,
+                        AT_SYMLINK_NOFOLLOW);
+    }
 }
 
-static int local_remove(FsContext *ctx, const char *path)
-{
-    return remove(rpath(ctx, path));
+static int local_remove(FsContext *fs_ctx, const char *path)
+ {
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        int pfd, err, serrno, flags;
+        char *old_path;
+        struct stat stbuf;
+        pfd = get_dirfd(fs_ctx, path);
+        if (pfd < 0) {
+            return -1;
+        }
+        old_path = qemu_strdup(path);
+        err = fstatat(pfd, basename(old_path), &stbuf, AT_SYMLINK_NOFOLLOW);
+        if (err < 0) {
+            return -1;
+        }
+        serrno = flags = 0;
+        if ((stbuf.st_mode & S_IFMT) == S_IFDIR) {
+            flags = AT_REMOVEDIR;
+        } else {
+            flags = 0;
+        }
+        err = unlinkat(pfd, basename(old_path), flags);
+        if (err == -1) {
+            serrno = errno;
+        }
+        qemu_free(old_path);
+        close(pfd);
+        errno = serrno;
+        return err;
+    } else {
+        return remove(rpath(fs_ctx, path));
+    }
 }
 
 static int local_fsync(FsContext *ctx, int fd, int datasync)
-- 
1.7.3.4

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

[Daniel P. Berrange]

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 <mohan@in.ibm.com>
> ---
>  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

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

[Stefan Hajnoczi]

On Tue, Feb 1, 2011 at 10:32 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
> 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.

In this particular patch the fork() call happens quite early so the
risk should be low but it would be nice to investigate this issue
fully.

Stefan

[Qemu-devel] Re: [V4 PATCH 5/8] Create support in chroot environment

[Stefan Hajnoczi]

On Tue, Feb 1, 2011 at 5:27 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> +    if (setfsuid(request->data.uid) < 0) {
> +        fd_info->fi_error = errno;
> +        return;
> +    }
> +    if (setfsgid(request->data.gid) < 0) {
> +        fd_info->fi_error = errno;
> +        goto unset_uid;
> +    }

fsuid is Linux-specific.  Just something to keep in mind if you wanted
this code to be portable (I think the rest *is* portable).

Stefan

Re: [Qemu-devel] [V4 PATCH 6/8] Support for creating special files

[Stefan Hajnoczi]

On Tue, Feb 1, 2011 at 5:27 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> +static int passthrough_mknod(FsContext *fs_ctx, const char *path, FsCred *credp)
> +{
> +    V9fsFileObjectRequest request;
> +    int retval, error = 0;
> +
> +    fill_request(&request, path, credp);
> +    request.data.type = T_MKNOD;
> +
> +    retval = v9fs_create_special(fs_ctx, &request, &error);
> +    if (retval < 0) {
> +        errno = error;
> +    }
> +    return retval;
> +}

It would be nice to avoid duplicating all these wrappers.  Would it be
possible to write one generic function which takes the request type
and some optional arguments and runs the request?

Stefan

[Qemu-devel] Re: [V4 PATCH 3/8] Add client side interfaces for chroot environment

[Stefan Hajnoczi]

On Tue, Feb 1, 2011 at 5:26 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> +/* Receive file descriptor and error status from chroot process */
> +static int v9fs_receivefd(int sockfd, int *error)

The return value and int *error overlap in functionality.  Would it be
possible to have only one mechanism for returning errors?

*error = 0 is never done so a caller that passes an uninitialized
local variable gets back junk when the function succeeds.  It would be
safer to clear it at the start of this function.

Inconsistent use of errno constants and -1:
return -EIO;
return -1; /* == -EPERM, probably not what you wanted */

How about getting rid of int *error and returning the -errno?  If
if_error is set then return -fd_info.error.

> +/*
> + * V9fsFileObjectRequest is written into the socket by QEMU process.
> + * Then this request is read by chroot process using read_request function
> + */
> +static int v9fs_write_request(int sockfd, V9fsFileObjectRequest *request)
> +{
> +    int retval, length;
> +    char *buff, *buffp;
> +
> +    length = sizeof(request->data) + request->data.path_len +
> +                    request->data.oldpath_len;
> +
> +    buff = qemu_malloc(length);
> +    buffp = buff;
> +    memcpy(buffp, &request->data, sizeof(request->data));
> +    buffp += sizeof(request->data);
> +    memcpy(buffp, request->path.path, request->data.path_len);
> +    buffp += request->data.path_len;
> +    memcpy(buffp, request->path.old_path, request->data.oldpath_len);
> +
> +    retval = qemu_write_full(sockfd, buff, length);

qemu_free(buff);

Also, weren't you doing the malloc() + single write() to avoid
interleaved write()?  Is that still necessary, I thought a mutex was
introduced?  It's probably worth adding a comment to explain why
you're doing the malloc + write.

Stefan

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

[M. Mohan Kumar]

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.

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 <mohan@in.ibm.com>
> > ---
> > 
> >  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

[Qemu-devel] Re: [V4 PATCH 3/8] Add client side interfaces for chroot environment

[M. Mohan Kumar]

On Wednesday 02 February 2011 3:24:16 pm Stefan Hajnoczi wrote:
> On Tue, Feb 1, 2011 at 5:26 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> > +/* Receive file descriptor and error status from chroot process */
> > +static int v9fs_receivefd(int sockfd, int *error)
> 
> The return value and int *error overlap in functionality.  Would it be
> possible to have only one mechanism for returning errors?
> 

v9fs_receivefd function returns 'fd' on success and returns EIO (and error as 
EIO) if there was a socket error and -1 on other errors.

We need to return -EIO and error as EIO to differentiate between generic EIO 
error and socket failure.
 
> *error = 0 is never done so a caller that passes an uninitialized
> local variable gets back junk when the function succeeds.  It would be
> safer to clear it at the start of this function.
I will update the patch.
> 
> Inconsistent use of errno constants and -1:
> return -EIO;
> return -1; /* == -EPERM, probably not what you wanted */

-1 denotes failure

> 
> How about getting rid of int *error and returning the -errno?  If
> if_error is set then return -fd_info.error.

Do you mean return fd on success and -errno on failure? In this case how to 
differentiate between actual EIO and EIO because of socket read/write failure?

> 
> > +/*
> > + * V9fsFileObjectRequest is written into the socket by QEMU process.
> > + * Then this request is read by chroot process using read_request
> > function + */
> > +static int v9fs_write_request(int sockfd, V9fsFileObjectRequest
> > *request) +{
> > +    int retval, length;
> > +    char *buff, *buffp;
> > +
> > +    length = sizeof(request->data) + request->data.path_len +
> > +                    request->data.oldpath_len;
> > +
> > +    buff = qemu_malloc(length);
> > +    buffp = buff;
> > +    memcpy(buffp, &request->data, sizeof(request->data));
> > +    buffp += sizeof(request->data);
> > +    memcpy(buffp, request->path.path, request->data.path_len);
> > +    buffp += request->data.path_len;
> > +    memcpy(buffp, request->path.old_path, request->data.oldpath_len);
> > +
> > +    retval = qemu_write_full(sockfd, buff, length);
> 
> qemu_free(buff);
> 
> Also, weren't you doing the malloc() + single write() to avoid
> interleaved write()?  Is that still necessary, I thought a mutex was
> introduced?  It's probably worth adding a comment to explain why
> you're doing the malloc + write.

This is used to avoid multiple write system calls. Probably I can update with 
comments. 

----
M. Mohan Kumar

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 <mohan@in.ibm.com>
> > > ---
> > > 
> > >  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 :|