On Thu, Mar 16, 2017 at 05:15:35PM +0100, Paolo Bonzini wrote: > qemu-ga's socket activation support was not obeying the LISTEN_PID > environment variable, which avoids that a process uses a socket-activation > file descriptor meant for its parent. > > Mess can for example ensue if a process forks a children before consuming > the socket-activation file descriptor and therefore setting O_CLOEXEC > on it. > > Luckily, qemu-nbd also got socket activation code, and its copy does > support LISTEN_PID. Some extra fixups are needed to ensure that the > code can be used for both, but that's what this patch does. The > main change is to replace get_listen_fds's "consume" argument with > the FIRST_SOCKET_ACTIVATION_FD macro from the qemu-nbd code. > > Cc: "Richard W.M. Jones" <rjo...@redhat.com> > Cc: Stefan Hajnoczi <stefa...@redhat.com> > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
Yes, it looks alright to me. Shame we can't support LISTEN_FDS > 1 :-( Reviewed-by: Richard W.M. Jones <rjo...@redhat.com> Rich. > include/qemu/systemd.h | 26 +++++++++++++ > qemu-nbd.c | 100 > ++++--------------------------------------------- > qga/main.c | 51 +++++++------------------ > util/Makefile.objs | 1 + > util/systemd.c | 77 +++++++++++++++++++++++++++++++++++++ > 5 files changed, 125 insertions(+), 130 deletions(-) > create mode 100644 include/qemu/systemd.h > create mode 100644 util/systemd.c > > diff --git a/include/qemu/systemd.h b/include/qemu/systemd.h > new file mode 100644 > index 0000000..e6a877e > --- /dev/null > +++ b/include/qemu/systemd.h > @@ -0,0 +1,26 @@ > +/* > + * systemd socket activation support > + * > + * Copyright 2017 Red Hat, Inc. and/or its affiliates > + * > + * Authors: > + * Richard W.M. Jones <rjo...@redhat.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + */ > + > +#ifndef QEMU_SYSTEMD_H > +#define QEMU_SYSTEMD_H 1 > + > +#define FIRST_SOCKET_ACTIVATION_FD 3 /* defined by systemd ABI */ > + > +/* > + * Check if socket activation was requested via use of the > + * LISTEN_FDS and LISTEN_PID environment variables. > + * > + * Returns 0 if no socket activation, or the number of FDs. > + */ > +unsigned int check_socket_activation(void); > + > +#endif > diff --git a/qemu-nbd.c b/qemu-nbd.c > index e4fede6..f332d30 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -28,6 +28,7 @@ > #include "qemu/config-file.h" > #include "qemu/bswap.h" > #include "qemu/log.h" > +#include "qemu/systemd.h" > #include "block/snapshot.h" > #include "qapi/util.h" > #include "qapi/qmp/qstring.h" > @@ -474,98 +475,6 @@ static void setup_address_and_port(const char **address, > const char **port) > } > } > > -#define FIRST_SOCKET_ACTIVATION_FD 3 /* defined by systemd ABI */ > - > -#ifndef _WIN32 > -/* > - * Check if socket activation was requested via use of the > - * LISTEN_FDS and LISTEN_PID environment variables. > - * > - * Returns 0 if no socket activation, or the number of FDs. > - */ > -static unsigned int check_socket_activation(void) > -{ > - const char *s; > - unsigned long pid; > - unsigned long nr_fds; > - unsigned int i; > - int fd; > - int err; > - > - s = getenv("LISTEN_PID"); > - if (s == NULL) { > - return 0; > - } > - err = qemu_strtoul(s, NULL, 10, &pid); > - if (err) { > - if (verbose) { > - fprintf(stderr, "malformed %s environment variable (ignored)\n", > - "LISTEN_PID"); > - } > - return 0; > - } > - if (pid != getpid()) { > - if (verbose) { > - fprintf(stderr, "%s was not for us (ignored)\n", > - "LISTEN_PID"); > - } > - return 0; > - } > - > - s = getenv("LISTEN_FDS"); > - if (s == NULL) { > - return 0; > - } > - err = qemu_strtoul(s, NULL, 10, &nr_fds); > - if (err) { > - if (verbose) { > - fprintf(stderr, "malformed %s environment variable (ignored)\n", > - "LISTEN_FDS"); > - } > - return 0; > - } > - assert(nr_fds <= UINT_MAX); > - > - /* A limitation of current qemu-nbd is that it can only listen on > - * a single socket. When that limitation is lifted, we can change > - * this function to allow LISTEN_FDS > 1, and remove the assertion > - * in the main function below. > - */ > - if (nr_fds > 1) { > - error_report("qemu-nbd does not support socket activation with %s > > 1", > - "LISTEN_FDS"); > - exit(EXIT_FAILURE); > - } > - > - /* So these are not passed to any child processes we might start. */ > - unsetenv("LISTEN_FDS"); > - unsetenv("LISTEN_PID"); > - > - /* So the file descriptors don't leak into child processes. */ > - for (i = 0; i < nr_fds; ++i) { > - fd = FIRST_SOCKET_ACTIVATION_FD + i; > - if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) { > - /* If we cannot set FD_CLOEXEC then it probably means the file > - * descriptor is invalid, so socket activation has gone wrong > - * and we should exit. > - */ > - error_report("Socket activation failed: " > - "invalid file descriptor fd = %d: %m", > - fd); > - exit(EXIT_FAILURE); > - } > - } > - > - return (unsigned int) nr_fds; > -} > - > -#else /* !_WIN32 */ > -static unsigned int check_socket_activation(void) > -{ > - return 0; > -} > -#endif > - > /* > * Check socket parameters compatibility when socket activation is used. > */ > @@ -892,6 +801,13 @@ int main(int argc, char **argv) > error_report("%s", err_msg); > exit(EXIT_FAILURE); > } > + > + /* qemu-nbd can only listen on a single socket. */ > + if (socket_activation > 1) { > + error_report("qemu-nbd does not support socket activation with > %s > 1", > + "LISTEN_FDS"); > + exit(EXIT_FAILURE); > + } > } > > if (tlscredsid) { > diff --git a/qga/main.c b/qga/main.c > index 92658bc..284dfbe 100644 > --- a/qga/main.c > +++ b/qga/main.c > @@ -29,6 +29,7 @@ > #include "qemu/bswap.h" > #include "qemu/help_option.h" > #include "qemu/sockets.h" > +#include "qemu/systemd.h" > #ifdef _WIN32 > #include "qga/service-win32.h" > #include "qga/vss-win32.h" > @@ -186,37 +187,6 @@ void reopen_fd_to_null(int fd) > } > #endif > > -/** > - * get_listen_fd: > - * @consume: true to prevent future calls from succeeding > - * > - * Fetch a listen file descriptor that was passed via systemd socket > - * activation. Use @consume to prevent child processes from thinking a file > - * descriptor was passed. > - * > - * Returns: file descriptor or -1 if no fd was passed > - */ > -static int get_listen_fd(bool consume) > -{ > -#ifdef _WIN32 > - return -1; /* no fd passing expected, unsetenv(3) not available */ > -#else > - const char *listen_fds = getenv("LISTEN_FDS"); > - int fd = STDERR_FILENO + 1; > - > - if (!listen_fds || strcmp(listen_fds, "1") != 0) { > - return -1; > - } > - > - if (consume) { > - unsetenv("LISTEN_FDS"); > - } > - > - qemu_set_cloexec(fd); > - return fd; > -#endif /* !_WIN32 */ > -} > - > static void usage(const char *cmd) > { > printf( > @@ -1251,7 +1221,7 @@ static bool check_is_frozen(GAState *s) > return false; > } > > -static int run_agent(GAState *s, GAConfig *config) > +static int run_agent(GAState *s, GAConfig *config, int socket_activation) > { > ga_state = s; > > @@ -1333,7 +1303,7 @@ static int run_agent(GAState *s, GAConfig *config) > s->main_loop = g_main_loop_new(NULL, false); > > if (!channel_init(ga_state, config->method, config->channel_path, > - get_listen_fd(true))) { > + socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1)) { > g_critical("failed to initialize guest agent channel"); > return EXIT_FAILURE; > } > @@ -1357,7 +1327,7 @@ int main(int argc, char **argv) > int ret = EXIT_SUCCESS; > GAState *s = g_new0(GAState, 1); > GAConfig *config = g_new0(GAConfig, 1); > - int listen_fd; > + int socket_activation; > > config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL; > > @@ -1379,8 +1349,13 @@ int main(int argc, char **argv) > config->method = g_strdup("virtio-serial"); > } > > - listen_fd = get_listen_fd(false); > - if (listen_fd >= 0) { > + socket_activation = check_socket_activation(); > + if (socket_activation > 1) { > + g_critical("qemu-ga only supports listening on one socket"); > + ret = EXIT_FAILURE; > + goto end; > + } > + if (socket_activation) { > SocketAddress *addr; > > g_free(config->method); > @@ -1388,7 +1363,7 @@ int main(int argc, char **argv) > config->method = NULL; > config->channel_path = NULL; > > - addr = socket_local_address(listen_fd, NULL); > + addr = socket_local_address(FIRST_SOCKET_ACTIVATION_FD , NULL); > if (addr) { > if (addr->type == SOCKET_ADDRESS_KIND_UNIX) { > config->method = g_strdup("unix-listen"); > @@ -1433,7 +1408,7 @@ int main(int argc, char **argv) > goto end; > } > > - ret = run_agent(s, config); > + ret = run_agent(s, config, socket_activation); > > end: > if (s->command_state) { > diff --git a/util/Makefile.objs b/util/Makefile.objs > index 06366b5..c6205eb 100644 > --- a/util/Makefile.objs > +++ b/util/Makefile.objs > @@ -42,3 +42,4 @@ util-obj-y += log.o > util-obj-y += qdist.o > util-obj-y += qht.o > util-obj-y += range.o > +util-obj-y += systemd.o > diff --git a/util/systemd.c b/util/systemd.c > new file mode 100644 > index 0000000..bf7a4ef > --- /dev/null > +++ b/util/systemd.c > @@ -0,0 +1,77 @@ > +/* > + * systemd socket activation support > + * > + * Copyright 2017 Red Hat, Inc. and/or its affiliates > + * > + * Authors: > + * Richard W.M. Jones <rjo...@redhat.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + */ > + > +#include "qemu/osdep.h" > +#include "qemu/systemd.h" > +#include "qemu/cutils.h" > +#include "qemu/error-report.h" > + > +#ifndef _WIN32 > +unsigned int check_socket_activation(void) > +{ > + const char *s; > + unsigned long pid; > + unsigned long nr_fds; > + unsigned int i; > + int fd; > + int err; > + > + s = getenv("LISTEN_PID"); > + if (s == NULL) { > + return 0; > + } > + err = qemu_strtoul(s, NULL, 10, &pid); > + if (err) { > + return 0; > + } > + if (pid != getpid()) { > + return 0; > + } > + > + s = getenv("LISTEN_FDS"); > + if (s == NULL) { > + return 0; > + } > + err = qemu_strtoul(s, NULL, 10, &nr_fds); > + if (err) { > + return 0; > + } > + assert(nr_fds <= UINT_MAX); > + > + /* So these are not passed to any child processes we might start. */ > + unsetenv("LISTEN_FDS"); > + unsetenv("LISTEN_PID"); > + > + /* So the file descriptors don't leak into child processes. */ > + for (i = 0; i < nr_fds; ++i) { > + fd = FIRST_SOCKET_ACTIVATION_FD + i; > + if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) { > + /* If we cannot set FD_CLOEXEC then it probably means the file > + * descriptor is invalid, so socket activation has gone wrong > + * and we should exit. > + */ > + error_report("Socket activation failed: " > + "invalid file descriptor fd = %d: %m", > + fd); > + exit(EXIT_FAILURE); > + } > + } > + > + return (unsigned int) nr_fds; > +} > + > +#else /* !_WIN32 */ > +static unsigned int check_socket_activation(void) > +{ > + return 0; > +} > +#endif > -- > 2.9.3 -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v