From 32020b4788f74449826c89b7ac043c8762196480 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 21 Jul 2024 17:01:22 +0200 Subject: [PATCH 01/11] core/device: minor refactors --- src/core/device.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index d8567676a7..f8ded6c838 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -54,8 +54,6 @@ static int device_by_path(Manager *m, const char *path, Unit **ret) { } static void device_unset_sysfs(Device *d) { - Hashmap *devices; - assert(d); if (!d->sysfs) @@ -63,7 +61,7 @@ static void device_unset_sysfs(Device *d) { /* Remove this unit from the chain of devices which share the same sysfs path. */ - devices = UNIT(d)->manager->devices_by_sysfs; + Hashmap *devices = ASSERT_PTR(UNIT(d)->manager->devices_by_sysfs); if (d->same_sysfs_prev) /* If this is not the first unit, then simply remove this unit. */ @@ -84,36 +82,35 @@ static void device_unset_sysfs(Device *d) { } static int device_set_sysfs(Device *d, const char *sysfs) { - _cleanup_free_ char *copy = NULL; - Device *first; + Unit *u = UNIT(ASSERT_PTR(d)); int r; - assert(d); - if (streq_ptr(d->sysfs, sysfs)) return 0; - r = hashmap_ensure_allocated(&UNIT(d)->manager->devices_by_sysfs, &path_hash_ops); + Hashmap **devices = &u->manager->devices_by_sysfs; + + r = hashmap_ensure_allocated(devices, &path_hash_ops); if (r < 0) return r; - copy = strdup(sysfs); + _cleanup_free_ char *copy = strdup(sysfs); if (!copy) return -ENOMEM; device_unset_sysfs(d); - first = hashmap_get(UNIT(d)->manager->devices_by_sysfs, sysfs); + Device *first = hashmap_get(*devices, sysfs); LIST_PREPEND(same_sysfs, first, d); - r = hashmap_replace(UNIT(d)->manager->devices_by_sysfs, copy, first); + r = hashmap_replace(*devices, copy, first); if (r < 0) { LIST_REMOVE(same_sysfs, first, d); return r; } d->sysfs = TAKE_PTR(copy); - unit_add_to_dbus_queue(UNIT(d)); + unit_add_to_dbus_queue(u); return 0; } @@ -350,13 +347,14 @@ static const struct { static int device_found_to_string_many(DeviceFound flags, char **ret) { _cleanup_free_ char *s = NULL; + assert(flags >= 0); assert(ret); - for (size_t i = 0; i < ELEMENTSOF(device_found_map); i++) { - if (!FLAGS_SET(flags, device_found_map[i].flag)) + FOREACH_ELEMENT(i, device_found_map) { + if (!FLAGS_SET(flags, i->flag)) continue; - if (!strextend_with_separator(&s, ",", device_found_map[i].name)) + if (!strextend_with_separator(&s, ",", i->name)) return -ENOMEM; } From 777f69006b33b3bffc316dc612ca68c9d296ed4a Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 21 Jul 2024 16:59:19 +0200 Subject: [PATCH 02/11] core/device: prefix DEVICE_FOUND_MASK with '_' In accordance to our usual style for special enum values. --- src/core/device.c | 6 +++--- src/core/device.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index f8ded6c838..4b62477d2c 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -332,7 +332,7 @@ static void device_catchup(Unit *u) { Device *d = ASSERT_PTR(DEVICE(u)); /* Second, let's update the state with the enumerated state */ - device_update_found_one(d, d->enumerated_found, DEVICE_FOUND_MASK); + device_update_found_one(d, d->enumerated_found, _DEVICE_FOUND_MASK); } static const struct { @@ -1097,7 +1097,7 @@ static void device_remove_old_on_move(Manager *m, sd_device *dev) { if (!syspath_old) return (void) log_oom(); - device_update_found_by_sysfs(m, syspath_old, DEVICE_NOT_FOUND, DEVICE_FOUND_MASK); + device_update_found_by_sysfs(m, syspath_old, DEVICE_NOT_FOUND, _DEVICE_FOUND_MASK); } static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void *userdata) { @@ -1177,7 +1177,7 @@ static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void * /* If we get notified that a device was removed by udev, then it's completely gone, hence * unset all found bits. Note this affects all .device units still point to the removed * device. */ - device_update_found_by_sysfs(m, sysfs, DEVICE_NOT_FOUND, DEVICE_FOUND_MASK); + device_update_found_by_sysfs(m, sysfs, DEVICE_NOT_FOUND, _DEVICE_FOUND_MASK); /* These devices are found and ready now, set the udev found bit. Note, this is also necessary to do * on remove uevent, as some devlinks may be updated and now point to other device nodes. */ diff --git a/src/core/device.h b/src/core/device.h index 9dd6fb57c2..4eb656777a 100644 --- a/src/core/device.h +++ b/src/core/device.h @@ -14,7 +14,7 @@ typedef enum DeviceFound { DEVICE_FOUND_UDEV = 1 << 0, /* The device has shown up in the udev database */ DEVICE_FOUND_MOUNT = 1 << 1, /* The device has shown up in /proc/self/mountinfo */ DEVICE_FOUND_SWAP = 1 << 2, /* The device has shown up in /proc/swaps */ - DEVICE_FOUND_MASK = DEVICE_FOUND_UDEV|DEVICE_FOUND_MOUNT|DEVICE_FOUND_SWAP, + _DEVICE_FOUND_MASK = DEVICE_FOUND_UDEV|DEVICE_FOUND_MOUNT|DEVICE_FOUND_SWAP, } DeviceFound; struct Device { From 71f2ba82cfc75527e40fdeefe95159199a3eb4a5 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 17 Jul 2024 17:52:00 +0200 Subject: [PATCH 03/11] core/dbus: make two functions return void --- src/core/dbus.c | 16 +++++++--------- src/core/dbus.h | 2 +- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/core/dbus.c b/src/core/dbus.c index 1c6f6fc8b3..39cce302db 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -49,24 +49,24 @@ static void destroy_bus(Manager *m, sd_bus **bus); -int bus_send_pending_reload_message(Manager *m) { +void bus_send_pending_reload_message(Manager *m) { int r; assert(m); if (!m->pending_reload_message) - return 0; + return; /* If we cannot get rid of this message we won't dispatch any D-Bus messages, so that we won't end up wanting * to queue another message. */ r = sd_bus_send(NULL, m->pending_reload_message, NULL); if (r < 0) - log_warning_errno(r, "Failed to send queued message, ignoring: %m"); + log_warning_errno(r, "Failed to send queued reload message, ignoring: %m"); m->pending_reload_message = sd_bus_message_unref(m->pending_reload_message); - return 0; + return; } int bus_forward_agent_released(Manager *m, const char *path) { @@ -857,7 +857,7 @@ int bus_init_api(Manager *m) { return 0; } -static int bus_setup_system(Manager *m, sd_bus *bus) { +static void bus_setup_system(Manager *m, sd_bus *bus) { int r; assert(m); @@ -878,7 +878,7 @@ static int bus_setup_system(Manager *m, sd_bus *bus) { } log_debug("Successfully connected to system bus."); - return 0; + return; } int bus_init_system(Manager *m) { @@ -905,9 +905,7 @@ int bus_init_system(Manager *m) { return r; } - r = bus_setup_system(m, bus); - if (r < 0) - return log_error_errno(r, "Failed to set up system bus: %m"); + bus_setup_system(m, bus); m->system_bus = TAKE_PTR(bus); diff --git a/src/core/dbus.h b/src/core/dbus.h index 50e7bb400e..ae75404076 100644 --- a/src/core/dbus.h +++ b/src/core/dbus.h @@ -5,7 +5,7 @@ #include "manager.h" -int bus_send_pending_reload_message(Manager *m); +void bus_send_pending_reload_message(Manager *m); int bus_init_private(Manager *m); int bus_init_api(Manager *m); From 93df5cf8d9718453bb372a3007cf583101e0c50c Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 17 Jul 2024 18:07:58 +0200 Subject: [PATCH 04/11] core/dbus: use Manager.prefix where appropriate --- src/core/dbus.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/core/dbus.c b/src/core/dbus.c index 39cce302db..b6480937ee 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -825,6 +825,8 @@ int bus_init_api(Manager *m) { _cleanup_(sd_bus_close_unrefp) sd_bus *bus = NULL; int r; + assert(m); + if (m->api_bus) return 0; @@ -932,22 +934,16 @@ int bus_init_private(Manager *m) { r = sockaddr_un_set_path(&sa.un, "/run/systemd/private"); } else { - _cleanup_free_ char *joined = NULL; - const char *e; + _cleanup_free_ char *p = NULL; - e = secure_getenv("XDG_RUNTIME_DIR"); - if (!e) - return log_error_errno(SYNTHETIC_ERRNO(EHOSTDOWN), - "XDG_RUNTIME_DIR is not set, refusing."); - - joined = path_join(e, "/systemd/private"); - if (!joined) + p = path_join(m->prefix[EXEC_DIRECTORY_RUNTIME], "systemd/private"); + if (!p) return log_oom(); - r = sockaddr_un_set_path(&sa.un, joined); + r = sockaddr_un_set_path(&sa.un, p); } if (r < 0) - return log_error_errno(r, "Can't set path for AF_UNIX socket to bind to: %m"); + return log_error_errno(r, "Failed set socket path for private bus: %m"); sa_len = r; (void) mkdir_parents_label(sa.un.sun_path, 0755); From 77f709ca84051f6955e87c2498386db915ddcd67 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Fri, 12 Jul 2024 17:15:50 +0200 Subject: [PATCH 05/11] pidref: move comments that doesn't belong in pidref_copy() to pidref_dup() Follow-up for 232e66217d64aeab51786b0d2794377874ab61ab --- src/basic/pidref.c | 14 +++++++------- src/basic/pidref.h | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/basic/pidref.c b/src/basic/pidref.c index d9380a84ab..8529236c5e 100644 --- a/src/basic/pidref.c +++ b/src/basic/pidref.c @@ -221,7 +221,7 @@ void pidref_done(PidRef *pidref) { }; } -PidRef *pidref_free(PidRef *pidref) { +PidRef* pidref_free(PidRef *pidref) { /* Regularly, this is an embedded structure. But sometimes we want it on the heap too */ if (!pidref) return NULL; @@ -234,13 +234,10 @@ int pidref_copy(const PidRef *pidref, PidRef *dest) { _cleanup_close_ int dup_fd = -EBADF; pid_t dup_pid = 0; - assert(dest); + /* If NULL is passed we'll generate a PidRef that refers to no process. This makes it easy to + * copy pidref fields that might or might not reference a process yet. */ - /* Allocates a new PidRef on the heap, making it a copy of the specified pidref. This does not try to - * acquire a pidfd if we don't have one yet! - * - * If NULL is passed we'll generate a PidRef that refers to no process. This makes it easy to copy - * pidref fields that might or might not reference a process yet. */ + assert(dest); if (pidref) { if (pidref->fd >= 0) { @@ -269,6 +266,9 @@ int pidref_dup(const PidRef *pidref, PidRef **ret) { _cleanup_(pidref_freep) PidRef *dup_pidref = NULL; int r; + /* Allocates a new PidRef on the heap, making it a copy of the specified pidref. This does not try to + * acquire a pidfd if we don't have one yet! */ + assert(ret); dup_pidref = newdup(PidRef, &PIDREF_NULL, 1); diff --git a/src/basic/pidref.h b/src/basic/pidref.h index 0581f1af1e..4738c6eb5d 100644 --- a/src/basic/pidref.h +++ b/src/basic/pidref.h @@ -39,7 +39,7 @@ static inline int pidref_set_self(PidRef *pidref) { bool pidref_is_self(const PidRef *pidref); void pidref_done(PidRef *pidref); -PidRef *pidref_free(PidRef *pidref); +PidRef* pidref_free(PidRef *pidref); DEFINE_TRIVIAL_CLEANUP_FUNC(PidRef*, pidref_free); int pidref_copy(const PidRef *pidref, PidRef *dest); From 8dc303d3c86190de9b6bf5aae60b64f1e11ad354 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 14 Jul 2024 18:25:20 +0200 Subject: [PATCH 06/11] process-util: modernize pidfd_get_pid() --- src/basic/fd-util.c | 6 ++++-- src/basic/process-util.c | 12 +++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index d5e733293c..5d7fc258b2 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -1123,6 +1123,8 @@ int proc_fd_enoent_errno(void) { if (r == 0) return -ENOSYS; /* /proc/ is not available or not set up properly, we're most likely in some chroot environment. */ - return r > 0 ? -EBADF : -ENOENT; /* If /proc/ is definitely around then this means the fd is - not valid, otherwise let's propagate the original ENOENT. */ + if (r > 0) + return -EBADF; /* If /proc/ is definitely around then this means the fd is not valid. */ + + return -ENOENT; /* Otherwise let's propagate the original ENOENT. */ } diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 15839cb447..ede5f85135 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1840,7 +1840,6 @@ int get_oom_score_adjust(int *ret) { int pidfd_get_pid(int fd, pid_t *ret) { char path[STRLEN("/proc/self/fdinfo/") + DECIMAL_STR_MAX(int)]; _cleanup_free_ char *fdinfo = NULL; - char *p; int r; /* Converts a pidfd into a pid. Well known errors: @@ -1852,22 +1851,21 @@ int pidfd_get_pid(int fd, pid_t *ret) { * -ESRCH → fd valid, but process is already reaped */ - if (fd < 0) - return -EBADF; + assert(fd >= 0); xsprintf(path, "/proc/self/fdinfo/%i", fd); r = read_full_virtual_file(path, &fdinfo, NULL); - if (r == -ENOENT) /* if fdinfo doesn't exist we assume the process does not exist */ - return proc_mounted() > 0 ? -EBADF : -ENOSYS; + if (r == -ENOENT) + return proc_fd_enoent_errno(); if (r < 0) return r; - p = find_line_startswith(fdinfo, "Pid:"); + char *p = find_line_startswith(fdinfo, "Pid:"); if (!p) return -ENOTTY; /* not a pidfd? */ - p += strspn(p, WHITESPACE); + p = skip_leading_chars(p, /* bad = */ NULL); p[strcspn(p, WHITESPACE)] = 0; if (streq(p, "0")) From 678f271cff278429d680dcbfd6cf1a14b1cff3fe Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 14 Jul 2024 17:32:12 +0200 Subject: [PATCH 07/11] core/manager: modernize manager_setup_notify() a bit --- src/core/manager.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 1504d3fd7d..879b73af3c 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1081,7 +1081,7 @@ static int manager_setup_notify(Manager *m) { if (fd < 0) return log_error_errno(errno, "Failed to allocate notification socket: %m"); - fd_increase_rxbuf(fd, NOTIFY_RCVBUF_SIZE); + (void) fd_increase_rxbuf(fd, NOTIFY_RCVBUF_SIZE); m->notify_socket = path_join(m->prefix[EXEC_DIRECTORY_RUNTIME], "systemd/notify"); if (!m->notify_socket) @@ -1098,11 +1098,11 @@ static int manager_setup_notify(Manager *m) { r = mac_selinux_bind(fd, &sa.sa, sa_len); if (r < 0) - return log_error_errno(r, "bind(%s) failed: %m", m->notify_socket); + return log_error_errno(r, "Failed to bind notify fd to '%s': %m", m->notify_socket); r = setsockopt_int(fd, SOL_SOCKET, SO_PASSCRED, true); if (r < 0) - return log_error_errno(r, "SO_PASSCRED failed: %m"); + return log_error_errno(r, "Failed to enable SO_PASSCRED for notify socket: %m"); m->notify_fd = TAKE_FD(fd); @@ -2042,7 +2042,7 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds, const char *roo * containers. */ manager_distribute_fds(m, fds); - /* We might have deserialized the notify fd, but if we didn't then let's create the bus now */ + /* We might have deserialized the notify fd, but if we didn't then let's create it now */ r = manager_setup_notify(m); if (r < 0) /* No sense to continue without notifications, our children would fail anyway. */ From 7d6e1293b2f0125809f45c2ee1d27ff72af28173 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 14 Jul 2024 17:16:30 +0200 Subject: [PATCH 08/11] core/manager: minor refactor for manager_dispatch_notify_fd() --- src/core/manager.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 879b73af3c..b8de50c2c0 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2718,7 +2718,6 @@ static int manager_get_units_for_pidref(Manager *m, const PidRef *pidref, Unit * static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t revents, void *userdata) { Manager *m = ASSERT_PTR(userdata); - _cleanup_fdset_free_ FDSet *fds = NULL; char buf[NOTIFY_BUFFER_MAX+1]; struct iovec iovec = { .iov_base = buf, @@ -2732,13 +2731,8 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t .msg_control = &control, .msg_controllen = sizeof(control), }; - - struct cmsghdr *cmsg; - struct ucred *ucred = NULL; - _cleanup_strv_free_ char **tags = NULL; - int r, *fd_array = NULL; - size_t n_fds = 0; ssize_t n; + int r; assert(m->notify_fd == fd); @@ -2751,7 +2745,7 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t if (ERRNO_IS_NEG_TRANSIENT(n)) return 0; /* Spurious wakeup, try again */ if (n == -EXFULL) { - log_warning("Got message with truncated control data (too many fds sent?), ignoring."); + log_warning_errno(n, "Got message with truncated control data (too many fds sent?), ignoring."); return 0; } if (n < 0) @@ -2761,20 +2755,28 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t * socket. */ return log_error_errno(n, "Failed to receive notification message: %m"); - CMSG_FOREACH(cmsg, &msghdr) - if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS) { + struct ucred *ucred = NULL; + int *fd_array = NULL; + size_t n_fds = 0; + struct cmsghdr *cmsg; + CMSG_FOREACH(cmsg, &msghdr) { + if (cmsg->cmsg_level != SOL_SOCKET) + continue; + + if (cmsg->cmsg_type == SCM_RIGHTS) { assert(!fd_array); fd_array = CMSG_TYPED_DATA(cmsg, int); n_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int); - - } else if (cmsg->cmsg_level == SOL_SOCKET && - cmsg->cmsg_type == SCM_CREDENTIALS && + } else if (cmsg->cmsg_type == SCM_CREDENTIALS && cmsg->cmsg_len == CMSG_LEN(sizeof(struct ucred))) { assert(!ucred); ucred = CMSG_TYPED_DATA(cmsg, struct ucred); } + } + + _cleanup_fdset_free_ FDSet *fds = NULL; if (n_fds > 0) { assert(fd_array); @@ -2782,7 +2784,7 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t r = fdset_new_array(&fds, fd_array, n_fds); if (r < 0) { close_many(fd_array, n_fds); - log_oom(); + log_oom_warning(); return 0; } } @@ -2806,9 +2808,10 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t /* Make sure it's NUL-terminated, then parse it to obtain the tags list. */ buf[n] = 0; - tags = strv_split_newlines(buf); + + _cleanup_strv_free_ char **tags = strv_split_newlines(buf); if (!tags) { - log_oom(); + log_oom_warning(); return 0; } From d3d0e36156836b560b8019090098b2c8da62918b Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 14 Jul 2024 20:32:34 +0200 Subject: [PATCH 09/11] core/service: use switch in service_notify_message_authorized() --- src/core/service.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 9213736867..b4d283686c 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -4345,24 +4345,31 @@ static bool service_notify_message_authorized(Service *s, pid_t pid) { assert(s); assert(pid_is_valid(pid)); - NotifyAccess notify_access = service_get_notify_access(s); + switch (service_get_notify_access(s)) { - if (notify_access == NOTIFY_NONE) { + case NOTIFY_NONE: /* Warn level only if no notifications are expected */ log_unit_warning(UNIT(s), "Got notification message from PID "PID_FMT", but reception is disabled", pid); return false; - } - if (notify_access == NOTIFY_MAIN && pid != s->main_pid.pid) { + case NOTIFY_ALL: + return true; + + case NOTIFY_MAIN: + if (pid == s->main_pid.pid) + return true; + if (pidref_is_set(&s->main_pid)) log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID "PID_FMT, pid, s->main_pid.pid); else log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID which is currently not known", pid); return false; - } - if (notify_access == NOTIFY_EXEC && pid != s->main_pid.pid && pid != s->control_pid.pid) { + case NOTIFY_EXEC: + if (pid == s->main_pid.pid || pid == s->control_pid.pid) + return true; + if (pidref_is_set(&s->main_pid) && pidref_is_set(&s->control_pid)) log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID "PID_FMT" and control PID "PID_FMT, pid, s->main_pid.pid, s->control_pid.pid); @@ -4374,9 +4381,10 @@ static bool service_notify_message_authorized(Service *s, pid_t pid) { log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID and control PID which are currently not known", pid); return false; - } - return true; + default: + assert_not_reached(); + } } static void service_notify_message( From 31d76a1702e930b48a3041fbf0b37c325d2c9bf2 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 21 Jul 2024 22:50:58 +0200 Subject: [PATCH 10/11] missing_socket: add SCM_PASSPIDFD and SCM_PIDFD Preparation for later commits. --- src/basic/missing_socket.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/basic/missing_socket.h b/src/basic/missing_socket.h index 47cc7626aa..5d9c5b483d 100644 --- a/src/basic/missing_socket.h +++ b/src/basic/missing_socket.h @@ -51,6 +51,10 @@ struct sockaddr_vm { #define SO_PEERGROUPS 59 #endif +#ifndef SO_PASSPIDFD +#define SO_PASSPIDFD 76 +#endif + #ifndef SO_PEERPIDFD #define SO_PEERPIDFD 77 #endif @@ -72,11 +76,14 @@ struct sockaddr_vm { #define SOL_SCTP 132 #endif -/* Not exposed yet. Defined in include/linux/socket.h */ #ifndef SCM_SECURITY #define SCM_SECURITY 0x03 #endif +#ifndef SCM_PIDFD +#define SCM_PIDFD 0x04 +#endif + /* netinet/in.h */ #ifndef IP_FREEBIND #define IP_FREEBIND 15 From 19eccb7667f316c94f08829809852158b6109528 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 14 Jul 2024 17:19:50 +0200 Subject: [PATCH 11/11] core: pin notify sender through pidfd (potentially SCM_PIDFD) --- TODO | 1 - src/core/manager.c | 43 +++++++++++++++++++++++++++++++++++++------ src/core/service.c | 28 +++++++++++++++------------- src/core/unit.h | 2 +- 4 files changed, 53 insertions(+), 21 deletions(-) diff --git a/TODO b/TODO index e375dd1f4f..2d37bc671f 100644 --- a/TODO +++ b/TODO @@ -579,7 +579,6 @@ Features: - cg_pid_get_xyz() - pid_from_same_root_fs() - get_ctty_devnr() - - pid1: sd_notify() receiver should use SCM_PIDFD to authenticate client - actually wait for POLLIN on pidref's pidfd in service logic - openpt_allocate_in_namespace() - unit_attach_pid_to_cgroup_via_bus() diff --git a/src/core/manager.c b/src/core/manager.c index b8de50c2c0..39fbbfa470 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1104,6 +1104,11 @@ static int manager_setup_notify(Manager *m) { if (r < 0) return log_error_errno(r, "Failed to enable SO_PASSCRED for notify socket: %m"); + r = setsockopt_int(fd, SOL_SOCKET, SO_PASSPIDFD, true); + if (r < 0 && r != -ENOPROTOOPT) + log_warning_errno(r, "Failed to enable SO_PASSPIDFD for notify socket, ignoring: %m"); + // TODO: maybe enforce SO_PASSPIDFD? + m->notify_fd = TAKE_FD(fd); log_debug("Using notification socket %s", m->notify_socket); @@ -2638,13 +2643,16 @@ static bool manager_process_barrier_fd(char * const *tags, FDSet *fds) { static void manager_invoke_notify_message( Manager *m, Unit *u, + PidRef *pidref, const struct ucred *ucred, char * const *tags, FDSet *fds) { assert(m); assert(u); + assert(pidref_is_set(pidref)); assert(ucred); + assert(pidref->pid == ucred->pid); assert(tags); if (u->notifygen == m->notifygen) /* Already invoked on this same unit in this same iteration? */ @@ -2652,7 +2660,7 @@ static void manager_invoke_notify_message( u->notifygen = m->notifygen; if (UNIT_VTABLE(u)->notify_message) - UNIT_VTABLE(u)->notify_message(u, ucred, tags, fds); + UNIT_VTABLE(u)->notify_message(u, pidref, ucred, tags, fds); else if (DEBUG_LOGGING) { _cleanup_free_ char *joined = strv_join(tags, ", "); @@ -2723,7 +2731,7 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t .iov_base = buf, .iov_len = sizeof(buf)-1, }; - CMSG_BUFFER_TYPE(CMSG_SPACE(sizeof(struct ucred)) + + CMSG_BUFFER_TYPE(CMSG_SPACE(sizeof(struct ucred)) + CMSG_SPACE(sizeof(int)) /* SCM_PIDFD */ + CMSG_SPACE(sizeof(int) * NOTIFY_FD_MAX)) control; struct msghdr msghdr = { .msg_iov = &iovec, @@ -2755,6 +2763,7 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t * socket. */ return log_error_errno(n, "Failed to receive notification message: %m"); + _cleanup_close_ int pidfd = -EBADF; struct ucred *ucred = NULL; int *fd_array = NULL; size_t n_fds = 0; @@ -2773,6 +2782,9 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t assert(!ucred); ucred = CMSG_TYPED_DATA(cmsg, struct ucred); + } else if (cmsg->cmsg_type == SCM_PIDFD) { + assert(pidfd < 0); + pidfd = *CMSG_TYPED_DATA(cmsg, int); } } @@ -2794,6 +2806,28 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t return 0; } + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; + + if (pidfd >= 0) + r = pidref_set_pidfd_consume(&pidref, TAKE_FD(pidfd)); + else + r = pidref_set_pid(&pidref, ucred->pid); + if (r < 0) { + if (r == -ESRCH) + log_debug_errno(r, "Notify sender died before message is processed. Ignoring."); + else + log_warning_errno(r, "Failed to pin notify sender, ignoring message: %m"); + return 0; + } + + if (pidref.pid != ucred->pid) { + assert(pidref.fd >= 0); + + log_warning("Got SCM_PIDFD for process " PID_FMT " but SCM_CREDENTIALS for process " PID_FMT ". Ignoring.", + pidref.pid, ucred->pid); + return 0; + } + if ((size_t) n >= sizeof(buf) || (msghdr.msg_flags & MSG_TRUNC)) { log_warning("Received notify message exceeded maximum size. Ignoring."); return 0; @@ -2824,9 +2858,6 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t /* Increase the generation counter used for filtering out duplicate unit invocations. */ m->notifygen++; - /* Generate lookup key from the PID (we have no pidfd here, after all) */ - PidRef pidref = PIDREF_MAKE_FROM_PID(ucred->pid); - /* Notify every unit that might be interested, which might be multiple. */ _cleanup_free_ Unit **array = NULL; @@ -2841,7 +2872,7 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t /* And now invoke the per-unit callbacks. Note that manager_invoke_notify_message() will handle * duplicate units – making sure we only invoke each unit's handler once. */ FOREACH_ARRAY(u, array, n_array) - manager_invoke_notify_message(m, *u, ucred, tags, fds); + manager_invoke_notify_message(m, *u, &pidref, ucred, tags, fds); if (!fdset_isempty(fds)) log_warning("Got extra auxiliary fds with notification message, closing them."); diff --git a/src/core/service.c b/src/core/service.c index b4d283686c..4403b209bd 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -4341,44 +4341,44 @@ static void service_force_watchdog(Service *s) { service_enter_signal(s, SERVICE_STOP_WATCHDOG, SERVICE_FAILURE_WATCHDOG); } -static bool service_notify_message_authorized(Service *s, pid_t pid) { +static bool service_notify_message_authorized(Service *s, PidRef *pid) { assert(s); - assert(pid_is_valid(pid)); + assert(pidref_is_set(pid)); switch (service_get_notify_access(s)) { case NOTIFY_NONE: /* Warn level only if no notifications are expected */ - log_unit_warning(UNIT(s), "Got notification message from PID "PID_FMT", but reception is disabled", pid); + log_unit_warning(UNIT(s), "Got notification message from PID "PID_FMT", but reception is disabled", pid->pid); return false; case NOTIFY_ALL: return true; case NOTIFY_MAIN: - if (pid == s->main_pid.pid) + if (pidref_equal(pid, &s->main_pid)) return true; if (pidref_is_set(&s->main_pid)) - log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID "PID_FMT, pid, s->main_pid.pid); + log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID "PID_FMT, pid->pid, s->main_pid.pid); else - log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID which is currently not known", pid); + log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID which is currently not known", pid->pid); return false; case NOTIFY_EXEC: - if (pid == s->main_pid.pid || pid == s->control_pid.pid) + if (pidref_equal(pid, &s->main_pid) || pidref_equal(pid, &s->control_pid)) return true; if (pidref_is_set(&s->main_pid) && pidref_is_set(&s->control_pid)) log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID "PID_FMT" and control PID "PID_FMT, - pid, s->main_pid.pid, s->control_pid.pid); + pid->pid, s->main_pid.pid, s->control_pid.pid); else if (pidref_is_set(&s->main_pid)) - log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID "PID_FMT, pid, s->main_pid.pid); + log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID "PID_FMT, pid->pid, s->main_pid.pid); else if (pidref_is_set(&s->control_pid)) - log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for control PID "PID_FMT, pid, s->control_pid.pid); + log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for control PID "PID_FMT, pid->pid, s->control_pid.pid); else - log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID and control PID which are currently not known", pid); + log_unit_debug(UNIT(s), "Got notification message from PID "PID_FMT", but reception only permitted for main PID and control PID which are currently not known", pid->pid); return false; @@ -4389,6 +4389,7 @@ static bool service_notify_message_authorized(Service *s, pid_t pid) { static void service_notify_message( Unit *u, + PidRef *pidref, const struct ucred *ucred, char * const *tags, FDSet *fds) { @@ -4396,14 +4397,15 @@ static void service_notify_message( Service *s = ASSERT_PTR(SERVICE(u)); int r; + assert(pidref_is_set(pidref)); assert(ucred); - if (!service_notify_message_authorized(s, ucred->pid)) + if (!service_notify_message_authorized(s, pidref)) return; if (DEBUG_LOGGING) { _cleanup_free_ char *cc = strv_join(tags, ", "); - log_unit_debug(u, "Got notification message from PID "PID_FMT": %s", ucred->pid, empty_to_na(cc)); + log_unit_debug(u, "Got notification message from PID "PID_FMT": %s", pidref->pid, empty_to_na(cc)); } usec_t monotonic_usec = USEC_INFINITY; diff --git a/src/core/unit.h b/src/core/unit.h index 31a1a13df1..4c900430b4 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -630,7 +630,7 @@ typedef struct UnitVTable { void (*notify_cgroup_oom)(Unit *u, bool managed_oom); /* Called whenever a process of this unit sends us a message */ - void (*notify_message)(Unit *u, const struct ucred *ucred, char * const *tags, FDSet *fds); + void (*notify_message)(Unit *u, PidRef *pidref, const struct ucred *ucred, char * const *tags, FDSet *fds); /* Called whenever we learn a handoff timestamp */ void (*notify_handoff_timestamp)(Unit *u, const struct ucred *ucred, const dual_timestamp *ts);