On Fri, 2015-09-04 at 12:40 +0100, Daniel P. Berrange wrote:

When running interactive sandboxes, don't drop privileges in the long running libvirt-sandbox-init-common process. This needs to be privileged in order to sync, unmount and shutdown the guest when the user command is finished. Push changing of user ID into the child process, between fork & exec.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- libvirt-sandbox/libvirt-sandbox-init-common.c | 60 +++++++++++++++------------ 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/libvirt-sandbox/libvirt-sandbox-init-common.c b/libvirt-sandbox/libvirt-sandbox-init-common.c index d35f760..760509f 100644 --- a/libvirt-sandbox/libvirt-sandbox-init-common.c +++ b/libvirt-sandbox/libvirt-sandbox-init-common.c @@ -390,30 +390,43 @@ static int change_user(const gchar *user, return 0; }

-static gboolean run_command(gboolean interactive, gchar **argv, +static gboolean run_command(GVirSandboxConfig *config, pid_t *child, int *appin, int *appout, int *apperr) { + GVirSandboxConfigInteractive *iconfig = GVIR_SANDBOX_CONFIG_INTERACTIVE(config); int pid; int master = -1; int slave = -1; int pipein[2] = { -1, -1}; int pipeout[2] = { -1, -1}; int pipeerr[2] = { -1, -1}; + gchar **appargv = gvir_sandbox_config_get_command(config); + gboolean wanttty = gvir_sandbox_config_interactive_get_tty(iconfig);

if (debug) fprintf(stderr, "libvirt-sandbox-init-common: running command %s %d\n", - argv[0], interactive); + appargv[0], wanttty);

*appin = *appout = -1;

- if (interactive) { + if (wanttty) { if (openpty(&master, &slave, NULL, NULL, NULL) < 0) { fprintf(stderr, "Cannot setup slave for child: %s\n", strerror(errno)); goto error; }

+ if (!g_str_equal(gvir_sandbox_config_get_username(config), "root")) { + if (fchown(slave, + gvir_sandbox_config_get_userid(config), + gvir_sandbox_config_get_groupid(config)) < 0) { + fprintf(stderr, "Cannot make PTY available to user: %s\n", + strerror(errno)); + goto error; + } + } + *appin = master; *appout = master; *apperr = master; @@ -436,7 +449,13 @@ static gboolean run_command(gboolean interactive, gchar **argv, }

if (pid == 0) { - if (interactive) { + if (change_user(gvir_sandbox_config_get_username(config), + gvir_sandbox_config_get_userid(config), + gvir_sandbox_config_get_groupid(config), + gvir_sandbox_config_get_homedir(config)) < 0) + exit(EXIT_FAILURE); + + if (wanttty) { close(master); close(STDIN_FILENO); close(STDOUT_FILENO); @@ -469,24 +488,25 @@ static gboolean run_command(gboolean interactive, gchar **argv, abort(); }

- execv(argv[0], argv); - fprintf(stderr, "Cannot execute '%s': %s\n", argv[0], strerror(errno)); + execv(appargv[0], appargv); + fprintf(stderr, "Cannot execute '%s': %s\n", appargv[0], strerror(errno)); exit(EXIT_FAILURE); }

- if (interactive) + if (wanttty) { close(slave); - else { + } else { close(pipein[0]); close(pipeout[1]); close(pipeerr[1]); }

*child = pid; + g_strfreev(appargv); return TRUE;

error: - if (interactive) { + if (wanttty) { if (master != -1) close(master); if (slave != -1) @@ -506,6 +526,7 @@ static gboolean run_command(gboolean interactive, gchar **argv, close(pipeerr[1]); } *appin = *appout = *apperr = -1; + g_strfreev(appargv); return FALSE; }

@@ -639,8 +660,7 @@ typedef enum { GVIR_SANDBOX_CONSOLE_STATE_RUNNING, } GVirSandboxConsoleState;

-static gboolean eventloop(gboolean interactive, - gchar **appargv, +static gboolean eventloop(GVirSandboxConfig *config, int sigread, int host) { @@ -825,8 +845,7 @@ static gboolean eventloop(gboolean interactive, if (rx->buffer[0] == GVIR_SANDBOX_PROTOCOL_HANDSHAKE_SYNC) { if (debug) fprintf(stderr, "Running command\n"); - if (!run_command(interactive, - appargv, + if (!run_command(config, &child, &appin, &appout, @@ -1097,13 +1116,11 @@ static gboolean eventloop(gboolean interactive, static int run_interactive(GVirSandboxConfig *config) { - GVirSandboxConfigInteractive *iconfig = GVIR_SANDBOX_CONFIG_INTERACTIVE(config); int sigpipe[2] = { -1, -1 }; int host = -1; int ret = -1; struct termios rawattr; const char *devname; - gchar **command = NULL;

if (pipe(sigpipe) < 0) { g_printerr(_("libvirt-sandbox-init-common: unable to create signal pipe: %s"), @@ -1138,17 +1155,7 @@ run_interactive(GVirSandboxConfig *config) cfmakeraw(&rawattr); tcsetattr(host, TCSAFLUSH, &rawattr);

- - - if (change_user(gvir_sandbox_config_get_username(config), - gvir_sandbox_config_get_userid(config), - gvir_sandbox_config_get_groupid(config), - gvir_sandbox_config_get_homedir(config)) < 0) - goto cleanup; - - command = gvir_sandbox_config_get_command(config); - if (!eventloop(gvir_sandbox_config_interactive_get_tty(iconfig), - command, + if (!eventloop(config, sigpipe[0], host)) goto cleanup; @@ -1156,7 +1163,6 @@ run_interactive(GVirSandboxConfig *config) ret = 0;

cleanup: - g_strfreev(command); signal(SIGCHLD, SIG_DFL);

if (sigpipe[0] != -1)

ACK -- Cedric

On Fri, 2015-09-04 at 12:40 +0100, Daniel P. Berrange wrote:

To ensure that all pending I/O for filesytems backed by block devices is flushed to disk, it is important to sync and unmount the filesystems during shutdown. To avoid relying on the kernel reboot-on-panic behaviour, we also explicitly call reboot to power off the guest.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- libvirt-sandbox/libvirt-sandbox-init-common.c | 166 ++++++++++++++++++++++++++ libvirt-sandbox/libvirt-sandbox-init-qemu.c | 1 + 2 files changed, 167 insertions(+)

diff --git a/libvirt-sandbox/libvirt-sandbox-init-common.c b/libvirt-sandbox/libvirt-sandbox-init-common.c index 760509f..a3b4687 100644 --- a/libvirt-sandbox/libvirt-sandbox-init-common.c +++ b/libvirt-sandbox/libvirt-sandbox-init-common.c @@ -44,6 +44,8 @@ #include <unistd.h> #include <limits.h> #include <grp.h> +#include <mntent.h> +#include <sys/reboot.h>

#include "libvirt-sandbox-rpcpacket.h"

@@ -52,6 +54,8 @@ static gboolean verbose = FALSE; static int sigwrite;

#define ATTR_UNUSED __attribute__((__unused__)) +static void sync_data(void); +static void umount_fs(void);

static void sig_child(int sig ATTR_UNUSED) { @@ -598,6 +602,14 @@ static GVirSandboxRPCPacket *gvir_sandbox_encode_exit(int status, pkt->header.type = GVIR_SANDBOX_PROTOCOL_TYPE_MESSAGE; pkt->header.serial = serial;

+ /* The host may destroy the guest any time after receiving + * the exit code messages. So although the main() has code + * to sync + unmount we can't rely on that running. So we + * opportunistically sync + unmount here too. + */ + sync_data(); + umount_fs(); + if (!gvir_sandbox_rpcpacket_encode_header(pkt, error)) goto error; if (!gvir_sandbox_rpcpacket_encode_payload_msg(pkt, @@ -613,7 +625,151 @@ static GVirSandboxRPCPacket *gvir_sandbox_encode_exit(int status, return NULL; }

+/* Copied & adapted from libguestfs daemon/sync.c under LGPLv2+ */ +static void sync_data(void) +{ + DIR *dir; + struct dirent *d; + char *dev_path; + int fd; + + if (debug) + fprintf(stderr, "Syncing data\n"); + sync(); + + /* On Linux, sync(2) doesn't perform a barrier, so qemu (which may + * have a writeback cache, even with cache=none) will still have + * some unwritten data. Force the data out of any qemu caches, by + * calling fsync on all block devices. Note we still need the + * call to sync above in order to schedule the writes. + * Thanks to: Avi Kivity, Kevin Wolf. + */ + + if (!(dir = opendir("/dev"))) { + fprintf(stderr, "opendir: /dev failed %s\n", strerror(errno)); + return; + } + + for (;;) { + errno = 0; + d = readdir(dir); + if (!d) + break; + + if (!(g_str_has_prefix(d->d_name, "sd") || + g_str_has_prefix(d->d_name, "hd") || + g_str_has_prefix(d->d_name, "ubd") || + g_str_has_prefix(d->d_name, "vd") || + g_str_has_prefix(d->d_name, "sr"))) { + continue; + } + + dev_path = g_strdup_printf("/dev/%s", d->d_name); + + if (debug) + fprintf(stderr, "Syncing fd %s\n", dev_path); + if ((fd = open(dev_path, O_RDONLY)) < 0) { + fprintf(stderr, "cannot open %s: %s\n", dev_path, + strerror(errno)); + g_free(dev_path); + continue; + } + + /* fsync the device. */ + if (debug) { + fprintf(stderr, "fsync %s\n", dev_path); + } + + if (fsync(fd) < 0) { + fprintf(stderr, "failed to fsync %s: %s\n", + dev_path, strerror(errno)); + } + if (close(fd) < 0) { + fprintf(stderr, "failed to close %s: %s\n", + dev_path, strerror(errno)); + } + g_free(dev_path); + } + + /* Check readdir didn't fail */ + if (errno != 0) { + fprintf(stderr, "Failed to read /dev: %s\n", + strerror(errno)); + } + + /* Close the directory handle */ + if (closedir(dir) < 0) { + fprintf(stderr, "Failed to block /dev: %s\n", + strerror(errno)); + } + + if (debug) + fprintf(stderr, "Syncing complete\n"); +} + + +static int +compare_longest_first (gconstpointer vp1, gconstpointer vp2) +{ + int n1 = strlen(vp1); + int n2 = strlen(vp2); + return n2 - n1; +} + + +/* Copied & adapted from libguestfs daemon/sync.c under LGPLv2+ */ +static void umount_fs(void) +{ + FILE *fp; + struct mntent *m; + GList *mounts = NULL, *tmp; + + if (debug) + fprintf(stderr, "Unmounting all filesystems\n"); + if (!(fp = setmntent ("/proc/mounts", "r"))) { + fprintf(stderr, "Failed to open /proc/mounts: %s\n", + strerror(errno)); + return; + } + + while ((m = getmntent (fp)) != NULL) { + if (debug) + fprintf(stderr, "Got fsname=%s dir=%s type=%s opts=%s freq=%d passno=%d\n", + m->mnt_fsname, m->mnt_dir, m->mnt_type, m->mnt_opts, + m->mnt_freq, m->mnt_passno); + + mounts = g_list_append(mounts, g_strdup(m->mnt_dir)); + }

+ endmntent(fp); + + mounts = g_list_sort(mounts, compare_longest_first); + + /* Unmount them. */ + tmp = mounts; + while (tmp) { + char *dir = tmp->data; + + if (debug) + fprintf(stderr, "Unmounting %s\n", dir); + if (umount(dir) < 0) { + /* We expect some failures, so don't pollute + * logs with them uneccessarily + */ + if (debug || errno != EBUSY) + fprintf(stderr, "cannot unmount %s: %s\n", + dir, strerror(errno)); + /* ignore failure */ + } + g_free(dir); + + tmp = tmp->next; + } + + g_list_free(mounts); + if (debug) + fprintf(stderr, "Unmounting complete\n"); +}

static gssize read_data(int fd, char *buf, size_t len) @@ -1204,6 +1360,7 @@ static void libvirt_sandbox_version(void)

int main(int argc, char **argv) { gchar *configfile = NULL; + gboolean poweroff = FALSE; GError *error = NULL; GOptionContext *context; GOptionEntry options[] = { @@ -1215,6 +1372,8 @@ int main(int argc, char **argv) { N_("display debugging information"), NULL }, { "config", 'c', 0, G_OPTION_ARG_STRING, &configfile, N_("config file path"), "URI"}, + { "poweroff", 'p', 0, G_OPTION_ARG_NONE, &poweroff, + N_("clean power off when exiting"), NULL}, { NULL, 0, 0, G_OPTION_ARG_NONE, NULL, NULL, NULL } }; const char *help_msg = N_("Run '" PACKAGE " --help' to see a full list of available command line options"); @@ -1290,6 +1449,13 @@ int main(int argc, char **argv) { if (error) g_error_free(error);

+ sync_data(); + + if (poweroff) { + umount_fs(); + reboot(RB_POWER_OFF); + /* Should not be reached, but if it is, kernel will panic anyway */ + } return ret;

error: diff --git a/libvirt-sandbox/libvirt-sandbox-init-qemu.c b/libvirt-sandbox/libvirt-sandbox-init-qemu.c index cd6055a..8bde224 100644 --- a/libvirt-sandbox/libvirt-sandbox-init-qemu.c +++ b/libvirt-sandbox/libvirt-sandbox-init-qemu.c @@ -424,6 +424,7 @@ main(int argc ATTR_UNUSED, char **argv ATTR_UNUSED)

args[narg++] = SANDBOXCONFIGDIR "/.libs/ld.so"; args[narg++] = SANDBOXCONFIGDIR "/.libs/libvirt-sandbox-init-common"; + args[narg++] = "--poweroff"; if (debug) args[narg++] = "-d";

ACK -- Cedric

On Fri, 2015-09-04 at 12:40 +0100, Daniel P. Berrange wrote:

The libvirt-sandbox-init-qemu command expects to see 'strace=' or 'strace=some,list,of,syscalls' but we only passed 'strace'. This meant strace could never be enabled.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- libvirt-sandbox/libvirt-sandbox-builder-machine.c | 7 ++----- libvirt-sandbox/libvirt-sandbox-init-qemu.c | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/libvirt-sandbox/libvirt-sandbox-builder-machine.c b/libvirt-sandbox/libvirt-sandbox-builder-machine.c index a458882..a142f68 100644 --- a/libvirt-sandbox/libvirt-sandbox-builder-machine.c +++ b/libvirt-sandbox/libvirt-sandbox-builder-machine.c @@ -239,11 +239,8 @@ static gchar *gvir_sandbox_builder_machine_cmdline(GVirSandboxConfig *config G_G g_string_append(str, " quiet loglevel=0");

if ((tmp = getenv("LIBVIRT_SANDBOX_STRACE"))) { - g_string_append(str, " strace"); - if (!g_str_equal(tmp, "1")) { - g_string_append(str, "="); - g_string_append(str, tmp); - } + g_string_append(str, " strace="); + g_string_append(str, tmp); }

/* These make boot a little bit faster */ diff --git a/libvirt-sandbox/libvirt-sandbox-init-qemu.c b/libvirt-sandbox/libvirt-sandbox-init-qemu.c index 8bde224..bbe70ad 100644 --- a/libvirt-sandbox/libvirt-sandbox-init-qemu.c +++ b/libvirt-sandbox/libvirt-sandbox-init-qemu.c @@ -414,7 +414,7 @@ main(int argc ATTR_UNUSED, char **argv ATTR_UNUSED) args[narg++] = "/tmp/sandbox.log"; args[narg++] = "-f"; args[narg++] = "-ff"; - if (strace) { + if (strace && STRNEQ(strace, "1")) { args[narg++] = "-e"; args[narg++] = strace; }

ACK -- Cedric

On Fri, 2015-09-04 at 12:40 +0100, Daniel P. Berrange wrote:

The RPC console is closed when the libvirt-sandbox-init-common binary reports the exit of the guest process. We still have some cleanup code that runs in the guest, for example, syncing and ummounting filesystems. We want to be able to see debug and/or error messages from this code, so we should not quit until we get a close on that console. This should happen a few ms after the close on the RPC console, but just in case something causes shutdown to hang, we have a delayed timer registered.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- bin/virt-sandbox.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/bin/virt-sandbox.c b/bin/virt-sandbox.c index 195515f..332e53e 100644 --- a/bin/virt-sandbox.c +++ b/bin/virt-sandbox.c @@ -34,6 +34,22 @@ static gboolean do_close(GVirSandboxConsole *con G_GNUC_UNUSED, return FALSE; }

+static gboolean do_delayed_close(gpointer opaque) +{ + GMainLoop *loop = opaque; + g_main_loop_quit(loop); + return FALSE; +} + +static gboolean do_pending_close(GVirSandboxConsole *con G_GNUC_UNUSED, + gboolean error G_GNUC_UNUSED, + gpointer opaque) +{ + GMainLoop *loop = opaque; + g_timeout_add(2000, do_delayed_close, loop); + return FALSE; +} + static gboolean do_exited(GVirSandboxConsole *con G_GNUC_UNUSED, int status, gpointer opaque) @@ -256,7 +272,12 @@ int main(int argc, char **argv) { error && error->message ? error->message : _("Unknown failure")); goto cleanup; } - g_signal_connect(con, "closed", (GCallback)do_close, loop); + /* We don't close right away - we want to ensure we read any + * final debug info from the log console. We should get an + * EOF on that console which will trigger the real close, + * but we schedule a timer just in case. + */ + g_signal_connect(con, "closed", (GCallback)do_pending_close, loop); g_signal_connect(con, "exited", (GCallback)do_exited, &ret);

if (!(gvir_sandbox_console_attach_stdio(con, &error))) {

ACK -- Cedric