Re: [Qemu-devel] [PATCH v2] qemu-ga: obey LISTEN_PID when using systemd socket activation

[Stefan Hajnoczi]

On Thu, Mar 16, 2017 at 05:36:40PM +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.

I intentionally wrote qga socket activation this way.  It allows socket
activation to work together with daemonization.  That combination is
probably not very useful so it's fine to get rid of it.

Please add an error message in qga/main.c if socket activation is used
in combination with the -d/--daemonize flag.

> Cc: "Richard W.M. Jones" <address@hidden>
> Cc: Stefan Hajnoczi <address@hidden>
> Reviewed-by: Daniel P. Berrange <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  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 <address@hidden>
> + *
> + * 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..e080fb7 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..d22e86c
> --- /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 <address@hidden>
> + *
> + * 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 */
> +unsigned int check_socket_activation(void)
> +{
> +    return 0;
> +}
> +#endif
> -- 
> 2.9.3
>

signature.asc
Description: PGP signature