From 9d5b6901007e6717c6a37c49eb73bc0260e93893 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 21 Nov 2023 17:25:23 +0800 Subject: [PATCH] logind: serialize session leader pidfd to fdstore --- src/login/logind-session.c | 138 ++++++++++++++++--------- src/login/logind-session.h | 2 + src/login/logind.c | 176 +++++++++++++++++++++----------- units/systemd-logind.service.in | 2 +- 4 files changed, 209 insertions(+), 109 deletions(-) diff --git a/src/login/logind-session.c b/src/login/logind-session.c index 7f007caf756..31d483ff305 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -15,6 +15,7 @@ #include "audit-util.h" #include "bus-error.h" #include "bus-util.h" +#include "daemon-util.h" #include "devnum-util.h" #include "env-file.h" #include "escape.h" @@ -87,9 +88,16 @@ int session_new(Session **ret, Manager *m, const char *id) { return 0; } -static void session_reset_leader(Session *s) { +static void session_reset_leader(Session *s, bool keep_fdstore) { assert(s); + if (!keep_fdstore) { + /* Clear fdstore if we're asked to, no matter if s->leader is set or not, so that when + * initially deserializing leader fd we clear the old fd too. */ + (void) notify_remove_fd_warnf("session-%s-leader-fd", s->id); + s->leader_fd_saved = false; + } + if (!pidref_is_set(&s->leader)) return; @@ -142,7 +150,7 @@ Session* session_free(Session *s) { free(s->scope_job); - session_reset_leader(s); + session_reset_leader(s, /* keep_fdstore = */ true); sd_bus_message_unref(s->create_message); @@ -188,7 +196,7 @@ int session_set_leader_consume(Session *s, PidRef _leader) { if (pidref_equal(&s->leader, &pidref)) return 0; - session_reset_leader(s); + session_reset_leader(s, /* keep_fdstore = */ false); s->leader = TAKE_PIDREF(pidref); @@ -197,6 +205,14 @@ int session_set_leader_consume(Session *s, PidRef _leader) { return r; assert(r > 0); + if (s->leader.fd >= 0) { + r = notify_push_fdf(s->leader.fd, "session-%s-leader-fd", s->id); + if (r < 0) + log_warning_errno(r, "Failed to push leader pidfd for session '%s', ignoring: %m", s->id); + else + s->leader_fd_saved = true; + } + (void) audit_session_from_pid(s->leader.pid, &s->audit_id); return 1; @@ -243,13 +259,15 @@ int session_save(Session *s) { "ACTIVE=%s\n" "IS_DISPLAY=%s\n" "STATE=%s\n" - "REMOTE=%s\n", + "REMOTE=%s\n" + "LEADER_FD_SAVED=%s\n", s->user->user_record->uid, s->user->user_record->user_name, one_zero(session_is_active(s)), one_zero(s->user->display == s), session_state_to_string(session_get_state(s)), - one_zero(s->remote)); + one_zero(s->remote), + one_zero(s->leader_fd_saved)); if (s->type >= 0) fprintf(f, "TYPE=%s\n", session_type_to_string(s->type)); @@ -408,7 +426,8 @@ int session_load(Session *s) { *vtnr = NULL, *state = NULL, *position = NULL, - *leader = NULL, + *leader_pid = NULL, + *leader_fd_saved = NULL, *type = NULL, *original_type = NULL, *class = NULL, @@ -425,32 +444,33 @@ int session_load(Session *s) { assert(s); r = parse_env_file(NULL, s->state_file, - "REMOTE", &remote, - "SCOPE", &s->scope, - "SCOPE_JOB", &s->scope_job, - "FIFO", &s->fifo_path, - "SEAT", &seat, - "TTY", &s->tty, - "TTY_VALIDITY", &tty_validity, - "DISPLAY", &s->display, - "REMOTE_HOST", &s->remote_host, - "REMOTE_USER", &s->remote_user, - "SERVICE", &s->service, - "DESKTOP", &s->desktop, - "VTNR", &vtnr, - "STATE", &state, - "POSITION", &position, - "LEADER", &leader, - "TYPE", &type, - "ORIGINAL_TYPE", &original_type, - "CLASS", &class, - "UID", &uid, - "REALTIME", &realtime, - "MONOTONIC", &monotonic, - "CONTROLLER", &controller, - "ACTIVE", &active, - "DEVICES", &devices, - "IS_DISPLAY", &is_display); + "REMOTE", &remote, + "SCOPE", &s->scope, + "SCOPE_JOB", &s->scope_job, + "FIFO", &s->fifo_path, + "SEAT", &seat, + "TTY", &s->tty, + "TTY_VALIDITY", &tty_validity, + "DISPLAY", &s->display, + "REMOTE_HOST", &s->remote_host, + "REMOTE_USER", &s->remote_user, + "SERVICE", &s->service, + "DESKTOP", &s->desktop, + "VTNR", &vtnr, + "STATE", &state, + "POSITION", &position, + "LEADER", &leader_pid, + "LEADER_FD_SAVED", &leader_fd_saved, + "TYPE", &type, + "ORIGINAL_TYPE", &original_type, + "CLASS", &class, + "UID", &uid, + "REALTIME", &realtime, + "MONOTONIC", &monotonic, + "CONTROLLER", &controller, + "ACTIVE", &active, + "DEVICES", &devices, + "IS_DISPLAY", &is_display); if (r < 0) return log_error_errno(r, "Failed to read %s: %m", s->state_file); @@ -517,19 +537,6 @@ int session_load(Session *s) { s->tty_validity = v; } - if (leader) { - _cleanup_(pidref_done) PidRef p = PIDREF_NULL; - - r = pidref_set_pidstr(&p, leader); - if (r < 0) - log_debug_errno(r, "Failed to parse leader PID of session: %s", leader); - else { - r = session_set_leader_consume(s, TAKE_PIDREF(p)); - if (r < 0) - log_warning_errno(r, "Failed to set session leader PID, ignoring: %m"); - } - } - if (type) { SessionType t; @@ -604,6 +611,33 @@ int session_load(Session *s) { session_restore_vt(s); } + if (leader_pid) { + assert(!pidref_is_set(&s->leader)); + + r = parse_pid(leader_pid, &s->deserialized_pid); + if (r < 0) + return log_error_errno(r, "Failed to parse LEADER=%s: %m", leader_pid); + + if (leader_fd_saved) { + r = parse_boolean(leader_fd_saved); + if (r < 0) + return log_error_errno(r, "Failed to parse LEADER_FD_SAVED=%s: %m", leader_fd_saved); + s->leader_fd_saved = r > 0; + + if (s->leader_fd_saved) + /* The leader fd will be acquired from fdstore later */ + return 0; + } + + _cleanup_(pidref_done) PidRef p = PIDREF_NULL; + + r = pidref_set_pid(&p, s->deserialized_pid); + if (r >= 0) + r = session_set_leader_consume(s, TAKE_PIDREF(p)); + if (r < 0) + log_warning_errno(r, "Failed to set leader PID for session '%s': %m", s->id); + } + return r; } @@ -942,7 +976,7 @@ int session_finalize(Session *s) { seat_save(s->seat); } - session_reset_leader(s); + session_reset_leader(s, /* keep_fdstore = */ false); user_save(s->user); user_send_changed(s->user, "Display", NULL); @@ -1267,15 +1301,17 @@ bool session_may_gc(Session *s, bool drop_not_started) { return true; r = pidref_is_alive(&s->leader); + if (r == -ESRCH) + /* Session has no leader. This is probably because the leader vanished before deserializing + * pidfd from FD store. */ + return true; if (r < 0) - log_debug_errno(r, "Unable to determine if leader PID " PID_FMT " is still alive, assuming not.", s->leader.pid); + log_debug_errno(r, "Unable to determine if leader PID " PID_FMT " is still alive, assuming not: %m", s->leader.pid); if (r > 0) return false; - if (s->fifo_fd >= 0) { - if (pipe_eof(s->fifo_fd) <= 0) - return false; - } + if (s->fifo_fd >= 0 && pipe_eof(s->fifo_fd) <= 0) + return false; if (s->scope_job) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; diff --git a/src/login/logind-session.h b/src/login/logind-session.h index 8b63843ed81..eaf9b4a6493 100644 --- a/src/login/logind-session.h +++ b/src/login/logind-session.h @@ -89,6 +89,8 @@ struct Session { int vtfd; PidRef leader; + bool leader_fd_saved; /* pidfd of leader uploaded to fdstore */ + pid_t deserialized_pid; /* PID deserialized from state file (for verification when pidfd is used) */ uint32_t audit_id; int fifo_fd; diff --git a/src/login/logind.c b/src/login/logind.c index e32e9e627d9..4124d3675c5 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -18,6 +18,7 @@ #include "constants.h" #include "daemon-util.h" #include "device-util.h" +#include "devnum-util.h" #include "dirent-util.h" #include "escape.h" #include "fd-util.h" @@ -349,116 +350,169 @@ static int manager_enumerate_users(Manager *m) { return r; } -static int parse_fdname(const char *fdname, char **session_id, dev_t *dev) { +static int parse_fdname(const char *fdname, char **ret_session_id, dev_t *ret_devno) { _cleanup_strv_free_ char **parts = NULL; _cleanup_free_ char *id = NULL; - unsigned major, minor; int r; + assert(ret_session_id); + assert(ret_devno); + parts = strv_split(fdname, "-"); if (!parts) return -ENOMEM; - if (strv_length(parts) != 5) - return -EINVAL; - if (!streq(parts[0], "session")) + if (_unlikely_(!streq(parts[0], "session"))) return -EINVAL; id = strdup(parts[1]); if (!id) return -ENOMEM; - if (!streq(parts[2], "device")) + if (streq(parts[2], "leader")) { + *ret_session_id = TAKE_PTR(id); + *ret_devno = 0; + + return 0; + } + + if (_unlikely_(!streq(parts[2], "device"))) return -EINVAL; + unsigned major, minor; + r = safe_atou(parts[3], &major); if (r < 0) return r; + r = safe_atou(parts[4], &minor); if (r < 0) return r; - *dev = makedev(major, minor); - *session_id = TAKE_PTR(id); + *ret_session_id = TAKE_PTR(id); + *ret_devno = makedev(major, minor); return 0; } -static int deliver_fd(Manager *m, const char *fdname, int fd) { - _cleanup_free_ char *id = NULL; +static int deliver_session_device_fd(Session *s, const char *fdname, int fd, dev_t devno) { SessionDevice *sd; struct stat st; - Session *s; - dev_t dev; - int r; - assert(m); + assert(s); + assert(fdname); assert(fd >= 0); - - r = parse_fdname(fdname, &id, &dev); - if (r < 0) - return log_debug_errno(r, "Failed to parse fd name %s: %m", fdname); - - s = hashmap_get(m->sessions, id); - if (!s) - /* If the session doesn't exist anymore, the associated session device attached to this fd - * doesn't either. Let's simply close this fd. */ - return log_debug_errno(SYNTHETIC_ERRNO(ENXIO), "Failed to attach fd for unknown session: %s", id); + assert(devno > 0); if (fstat(fd, &st) < 0) /* The device is allowed to go away at a random point, in which case fstat() failing is * expected. */ - return log_debug_errno(errno, "Failed to stat device fd for session %s: %m", id); + return log_debug_errno(errno, "Failed to stat device fd '%s' for session '%s': %m", + fdname, s->id); - if (!S_ISCHR(st.st_mode) || st.st_rdev != dev) - return log_debug_errno(SYNTHETIC_ERRNO(ENODEV), "Device fd doesn't point to the expected character device node"); + if (!S_ISCHR(st.st_mode) || st.st_rdev != devno) + return log_debug_errno(SYNTHETIC_ERRNO(ENODEV), + "Device fd '%s' doesn't point to the expected character device node.", + fdname); - sd = hashmap_get(s->devices, &dev); + sd = hashmap_get(s->devices, &devno); if (!sd) /* Weird, we got an fd for a session device which wasn't recorded in the session state * file... */ - return log_warning_errno(SYNTHETIC_ERRNO(ENODEV), "Got fd for missing session device [%u:%u] in session %s", - major(dev), minor(dev), s->id); + return log_warning_errno(SYNTHETIC_ERRNO(ENODEV), + "Got session device fd '%s' [" DEVNUM_FORMAT_STR "], but not present in session state.", + fdname, DEVNUM_FORMAT_VAL(devno)); - log_debug("Attaching fd to session device [%u:%u] for session %s", - major(dev), minor(dev), s->id); + log_debug("Attaching session device fd '%s' [" DEVNUM_FORMAT_STR "] to session '%s'.", + fdname, DEVNUM_FORMAT_VAL(devno), s->id); session_device_attach_fd(sd, fd, s->was_active); return 0; } -static int manager_attach_fds(Manager *m) { - _cleanup_strv_free_ char **fdnames = NULL; - int n; +static int deliver_session_leader_fd_consume(Session *s, const char *fdname, int fd) { + _cleanup_(pidref_done) PidRef leader_fdstore = PIDREF_NULL; + int r; - /* Upon restart, PID1 will send us back all fds of session devices that we previously opened. Each - * file descriptor is associated with a given session. The session ids are passed through FDNAMES. */ + assert(s); + assert(fdname); + assert(fd >= 0); - assert(m); + if (!s->leader_fd_saved) + log_warning("Got leader pidfd for session '%s', but not recorded in session state, proceeding anyway.", + s->id); + else + assert(!pidref_is_set(&s->leader)); - n = sd_listen_fds_with_names(true, &fdnames); - if (n < 0) - return log_warning_errno(n, "Failed to acquire passed fd list: %m"); - if (n == 0) - return 0; + r = pidref_set_pidfd_take(&leader_fdstore, fd); + if (r < 0) { + if (r == -ESRCH) + log_debug_errno(r, "Leader of session '%s' is gone while deserializing.", s->id); + else + log_warning_errno(r, "Failed to create reference to leader of session '%s': %m", s->id); - for (int i = 0; i < n; i++) { - int fd = SD_LISTEN_FDS_START + i; - - if (deliver_fd(m, fdnames[i], fd) >= 0) - continue; - - /* Hmm, we couldn't deliver the fd to any session device object? If so, let's close the fd - * and remove it from fdstore. */ - close_and_notify_warn(fd, fdnames[i]); + close_and_notify_warn(fd, fdname); + return r; } + assert(pid_is_valid(s->deserialized_pid)); + + if (leader_fdstore.pid != s->deserialized_pid) + log_warning("Leader from pidfd (" PID_FMT ") doesn't match with LEADER=" PID_FMT " for session '%s', proceeding anyway.", + leader_fdstore.pid, s->deserialized_pid, s->id); + + r = session_set_leader_consume(s, TAKE_PIDREF(leader_fdstore)); + if (r < 0) + return log_warning_errno(r, "Failed to attach leader pidfd for session '%s': %m", s->id); + + // FIXME: Maybe call session_watch_pidfd() here? + return 0; } +static int manager_attach_session_fd_one_consume(Manager *m, const char *fdname, int fd) { + _cleanup_free_ char *id = NULL; + dev_t devno; + Session *s; + int r; + + assert(m); + assert(fdname); + assert(fd >= 0); + + r = parse_fdname(fdname, &id, &devno); + if (r < 0) { + log_warning_errno(r, "Failed to parse stored fd name '%s': %m", fdname); + goto fail_close; + } + + s = hashmap_get(m->sessions, id); + if (!s) { + /* If the session doesn't exist anymore, let's simply close this fd. */ + r = log_debug_errno(SYNTHETIC_ERRNO(ENXIO), + "Cannot attach fd '%s' to unknown session '%s', ignoring.", fdname, id); + goto fail_close; + } + + if (devno > 0) { + r = deliver_session_device_fd(s, fdname, fd, devno); + if (r < 0) + goto fail_close; + return 0; + } + + /* Takes ownership of fd on both success and failure */ + return deliver_session_leader_fd_consume(s, fdname, fd); + +fail_close: + close_and_notify_warn(fd, fdname); + return r; +} + static int manager_enumerate_sessions(Manager *m) { + _cleanup_strv_free_ char **fdnames = NULL; _cleanup_closedir_ DIR *d = NULL; - int r = 0; + int r = 0, n; assert(m); @@ -486,12 +540,20 @@ static int manager_enumerate_sessions(Manager *m) { session_add_to_gc_queue(s); - RET_GATHER(r, session_load(s)); + k = session_load(s); + if (k < 0) + RET_GATHER(r, log_warning_errno(k, "Failed to deserialize session '%s', ignoring: %m", s->id)); } - /* We might be restarted and PID1 could have sent us back the session device fds we previously - * saved. */ - (void) manager_attach_fds(m); + n = sd_listen_fds_with_names(/* unset_environment = */ true, &fdnames); + if (n < 0) + return log_error_errno(n, "Failed to acquire passed fd list: %m"); + + for (int i = 0; i < n; i++) { + int fd = SD_LISTEN_FDS_START + i; + + RET_GATHER(r, manager_attach_session_fd_one_consume(m, fdnames[i], fd)); + } return r; } diff --git a/units/systemd-logind.service.in b/units/systemd-logind.service.in index 39dc0c22419..cc1b6be429c 100644 --- a/units/systemd-logind.service.in +++ b/units/systemd-logind.service.in @@ -31,7 +31,7 @@ DeviceAllow=char-input rw DeviceAllow=char-tty rw DeviceAllow=char-vcs rw ExecStart={{LIBEXECDIR}}/systemd-logind -FileDescriptorStoreMax=512 +FileDescriptorStoreMax=768 IPAddressDeny=any LockPersonality=yes MemoryDenyWriteExecute=yes