homed: port to notify_recv() + convert to PidRef (#36557)

Just some refactoring/modernization

src/basic/pidref.c

@@ -457,12 +457,36 @@ bool pidref_is_automatic(const PidRef *pidref) {
         return pidref && pid_is_automatic(pidref->pid);
 }
 
-static void pidref_hash_func(const PidRef *pidref, struct siphash *state) {
+void pidref_hash_func(const PidRef *pidref, struct siphash *state) {
         siphash24_compress_typesafe(pidref->pid, state);
 }
 
-static int pidref_compare_func(const PidRef *a, const PidRef *b) {
-        return CMP(a->pid, b->pid);
+int pidref_compare_func(const PidRef *a, const PidRef *b) {
+        int r;
+
+        assert(a);
+        assert(b);
+
+        r = CMP(pidref_is_set(a), pidref_is_set(b));
+        if (r != 0)
+                return r;
+
+        r = CMP(pidref_is_automatic(a), pidref_is_automatic(b));
+        if (r != 0)
+                return r;
+
+        r = CMP(pidref_is_remote(a), pidref_is_remote(b));
+        if (r != 0)
+                return r;
+
+        r = CMP(a->pid, b->pid);
+        if (r != 0)
+                return r;
+
+        if (a->fd_id != 0 && b->fd_id != 0)
+                return CMP(a->fd_id, b->fd_id);
+
+        return 0;
 }
 
 DEFINE_HASH_OPS(pidref_hash_ops, PidRef, pidref_hash_func, pidref_compare_func);

src/basic/pidref.h

@@ -108,5 +108,9 @@ int pidref_verify(const PidRef *pidref);
 
 #define TAKE_PIDREF(p) TAKE_GENERIC((p), PidRef, PIDREF_NULL)
 
+struct siphash;
+void pidref_hash_func(const PidRef *pidref, struct siphash *state);
+int pidref_compare_func(const PidRef *a, const PidRef *b);
+
 extern const struct hash_ops pidref_hash_ops;
 extern const struct hash_ops pidref_hash_ops_free; /* Has destructor call for pidref_free(), i.e. expects heap allocated PidRef as keys */

src/core/manager.c

@@ -2798,7 +2798,6 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t
         Manager *m = ASSERT_PTR(userdata);
         _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL;
         struct ucred ucred;
-        _cleanup_free_ char *buf = NULL;
         _cleanup_(fdset_free_asyncp) FDSet *fds = NULL;
         int r;
 
@@ -2809,7 +2808,8 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t
                 return 0;
         }
 
-        r = notify_recv_with_fds(m->notify_fd, &buf, &ucred, &pidref, &fds);
+        _cleanup_strv_free_ char **tags = NULL;
+        r = notify_recv_with_fds_strv(m->notify_fd, &tags, &ucred, &pidref, &fds);
         if (r == -EAGAIN)
                 return 0;
         if (r < 0)
@@ -2819,12 +2819,6 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t
                  * socket. */
                 return r;
 
-        _cleanup_strv_free_ char **tags = strv_split_newlines(buf);
-        if (!tags) {
-                log_oom_warning();
-                return 0;
-        }
-
         /* Possibly a barrier fd, let's see. */
         if (manager_process_barrier_fd(tags, fds)) {
                 log_debug("Received barrier notification message from PID " PID_FMT ".", pidref.pid);

src/home/homed-home.c

@@ -12,6 +12,7 @@
 #include "env-util.h"
 #include "errno-list.h"
 #include "errno-util.h"
+#include "event-util.h"
 #include "fd-util.h"
 #include "fileio.h"
 #include "filesystems.h"
@@ -153,6 +154,7 @@ int home_new(Manager *m, UserRecord *hr, const char *sysfs, Home **ret) {
                 .aliases = TAKE_PTR(aliases),
                 .uid = hr->uid,
                 .state = _HOME_STATE_INVALID,
+                .worker_pid = PIDREF_NULL,
                 .worker_stdout_fd = -EBADF,
                 .sysfs = TAKE_PTR(ns),
                 .signed_locally = -1,
@@ -224,8 +226,8 @@ Home *home_free(Home *h) {
                 if (h->sysfs)
                         (void) hashmap_remove_value(h->manager->homes_by_sysfs, h->sysfs, h);
 
-                if (h->worker_pid > 0)
-                        (void) hashmap_remove_value(h->manager->homes_by_worker_pid, PID_TO_PTR(h->worker_pid), h);
+                if (pidref_is_set(&h->worker_pid))
+                        (void) hashmap_remove_value(h->manager->homes_by_worker_pid, &h->worker_pid, h);
 
                 if (h->manager->gc_focus == h)
                         h->manager->gc_focus = NULL;
@@ -236,6 +238,7 @@ Home *home_free(Home *h) {
         user_record_unref(h->record);
         user_record_unref(h->secret);
 
+        pidref_done_sigkill_wait(&h->worker_pid);
         h->worker_event_source = sd_event_source_disable_unref(h->worker_event_source);
         safe_close(h->worker_stdout_fd);
         free(h->user_name);
@@ -1137,13 +1140,13 @@ static int home_on_worker_process(sd_event_source *s, const siginfo_t *si, void
         assert(s);
         assert(si);
 
-        assert(h->worker_pid == si->si_pid);
+        assert(h->worker_pid.pid == si->si_pid);
         assert(h->worker_event_source);
         assert(h->worker_stdout_fd >= 0);
 
-        (void) hashmap_remove_value(h->manager->homes_by_worker_pid, PID_TO_PTR(h->worker_pid), h);
+        (void) hashmap_remove_value(h->manager->homes_by_worker_pid, &h->worker_pid, h);
 
-        h->worker_pid = 0;
+        pidref_done(&h->worker_pid);
         h->worker_event_source = sd_event_source_disable_unref(h->worker_event_source);
 
         if (si->si_code != CLD_EXITED) {
@@ -1227,14 +1230,13 @@ static int home_start_work(
         _cleanup_(erase_and_freep) char *formatted = NULL;
         _cleanup_close_ int stdin_fd = -EBADF, stdout_fd = -EBADF;
         _cleanup_free_ int *blob_fds = NULL;
-        pid_t pid = 0;
         int r;
 
         assert(h);
         assert(verb);
         assert(hr);
 
-        if (h->worker_pid != 0)
+        if (pidref_is_set(&h->worker_pid))
                 return -EBUSY;
 
         assert(h->worker_stdout_fd < 0);
@@ -1299,7 +1301,8 @@ static int home_start_work(
         if (stdout_fd < 0)
                 return stdout_fd;
 
-        r = safe_fork_full("(sd-homework)",
+        _cleanup_(pidref_done_sigkill_wait) PidRef pid = PIDREF_NULL;
+        r = pidref_safe_fork_full("(sd-homework)",
                            (int[]) { stdin_fd, stdout_fd, STDERR_FILENO },
                            blob_fds, hashmap_size(blobs),
                            FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_CLOEXEC_OFF|FORK_PACK_FDS|FORK_DEATHSIG_SIGTERM|
@@ -1362,20 +1365,21 @@ static int home_start_work(
                 _exit(EXIT_FAILURE);
         }
 
-        r = sd_event_add_child(h->manager->event, &h->worker_event_source, pid, WEXITED, home_on_worker_process, h);
+        r = event_add_child_pidref(h->manager->event, &h->worker_event_source, &pid, WEXITED, home_on_worker_process, h);
         if (r < 0)
                 return r;
 
         (void) sd_event_source_set_description(h->worker_event_source, "worker");
 
-        r = hashmap_put(h->manager->homes_by_worker_pid, PID_TO_PTR(pid), h);
+        h->worker_pid = TAKE_PIDREF(pid);
+        r = hashmap_put(h->manager->homes_by_worker_pid, &h->worker_pid, h);
         if (r < 0) {
+                pidref_done_sigkill_wait(&h->worker_pid);
                 h->worker_event_source = sd_event_source_disable_unref(h->worker_event_source);
                 return r;
         }
 
         h->worker_stdout_fd = TAKE_FD(stdout_fd);
-        h->worker_pid = pid;
         h->worker_error_code = 0;
 
         return 0;
@@ -3278,19 +3282,19 @@ int home_wait_for_worker(Home *h) {
 
         assert(h);
 
-        if (h->worker_pid <= 0)
+        if (!pidref_is_set(&h->worker_pid))
                 return 0;
 
         log_info("Worker process for home %s is still running while exiting. Waiting for it to finish.", h->user_name);
 
-        r = wait_for_terminate_with_timeout(h->worker_pid, 30 * USEC_PER_SEC);
+        r = wait_for_terminate_with_timeout(h->worker_pid.pid, 30 * USEC_PER_SEC);
         if (r == -ETIMEDOUT)
                 log_warning_errno(r, "Waiting for worker process for home %s timed out. Ignoring.", h->user_name);
         else if (r < 0)
                 log_warning_errno(r, "Failed to wait for worker process for home %s. Ignoring.", h->user_name);
 
-        (void) hashmap_remove_value(h->manager->homes_by_worker_pid, PID_TO_PTR(h->worker_pid), h);
-        h->worker_pid = 0;
+        (void) hashmap_remove_value(h->manager->homes_by_worker_pid, &h->worker_pid, h);
+        pidref_done(&h->worker_pid);
         return 1;
 }
 

src/home/homed-home.h

@@ -8,6 +8,7 @@ typedef struct Home Home;
 #include "homed-operation.h"
 #include "list.h"
 #include "ordered-set.h"
+#include "pidref.h"
 #include "stat-util.h"
 #include "user-record.h"
 
@@ -128,7 +129,7 @@ struct Home {
 
         UserRecord *record;
 
-        pid_t worker_pid;
+        PidRef worker_pid;
         int worker_stdout_fd;
         sd_event_source *worker_event_source;
         int worker_error_code;

src/home/homed-manager.c

@@ -37,6 +37,7 @@
 #include "homed-varlink.h"
 #include "io-util.h"
 #include "mkdir.h"
+#include "notify-recv.h"
 #include "openssl-util.h"
 #include "process-util.h"
 #include "quota-util.h"
@@ -76,7 +77,7 @@ static bool uid_is_home(uid_t uid) {
 #define UID_CLAMP_INTO_HOME_RANGE(rnd) (((uid_t) (rnd) % (HOME_UID_MAX - HOME_UID_MIN + 1)) + HOME_UID_MIN)
 
 DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(homes_by_uid_hash_ops, void, trivial_hash_func, trivial_compare_func, Home, home_free);
-DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(homes_by_worker_pid_hash_ops, void, trivial_hash_func, trivial_compare_func, Home, home_free);
+DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(homes_by_worker_pid_hash_ops, PidRef, pidref_hash_func, pidref_compare_func, Home, home_free);
 DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(homes_by_sysfs_hash_ops, char, path_hash_func, path_compare, Home, home_free);
 
 static int on_home_inotify(sd_event_source *s, const struct inotify_event *event, void *userdata);
@@ -1068,123 +1069,33 @@ static int manager_bind_varlink(Manager *m) {
         return 0;
 }
 
-static ssize_t read_datagram(
-                int fd,
-                struct ucred *ret_sender,
-                void **ret,
-                int *ret_passed_fd) {
-
-        CMSG_BUFFER_TYPE(CMSG_SPACE(sizeof(struct ucred)) + CMSG_SPACE(sizeof(int))) control;
-        _cleanup_free_ void *buffer = NULL;
-        _cleanup_close_ int passed_fd = -EBADF;
-        struct ucred *sender = NULL;
-        struct cmsghdr *cmsg;
-        struct msghdr mh;
-        struct iovec iov;
-        ssize_t n, m;
-
-        assert(fd >= 0);
-        assert(ret_sender);
-        assert(ret);
-        assert(ret_passed_fd);
-
-        n = next_datagram_size_fd(fd);
-        if (n < 0)
-                return n;
-
-        buffer = malloc(n + 2);
-        if (!buffer)
-                return -ENOMEM;
-
-        /* Pass one extra byte, as a size check */
-        iov = IOVEC_MAKE(buffer, n + 1);
-
-        mh = (struct msghdr) {
-                .msg_iov = &iov,
-                .msg_iovlen = 1,
-                .msg_control = &control,
-                .msg_controllen = sizeof(control),
-        };
-
-        m = recvmsg_safe(fd, &mh, MSG_DONTWAIT|MSG_CMSG_CLOEXEC);
-        if (m < 0)
-                return m;
-
-        /* Ensure the size matches what we determined before */
-        if (m != n) {
-                cmsg_close_all(&mh);
-                return -EMSGSIZE;
-        }
-
-        CMSG_FOREACH(cmsg, &mh) {
-                if (cmsg->cmsg_level == SOL_SOCKET &&
-                    cmsg->cmsg_type == SCM_CREDENTIALS &&
-                    cmsg->cmsg_len == CMSG_LEN(sizeof(struct ucred))) {
-                        assert(!sender);
-                        sender = CMSG_TYPED_DATA(cmsg, struct ucred);
-                }
-
-                if (cmsg->cmsg_level == SOL_SOCKET &&
-                    cmsg->cmsg_type == SCM_RIGHTS) {
-
-                        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int))) {
-                                cmsg_close_all(&mh);
-                                return -EMSGSIZE;
-                        }
-
-                        assert(passed_fd < 0);
-                        passed_fd = *CMSG_TYPED_DATA(cmsg, int);
-                }
-        }
-
-        if (sender)
-                *ret_sender = *sender;
-        else
-                *ret_sender = (struct ucred) UCRED_INVALID;
-
-        *ret_passed_fd = TAKE_FD(passed_fd);
-
-        /* For safety reasons: let's always NUL terminate.  */
-        ((char*) buffer)[n] = 0;
-        *ret = TAKE_PTR(buffer);
-
-        return 0;
-}
-
 static int on_notify_socket(sd_event_source *s, int fd, uint32_t revents, void *userdata) {
-        _cleanup_strv_free_ char **l = NULL;
-        _cleanup_free_ void *datagram = NULL;
-        _cleanup_close_ int passed_fd = -EBADF;
-        struct ucred sender = UCRED_INVALID;
         Manager *m = ASSERT_PTR(userdata);
-        ssize_t n;
-        Home *h;
+        int r;
 
         assert(s);
 
-        n = read_datagram(fd, &sender, &datagram, &passed_fd);
-        if (n < 0) {
-                if (ERRNO_IS_TRANSIENT(n))
-                        return 0;
-                return log_error_errno(n, "Failed to read notify datagram: %m");
-        }
+        _cleanup_(fdset_free_asyncp) FDSet *passed_fds = NULL;
+        _cleanup_(pidref_done) PidRef sender = PIDREF_NULL;
+        _cleanup_strv_free_ char **l = NULL;
+        r = notify_recv_with_fds_strv(fd, &l, /* ret_ucred= */ NULL, &sender, &passed_fds);
+        if (r == -EAGAIN)
+                return 0;
+        if (r < 0)
+                return r;
 
-        if (sender.pid <= 0) {
-                log_warning("Received notify datagram without valid sender PID, ignoring.");
+        if (fdset_size(passed_fds) > 1) {
+                log_warning("Received notify datagram with multiple fds, ignoring.");
                 return 0;
         }
 
-        h = hashmap_get(m->homes_by_worker_pid, PID_TO_PTR(sender.pid));
+        Home *h = hashmap_get(m->homes_by_worker_pid, &sender);
         if (!h) {
                 log_warning("Received notify datagram of unknown process, ignoring.");
                 return 0;
         }
 
-        l = strv_split(datagram, "\n");
-        if (!l)
-                return log_oom();
-
-        home_process_notify(h, l, TAKE_FD(passed_fd));
+        home_process_notify(h, l, fdset_steal_first(passed_fds));
         return 0;
 }
 
@@ -1224,7 +1135,11 @@ static int manager_listen_notify(Manager *m) {
 
         r = setsockopt_int(fd, SOL_SOCKET, SO_PASSCRED, true);
         if (r < 0)
-                return r;
+                return log_error_errno(r, "Failed to enable SO_PASSCRED on notify socket: %m");
+
+        r = setsockopt_int(fd, SOL_SOCKET, SO_PASSPIDFD, true);
+        if (r < 0)
+                log_warning_errno(r, "Failed to enable SO_PASSPIDFD on notify socket, ignoring: %m");
 
         r = sd_event_add_io(m->event, &m->notify_socket_event_source, fd, EPOLLIN, on_notify_socket, m);
         if (r < 0)

src/nspawn/nspawn.c

@@ -4622,8 +4622,8 @@ static int nspawn_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t r
         assert(userdata);
 
         _cleanup_(pidref_done) PidRef sender_pid = PIDREF_NULL;
-        _cleanup_free_ char *buf = NULL;
-        r = notify_recv(fd, &buf, /* ret_ucred= */ NULL, &sender_pid);
+        _cleanup_strv_free_ char **tags = NULL;
+        r = notify_recv_strv(fd, &tags, /* ret_ucred= */ NULL, &sender_pid);
         if (r == -EAGAIN)
                 return 0;
         if (r < 0)
@@ -4634,10 +4634,6 @@ static int nspawn_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t r
                 return 0;
         }
 
-        _cleanup_strv_free_ char **tags = strv_split(buf, "\n\r");
-        if (!tags)
-                return log_oom();
-
         if (DEBUG_LOGGING) {
                 _cleanup_free_ char *joined = strv_join(tags, " ");
 

src/shared/notify-recv.c

@@ -4,6 +4,8 @@
 #include "fd-util.h"
 #include "notify-recv.h"
 #include "socket-util.h"
+#include "strv.h"
+#include "user-util.h"
 
 int notify_recv_with_fds(
                 int fd,
@@ -140,3 +142,45 @@ int notify_recv_with_fds(
 
         return 0;
 }
+
+int notify_recv_with_fds_strv(
+                int fd,
+                char ***ret_list,
+                struct ucred *ret_ucred,
+                PidRef *ret_pidref,
+                FDSet **ret_fds) {
+
+        _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL;
+        _cleanup_(fdset_free_asyncp) FDSet *fds = NULL;
+        struct ucred ucred = UCRED_INVALID;
+        _cleanup_free_ char *text = NULL;
+        int r;
+
+        r = notify_recv_with_fds(
+                        fd,
+                        ret_list ? &text : NULL,
+                        ret_ucred ? &ucred : NULL,
+                        ret_pidref ? &pidref : NULL,
+                        ret_fds ? &fds : NULL);
+        if (r < 0)
+                return r;
+
+        if (ret_list) {
+                char **l = strv_split_newlines(text);
+                if (!l) {
+                        log_oom_warning();
+                        return -EAGAIN;
+                }
+
+                *ret_list = l;
+        }
+
+        if (ret_ucred)
+                *ret_ucred = ucred;
+        if (ret_pidref)
+                *ret_pidref = TAKE_PIDREF(pidref);
+        if (ret_fds)
+                *ret_fds = TAKE_PTR(fds);
+
+        return 0;
+}

src/shared/notify-recv.h

@@ -16,3 +16,14 @@ int notify_recv_with_fds(
 static inline int notify_recv(int fd, char **ret_text, struct ucred *ret_ucred, PidRef *ret_pidref) {
         return notify_recv_with_fds(fd, ret_text, ret_ucred, ret_pidref, NULL);
 }
+
+int notify_recv_with_fds_strv(
+                int fd,
+                char ***ret_list,
+                struct ucred *ret_ucred,
+                PidRef *ret_pidref,
+                FDSet **ret_fds);
+
+static inline int notify_recv_strv(int fd, char ***ret_list, struct ucred *ret_ucred, PidRef *ret_pidref) {
+        return notify_recv_with_fds_strv(fd, ret_list, ret_ucred, ret_pidref, NULL);
+}