From 5f41d1f10fd97e93517b6a762b1bec247f4d1171 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 6 Feb 2014 18:32:14 +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. --- src/login/logind-dbus.c | 51 +++++++++++++------ src/login/logind-session.c | 100 +++++++++++++++++++++++++------------ src/login/logind-session.h | 5 +- src/login/logind-user.c | 23 ++++++--- src/login/logind-user.h | 1 + src/login/logind.c | 14 +++++- src/login/logind.h | 3 +- src/login/pam-module.c | 28 ++++------- 8 files changed, 147 insertions(+), 78 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 474596177..40885557c 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 bec59c044..95105e532 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 7bf193203..42552bc2b 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 bdb691563..fdbf6e3aa 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 006288056..b0fefe9b9 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 48da7b173..a6f84e853 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 b84137c78..c1a5b6a80 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 3b2966b30..79a9042ff 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; } -- 2.30.2