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