[systemd-devel] [PATCH 2/7] logind: close races on user and session states during login
Djalal Harouni
tixxdz at opendz.org
Thu Feb 6 12:37:15 PST 2014
Currently the user and session states are not stable, they are affected
by several races during login:
1) session state:
To get the session state the function session_get_state() is used.
Opening state:
At login the D-Bus CreateSession() method will call session_start() which
calls user_start() and session_start_scope() to queue the scope job and
set the session->scope_job
=> session_get_state() == SESSION_OPENING (correct)
Then execution will continue from session_send_create() which is called
by the D-Bus match_job_removed() signal. math_job_removed() will check if
this is the session scope and if this is the previously queued scope job.
If so it will clear the session->scope_job
=> session_get_state() == SESSION_CLOSING (incorrect)
(session closing since fifo_fd == -1)
So scope job has finished and scope was created successfully, later the
session_send_create_reply() will wait for the session scope *and* the
user service to be successfully created.
/* user service is still pending */
if (s->scope_job || s->user->service_job))
return 0
=> session_get_state() == SESSION_CLOSING (incorrect)
else
session_create_fifo() /* fifo_fd finally created */
=> session_get_state() == SESSION_ACTIVE (correct)
To sum it up, current state during login:
"SESSION_OPENING"=>"SESSION_CLOSING"x2=>"SESSION_ACTIVE"
To fix the session state and remove the two incorrect SESSION_CLOSING,
we do not clear up the "session->scope_job" when we detect that this is
the session scope, we setup a temporary variable "scope_job" that will
get the current value of "session->scope_job" and update it if
necessary.
Add a new "active" variable to check if the session scope and user
service jobs are still being created.
Update session_jobs_replay() and session_send_create_reply() function to
receive the "opening" variable as an argument, so it will still wait for
the scope and service jobs to finish before creating the session fifo.
The "session->scope_job" will be cleared when session_jobs_reply()
finishes. This ensures that the state will just go from:
"SESSION_OPENING" => "SESSION_ACTIVE"
2) user state:
To get the user state the function user_get_state() is used.
I'll add that the user_get_state() and session_get_state() do not have
the same logic when it comes to sessions, this will just add ambiguity.
user_get_state() calls session_is_active() before checking
session_get_state(), and session_is_active() will return true right from
the start since the logic is set during D-Bus CreateSession(). This will
we be fixed in the followup patches.
Opening state:
At login we have session_start() which calls user_start()
here we already:
=> user_get_state() == USER_ACTIVE (incorrect)
(not fixed in this patch)
user_start() calls:
user_start_slice() queue the slice and set user->slice_job
user_start_service() queue the service and set user->service_job
=> user_get_state() == USER_OPENING (correct)
Then execution will continue from session_send_create() which is called
by the D-Bus match_job_removed() signal. math_job_removed() will check if
these are the user service and slice and if they are the previously queued
jobs. If so it will clear the: user->service_job and user->slice_job
=> user_get_state() == USER_ACTIVE (incorrect)
(incorrect since the fifo_fd has not been created,
here the state should stay USER_OPENING)
Later when the user service is created successfully,
session_send_create_reply() will also wait for the session scope to be
created. If so then session_send_create_reply() will continue and call
session_create_fifo()
=> user_get_state() == USER_ACTIVE (correct)
(fifo_fd was created)
To fix this, we use the same logic as used to fix session states. In
order to remove the incorrect state when the fifo_fd is not created but
the state shows USER_ACTIVE, we do not clear the "user->service_job" and
"user->slice_job" right away, we store the state in a temporary variable
"service_job" and update it later if we detect that this is the user
service.
The new "active" variable will be used to check if the session scope and
user service are still being created. If so we'll wait for the next
match_job_removed() signal and continue, otherwise we proceed with
session_jobs_reply() and session_send_create_reply() in order to notify
clients.
---
src/login/logind-dbus.c | 44 ++++++++++++++++++++++++++++++-----------
src/login/logind-session-dbus.c | 8 +++++---
src/login/logind-session.h | 2 +-
3 files changed, 39 insertions(+), 15 deletions(-)
diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
index 7b050fb..0560707 100644
--- a/src/login/logind-dbus.c
+++ b/src/login/logind-dbus.c
@@ -1919,7 +1919,9 @@ const sd_bus_vtable manager_vtable[] = {
SD_BUS_VTABLE_END
};
-static int session_jobs_reply(Session *s, const char *unit, const char *result) {
+/* session_jobs_reply() calls session_send_create_reply() to
+ * notify client that they are able to login now. */
+static int session_jobs_reply(Session *s, const char *unit, const char *result, bool opening) {
int r = 0;
assert(s);
@@ -1929,12 +1931,12 @@ static int session_jobs_reply(Session *s, const char *unit, const char *result)
return r;
if (streq(result, "done"))
- r = session_send_create_reply(s, NULL);
+ r = session_send_create_reply(s, NULL, opening);
else {
_cleanup_bus_error_free_ sd_bus_error e = SD_BUS_ERROR_NULL;
sd_bus_error_setf(&e, BUS_ERROR_JOB_FAILED, "Start job for unit %s failed with '%s'", unit, result);
- r = session_send_create_reply(s, &e);
+ r = session_send_create_reply(s, &e, opening);
}
return r;
@@ -1973,22 +1975,46 @@ int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_b
session = hashmap_get(m->session_units, unit);
if (session) {
+ bool active, scope_job = !!session->scope_job;
- if (streq_ptr(path, session->scope_job)) {
+ /* Set to false if the scope job has finished */
+ if (streq_ptr(path, session->scope_job))
+ scope_job = false;
+
+ /* If the session scope and the user service are still
+ * being created this will be set to true, otherwise
+ * it will be false */
+ active = scope_job || !!session->user->service_job;
+ session_jobs_reply(session, unit, result, active);
+
+ if (!scope_job) {
+ /* Clean this up after session_jobs_reply() */
free(session->scope_job);
session->scope_job = NULL;
}
- session_jobs_reply(session, unit, result);
-
session_save(session);
session_add_to_gc_queue(session);
}
user = hashmap_get(m->user_units, unit);
if (user) {
+ bool active, service_job = !!user->service_job;
+
+ /* Set to false if the user service has finished */
+ if (streq_ptr(path, user->service_job))
+ service_job = false;
+
+ LIST_FOREACH(sessions_by_user, session, user->sessions) {
+ /* If the session scope and the user service are
+ * still being created this will be set to true,
+ * otherwise it will be false */
+ active = service_job || !!session->scope_job;
+ session_jobs_reply(session, unit, result, active);
+ }
- if (streq_ptr(path, user->service_job)) {
+ if (!service_job) {
+ /* Clean this up after session_jobs_reply() */
free(user->service_job);
user->service_job = NULL;
}
@@ -1998,10 +2024,6 @@ int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_b
user->slice_job = NULL;
}
- LIST_FOREACH(sessions_by_user, session, user->sessions) {
- session_jobs_reply(session, unit, result);
- }
-
user_save(user);
user_add_to_gc_queue(user);
}
diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c
index 7ee4956..54db864 100644
--- a/src/login/logind-session-dbus.c
+++ b/src/login/logind-session-dbus.c
@@ -641,7 +641,7 @@ int session_send_lock_all(Manager *m, bool lock) {
return r;
}
-int session_send_create_reply(Session *s, sd_bus_error *error) {
+int session_send_create_reply(Session *s, sd_bus_error *error, bool opening) {
_cleanup_bus_message_unref_ sd_bus_message *c = NULL;
_cleanup_close_ int fifo_fd = -1;
_cleanup_free_ char *p = NULL;
@@ -650,12 +650,12 @@ int session_send_create_reply(Session *s, sd_bus_error *error) {
/* This is called after the session scope and the user service
* were successfully created, and finishes where
- * bus_manager_create_session() left off. */
+ * method_create_session() left off. */
if (!s->create_message)
return 0;
- if (!sd_bus_error_is_set(error) && (s->scope_job || s->user->service_job))
+ if (!sd_bus_error_is_set(error) && opening)
return 0;
c = s->create_message;
@@ -664,6 +664,8 @@ int session_send_create_reply(Session *s, sd_bus_error *error) {
if (error)
return sd_bus_reply_method_error(c, error);
+ /* At this stage the session scope and the user service
+ * were successfully created */
fifo_fd = session_create_fifo(s);
if (fifo_fd < 0)
return fifo_fd;
diff --git a/src/login/logind-session.h b/src/login/logind-session.h
index 7bf1932..ebe3902 100644
--- a/src/login/logind-session.h
+++ b/src/login/logind-session.h
@@ -152,7 +152,7 @@ int session_send_changed(Session *s, const char *properties, ...) _sentinel_;
int session_send_lock(Session *s, bool lock);
int session_send_lock_all(Manager *m, bool lock);
-int session_send_create_reply(Session *s, sd_bus_error *error);
+int session_send_create_reply(Session *s, sd_bus_error *error, bool opening);
const char* session_state_to_string(SessionState t) _const_;
SessionState session_state_from_string(const char *s) _pure_;
--
1.8.3.1
More information about the systemd-devel
mailing list