logind: drop session fifo logic, rely solely on pidfd for exit notification

YHNdnzj · yuwata · commit 3180c4d46151 · 2025-04-19T04:43:04.000+09:00
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 (76f2191) 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
@@ -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
@@ -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
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: LGPL-2.1-or-later */
 
 #include <errno.h>
+#include <sys/eventfd.h>
 
 #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
@@ -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
@@ -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
@@ -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
diff --git a/src/shared/varlink-io.systemd.Login.c b/src/shared/varlink-io.systemd.Login.c
