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

[Qemu-devel] [V7 PATCH 0/9] 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 V6:
* Send only fd/errno in socket operations instead of FdInfo structure
* Minor cleanups

Changes from version V5:
* Return errno on failure instead of setting errno
* Minor cleanups like updated comments, enable CONFIG_THREAD if
  CONFIG_VIRTFS is enabled

Changes from version V4:
* Avoid using malloc/free inside chroot process
* Seperate chroot server and client functions

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

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 (9):
  Implement qemu_read_full
  virtio-9p: Enable CONFIG_THREAD if CONFIG_VIRTFS is enabled
  virtio-9p: Provide chroot worker side interfaces
  virtio-9p: Add qemu side interfaces for chroot environment
  virtio-9p: Add support to open a file in chroot environment
  virtio-9p: Create support in chroot environment
  virtio-9p: Support for creating special files
  virtio-9p: Move file post creation changes to none security model
  virtio-9p: Chroot environment for other functions

 Makefile.objs                     |    1 +
 configure                         |    1 +
 hw/9pfs/virtio-9p-chroot-worker.c |  271 +++++++++++++++++++++++
 hw/9pfs/virtio-9p-chroot.c        |  127 +++++++++++
 hw/9pfs/virtio-9p-chroot.h        |   45 ++++
 hw/9pfs/virtio-9p-local.c         |  433 ++++++++++++++++++++++++++++++-------
 hw/9pfs/virtio-9p.c               |   25 +++
 hw/file-op-9p.h                   |    4 +
 osdep.c                           |   32 +++
 qemu-common.h                     |    2 +
 10 files changed, 861 insertions(+), 80 deletions(-)
 create mode 100644 hw/9pfs/virtio-9p-chroot-worker.c
 create mode 100644 hw/9pfs/virtio-9p-chroot.c
 create mode 100644 hw/9pfs/virtio-9p-chroot.h

-- 
1.7.3.4

[Qemu-devel] [V7 PATCH 1/9] 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 40dad52..325b16a 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -207,6 +207,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] [V7 PATCH 2/9] virtio-9p: Enable CONFIG_THREAD if CONFIG_VIRTFS is enabled

[M. Mohan Kumar]

9p Chroot environment needs APIs defined in qemu-thread.c, so enable
CONFIG_THREAD if virtfs is enabled

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 configure |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 2560357..9eddd38 100755
--- a/configure
+++ b/configure
@@ -2767,6 +2767,7 @@ fi
 if test "$linux" = "yes" ; then
   if test "$attr" = "yes" ; then
     echo "CONFIG_VIRTFS=y" >> $config_host_mak
+    echo "CONFIG_THREAD=y" >> $config_host_mak
   fi
 fi
 if test "$blobs" = "yes" ; then
-- 
1.7.3.4

[Qemu-devel] [V7 PATCH 3/9] virtio-9p: Provide chroot worker side interfaces

[M. Mohan Kumar]

Implement chroot worker 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-worker.c |  183 +++++++++++++++++++++++++++++++++++++
 hw/9pfs/virtio-9p-chroot.h        |   43 +++++++++
 hw/9pfs/virtio-9p.c               |   25 +++++
 hw/file-op-9p.h                   |    4 +
 5 files changed, 256 insertions(+), 0 deletions(-)
 create mode 100644 hw/9pfs/virtio-9p-chroot-worker.c
 create mode 100644 hw/9pfs/virtio-9p-chroot.h

diff --git a/Makefile.objs b/Makefile.objs
index 6a9f765..44891c1 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -283,6 +283,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-worker.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-worker.c b/hw/9pfs/virtio-9p-chroot-worker.c
new file mode 100644
index 0000000..e7bc6c2
--- /dev/null
+++ b/hw/9pfs/virtio-9p-chroot-worker.c
@@ -0,0 +1,183 @@
+/*
+ * Virtio 9p chroot environment for contained access to the exported path
+ * Code path handles chroot worker side interfaces
+ * 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"
+
+/* Send file descriptor and error status to qemu process */
+static void chroot_sendfd(int sockfd, int fd, int fd_valid)
+{
+    struct msghdr msg = { };
+    struct iovec iov;
+    struct cmsghdr *cmsg;
+    int retval;
+    union MsgControl msg_control;
+
+    iov.iov_base = &fd;
+    iov.iov_len = sizeof(fd);
+
+    memset(&msg, 0, sizeof(msg));
+    msg.msg_iov = &iov;
+    msg.msg_iovlen = 1;
+    /* No ancillary data on error */
+    if (fd_valid) {
+        msg.msg_control = &msg_control;
+        msg.msg_controllen = sizeof(msg_control);
+
+        cmsg = &msg_control.cmsg;
+        cmsg->cmsg_len = CMSG_LEN(sizeof(fd));
+        cmsg->cmsg_level = SOL_SOCKET;
+        cmsg->cmsg_type = SCM_RIGHTS;
+        memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));
+    }
+    do {
+        retval = sendmsg(sockfd, &msg, 0);
+    } while (retval < 0 && errno == EINTR);
+    if (retval < 0) {
+        _exit(1);
+    }
+    if (fd_valid) {
+        close(fd);
+    }
+}
+
+/* Read V9fsFileObjectRequest sent by QEMU process */
+static int chroot_read_request(int sockfd, V9fsFileObjectRequest *request)
+{
+    int retval;
+    retval = qemu_read_full(sockfd, request, sizeof(*request));
+    if (retval != sizeof(*request)) {
+        if (errno == EBADF) {
+            _exit(1);
+        }
+        return -EIO;
+    }
+    return 0;
+}
+
+/*
+ * Helper routine to open a file
+ */
+static int chroot_do_open(V9fsFileObjectRequest *request)
+{
+    int fd;
+    fd = open(request->path.path, request->data.flags);
+    if (fd < 0) {
+        fd = -errno;
+    }
+    return fd;
+}
+
+static void chroot_daemonize(int chroot_sock)
+{
+    sigset_t sigset;
+    struct rlimit nr_fd;
+    int fd;
+
+    /* Block all signals for this process */
+    sigfillset(&sigset);
+    sigprocmask(SIG_SETMASK, &sigset, NULL);
+
+    /* 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 per request */
+    umask(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).
+ * Note: To avoid errors in forked process in multi threaded environment
+ * only async-signal safe functions used. For more information see
+ * man fork(3p), signal(7)
+ */
+int v9fs_chroot(FsContext *fs_ctx)
+{
+    int fd_pair[2], chroot_sock, error;
+    V9fsFileObjectRequest request;
+    pid_t pid;
+    uint32_t code;
+    int fd, valid_fd;
+
+    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 = errno;
+        error = qemu_write_full(chroot_sock, &code, sizeof(code));
+        _exit(1);
+    }
+
+    chroot_daemonize(chroot_sock);
+
+    /*
+     * 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) {
+        valid_fd = 0;
+        if (chroot_read_request(chroot_sock, &request) < 0) {
+            chroot_sendfd(chroot_sock, -1, valid_fd);
+            continue;
+        }
+        switch (request.data.type) {
+        case T_OPEN:
+            fd = chroot_do_open(&request);
+            if (fd >= 0) {
+                valid_fd = 1;
+            }
+            break;
+        default:
+            fd = -1;
+            break;
+        }
+        chroot_sendfd(chroot_sock, fd, valid_fd);
+    }
+}
diff --git a/hw/9pfs/virtio-9p-chroot.h b/hw/9pfs/virtio-9p-chroot.h
new file mode 100644
index 0000000..97f616c
--- /dev/null
+++ b/hw/9pfs/virtio-9p-chroot.h
@@ -0,0 +1,43 @@
+#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
+
+union MsgControl {
+    struct cmsghdr cmsg;
+    char control[CMSG_SPACE(sizeof(int))];
+};
+
+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
+{
+    char path[PATH_MAX];
+    char old_path[PATH_MAX];
+};
+
+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 08c0399..4dea542 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -14,10 +14,12 @@
 #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"
 
 int debug_9p_pdu;
 static void v9fs_reclaim_fd(V9fsState *s);
@@ -3925,5 +3927,28 @@ 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) {
+        uint32_t code;
+        qemu_mutex_init(&s->ctx.chroot_mutex);
+        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 (qemu_read_full(s->ctx.chroot_socket, &code,
+                                sizeof(code)) != sizeof(code)) {
+            error_report("chroot process creation failed");
+            exit(1);
+        }
+        if (code != 0) {
+            error_report("chroot system call failed: %s", strerror(code));
+            exit(1);
+        }
+    }
+
     return &s->vdev;
 }
diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h
index 126e60e..b52b432 100644
--- a/hw/file-op-9p.h
+++ b/hw/file-op-9p.h
@@ -19,6 +19,7 @@
 #include <sys/stat.h>
 #include <sys/uio.h>
 #include <sys/vfs.h>
+#include "qemu-thread.h"
 #define SM_LOCAL_MODE_BITS    0600
 #define SM_LOCAL_DIR_MODE_BITS    0700
 
@@ -55,6 +56,9 @@ typedef struct FsContext
     SecModel fs_sm;
     uid_t uid;
     struct xattr_operations **xops;
+    QemuMutex chroot_mutex;
+    int chroot_socket;
+    int chroot_ioerror;
 } FsContext;
 
 void cred_init(FsCred *);
-- 
1.7.3.4

[Qemu-devel] [V7 PATCH 4/9] virtio-9p: Add qemu side interfaces for chroot environment

[M. Mohan Kumar]

QEMU side interfaces to communicate with chroot worker process.

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 Makefile.objs              |    2 +-
 hw/9pfs/virtio-9p-chroot.c |   92 ++++++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/virtio-9p-chroot.h |    1 +
 3 files changed, 94 insertions(+), 1 deletions(-)
 create mode 100644 hw/9pfs/virtio-9p-chroot.c

diff --git a/Makefile.objs b/Makefile.objs
index 44891c1..6610de9 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -283,7 +283,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-worker.o
+9pfs-nested-$(CONFIG_VIRTFS) +=   virtio-9p-chroot-worker.o 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..4aa3b43
--- /dev/null
+++ b/hw/9pfs/virtio-9p-chroot.c
@@ -0,0 +1,92 @@
+/*
+ * Virtio 9p chroot environment for contained access to exported path
+ * Code handles qemu side interfaces to communicate with chroot worker process
+ * 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"
+
+/*
+ * Return received file descriptor on success and -errno on failure.
+ * sock_error is set to 1 whenever there is error in socket IO
+ */
+static int v9fs_receivefd(int sockfd, int *sock_error)
+{
+    struct msghdr msg = { };
+    struct iovec iov;
+    union MsgControl msg_control;
+    struct cmsghdr *cmsg;
+    int retval, fd;
+
+    iov.iov_base = &fd;
+    iov.iov_len = sizeof(fd);
+
+    *sock_error = 0;
+    memset(&msg, 0, sizeof(msg));
+    msg.msg_iov = &iov;
+    msg.msg_iovlen = 1;
+    msg.msg_control = &msg_control;
+    msg.msg_controllen = sizeof(msg_control);
+
+    do {
+        retval = recvmsg(sockfd, &msg, 0);
+    } while (retval < 0 && errno == EINTR);
+    if (retval <= 0) {
+        *sock_error = 1;
+        return -EIO;
+    }
+
+    if (fd < 0) {
+        return fd;
+    }
+    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;
+    }
+    return fd;
+}
+
+/*
+ * V9fsFileObjectRequest is written into the socket by QEMU process.
+ * Then this request is read by chroot process using v9fs_read_request function
+ */
+static int v9fs_write_request(int sockfd, V9fsFileObjectRequest *request)
+{
+    int retval;
+    retval = qemu_write_full(sockfd, request, sizeof(*request));
+    if (retval != sizeof(*request)) {
+        return -EIO;
+    }
+    return 0;
+}
+
+/*
+ * This patch adds v9fs_receivefd and v9fs_write_request functions,
+ * but there is no callers. To avoid compiler warning message,
+ * refer these two functions
+ */
+void chroot_dummy(void)
+{
+    (void)v9fs_receivefd;
+    (void)v9fs_write_request;
+}
diff --git a/hw/9pfs/virtio-9p-chroot.h b/hw/9pfs/virtio-9p-chroot.h
index 97f616c..0c020f8 100644
--- a/hw/9pfs/virtio-9p-chroot.h
+++ b/hw/9pfs/virtio-9p-chroot.h
@@ -39,5 +39,6 @@ typedef struct V9fsFileObjectRequest
 } V9fsFileObjectRequest;
 
 int v9fs_chroot(FsContext *fs_ctx);
+void chroot_dummy(void);
 
 #endif /* _QEMU_VIRTIO_9P_CHROOT_H */
-- 
1.7.3.4

[Qemu-devel] [V7 PATCH 5/9] virtio-9p: Add support to open a file in chroot environment

[M. Mohan Kumar]

This patch adds both chroot worker and qemu 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 |   28 ++++++++++++++-----
 hw/9pfs/virtio-9p-chroot.h |    2 +-
 hw/9pfs/virtio-9p-local.c  |   62 +++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 79 insertions(+), 13 deletions(-)

diff --git a/hw/9pfs/virtio-9p-chroot.c b/hw/9pfs/virtio-9p-chroot.c
index 4aa3b43..0c91db6 100644
--- a/hw/9pfs/virtio-9p-chroot.c
+++ b/hw/9pfs/virtio-9p-chroot.c
@@ -80,13 +80,25 @@ static int v9fs_write_request(int sockfd, V9fsFileObjectRequest *request)
     return 0;
 }
 
-/*
- * This patch adds v9fs_receivefd and v9fs_write_request functions,
- * but there is no callers. To avoid compiler warning message,
- * refer these two functions
- */
-void chroot_dummy(void)
+/* Return opened file descriptor on success or -errno on error */
+int v9fs_request(FsContext *fs_ctx, V9fsFileObjectRequest *request)
 {
-    (void)v9fs_receivefd;
-    (void)v9fs_write_request;
+    int fd, sock_error;
+    qemu_mutex_lock(&fs_ctx->chroot_mutex);
+    if (fs_ctx->chroot_ioerror) {
+        fd = -EIO;
+        goto unlock;
+    }
+    if (v9fs_write_request(fs_ctx->chroot_socket, request) < 0) {
+        fs_ctx->chroot_ioerror = 1;
+        fd = -EIO;
+        goto unlock;
+    }
+    fd = v9fs_receivefd(fs_ctx->chroot_socket, &sock_error);
+    if (fd < 0 && sock_error) {
+        fs_ctx->chroot_ioerror = 1;
+    }
+unlock:
+    qemu_mutex_unlock(&fs_ctx->chroot_mutex);
+    return fd;
 }
diff --git a/hw/9pfs/virtio-9p-chroot.h b/hw/9pfs/virtio-9p-chroot.h
index 0c020f8..6f676e9 100644
--- a/hw/9pfs/virtio-9p-chroot.h
+++ b/hw/9pfs/virtio-9p-chroot.h
@@ -39,6 +39,6 @@ typedef struct V9fsFileObjectRequest
 } V9fsFileObjectRequest;
 
 int v9fs_chroot(FsContext *fs_ctx);
-void chroot_dummy(void);
+int v9fs_request(FsContext *fs_ctx, V9fsFileObjectRequest *or);
 
 #endif /* _QEMU_VIRTIO_9P_CHROOT_H */
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 0a015de..8c187d7 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -13,6 +13,9 @@
 #include "virtio.h"
 #include "virtio-9p.h"
 #include "virtio-9p-xattr.h"
+#include "qemu_socket.h"
+#include "fsdev/qemu-fsdev.h"
+#include "virtio-9p-chroot.h"
 #include <arpa/inet.h>
 #include <pwd.h>
 #include <grp.h>
@@ -20,6 +23,44 @@
 #include <sys/un.h>
 #include <attr/xattr.h>
 
+/* Helper routine to fill V9fsFileObjectRequest structure */
+static int fill_fileobjectrequest(V9fsFileObjectRequest *request,
+                const char *path, FsCred *credp)
+{
+    if (strlen(path) >= PATH_MAX) {
+        return -ENAMETOOLONG;
+    }
+    memset(request, 0, sizeof(*request));
+    request->data.path_len = strlen(path);
+    strcpy(request->path.path, 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;
+    }
+    return 0;
+}
+
+static int passthrough_open(FsContext *fs_ctx, const char *path, int flags)
+{
+    V9fsFileObjectRequest request;
+    int fd;
+
+    fd = fill_fileobjectrequest(&request, path, NULL);
+    if (fd < 0) {
+        errno = -fd;
+        return -1;
+    }
+    request.data.flags = flags;
+    request.data.type = T_OPEN;
+    fd = v9fs_request(fs_ctx, &request);
+    if (fd < 0) {
+        errno = -fd;
+        fd = -1;
+    }
+    return fd;
+}
 
 static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
 {
@@ -138,14 +179,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] [V7 PATCH 6/9] virtio-9p: Create support in chroot environment

[M. Mohan Kumar]

Add both chroot worker & qemu side interfaces to create regular files in
chroot environment

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

diff --git a/hw/9pfs/virtio-9p-chroot-worker.c b/hw/9pfs/virtio-9p-chroot-worker.c
index e7bc6c2..cdcb303 100644
--- a/hw/9pfs/virtio-9p-chroot-worker.c
+++ b/hw/9pfs/virtio-9p-chroot-worker.c
@@ -85,6 +85,36 @@ static int chroot_do_open(V9fsFileObjectRequest *request)
     return fd;
 }
 
+/*
+ * Helper routine to create a file and return the file descriptor
+ */
+static int chroot_do_create(V9fsFileObjectRequest *request)
+{
+    uid_t cur_uid;
+    gid_t cur_gid;
+    int fd = -1;
+
+    cur_uid = geteuid();
+    cur_gid = getegid();
+
+    if (setfsuid(request->data.uid) < 0) {
+        return -errno;
+    }
+    if (setfsgid(request->data.gid) < 0) {
+        fd = -errno;
+        goto unset_uid;
+    }
+
+    fd = open(request->path.path, request->data.flags, request->data.mode);
+    if (fd < 0) {
+        fd = -errno;
+    }
+    setfsgid(cur_gid);
+unset_uid:
+    setfsuid(cur_uid);
+    return fd;
+}
+
 static void chroot_daemonize(int chroot_sock)
 {
     sigset_t sigset;
@@ -174,6 +204,12 @@ int v9fs_chroot(FsContext *fs_ctx)
                 valid_fd = 1;
             }
             break;
+        case T_CREATE:
+            fd = chroot_do_create(&request);
+            if (fd >= 0) {
+                valid_fd = 1;
+            }
+            break;
         default:
             fd = -1;
             break;
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 8c187d7..aa8ebff 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -62,6 +62,27 @@ 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;
+
+    fd = fill_fileobjectrequest(&request, path, credp);
+    if (fd < 0) {
+        errno = -1;
+        return -1;
+    }
+    request.data.flags = flags;
+    request.data.type = T_CREATE;
+    fd = v9fs_request(fs_ctx, &request);
+    if (fd < 0) {
+        errno = -fd;
+        fd = -1;
+    }
+    return fd;
+}
+
 static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
 {
     int err;
@@ -386,8 +407,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;
@@ -397,6 +417,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] [V7 PATCH 7/9] virtio-9p: Support for creating special files

[M. Mohan Kumar]

Add both chroot worker and qemu 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-worker.c |   52 ++++++++++++++++++++
 hw/9pfs/virtio-9p-chroot.c        |   23 +++++++++
 hw/9pfs/virtio-9p-chroot.h        |    1 +
 hw/9pfs/virtio-9p-local.c         |   97 +++++++++++++++++++++++++------------
 4 files changed, 142 insertions(+), 31 deletions(-)

diff --git a/hw/9pfs/virtio-9p-chroot-worker.c b/hw/9pfs/virtio-9p-chroot-worker.c
index cdcb303..fcc0be1 100644
--- a/hw/9pfs/virtio-9p-chroot-worker.c
+++ b/hw/9pfs/virtio-9p-chroot-worker.c
@@ -115,6 +115,52 @@ unset_uid:
     return fd;
 }
 
+/*
+ * 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 as file descriptor on success and -errno on failure
+ */
+static int chroot_do_create_special(V9fsFileObjectRequest *request)
+{
+    int cur_uid, cur_gid;
+    int retval = -1;
+
+    cur_uid = geteuid();
+    cur_gid = getegid();
+
+    if (setfsuid(request->data.uid) < 0) {
+        return -errno;
+    }
+    if (setfsgid(request->data.gid) < 0) {
+        retval = -errno;
+        goto unset_uid;
+    }
+
+    switch (request->data.type) {
+    case T_MKDIR:
+        retval = mkdir(request->path.path, request->data.mode);
+        break;
+    case T_SYMLINK:
+        retval = symlink(request->path.old_path, request->path.path);
+        break;
+    case T_LINK:
+        retval = link(request->path.old_path, request->path.path);
+        break;
+    default:
+        retval = mknod(request->path.path, request->data.mode,
+                        request->data.dev);
+        break;
+    }
+
+    if (retval < 0) {
+        retval = -errno;
+    }
+    setfsgid(cur_gid);
+unset_uid:
+    setfsuid(cur_uid);
+    return retval;
+}
+
 static void chroot_daemonize(int chroot_sock)
 {
     sigset_t sigset;
@@ -210,6 +256,12 @@ int v9fs_chroot(FsContext *fs_ctx)
                 valid_fd = 1;
             }
             break;
+        case T_MKDIR:
+        case T_SYMLINK:
+        case T_LINK:
+        case T_MKNOD:
+            fd = chroot_do_create_special(&request);
+            break;
         default:
             fd = -1;
             break;
diff --git a/hw/9pfs/virtio-9p-chroot.c b/hw/9pfs/virtio-9p-chroot.c
index 0c91db6..ff0a53e 100644
--- a/hw/9pfs/virtio-9p-chroot.c
+++ b/hw/9pfs/virtio-9p-chroot.c
@@ -102,3 +102,26 @@ unlock:
     qemu_mutex_unlock(&fs_ctx->chroot_mutex);
     return fd;
 }
+
+/* Return 0 on success or -errno on error */
+int v9fs_create_special(FsContext *fs_ctx, V9fsFileObjectRequest *request)
+{
+    int retval, sock_error;
+    qemu_mutex_lock(&fs_ctx->chroot_mutex);
+    if (fs_ctx->chroot_ioerror) {
+        retval = -EIO;
+        goto unlock;
+    }
+    if (v9fs_write_request(fs_ctx->chroot_socket, request) < 0) {
+        fs_ctx->chroot_ioerror = 1;
+        retval = -EIO;
+        goto unlock;
+    }
+    retval = v9fs_receivefd(fs_ctx->chroot_socket, &sock_error);
+    if (retval < 0 && sock_error) {
+        fs_ctx->chroot_ioerror = 1;
+    }
+unlock:
+    qemu_mutex_unlock(&fs_ctx->chroot_mutex);
+    return retval;
+}
diff --git a/hw/9pfs/virtio-9p-chroot.h b/hw/9pfs/virtio-9p-chroot.h
index 6f676e9..a7f60e2 100644
--- a/hw/9pfs/virtio-9p-chroot.h
+++ b/hw/9pfs/virtio-9p-chroot.h
@@ -40,5 +40,6 @@ typedef struct V9fsFileObjectRequest
 
 int v9fs_chroot(FsContext *fs_ctx);
 int v9fs_request(FsContext *fs_ctx, V9fsFileObjectRequest *or);
+int v9fs_create_special(FsContext *fs_ctx, V9fsFileObjectRequest *request);
 
 #endif /* _QEMU_VIRTIO_9P_CHROOT_H */
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index aa8ebff..904ee71 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -83,6 +83,32 @@ static int passthrough_create(FsContext *fs_ctx, const char *path, int flags,
     return fd;
 }
 
+static int passthrough_create_special(FsContext *fs_ctx, const char *oldpath,
+                const char *path, FsCred *credp, int type)
+{
+    V9fsFileObjectRequest request;
+    int retval;
+
+    retval = fill_fileobjectrequest(&request, path, credp);
+    if (retval < 0) {
+        return retval;
+    }
+    request.data.type = type;
+    if (oldpath) {
+        request.data.oldpath_len = strlen(oldpath);
+        if (strlen(oldpath) >= PATH_MAX) {
+            return -ENAMETOOLONG;
+        }
+        strcpy(request.path.old_path, oldpath);
+    }
+    retval = v9fs_create_special(fs_ctx, &request);
+    if (retval < 0) {
+        errno = -retval;
+        retval = -1;
+    }
+    return retval;
+}
+
 static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
 {
     int err;
@@ -300,8 +326,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;
@@ -311,6 +336,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_create_special(fs_ctx, NULL, path, credp, T_MKNOD);
+        if (err < 0) {
+            serrno = errno;
+            goto err_end;
+        }
     }
     return err;
 
@@ -337,8 +368,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;
@@ -348,6 +378,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_create_special(fs_ctx, NULL, path, credp, T_MKDIR);
+        if (err < 0) {
+            serrno = errno;
+            goto err_end;
+        }
     }
     return err;
 
@@ -465,23 +501,19 @@ 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_create_special(fs_ctx, oldpath, newpath, credp,
+                        T_SYMLINK);
+        if (err < 0) {
+            serrno = errno;
+            goto err_end;
         }
     }
     return err;
@@ -492,24 +524,27 @@ 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_create_special(fs_ctx, oldpath, newpath, NULL,
+                        T_LINK);
+        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] [V7 PATCH 8/9] virtio-9p: 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 9p 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 904ee71..864334d 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -176,21 +176,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;
 }
 
@@ -331,7 +324,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;
@@ -373,7 +366,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;
@@ -448,7 +441,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] [V7 PATCH 9/9] virtio-9p: 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 |  229 +++++++++++++++++++++++++++++++++++++++------
 1 files changed, 199 insertions(+), 30 deletions(-)

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 864334d..cb94ab7 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -22,6 +22,7 @@
 #include <sys/socket.h>
 #include <sys/un.h>
 #include <attr/xattr.h>
+#include <libgen.h>
 
 /* Helper routine to fill V9fsFileObjectRequest structure */
 static int fill_fileobjectrequest(V9fsFileObjectRequest *request,
@@ -109,14 +110,39 @@ static int passthrough_create_special(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;
+    char *dpath = qemu_strdup(path);
+
+    fd = fill_fileobjectrequest(&request, dirname(dpath), NULL);
+    if (fd < 0) {
+        return fd;
+    }
+    request.data.type = T_OPEN;
+    fd = v9fs_request(fs_ctx, &request);
+    qemu_free(dpath);
+    if (fd < 0) {
+        errno = -fd;
+        fd = -1;
+    }
+    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;
@@ -138,6 +164,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;
 }
@@ -202,9 +249,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;
 }
@@ -296,8 +357,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;
@@ -545,53 +621,146 @@ 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 -1;
+        }
+        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_PASSTHROUGH &&
+            (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_NONE) {
         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 | O_NONBLOCK);
+        if (fd < 0) {
+            return -1;
+        }
+        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

[Qemu-devel] Re: [V7 PATCH 6/9] virtio-9p: Create support in chroot environment

[Stefan Hajnoczi]

On Fri, Mar 4, 2011 at 9:25 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> +static int passthrough_create(FsContext *fs_ctx, const char *path, int flags,
> +                    FsCred *credp)
> +{
> +    V9fsFileObjectRequest request;
> +    int fd;
> +
> +    fd = fill_fileobjectrequest(&request, path, credp);
> +    if (fd < 0) {
> +        errno = -1;
> +        return -1;
> +    }

I think it should be errno = fd since fill_fileobjectrequest() returns
an -errno.

Stefan

[Qemu-devel] Re: [V7 PATCH 3/9] virtio-9p: Provide chroot worker side interfaces

[Stefan Hajnoczi]

On Fri, Mar 4, 2011 at 9:25 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> +static int chroot_do_open(V9fsFileObjectRequest *request)
> +{
> +    int fd;
> +    fd = open(request->path.path, request->data.flags);
> +    if (fd < 0) {
> +        fd = -errno;
> +    }
> +    return fd;
> +}

How do access checks work without fsuid/fsgid being set?

Stefan

[Qemu-devel] Re: [V7 PATCH 7/9] virtio-9p: Support for creating special files

[Stefan Hajnoczi]

On Fri, Mar 4, 2011 at 9:25 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> +static int chroot_do_create_special(V9fsFileObjectRequest *request)
> +{
> +    int cur_uid, cur_gid;
> +    int retval = -1;
> +
> +    cur_uid = geteuid();
> +    cur_gid = getegid();
> +
> +    if (setfsuid(request->data.uid) < 0) {
> +        return -errno;
> +    }
> +    if (setfsgid(request->data.gid) < 0) {
> +        retval = -errno;
> +        goto unset_uid;
> +    }
> +
> +    switch (request->data.type) {
> +    case T_MKDIR:
> +        retval = mkdir(request->path.path, request->data.mode);
> +        break;
> +    case T_SYMLINK:
> +        retval = symlink(request->path.old_path, request->path.path);
> +        break;
> +    case T_LINK:
> +        retval = link(request->path.old_path, request->path.path);
> +        break;
> +    default:
> +        retval = mknod(request->path.path, request->data.mode,
> +                        request->data.dev);
> +        break;
> +    }
> +
> +    if (retval < 0) {
> +        retval = -errno;
> +    }
> +    setfsgid(cur_gid);
> +unset_uid:
> +    setfsuid(cur_uid);
> +    return retval;
> +}

It would be nice to take this one step further and move file create
and open here too.  The prototype we need is:

static int chroot_handle_request(V9fsFileObjectRequest *request, int *fd)
{
    *fd = -1;

It returns 0 on success or -errno and *fd >= 0 if a file descriptor
was opened and -1 otherwise.

This function becomes the main request processing function called from
v9fs_chroot() and the switch statement there can be eliminated.

Sending the response back to QEMU then gets a cleaned up prototype:
chroot_sendfd(int chroot_sock, int result, int fd) where result is 0
on success or -errno and fd >= 0 if present or -1 if not.

> +int v9fs_create_special(FsContext *fs_ctx, V9fsFileObjectRequest *request)
> +{
> +    int retval, sock_error;
> +    qemu_mutex_lock(&fs_ctx->chroot_mutex);
> +    if (fs_ctx->chroot_ioerror) {
> +        retval = -EIO;
> +        goto unlock;
> +    }
> +    if (v9fs_write_request(fs_ctx->chroot_socket, request) < 0) {
> +        fs_ctx->chroot_ioerror = 1;
> +        retval = -EIO;
> +        goto unlock;
> +    }
> +    retval = v9fs_receivefd(fs_ctx->chroot_socket, &sock_error);
> +    if (retval < 0 && sock_error) {
> +        fs_ctx->chroot_ioerror = 1;
> +    }
> +unlock:
> +    qemu_mutex_unlock(&fs_ctx->chroot_mutex);
> +    return retval;
> +}

This function is a duplicate of v9fs_request().  Can't there be just
one function?

Stefan

[Qemu-devel] Re: [V7 PATCH 9/9] virtio-9p: Chroot environment for other functions

[Stefan Hajnoczi]

On Fri, Mar 4, 2011 at 9:25 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> 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 |  229 +++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 199 insertions(+), 30 deletions(-)

Is this code supposed to build on non-Linux hosts?  If so, then please
confirm that the *at() system calls used are standard and available on
other hosts (e.g. FreeBSD, Darwin, Solaris).

> +/*
> + * 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;
> +    char *dpath = qemu_strdup(path);
> +
> +    fd = fill_fileobjectrequest(&request, dirname(dpath), NULL);
> +    if (fd < 0) {
> +        return fd;
> +    }

Leaks dpath, fails to set errno, and does not return -1.

> @@ -545,53 +621,146 @@ 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 -1;
> +        }
> +        retval = ftruncate(fd, size);
> +        close(fd);
> +        return retval;

This is an example of where errno is not guaranteed to be preserved.
When ftruncate(2) fails close(2) is allowed to affect errno and we
cannot rely on it holding the ftruncate(2) error code.  Please check
for other cases of this.

>  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;

Why can't this be done as a chroot worker operation in a single syscall?

> -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 | O_NONBLOCK);

This follows symlinks but the SM_PASSTHROUGH case below does not?

> +        if (fd < 0) {
> +            return -1;
> +        }
> +        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) {

old_path and pfd are leaked.

> +            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;

Why is SM_PASSTHROUGH so complicated...

> +    } else {
> +        return remove(rpath(fs_ctx, path));

...but this so simple?

Could we just send a remove operation to the chroot worker instead?

Stefan

Re: [Qemu-devel] Re: [V7 PATCH 3/9] virtio-9p: Provide chroot worker side interfaces

[Venkateswararao Jujjuri (JV)]

On 3/4/2011 2:53 AM, Stefan Hajnoczi wrote:
> On Fri, Mar 4, 2011 at 9:25 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
>> +static int chroot_do_open(V9fsFileObjectRequest *request)
>> +{
>> +    int fd;
>> +    fd = open(request->path.path, request->data.flags);
>> +    if (fd < 0) {
>> +        fd = -errno;
>> +    }
>> +    return fd;
>> +}
> 
> How do access checks work without fsuid/fsgid being set?

VirtFS access checks are done on the cleint. Server just does what
client asked for.. as long as the operations on the exported region.

- JV

> 
> Stefan
> 

[Qemu-devel] Re: [V7 PATCH 7/9] virtio-9p: Support for creating special files

[M. Mohan Kumar]

On Friday 04 March 2011 4:36:35 pm Stefan Hajnoczi wrote:
> On Fri, Mar 4, 2011 at 9:25 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> > +static int chroot_do_create_special(V9fsFileObjectRequest *request)
> > +{
> > +    int cur_uid, cur_gid;
> > +    int retval = -1;
> > +
> > +    cur_uid = geteuid();
> > +    cur_gid = getegid();
> > +
> > +    if (setfsuid(request->data.uid) < 0) {
> > +        return -errno;
> > +    }
> > +    if (setfsgid(request->data.gid) < 0) {
> > +        retval = -errno;
> > +        goto unset_uid;
> > +    }
> > +
> > +    switch (request->data.type) {
> > +    case T_MKDIR:
> > +        retval = mkdir(request->path.path, request->data.mode);
> > +        break;
> > +    case T_SYMLINK:
> > +        retval = symlink(request->path.old_path, request->path.path);
> > +        break;
> > +    case T_LINK:
> > +        retval = link(request->path.old_path, request->path.path);
> > +        break;
> > +    default:
> > +        retval = mknod(request->path.path, request->data.mode,
> > +                        request->data.dev);
> > +        break;
> > +    }
> > +
> > +    if (retval < 0) {
> > +        retval = -errno;
> > +    }
> > +    setfsgid(cur_gid);
> > +unset_uid:
> > +    setfsuid(cur_uid);
> > +    return retval;
> > +}
> 
> It would be nice to take this one step further and move file create
> and open here too.  The prototype we need is:
> 
> static int chroot_handle_request(V9fsFileObjectRequest *request, int *fd)
> {
>     *fd = -1;
> 
> It returns 0 on success or -errno and *fd >= 0 if a file descriptor
> was opened and -1 otherwise.
> 
> This function becomes the main request processing function called from
> v9fs_chroot() and the switch statement there can be eliminated.
>

We don't need setfsgid, setfsuid for normal open. Also I think having separate 
function based on the functionality helps better code readability. 
 
> Sending the response back to QEMU then gets a cleaned up prototype:
> chroot_sendfd(int chroot_sock, int result, int fd) where result is 0
> on success or -errno and fd >= 0 if present or -1 if not.
> 
> > +int v9fs_create_special(FsContext *fs_ctx, V9fsFileObjectRequest
> > *request) +{
> > +    int retval, sock_error;
> > +    qemu_mutex_lock(&fs_ctx->chroot_mutex);
> > +    if (fs_ctx->chroot_ioerror) {
> > +        retval = -EIO;
> > +        goto unlock;
> > +    }
> > +    if (v9fs_write_request(fs_ctx->chroot_socket, request) < 0) {
> > +        fs_ctx->chroot_ioerror = 1;
> > +        retval = -EIO;
> > +        goto unlock;
> > +    }
> > +    retval = v9fs_receivefd(fs_ctx->chroot_socket, &sock_error);
> > +    if (retval < 0 && sock_error) {
> > +        fs_ctx->chroot_ioerror = 1;
> > +    }
> > +unlock:
> > +    qemu_mutex_unlock(&fs_ctx->chroot_mutex);
> > +    return retval;
> > +}
> 
> This function is a duplicate of v9fs_request().  Can't there be just
> one function?
> 
Yeah, I will make it as a single function.

----
M. Mohan Kumar

[Qemu-devel] Re: [V7 PATCH 9/9] virtio-9p: Chroot environment for other functions

[M. Mohan Kumar]

Thanks Stefan for your review!

On Friday 04 March 2011 5:22:00 pm Stefan Hajnoczi wrote:
> 
> Is this code supposed to build on non-Linux hosts?  If so, then please
> confirm that the *at() system calls used are standard and available on
> other hosts (e.g. FreeBSD, Darwin, Solaris).
> 
No, this (virtio-9p) is not supported on non-linux hosts.

> > +/*
> > + * 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;
> > +    char *dpath = qemu_strdup(path);
> > +
> > +    fd = fill_fileobjectrequest(&request, dirname(dpath), NULL);
> > +    if (fd < 0) {
> > +        return fd;
> > +    }
> 
> Leaks dpath, fails to set errno, and does not return -1.
Will fix in next patchset.

> 
> > @@ -545,53 +621,146 @@ 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 -1;
> > +        }
> > +        retval = ftruncate(fd, size);
> > +        close(fd);
> > +        return retval;
> 
> This is an example of where errno is not guaranteed to be preserved.
> When ftruncate(2) fails close(2) is allowed to affect errno and we
> cannot rely on it holding the ftruncate(2) error code.  Please check
> for other cases of this.
Will fix in next patchset.
> 
> >  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;
> 
> Why can't this be done as a chroot worker operation in a single syscall?
Ok, in next patchset, T_RENAME & T_REMOVE types will be added.

> 
> > -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 | O_NONBLOCK);
> 
> This follows symlinks but the SM_PASSTHROUGH case below does not?
Will fix.
> 
> > +        if (fd < 0) {
> > +            return -1;
> > +        }
> > +        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) {
> 
> old_path and pfd are leaked.
> 
> > +            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;
> 
> Why is SM_PASSTHROUGH so complicated...
> 
> > +    } else {
> > +        return remove(rpath(fs_ctx, path));
> 
> ...but this so simple?
> 
> Could we just send a remove operation to the chroot worker instead?
> 

 

----
M. Mohan Kumar

[Qemu-devel] Re: [V7 PATCH 9/9] virtio-9p: Chroot environment for other functions

[Stefan Hajnoczi]

On Fri, Mar 4, 2011 at 9:25 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> @@ -296,8 +357,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);

AT_SYMLINK_NOFOLLOW is missing.

Stefan