logind: rework session shutdown logic

Simplify the shutdown logic a bit:

- Keep the session FIFO around in the PAM module, even after the session
  shutdown hook has been finished. This allows logind to track precisely
  when the PAM handler goes away.

- In the ReleaseSession() call start a timer, that will stop terminate
  the session when elapsed.

- Never fiddle with the KillMode of scopes to configure whether user
  processes should be killed or not. Instead, simply leave the scope
  units around when we terminate a session whose processes should not be
  killed.

- When killing is enabled, stop the session scope on FIFO EOF or after
  the ReleaseSession() timeout. When killing is disabled, simply tell
  PID 1 to abandon the scope.

Because the scopes stay around and hence all processes are always member
of a scope, the system shutdown logic should be more robust, as the
scopes can be shutdown as part of the usual shutdown logic.

8 files changed, 147 insertions, 78 deletions

diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
index 4745961773..40885557c1 100644
--- a/src/login/logind-dbus.c
+++ b/src/login/logind-dbus.c
@@ -748,15 +748,7 @@ static int method_release_session(sd_bus *bus, sd_bus_message *message, void *us
         if (!session)
                 return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_SESSION, "No session '%s' known", name);
 
-        /* We use the FIFO to detect stray sessions where the process
-           invoking PAM dies abnormally. We need to make sure that
-           that process is not killed if at the clean end of the
-           session it closes the FIFO. Hence, with this call
-           explicitly turn off the FIFO logic, so that the PAM code
-           can finish clean up on its own */
-        session_remove_fifo(session);
-        session_save(session);
-        user_save(session->user);
+        session_release(session);
 
         return sd_bus_reply_method_return(message, NULL);
 }
@@ -2185,7 +2177,6 @@ int manager_start_scope(
                 const char *slice,
                 const char *description,
                 const char *after,
-                const char *kill_mode,
                 sd_bus_error *error,
                 char **job) {
 
@@ -2232,12 +2223,6 @@ int manager_start_scope(
                         return r;
         }
 
-        if (!isempty(kill_mode)) {
-                r = sd_bus_message_append(m, "(sv)", "KillMode", "s", kill_mode);
-                if (r < 0)
-                        return r;
-        }
-
         /* cgroup empty notification is not available in containers
          * currently. To make this less problematic, let's shorten the
          * stop timeout for sessions, so that we don't wait
@@ -2372,6 +2357,40 @@ int manager_stop_unit(Manager *manager, const char *unit, sd_bus_error *error, c
         return 1;
 }
 
+int manager_abandon_scope(Manager *manager, const char *scope, sd_bus_error *error) {
+        _cleanup_bus_message_unref_ sd_bus_message *reply = NULL;
+        _cleanup_free_ char *path = NULL;
+        int r;
+
+        assert(manager);
+        assert(scope);
+
+        path = unit_dbus_path_from_name(scope);
+        if (!path)
+                return -ENOMEM;
+
+        r = sd_bus_call_method(
+                        manager->bus,
+                        "org.freedesktop.systemd1",
+                        path,
+                        "org.freedesktop.systemd1.Scope",
+                        "Abandon",
+                        error,
+                        NULL,
+                        NULL);
+        if (r < 0) {
+                if (sd_bus_error_has_name(error, BUS_ERROR_NO_SUCH_UNIT) ||
+                    sd_bus_error_has_name(error, BUS_ERROR_LOAD_FAILED)) {
+                        sd_bus_error_free(error);
+                        return 0;
+                }
+
+                return r;
+        }
+
+        return 1;
+}
+
 int manager_kill_unit(Manager *manager, const char *unit, KillWho who, int signo, sd_bus_error *error) {
         assert(manager);
         assert(unit);
diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index bec59c044b..95105e5325 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -40,6 +40,10 @@
 #include "bus-error.h"
 #include "logind-session.h"
 
+#define RELEASE_USEC (20*USEC_PER_SEC)
+
+static void session_remove_fifo(Session *s);
+
 static unsigned long devt_hash_func(const void *p, const uint8_t hash_key[HASH_KEY_SIZE]) {
         uint64_t u = *(const dev_t*)p;
 
@@ -103,6 +107,8 @@ void session_free(Session *s) {
         if (s->in_gc_queue)
                 LIST_REMOVE(gc_queue, s->manager->session_gc_queue, s);
 
+        s->timer_event_source = sd_event_source_unref(s->timer_event_source);
+
         session_remove_fifo(s);
 
         session_drop_controller(s);
@@ -147,6 +153,8 @@ void session_free(Session *s) {
 
         hashmap_remove(s->manager->sessions, s->id);
 
+        s->vt_source = sd_event_source_unref(s->vt_source);
+
         free(s->state_file);
         free(s);
 }
@@ -472,7 +480,6 @@ static int session_start_scope(Session *s) {
         if (!s->scope) {
                 _cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
                 _cleanup_free_ char *description = NULL;
-                const char *kill_mode;
                 char *scope, *job;
 
                 description = strjoin("Session ", s->id, " of user ", s->user->name, NULL);
@@ -483,9 +490,7 @@ static int session_start_scope(Session *s) {
                 if (!scope)
                         return log_oom();
 
-                kill_mode = manager_shall_kill(s->manager, s->user->name) ? "control-group" : "none";
-
-                r = manager_start_scope(s->manager, scope, s->leader, s->user->slice, description, "systemd-user-sessions.service", kill_mode, &error, &job);
+                r = manager_start_scope(s->manager, scope, s->leader, s->user->slice, description, "systemd-logind.service", &error, &job);
                 if (r < 0) {
                         log_error("Failed to start session scope %s: %s %s",
                                   scope, bus_error_message(&error, r), error.name);
@@ -541,23 +546,22 @@ int session_start(Session *s) {
 
         s->started = true;
 
-        /* Save session data */
+        /* Save data */
         session_save(s);
         user_save(s->user);
+        if (s->seat)
+                seat_save(s->seat);
 
+        /* Send signals */
         session_send_signal(s, true);
-
+        user_send_changed(s->user, "Sessions", NULL);
         if (s->seat) {
-                seat_save(s->seat);
-
                 if (s->seat->active == s)
                         seat_send_changed(s->seat, "Sessions", "ActiveSession", NULL);
                 else
                         seat_send_changed(s->seat, "Sessions", NULL);
         }
 
-        user_send_changed(s->user, "Sessions", NULL);
-
         return 0;
 }
 
@@ -571,14 +575,22 @@ static int session_stop_scope(Session *s) {
         if (!s->scope)
                 return 0;
 
-        r = manager_stop_unit(s->manager, s->scope, &error, &job);
-        if (r < 0) {
-                log_error("Failed to stop session scope: %s", bus_error_message(&error, r));
-                return r;
-        }
+        if (manager_shall_kill(s->manager, s->user->name)) {
+                r = manager_stop_unit(s->manager, s->scope, &error, &job);
+                if (r < 0) {
+                        log_error("Failed to stop session scope: %s", bus_error_message(&error, r));
+                        return r;
+                }
 
-        free(s->scope_job);
-        s->scope_job = job;
+                free(s->scope_job);
+                s->scope_job = job;
+        } else {
+                r = manager_abandon_scope(s->manager, s->scope, &error);
+                if (r < 0) {
+                        log_error("Failed to abandon session scope: %s", bus_error_message(&error, r));
+                        return r;
+                }
+        }
 
         return 0;
 }
@@ -591,9 +603,16 @@ int session_stop(Session *s) {
         if (!s->user)
                 return -ESTALE;
 
+        s->timer_event_source = sd_event_source_unref(s->timer_event_source);
+
+        /* We are going down, don't care about FIFOs anymore */
+        session_remove_fifo(s);
+
         /* Kill cgroup */
         r = session_stop_scope(s);
 
+        s->stopping = true;
+
         session_save(s);
         user_save(s->user);
 
@@ -618,6 +637,8 @@ int session_finalize(Session *s) {
                            "MESSAGE=Removed session %s.", s->id,
                            NULL);
 
+        s->timer_event_source = sd_event_source_unref(s->timer_event_source);
+
         /* Kill session devices */
         while ((sd = hashmap_first(s->devices)))
                 session_device_free(sd);
@@ -635,16 +656,36 @@ int session_finalize(Session *s) {
                 if (s->seat->active == s)
                         seat_set_active(s->seat, NULL);
 
-                seat_send_changed(s->seat, "Sessions", NULL);
                 seat_save(s->seat);
+                seat_send_changed(s->seat, "Sessions", NULL);
         }
 
-        user_send_changed(s->user, "Sessions", NULL);
         user_save(s->user);
+        user_send_changed(s->user, "Sessions", NULL);
 
         return r;
 }
 
+static int release_timeout_callback(sd_event_source *es, uint64_t usec, void *userdata) {
+        Session *s = userdata;
+
+        assert(es);
+        assert(s);
+
+        session_stop(s);
+        return 0;
+}
+
+void session_release(Session *s) {
+        assert(s);
+
+        if (!s->started || s->stopping)
+                return;
+
+        if (!s->timer_event_source)
+                sd_event_add_monotonic(s->manager->event, now(CLOCK_MONOTONIC) + RELEASE_USEC, 0, release_timeout_callback, s, &s->timer_event_source);
+}
+
 bool session_is_active(Session *s) {
         assert(s);
 
@@ -820,7 +861,7 @@ int session_create_fifo(Session *s) {
         return r;
 }
 
-void session_remove_fifo(Session *s) {
+static void session_remove_fifo(Session *s) {
         assert(s);
 
         if (s->fifo_event_source)
@@ -839,8 +880,6 @@ void session_remove_fifo(Session *s) {
 }
 
 bool session_check_gc(Session *s, bool drop_not_started) {
-        int r;
-
         assert(s);
 
         if (drop_not_started && !s->started)
@@ -850,11 +889,7 @@ bool session_check_gc(Session *s, bool drop_not_started) {
                 return false;
 
         if (s->fifo_fd >= 0) {
-                r = pipe_eof(s->fifo_fd);
-                if (r < 0)
-                        return true;
-
-                if (r == 0)
+                if (pipe_eof(s->fifo_fd) <= 0)
                         return true;
         }
 
@@ -880,12 +915,12 @@ void session_add_to_gc_queue(Session *s) {
 SessionState session_get_state(Session *s) {
         assert(s);
 
+        if (s->stopping || s->timer_event_source)
+                return SESSION_CLOSING;
+
         if (s->scope_job)
                 return SESSION_OPENING;
 
-        if (s->fifo_fd < 0)
-                return SESSION_CLOSING;
-
         if (session_is_active(s))
                 return SESSION_ACTIVE;
 
@@ -902,7 +937,7 @@ int session_kill(Session *s, KillWho who, int signo) {
 }
 
 static int session_open_vt(Session *s) {
-        char path[128];
+        char path[sizeof("/dev/tty") + DECIMAL_STR_MAX(s->vtnr)];
 
         if (!s->vtnr)
                 return -1;
@@ -980,8 +1015,7 @@ void session_restore_vt(Session *s) {
         if (vt < 0)
                 return;
 
-        sd_event_source_unref(s->vt_source);
-        s->vt_source = NULL;
+        s->vt_source = sd_event_source_unref(s->vt_source);
 
         ioctl(vt, KDSETMODE, KD_TEXT);
 
diff --git a/src/login/logind-session.h b/src/login/logind-session.h
index 7bf1932032..42552bc2bd 100644
--- a/src/login/logind-session.h
+++ b/src/login/logind-session.h
@@ -110,9 +110,12 @@ struct Session {
 
         bool in_gc_queue:1;
         bool started:1;
+        bool stopping:1;
 
         sd_bus_message *create_message;
 
+        sd_event_source *timer_event_source;
+
         char *controller;
         Hashmap *devices;
 
@@ -132,10 +135,10 @@ bool session_is_active(Session *s);
 int session_get_idle_hint(Session *s, dual_timestamp *t);
 void session_set_idle_hint(Session *s, bool b);
 int session_create_fifo(Session *s);
-void session_remove_fifo(Session *s);
 int session_start(Session *s);
 int session_stop(Session *s);
 int session_finalize(Session *s);
+void session_release(Session *s);
 int session_save(Session *s);
 int session_load(Session *s);
 int session_kill(Session *s, KillWho who, int signo);
diff --git a/src/login/logind-user.c b/src/login/logind-user.c
index bdb6915630..fdbf6e3aa0 100644
--- a/src/login/logind-user.c
+++ b/src/login/logind-user.c
@@ -515,6 +515,8 @@ int user_stop(User *u) {
         if (k < 0)
                 r = k;
 
+        u->stopping = true;
+
         user_save(u);
 
         return r;
@@ -633,22 +635,27 @@ void user_add_to_gc_queue(User *u) {
 
 UserState user_get_state(User *u) {
         Session *i;
-        bool all_closing = true;
 
         assert(u);
 
+        if (u->stopping)
+                return USER_CLOSING;
+
         if (u->slice_job || u->service_job)
                 return USER_OPENING;
 
-        LIST_FOREACH(sessions_by_user, i, u->sessions) {
-                if (session_is_active(i))
-                        return USER_ACTIVE;
-                if (session_get_state(i) != SESSION_CLOSING)
-                        all_closing = false;
-        }
+        if (u->sessions) {
+                bool all_closing = true;
+
+                LIST_FOREACH(sessions_by_user, i, u->sessions) {
+                        if (session_is_active(i))
+                                return USER_ACTIVE;
+                        if (session_get_state(i) != SESSION_CLOSING)
+                                all_closing = false;
+                }
 
-        if (u->sessions)
                 return all_closing ? USER_CLOSING : USER_ONLINE;
+        }
 
         if (user_check_linger_file(u) > 0)
                 return USER_LINGERING;
diff --git a/src/login/logind-user.h b/src/login/logind-user.h
index 0062880560..b0fefe9b9c 100644
--- a/src/login/logind-user.h
+++ b/src/login/logind-user.h
@@ -61,6 +61,7 @@ struct User {
 
         bool in_gc_queue:1;
         bool started:1;
+        bool stopping:1;
 
         LIST_HEAD(Session, sessions);
         LIST_FIELDS(User, gc_queue);
diff --git a/src/login/logind.c b/src/login/logind.c
index 48da7b173b..a6f84e8536 100644
--- a/src/login/logind.c
+++ b/src/login/logind.c
@@ -871,8 +871,15 @@ void manager_gc(Manager *m, bool drop_not_started) {
                 LIST_REMOVE(gc_queue, m->session_gc_queue, session);
                 session->in_gc_queue = false;
 
-                if (!session_check_gc(session, drop_not_started)) {
+                /* First, if we are not closing yet, initiate stopping */
+                if (!session_check_gc(session, drop_not_started) &&
+                    session_get_state(session) != SESSION_CLOSING)
                         session_stop(session);
+
+                /* Normally, this should make the session busy again,
+                 * if it doesn't then let's get rid of it
+                 * immediately */
+                if (!session_check_gc(session, drop_not_started)) {
                         session_finalize(session);
                         session_free(session);
                 }
@@ -882,8 +889,11 @@ void manager_gc(Manager *m, bool drop_not_started) {
                 LIST_REMOVE(gc_queue, m->user_gc_queue, user);
                 user->in_gc_queue = false;
 
-                if (!user_check_gc(user, drop_not_started)) {
+                if (!user_check_gc(user, drop_not_started) &&
+                    user_get_state(user) != USER_CLOSING)
                         user_stop(user);
+
+                if (!user_check_gc(user, drop_not_started)) {
                         user_finalize(user);
                         user_free(user);
                 }
diff --git a/src/login/logind.h b/src/login/logind.h
index b84137c78b..c1a5b6a80f 100644
--- a/src/login/logind.h
+++ b/src/login/logind.h
@@ -162,9 +162,10 @@ int manager_send_changed(Manager *manager, const char *property, ...) _sentinel_
 
 int manager_dispatch_delayed(Manager *manager);
 
-int manager_start_scope(Manager *manager, const char *scope, pid_t pid, const char *slice, const char *description, const char *after, const char *kill_mode, sd_bus_error *error, char **job);
+int manager_start_scope(Manager *manager, const char *scope, pid_t pid, const char *slice, const char *description, const char *after, sd_bus_error *error, char **job);
 int manager_start_unit(Manager *manager, const char *unit, sd_bus_error *error, char **job);
 int manager_stop_unit(Manager *manager, const char *unit, sd_bus_error *error, char **job);
+int manager_abandon_scope(Manager *manager, const char *scope, sd_bus_error *error);
 int manager_kill_unit(Manager *manager, const char *unit, KillWho who, int signo, sd_bus_error *error);
 int manager_unit_is_active(Manager *manager, const char *unit);
 int manager_job_is_active(Manager *manager, const char *path);
diff --git a/src/login/pam-module.c b/src/login/pam-module.c
index 3b2966b30c..79a9042ffd 100644
--- a/src/login/pam-module.c
+++ b/src/login/pam-module.c
@@ -499,7 +499,7 @@ _public_ PAM_EXTERN int pam_sm_close_session(
 
         _cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
         _cleanup_bus_unref_ sd_bus *bus = NULL;
-        const void *p = NULL, *existing = NULL;
+        const void *existing = NULL;
         const char *id;
         int r;
 
@@ -519,10 +519,8 @@ _public_ PAM_EXTERN int pam_sm_close_session(
 
                 r = sd_bus_open_system(&bus);
                 if (r < 0) {
-                        pam_syslog(handle, LOG_ERR,
-                                  "Failed to connect to system bus: %s", strerror(-r));
-                        r = PAM_SESSION_ERR;
-                        goto finish;
+                        pam_syslog(handle, LOG_ERR, "Failed to connect to system bus: %s", strerror(-r));
+                        return PAM_SESSION_ERR;
                 }
 
                 r = sd_bus_call_method(bus,
@@ -535,20 +533,16 @@ _public_ PAM_EXTERN int pam_sm_close_session(
                                        "s",
                                        id);
                 if (r < 0) {
-                        pam_syslog(handle, LOG_ERR,
-                                   "Failed to release session: %s", bus_error_message(&error, r));
-
-                        r = PAM_SESSION_ERR;
-                        goto finish;
+                        pam_syslog(handle, LOG_ERR, "Failed to release session: %s", bus_error_message(&error, r));
+                        return PAM_SESSION_ERR;
                 }
         }
 
-        r = PAM_SUCCESS;
+        /* 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. */
 
-finish:
-        pam_get_data(handle, "systemd.session-fd", &p);
-        if (p)
-                close_nointr(PTR_TO_INT(p) - 1);
-
-        return r;
+        return PAM_SUCCESS;
 }