From d4e5c66ed469c822ca5346c7a445ec1446b1d17f Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 23 Jul 2024 17:55:12 +0200 Subject: [PATCH 1/4] core: reliably check if varlink socket has been deserialized Follow-up for 6906c028e83b77b35eaaf87b27d0fe5c6e1984b7 The mentioned commit uses access() to check if varlink socket already exists in the filesystem, but that isn't sufficient. > Varlink sockets are not serialized until v252, so upgrading from > v251 or older means we will not listen anymore on the varlink sockets. > > See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1074789 > for more details as this was found when updating from Debian Bullseye to a new version. After this commit, the set up of varlink_server is effectively split into two steps. manager_varlink_init_system(), which is called after deserialization, would no longer skip listening even if Manager.varlink_server is in place, but actually check if we're listening on desired sockets. Then, manager_deserialize() can be switched back to using manager_setup_varlink_server(). Alternative to #33817 Co-authored-by: Luca Boccassi --- src/core/core-varlink.c | 50 ++++++++++++++++++++---------------- src/core/core-varlink.h | 2 ++ src/core/manager-serialize.c | 2 +- 3 files changed, 31 insertions(+), 23 deletions(-) diff --git a/src/core/core-varlink.c b/src/core/core-varlink.c index a336430bf4..b22b8fb0c2 100644 --- a/src/core/core-varlink.c +++ b/src/core/core-varlink.c @@ -7,6 +7,7 @@ #include "mkdir-label.h" #include "strv.h" #include "user-util.h" +#include "varlink-internal.h" #include "varlink-io.systemd.UserDatabase.h" #include "varlink-io.systemd.ManagedOOM.h" @@ -500,12 +501,17 @@ static void vl_disconnect(sd_varlink_server *s, sd_varlink *link, void *userdata m->managed_oom_varlink = sd_varlink_unref(link); } -static int manager_setup_varlink_server(Manager *m, sd_varlink_server **ret) { +int manager_setup_varlink_server(Manager *m) { _cleanup_(sd_varlink_server_unrefp) sd_varlink_server *s = NULL; int r; assert(m); - assert(ret); + + if (m->varlink_server) + return 0; + + if (!MANAGER_IS_SYSTEM(m)) + return -EINVAL; r = sd_varlink_server_new(&s, SD_VARLINK_SERVER_ACCOUNT_UID|SD_VARLINK_SERVER_INHERIT_USERDATA); if (r < 0) @@ -533,51 +539,51 @@ static int manager_setup_varlink_server(Manager *m, sd_varlink_server **ret) { if (r < 0) return log_debug_errno(r, "Failed to register varlink disconnect handler: %m"); - *ret = TAKE_PTR(s); - return 0; + r = sd_varlink_server_attach_event(s, m->event, EVENT_PRIORITY_IPC); + if (r < 0) + return log_debug_errno(r, "Failed to attach varlink connection to event loop: %m"); + + m->varlink_server = TAKE_PTR(s); + return 1; } static int manager_varlink_init_system(Manager *m) { - _cleanup_(sd_varlink_server_unrefp) sd_varlink_server *s = NULL; int r; assert(m); - if (m->varlink_server) - return 1; - if (!MANAGER_IS_SYSTEM(m)) return 0; - r = manager_setup_varlink_server(m, &s); + r = manager_setup_varlink_server(m); if (r < 0) return log_error_errno(r, "Failed to set up varlink server: %m"); + bool fresh = r > 0; if (!MANAGER_IS_TEST_RUN(m)) { (void) mkdir_p_label("/run/systemd/userdb", 0755); FOREACH_STRING(address, "/run/systemd/userdb/io.systemd.DynamicUser", VARLINK_ADDR_PATH_MANAGED_OOM_SYSTEM) { - if (MANAGER_IS_RELOADING(m)) { - /* If manager is reloading, we skip listening on existing addresses, since - * the fd should be acquired later through deserialization. */ - if (access(address, F_OK) >= 0) + if (!fresh) { + /* We might have got sockets through deserialization. Do not bind to them twice. */ + + bool found = false; + LIST_FOREACH(sockets, ss, m->varlink_server->sockets) + if (path_equal(ss->address, address)) { + found = true; + break; + } + + if (found) continue; - if (errno != ENOENT) - return log_error_errno(errno, - "Failed to check if varlink socket '%s' exists: %m", address); } - r = sd_varlink_server_listen_address(s, address, 0666); + r = sd_varlink_server_listen_address(m->varlink_server, address, 0666); if (r < 0) return log_error_errno(r, "Failed to bind to varlink socket '%s': %m", address); } } - r = sd_varlink_server_attach_event(s, m->event, EVENT_PRIORITY_IPC); - if (r < 0) - return log_error_errno(r, "Failed to attach varlink connection to event loop: %m"); - - m->varlink_server = TAKE_PTR(s); return 1; } diff --git a/src/core/core-varlink.h b/src/core/core-varlink.h index 20507a4187..4b77620028 100644 --- a/src/core/core-varlink.h +++ b/src/core/core-varlink.h @@ -3,6 +3,8 @@ #include "manager.h" +int manager_setup_varlink_server(Manager *m); + int manager_varlink_init(Manager *m); void manager_varlink_done(Manager *m); diff --git a/src/core/manager-serialize.c b/src/core/manager-serialize.c index 8d998d40c1..62dfce93a0 100644 --- a/src/core/manager-serialize.c +++ b/src/core/manager-serialize.c @@ -508,7 +508,7 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { return r; } else if ((val = startswith(l, "varlink-server-socket-address="))) { if (!m->varlink_server && MANAGER_IS_SYSTEM(m)) { - r = manager_varlink_init(m); + r = manager_setup_varlink_server(m); if (r < 0) { log_warning_errno(r, "Failed to setup varlink server, ignoring: %m"); continue; From 16d8249ae4c704450f3fe944cb00ae246f133e8c Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 23 Jul 2024 17:01:01 +0200 Subject: [PATCH 2/4] core-varlink: add missing runtime_scope check for manager_varlink_init_user() --- src/core/core-varlink.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/core/core-varlink.c b/src/core/core-varlink.c index b22b8fb0c2..9051ab3111 100644 --- a/src/core/core-varlink.c +++ b/src/core/core-varlink.c @@ -619,6 +619,9 @@ static int manager_varlink_init_user(Manager *m) { if (m->managed_oom_varlink) return 1; + if (!MANAGER_IS_USER(m)) + return 0; + if (MANAGER_IS_TEST_RUN(m)) return 0; From 3ff91850a5ae783dc70416025908b4eac211c199 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 23 Jul 2024 17:06:57 +0200 Subject: [PATCH 3/4] core-varlink: do not log about ENOENT if oomd isn't available This is simply too noisy, since every invocation of manager_varlink_send_managed_oom_update() would try to connect to oomd if not already. --- src/core/core-varlink.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/core/core-varlink.c b/src/core/core-varlink.c index 9051ab3111..93dfdd2e26 100644 --- a/src/core/core-varlink.c +++ b/src/core/core-varlink.c @@ -626,13 +626,14 @@ static int manager_varlink_init_user(Manager *m) { return 0; r = sd_varlink_connect_address(&link, VARLINK_ADDR_PATH_MANAGED_OOM_USER); - if (r < 0) { - if (r == -ENOENT || ERRNO_IS_DISCONNECT(r)) { - log_debug("systemd-oomd varlink unix socket not found, skipping user manager varlink setup"); - return 0; - } - return log_error_errno(r, "Failed to connect to %s: %m", VARLINK_ADDR_PATH_MANAGED_OOM_USER); + if (r == -ENOENT) + return 0; + if (ERRNO_IS_NEG_DISCONNECT(r)) { + log_debug_errno(r, "systemd-oomd varlink socket isn't available, skipping user manager varlink setup: %m"); + return 0; } + if (r < 0) + return log_error_errno(r, "Failed to connect to '%s': %m", VARLINK_ADDR_PATH_MANAGED_OOM_USER); sd_varlink_set_userdata(link, m); From b4ff314ea08b2128ca2ee203b746744c5e64ac89 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 23 Jul 2024 17:14:39 +0200 Subject: [PATCH 4/4] core-varlink: switch to PidRef + manager_get_unit_by_pidref() --- src/core/core-varlink.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/core/core-varlink.c b/src/core/core-varlink.c index 93dfdd2e26..ba6d50b504 100644 --- a/src/core/core-varlink.c +++ b/src/core/core-varlink.c @@ -10,6 +10,7 @@ #include "varlink-internal.h" #include "varlink-io.systemd.UserDatabase.h" #include "varlink-io.systemd.ManagedOOM.h" +#include "varlink-util.h" typedef struct LookupParameters { const char *user_name; @@ -218,19 +219,18 @@ static int vl_method_subscribe_managed_oom_cgroups( sd_varlink_method_flags_t flags, void *userdata) { - _cleanup_(sd_json_variant_unrefp) sd_json_variant *v = NULL; Manager *m = ASSERT_PTR(userdata); - pid_t pid; + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; Unit *u; int r; assert(link); - r = sd_varlink_get_peer_pid(link, &pid); + r = varlink_get_peer_pidref(link, &pidref); if (r < 0) return r; - u = manager_get_unit_by_pid(m, pid); + u = manager_get_unit_by_pidref(m, &pidref); if (!u) return sd_varlink_error(link, SD_VARLINK_ERROR_PERMISSION_DENIED, NULL); @@ -247,6 +247,8 @@ static int vl_method_subscribe_managed_oom_cgroups( if (FLAGS_SET(flags, SD_VARLINK_METHOD_MORE) && m->managed_oom_varlink) return sd_varlink_error(link, "io.systemd.ManagedOOM.SubscriptionTaken", NULL); + _cleanup_(sd_json_variant_unrefp) sd_json_variant *v = NULL; + r = build_managed_oom_cgroups_json(m, &v); if (r < 0) return r;