[libvirt] PATCH: 7/7: Allow replacing root filesystem

[Jim Meyering]

"Daniel P. Berrange" <berrange at redhat.com> wrote:
...
> -static int lxcContainerSetStdio(int control, const char *ttyPath)
> +static int lxcContainerSetStdio(int control, int ttyfd)
>  {

Hi Dan,

Not a big deal, but since ttyfd is now an input to this function,
it'd be less surprising if the caller were to close it.
Probably not worth changing...

>      int rc = -1;
> -    int ttyfd;
>      int open_max, i;
>
>      if (setsid() < 0) {
>          lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
>                   _("setsid failed: %s"), strerror(errno));
> -        goto error_out;
> -    }
> -
> -    ttyfd = open(ttyPath, O_RDWR|O_NOCTTY);
> -    if (ttyfd < 0) {
> -        lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> -                 _("open(%s) failed: %s"), ttyPath, strerror(errno));
> -        goto error_out;
> +        goto cleanup;
>      }
>
>      if (ioctl(ttyfd, TIOCSCTTY, NULL) < 0) {
> @@ -159,8 +161,6 @@
>
>  cleanup:
>      close(ttyfd);
> -
> -error_out:
>      return rc;
>  }
>
> @@ -223,6 +223,7 @@
>      return 0;
>  }
>
> +
>  /**
>   * lxcEnableInterfaces:
>   * @vm: Pointer to vm structure
> @@ -252,6 +253,20 @@
>      return rc;
>  }
...

> +    if (root) {
> +        char *oldroot;
> +        struct mntent *mntent;
> +        char **mounts = NULL;
> +        int nmounts = 0;
> +        FILE *procmnt;

This can be "const":

> +        struct {
> +            int maj;
> +            int min;
> +            const char *path;
> +        } devs[] = {
> +            { 1, 3, "/dev/null" },
> +            { 1, 5, "/dev/zero" },
> +            { 1, 7, "/dev/full" },
> +            { 5, 1, "/dev/console" },

Might be good to add /dev/random and /dev/urandom.
I recently had trouble on a system where udev malfunctioned,
and some /dev/{u,}random-requiring libs/services failed in unusual ways.

Also, how about adding permission bits to the table, so that
console doesn't end up being mode 0777:

      struct {
        int maj;
        int min;
        mode_t mode,
        const char *path;
      } const devs[] = {
        { 1, 3, 0666, "/dev/null" },
        { 1, 5, 0666, "/dev/zero" },
        { 1, 7, 0666, "/dev/full" },
        { 5, 1, 0600, "/dev/console" },
        { 1, 8, 0666, "/dev/random" },
        { 1, 9, 0666, "/dev/urandom" },
      };

> +        if (virFileMakePath("/dev/pts") < 0 ||
> +            mount("/.oldroot/dev/pts", "/dev/pts", NULL,
> +                  MS_MOVE, NULL) < 0) {
> +            lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                     _("failed to move /dev/pts into container: %s"),
> +                     strerror(errno));
> +            return -1;
> +        }
> +
> +        /* Populate /dev/ with a few important bits */
> +        umask(0);

In principle, it's better never to change umask.  Otherwise, when
multi-threaded, that temporarily-cleared umask could hose a file-creation
operation in another thread -- and it'd be a race, so probably hard to
reproduce.

> +        for (i = 0 ; i < ARRAY_CARDINALITY(devs) ; i++) {
> +            dev_t dev = makedev(devs[i].maj, devs[i].min);
> +            if (mknod(devs[i].path,
> +                      0777 | S_IFCHR,

Dropping the umask, you might do s/0777/0/ here, and then
call chmod to set each mode to devs[i].mode

> +                      dev) < 0) {
> +                lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                         _("failed to make device %s: %s"),
> +                         devs[i].path, strerror(errno));

Returning here would leave umask set to 0.
Another reason not to change it in the first place.

> +                return -1;
> +            }
> +        }
> +        umask(0700);

When you do change umask, be sure to restore to the previous value.

> +        /* Pull in rest of container's mounts */
> +        for (tmp = vmDef->fss; tmp; tmp = tmp->next) {
> +            char *src;
> +            if (STREQ(tmp->dst, "/"))
> +                continue;
> +            // XXX fix
> +            if (tmp->type != VIR_DOMAIN_FS_TYPE_MOUNT)
> +                continue;
> +
> +            if (asprintf(&src, "/.oldroot/%s", tmp->src) < 0)
> +                return -1;
> +
> +            if (virFileMakePath(tmp->dst) < 0 ||
> +                mount(src, tmp->dst, NULL, MS_BIND, NULL) < 0) {
> +                lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                         _("failed to mount %s at %s for container: %s"),
> +                         tmp->src, tmp->dst, strerror(errno));

Call VIR_FREE(src) here to avoid a leak.

> +                return -1;
> +            }
> +            VIR_FREE(src);
> +        }
> +
> +        if (!(procmnt = setmntent("/proc/mounts", "r"))) {
> +            lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                     _("failed to read /proc/mounts: %s"),
> +                     strerror(errno));
> +            return -1;
> +        }
> +        while ((mntent = getmntent(procmnt)) != NULL) {
> +            if (!STRPREFIX(mntent->mnt_dir, "/.oldroot"))
> +                continue;
> +            if (VIR_REALLOC_N(mounts, nmounts+1) < 0)

Call endmntent(procmnt) here, to
avoid a potential file descriptor leak.

> +                return -1;

> +            mounts[nmounts++] = strdup(mntent->mnt_dir);

A failed strdup here would lead to segfault via qsort's comparator.

> +        }
> +        endmntent(procmnt);
> +
> +        qsort(mounts, nmounts, sizeof(mounts[0]),
> +              lxcContainerChildMountSort);
> +
> +        for (i = 0 ; i < nmounts ; i++) {
> +            if (umount(mounts[i]) < 0) {
> +                lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                         _("failed to unmount %s: %s"),
> +                         mounts[i], strerror(errno));
> +                return -1;

Maybe try to unmount all of them before returning, even if an
earlier one fails?

Also, be sure to free "mounts" before returning.

> +            }
> +        }
> +    } else {
...