nspawn: socket activation fixes

When nspawn is socket activated, it passes the file descriptors and sets
LISTEN_FDS and LISTEN_PID. However it would set the LISTEN_PID to 1 when
--as-pid2 was passed, so a program would not properly accept the file
descriptors, and LISTEN_{FDNAMES,PIDFDID} were not set.

Fixes https://github.com/systemd/systemd/issues/43764

src/nspawn/nspawn.c

@@ -102,6 +102,7 @@
 #include "parse-util.h"
 #include "path-lookup.h"
 #include "path-util.h"
+#include "pidfd-util.h"
 #include "pidref.h"
 #include "polkit-agent.h"
 #include "pretty-print.h"
@@ -3303,6 +3304,7 @@ static int inner_child(
                 Barrier *barrier,
                 int fd_inner_socket,
                 FDSet *fds,
+                char **fdnames,
                 char **os_release_pairs) {
 
         _cleanup_free_ char *home = NULL;
@@ -3318,7 +3320,7 @@ static int inner_child(
                 NULL, /* LOGNAME */
                 NULL, /* container_uuid */
                 NULL, /* LISTEN_FDS */
-                NULL, /* LISTEN_PID */
+                NULL, /* LISTEN_FDNAMES */
                 NULL, /* NOTIFY_SOCKET */
                 NULL, /* CREDENTIALS_DIRECTORY */
                 NULL, /* LANG */
@@ -3592,9 +3594,17 @@ static int inner_child(
                 if (r < 0)
                         return log_error_errno(r, "Failed to unset O_CLOEXEC for file descriptors.");
 
-                if ((asprintf(envp + n_env++, "LISTEN_FDS=%u", fdset_size(fds)) < 0) ||
-                    (asprintf(envp + n_env++, "LISTEN_PID=1") < 0))
+                if (asprintf(envp + n_env++, "LISTEN_FDS=%u", fdset_size(fds)) < 0)
                         return log_oom();
+
+                if (fdnames) {
+                        _cleanup_free_ char *joined = strv_join(fdnames, ":");
+                        if (!joined)
+                                return log_oom();
+
+                        if (asprintf(envp + n_env++, "LISTEN_FDNAMES=%s", joined) < 0)
+                                return log_oom();
+                }
         }
         if (asprintf(envp + n_env++, "NOTIFY_SOCKET=%s", NSPAWN_NOTIFY_SOCKET_PATH) < 0)
                 return log_oom();
@@ -3634,6 +3644,23 @@ static int inner_child(
                         return r;
         }
 
+        /* Now that we know which process is going to execute the payload, set $LISTEN_PID and
+         * $LISTEN_PIDFDID. If $LISTEN_PID is overridden via --setenv=, leave $LISTEN_PIDFDID alone too, as
+         * it would refer to a different process than $LISTEN_PID then. */
+        if (!fdset_isempty(fds) && !strv_env_get(arg_setenv, "LISTEN_PID")) {
+                r = strv_env_assignf(&env_use, "LISTEN_PID", PID_FMT, getpid_cached());
+                if (r < 0)
+                        return log_error_errno(r, "Failed to set $LISTEN_PID: %m");
+
+                uint64_t pidfdid;
+                if (!strv_env_get(arg_setenv, "LISTEN_PIDFDID") &&
+                    pidfd_get_inode_id_self_cached(&pidfdid) >= 0) {
+                        r = strv_env_assignf(&env_use, "LISTEN_PIDFDID", "%" PRIu64, pidfdid);
+                        if (r < 0)
+                                return log_error_errno(r, "Failed to set $LISTEN_PIDFDID: %m");
+                }
+        }
+
         if (arg_console_mode != CONSOLE_PIPE) {
                 /* So far our pty wasn't controlled by any process. Finally, it's time to change that, if we
                  * are configured for that. Acquire it as controlling tty. */
@@ -3891,6 +3918,7 @@ static int outer_child(
                 int fd_outer_socket,
                 int fd_inner_socket,
                 FDSet *fds,
+                char **fdnames,
                 int netns_fd,
                 const char *unix_export_path) {
 
@@ -4459,7 +4487,7 @@ static int outer_child(
                                 return log_error_errno(r, "Failed to move root directory: %m");
                 }
 
-                r = inner_child(barrier, fd_inner_socket, fds, os_release_pairs);
+                r = inner_child(barrier, fd_inner_socket, fds, fdnames, os_release_pairs);
                 if (r < 0)
                         _exit(EXIT_FAILURE);
 
@@ -4588,18 +4616,23 @@ static int setup_uid_map(
         return 0;
 }
 
+static bool fd_store_forwarding_enabled(void) {
+        /* We can only forward fd-store related messages if we have an upstream supervisor (NOTIFY_SOCKET)
+         * with an fd store available (FDSTORE). */
+        return getenv("NOTIFY_SOCKET") && fdstore_detected();
+}
+
 static int forward_fd_store(char **tags, FDSet *fds) {
         int r;
 
         /* Forward fd-store related messages to our own service manager, so that file descriptors stored
-         * by the inner payload propagate up the chain and are preserved across restarts. Skip entirely
-         * if we have no upstream supervisor (no NOTIFY_SOCKET) or no fd store available (no FDSTORE).
+         * by the inner payload propagate up the chain and are preserved across restarts.
          *
          * Forwarded entries are namespaced with a "payload-" prefix on their FDNAME so that they
          * cannot collide with fd-store entries that nspawn itself might want to push to its own
          * upstream supervisor (the container payload and nspawn share a single upstream fdstore
          * namespace, since there's only one init system per container). */
-        if (!getenv("NOTIFY_SOCKET") || !fdstore_detected())
+        if (!fd_store_forwarding_enabled())
                 return 0;
 
         const char *pattern = strv_find_startswith(tags, "FDSTOREWIPE=");
@@ -5330,6 +5363,7 @@ static int run_container(
                 MStack *mstack,
                 int userns_fd,
                 FDSet *fds,
+                char **fdnames,
                 char veth_name[IFNAMSIZ],
                 bool *veth_created,
                 struct ExposeArgs *expose_args,
@@ -5491,6 +5525,7 @@ static int run_container(
                                 fd_outer_socket_pair[1],
                                 fd_inner_socket_pair[1],
                                 fds,
+                                fdnames,
                                 child_netns_fd,
                                 unix_export_host_dir);
                 if (r < 0)
@@ -6200,10 +6235,70 @@ static int do_cleanup(void) {
         return 0;
 }
 
+static int acquire_listen_fdnames(FDSet *fds, char ***ret) {
+        _cleanup_strv_free_ char **l = NULL;
+        const char *e;
+        int r;
+
+        assert(ret);
+
+        e = getenv("LISTEN_FDNAMES");
+        if (!e || fdset_isempty(fds)) {
+                *ret = NULL;
+                return 0;
+        }
+
+        /* Split the same way sd_listen_fds_with_names() does, so that we end up with the same names. */
+        r = strv_split_full(&l, e, ":", EXTRACT_DONT_COALESCE_SEPARATORS);
+        if (r == -ENOMEM)
+                return log_oom();
+        if (r < 0) {
+                log_warning_errno(r, "Failed to parse $LISTEN_FDNAMES, ignoring: %m");
+                *ret = NULL;
+                return 0;
+        }
+
+        if ((size_t) r != fdset_size(fds)) {
+                log_warning("Number of entries in $LISTEN_FDNAMES (%i) doesn't match number of passed file descriptors (%u), ignoring.",
+                            r, fdset_size(fds));
+                *ret = NULL;
+                return 0;
+        }
+
+        /* forward_fd_store() prefixes the names of the payload's fd store entries with "payload-" when
+         * pushing them upstream. Strip it again when they are passed back to us, so that the payload gets
+         * them back under the name it stored them with. Leave the names alone if we wouldn't forward fd
+         * store entries, as they can't have come from the payload then. */
+        bool strip_prefix = fd_store_forwarding_enabled();
+
+        STRV_FOREACH(n, l) {
+                if (!fdname_is_valid(*n)) {
+                        _cleanup_free_ char *esc = cescape(*n);
+                        log_warning("$LISTEN_FDNAMES contains invalid file descriptor name '%s', ignoring.", strna(esc));
+                        *ret = NULL;
+                        return 0;
+                }
+
+                const char *name = (strip_prefix ? startswith(*n, "payload-") : NULL) ?: *n;
+
+                /* The payload's sd_listen_fds_with_names() will unescape the names again, hence escape
+                 * them here. */
+                char *esc = shell_escape(name, ":");
+                if (!esc)
+                        return log_oom();
+
+                free_and_replace(*n, esc);
+        }
+
+        *ret = TAKE_PTR(l);
+        return 0;
+}
+
 static int run(int argc, char *argv[]) {
         bool remove_image = false, veth_created = false;
         _cleanup_close_ int master = -EBADF, userns_fd = -EBADF, mount_fd = -EBADF;
         _cleanup_fdset_free_ FDSet *fds = NULL;
+        _cleanup_strv_free_ char **fdnames = NULL;
         int r, ret = EXIT_SUCCESS;
         char veth_name[IFNAMSIZ] = "";
         struct ExposeArgs expose_args = {};
@@ -6316,6 +6411,10 @@ static int run(int argc, char *argv[]) {
                 goto finish;
         }
 
+        r = acquire_listen_fdnames(fds, &fdnames);
+        if (r < 0)
+                goto finish;
+
         /* The "default" umask. This is appropriate for most file and directory
         * operations performed by nspawn, and is the umask that will be used for
         * the child. Functions like copy_devnodes() change the umask temporarily. */
@@ -6841,6 +6940,7 @@ static int run(int argc, char *argv[]) {
                                 mstack,
                                 userns_fd,
                                 fds,
+                                fdnames,
                                 veth_name,
                                 &veth_created,
                                 &expose_args, &master,

src/test/test-fdstore.c

@@ -2,8 +2,10 @@
 
 /* In 'store' mode pushes a couple of memfds with known content into the supervisor's fd store via FDSTORE=1
  * sd_notify() messages. In 'check' mode reads back the fds passed via LISTEN_FDS and verifies the content
- * matches what was pushed. In 'wipe[=GLOB]' mode removes all or glob-matched stored fds.
- * Multiple modes may be run in sequence.
+ * matches what was pushed, and that they are passed back under the names they were pushed with. In
+ * 'wipe[=GLOB]' mode removes all or glob-matched stored fds. In 'listen' mode verifies that sd_listen_fds()
+ * accepts all fds passed via LISTEN_FDS, i.e. that LISTEN_PID and LISTEN_PIDFDID refer to us. In 'exit' mode
+ * exits immediately instead of staying alive. Multiple modes may be run in sequence.
  *
  * This binary is intentionally linked against libsystemd only so that it can go in the minimal image. */
 
@@ -17,6 +19,8 @@
 
 #include "sd-daemon.h"
 
+#define NAME_A "test-fd-a"
+#define NAME_B "test-fd-b"
 #define DATA_A "fdstore-data-a"
 #define DATA_B "fdstore-data-b"
 
@@ -30,6 +34,15 @@ static void closep(int *fd) {
         *fd = -EBADF;
 }
 
+static void strv_freep(char ***l) {
+        if (!l || !*l)
+                return;
+
+        for (char **i = *l; *i; i++)
+                free(*i);
+        free(*l);
+}
+
 static _Noreturn void exit_handler(int sig) {
         _exit(EXIT_SUCCESS);
 }
@@ -79,10 +92,10 @@ static int push_one(const char *fdname, const char *content) {
 static int do_store(void) {
         int r;
 
-        if (push_one("test-fd-a", DATA_A) < 0)
+        if (push_one(NAME_A, DATA_A) < 0)
                 return EXIT_FAILURE;
 
-        if (push_one("test-fd-b", DATA_B) < 0)
+        if (push_one(NAME_B, DATA_B) < 0)
                 return EXIT_FAILURE;
 
         /* Wait for our supervisor to actually process the FDSTORE messages before we exit, otherwise
@@ -98,13 +111,14 @@ static int do_store(void) {
 }
 
 static int do_check(void) {
+        _cleanup_(strv_freep) char **names = NULL;
         bool seen_a = false, seen_b = false;
         int n;
 
-        n = sd_listen_fds(/* unset_environment= */ 0);
+        n = sd_listen_fds_with_names(/* unset_environment= */ 0, &names);
         if (n < 0) {
                 errno = -n;
-                fprintf(stderr, "sd_listen_fds failed: %m\n");
+                fprintf(stderr, "sd_listen_fds_with_names failed: %m\n");
                 return EXIT_FAILURE;
         }
         if (n < 2) {
@@ -128,12 +142,23 @@ static int do_check(void) {
                 }
                 buf[k] = 0;
 
-                if (strcmp(buf, DATA_A) == 0)
+                const char *expected_name;
+                if (strcmp(buf, DATA_A) == 0) {
                         seen_a = true;
-                else if (strcmp(buf, DATA_B) == 0)
+                        expected_name = NAME_A;
+                } else if (strcmp(buf, DATA_B) == 0) {
                         seen_b = true;
-                else
+                        expected_name = NAME_B;
+                } else {
                         fprintf(stderr, "Unexpected fd content: '%s'\n", buf);
+                        continue;
+                }
+
+                if (strcmp(names[i], expected_name) != 0) {
+                        fprintf(stderr, "Unexpected name '%s' for fd with content '%s', expected '%s'\n",
+                                names[i], buf, expected_name);
+                        return EXIT_FAILURE;
+                }
         }
 
         if (!seen_a || !seen_b) {
@@ -141,7 +166,42 @@ static int do_check(void) {
                 return EXIT_FAILURE;
         }
 
-        printf("Payload received both preserved fds with matching content.\n");
+        printf("Payload received both preserved fds with matching content and names.\n");
+        return EXIT_SUCCESS;
+}
+
+static int do_listen(void) {
+        const char *e;
+        char *end;
+        long m;
+        int n;
+
+        e = getenv("LISTEN_FDS");
+        if (!e) {
+                fprintf(stderr, "$LISTEN_FDS not set\n");
+                return EXIT_FAILURE;
+        }
+
+        errno = 0;
+        m = strtol(e, &end, 10);
+        if (errno != 0 || end == e || *end != 0 || m <= 0) {
+                fprintf(stderr, "Invalid $LISTEN_FDS: '%s'\n", e);
+                return EXIT_FAILURE;
+        }
+
+        /* sd_listen_fds() ignores $LISTEN_FDS if $LISTEN_PID or $LISTEN_PIDFDID refer to another process */
+        n = sd_listen_fds(/* unset_environment= */ 0);
+        if (n < 0) {
+                errno = -n;
+                fprintf(stderr, "sd_listen_fds failed: %m\n");
+                return EXIT_FAILURE;
+        }
+        if (n != m) {
+                fprintf(stderr, "sd_listen_fds returned %d, but $LISTEN_FDS=%s\n", n, e);
+                return EXIT_FAILURE;
+        }
+
+        printf("sd_listen_fds accepted all %d fds.\n", n);
         return EXIT_SUCCESS;
 }
 
@@ -166,7 +226,7 @@ int main(int argc, char *argv[]) {
         int r;
 
         if (argc < 2) {
-                fprintf(stderr, "Usage: %s {store|check|wipe[=GLOB]}...\n", argv[0]);
+                fprintf(stderr, "Usage: %s {store|check|listen|wipe[=GLOB]|exit}...\n", argv[0]);
                 return EXIT_FAILURE;
         }
 
@@ -175,6 +235,10 @@ int main(int argc, char *argv[]) {
                         r = do_store();
                 else if (strcmp(argv[i], "check") == 0)
                         r = do_check();
+                else if (strcmp(argv[i], "listen") == 0)
+                        r = do_listen();
+                else if (strcmp(argv[i], "exit") == 0)
+                        return EXIT_SUCCESS;
                 else if (strcmp(argv[i], "wipe") == 0)
                         r = notify("FDSTOREWIPE", "*");
                 else if (strncmp(argv[i], "wipe=", strlen("wipe=")) == 0)

test/units/TEST-13-NSPAWN.nspawn.sh

@@ -1178,6 +1178,115 @@ EOF
     rm -fr "$root"
 }
 
+testcase_socket_activation() {
+    # Verify that the socket activation environment variables are properly propagated into the
+    # container, i.e. that $LISTEN_FDNAMES survives, and that $LISTEN_PID refers to the process that
+    # actually ends up running the payload (PID 2 when --as-pid2 is used).
+    # https://github.com/systemd/systemd/issues/43764
+    local root entrypoint socket1 socket2 nspawn
+
+    root="$(mktemp -d /var/lib/machines/TEST-13-NSPAWN.socket-activation.XXX)"
+    create_dummy_container "$root"
+    entrypoint="$root/entrypoint.sh"
+    socket1="/run/TEST-13-NSPAWN-socket-activation-1.sock"
+    socket2="/run/TEST-13-NSPAWN-socket-activation-2.sock"
+    rm -f "$socket1" "$socket2"
+
+    cat >"$entrypoint" <<\EOF
+#!/usr/bin/env bash
+set -ex
+
+env | grep '^LISTEN_'
+test "$LISTEN_FDS" = "${EXPECTED_FDS:-1}"
+test "${LISTEN_FDNAMES-unset}" = "$EXPECTED_FDNAMES"
+if [[ -v EXPECTED_PID ]]; then
+    test "$LISTEN_PID" = "$EXPECTED_PID"
+    # $LISTEN_PIDFDID must not refer to a different process than $LISTEN_PID
+    test "${LISTEN_PIDFDID-unset}" = "unset"
+else
+    test "$LISTEN_PID" = "$$"
+fi
+for ((fd = 3; fd < 3 + LISTEN_FDS; fd++)); do
+    test -S "/proc/self/fd/$fd"
+done
+if [[ ! -v EXPECTED_PID ]]; then
+    # sd_listen_fds() only accepts the fds if both $LISTEN_PID and $LISTEN_PIDFDID refer to the calling
+    # process. exec keeps both our PID and our pidfd inode ID.
+    exec /usr/bin/test-fdstore listen exit
+fi
+EOF
+    chmod +x "$entrypoint"
+
+    nspawn=(systemd-nspawn --register=no --directory="$root")
+
+    # Without --as-pid2 the payload is PID 1 in the container...
+    systemd-socket-activate --listen="$socket1" --fdname=hoge --now -- \
+        "${nspawn[@]}" --setenv=EXPECTED_FDNAMES=hoge "${entrypoint##"$root"}"
+    rm -f "$socket1"
+
+    # ...and with --as-pid2 it's PID 2, as the stub PID 1 forks it off.
+    systemd-socket-activate --listen="$socket1" --fdname=hoge --now -- \
+        "${nspawn[@]}" --as-pid2 --setenv=EXPECTED_FDNAMES=hoge "${entrypoint##"$root"}"
+    rm -f "$socket1"
+
+    # Multiple file descriptors are passed on in order
+    systemd-socket-activate --listen="$socket1" --listen="$socket2" --fdname=hoge --fdname=fuga --now -- \
+        "${nspawn[@]}" --as-pid2 --setenv=EXPECTED_FDS=2 --setenv=EXPECTED_FDNAMES=hoge:fuga \
+                       "${entrypoint##"$root"}"
+    rm -f "$socket1" "$socket2"
+
+    # Escape sequences in $LISTEN_FDNAMES are resolved the same way sd_listen_fds_with_names() does, and the
+    # names are escaped again when passed on, so that the payload ends up with the same names
+    systemd-socket-activate --listen="$socket1" --listen="$socket2" \
+                            --fdname='ho\ge' --fdname='fu\\ga' --now -- \
+        "${nspawn[@]}" --as-pid2 --setenv=EXPECTED_FDS=2 --setenv='EXPECTED_FDNAMES=hoge:fu\\ga' \
+                       "${entrypoint##"$root"}"
+    rm -f "$socket1" "$socket2"
+
+    # The prefix nspawn adds to the names of fd store entries it forwards upstream is stripped again, but
+    # only if nspawn actually forwards fd store entries, i.e. it has an upstream fd store
+    systemd-socket-activate --listen="$socket1" --fdname=payload-hoge --now -- \
+        "${nspawn[@]}" --as-pid2 --setenv=EXPECTED_FDNAMES=payload-hoge "${entrypoint##"$root"}"
+    rm -f "$socket1"
+    systemd-run --wait --pipe -p Type=notify -p NotifyAccess=all -p FileDescriptorStoreMax=1 -- \
+        bash -c 'exec systemd-socket-activate -E NOTIFY_SOCKET -E FDSTORE "$@"' _ \
+            --listen="$socket1" --fdname=payload-hoge --now -- \
+            "${nspawn[@]}" --keep-unit --as-pid2 --setenv=EXPECTED_FDNAMES=hoge "${entrypoint##"$root"}"
+    rm -f "$socket1"
+
+    # $LISTEN_FDNAMES is not passed on if it is not set, ...
+    systemd-socket-activate --listen="$socket1" --now -- \
+        "${nspawn[@]}" --as-pid2 --setenv=EXPECTED_FDNAMES=unset "${entrypoint##"$root"}"
+    rm -f "$socket1"
+
+    # ...doesn't match the number of file descriptors, ...
+    systemd-socket-activate --listen="$socket1" --fdname=hoge --fdname=fuga --now -- \
+        "${nspawn[@]}" --as-pid2 --setenv=EXPECTED_FDNAMES=unset "${entrypoint##"$root"}"
+    rm -f "$socket1"
+
+    # ...cannot be parsed, ...
+    # shellcheck disable=SC1003
+    systemd-socket-activate --listen="$socket1" --fdname='hoge\' --now -- \
+        "${nspawn[@]}" --as-pid2 --setenv=EXPECTED_FDNAMES=unset "${entrypoint##"$root"}"
+    rm -f "$socket1"
+
+    # ...or contains invalid names
+    systemd-socket-activate --listen="$socket1" --fdname=$'ho\tge' --now -- \
+        "${nspawn[@]}" --as-pid2 --setenv=EXPECTED_FDNAMES=unset "${entrypoint##"$root"}"
+    rm -f "$socket1"
+
+    # An explicit --setenv= takes precedence over the socket activation variables, and $LISTEN_PIDFDID is
+    # not set if $LISTEN_PID is overridden
+    systemd-socket-activate --listen="$socket1" --fdname=hoge --now -- \
+        "${nspawn[@]}" --as-pid2 \
+                       --setenv=LISTEN_FDNAMES=override --setenv=EXPECTED_FDNAMES=override \
+                       --setenv=LISTEN_PID=4711 --setenv=EXPECTED_PID=4711 \
+                       "${entrypoint##"$root"}"
+    rm -f "$socket1"
+
+    rm -fr "$root"
+}
+
 testcase_os_release() {
     local root entrypoint os_release_source