From: Mike Yuan Date: Wed, 8 Jan 2025 12:50:35 +0000 (+0100) Subject: logind: drop session fifo logic, rely solely on pidfd for exit notification X-Git-Tag: v258-rc1~777 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3180c4d46151673a9c985e60f205d4c76a81573f;p=thirdparty%2Fsystemd.git logind: drop session fifo logic, rely solely on pidfd for exit notification Traditionally, logind installed a fifo in the PAM session and used EOF on the fd as signal for session close. With the addition of pidfd (76f2191d8eb54d7b9e39ab230c9c62b8a8c42265) however, logind tracks the leader process and the session is terminated as soon as that exits. I think the new behavior generally makes more sense, and the behavior got changed *in the mentioned commit already* without anyone ever showing up to complain. It hence feels safe to kill the concept now (also before the varlink interface gets rolled out). Note that the 'PID' field in CreateSession() Varlink method is now marked as strict, i.e. failure to acquire pidfd is immediately treated as fatal. --- diff --git a/NEWS b/NEWS index 5afaf8e0832..d5a039326c2 100644 --- a/NEWS +++ b/NEWS @@ -73,6 +73,12 @@ CHANGES WITH 258 in spe: removed. Also, 'cryptolib' meson option has been deprecated, and will be removed in a future release. + * systemd-logind's session tracking, which used to be performed via + fifo fd installed in the client, has been fully switched to be + pidfd-based. The fd returned by CreateSession() and related calls + is therefore unused. Moreover, the exit of session leader process + will immediately cause the session to be stopped. + Announcements of Future Feature Removals: * The D-Bus method org.freedesktop.systemd1.StartAuxiliaryScope() is diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 404bee5a858..37eb842e20a 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1113,6 +1113,9 @@ static int manager_create_session_by_bus( if (leader.pid == 1 || pidref_is_self(&leader)) return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid leader PID"); + if (leader.fd < 0) + return sd_bus_error_set_errnof(error, EUNATCH, "Leader PIDFD not available"); + SessionType t; if (isempty(type)) t = _SESSION_TYPE_INVALID; diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c index 3a00a700c93..dbbadfedf7e 100644 --- a/src/login/logind-session-dbus.c +++ b/src/login/logind-session-dbus.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #include +#include #include "alloc-util.h" #include "bus-common-errors.h" @@ -912,25 +913,23 @@ int session_send_create_reply_bus(Session *s, const sd_bus_error *error) { if (sd_bus_error_is_set(error)) return sd_bus_reply_method_error(c, error); - _cleanup_close_ int fifo_fd = session_create_fifo(s); - if (fifo_fd < 0) - return fifo_fd; - - /* Update the session state file before we notify the client about the result. */ - session_save(s); + /* Prior to v258, logind tracked sessions by installing a fifo in client and subscribe to its EOF. + * Now we can fully rely on pidfd for this, but still need to return *something* to the client. + * Allocate something lightweight and isolated as placeholder. */ + _cleanup_close_ int fd = eventfd(0, EFD_CLOEXEC); + if (fd < 0) + return -errno; _cleanup_free_ char *p = session_bus_path(s); if (!p) return -ENOMEM; log_debug("Sending D-Bus reply about created session: " - "id=%s object_path=%s uid=" UID_FMT " runtime_path=%s " - "session_fd=%d seat=%s vtnr=%u", + "id=%s object_path=%s uid=" UID_FMT " runtime_path=%s seat=%s vtnr=%u", s->id, p, s->user->user_record->uid, s->user->runtime_path, - fifo_fd, s->seat ? s->seat->id : "", s->vtnr); @@ -939,7 +938,7 @@ int session_send_create_reply_bus(Session *s, const sd_bus_error *error) { s->id, p, s->user->runtime_path, - fifo_fd, + fd, /* not really used - see comments above */ (uint32_t) s->user->user_record->uid, s->seat ? s->seat->id : "", (uint32_t) s->vtnr, diff --git a/src/login/logind-session.c b/src/login/logind-session.c index 8e518db865d..7def7237233 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -44,7 +44,6 @@ #define RELEASE_USEC (20*USEC_PER_SEC) -static void session_remove_fifo(Session *s); static void session_restore_vt(Session *s); int session_new(Manager *m, const char *id, Session **ret) { @@ -66,7 +65,6 @@ int session_new(Manager *m, const char *id, Session **ret) { .manager = m, .id = strdup(id), .state_file = path_join("/run/systemd/sessions/", id), - .fifo_fd = -EBADF, .vtfd = -EBADF, .audit_id = AUDIT_SESSION_INVALID, .tty_validity = _TTY_VALIDITY_INVALID, @@ -107,11 +105,9 @@ static int session_watch_pidfd(Session *s) { assert(s); assert(s->manager); assert(pidref_is_set(&s->leader)); + assert(s->leader.fd >= 0); assert(!s->leader_pidfd_event_source); - if (s->leader.fd < 0) - return 0; - r = sd_event_add_io(s->manager->event, &s->leader_pidfd_event_source, s->leader.fd, EPOLLIN, session_dispatch_leader_pidfd, s); if (r < 0) return r; @@ -209,12 +205,7 @@ Session* session_free(Session *s) { hashmap_remove(s->manager->sessions, s->id); - sd_event_source_unref(s->fifo_event_source); - safe_close(s->fifo_fd); - - /* Note that we remove neither the state file nor the fifo path here, since we want both to survive - * daemon restarts */ - free(s->fifo_path); + /* Note that we don't remove the state file here, since it's supposed to survive daemon restarts */ free(s->state_file); free(s->id); @@ -237,6 +228,7 @@ int session_set_leader_consume(Session *s, PidRef _leader) { assert(s); assert(pidref_is_set(&pidref)); + assert(pidref.fd >= 0); if (pidref_equal(&s->leader, &pidref)) return 0; @@ -332,9 +324,6 @@ int session_save(Session *s) { if (s->scope_job) fprintf(f, "SCOPE_JOB=%s\n", s->scope_job); - if (s->fifo_path) - fprintf(f, "FIFO=%s\n", s->fifo_path); - if (s->seat) fprintf(f, "SEAT=%s\n", s->seat->id); @@ -486,7 +475,8 @@ int session_load(Session *s) { *controller = NULL, *active = NULL, *devices = NULL, - *is_display = NULL; + *is_display = NULL, + *fifo_path = NULL; /* compat only, not used */ int k, r; @@ -496,7 +486,7 @@ int session_load(Session *s) { "REMOTE", &remote, "SCOPE", &s->scope, "SCOPE_JOB", &s->scope_job, - "FIFO", &s->fifo_path, + "FIFO", &fifo_path, "SEAT", &seat, "TTY", &s->tty, "TTY_VALIDITY", &tty_validity, @@ -615,19 +605,10 @@ int session_load(Session *s) { if (streq_ptr(state, "closing")) s->stopping = true; - if (s->fifo_path) { - int fd; - - /* If we open an unopened pipe for reading we will not - get an EOF. to trigger an EOF we hence open it for - writing, but close it right away which then will - trigger the EOF. This will happen immediately if no - other process has the FIFO open for writing, i. e. - when the session died before logind (re)started. */ - - fd = session_create_fifo(s); - safe_close(fd); - } + /* logind before v258 used a fifo for session close notification. Since v258 we fully employ + * pidfd for the job, hence just unlink the legacy fifo. */ + if (fifo_path) + (void) unlink(fifo_path); if (realtime) (void) deserialize_usec(realtime, &s->timestamp.realtime); @@ -681,13 +662,19 @@ int session_load(Session *s) { _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 log_error_errno(r, "Failed to deserialize leader PID for session '%s': %m", s->id); + if (p.fd < 0) + return log_error_errno(SYNTHETIC_ERRNO(ENOTRECOVERABLE), + "Failed to acquire pidfd for session leader '" PID_FMT "', refusing.", + p.pid); + + r = session_set_leader_consume(s, TAKE_PIDREF(p)); + if (r < 0) + return log_error_errno(r, "Failed to set leader PID for session '%s': %m", s->id); } - return r; + return 0; } int session_activate(Session *s) { @@ -968,9 +955,6 @@ int session_stop(Session *s, bool force) { if (s->seat) seat_evict_position(s->seat, s); - /* We are going down, don't care about FIFOs anymore */ - session_remove_fifo(s); - /* Kill cgroup */ r = session_stop_scope(s, force); @@ -1264,71 +1248,6 @@ int session_set_tty(Session *s, const char *tty) { return 1; } -static int session_dispatch_fifo(sd_event_source *es, int fd, uint32_t revents, void *userdata) { - Session *s = ASSERT_PTR(userdata); - - assert(s->fifo_fd == fd); - - /* EOF on the FIFO means the session died abnormally. */ - - session_remove_fifo(s); - session_stop(s, /* force = */ false); - - session_add_to_gc_queue(s); - - return 1; -} - -int session_create_fifo(Session *s) { - int r; - - assert(s); - - /* Create FIFO */ - if (!s->fifo_path) { - r = mkdir_safe_label("/run/systemd/sessions", 0755, 0, 0, MKDIR_WARN_MODE); - if (r < 0) - return r; - - s->fifo_path = strjoin("/run/systemd/sessions/", s->id, ".ref"); - if (!s->fifo_path) - return -ENOMEM; - - if (mkfifo(s->fifo_path, 0600) < 0 && errno != EEXIST) - return -errno; - } - - /* Open reading side */ - if (s->fifo_fd < 0) { - s->fifo_fd = open(s->fifo_path, O_RDONLY|O_CLOEXEC|O_NONBLOCK); - if (s->fifo_fd < 0) - return -errno; - } - - if (!s->fifo_event_source) { - r = sd_event_add_io(s->manager->event, &s->fifo_event_source, s->fifo_fd, 0, session_dispatch_fifo, s); - if (r < 0) - return r; - - /* Let's make sure we noticed dead sessions before we process new bus requests (which might - * create new sessions). */ - r = sd_event_source_set_priority(s->fifo_event_source, SD_EVENT_PRIORITY_NORMAL-10); - if (r < 0) - return r; - } - - /* Open writing side */ - return RET_NERRNO(open(s->fifo_path, O_WRONLY|O_CLOEXEC|O_NONBLOCK)); -} - -static void session_remove_fifo(Session *s) { - assert(s); - - s->fifo_event_source = sd_event_source_unref(s->fifo_event_source); - s->fifo_fd = safe_close(s->fifo_fd); - s->fifo_path = unlink_and_free(s->fifo_path); -} - bool session_may_gc(Session *s, bool drop_not_started) { int r; @@ -1350,9 +1269,6 @@ bool session_may_gc(Session *s, bool drop_not_started) { if (r > 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; @@ -1393,7 +1309,7 @@ SessionState session_get_state(Session *s) { if (s->stopping || s->timer_event_source) return SESSION_CLOSING; - if (s->scope_job || (!pidref_is_set(&s->leader) && s->fifo_fd < 0)) + if (s->scope_job || !pidref_is_set(&s->leader)) return SESSION_OPENING; if (session_is_active(s)) diff --git a/src/login/logind-session.h b/src/login/logind-session.h index 9b32d4bd7c6..56ca70da848 100644 --- a/src/login/logind-session.h +++ b/src/login/logind-session.h @@ -138,10 +138,6 @@ struct Session { pid_t deserialized_pid; /* PID deserialized from state file (for verification when pidfd is used) */ uint32_t audit_id; - int fifo_fd; - char *fifo_path; - - sd_event_source *fifo_event_source; sd_event_source *leader_pidfd_event_source; bool in_gc_queue; @@ -194,7 +190,6 @@ void session_set_type(Session *s, SessionType t); void session_set_class(Session *s, SessionClass c); int session_set_display(Session *s, const char *display); int session_set_tty(Session *s, const char *tty); -int session_create_fifo(Session *s); int session_start(Session *s, sd_bus_message *properties, sd_bus_error *error); int session_stop(Session *s, bool force); int session_finalize(Session *s); diff --git a/src/login/logind-varlink.c b/src/login/logind-varlink.c index d812d81b200..ce2b48cdc14 100644 --- a/src/login/logind-varlink.c +++ b/src/login/logind-varlink.c @@ -101,36 +101,18 @@ int session_send_create_reply_varlink(Session *s, const sd_bus_error *error) { if (sd_bus_error_is_set(error)) return sd_varlink_error(vl, "io.systemd.Login.UnitAllocationFailed", /* parameters= */ NULL); - _cleanup_close_ int fifo_fd = session_create_fifo(s); - if (fifo_fd < 0) - return fifo_fd; - - /* Update the session state file before we notify the client about the result. */ - session_save(s); - log_debug("Sending Varlink reply about created session: " - "id=%s uid=" UID_FMT " runtime_path=%s " - "session_fd=%d seat=%s vtnr=%u", + "id=%s uid=" UID_FMT " runtime_path=%s seat=%s vtnr=%u", s->id, s->user->user_record->uid, s->user->runtime_path, - fifo_fd, s->seat ? s->seat->id : "", s->vtnr); - int fifo_fd_idx = sd_varlink_push_fd(vl, fifo_fd); - if (fifo_fd_idx < 0) { - log_error_errno(fifo_fd_idx, "Failed to push FIFO fd to Varlink: %m"); - return sd_varlink_error_errno(vl, fifo_fd_idx); - } - - TAKE_FD(fifo_fd); - return sd_varlink_replybo( vl, SD_JSON_BUILD_PAIR_STRING("Id", s->id), SD_JSON_BUILD_PAIR_STRING("RuntimePath", s->user->runtime_path), - SD_JSON_BUILD_PAIR_UNSIGNED("SessionFileDescriptor", fifo_fd_idx), SD_JSON_BUILD_PAIR_UNSIGNED("UID", s->user->user_record->uid), SD_JSON_BUILD_PAIR_CONDITION(!!s->seat, "Seat", SD_JSON_BUILD_STRING(s->seat ? s->seat->id : NULL)), SD_JSON_BUILD_PAIR_CONDITION(s->vtnr > 0, "VTNr", SD_JSON_BUILD_UNSIGNED(s->vtnr)), @@ -167,7 +149,7 @@ static int vl_method_create_session(sd_varlink *link, sd_json_variant *parameter static const sd_json_dispatch_field dispatch_table[] = { { "UID", _SD_JSON_VARIANT_TYPE_INVALID, sd_json_dispatch_uid_gid, offsetof(CreateSessionParameters, uid), SD_JSON_MANDATORY }, - { "PID", _SD_JSON_VARIANT_TYPE_INVALID, json_dispatch_pidref, offsetof(CreateSessionParameters, pid), SD_JSON_RELAX }, + { "PID", _SD_JSON_VARIANT_TYPE_INVALID, json_dispatch_pidref, offsetof(CreateSessionParameters, pid), SD_JSON_STRICT }, { "Service", SD_JSON_VARIANT_STRING, sd_json_dispatch_const_string, offsetof(CreateSessionParameters, service), 0 }, { "Type", SD_JSON_VARIANT_STRING, json_dispatch_session_type, offsetof(CreateSessionParameters, type), SD_JSON_MANDATORY }, { "Class", SD_JSON_VARIANT_STRING, json_dispatch_session_class, offsetof(CreateSessionParameters, class), SD_JSON_MANDATORY }, @@ -252,6 +234,9 @@ static int vl_method_create_session(sd_varlink *link, sd_json_variant *parameter return log_debug_errno(r, "Failed to get peer pidref: %m"); } + if (p.pid.fd < 0) + return sd_varlink_error(link, "io.systemd.Login.NoSessionPIDFD", /* parameters= */ NULL); + Session *session; r = manager_create_session( m, diff --git a/src/login/pam_systemd.c b/src/login/pam_systemd.c index c4f35e3a558..7b1fd1b1480 100644 --- a/src/login/pam_systemd.c +++ b/src/login/pam_systemd.c @@ -1153,7 +1153,7 @@ static int register_session( _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; /* the following variables point into this message, hence pin it for longer */ _cleanup_(sd_varlink_unrefp) sd_varlink *vl = NULL; /* similar */ const char *id = NULL, *object_path = NULL, *runtime_path = NULL, *real_seat = NULL; - int session_fd = -EBADF, existing = false; + int existing = false; uint32_t original_uid = UID_INVALID, real_vtnr = 0; bool done = false; @@ -1163,10 +1163,6 @@ static int register_session( if (r < 0) log_debug_errno(r, "Failed to connect to logind via Varlink, falling back to D-Bus: %m"); else { - r = sd_varlink_set_allow_fd_passing_input(vl, true); - if (r < 0) - return pam_syslog_errno(handle, LOG_ERR, r, "Failed to enable input fd passing on Varlink socket: %m"); - r = sd_varlink_set_allow_fd_passing_output(vl, true); if (r < 0) return pam_syslog_errno(handle, LOG_ERR, r, "Failed to enable output fd passing on Varlink socket: %m"); @@ -1216,20 +1212,17 @@ static int register_session( struct { const char *id; const char *runtime_path; - unsigned session_fd_idx; uid_t uid; const char *seat; unsigned vtnr; bool existing; } p = { - .session_fd_idx = UINT_MAX, .uid = UID_INVALID, }; static const sd_json_dispatch_field dispatch_table[] = { { "Id", SD_JSON_VARIANT_STRING, sd_json_dispatch_const_string, voffsetof(p, id), SD_JSON_MANDATORY }, { "RuntimePath", SD_JSON_VARIANT_STRING, json_dispatch_const_path, voffsetof(p, runtime_path), SD_JSON_MANDATORY }, - { "SessionFileDescriptor", _SD_JSON_VARIANT_TYPE_INVALID, sd_json_dispatch_uint64, voffsetof(p, session_fd_idx), SD_JSON_MANDATORY }, { "UID", _SD_JSON_VARIANT_TYPE_INVALID, sd_json_dispatch_uid_gid, voffsetof(p, uid), SD_JSON_MANDATORY }, { "Seat", SD_JSON_VARIANT_STRING, sd_json_dispatch_const_string, voffsetof(p, seat), 0 }, { "VTNr", _SD_JSON_VARIANT_TYPE_INVALID, sd_json_dispatch_uint, voffsetof(p, vtnr), 0 }, @@ -1240,10 +1233,6 @@ static int register_session( if (r < 0) return pam_syslog_errno(handle, LOG_ERR, r, "Failed to parse CreateSession() reply: %m"); - session_fd = sd_varlink_peek_fd(vl, p.session_fd_idx); - if (session_fd < 0) - return pam_syslog_errno(handle, LOG_ERR, session_fd, "Failed to extract session fd from CreateSession() reply: %m"); - id = p.id; runtime_path = p.runtime_path; original_uid = p.uid; @@ -1318,7 +1307,7 @@ static int register_session( &id, &object_path, &runtime_path, - &session_fd, + /* session_fd = */ NULL, &original_uid, &real_seat, &real_vtnr, @@ -1329,8 +1318,8 @@ static int register_session( pam_debug_syslog(handle, debug, "Reply from logind: " - "id=%s object_path=%s runtime_path=%s session_fd=%d seat=%s vtnr=%u original_uid=%u", - id, strna(object_path), runtime_path, session_fd, real_seat, real_vtnr, original_uid); + "id=%s object_path=%s runtime_path=%s seat=%s vtnr=%u original_uid=%u", + id, strna(object_path), runtime_path, real_seat, real_vtnr, original_uid); /* Please update manager_default_environment() in core/manager.c accordingly if more session envvars * shall be added. */ @@ -1376,17 +1365,6 @@ static int register_session( if (r != PAM_SUCCESS) return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to install existing flag: @PAMERR@"); - if (session_fd >= 0) { - _cleanup_close_ int fd = fcntl(session_fd, F_DUPFD_CLOEXEC, 3); - if (fd < 0) - return pam_syslog_errno(handle, LOG_ERR, errno, "Failed to dup session fd: %m"); - - r = pam_set_data(handle, "systemd.session-fd", FD_TO_PTR(fd), NULL); - if (r != PAM_SUCCESS) - return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to install session fd: @PAMERR@"); - TAKE_FD(fd); - } - /* Don't set $XDG_RUNTIME_DIR if the user we now authenticated for does not match the * original user of the session. We do this in order not to result in privileged apps * clobbering the runtime directory unnecessarily. */ @@ -1898,9 +1876,5 @@ _public_ PAM_EXTERN int pam_sm_close_session( } } - /* Note that we are knowingly leaking the FIFO fd here. This way, logind can watch us die. If we - * closed it here it would not have any clue when that is completed. Given that one cannot really - * have multiple PAM sessions open from the same process this means we will leak one FD at max. */ - return PAM_SUCCESS; } diff --git a/src/shared/varlink-io.systemd.Login.c b/src/shared/varlink-io.systemd.Login.c index f5c5664f66b..e76a9d268f4 100644 --- a/src/shared/varlink-io.systemd.Login.c +++ b/src/shared/varlink-io.systemd.Login.c @@ -70,8 +70,6 @@ static SD_VARLINK_DEFINE_METHOD( SD_VARLINK_DEFINE_OUTPUT(Id, SD_VARLINK_STRING, 0), SD_VARLINK_FIELD_COMMENT("The runtime path ($XDG_RUNTIME_DIR) of the user."), SD_VARLINK_DEFINE_OUTPUT(RuntimePath, SD_VARLINK_STRING, 0), - SD_VARLINK_FIELD_COMMENT("Index into the file descriptor table of this reply with the session tracking fd for this session."), - SD_VARLINK_DEFINE_OUTPUT(SessionFileDescriptor, SD_VARLINK_INT, 0), SD_VARLINK_FIELD_COMMENT("The original UID of this session."), SD_VARLINK_DEFINE_OUTPUT(UID, SD_VARLINK_INT, 0), SD_VARLINK_FIELD_COMMENT("The seat this session has been assigned to"),