On Wed, Aug 13, 2014 at 03:54:02PM +0100, Daniel P. Berrange wrote:

On Wed, Jul 23, 2014 at 04:27:07PM +0200, Martin Kletzander wrote:

...

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/locking/lock_daemon.c | 2 +- src/rpc/virnetserverservice.c | 5 +++++ src/rpc/virnetserverservice.h | 1 + 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index e9219d5..02d77e3 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -614,7 +614,7 @@ virLockDaemonSetupNetworkingSystemD(virNetServerPtr srv) #if WITH_GNUTLS NULL, #endif - false, 1))) + false, 0, 1))) return -1;

if (virNetServerAddService(srv, svc, NULL) < 0) { diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index e85889b..fea05c3 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -133,6 +133,7 @@ virNetServerServiceNewFDOrUNIX(const char *path, tls, #endif readonly, + max_queued_clients, nrequests_client_max); } } @@ -265,6 +266,7 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd, virNetTLSContextPtr tls, #endif bool readonly, + size_t max_queued_clients, size_t nrequests_client_max) { virNetServerServicePtr svc; @@ -292,6 +294,9 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd, goto error;

for (i = 0; i < svc->nsocks; i++) { + if (virNetSocketListen(svc->socks[i], max_queued_clients) < 0) + goto error;

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.

Yes, it does, but we call listen() here to adjust the backlog value. It still works, no clients get disconnected, but kernel knows what backlog we want from now on. Because of this and other settings that must be done by systemd (or virsh), there is a new comment in the configuration file saying that systemd should be set the same way libvirt is (socket permissions, backlog, etc.)

Regards, Daniel

On Wed, Aug 13, 2014 at 04:01:42PM +0100, Daniel P. Berrange wrote:

On Wed, Jul 23, 2014 at 04:27:08PM +0200, Martin Kletzander wrote:

...

First FD is the RW unix socket to listen on, second one (if applicable) is the RO unix socket.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- daemon/libvirtd.c | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 4c926b3..15f0ce2 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -56,6 +56,7 @@ #include "virstring.h" #include "locking/lock_manager.h" #include "viraccessmanager.h" +#include "virutil.h"

#ifdef WITH_DRIVER_MODULES # include "driver.h" @@ -476,11 +477,19 @@ static int daemonSetupNetworking(virNetServerPtr srv, int unix_sock_ro_mask = 0; int unix_sock_rw_mask = 0;

+ unsigned int cur_fd = STDERR_FILENO + 1; + unsigned int nfds = virGetListenFDs(); + if (config->unix_sock_group) { if (virGetGroupID(config->unix_sock_group, &unix_sock_gid) < 0) return -1; }

+ if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) { + VIR_ERROR(_("Too many (%u) FDs passed from caller"), nfds); + return -1; + } + if (virStrToLong_i(config->unix_sock_ro_perms, NULL, 8, &unix_sock_ro_mask) != 0) { VIR_ERROR(_("Failed to parse mode '%s'"), config->unix_sock_ro_perms); goto error; @@ -491,30 +500,30 @@ static int daemonSetupNetworking(virNetServerPtr srv, goto error; }

- VIR_DEBUG("Registering unix socket %s", sock_path); - if (!(svc = virNetServerServiceNewUNIX(sock_path, - unix_sock_rw_mask, - unix_sock_gid, - config->auth_unix_rw, + if (!(svc = virNetServerServiceNewFDOrUNIX(sock_path, + unix_sock_rw_mask, + unix_sock_gid, + config->auth_unix_rw, #if WITH_GNUTLS - NULL, + NULL, #endif - false, - config->max_queued_clients, - config->max_client_requests))) + false, + config->max_queued_clients, + config->max_client_requests, + nfds, &cur_fd))) goto error; if (sock_path_ro) { - VIR_DEBUG("Registering unix socket %s", sock_path_ro); - if (!(svcRO = virNetServerServiceNewUNIX(sock_path_ro, - unix_sock_ro_mask, - unix_sock_gid, - config->auth_unix_ro, + if (!(svcRO = virNetServerServiceNewFDOrUNIX(sock_path_ro, + unix_sock_ro_mask, + unix_sock_gid, + config->auth_unix_ro, #if WITH_GNUTLS - NULL, + NULL, #endif - true, - config->max_queued_clients, - config->max_client_requests))) + true, + config->max_queued_clients, + config->max_client_requests, + nfds, &cur_fd))) goto error; }

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.

Is this really necessary? I mean I went with the approach that we can get more than 1 FD passed even though it's not needed to fix the 20s timeout (the original bug). And if we decide we need more it can be reworked that way, yes. But right now I really don't think we do, especially since race condition solved by passing TCP or TLS ports would be *really* rare; and anyone connecting during the time system daemon is starting will just try again in a while.

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

I don't think right now this approach would add as much benefit as how much code it would add (and with this potential problems, too). I think that from the .service file it is pretty obvious that we just support passing FDs for the sockets. Martin

On Wed, Jul 23, 2014 at 04:27:12PM +0200, Martin Kletzander wrote:

This eliminates the need for active waiting.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- 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)

s/int/bool/ perhaps ?

{ 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; + }

Perhaps I'm miss-reading the code, but isn't this block gonig to result in failure if libvirtd is already running (& listening on the socket we try to bind to) and we requested auto-spawn ?

+ } + + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + virReportSystemError(errno, _("Failed to connect socket to '%s'"), path); goto error; }

Also I think there's still a race here because the already running libvirtd daemon could be unfortunately shutting down right at the same time we try to connect. So I think we still need to have a re-try loop around the connect attempt to address that race. Obviously it should be pretty damn rare now so won't have the problems that the current loop has.

+ if (spawnDaemon && virNetSocketForkDaemon(binary, passfd) < 0) + goto error; +

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 Thu, Aug 14, 2014 at 11:23:27AM +0200, Martin Kletzander wrote:

On Wed, Aug 13, 2014 at 04:09:09PM +0100, Daniel P. Berrange wrote:

...

On Wed, Jul 23, 2014 at 04:27:12PM +0200, Martin Kletzander wrote:

...

This eliminates the need for active waiting.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- 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)

s/int/bool/ perhaps ?

Which one you mean? I'm keeping the return value as it was and the passfd is the fd that will be passed.

Never mind, I mis-interpreted 'passfd' as an instruction not a value.

[...]

...

...

@@ -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; + }

Perhaps I'm miss-reading the code, but isn't this block gonig to result in failure if libvirtd is already running (& listening on the socket we try to bind to) and we requested auto-spawn ?

...

+ } + + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + virReportSystemError(errno, _("Failed to connect socket to '%s'"), path); goto error; }

Also I think there's still a race here because the already running libvirtd daemon could be unfortunately shutting down right at the same time we try to connect. So I think we still need to have a re-try loop around the connect attempt to address that race. Obviously it should be pretty damn rare now so won't have the problems that the current loop has.

Wouldn't connect(), bind(), connect() be enough (for both issues)? Or do we need to try the dance few times again?

We need to retry the whole block including daemon spawn to be able to handle it if the existing daemon is in the process of shutting down I believe. 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 Thu, Aug 14, 2014 at 01:02:28PM +0200, Martin Kletzander wrote:

On Thu, Aug 14, 2014 at 10:26:41AM +0100, Daniel P. Berrange wrote:

...

On Thu, Aug 14, 2014 at 11:23:27AM +0200, Martin Kletzander wrote:

...

On Wed, Aug 13, 2014 at 04:09:09PM +0100, Daniel P. Berrange wrote:

...

On Wed, Jul 23, 2014 at 04:27:12PM +0200, Martin Kletzander wrote:

...

This eliminates the need for active waiting.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetsocket.c | 102 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 83 insertions(+), 19 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c [...] @@ -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; + }

Perhaps I'm miss-reading the code, but isn't this block gonig to result in failure if libvirtd is already running (& listening on the socket we try to bind to) and we requested auto-spawn ?

...

+ } + + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + virReportSystemError(errno, _("Failed to connect socket to '%s'"), path); goto error; }

Also I think there's still a race here because the already running libvirtd daemon could be unfortunately shutting down right at the same time we try to connect. So I think we still need to have a re-try loop around the connect attempt to address that race. Obviously it should be pretty damn rare now so won't have the problems that the current loop has.

Wouldn't connect(), bind(), connect() be enough (for both issues)? Or do we need to try the dance few times again?

We need to retry the whole block including daemon spawn to be able to handle it if the existing daemon is in the process of shutting down I believe.

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) {

It is a bit hard to follow the diff-upon-diff :-) IIUC the logic is 1. Try to connect() ...fails because no daemon is running... 2. fd = socket() 3. bind(fd) 4. connect(fd) 5. spawn daemon with fd What I'm unclear on is what happens if 2 separate processes are runing steps 2/3/4 in parallel. IIUC with the changes here one process is going to succeeed with bind() and the other process will get a failure. That other process getting a failure should go back to step 1 and try to connect to the socket that the first process successsfully bound to. I don't think your code is handling that scenario, which I think requires at least a single retry loop. 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:13PM +0200, Martin Kletzander wrote:

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- .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

Perhaps add a comment in this file about Mode=0777 *only* being safe if you have libvirtd.conf doing authentication (eg polkit) on both UNIX sockets. ACK to the chagne though 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 Thu, Aug 14, 2014 at 10:38:42AM +0200, Martin Kletzander wrote:

On Wed, Aug 13, 2014 at 04:10:12PM +0100, Daniel P. Berrange wrote:

...

On Wed, Jul 23, 2014 at 04:27:13PM +0200, Martin Kletzander wrote:

...

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- .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

Perhaps add a comment in this file about Mode=0777 *only* being safe if you have libvirtd.conf doing authentication (eg polkit) on both UNIX sockets.

[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.

Actually there's a 3rd option - Don't run 'systemctl enable libvirtd.socket' That way we provide the ability to use it, but don't turn it on - people have to explicitly opt-in. 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 :|