daandemeyer / rpms / systemd

Forked from rpms/systemd 2 years ago
Clone
Blob Blame History Raw
From 553a962a570cade5c44953593209d25e28ec5145 Mon Sep 17 00:00:00 2001
From: Michal Sekletar <msekleta@redhat.com>
Date: Mon, 3 Mar 2014 15:37:49 +0100
Subject: [PATCH] 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.

Based-on: 5f41d1f10fd97e93517b6a762b1bec247f4d1171
---
 src/login/logind-dbus.c    |  51 +++++++++++-------
 src/login/logind-session.c | 130 +++++++++++++++++++++++++++++++++++----------
 src/login/logind-session.h |   4 ++
 src/login/logind-user.c    |  23 +++++---
 src/login/logind-user.h    |   1 +
 src/login/logind.c         |  23 ++++++--
 src/login/logind.h         |   4 +-
 src/login/pam-module.c     |  11 ++--
 8 files changed, 182 insertions(+), 65 deletions(-)

diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
index 69e94aa..8de301e 100644
--- a/src/login/logind-dbus.c
+++ b/src/login/logind-dbus.c
@@ -1747,13 +1747,7 @@ static DBusHandlerResult manager_message_handler(
                 if (!session)
                         return bus_send_error_reply(connection, message, &error, -ENOENT);
 
-                /* 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_release(session);
 
                 reply = dbus_message_new_method_return(message);
                 if (!reply)
@@ -2551,7 +2545,6 @@ int manager_start_scope(
                 const char *slice,
                 const char *description,
                 const char *after,
-                const char *kill_mode,
                 DBusError *error,
                 char **job) {
 
@@ -2623,18 +2616,6 @@ int manager_start_scope(
                         return log_oom();
         }
 
-        if (!isempty(kill_mode)) {
-                const char *kill_mode_property = "KillMode";
-
-                if (!dbus_message_iter_open_container(&sub, DBUS_TYPE_STRUCT, NULL, &sub2) ||
-                    !dbus_message_iter_append_basic(&sub2, DBUS_TYPE_STRING, &kill_mode_property) ||
-                    !dbus_message_iter_open_container(&sub2, DBUS_TYPE_VARIANT, "s", &sub3) ||
-                    !dbus_message_iter_append_basic(&sub3, DBUS_TYPE_STRING, &kill_mode) ||
-                    !dbus_message_iter_close_container(&sub2, &sub3) ||
-                    !dbus_message_iter_close_container(&sub, &sub2))
-                        return log_oom();
-        }
-
         /* 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
@@ -2793,6 +2774,36 @@ int manager_stop_unit(Manager *manager, const char *unit, DBusError *error, char
         return 1;
 }
 
+int manager_abandon_scope(Manager *manager, const char *scope, DBusError *error) {
+        _cleanup_dbus_message_unref_ DBusMessage *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 = bus_method_call_with_reply(
+                manager->bus,
+                "org.freedesktop.systemd1",
+                path,
+                "org.freedesktop.systemd1.Scope",
+                "Abandon",
+                &reply,
+                error,
+                DBUS_TYPE_INVALID);
+
+        if (r < 0) {
+                log_error("Failed to abandon scope %s", scope);
+                return r;
+        }
+
+        return 1;
+}
+
 int manager_kill_unit(Manager *manager, const char *unit, KillWho who, int signo, DBusError *error) {
         _cleanup_dbus_message_unref_ DBusMessage *reply = NULL;
         const char *w;
diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index 27aa335..78e6d74 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -24,6 +24,7 @@
 #include <unistd.h>
 #include <sys/epoll.h>
 #include <fcntl.h>
+#include <sys/timerfd.h>
 
 #include <systemd/sd-id128.h>
 #include <systemd/sd-messages.h>
@@ -36,6 +37,8 @@
 #include "dbus-common.h"
 #include "logind-session.h"
 
+#define RELEASE_SEC 20
+
 static unsigned devt_hash_func(const void *p) {
         uint64_t u = *(const dev_t*)p;
 
@@ -505,7 +508,6 @@ static int session_start_scope(Session *s) {
 
         if (!s->scope) {
                 _cleanup_free_ char *description = NULL;
-                const char *kill_mode;
                 char *scope, *job;
 
                 description = strjoin("Session ", s->id, " of user ", s->user->name, NULL);
@@ -516,9 +518,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(&error, r), error.name);
@@ -579,23 +579,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);
 
         if (s->seat) {
-                seat_save(s->seat);
-
                 if (s->seat->active == s)
                         seat_send_changed(s->seat, "Sessions\0ActiveSession\0");
                 else
                         seat_send_changed(s->seat, "Sessions\0");
         }
 
-        user_send_changed(s->user, "Sessions\0");
-
         return 0;
 }
 
@@ -611,15 +610,24 @@ 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(&error, r));
-                dbus_error_free(&error);
-                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(&error, r));
+                        dbus_error_free(&error);
+                        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(&error, r));
+                        dbus_error_free(&error);
+                        return r;
+                }
+        }
 
         return 0;
 }
@@ -644,6 +652,19 @@ static int session_unlink_x11_socket(Session *s) {
         return r < 0 ? -errno : 0;
 }
 
+static void session_close_timer_fd(Session *s) {
+        assert(s);
+
+        if (s->timer_fd <= 0)
+                return;
+
+        hashmap_remove(s->manager->timer_fds, INT_TO_PTR(s->timer_fd + 1));
+        epoll_ctl(s->manager->epoll_fd, EPOLL_CTL_DEL, s->timer_fd, NULL);
+
+        close_nointr(s->timer_fd);
+        s->timer_fd = -1;
+}
+
 int session_stop(Session *s) {
         int r;
 
@@ -652,11 +673,18 @@ int session_stop(Session *s) {
         if (!s->user)
                 return -ESTALE;
 
+        session_close_timer_fd(s);
+
+        /* We are going down, don't care about FIFOs anymore */
+        session_remove_fifo(s);
+
         /* Kill cgroup */
         r = session_stop_scope(s);
 
         session_save(s);
 
+        s->stopping = true;
+
         return r;
 }
 
@@ -678,6 +706,8 @@ int session_finalize(Session *s) {
                            "MESSAGE=Removed session %s.", s->id,
                            NULL);
 
+        session_close_timer_fd(s);
+
         /* Kill session devices */
         while ((sd = hashmap_first(s->devices)))
                 session_device_free(sd);
@@ -698,16 +728,64 @@ int session_finalize(Session *s) {
                 if (s->seat->active == s)
                         seat_set_active(s->seat, NULL);
 
-                seat_send_changed(s->seat, "Sessions\0");
                 seat_save(s->seat);
+                seat_send_changed(s->seat, "Sessions\0");
         }
 
-        user_send_changed(s->user, "Sessions\0");
         user_save(s->user);
+        user_send_changed(s->user, "Sessions\0");
 
         return r;
 }
 
+void session_release(Session *s) {
+        int r;
+
+        struct itimerspec its = { .it_value.tv_sec = RELEASE_SEC };
+        struct epoll_event ev = {};
+
+        assert(s);
+
+        if (!s->started || s->stopping)
+                return;
+
+        if (s->timer_fd >= 0)
+                return;
+
+        s->timer_fd = timerfd_create(CLOCK_MONOTONIC, TFD_NONBLOCK|TFD_CLOEXEC);
+        if (s->timer_fd < 0) {
+                log_error("Failed to create session release timer fd");
+                goto out;
+        }
+
+        r = hashmap_put(s->manager->timer_fds, INT_TO_PTR(s->timer_fd + 1), s);
+        if (r < 0) {
+                log_error("Failed to store session release timer fd");
+                goto out;
+        }
+
+        ev.events = EPOLLONESHOT;
+        ev.data.u32 = FD_OTHER_BASE + s->timer_fd;
+
+        r = epoll_ctl(s->manager->epoll_fd, EPOLL_CTL_ADD, s->timer_fd, &ev);
+        if (r < 0) {
+                log_error("Failed to add session release timer fd to epoll instance");
+                goto out;
+        }
+
+        r = timerfd_settime(s->timer_fd, TFD_TIMER_ABSTIME, &its, NULL);
+        if (r < 0) {
+                log_error("Failed to arm timer : %m");
+                goto out;
+        }
+
+out:
+        if (s->timer_fd >= 0) {
+                close_nointr(s->timer_fd);
+                s->timer_fd = -1;
+        }
+}
+
 bool session_is_active(Session *s) {
         assert(s);
 
@@ -904,8 +982,6 @@ void session_remove_fifo(Session *s) {
 }
 
 int session_check_gc(Session *s, bool drop_not_started) {
-        int r;
-
         assert(s);
 
         if (drop_not_started && !s->started)
@@ -915,11 +991,7 @@ int session_check_gc(Session *s, bool drop_not_started) {
                 return 0;
 
         if (s->fifo_fd >= 0) {
-                r = pipe_eof(s->fifo_fd);
-                if (r < 0)
-                        return r;
-
-                if (r == 0)
+                if (pipe_eof(s->fifo_fd) <= 0)
                         return 1;
         }
 
@@ -945,15 +1017,15 @@ void session_add_to_gc_queue(Session *s) {
 SessionState session_get_state(Session *s) {
         assert(s);
 
+        if (s->stopping || s->timer_fd >= 0)
+                return SESSION_CLOSING;
+
         if (s->closing)
                 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;
 
diff --git a/src/login/logind-session.h b/src/login/logind-session.h
index f175a89..9b76582 100644
--- a/src/login/logind-session.h
+++ b/src/login/logind-session.h
@@ -98,11 +98,14 @@ struct Session {
         int fifo_fd;
         char *fifo_path;
 
+        int timer_fd;
+
         bool idle_hint;
         dual_timestamp idle_hint_timestamp;
 
         bool in_gc_queue:1;
         bool started:1;
+        bool stopping:1;
         bool closing:1;
 
         DBusMessage *create_message;
@@ -130,6 +133,7 @@ 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 8e7256b..653574e 100644
--- a/src/login/logind-user.c
+++ b/src/login/logind-user.c
@@ -487,6 +487,8 @@ int user_stop(User *u) {
         if (k < 0)
                 r = k;
 
+        u->stopping = true;
+
         user_save(u);
 
         return r;
@@ -602,22 +604,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 a36f456..a12532e 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 0628032..5180be7 100644
--- a/src/login/logind.c
+++ b/src/login/logind.c
@@ -80,10 +80,11 @@ Manager *manager_new(void) {
         m->session_fds = hashmap_new(trivial_hash_func, trivial_compare_func);
         m->inhibitor_fds = hashmap_new(trivial_hash_func, trivial_compare_func);
         m->button_fds = hashmap_new(trivial_hash_func, trivial_compare_func);
+        m->timer_fds = hashmap_new(trivial_hash_func, trivial_compare_func);
 
         if (!m->devices || !m->seats || !m->sessions || !m->users || !m->inhibitors || !m->buttons || !m->busnames ||
             !m->user_units || !m->session_units ||
-            !m->session_fds || !m->inhibitor_fds || !m->button_fds) {
+            !m->session_fds || !m->inhibitor_fds || !m->button_fds || !m->timer_fds) {
                 manager_free(m);
                 return NULL;
         }
@@ -149,6 +150,7 @@ void manager_free(Manager *m) {
         hashmap_free(m->session_fds);
         hashmap_free(m->inhibitor_fds);
         hashmap_free(m->button_fds);
+        hashmap_free(m->timer_fds);
 
         if (m->console_active_fd >= 0)
                 close_nointr_nofail(m->console_active_fd);
@@ -620,6 +622,13 @@ static void manager_dispatch_other(Manager *m, int fd) {
                 return;
         }
 
+        s = hashmap_get(m->timer_fds, INT_TO_PTR(fd + 1));
+        if (s) {
+                assert(s->timer_fd == fd);
+                session_stop(s);
+                return;
+        }
+
         i = hashmap_get(m->inhibitor_fds, INT_TO_PTR(fd + 1));
         if (i) {
                 assert(i->fifo_fd == fd);
@@ -942,8 +951,12 @@ void manager_gc(Manager *m, bool drop_not_started) {
                 LIST_REMOVE(Session, gc_queue, m->session_gc_queue, session);
                 session->in_gc_queue = false;
 
-                if (session_check_gc(session, drop_not_started) == 0) {
+                /* 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);
+
+                if (!session_check_gc(session, drop_not_started)) {
                         session_finalize(session);
                         session_free(session);
                 }
@@ -953,8 +966,11 @@ void manager_gc(Manager *m, bool drop_not_started) {
                 LIST_REMOVE(User, gc_queue, m->user_gc_queue, user);
                 user->in_gc_queue = false;
 
-                if (user_check_gc(user, drop_not_started) == 0) {
+                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);
                 }
@@ -1032,6 +1048,7 @@ finish:
 
         return r;
 }
+
 int manager_startup(Manager *m) {
         int r;
         Seat *seat;
diff --git a/src/login/logind.h b/src/login/logind.h
index 9e6296c..0d2248f 100644
--- a/src/login/logind.h
+++ b/src/login/logind.h
@@ -88,6 +88,7 @@ struct Manager {
         Hashmap *session_fds;
         Hashmap *inhibitor_fds;
         Hashmap *button_fds;
+        Hashmap *timer_fds;
 
         usec_t inhibit_delay_max;
 
@@ -183,9 +184,10 @@ int manager_send_changed(Manager *manager, const char *properties);
 
 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, DBusError *error, char **job);
+int manager_start_scope(Manager *manager, const char *scope, pid_t pid, const char *slice, const char *description, const char *after, DBusError *error, char **job);
 int manager_start_unit(Manager *manager, const char *unit, DBusError *error, char **job);
 int manager_stop_unit(Manager *manager, const char *unit, DBusError *error, char **job);
+int manager_abandon_scope(Manager *manager, const char *scope, DBusError *error);
 int manager_kill_unit(Manager *manager, const char *unit, KillWho who, int signo, DBusError *error);
 int manager_unit_is_active(Manager *manager, const char *unit);
 
diff --git a/src/login/pam-module.c b/src/login/pam-module.c
index 22d9733..7bd4783 100644
--- a/src/login/pam-module.c
+++ b/src/login/pam-module.c
@@ -478,7 +478,7 @@ _public_ PAM_EXTERN int pam_sm_close_session(
                 int flags,
                 int argc, const char **argv) {
 
-        const void *p = NULL, *existing = NULL;
+        const void *existing = NULL;
         const char *id;
         DBusConnection *bus = NULL;
         DBusMessage *m = NULL, *reply = NULL;
@@ -535,12 +535,15 @@ _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. */
         r = PAM_SUCCESS;
 
 finish:
-        pam_get_data(handle, "systemd.session-fd", &p);
-        if (p)
-                close_nointr(PTR_TO_INT(p) - 1);
 
         dbus_error_free(&error);