diff --git a/NEWS b/NEWS index 5afaf8e083..d5a039326c 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 404bee5a85..37eb842e20 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 3a00a700c9..dbbadfedf7 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 8e518db865..7def723723 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 9b32d4bd7c..56ca70da84 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 d812d81b20..ce2b48cdc1 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 c4f35e3a55..7b1fd1b148 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 f5c5664f66..e76a9d268f 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"),