From 553a962a570cade5c44953593209d25e28ec5145 Mon Sep 17 00:00:00 2001 From: Michal Sekletar 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 #include #include +#include #include #include @@ -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);