On Wed, Jul 23, 2014 at 04:27:07PM +0200, Martin Kletzander wrote: Hmm, systemd should have already called listen() on the socket FD it passes to us. I wonder if it is better than we do the same when auto-spawning libvirtd passing it a listening socket. You kind of need to listen() to avoid a race condition I believe when spawning libvirtd twice. eg if libvirt.so calls listen() and it fails then we know there's another libvirtd in the process of being started on that socket, so we can go back to trying to connect() until it succeeeds. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Wed, Aug 13, 2014 at 05:05:26PM +0200, Martin Kletzander wrote: ACK, and it was actually 'bind' I was getting confused with which fails if already previously invoked. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Wed, Jul 23, 2014 at 04:27:08PM +0200, Martin Kletzander wrote: I'm wondering how we deal with TCP socket listening too. eg if someone modifies the systemd config so that it listens on the TCP sockets too then we're not setting up libvirt to deal with those. Rather than doing this NewFDOrUnix() approach, I'm wondering if we would be better off doing. if (getenv(LISTEN_FDS) != NULL) { ....call NewFD() on all the FDs we've been given... } else { .. do normal NewUnix/NewTCP/NewTLS setup... } though it gets harder than that because we'll need to call getsockname on the FD to determine if the socket we've received is a UNIX or TCP socket and what port number it is on (so we can see if it is TCP or TLS protocol) and we need to passin the TLS context in some cases. So perhaps the virNetServerServiceNewTCP and virNetServerServiceNewTLS method should simply accept a pre-opened FD. so we could do int unixfd = virGetInheritedUNIXSocket("/some/path"); int unixrofd = virGetInheritedTCPSocket("/some/other/path") int tcpfd = virGetInheritedTCPSocket(16509) int tlsfd = virGetInheritedTLSSocket(16514) and then pass these FDs into the regular virNetServerServiceNew* methods. Assume '-1' means pre-open the socket the normal way Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Even line like this: int asdf = i - somevar; was matched by the regex, so this patch adds '=' to the list of characters that break the regexp. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- cfg.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index 0559edd..f647aed 100644 --- a/cfg.mk +++ b/cfg.mk @@ -568,7 +568,7 @@ sc_avoid_attribute_unused_in_header: $(_sc_search_regexp) sc_prohibit_int_ijk: - @prohibit='\<(int|unsigned) ([^(]* )*(i|j|k)\>(\s|,|;)' \ + @prohibit='\<(int|unsigned) ([^(=]* )*(i|j|k)\>(\s|,|;)' \ halt='use size_t, not int/unsigned int for loop vars i, j, k' \ $(_sc_search_regexp) -- 2.0.2

Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- tests/commandtest.c | 49 ++++++++++++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 7d2161c..ba823f7 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -62,7 +62,8 @@ main(void) #else -static int checkoutput(const char *testname) +static int checkoutput(const char *testname, + char *prefix) { int ret = -1; char *expectname = NULL; @@ -86,6 +87,16 @@ static int checkoutput(const char *testname) goto cleanup; } + if (prefix) { + char *tmp = NULL; + + if (virAsprintf(&tmp, "%s%s", prefix, expectlog) < 0) + goto cleanup; + + VIR_FREE(expectlog); + expectlog = tmp; + } + if (STRNEQ(expectlog, actuallog)) { virtTestDifference(stderr, expectlog, actuallog); goto cleanup; @@ -173,7 +184,7 @@ static int test2(const void *unused ATTRIBUTE_UNUSED) return -1; } - if ((ret = checkoutput("test2")) != 0) { + if ((ret = checkoutput("test2", NULL)) != 0) { virCommandFree(cmd); return ret; } @@ -187,7 +198,7 @@ static int test2(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test2"); + return checkoutput("test2", NULL); } /* @@ -219,7 +230,7 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test3"); + ret = checkoutput("test3", NULL); cleanup: virCommandFree(cmd); @@ -261,7 +272,7 @@ static int test4(const void *unused ATTRIBUTE_UNUSED) while (kill(pid, 0) != -1) usleep(100*1000); - ret = checkoutput("test4"); + ret = checkoutput("test4", NULL); cleanup: virCommandFree(cmd); @@ -291,7 +302,7 @@ static int test5(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test5"); + return checkoutput("test5", NULL); } @@ -315,7 +326,7 @@ static int test6(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test6"); + return checkoutput("test6", NULL); } @@ -340,7 +351,7 @@ static int test7(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test7"); + return checkoutput("test7", NULL); } /* @@ -365,7 +376,7 @@ static int test8(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test8"); + return checkoutput("test8", NULL); } @@ -403,7 +414,7 @@ static int test9(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test9"); + return checkoutput("test9", NULL); } @@ -429,7 +440,7 @@ static int test10(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test10"); + return checkoutput("test10", NULL); } /* @@ -453,7 +464,7 @@ static int test11(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test11"); + return checkoutput("test11", NULL); } /* @@ -475,7 +486,7 @@ static int test12(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test12"); + return checkoutput("test12", NULL); } /* @@ -510,7 +521,7 @@ static int test13(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test13"); + ret = checkoutput("test13", NULL); cleanup: virCommandFree(cmd); @@ -582,7 +593,7 @@ static int test14(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test14"); + ret = checkoutput("test14", NULL); cleanup: virCommandFree(cmd); @@ -613,7 +624,7 @@ static int test15(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test15"); + ret = checkoutput("test15", NULL); cleanup: VIR_FREE(cwd); @@ -659,7 +670,7 @@ static int test16(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test16"); + ret = checkoutput("test16", NULL); cleanup: virCommandFree(cmd); @@ -841,7 +852,7 @@ static int test20(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test20"); + ret = checkoutput("test20", NULL); cleanup: virCommandFree(cmd); VIR_FREE(buf); @@ -900,7 +911,7 @@ static int test21(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test21"); + ret = checkoutput("test21", NULL); cleanup: VIR_FREE(outbuf); VIR_FREE(errbuf); -- 2.0.2

That sets a new flag, but that flag does mean the child will get LISTEN_FDS and LISTEN_PID environment variables properly set and passed FDs reordered so that it corresponds with LISTEN_FDS (they must start right after STDERR_FILENO). Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 99 ++++++++++++++++++++++++++++++++++++++++++++ src/util/vircommand.h | 4 +- tests/commanddata/test24.log | 7 ++++ tests/commandtest.c | 56 +++++++++++++++++++++++++ 5 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 tests/commanddata/test24.log diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d883990..4d13ab4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1144,6 +1144,7 @@ virCommandNewArgList; virCommandNewArgs; virCommandNonblockingFDs; virCommandPassFD; +virCommandPassListenFDs; virCommandRawStatus; virCommandRequireHandshake; virCommandRun; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index e775ba6..3b3e6f5 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -66,6 +66,7 @@ enum { VIR_EXEC_CLEAR_CAPS = (1 << 2), VIR_EXEC_RUN_SYNC = (1 << 3), VIR_EXEC_ASYNC_IO = (1 << 4), + VIR_EXEC_LISTEN_FDS = (1 << 5), }; typedef struct _virCommandFD virCommandFD; @@ -200,6 +201,78 @@ virCommandFDSet(virCommandPtr cmd, return 0; } +static void +virCommandReorderFDs(virCommandPtr cmd) +{ + int maxfd = 0; + int openmax = 0; + size_t i = 0; + + if (!cmd || cmd->has_error || !cmd->npassfd) + return; + + for (i = 0; i < cmd->npassfd; i++) + maxfd = MAX(cmd->passfd[i].fd, maxfd); + + openmax = sysconf(_SC_OPEN_MAX); + if (openmax < 0 || + maxfd + cmd->npassfd > openmax) + goto error; + + /* + * Simple two-pass sort, nothing fancy. This is not designed for + * anything else than passing around 2 FDs into the child. + * + * So first dup2() them somewhere else. + */ + for (i = 0; i < cmd->npassfd; i++) { + int newfd = maxfd + i + 1; + int oldfd = cmd->passfd[i].fd; + if (dup2(oldfd, newfd) != newfd) { + virReportSystemError(errno, + _("Cannot dup2() fd %d before " + "passing it to the child"), + oldfd); + goto error; + } + VIR_FORCE_CLOSE(cmd->passfd[i].fd); + } + + VIR_DEBUG("First reorder pass done"); + + /* + * And then dup2() them in orderly manner. + */ + for (i = 0; i < cmd->npassfd; i++) { + int newfd = STDERR_FILENO + i + 1; + int oldfd = maxfd + i + 1; + if (dup2(oldfd, newfd) != newfd) { + virReportSystemError(errno, + _("Cannot dup2() fd %d before " + "passing it to the child"), + oldfd); + goto error; + } + if (virSetInherit(newfd, true) < 0) { + virReportSystemError(errno, + _("Cannot set O_CLOEXEC on fd %d before " + "passing it to the child"), + newfd); + goto error; + } + VIR_FORCE_CLOSE(oldfd); + cmd->passfd[i].fd = newfd; + } + + VIR_DEBUG("Second reorder pass done"); + + return; + + error: + cmd->has_error = -1; + return; +} + #ifndef WIN32 /** @@ -678,6 +751,15 @@ virExec(virCommandPtr cmd) goto fork_error; } + if (cmd->flags & VIR_EXEC_LISTEN_FDS) { + virCommandReorderFDs(cmd); + virCommandAddEnvFormat(cmd, "LISTEN_PID=%u", getpid()); + virCommandAddEnvFormat(cmd, "LISTEN_FDS=%zu", cmd->npassfd); + + if (cmd->has_error) + goto fork_error; + } + /* Close logging again to ensure no FDs leak to child */ virLogReset(); @@ -919,6 +1001,23 @@ virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags) } /** + * virCommandPassListenFDs: + * @cmd: the command to modify + * + * Pass LISTEN_FDS and LISTEN_PID environment variables into the + * child. LISTEN_PID has the value of the child's PID and LISTEN_FDS + * is a number of passed file descriptors starting from 3. + */ +void +virCommandPassListenFDs(virCommandPtr cmd) +{ + if (!cmd || cmd->has_error) + return; + + cmd->flags |= VIR_EXEC_LISTEN_FDS; +} + +/** * virCommandSetPidFile: * @cmd: the command to modify * @pidfile: filename to use diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 8cdb31c..d3b286d 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -1,7 +1,7 @@ /* * vircommand.h: Child command execution * - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -60,6 +60,8 @@ void virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags); +void virCommandPassListenFDs(virCommandPtr cmd); + void virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) ATTRIBUTE_NONNULL(2); diff --git a/tests/commanddata/test24.log b/tests/commanddata/test24.log new file mode 100644 index 0000000..8670952 --- /dev/null +++ b/tests/commanddata/test24.log @@ -0,0 +1,7 @@ +FD:0 +FD:1 +FD:2 +FD:3 +FD:4 +DAEMON:yes +CWD:/ diff --git a/tests/commandtest.c b/tests/commandtest.c index ba823f7..b3287fa 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1032,6 +1032,61 @@ test23(const void *unused ATTRIBUTE_UNUSED) return ret; } +static int test24(const void *unused ATTRIBUTE_UNUSED) +{ + char *pidfile = virPidFileBuildPath(abs_builddir, "commandhelper"); + char *prefix = NULL; + int newfd1 = dup(STDERR_FILENO); + int newfd2 = dup(STDERR_FILENO); + int newfd3 = dup(STDERR_FILENO); + int ret = -1; + pid_t pid; + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + + if (!pidfile) + goto cleanup; + + if (VIR_CLOSE(newfd1) < 0) + printf("Cannot close fd %d\n", newfd1); + + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd); + virCommandPassFD(cmd, newfd2, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandPassFD(cmd, newfd3, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandPassListenFDs(cmd); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + goto cleanup; + } + + if (virPidFileRead(abs_builddir, "commandhelper", &pid) < 0) { + printf("cannot read pidfile\n"); + goto cleanup; + } + + if (virAsprintf(&prefix, + "ENV:LISTEN_FDS=2\nENV:LISTEN_PID=%u\n", + pid) < 0) + goto cleanup; + + while (kill(pid, 0) != -1) + usleep(100*1000); + + ret = checkoutput("test24", prefix); + + cleanup: + if (pidfile) + unlink(pidfile); + VIR_FREE(pidfile); + virCommandFree(cmd); + /* coverity[double_close] */ + VIR_FORCE_CLOSE(newfd2); + VIR_FORCE_CLOSE(newfd3); + return ret; +} + static void virCommandThreadWorker(void *opaque) { virCommandTestDataPtr test = opaque; @@ -1181,6 +1236,7 @@ mymain(void) DO_TEST(test21); DO_TEST(test22); DO_TEST(test23); + DO_TEST(test24); virMutexLock(&test->lock); if (test->running) { -- 2.0.2

This eliminates the need for active waiting. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/rpc/virnetsocket.c | 102 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 83 insertions(+), 19 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index a94b2bc..46be541 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -1,7 +1,7 @@ /* * virnetsocket.c: generic network socket handling * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -121,7 +121,7 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket) #ifndef WIN32 -static int virNetSocketForkDaemon(const char *binary) +static int virNetSocketForkDaemon(const char *binary, int passfd) { int ret; virCommandPtr cmd = virCommandNewArgList(binary, @@ -134,6 +134,10 @@ static int virNetSocketForkDaemon(const char *binary) virCommandAddEnvPassBlockSUID(cmd, "XDG_RUNTIME_DIR", NULL); virCommandClearCaps(cmd); virCommandDaemonize(cmd); + if (passfd) { + virCommandPassFD(cmd, passfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandPassListenFDs(cmd); + } ret = virCommandRun(cmd, NULL); virCommandFree(cmd); return ret; @@ -540,10 +544,11 @@ int virNetSocketNewConnectUNIX(const char *path, const char *binary, virNetSocketPtr *retsock) { + char *buf = NULL; + int errfd[2] = { -1, -1 }; + int fd, passfd; virSocketAddr localAddr; virSocketAddr remoteAddr; - int fd; - int retries = 0; memset(&localAddr, 0, sizeof(localAddr)); memset(&remoteAddr, 0, sizeof(remoteAddr)); @@ -569,28 +574,82 @@ int virNetSocketNewConnectUNIX(const char *path, if (remoteAddr.data.un.sun_path[0] == '@') remoteAddr.data.un.sun_path[0] = '\0'; - retry: - if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { - if ((errno == ECONNREFUSED || - errno == ENOENT) && - spawnDaemon && retries < 20) { - VIR_DEBUG("Connection refused for %s, trying to spawn %s", - path, binary); - if (retries == 0 && - virNetSocketForkDaemon(binary) < 0) - goto error; + if (spawnDaemon) { + int err = 0; + int rv = -1; + int status = 0; + pid_t pid = 0; - retries++; - usleep(1000 * 100 * retries); - goto retry; + if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { + virReportSystemError(errno, "%s", _("Failed to create socket")); + goto error; } - virReportSystemError(errno, - _("Failed to connect socket to '%s'"), + if (pipe2(errfd, O_CLOEXEC) < 0) { + virReportSystemError(errno, "%s", + _("Cannot create pipe for child")); + goto error; + } + + /* + * We have to fork() here, because umask() is set + * per-process, chmod() is racy and fchmod() has undefined + * behaviour on sockets according to POSIX, so it doesn't + * work outside Linux. + */ + + if ((pid = virFork()) < 0) + goto error; + + if (pid == 0) { + VIR_FORCE_CLOSE(errfd[0]); + + umask(0077); + rv = bind(passfd, &remoteAddr.data.sa, remoteAddr.len); + if (rv < 0) { + ignore_value(safewrite(errfd[1], &errno, sizeof(int))); + } + VIR_FORCE_CLOSE(errfd[1]); + _exit(rv < 0 ? EXIT_FAILURE : EXIT_SUCCESS); + } + + VIR_FORCE_CLOSE(errfd[1]); + rv = virProcessWait(pid, &status, false); + ignore_value(saferead(errfd[0], &err, sizeof(int))); + VIR_FORCE_CLOSE(errfd[0]); + + if (rv < 0 || status != EXIT_SUCCESS) { + if (err) { + virReportSystemError(err, + _("Failed to bind socket to %s " + "in child process"), + path); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to bind socket to %s " + "in child process"), + path); + } + goto error; + } + + if (listen(passfd, 0) < 0) { + virReportSystemError(errno, "%s", + _("Failed to listen on socket that's about " + "to be passed to the daemon")); + goto error; + } + } + + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + virReportSystemError(errno, _("Failed to connect socket to '%s'"), path); goto error; } + if (spawnDaemon && virNetSocketForkDaemon(binary, passfd) < 0) + goto error; + localAddr.len = sizeof(localAddr.data); if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) { virReportSystemError(errno, "%s", _("Unable to get local socket name")); @@ -603,7 +662,12 @@ int virNetSocketNewConnectUNIX(const char *path, return 0; error: + VIR_FREE(buf); VIR_FORCE_CLOSE(fd); + VIR_FORCE_CLOSE(errfd[0]); + VIR_FORCE_CLOSE(errfd[1]); + if (spawnDaemon) + unlink(path); return -1; } #else -- 2.0.2

On Wed, Aug 13, 2014 at 04:09:09PM +0100, Daniel P. Berrange wrote: Which one you mean? I'm keeping the return value as it was and the passfd is the fd that will be passed. [...] Wouldn't connect(), bind(), connect() be enough (for both issues)? Or do we need to try the dance few times again? Martin

On Thu, Aug 14, 2014 at 10:26:41AM +0100, Daniel P. Berrange wrote: [...] I probably expressed myself badly with that connect, bind, connect. This was the difference I was talking about: diff --git i/src/rpc/virnetsocket.c w/src/rpc/virnetsocket.c index 46be541..a4d5dd5 100644 --- i/src/rpc/virnetsocket.c +++ w/src/rpc/virnetsocket.c @@ -574,7 +574,12 @@ int virNetSocketNewConnectUNIX(const char *path, if (remoteAddr.data.un.sun_path[0] == '@') remoteAddr.data.un.sun_path[0] = '\0'; - if (spawnDaemon) { + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0 && + !spawnDaemon) { + virReportSystemError(errno, _("Failed to connect socket to '%s'"), + path); + goto error; + } else if (spawnDaemon) { int err = 0; int rv = -1; int status = 0; @@ -639,16 +644,16 @@ int virNetSocketNewConnectUNIX(const char *path, "to be passed to the daemon")); goto error; } - } - if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { - virReportSystemError(errno, _("Failed to connect socket to '%s'"), - path); - goto error; - } + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + virReportSystemError(errno, _("Failed to connect socket to '%s'"), + path); + goto error; + } - if (spawnDaemon && virNetSocketForkDaemon(binary, passfd) < 0) - goto error; + if (virNetSocketForkDaemon(binary, passfd) < 0) + goto error; + } localAddr.len = sizeof(localAddr.data); if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) { -- Martin

Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- .gitignore | 1 + daemon/Makefile.am | 14 ++++++++++++-- daemon/libvirtd.conf | 5 +++++ daemon/libvirtd.service.in | 5 ----- daemon/libvirtd.socket.in | 6 ++++++ libvirt.spec.in | 26 +++++++++++++++++++++----- 6 files changed, 45 insertions(+), 12 deletions(-) create mode 100644 daemon/libvirtd.socket.in diff --git a/.gitignore b/.gitignore index 90fee91..9776ea1 100644 --- a/.gitignore +++ b/.gitignore @@ -60,6 +60,7 @@ /daemon/libvirtd.pod /daemon/libvirtd.policy /daemon/libvirtd.service +/daemon/libvirtd.socket /daemon/test_libvirtd.aug /docs/aclperms.htmlinc /docs/apibuild.py.stamp diff --git a/daemon/Makefile.am b/daemon/Makefile.am index 00221ab..70b7655 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -55,6 +55,7 @@ EXTRA_DIST = \ libvirtd.policy.in \ libvirtd.sasl \ libvirtd.service.in \ + libvirtd.socket.in \ libvirtd.sysconf \ libvirtd.sysctl \ libvirtd.aug \ @@ -388,15 +389,18 @@ endif ! LIBVIRT_INIT_SCRIPT_UPSTART if LIBVIRT_INIT_SCRIPT_SYSTEMD SYSTEMD_UNIT_DIR = $(prefix)/lib/systemd/system -BUILT_SOURCES += libvirtd.service +BUILT_SOURCES += libvirtd.service libvirtd.socket -install-init-systemd: install-sysconfig libvirtd.service +install-init-systemd: install-sysconfig libvirtd.service libvirtd.socket $(MKDIR_P) $(DESTDIR)$(SYSTEMD_UNIT_DIR) $(INSTALL_DATA) libvirtd.service \ $(DESTDIR)$(SYSTEMD_UNIT_DIR)/libvirtd.service + $(INSTALL_DATA) libvirtd.socket \ + $(DESTDIR)$(SYSTEMD_UNIT_DIR)/libvirtd.socket uninstall-init-systemd: uninstall-sysconfig rm -f $(DESTDIR)$(SYSTEMD_UNIT_DIR)/libvirtd.service + rm -f $(DESTDIR)$(SYSTEMD_UNIT_DIR)/libvirtd.socket rmdir $(DESTDIR)$(SYSTEMD_UNIT_DIR) || : else ! LIBVIRT_INIT_SCRIPT_SYSTEMD install-init-systemd: @@ -420,6 +424,12 @@ libvirtd.service: libvirtd.service.in $(top_builddir)/config.status < $< > $@-t && \ mv $@-t $@ +libvirtd.socket: libvirtd.socket.in $(top_builddir)/config.status + $(AM_V_GEN)sed \ + -e 's|[@]runstatedir[@]|$(runstatedir)|g' \ + < $< > $@-t && \ + mv $@-t $@ + check-local: check-augeas diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf index e5856d4..b644e81 100644 --- a/daemon/libvirtd.conf +++ b/daemon/libvirtd.conf @@ -77,6 +77,11 @@ # UNIX socket access controls # +# Beware that if you are changing *any* of these options, and you use +# socket activation with systemd, you need to adjust the settings in +# the libvirtd.socket file as well since it could impose a security +# risk if you rely on file permission checking only. + # Set the UNIX domain socket group ownership. This can be used to # allow a 'trusted' set of users access to management capabilities # without becoming root. diff --git a/daemon/libvirtd.service.in b/daemon/libvirtd.service.in index 086da36..1759ac8 100644 --- a/daemon/libvirtd.service.in +++ b/daemon/libvirtd.service.in @@ -1,8 +1,3 @@ -# NB we don't use socket activation. When libvirtd starts it will -# spawn any virtual machines registered for autostart. We want this -# to occur on every boot, regardless of whether any client connects -# to a socket. Thus socket activation doesn't have any benefit - [Unit] Description=Virtualization daemon Before=libvirt-guests.service diff --git a/daemon/libvirtd.socket.in b/daemon/libvirtd.socket.in new file mode 100644 index 0000000..86cc3f4 --- /dev/null +++ b/daemon/libvirtd.socket.in @@ -0,0 +1,6 @@ +[Socket] +ListenStream=@runstatedir@/libvirt/libvirt-sock +ListenStream=@runstatedir@/libvirt/libvirt-sock-ro +SocketMode=0777 +SocketUser=root +SocketGroup=root diff --git a/libvirt.spec.in b/libvirt.spec.in index 76e57aa..272f381 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1669,11 +1669,14 @@ done %if %{with_systemd} %if %{with_systemd_macros} - %systemd_post virtlockd.socket libvirtd.service + %systemd_post virtlockd.socket libvirtd.service libvirtd.socket %else if [ $1 -eq 1 ] ; then # Initial installation - /bin/systemctl enable virtlockd.socket libvirtd.service >/dev/null 2>&1 || : + /bin/systemctl enable \ + virtlockd.socket \ + libvirtd.service \ + libvirtd.socket >/dev/null 2>&1 || : fi %endif %else @@ -1694,12 +1697,24 @@ fi %preun daemon %if %{with_systemd} %if %{with_systemd_macros} - %systemd_preun libvirtd.service virtlockd.socket virtlockd.service + %systemd_preun \ + libvirtd.socket \ + libvirtd.service \ + virtlockd.socket \ + virtlockd.service %else if [ $1 -eq 0 ] ; then # Package removal, not upgrade - /bin/systemctl --no-reload disable libvirtd.service virtlockd.socket virtlockd.service > /dev/null 2>&1 || : - /bin/systemctl stop libvirtd.service virtlockd.socket virtlockd.service > /dev/null 2>&1 || : + /bin/systemctl --no-reload disable \ + libvirtd.socket \ + libvirtd.service \ + virtlockd.socket \ + virtlockd.service > /dev/null 2>&1 || : + /bin/systemctl stop \ + libvirtd.socket \ + libvirtd.service \ + virtlockd.socket \ + virtlockd.service > /dev/null 2>&1 || : fi %endif %else @@ -1856,6 +1871,7 @@ exit 0 %if %{with_systemd} %{_unitdir}/libvirtd.service +%{_unitdir}/libvirtd.socket %{_unitdir}/virtlockd.service %{_unitdir}/virtlockd.socket %else -- 2.0.2

On Wed, Aug 13, 2014 at 04:10:12PM +0100, Daniel P. Berrange wrote: [I'm starting to regret that I wanted to fix some simple timeout-before-error issue :)] I can add the comment, but I just realized that we can't ship it this way. If someone has no authentication set up and the socket allowed only for root (for example), the machine would be vulnerable after update to the version with this libvirtd.socket. If, on the other hand, we put here 0700 for example, lot of applications may stop working, because they rely on authentication with 0777. And the daemon can do chmod() on the socket *only* to more lax permissions (not the other way around, as it would result in the same problem why we needed to add the comment in the first place). The solutions I came up with are: - Have SocketMode=0700 and do chmod() in the daemon to adjust the mode to permissions from the config file. And the better one: - Drop this whole socket starting stuff, because if there's a race, it's a systemd's problem. We call sd_notify(0, "READY=1") when everything is set up as systemd wants us to! I just discovered that now. Martin

On Thu, Aug 14, 2014 at 10:07:34AM +0100, Daniel P. Berrange wrote: I still don't see what would be the added value, but it certainly is. I'll do that (with the added comment) in next version. Martin