From f220d98945d0a365f71814a3c92157ed129e84f0 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 25 Aug 2024 23:29:26 +0200 Subject: [PATCH 1/5] core/socket: use UNIT_ISSET rather than _DEREF where suitable --- src/core/socket.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/socket.c b/src/core/socket.c index 9cb188abc79..3c866cee935 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -328,7 +328,7 @@ static int socket_add_extras(Socket *s) { if (have_non_accept_socket(s)) { - if (!UNIT_DEREF(s->service)) { + if (!UNIT_ISSET(s->service)) { Unit *x; r = unit_load_related_unit(u, ".service", &x); @@ -416,7 +416,7 @@ static int socket_verify(Socket *s) { if (s->accept && s->max_connections <= 0) return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "MaxConnection= setting too small. Refusing."); - if (s->accept && UNIT_DEREF(s->service)) + if (s->accept && UNIT_ISSET(s->service)) return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "Explicit service configuration for accepting socket units not supported. Refusing."); if (s->exec_context.pam_name && s->kill_context.kill_mode != KILL_CONTROL_GROUP) From bca5505839c33549040c2ed22e751ba9edf81728 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 25 Aug 2024 23:30:47 +0200 Subject: [PATCH 2/5] core/socket: refuse MaxConnection=0 for Accept=no sockets too This makes no sense at all, and we already refuse such setting for Accept=yes sockets. I see no reason not to extend this to Accept=no ones. --- src/core/socket.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/socket.c b/src/core/socket.c index 3c866cee935..88c92876a14 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -410,12 +410,12 @@ static int socket_verify(Socket *s) { if (!s->ports) return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "Unit has no Listen setting (ListenStream=, ListenDatagram=, ListenFIFO=, ...). Refusing."); + if (s->max_connections <= 0) + return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "MaxConnection= setting too small. Refusing."); + if (s->accept && have_non_accept_socket(s)) return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "Unit configured for accepting sockets, but sockets are non-accepting. Refusing."); - if (s->accept && s->max_connections <= 0) - return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "MaxConnection= setting too small. Refusing."); - if (s->accept && UNIT_ISSET(s->service)) return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "Explicit service configuration for accepting socket units not supported. Refusing."); From 2234032c47f0abaf59e93b2a42ee926f9233f05c Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 25 Aug 2024 23:42:07 +0200 Subject: [PATCH 3/5] core: move check for combination of PAMName= + KillMode= to unit_verify_contexts() While at it, allow "mixed" for all unit types too, i.e. also apply ebc2259da1d1579347b86fc2ebca9f96334b6f22 to socket/mount/swap units. --- src/core/mount.c | 3 --- src/core/service.c | 3 --- src/core/socket.c | 3 --- src/core/swap.c | 3 --- src/core/unit.c | 10 ++++++++-- src/core/unit.h | 12 ++++++------ 6 files changed, 14 insertions(+), 20 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 3f53b2be7bc..28701df231b 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -574,9 +574,6 @@ static int mount_verify(Mount *m) { if (p && !p->what && !UNIT(m)->perpetual) return log_unit_error_errno(UNIT(m), SYNTHETIC_ERRNO(ENOEXEC), "What= setting is missing. Refusing."); - if (m->exec_context.pam_name && m->kill_context.kill_mode != KILL_CONTROL_GROUP) - return log_unit_error_errno(UNIT(m), SYNTHETIC_ERRNO(ENOEXEC), "Unit has PAM enabled. Kill mode must be set to control-group'. Refusing."); - return 0; } diff --git a/src/core/service.c b/src/core/service.c index ca6040a0557..eda355ad9ed 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -683,9 +683,6 @@ static int service_verify(Service *s) { if (s->type == SERVICE_DBUS && !s->bus_name) return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "Service is of type D-Bus but no D-Bus service name has been specified. Refusing."); - if (s->exec_context.pam_name && !IN_SET(s->kill_context.kill_mode, KILL_CONTROL_GROUP, KILL_MIXED)) - return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "Service has PAM enabled. Kill mode must be set to 'control-group' or 'mixed'. Refusing."); - if (s->usb_function_descriptors && !s->usb_function_strings) log_unit_warning(UNIT(s), "Service has USBFunctionDescriptors= setting, but no USBFunctionStrings=. Ignoring."); diff --git a/src/core/socket.c b/src/core/socket.c index 88c92876a14..333079277b3 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -419,9 +419,6 @@ static int socket_verify(Socket *s) { if (s->accept && UNIT_ISSET(s->service)) return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "Explicit service configuration for accepting socket units not supported. Refusing."); - if (s->exec_context.pam_name && s->kill_context.kill_mode != KILL_CONTROL_GROUP) - return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "Unit has PAM enabled. Kill mode must be set to 'control-group'. Refusing."); - if (!strv_isempty(s->symlinks) && !socket_find_symlink_target(s)) return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "Unit has symlinks set but none or more than one node in the file system. Refusing."); diff --git a/src/core/swap.c b/src/core/swap.c index 3b76ae33a3c..ff6c4255ab7 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -256,9 +256,6 @@ static int swap_verify(Swap *s) { if (!unit_has_name(UNIT(s), e)) return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "Value of What= and unit name do not match, not loading."); - if (s->exec_context.pam_name && s->kill_context.kill_mode != KILL_CONTROL_GROUP) - return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "Unit has PAM enabled. Kill mode must be set to 'control-group'. Refusing to load."); - return 0; } diff --git a/src/core/unit.c b/src/core/unit.c index 5d8b940608c..b52ee90936f 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4216,9 +4216,10 @@ static int user_from_unit_name(Unit *u, char **ret) { return 0; } -static int unit_verify_contexts(const Unit *u, const ExecContext *ec) { +static int unit_verify_contexts(const Unit *u) { assert(u); + const ExecContext *ec = unit_get_exec_context(u); if (!ec) return 0; @@ -4232,6 +4233,11 @@ static int unit_verify_contexts(const Unit *u, const ExecContext *ec) { exec_needs_mount_namespace(ec, /* params = */ NULL, /* runtime = */ NULL)) return log_unit_error_errno(u, SYNTHETIC_ERRNO(ENOEXEC), "WorkingDirectory= may not be below /proc/, /sys/ or /dev/ when using mount namespacing. Refusing."); + const KillContext *kc = unit_get_kill_context(u); + + if (ec->pam_name && kc && !IN_SET(kc->kill_mode, KILL_CONTROL_GROUP, KILL_MIXED)) + return log_unit_error_errno(u, SYNTHETIC_ERRNO(ENOEXEC), "Unit has PAM enabled. Kill mode must be set to 'control-group' or 'mixed'. Refusing."); + return 0; } @@ -4362,7 +4368,7 @@ int unit_patch_contexts(Unit *u) { } } - return unit_verify_contexts(u, ec); + return unit_verify_contexts(u); } ExecContext *unit_get_exec_context(const Unit *u) { diff --git a/src/core/unit.h b/src/core/unit.h index cabf7cc4219..04a4189de38 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -926,15 +926,15 @@ void unit_ref_unset(UnitRef *ref); int unit_patch_contexts(Unit *u); -ExecContext *unit_get_exec_context(const Unit *u) _pure_; -KillContext *unit_get_kill_context(const Unit *u) _pure_; -CGroupContext *unit_get_cgroup_context(const Unit *u) _pure_; +ExecContext* unit_get_exec_context(const Unit *u) _pure_; +KillContext* unit_get_kill_context(const Unit *u) _pure_; +CGroupContext* unit_get_cgroup_context(const Unit *u) _pure_; -ExecRuntime *unit_get_exec_runtime(const Unit *u) _pure_; -CGroupRuntime *unit_get_cgroup_runtime(const Unit *u) _pure_; +ExecRuntime* unit_get_exec_runtime(const Unit *u) _pure_; +CGroupRuntime* unit_get_cgroup_runtime(const Unit *u) _pure_; int unit_setup_exec_runtime(Unit *u); -CGroupRuntime *unit_setup_cgroup_runtime(Unit *u); +CGroupRuntime* unit_setup_cgroup_runtime(Unit *u); const char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf); char* unit_concat_strv(char **l, UnitWriteFlags flags); From daa78907af9ea3bfdce937ffd4df2a87dd720fcf Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 25 Aug 2024 23:21:47 +0200 Subject: [PATCH 4/5] core: honor FileDescriptorName= too for Accept=yes sockets So far we manually hardcoded $LISTEN_FDNAMES to "varlink" in various varlink service units we ship, even though FileDescriptorName=varlink is specified in associated socket units already, because FileDescriptorName= is currently silently ignored when combined with Accept=yes. Let's step away from this, which seems saner. Note that this is technically a compat break, but a mostly negligible one as there shall be few users setting FileDescriptorName= but still expecting LISTEN_FDNAMES=connection in the actual executable. Preparation for #34080 --- NEWS | 4 ++++ man/systemd.socket.xml | 19 ++++++++----------- src/core/service.c | 3 ++- src/core/socket.c | 22 +++++++++++++--------- src/core/socket.h | 2 +- 5 files changed, 28 insertions(+), 22 deletions(-) diff --git a/NEWS b/NEWS index 947b831001d..3bc3c721217 100644 --- a/NEWS +++ b/NEWS @@ -31,6 +31,10 @@ CHANGES WITH 257 in spe: by default when combined with --scope, will be changed in a future release to be enabled by default. + * The FileDescriptorName= setting for socket units is now honored by + Accept=yes sockets too, where it was previously silently ignored and + "connection" was used unconditionally. + * systemd-logind now always obeys inhibitor locks, where previously it ignored locks taken by the caller or when the caller was root. A privileged caller can always close the other sessions, remove the diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml index 67384bfcc4d..e9fd39bf7f2 100644 --- a/man/systemd.socket.xml +++ b/man/systemd.socket.xml @@ -853,18 +853,15 @@ FileDescriptorName= - Assigns a name to all file descriptors this - socket unit encapsulates. This is useful to help activated - services identify specific file descriptors, if multiple fds - are passed. Services may use the + Assigns a name to all file descriptors this socket unit encapsulates. + This is useful to help activated services identify specific file descriptors, if multiple fds are passed. + Services may use the sd_listen_fds_with_names3 - call to acquire the names configured for the received file - descriptors. Names may contain any ASCII character, but must - exclude control characters and :, and must - be at most 255 characters in length. If this setting is not - used, the file descriptor name defaults to the name of the - socket unit, including its .socket - suffix. + call to acquire the names configured for the received file descriptors. Names may contain any ASCII character, + but must exclude control characters and :, and must be at most 255 characters in length. + If this setting is not used, the file descriptor name defaults to the name of the socket unit + (including its .socket suffix) when Accept=no, + connection otherwise. diff --git a/src/core/service.c b/src/core/service.c index eda355ad9ed..6d3118b9639 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1426,6 +1426,7 @@ static int service_collect_fds( assert(n_storage_fds); if (s->socket_fd >= 0) { + Socket *sock = ASSERT_PTR(SOCKET(UNIT_DEREF(s->accept_socket))); /* Pass the per-connection socket */ @@ -1433,7 +1434,7 @@ static int service_collect_fds( if (!rfds) return -ENOMEM; - rfd_names = strv_new("connection"); + rfd_names = strv_new(socket_fdname(sock)); if (!rfd_names) return -ENOMEM; diff --git a/src/core/socket.c b/src/core/socket.c index 333079277b3..a1553bcc68b 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -2393,10 +2393,9 @@ static void socket_enter_running(Socket *s, int cfd_in) { s->n_accepted++; r = service_set_socket_fd(SERVICE(service), cfd, s, p, s->selinux_context_from_net); + if (ERRNO_IS_NEG_DISCONNECT(r)) + return; if (r < 0) { - if (ERRNO_IS_DISCONNECT(r)) - return; - log_unit_warning_errno(UNIT(s), r, "Failed to set socket on service: %m"); goto fail; } @@ -3419,17 +3418,22 @@ static int socket_get_timeout(Unit *u, usec_t *timeout) { return 1; } -char* socket_fdname(Socket *s) { +const char* socket_fdname(Socket *s) { assert(s); - /* Returns the name to use for $LISTEN_NAMES. If the user - * didn't specify anything specifically, use the socket unit's - * name as fallback. */ + /* Returns the name to use for $LISTEN_FDNAMES. If the user didn't specify anything specifically, + * use the socket unit's name as fallback for Accept=no sockets, "connection" otherwise. */ - return s->fdname ?: UNIT(s)->id; + if (s->fdname) + return s->fdname; + + if (s->accept) + return "connection"; + + return UNIT(s)->id; } -static PidRef *socket_control_pid(Unit *u) { +static PidRef* socket_control_pid(Unit *u) { return &ASSERT_PTR(SOCKET(u))->control_pid; } diff --git a/src/core/socket.h b/src/core/socket.h index d4b39d559d8..f21fc7f5446 100644 --- a/src/core/socket.h +++ b/src/core/socket.h @@ -185,7 +185,7 @@ int socket_port_to_address(const SocketPort *s, char **ret); int socket_load_service_unit(Socket *s, int cfd, Unit **ret); -char* socket_fdname(Socket *s); +const char* socket_fdname(Socket *s); extern const UnitVTable socket_vtable; From e7c8d78e7f4c67064f8e934e2022a5a1a51f876c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 22 Aug 2024 11:49:25 +0200 Subject: [PATCH 5/5] units: don't set LISTEN_FDNAMES for varlink services explicitly Now that FileDescriptorName= is properly honored by Accept=yes sockets, this explicit override is pointless. --- units/systemd-bootctl@.service | 1 - units/systemd-creds@.service | 1 - units/systemd-pcrextend@.service.in | 1 - units/systemd-pcrlock@.service.in | 1 - units/systemd-sysext@.service | 1 - 5 files changed, 5 deletions(-) diff --git a/units/systemd-bootctl@.service b/units/systemd-bootctl@.service index 5de61566380..3f49e104573 100644 --- a/units/systemd-bootctl@.service +++ b/units/systemd-bootctl@.service @@ -16,5 +16,4 @@ After=local-fs.target Before=shutdown.target [Service] -Environment=LISTEN_FDNAMES=varlink ExecStart=bootctl diff --git a/units/systemd-creds@.service b/units/systemd-creds@.service index d565836d3ad..014444161f3 100644 --- a/units/systemd-creds@.service +++ b/units/systemd-creds@.service @@ -15,5 +15,4 @@ Conflicts=shutdown.target initrd-switch-root.target Before=shutdown.target initrd-switch-root.target [Service] -Environment=LISTEN_FDNAMES=varlink ExecStart=-systemd-creds diff --git a/units/systemd-pcrextend@.service.in b/units/systemd-pcrextend@.service.in index 68b71d40687..79841a5fab1 100644 --- a/units/systemd-pcrextend@.service.in +++ b/units/systemd-pcrextend@.service.in @@ -16,5 +16,4 @@ Conflicts=shutdown.target initrd-switch-root.target Before=shutdown.target initrd-switch-root.target [Service] -Environment=LISTEN_FDNAMES=varlink ExecStart=-{{LIBEXECDIR}}/systemd-pcrextend diff --git a/units/systemd-pcrlock@.service.in b/units/systemd-pcrlock@.service.in index 3528c565c8a..143d6c5154c 100644 --- a/units/systemd-pcrlock@.service.in +++ b/units/systemd-pcrlock@.service.in @@ -17,5 +17,4 @@ Before=sysinit.target shutdown.target After=systemd-remount-fs.service var.mount [Service] -Environment=LISTEN_FDNAMES=varlink ExecStart={{LIBEXECDIR}}/systemd-pcrlock --location=770 diff --git a/units/systemd-sysext@.service b/units/systemd-sysext@.service index 9dcbf9f1dd8..ec9c6b0d6db 100644 --- a/units/systemd-sysext@.service +++ b/units/systemd-sysext@.service @@ -16,5 +16,4 @@ Conflicts=shutdown.target initrd-switch-root.target Before=shutdown.target initrd-switch-root.target [Service] -Environment=LISTEN_FDNAMES=varlink ExecStart=-systemd-sysext