[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 <address@hidden> wrote:
> Add chroot functionality for systemcalls that can operate on a file
> using relative directory file descriptor.
>
> Signed-off-by: M. Mohan Kumar <address@hidden>
> ---
>  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