The old semantics of virFork() violates the priciple of good usability: it requires the caller to check the pid argument after use, *even when virFork returned -1*, in order to properly abort a child process that failed setup done immediately after fork() - that is, the caller must call _exit() in the child. While uses in virfile.c did this correctly, uses in 'virsh lxc-enter-namespace' and 'virt-login-shell' would happily return from the calling function in both the child and the parent, leading to very confusing results. [Thankfully, I found the problem by inspection, and can't actually trigger the double return on error without an LD_PRELOAD library.] It is much better if the semantics of virFork are impossible to abuse. Looking at virFork(), the parent could only ever return -1 with a non-negative pid if it misused pthread_sigmask, but this never happens. The child could return -1 with non-negative pid if it fails to set up signals correctly, which is less likely but harder to guarantee - but if we instead make the child call _exit() at that point instead of forcing the caller to do it, then it is harder for the caller to misuse the virFork interface. Note that once we make that change, the semantics of the return value and of the pid argument are now redundant (a -1 return now happens only for failure to fork, a child 0 return only happens for a successful 0 pid, and a parent 0 return only happens for a successful non-zero pid), so we might as well return the pid directly rather than an integer of whether it succeeded or failed; this is also good from the interface design perspective as users are already familiar with fork() semantics. One last change in this patch: before returning the pid directly, I found cases where using virProcessWait unconditionally on a cleanup path of a virFork's -1 pid return would be nicer if there were a way to avoid it overwriting an earlier message. While such paths are a bit harder to come by with my change to a direct pid return, I decided to keep the virProcessWait change in this patch. * src/util/virprocess.c (virProcessWait): Tweak semantics when pid is -1. * src/util/vircommand.h (virFork): Change signature. * src/util/vircommand.c (virFork): Guarantee that child will only return on success, to simplify callers. Return pid rather than status, now that the situations are always the same. (virExec): Adjust caller, also avoid open-coding process death. * src/util/virfile.c (virFileAccessibleAs, virFileOpenForked) (virDirCreate): Adjust callers. * tools/virsh-domain.c (cmdLxcEnterNamespace): Likewise. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/util/vircommand.c | 127 ++++++++++++++++++----------------------------- src/util/vircommand.h | 2 +- src/util/virfile.c | 25 ++-------- src/util/virprocess.c | 15 +++--- tools/virsh-domain.c | 7 +-- tools/virt-login-shell.c | 12 +++-- 6 files changed, 75 insertions(+), 113 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index a52a1ab..ad767d7 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -193,28 +193,27 @@ virCommandFDSet(virCommandPtr cmd, /** * virFork: - * @pid - a pointer to a pid_t that will receive the return value from - * fork() * - * fork a new process while avoiding various race/deadlock conditions - * - * on return from virFork(), if *pid < 0, the fork failed and there is - * no new process. Otherwise, just like fork(), if *pid == 0, it is the - * child process returning, and if *pid > 0, it is the parent. - * - * Even if *pid >= 0, if the return value from virFork() is < 0, it - * indicates a failure that occurred in the parent or child process - * after the fork. In this case, the child process should call - * _exit(EXIT_FAILURE) after doing any additional error reporting. + * Wrapper around fork() that avoids various race/deadlock conditions. + * + * Like fork(), there are several return possibilities: + * 1. No child was created: the return is -1, errno is set, and an error + * message has been reported. The semantics of virWaitProcess() recognize + * this to avoid clobbering the error message from here. + * 2. This is the parent: the return is > 0. The parent can now attempt + * to interact with the child (but be aware that unlike raw fork(), the + * child may not return - some failures in the child result in this + * function calling _exit(255) if the child cannot be set up correctly). + * 3. This is the child: the return is 0. If this happens, the parent + * is also guaranteed to return. */ -int -virFork(pid_t *pid) +pid_t +virFork(void) { sigset_t oldmask, newmask; struct sigaction sig_action; - int saved_errno, ret = -1; - - *pid = -1; + int saved_errno; + pid_t pid; /* * Need to block signals now, so that child process can safely @@ -222,54 +221,47 @@ virFork(pid_t *pid) */ sigfillset(&newmask); if (pthread_sigmask(SIG_SETMASK, &newmask, &oldmask) != 0) { - saved_errno = errno; virReportSystemError(errno, "%s", _("cannot block signals")); - goto cleanup; + return -1; } /* Ensure we hold the logging lock, to protect child processes * from deadlocking on another thread's inherited mutex state */ virLogLock(); - *pid = fork(); + pid = fork(); saved_errno = errno; /* save for caller */ /* Unlock for both parent and child process */ virLogUnlock(); - if (*pid < 0) { + if (pid < 0) { /* attempt to restore signal mask, but ignore failure, to - avoid obscuring the fork failure */ + * avoid obscuring the fork failure */ ignore_value(pthread_sigmask(SIG_SETMASK, &oldmask, NULL)); virReportSystemError(saved_errno, "%s", _("cannot fork child process")); - goto cleanup; - } - - if (*pid) { + errno = saved_errno; + } else if (pid) { /* parent process */ /* Restore our original signal mask now that the child is - safely running */ - if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) != 0) { - saved_errno = errno; /* save for caller */ - virReportSystemError(errno, "%s", _("cannot unblock signals")); - goto cleanup; - } - ret = 0; + * safely running. Only documented failures are EFAULT (not + * possible, since we are using just-grabbed mask) or EINVAL + * (not possible, since we are using correct arguments). */ + ignore_value(pthread_sigmask(SIG_SETMASK, &oldmask, NULL)); } else { - /* child process */ int logprio; size_t i; - /* Remove any error callback so errors in child now - get sent to stderr where they stand a fighting chance - of being seen / logged */ + /* Remove any error callback so errors in child now get sent + * to stderr where they stand a fighting chance of being seen + * and logged */ virSetErrorFunc(NULL, NULL); virSetErrorLogPriorityFunc(NULL); @@ -280,36 +272,30 @@ virFork(pid_t *pid) virLogSetDefaultPriority(logprio); /* Clear out all signal handlers from parent so nothing - unexpected can happen in our child once we unblock - signals */ + * unexpected can happen in our child once we unblock + * signals */ sig_action.sa_handler = SIG_DFL; sig_action.sa_flags = 0; sigemptyset(&sig_action.sa_mask); for (i = 1; i < NSIG; i++) { - /* Only possible errors are EFAULT or EINVAL - The former wont happen, the latter we - expect, so no need to check return value */ - - sigaction(i, &sig_action, NULL); + /* Only possible errors are EFAULT or EINVAL The former + * won't happen, the latter we expect, so no need to check + * return value */ + ignore_value(sigaction(i, &sig_action, NULL)); } - /* Unmask all signals in child, since we've no idea - what the caller's done with their signal mask - and don't want to propagate that to children */ + /* Unmask all signals in child, since we've no idea what the + * caller's done with their signal mask and don't want to + * propagate that to children */ sigemptyset(&newmask); if (pthread_sigmask(SIG_SETMASK, &newmask, NULL) != 0) { - saved_errno = errno; /* save for caller */ virReportSystemError(errno, "%s", _("cannot unblock signals")); - goto cleanup; + virDispatchError(NULL); + _exit(255); } - ret = 0; } - -cleanup: - if (ret < 0) - errno = saved_errno; - return ret; + return pid; } /* @@ -407,7 +393,7 @@ virExec(virCommandPtr cmd) int tmpfd; char *binarystr = NULL; const char *binary = NULL; - int forkRet, ret; + int ret; struct sigaction waxon, waxoff; gid_t *groups = NULL; int ngroups; @@ -484,17 +470,13 @@ virExec(virCommandPtr cmd) if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0) goto cleanup; - forkRet = virFork(&pid); + pid = virFork(); if (pid < 0) { goto cleanup; } if (pid) { /* parent */ - if (forkRet < 0) { - goto cleanup; - } - VIR_FORCE_CLOSE(null); if (cmd->outfdptr && *cmd->outfdptr == -1) { VIR_FORCE_CLOSE(pipeout[1]); @@ -514,14 +496,6 @@ virExec(virCommandPtr cmd) } /* child */ - - if (forkRet < 0) { - /* The fork was successful, but after that there was an error - * in the child (which was already logged). - */ - goto fork_error; - } - openmax = sysconf(_SC_OPEN_MAX); if (openmax < 0) { virReportSystemError(errno, "%s", @@ -592,12 +566,10 @@ virExec(virCommandPtr cmd) if (pid > 0) { if (cmd->pidfile && (virPidFileWritePath(cmd->pidfile, pid) < 0)) { - kill(pid, SIGTERM); - usleep(500*1000); - kill(pid, SIGTERM); - virReportSystemError(errno, - _("could not write pidfile %s for %d"), - cmd->pidfile, pid); + if (virProcessKillPainfully(pid, true) >= 0) + virReportSystemError(errno, + _("could not write pidfile %s for %d"), + cmd->pidfile, pid); goto fork_error; } _exit(0); @@ -778,10 +750,9 @@ virExec(virCommandPtr cmd ATTRIBUTE_UNUSED) return -1; } -int -virFork(pid_t *pid) +pid_t +virFork(void) { - *pid = -1; errno = ENOTSUP; return -1; diff --git a/src/util/vircommand.h b/src/util/vircommand.h index e977f93..4372a7f 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -33,7 +33,7 @@ typedef virCommand *virCommandPtr; * call any function that is not async-signal-safe. */ typedef int (*virExecHook)(void *data); -int virFork(pid_t *pid) ATTRIBUTE_RETURN_CHECK; +pid_t virFork(void) ATTRIBUTE_RETURN_CHECK; int virRun(const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; diff --git a/src/util/virfile.c b/src/util/virfile.c index 631cd06..92d4815 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1730,7 +1730,7 @@ virFileAccessibleAs(const char *path, int mode, if (ngroups < 0) return -1; - forkRet = virFork(&pid); + pid = virFork(); if (pid < 0) { VIR_FREE(groups); @@ -1740,8 +1740,7 @@ virFileAccessibleAs(const char *path, int mode, if (pid) { /* parent */ VIR_FREE(groups); if (virProcessWait(pid, &status) < 0) { - /* virProcessWait() already - * reported error */ + /* virProcessWait() already reported error */ return -1; } @@ -1842,7 +1841,6 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, int waitret, status, ret = 0; int fd = -1; int pair[2] = { -1, -1 }; - int forkRet; gid_t *groups; int ngroups; @@ -1864,7 +1862,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, return ret; } - forkRet = virFork(&pid); + pid = virFork(); if (pid < 0) return -errno; @@ -1872,15 +1870,8 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, /* child */ - VIR_FORCE_CLOSE(pair[0]); /* preserves errno */ - if (forkRet < 0) { - /* error encountered and logged in virFork() after the fork. */ - ret = -errno; - goto childerror; - } - /* set desired uid/gid, then attempt to create the file */ - + VIR_FORCE_CLOSE(pair[0]); if (virSetUIDGID(uid, gid, groups, ngroups) < 0) { ret = -errno; goto childerror; @@ -2151,7 +2142,7 @@ virDirCreate(const char *path, if (ngroups < 0) return -errno; - int forkRet = virFork(&pid); + pid = virFork(); if (pid < 0) { ret = -errno; @@ -2181,13 +2172,7 @@ parenterror: /* child */ - if (forkRet < 0) { - /* error encountered and logged in virFork() after the fork. */ - goto childerror; - } - /* set desired uid/gid, then attempt to create the directory */ - if (virSetUIDGID(uid, gid, groups, ngroups) < 0) { ret = -errno; goto childerror; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 9fc3207..5fbed25 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -151,10 +151,12 @@ virProcessAbort(pid_t pid) * @exitstatus: optional status collection * * Wait for a child process to complete. - * Return -1 on any error waiting for - * completion. Returns 0 if the command - * finished with the exit status set. If @exitstatus is NULL, then the - * child must exit with status 0 for this to succeed. + * If @pid is -1, do nothing, but return -1 (useful for error cleanup, + * and assumes an earlier message was already issued). Otherwise, + * return -1 on any error waiting for completion, with an error message + * issued. If @exitstatus is NULL, require the process to complete normally + * with status 0; otherwise, populate @exitstatus (whether the process + * completed normally or was terminated by a signal). */ int virProcessWait(pid_t pid, int *exitstatus) @@ -163,8 +165,9 @@ virProcessWait(pid_t pid, int *exitstatus) int status; if (pid <= 0) { - virReportSystemError(EINVAL, _("unable to wait for process %lld"), - (long long) pid); + if (pid != -1) + virReportSystemError(EINVAL, _("unable to wait for process %lld"), + (long long) pid); return -1; } diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 760dca5..d9b542e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8134,9 +8134,10 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd) } /* Fork once because we don't want to affect - * virsh's namespace itself + * virsh's namespace itself, and because user namespace + * can only be changed in single-threaded process */ - if (virFork(&pid) < 0) + if ((pid = virFork()) < 0) goto cleanup; if (pid == 0) { if (setlabel && @@ -8157,7 +8158,7 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd) /* Fork a second time because entering the * pid namespace only takes effect after fork */ - if (virFork(&pid) < 0) + if ((pid = virFork()) < 0) _exit(255); if (pid == 0) { execv(cmdargv[0], cmdargv); diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index ec78fbc..e26b7a1 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -309,7 +309,10 @@ main(int argc, char **argv) if (virDomainGetSecurityLabel(dom, seclabel) < 0) goto cleanup; - if (virFork(&cpid) < 0) + /* Fork once because we don't want to affect virt-login-shell's + * namespace itself. FIXME: is this necessary? + */ + if ((cpid = virFork()) < 0) goto cleanup; if (cpid == 0) { @@ -318,9 +321,6 @@ main(int argc, char **argv) int openmax = sysconf(_SC_OPEN_MAX); int fd; - /* Fork once because we don't want to affect - * virt-login-shell's namespace itself - */ if (virSetUIDGID(0, 0, NULL, 0) < 0) return EXIT_FAILURE; @@ -355,7 +355,9 @@ main(int argc, char **argv) VIR_MASS_CLOSE(tmpfd); } - if (virFork(&ccpid) < 0) + /* A fork is required to create new process in correct pid + * namespace. */ + if ((ccpid = virFork()) < 0) return EXIT_FAILURE; if (ccpid == 0) { -- 1.8.4.2

virt-login-shell was exiting with status 0, regardless of what the wrapped shell returned. This is unkind to users; we should behave more like env(1), nice(1), su(1), and other wrapper programs, by preserving the invoked application's status (which includes the distinction between death due to signal vs. normal death). * src/internal.h (EXIT_CANCELED, EXIT_CANNOT_INVOKE) (EXIT_ENOENT): New enum. * src/util/vircommand.c (virFork): Document specific exit value if child aborts early. * tools/virt-login-shell.c (main): Pass through child exit status. * tools/virt-login-shell.pod: Document exit status. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/internal.h | 7 +++++++ src/util/vircommand.c | 5 +++-- tools/virt-login-shell.c | 44 ++++++++++++++++++++++++++++---------------- tools/virt-login-shell.pod | 23 ++++++++++++++++++++++- 4 files changed, 60 insertions(+), 19 deletions(-) diff --git a/src/internal.h b/src/internal.h index 5e29694..c90c83f 100644 --- a/src/internal.h +++ b/src/internal.h @@ -431,5 +431,12 @@ #NAME ": " FMT, __VA_ARGS__); # endif +/* Specific error values for use in forwarding programs such as + * virt-login-shell; these values match what GNU env does. */ +enum { + EXIT_CANCELED = 125, /* Failed before attempting exec */ + EXIT_CANNOT_INVOKE = 126, /* Exists but couldn't exec */ + EXIT_ENOENT = 127, /* Could not find program to exec */ +}; #endif /* __VIR_INTERNAL_H__ */ diff --git a/src/util/vircommand.c b/src/util/vircommand.c index ad767d7..3bbbe5f 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -203,7 +203,8 @@ virCommandFDSet(virCommandPtr cmd, * 2. This is the parent: the return is > 0. The parent can now attempt * to interact with the child (but be aware that unlike raw fork(), the * child may not return - some failures in the child result in this - * function calling _exit(255) if the child cannot be set up correctly). + * function calling _exit(EXIT_CANCELED) if the child cannot be set up + * correctly). * 3. This is the child: the return is 0. If this happens, the parent * is also guaranteed to return. */ @@ -292,7 +293,7 @@ virFork(void) if (pthread_sigmask(SIG_SETMASK, &newmask, NULL) != 0) { virReportSystemError(errno, "%s", _("cannot unblock signals")); virDispatchError(NULL); - _exit(255); + _exit(EXIT_CANCELED); } } return pid; diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index bc8394e..2172325 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -21,13 +21,14 @@ */ #include <config.h> -#include <stdarg.h> -#include <getopt.h> -#include <stdio.h> #include <errno.h> -#include <stdlib.h> #include <fnmatch.h> +#include <getopt.h> #include <locale.h> +#include <signal.h> +#include <stdarg.h> +#include <stdio.h> +#include <stdlib.h> #include "internal.h" #include "virerror.h" @@ -168,8 +169,8 @@ main(int argc, char **argv) { virConfPtr conf = NULL; const char *login_shell_path = conf_file; - pid_t cpid; - int ret = EXIT_FAILURE; + pid_t cpid = -1; + int ret = EXIT_CANCELED; int status; uid_t uid = getuid(); gid_t gid = getgid(); @@ -195,8 +196,8 @@ main(int argc, char **argv) {NULL, 0, NULL, 0} }; if (virInitialize() < 0) { - fprintf(stderr, _("Failed to initialize libvirt Error Handling")); - return EXIT_FAILURE; + fprintf(stderr, _("Failed to initialize libvirt error handling")); + return EXIT_CANCELED; } setenv("PATH", "/bin:/usr/bin", 1); @@ -231,7 +232,7 @@ main(int argc, char **argv) case '?': default: usage(); - exit(EXIT_FAILURE); + exit(EXIT_CANCELED); } } @@ -330,15 +331,13 @@ main(int argc, char **argv) if (execv(shargv[0], (char *const*) shargv) < 0) { virReportSystemError(errno, _("Unable to exec shell %s"), shargv[0]); - return EXIT_FAILURE; + virDispatchError(NULL); + return errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE; } - return EXIT_SUCCESS; } - if (virProcessWait(cpid, &status) < 0) - goto cleanup; - ret = EXIT_SUCCESS; - + /* At this point, the parent is now waiting for the child to exit, + * but as that may take a long time, we release resources now. */ cleanup: for (i = 0; i < nfdlist; i++) VIR_FORCE_CLOSE(fdlist[i]); @@ -354,7 +353,20 @@ cleanup: VIR_FREE(seclabel); VIR_FREE(secmodel); VIR_FREE(groups); - if (ret) + + if (virProcessWait(cpid, &status) == 0) { + if (WIFEXITED(status)) { + ret = WEXITSTATUS(status); + } else if (WIFSIGNALED(status)) { + /* Try to kill the parent with the same signal as the + * child, but if that fails, at least give a decent exit + * status value. */ + raise(WTERMSIG(status)); + ret = 128 + WTERMSIG(status); + } + } + + if (virGetLastError()) virDispatchError(NULL); return ret; } diff --git a/tools/virt-login-shell.pod b/tools/virt-login-shell.pod index bcd7855..c1d0cb3 100644 --- a/tools/virt-login-shell.pod +++ b/tools/virt-login-shell.pod @@ -4,7 +4,7 @@ virt-login-shell - tool to execute a shell within a container matching the users =head1 SYNOPSIS -B<virt-login-shell> +B<virt-login-shell> [I<OPTION>] =head1 DESCRIPTION @@ -47,6 +47,27 @@ variable in /etc/libvirt/virt-login-shell.conf. eg. allowed_users = [ "tom", "dick", "harry" ] +=head1 EXIT STATUS + +B<virt-login-shell> normally returns the exit status of the command it +executed. If the command was killed by a signal, but that signal is not +fatal to virt-login-shell, then it returns the signal number plus 128. + +Exit status generated by B<virt-login-shell> itself: + +=over 4 + +=item B<0> An option was used to learn more about this binary. + +=item B<125> Generic error before attempting execution of the configured +shell; for example, if libvirtd is not running. + +=item B<126> The configured shell exists but could not be executed. + +=item B<127> The configured shell could not be found. + +=back + =head1 BUGS Report any bugs discovered to the libvirt community via the mailing -- 1.8.4.2

Implement virProcessRunInMountNamespace, which runs callback of type virProcessNamespaceCallback in a container namespace. Idea by Dan Berrange, based on an initial report by Reco <recoverym4n(a)gmail.com&gt; at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394 Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- setns() is a per-thread call. Would it be any simpler to just pthread_create() a short-lived helper thread, so that we don't have to worry about full-blown async-signal safety, and so that the thread can pass more information back rather than the limitation of an exit status? --- src/libvirt_private.syms | 1 + src/util/virprocess.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 11 ++++++++ 3 files changed, 78 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2dbb8f8..e210fd0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1646,6 +1646,7 @@ virProcessGetNamespaces; virProcessGetStartTime; virProcessKill; virProcessKillPainfully; +virProcessRunInMountNamespace; virProcessSetAffinity; virProcessSetMaxFiles; virProcessSetMaxMemLock; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 5fbed25..c99b75a 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -31,6 +31,7 @@ # include <sys/resource.h> #endif #include <sched.h> +#include <stdlib.h> #ifdef __FreeBSD__ # include <sys/param.h> @@ -39,6 +40,7 @@ #endif #include "viratomic.h" +#include "vircommand.h" #include "virprocess.h" #include "virerror.h" #include "viralloc.h" @@ -850,3 +852,67 @@ int virProcessGetStartTime(pid_t pid, return 0; } #endif + + +#ifdef HAVE_SETNS +/* Run cb(opaque) in the mount namespace of pid. Return -1 with error + * message raised if we fail to run the child, if the child dies from + * a signal, or if the child has status EXIT_CANCELED; otherwise + * return the exit status of the child. */ +int +virProcessRunInMountNamespace(pid_t pid, + virProcessNamespaceCallback cb, + void *opaque) +{ + char *path = NULL; + int ret = -1; + int cpid = -1; + int fd = -1; + int status; + + if (virAsprintf(&path, "/proc/%llu/ns/mnt", (unsigned long long)pid) < 0) + goto cleanup; + + if ((fd = open(path, O_RDONLY)) < 0) { + virReportSystemError(errno, "%s", + _("Kernel does not provide mount namespace")); + goto cleanup; + } + + if ((cpid = virFork() < 0)) + goto cleanup; + if (cpid == 0) { + /* child */ + if (setns(fd, 0) == -1) + _exit(EXIT_CANCELED); + + ret = cb(pid, opaque); + _exit(ret < 0 ? EXIT_CANCELED : ret); + } + + /* parent */ + if (virProcessWait(cpid, &status) < 0) + goto cleanup; + if (!WIFEXITED(status) || WEXITSTATUS(status) == EXIT_CANCELED) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("mount namespace callback did not complete normally")); + goto cleanup; + } + ret = WEXITSTATUS(status); + +cleanup: + VIR_FREE(path); + VIR_FORCE_CLOSE(fd); + return ret; +} +#else /* !HAVE_SETNS */ +int +virProcessRunInMountNamespace(pid_t pid ATTRIBUTE_UNUSED, + virProcessNamespaceCallback cb ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Mount namespaces are not available on this platform")); + return -1; +} +#endif diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 9f77bc5..75c7d1b 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -60,4 +60,15 @@ int virProcessSetNamespaces(size_t nfdlist, int virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes); int virProcessSetMaxProcesses(pid_t pid, unsigned int procs); int virProcessSetMaxFiles(pid_t pid, unsigned int files); + +/* Callback to run code within the mount namespace tied to the given + * pid. This function must use only async-signal-safe functions, as + * it gets run after a fork of a multi-threaded process. The return + * value of this function is passed to _exit(), except that a + * negative value is treated as EXIT_CANCELED. */ +typedef int (*virProcessNamespaceCallback)(pid_t pid, void *opaque); + +int virProcessRunInMountNamespace(pid_t pid, + virProcessNamespaceCallback cb, + void *opaque); #endif /* __VIR_PROCESS_H__ */ -- 1.8.4.2

On 12/24/2013 12:08 AM, Reco wrote: Whoops - why is the parent trying to call the same thing... ...as the child? Oh, I see. [I _did_ say I hadn't tested patches 6 and 7 as much as the first five.] Please squash in this typo fix to 6/7, and that should clean up the problem (but I do appreciate you testing it): diff --git i/src/util/virprocess.c w/src/util/virprocess.c index c99b75a..e069483 100644 --- i/src/util/virprocess.c +++ w/src/util/virprocess.c @@ -879,7 +879,7 @@ virProcessRunInMountNamespace(pid_t pid, goto cleanup; } - if ((cpid = virFork() < 0)) + if ((cpid = virFork()) < 0) goto cleanup; if (cpid == 0) { /* child */ -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 12/24/2013 06:45 AM, Reco wrote: Thanks again for the functional review. I'm still waiting for a code review from anyone willing, since this does fix a security issue and I don't want to introduce an unintentional regression. And I guess there's still the need to fix the access to the namespace /dev during device hotplog... -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 01/07/2014 06:32 PM, Gao feng wrote: s/serial/series/ (English is weird) I've pushed patch 1, but am seeing if I can work up patches for the /dev issue before I push any others (in particular, if that work turns up any need to rethink the strategy, I'd like to avoid the churn - because I still want this CVE fixed in time for the 1.2.1 release). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org