Patchwork [V7,9/9] virtio-9p: Chroot environment for other functions

login
register
mail settings
Submitter Mohan Kumar M
Date March 4, 2011, 9:25 a.m.
Message ID <1299230756-1644-10-git-send-email-mohan@in.ibm.com>
Download mbox | patch
Permalink /patch/85389/
State New
Headers show

Comments

Mohan Kumar M - March 4, 2011, 9:25 a.m.
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(-)
Stefan Hajnoczi - March 4, 2011, 11:52 a.m.
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
Mohan Kumar M - March 9, 2011, 8:10 a.m.
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
Stefan Hajnoczi - March 11, 2011, 5:12 p.m.
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

Patch

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)