From 3b3875ead34bdd14b853e9c77565647244263fa0 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 23 Jul 2024 17:55:12 +0200 Subject: [PATCH] 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 (cherry picked from commit d4e5c66ed469c822ca5346c7a445ec1446b1d17f) (cherry picked from commit b825a8be0b7b857a715e982cee861e8ae6995ee8) Conflict:NA Reference:https://github.com/systemd/systemd-stable/commit/3b3875ead34bdd14b853e9c77565647244263fa0 --- src/core/core-varlink.c | 50 ++++++++++++++++++++--------------- src/core/core-varlink.h | 2 +- src/core/manager-serialize.c | 2 +- src/shared/varlink-internal.h | 46 ++++++++++++++++++++++++++++++++ src/shared/varlink.c | 46 -------------------------------- 5 files changed, 76 insertions(+), 70 deletions(-) diff --git a/src/core/core-varlink.c b/src/core/core-varlink.c index cd913817d2..5cac0e2b06 100644 --- a/src/core/core-varlink.c +++ b/src/core/core-varlink.c @@ -5,6 +5,7 @@ #include "strv.h" #include "user-util.h" #include "varlink.h" +#include "varlink-internal.h" #include "varlink-io.systemd.UserDatabase.h" #include "varlink-io.systemd.ManagedOOM.h" @@ -492,46 +493,42 @@ static void vl_disconnect(VarlinkServer *s, Varlink *link, void *userdata) { } static int manager_varlink_init_system(Manager *m) { - _cleanup_(varlink_server_unrefp) VarlinkServer *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 = varlink_server_listen_address(s, address, 0666); + r = 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 = varlink_server_attach_event(s, m->event, SD_EVENT_PRIORITY_NORMAL); - 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; } @@ -597,12 +594,17 @@ static int manager_varlink_init_user(Manager *m) { return 1; } -int manager_setup_varlink_server(Manager *m, VarlinkServer **ret) { +int manager_setup_varlink_server(Manager *m) { _cleanup_(varlink_server_unrefp) VarlinkServer *s = NULL; int r; assert(m); - assert(ret); + + if (m->varlink_server) + return 0; + + if (!MANAGER_IS_SYSTEM(m)) + return -EINVAL; r = varlink_server_new(&s, VARLINK_SERVER_ACCOUNT_UID|VARLINK_SERVER_INHERIT_USERDATA); if (r < 0) @@ -630,8 +632,12 @@ int manager_setup_varlink_server(Manager *m, VarlinkServer **ret) { if (r < 0) return log_debug_errno(r, "Failed to register varlink disconnect handler: %m"); - *ret = TAKE_PTR(s); - return 0; + r = varlink_server_attach_event(s, m->event, SD_EVENT_PRIORITY_NORMAL); + 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; } int manager_varlink_init(Manager *m) { diff --git a/src/core/core-varlink.h b/src/core/core-varlink.h index 7f810d1f25..e053332877 100644 --- a/src/core/core-varlink.h +++ b/src/core/core-varlink.h @@ -8,7 +8,7 @@ void manager_varlink_done(Manager *m); /* Creates a new VarlinkServer and binds methods. Does not set up sockets or attach events. * Used for manager serialize/deserialize. */ -int manager_setup_varlink_server(Manager *m, VarlinkServer **ret_s); +int manager_setup_varlink_server(Manager *m); /* The manager is expected to send an update to systemd-oomd if one of the following occurs: * - The value of ManagedOOM*= properties change diff --git a/src/core/manager-serialize.c b/src/core/manager-serialize.c index 1ac26360a7..42f684e171 100644 --- a/src/core/manager-serialize.c +++ b/src/core/manager-serialize.c @@ -499,7 +499,7 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { return -ENOMEM; } 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; diff --git a/src/shared/varlink-internal.h b/src/shared/varlink-internal.h index 715202a49e..cb1888ab9e 100644 --- a/src/shared/varlink-internal.h +++ b/src/shared/varlink-internal.h @@ -6,5 +6,51 @@ #include "fdset.h" #include "varlink.h" +typedef struct VarlinkServerSocket VarlinkServerSocket; + +struct VarlinkServerSocket { + VarlinkServer *server; + + int fd; + char *address; + + sd_event_source *event_source; + + LIST_FIELDS(VarlinkServerSocket, sockets); +}; + +struct VarlinkServer { + unsigned n_ref; + VarlinkServerFlags flags; + + LIST_HEAD(VarlinkServerSocket, sockets); + + Hashmap *methods; /* Fully qualified symbol name of a method → VarlinkMethod */ + Hashmap *interfaces; /* Fully qualified interface name → VarlinkInterface* */ + Hashmap *symbols; /* Fully qualified symbol name of method/error → VarlinkSymbol* */ + VarlinkConnect connect_callback; + VarlinkDisconnect disconnect_callback; + + sd_event *event; + int64_t event_priority; + + unsigned n_connections; + Hashmap *by_uid; /* UID_TO_PTR(uid) → UINT_TO_PTR(n_connections) */ + + void *userdata; + char *description; + + unsigned connections_max; + unsigned connections_per_uid_max; + + bool exit_on_idle; +}; + +typedef struct VarlinkCollectContext { + JsonVariant *parameters; + const char *error_id; + VarlinkReplyFlags flags; +} VarlinkCollectContext ; + int varlink_server_serialize(VarlinkServer *s, FILE *f, FDSet *fds); int varlink_server_deserialize_one(VarlinkServer *s, const char *value, FDSet *fds); diff --git a/src/shared/varlink.c b/src/shared/varlink.c index 782458dfb9..ede8099ea7 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -201,52 +201,6 @@ struct Varlink { pid_t exec_pid; }; -typedef struct VarlinkServerSocket VarlinkServerSocket; - -struct VarlinkServerSocket { - VarlinkServer *server; - - int fd; - char *address; - - sd_event_source *event_source; - - LIST_FIELDS(VarlinkServerSocket, sockets); -}; - -struct VarlinkServer { - unsigned n_ref; - VarlinkServerFlags flags; - - LIST_HEAD(VarlinkServerSocket, sockets); - - Hashmap *methods; /* Fully qualified symbol name of a method → VarlinkMethod */ - Hashmap *interfaces; /* Fully qualified interface name → VarlinkInterface* */ - Hashmap *symbols; /* Fully qualified symbol name of method/error → VarlinkSymbol* */ - VarlinkConnect connect_callback; - VarlinkDisconnect disconnect_callback; - - sd_event *event; - int64_t event_priority; - - unsigned n_connections; - Hashmap *by_uid; /* UID_TO_PTR(uid) → UINT_TO_PTR(n_connections) */ - - void *userdata; - char *description; - - unsigned connections_max; - unsigned connections_per_uid_max; - - bool exit_on_idle; -}; - -typedef struct VarlinkCollectContext { - JsonVariant *parameters; - const char *error_id; - VarlinkReplyFlags flags; -} VarlinkCollectContext ; - static const char* const varlink_state_table[_VARLINK_STATE_MAX] = { [VARLINK_IDLE_CLIENT] = "idle-client", [VARLINK_AWAITING_REPLY] = "awaiting-reply", -- 2.33.0