From 8ba3efed8669639457fc9b482d61d87e77f79b33 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Thu, 21 Mar 2024 15:48:54 +0100 Subject: [PATCH 1/2] logind: Add fallback for when the PIDFDs= property is not available logind is not zero-downtime restartable yet, specifically it's not yet restarted in the Fedora spec, so we can end up in situations where we're running newer logind with older pid1 which doesn't know about the PIDFDs= property, so let's make sure we have a fallback in place for when that happens. --- src/login/logind-dbus.c | 27 +++++++++++++++++++++++++-- src/login/logind-dbus.h | 1 + src/login/logind-session.c | 1 + 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index c342522e3f..678e077d90 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -4230,6 +4230,7 @@ int manager_start_scope( Manager *manager, const char *scope, const PidRef *pidref, + bool allow_pidfd, const char *slice, const char *description, const char * const *requires, @@ -4299,7 +4300,10 @@ int manager_start_scope( if (r < 0) return r; - r = bus_append_scope_pidref(m, pidref); + if (allow_pidfd) + r = bus_append_scope_pidref(m, pidref); + else + r = sd_bus_message_append(m, "(sv)", "PIDs", "au", 1, (uint32_t) pidref->pid); if (r < 0) return r; @@ -4330,8 +4334,27 @@ int manager_start_scope( return r; r = sd_bus_call(manager->bus, m, 0, error, &reply); - if (r < 0) + if (r < 0) { + /* If this failed with a property we couldn't write, this is quite likely because the server + * doesn't support PIDFDs yet, let's try without. */ + if (allow_pidfd && + sd_bus_error_has_names(error, SD_BUS_ERROR_UNKNOWN_PROPERTY, SD_BUS_ERROR_PROPERTY_READ_ONLY)) + return manager_start_scope( + manager, + scope, + pidref, + /* allow_pidfd = */ false, + slice, + description, + requires, + extra_after, + requires_mounts_for, + more_properties, + error, + ret_job); + return r; + } return strdup_job(reply, ret_job); } diff --git a/src/login/logind-dbus.h b/src/login/logind-dbus.h index 5c864a9bc0..8459d048b8 100644 --- a/src/login/logind-dbus.h +++ b/src/login/logind-dbus.h @@ -28,6 +28,7 @@ int manager_start_scope( Manager *manager, const char *scope, const PidRef *pidref, + bool allow_pidfd, const char *slice, const char *description, const char * const *requires, diff --git a/src/login/logind-session.c b/src/login/logind-session.c index 2713215e10..4713aa05b5 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -742,6 +742,7 @@ static int session_start_scope(Session *s, sd_bus_message *properties, sd_bus_er s->manager, scope, &s->leader, + /* allow_pidfd = */ true, s->user->slice, description, /* These should have been pulled in explicitly in user_start(). Just to be sure. */ From 66b574b0a278d810ea6367fec84f70318bc3ae44 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Fri, 22 Mar 2024 17:03:35 +0100 Subject: [PATCH 2/2] tree-wide: Add allow_pidfd argument to bus_append_scope_pidref() --- src/login/logind-dbus.c | 5 +---- src/machine/machine.c | 2 +- src/nspawn/nspawn-register.c | 13 +++++-------- src/run/run.c | 15 +++++---------- src/shared/bus-unit-util.c | 4 ++-- src/shared/bus-unit-util.h | 2 +- src/vmspawn/vmspawn-scope.c | 13 +++++-------- 7 files changed, 20 insertions(+), 34 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 678e077d90..5d27491af3 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -4300,10 +4300,7 @@ int manager_start_scope( if (r < 0) return r; - if (allow_pidfd) - r = bus_append_scope_pidref(m, pidref); - else - r = sd_bus_message_append(m, "(sv)", "PIDs", "au", 1, (uint32_t) pidref->pid); + r = bus_append_scope_pidref(m, pidref, allow_pidfd); if (r < 0) return r; diff --git a/src/machine/machine.c b/src/machine/machine.c index af8a88f26c..309bffa3c8 100644 --- a/src/machine/machine.c +++ b/src/machine/machine.c @@ -385,7 +385,7 @@ static int machine_start_scope( if (r < 0) return r; - r = bus_append_scope_pidref(m, &machine->leader); + r = bus_append_scope_pidref(m, &machine->leader, /* allow_pidfd = */ true); if (r < 0) return r; diff --git a/src/nspawn/nspawn-register.c b/src/nspawn/nspawn-register.c index 358958b82b..b63516d13e 100644 --- a/src/nspawn/nspawn-register.c +++ b/src/nspawn/nspawn-register.c @@ -297,15 +297,12 @@ int allocate_scope( description = strjoina("Container ", machine_name); - if (allow_pidfd) { - _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; - r = pidref_set_pid(&pidref, pid); - if (r < 0) - return log_error_errno(r, "Failed to allocate PID reference: %m"); + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; + r = pidref_set_pid(&pidref, pid); + if (r < 0) + return log_error_errno(r, "Failed to allocate PID reference: %m"); - r = bus_append_scope_pidref(m, &pidref); - } else - r = sd_bus_message_append(m, "(sv)", "PIDs", "au", 1, (uint32_t) pid); + r = bus_append_scope_pidref(m, &pidref, allow_pidfd); if (r < 0) return bus_log_create_error(r); diff --git a/src/run/run.c b/src/run/run.c index e8dc3388c0..93e061a3c3 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -1265,18 +1265,13 @@ static int transient_scope_set_properties(sd_bus_message *m, bool allow_pidfd) { if (r < 0) return r; - if (allow_pidfd) { - _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; - r = pidref_set_self(&pidref); - if (r < 0) - return r; + r = pidref_set_self(&pidref); + if (r < 0) + return r; - r = bus_append_scope_pidref(m, &pidref); - } else - r = sd_bus_message_append( - m, "(sv)", - "PIDs", "au", 1, getpid_cached()); + r = bus_append_scope_pidref(m, &pidref, allow_pidfd); if (r < 0) return bus_log_create_error(r); diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index 7d847b9425..2fcfb1d3b9 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -2821,13 +2821,13 @@ int bus_append_unit_property_assignment_many(sd_bus_message *m, UnitType t, char return 0; } -int bus_append_scope_pidref(sd_bus_message *m, const PidRef *pidref) { +int bus_append_scope_pidref(sd_bus_message *m, const PidRef *pidref, bool allow_pidfd) { assert(m); if (!pidref_is_set(pidref)) return -ESRCH; - if (pidref->fd >= 0) + if (pidref->fd >= 0 && allow_pidfd) return sd_bus_message_append( m, "(sv)", "PIDFDs", "ah", 1, pidref->fd); diff --git a/src/shared/bus-unit-util.h b/src/shared/bus-unit-util.h index a1518158ff..e4168a4425 100644 --- a/src/shared/bus-unit-util.h +++ b/src/shared/bus-unit-util.h @@ -26,7 +26,7 @@ int bus_parse_unit_info(sd_bus_message *message, UnitInfo *u); int bus_append_unit_property_assignment(sd_bus_message *m, UnitType t, const char *assignment); int bus_append_unit_property_assignment_many(sd_bus_message *m, UnitType t, char **l); -int bus_append_scope_pidref(sd_bus_message *m, const PidRef *pidref); +int bus_append_scope_pidref(sd_bus_message *m, const PidRef *pidref, bool allow_pidfd); int bus_deserialize_and_dump_unit_file_changes(sd_bus_message *m, bool quiet); diff --git a/src/vmspawn/vmspawn-scope.c b/src/vmspawn/vmspawn-scope.c index ff986b09d6..a8d27afa11 100644 --- a/src/vmspawn/vmspawn-scope.c +++ b/src/vmspawn/vmspawn-scope.c @@ -61,15 +61,12 @@ int start_transient_scope(sd_bus *bus, const char *machine_name, bool allow_pidf "AddRef", "b", 1, "CollectMode", "s", "inactive-or-failed"); - if (allow_pidfd) { - _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; - r = pidref_set_pid(&pidref, getpid_cached()); - if (r < 0) - return log_error_errno(r, "Failed to allocate PID reference: %m"); + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; + r = pidref_set_self(&pidref); + if (r < 0) + return log_error_errno(r, "Failed to allocate PID reference: %m"); - r = bus_append_scope_pidref(m, &pidref); - } else - r = sd_bus_message_append(m, "(sv)", "PIDs", "au", 1, getpid_cached()); + r = bus_append_scope_pidref(m, &pidref, allow_pidfd); if (r < 0) return bus_log_create_error(r);