From: Simon McVittie <simon.mcvittie@collabora.co.uk>
Date: Wed, 17 Jun 2015 15:45:49 +0000 (+0100)
Subject: logind: save /run/systemd/users/UID before starting user@.service
X-Git-Tag: v226.4~1^2~270
X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?p=elogind.git;a=commitdiff_plain;h=31e57d94c86e0a4f332f30033b10e402fec4aac7

logind: save /run/systemd/users/UID before starting user@.service

Previously, this had a race condition during a user's first login.
Some component calls CreateSession (most likely by a PAM service
other than 'systemd-user' running pam_systemd), with the following
results:

- logind:
  * create the user's XDG_RUNTIME_DIR
  * tell pid 1 to create user-UID.slice
  * tell pid 1 to start user@UID.service

Then these two processes race:

- logind:
  * save information including XDG_RUNTIME_DIR to /run/systemd/users/UID

- the subprocess of pid 1 responsible for user@service:
  * start a 'systemd-user' PAM session, which reads XDG_RUNTIME_DIR
    and puts it in the environment
  * run systemd --user, which requires XDG_RUNTIME_DIR in the
    environment

If logind wins the race, which usually happens, everything is fine;
but if the subprocesses of pid 1 win the race, which can happen
under load, then systemd --user exits unsuccessfully.

To avoid this race, we have to write out /run/systemd/users/UID
even though the service has not "officially" started yet;
previously this did an early-return without saving anything.
Record its state as OPENING in this case.

Bug: https://github.com/systemd/systemd/issues/232
Reviewed-by: Philip Withnall <philip.withnall@collabora.co.uk>
---

diff --git a/src/login/logind-user.c b/src/login/logind-user.c
index 738e69cce..21d726812 100644
--- a/src/login/logind-user.c
+++ b/src/login/logind-user.c
@@ -106,7 +106,7 @@ void user_free(User *u) {
         free(u);
 }
 
-int user_save(User *u) {
+static int user_save_internal(User *u) {
         _cleanup_free_ char *temp_path = NULL;
         _cleanup_fclose_ FILE *f = NULL;
         int r;
@@ -114,9 +114,6 @@ int user_save(User *u) {
         assert(u);
         assert(u->state_file);
 
-        if (!u->started)
-                return 0;
-
         r = mkdir_safe_label("/run/systemd/users", 0755, 0, 0);
         if (r < 0)
                 goto finish;
@@ -259,6 +256,15 @@ finish:
         return r;
 }
 
+int user_save(User *u) {
+        assert(u);
+
+        if (!u->started)
+                return 0;
+
+        return user_save_internal (u);
+}
+
 int user_load(User *u) {
         _cleanup_free_ char *display = NULL, *realtime = NULL, *monotonic = NULL;
         Session *s = NULL;
@@ -458,6 +464,12 @@ int user_start(User *u) {
         if (r < 0)
                 return r;
 
+        /* Save the user data so far, because pam_systemd will read the
+         * XDG_RUNTIME_DIR out of it while starting up systemd --user.
+         * We need to do user_save_internal() because we have not
+         * "officially" started yet. */
+        user_save_internal(u);
+
         /* Spawn user systemd */
         r = user_start_service(u);
         if (r < 0)
@@ -709,7 +721,7 @@ UserState user_get_state(User *u) {
         if (u->stopping)
                 return USER_CLOSING;
 
-        if (u->slice_job || u->service_job)
+        if (!u->started || u->slice_job || u->service_job)
                 return USER_OPENING;
 
         if (u->sessions) {