Merge 2151b64b44 into 60bcd98228

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 (76f2191d8e) 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.

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;

src/login/logind-session-dbus.c

@@ -911,25 +911,16 @@ 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);
-
         _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 "
+                  "id=%s object_path=%s uid=" UID_FMT " runtime_path=%s seat=%s vtnr=%u",
-                  "session_fd=%d 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);
 
@@ -938,7 +929,10 @@ int session_send_create_reply_bus(Session *s, const sd_bus_error *error) {
                         s->id,
                         p,
                         s->user->runtime_path,
-                        fifo_fd,
+                        s->leader.fd, /* This is supposed to be a fifo fd which older versions logind would
+                                         return to clients and subscribe to EOF upon which the session gets
+                                         closed. We've bumped our baseline to v5.4 where pidfd is better
+                                         suited for the job. But still need to return *something* to the client. */
                         (uint32_t) s->user->user_record->uid,
                         s->seat ? s->seat->id : "",
                         (uint32_t) s->vtnr,

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);
+        /* Note that we don't remove the state file here, since it's supposed to survive daemon restarts */
-        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);
         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) {
+        /* logind before v258 used a fifo for session close notification. Since v258 we fully employ
-                int fd;
+         * pidfd for the job, hence just unlink the legacy fifo. */
-
+        if (fifo_path)
-                /* If we open an unopened pipe for reading we will not
+                (void) unlink(fifo_path);
-                   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);
-        }
 
         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)
+                if (r < 0)
+                        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)
-                        log_warning_errno(r, "Failed to set leader PID for session '%s': %m", s->id);
+                        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))

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);

src/login/logind-varlink.c

@@ -100,36 +100,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 "
+                  "id=%s uid=" UID_FMT " runtime_path=%s seat=%s vtnr=%u",
-                  "session_fd=%d 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)),
@@ -166,7 +148,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 },
@@ -251,6 +233,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,

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=%s object_path=%s runtime_path=%s seat=%s vtnr=%u original_uid=%u",
-                         id, strna(object_path), runtime_path, session_fd, real_seat, real_vtnr, original_uid);
+                         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;
 }

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"),