Discussion:
[PATCH v2 0/7] logind: close races on user and session states
(too old to reply)
Djalal Harouni
2014-02-06 20:37:13 UTC
Permalink
Summary:
Currently logind will not clear sessions on logout. The bug is confirmed
for getty and ssh logins. This series is preparation for next patches to
address that bug, it does not fix it.

However, this series also fixe real races on user and session states.
This ensures that user_save() and session_save() functions will write the
correct user and session state to the state files.


The logind bug was already discussed here:
http://lists.freedesktop.org/archives/systemd-devel/2014-January/015968.html

Patches were proposed, but they failed to address the bug since there
are other problems related to user and session states:
http://lists.freedesktop.org/archives/systemd-devel/2014-January/016372.html

A first version to fix these race conditions on user and sessions
states was proposed:
http://lists.freedesktop.org/archives/systemd-devel/2014-January/016373.html


This series is a v2, it will close all the discovered races on user and
session states. The commit logs will tell you the story of each case.


Now as noted above, this series fix real races and in the same time it
is needed to fix the logind bug where sessions are not cleaned after
logouts.

Proposed plan to clean logind closed sessions:
1) Make the user and session states stable (this series fix it).

2) Improve session_check_gc() and user_check_gc() to check if:
the state is closing and the cgroup is empty.

3) Push session and user into the gc during logout, in pam_systemd
method_release_session()

Now I've a patch that implements 2) and 3) on top of 1), sometimes it
will work and successfully clean closed sessions, and sometimes it will
not. This is due to a race when we close the session and try to
terminate session processes and in the same time we are trying to see if
the cgroup is empty... which is another problem on its own.


So here are the patches, please consider this series since it will fix
real races and it will make sure that user_save() and session_save()
will write the correct state to the state files.


Patches 1,6,7 are code cleanup.

Patches 2,3,4,5 close races on user and session states.


Djalal Harouni (7):
0001 logind: add function session_jobs_reply() to unify the create reply
unify shared code in a single function.

0002 logind: close races on user and session states during login
0003 logind: close races on session state at logout
0004 logind: close races on user state at logout
close race conditions on user and session states.

0005 logind: just call session_get_state() to get the session state
make user_get_state() consistent with session_get_state()

0006 logind: add user_is_opening() and session_is_opening()
0007 logind: do not call session_jobs_reply() on CLOSING
make sure to not call session_send_create_reply() when jobs finish
during closing state.

src/login/logind-dbus.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------
src/login/logind-session-dbus.c | 11 ++++++++---
src/login/logind-session.c | 10 +++++++++-
src/login/logind-session.h | 4 +++-
src/login/logind-user.c | 20 +++++++++++++++++---
src/login/logind-user.h | 3 +++
6 files changed, 99 insertions(+), 36 deletions(-)
Djalal Harouni
2014-02-06 20:37:14 UTC
Permalink
The session_send_create_reply() function which notifies clients about
session creation is used for both session and user units. Unify the
shared code in a new function session_jobs_reply().

The session_save() will be called unconditionally on sessions since it
does not make sense to only call it if '!session->started', this will
also allow to update the session state as soon as possible.
---
src/login/logind-dbus.c | 46 ++++++++++++++++++++++++----------------------
1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
index 4745961..7b050fb 100644
--- a/src/login/logind-dbus.c
+++ b/src/login/logind-dbus.c
@@ -1919,6 +1919,27 @@ const sd_bus_vtable manager_vtable[] = {
SD_BUS_VTABLE_END
};

+static int session_jobs_reply(Session *s, const char *unit, const char *result) {
+ int r = 0;
+
+ assert(s);
+ assert(unit);
+
+ if (!s->started)
+ return r;
+
+ if (streq(result, "done"))
+ r = session_send_create_reply(s, NULL);
+ 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);
+ }
+
+ return r;
+}
+
int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_bus_error *error) {
const char *path, *result, *unit;
Manager *m = userdata;
@@ -1958,18 +1979,9 @@ int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_b
session->scope_job = NULL;
}

- if (session->started) {
- if (streq(result, "done"))
- session_send_create_reply(session, NULL);
- 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);
- session_send_create_reply(session, &e);
- }
- } else
- session_save(session);
+ session_jobs_reply(session, unit, result);

+ session_save(session);
session_add_to_gc_queue(session);
}

@@ -1987,17 +1999,7 @@ int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_b
}

LIST_FOREACH(sessions_by_user, session, user->sessions) {
- if (!session->started)
- continue;
-
- if (streq(result, "done"))
- session_send_create_reply(session, NULL);
- 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);
- session_send_create_reply(session, &e);
- }
+ session_jobs_reply(session, unit, result);
}

user_save(user);
--
1.8.3.1
Lennart Poettering
2014-02-07 15:35:06 UTC
Permalink
On Thu, 06.02.14 21:37, Djalal Harouni (***@opendz.org) wrote:

Applied this one! Thanks!
Post by Djalal Harouni
The session_send_create_reply() function which notifies clients about
session creation is used for both session and user units. Unify the
shared code in a new function session_jobs_reply().
The session_save() will be called unconditionally on sessions since it
does not make sense to only call it if '!session->started', this will
also allow to update the session state as soon as possible.
---
src/login/logind-dbus.c | 46 ++++++++++++++++++++++++----------------------
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
index 4745961..7b050fb 100644
--- a/src/login/logind-dbus.c
+++ b/src/login/logind-dbus.c
@@ -1919,6 +1919,27 @@ const sd_bus_vtable manager_vtable[] = {
SD_BUS_VTABLE_END
};
+static int session_jobs_reply(Session *s, const char *unit, const char *result) {
+ int r = 0;
+
+ assert(s);
+ assert(unit);
+
+ if (!s->started)
+ return r;
+
+ if (streq(result, "done"))
+ r = session_send_create_reply(s, NULL);
+ 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);
+ }
+
+ return r;
+}
+
int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_bus_error *error) {
const char *path, *result, *unit;
Manager *m = userdata;
@@ -1958,18 +1979,9 @@ int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_b
session->scope_job = NULL;
}
- if (session->started) {
- if (streq(result, "done"))
- session_send_create_reply(session, NULL);
- 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);
- session_send_create_reply(session, &e);
- }
- } else
- session_save(session);
+ session_jobs_reply(session, unit, result);
+ session_save(session);
session_add_to_gc_queue(session);
}
@@ -1987,17 +1999,7 @@ int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_b
}
LIST_FOREACH(sessions_by_user, session, user->sessions) {
- if (!session->started)
- continue;
-
- if (streq(result, "done"))
- session_send_create_reply(session, NULL);
- 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);
- session_send_create_reply(session, &e);
- }
+ session_jobs_reply(session, unit, result);
}
user_save(user);
Lennart
--
Lennart Poettering, Red Hat
Djalal Harouni
2014-02-06 20:37:15 UTC
Permalink
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
Lennart Poettering
2014-02-07 15:48:39 UTC
Permalink
On Thu, 06.02.14 21:37, Djalal Harouni (***@opendz.org) wrote:

I think this one I fixed by adding a new "stopping" variable. Could you check?
Post by Djalal Harouni
Currently the user and session states are not stable, they are affected
To get the session state the function session_get_state() is used.
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)
"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()
"SESSION_OPENING" => "SESSION_ACTIVE"
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.
At login we have session_start() which calls user_start()
=> user_get_state() == USER_ACTIVE (incorrect)
(not fixed in this patch)
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_;
Lennart
--
Lennart Poettering, Red Hat
Djalal Harouni
2014-02-13 22:19:31 UTC
Permalink
Post by Lennart Poettering
I think this one I fixed by adding a new "stopping" variable. Could you check?
Yes, the stopping variable caught most of the races, I've sent another
patch that will close which I think is the last one:
http://lists.freedesktop.org/archives/systemd-devel/2014-February/016889.html

Thanks Lennart!
Post by Lennart Poettering
Post by Djalal Harouni
Currently the user and session states are not stable, they are affected
To get the session state the function session_get_state() is used.
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)
"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()
"SESSION_OPENING" => "SESSION_ACTIVE"
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.
At login we have session_start() which calls user_start()
=> user_get_state() == USER_ACTIVE (incorrect)
(not fixed in this patch)
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_;
Lennart
--
Lennart Poettering, Red Hat
--
Djalal Harouni
http://opendz.org
Zbigniew Jędrzejewski-Szmek
2014-02-17 07:32:42 UTC
Permalink
At login there is a small race window where session_get_state() will
return SESSION_ACTIVE instead of SESSION_OPENING. This must be fixed
since during that time there are calls to session_save() to save
session states and we want to write the correct state.
When we queue the start scope and service jobs, we wait for both of them
"session_jobs_reply() => session_send_create_reply()"
to create the session fifo and notify clients.
However, in the match_job_removed() D-Bus signal, we may hit situations
where the scope job has successfully finished and we are still waiting
for the user service job to finish. During that time the
"session->scope_job" will be freed and set to NULL, this makes
session_get_state() return SESSION_ACTIVE before it is really active, it
should return SESSION_OPENING since we are still waiting for the service
job to finish in order to create the session fifo.
To fix this, we also check if the session fifo fd was created, if so then
the session has entered the SESSION_ACTIVE state, if not then it is still
in the SESSION_OPENING state and it is waiting for the scope and service
jobs to finish.
Looks correct. Applied.

Zbyszek

Djalal Harouni
2014-02-06 20:37:16 UTC
Permalink
To get the state of the session, the session_get_state() is used.
This function will check if the session->scope_job is set then it will
automatically return SESSION_OPENING. This is buggy in the context of
session closing:

At logout or D-Bus TerminateSession() fifo_fd is removed:

=> session_get_state() == SESSION_CLOSING (correct)

Then we have session_stop() which calls session_stop_scope(), this
will queue the scope job to be removed and set the session->scope_job

=> session_get_state() == SESSION_OPENING (incorrect)

After the scope job is removed the state will be again correct

=> session_get_state() == SESSION_CLOSING (correct)

To fix this and make sure that the state will always be SESSION_CLOSING
we add a flag that is used to differentiate between SESSION_OPENING and
SESSION_CLOSING.

The 'scope_opening' flag will be set to true only during real session
opening in session_start_scope(), and it will be set to false just after
the session fifo fd was successfully created, which means that during
session closing it will be false.

And update session_get_state() to check if the 'scope_opening' is true
before returning the SESSION_OPENING, if it is not then SESSION_CLOSING
will be returned which is the correct behaviour.
---
src/login/logind-session-dbus.c | 3 +++
src/login/logind-session.c | 4 +++-
src/login/logind-session.h | 1 +
3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c
index 54db864..099ade6 100644
--- a/src/login/logind-session-dbus.c
+++ b/src/login/logind-session-dbus.c
@@ -670,6 +670,9 @@ int session_send_create_reply(Session *s, sd_bus_error *error, bool opening) {
if (fifo_fd < 0)
return fifo_fd;

+ /* Clean this up as soon as we have the fifo_fd */
+ s->scope_opening = false;
+
/* Update the session state file before we notify the client
* about the result. */
session_save(s);
diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index bec59c0..848e8a1 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -496,6 +496,8 @@ static int session_start_scope(Session *s) {

free(s->scope_job);
s->scope_job = job;
+ /* session scope is being created */
+ s->scope_opening = true;
}
}

@@ -880,7 +882,7 @@ void session_add_to_gc_queue(Session *s) {
SessionState session_get_state(Session *s) {
assert(s);

- if (s->scope_job)
+ if (s->scope_opening)
return SESSION_OPENING;

if (s->fifo_fd < 0)
diff --git a/src/login/logind-session.h b/src/login/logind-session.h
index ebe3902..205491a 100644
--- a/src/login/logind-session.h
+++ b/src/login/logind-session.h
@@ -110,6 +110,7 @@ struct Session {

bool in_gc_queue:1;
bool started:1;
+ bool scope_opening:1; /* session scope is being created */

sd_bus_message *create_message;
--
1.8.3.1
Djalal Harouni
2014-02-06 20:37:17 UTC
Permalink
To get the state of the user, the user_get_state() is used.
This function will check if the user->slice_job or the user->service_job
are set then it will automatically return USER_OPENING. This is buggy
in the context of user closing:

At logout or D-Bus TerminateUser() calls user_stop()
user_stop() => session_stop() on sessions => session_stop_scope()

=> user_get_state() == USER_CLOSING or USER_ACTIVE or USER_ONLINE
(incorrect)

(depends on session closing execution and if we have finished the
session scope jobs or not, if all the scopes are being queued then
their session->scope_job will be set which means all sessions are in
the OPENING_STATE, so user state will be USER_ONLINE).

The previous patch improves session_get_state() logic, and if scopes
are being queued during logout then sessions are in SESSION_CLOSING
state which makes user_get_state() return USER_CLOSING.

So at user_stop()
user_stop_slice() queues the slice and sets user->slice_job
user_stop_service() queues the service and sets user->service_job

=> user_get_state() == USER_OPENING (incorrect)

After the slice and service jobs are removed the state will be again
correct.

=> user_get_state() == USER_CLOSING (correct)

To fix this and make sure that the state will always be USER_CLOSING
we add a flag that is used to differentiate between USER_OPENING and
USER_CLOSING when 'slice_job' and 'service_job' are set.

The 'slice_opening' flag for 'user->slice_job' and 'service_opening'
for 'user->service_job' are set to true only during real user opening,
later if the service and slice were successfully created their
corresponding 'opening' flag will be set to false, which means that
during user closing they are already false.

Update user_get_state() to check if 'slice_opening' or
'service_opening' are set, if so return USER_OPENING.
---
src/login/logind-dbus.c | 2 ++
src/login/logind-user.c | 6 +++++-
src/login/logind-user.h | 2 ++
3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
index 0560707..24482fd 100644
--- a/src/login/logind-dbus.c
+++ b/src/login/logind-dbus.c
@@ -2017,11 +2017,13 @@ int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_b
/* Clean this up after session_jobs_reply() */
free(user->service_job);
user->service_job = NULL;
+ user->service_opening = false;
}

if (streq_ptr(path, user->slice_job)) {
free(user->slice_job);
user->slice_job = NULL;
+ user->slice_opening = false;
}

user_save(user);
diff --git a/src/login/logind-user.c b/src/login/logind-user.c
index bdb6915..8183721 100644
--- a/src/login/logind-user.c
+++ b/src/login/logind-user.c
@@ -352,6 +352,8 @@ static int user_start_slice(User *u) {

free(u->slice_job);
u->slice_job = job;
+ /* User slice is being created */
+ u->slice_opening = true;
}
}

@@ -385,6 +387,8 @@ static int user_start_service(User *u) {

free(u->service_job);
u->service_job = job;
+ /* User service is being created */
+ u->service_opening = true;
}
}

@@ -637,7 +641,7 @@ UserState user_get_state(User *u) {

assert(u);

- if (u->slice_job || u->service_job)
+ if (u->slice_opening || u->service_opening)
return USER_OPENING;

LIST_FOREACH(sessions_by_user, i, u->sessions) {
diff --git a/src/login/logind-user.h b/src/login/logind-user.h
index 0062880..ac43361 100644
--- a/src/login/logind-user.h
+++ b/src/login/logind-user.h
@@ -61,6 +61,8 @@ struct User {

bool in_gc_queue:1;
bool started:1;
+ bool service_opening:1; /* User service is being created */
+ bool slice_opening:1; /* User slice is being created */

LIST_HEAD(Session, sessions);
LIST_FIELDS(User, gc_queue);
--
1.8.3.1
Djalal Harouni
2014-02-06 20:37:18 UTC
Permalink
In function user_get_state() remove the session_is_active() check, just
count on the session_get_state() function to get the correct session
state.

session_is_active() may return true before starting the session scope and
user service, this means it will return true even before the creation of
the session fifo_fd which will produce incorrect states.

Another point is that session_get_state() will check if the fifo_fd was
created before checking if the session is active, this is the correct
behaviour since the session fifo_fd should be considered the point of
session states synchronization.

So be consistent and follow the session_get_state() logic.
---
src/login/logind-user.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/login/logind-user.c b/src/login/logind-user.c
index 8183721..9ed216d 100644
--- a/src/login/logind-user.c
+++ b/src/login/logind-user.c
@@ -637,6 +637,7 @@ void user_add_to_gc_queue(User *u) {

UserState user_get_state(User *u) {
Session *i;
+ SessionState session_state;
bool all_closing = true;

assert(u);
@@ -645,9 +646,12 @@ UserState user_get_state(User *u) {
return USER_OPENING;

LIST_FOREACH(sessions_by_user, i, u->sessions) {
- if (session_is_active(i))
+ /* session_get_state() will check for fifo_fd */
+ session_state = session_get_state(i);
+
+ if (session_state == SESSION_ACTIVE)
return USER_ACTIVE;
- if (session_get_state(i) != SESSION_CLOSING)
+ else if (session_state != SESSION_CLOSING)
all_closing = false;
}
--
1.8.3.1
Lennart Poettering
2014-02-07 15:50:21 UTC
Permalink
On Thu, 06.02.14 21:37, Djalal Harouni (***@opendz.org) wrote:

This one looks good, but could you rebase it please?

Thanks!
Post by Djalal Harouni
In function user_get_state() remove the session_is_active() check, just
count on the session_get_state() function to get the correct session
state.
session_is_active() may return true before starting the session scope and
user service, this means it will return true even before the creation of
the session fifo_fd which will produce incorrect states.
Another point is that session_get_state() will check if the fifo_fd was
created before checking if the session is active, this is the correct
behaviour since the session fifo_fd should be considered the point of
session states synchronization.
So be consistent and follow the session_get_state() logic.
---
src/login/logind-user.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/login/logind-user.c b/src/login/logind-user.c
index 8183721..9ed216d 100644
--- a/src/login/logind-user.c
+++ b/src/login/logind-user.c
@@ -637,6 +637,7 @@ void user_add_to_gc_queue(User *u) {
UserState user_get_state(User *u) {
Session *i;
+ SessionState session_state;
bool all_closing = true;
assert(u);
@@ -645,9 +646,12 @@ UserState user_get_state(User *u) {
return USER_OPENING;
LIST_FOREACH(sessions_by_user, i, u->sessions) {
- if (session_is_active(i))
+ /* session_get_state() will check for fifo_fd */
+ session_state = session_get_state(i);
+
+ if (session_state == SESSION_ACTIVE)
return USER_ACTIVE;
- if (session_get_state(i) != SESSION_CLOSING)
+ else if (session_state != SESSION_CLOSING)
all_closing = false;
}
Lennart
--
Lennart Poettering, Red Hat
Djalal Harouni
2014-02-08 19:51:57 UTC
Permalink
In function user_get_state() remove the session_is_active() check, just
count on the session_get_state() function to get the correct session
state.

session_is_active() may return true before starting the session scope
and user service, this means it will return true even before the creation
of the session fifo_fd which will produce incorrect states.

So be consistent and just use session_get_state().
---
src/login/logind-user.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/login/logind-user.c b/src/login/logind-user.c
index fdbf6e3..2a11bab 100644
--- a/src/login/logind-user.c
+++ b/src/login/logind-user.c
@@ -648,9 +648,11 @@ UserState user_get_state(User *u) {
bool all_closing = true;

LIST_FOREACH(sessions_by_user, i, u->sessions) {
- if (session_is_active(i))
+ SessionState state = session_get_state(i);
+
+ if (state == SESSION_ACTIVE)
return USER_ACTIVE;
- if (session_get_state(i) != SESSION_CLOSING)
+ else if (state != SESSION_CLOSING)
all_closing = false;
}
--
1.8.3.1
Zbigniew Jędrzejewski-Szmek
2014-02-08 21:01:18 UTC
Permalink
Post by Djalal Harouni
In function user_get_state() remove the session_is_active() check, just
count on the session_get_state() function to get the correct session
state.
session_is_active() may return true before starting the session scope
and user service, this means it will return true even before the creation
of the session fifo_fd which will produce incorrect states.
So be consistent and just use session_get_state().
Sooo... with your patch applied, I see:

sshd[18756]: pam_unix(sshd:session): session closed for user user2
systemd-logind[18687]: Sent message type=method_call sender=n/a destination=org.freedesktop.systemd1 object=/org/freedesktop/systemd1/unit/session_2d10_2escope interface=org.freedesktop.systemd1.Scope member=Abandon cookie=27 reply_cookie=0 error=n/a

And nothing afterwards. User manager for user2 is undisturbed.

Zbyszek
Djalal Harouni
2014-02-08 21:20:53 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Djalal Harouni
In function user_get_state() remove the session_is_active() check, just
count on the session_get_state() function to get the correct session
state.
session_is_active() may return true before starting the session scope
and user service, this means it will return true even before the creation
of the session fifo_fd which will produce incorrect states.
So be consistent and just use session_get_state().
sshd[18756]: pam_unix(sshd:session): session closed for user user2
systemd-logind[18687]: Sent message type=method_call sender=n/a destination=org.freedesktop.systemd1 object=/org/freedesktop/systemd1/unit/session_2d10_2escope interface=org.freedesktop.systemd1.Scope member=Abandon cookie=27 reply_cookie=0 error=n/a
And nothing afterwards. User manager for user2 is undisturbed.
Ah this patch fixes the user state.

The one you should apply is from the other thread:
http://lists.freedesktop.org/archives/systemd-devel/2014-February/016754.html

It should work, please give it a try!
Post by Zbigniew Jędrzejewski-Szmek
Zbyszek
--
Djalal Harouni
http://opendz.org
Zbigniew Jędrzejewski-Szmek
2014-02-09 00:48:04 UTC
Permalink
Post by Djalal Harouni
Post by Zbigniew Jędrzejewski-Szmek
Post by Djalal Harouni
In function user_get_state() remove the session_is_active() check, just
count on the session_get_state() function to get the correct session
state.
session_is_active() may return true before starting the session scope
and user service, this means it will return true even before the creation
of the session fifo_fd which will produce incorrect states.
So be consistent and just use session_get_state().
sshd[18756]: pam_unix(sshd:session): session closed for user user2
systemd-logind[18687]: Sent message type=method_call sender=n/a destination=org.freedesktop.systemd1 object=/org/freedesktop/systemd1/unit/session_2d10_2escope interface=org.freedesktop.systemd1.Scope member=Abandon cookie=27 reply_cookie=0 error=n/a
And nothing afterwards. User manager for user2 is undisturbed.
Ah this patch fixes the user state.
http://lists.freedesktop.org/archives/systemd-devel/2014-February/016754.html
It should work, please give it a try!
You're right, I applied the wrong patch. It really seems that with your
* logind: use session_get_state() to get sessions state of the user
* logind: just call user_stop() if user_check_gc() returns false
things actually work.

One thing that still does not work is terminate-user, even though
kill-session works.

method_terminate_user -> user_stop -*> session_stop -> session_stop_scope -> manager_abandom_scope
-> user_stop_service
-> user_stop_slice

manager_stop_scope calls manager_shall_kill which returns false, so it
only calls manager_abandon_scope. There seems to be a conflation between
explicit termination of users and sessions, and automatic termination when
they log out. According to the man page:

KillUserProcesses=
Takes a boolean argument. Configures whether the processes
of a user should be killed when she or he completely logs
out (i.e. after her/his last session ended). Defaults to no.

KillUserProcesses should not apply to explicit termination. I think
session_stop_scope should be changed to unconditionally kill the
session when called from method_terminate_user.

Zbyszek
Zbigniew Jędrzejewski-Szmek
2014-02-09 01:48:13 UTC
Permalink
KillUserProcesses=yes/no should be ignored when termination is
explicitly requested.
---
This goes on top of
* logind: use session_get_state() to get sessions state of the user
* logind: just call user_stop() if user_check_gc() returns false

With this patch loginctl terminate-user/session work for me.

Zbyszek

src/login/logind-dbus.c | 6 +++---
src/login/logind-seat-dbus.c | 2 +-
src/login/logind-seat.c | 8 ++++----
src/login/logind-seat.h | 4 ++--
src/login/logind-session-dbus.c | 2 +-
src/login/logind-session.c | 12 ++++++------
src/login/logind-session.h | 2 +-
src/login/logind-user-dbus.c | 2 +-
src/login/logind-user.c | 4 ++--
src/login/logind-user.h | 2 +-
src/login/logind.c | 6 +++---
11 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
index d671346..bd0de33 100644
--- a/src/login/logind-dbus.c
+++ b/src/login/logind-dbus.c
@@ -939,7 +939,7 @@ static int method_terminate_session(sd_bus *bus, sd_bus_message *message, void *
if (!session)
return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_SESSION, "No session '%s' known", name);

- r = session_stop(session);
+ r = session_stop(session, true);
if (r < 0)
return r;

@@ -964,7 +964,7 @@ static int method_terminate_user(sd_bus *bus, sd_bus_message *message, void *use
if (!user)
return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_USER, "No user '%lu' known or logged in", (unsigned long) uid);

- r = user_stop(user);
+ r = user_stop(user, true);
if (r < 0)
return r;

@@ -989,7 +989,7 @@ static int method_terminate_seat(sd_bus *bus, sd_bus_message *message, void *use
if (!seat)
return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_SEAT, "No seat '%s' known", name);

- r = seat_stop_sessions(seat);
+ r = seat_stop_sessions(seat, true);
if (r < 0)
return r;

diff --git a/src/login/logind-seat-dbus.c b/src/login/logind-seat-dbus.c
index 909007c..26cddfe 100644
--- a/src/login/logind-seat-dbus.c
+++ b/src/login/logind-seat-dbus.c
@@ -201,7 +201,7 @@ static int method_terminate(sd_bus *bus, sd_bus_message *message, void *userdata
assert(message);
assert(s);

- r = seat_stop_sessions(s);
+ r = seat_stop_sessions(s, true);
if (r < 0)
return r;

diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c
index c7f112a..631be5f 100644
--- a/src/login/logind-seat.c
+++ b/src/login/logind-seat.c
@@ -418,7 +418,7 @@ int seat_start(Seat *s) {
return 0;
}

-int seat_stop(Seat *s) {
+int seat_stop(Seat *s, bool force) {
int r = 0;

assert(s);
@@ -430,7 +430,7 @@ int seat_stop(Seat *s) {
"MESSAGE=Removed seat %s.", s->id,
NULL);

- seat_stop_sessions(s);
+ seat_stop_sessions(s, force);

unlink(s->state_file);
seat_add_to_gc_queue(s);
@@ -443,14 +443,14 @@ int seat_stop(Seat *s) {
return r;
}

-int seat_stop_sessions(Seat *s) {
+int seat_stop_sessions(Seat *s, bool force) {
Session *session;
int r = 0, k;

assert(s);

LIST_FOREACH(sessions_by_seat, session, s->sessions) {
- k = session_stop(session);
+ k = session_stop(session, force);
if (k < 0)
r = k;
}
diff --git a/src/login/logind-seat.h b/src/login/logind-seat.h
index 9e21e3a..9e469d4 100644
--- a/src/login/logind-seat.h
+++ b/src/login/logind-seat.h
@@ -80,8 +80,8 @@ bool seat_can_graphical(Seat *s);
int seat_get_idle_hint(Seat *s, dual_timestamp *t);

int seat_start(Seat *s);
-int seat_stop(Seat *s);
-int seat_stop_sessions(Seat *s);
+int seat_stop(Seat *s, bool force);
+int seat_stop_sessions(Seat *s, bool force);

bool seat_check_gc(Seat *s, bool drop_not_started);
void seat_add_to_gc_queue(Seat *s);
diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c
index 7ee4956..f9305dd 100644
--- a/src/login/logind-session-dbus.c
+++ b/src/login/logind-session-dbus.c
@@ -188,7 +188,7 @@ static int method_terminate(sd_bus *bus, sd_bus_message *message, void *userdata
assert(message);
assert(s);

- r = session_stop(s);
+ r = session_stop(s, true);
if (r < 0)
return r;

diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index f661cc8..d4742e1 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -565,7 +565,7 @@ int session_start(Session *s) {
return 0;
}

-static int session_stop_scope(Session *s) {
+static int session_stop_scope(Session *s, bool force) {
_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
char *job;
int r;
@@ -575,7 +575,7 @@ static int session_stop_scope(Session *s) {
if (!s->scope)
return 0;

- if (manager_shall_kill(s->manager, s->user->name)) {
+ if (force || 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));
@@ -595,7 +595,7 @@ static int session_stop_scope(Session *s) {
return 0;
}

-int session_stop(Session *s) {
+int session_stop(Session *s, bool force) {
int r;

assert(s);
@@ -609,7 +609,7 @@ int session_stop(Session *s) {
session_remove_fifo(s);

/* Kill cgroup */
- r = session_stop_scope(s);
+ r = session_stop_scope(s, force);

s->stopping = true;

@@ -672,7 +672,7 @@ static int release_timeout_callback(sd_event_source *es, uint64_t usec, void *us
assert(es);
assert(s);

- session_stop(s);
+ session_stop(s, false);
return 0;
}

@@ -812,7 +812,7 @@ static int session_dispatch_fifo(sd_event_source *es, int fd, uint32_t revents,
/* EOF on the FIFO means the session died abnormally. */

session_remove_fifo(s);
- session_stop(s);
+ session_stop(s, false);

return 1;
}
diff --git a/src/login/logind-session.h b/src/login/logind-session.h
index 42552bc..c9af5eb 100644
--- a/src/login/logind-session.h
+++ b/src/login/logind-session.h
@@ -136,7 +136,7 @@ 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);
int session_start(Session *s);
-int session_stop(Session *s);
+int session_stop(Session *s, bool force);
int session_finalize(Session *s);
void session_release(Session *s);
int session_save(Session *s);
diff --git a/src/login/logind-user-dbus.c b/src/login/logind-user-dbus.c
index 2d49b8b..18eea89 100644
--- a/src/login/logind-user-dbus.c
+++ b/src/login/logind-user-dbus.c
@@ -180,7 +180,7 @@ static int method_terminate(sd_bus *bus, sd_bus_message *message, void *userdata
assert(message);
assert(u);

- r = user_stop(u);
+ r = user_stop(u, true);
if (r < 0)
return r;

diff --git a/src/login/logind-user.c b/src/login/logind-user.c
index 2a11bab..a1e35b2 100644
--- a/src/login/logind-user.c
+++ b/src/login/logind-user.c
@@ -494,13 +494,13 @@ static int user_remove_runtime_path(User *u) {
return r;
}

-int user_stop(User *u) {
+int user_stop(User *u, bool force) {
Session *s;
int r = 0, k;
assert(u);

LIST_FOREACH(sessions_by_user, s, u->sessions) {
- k = session_stop(s);
+ k = session_stop(s, force);
if (k < 0)
r = k;
}
diff --git a/src/login/logind-user.h b/src/login/logind-user.h
index b0fefe9..f237d2a 100644
--- a/src/login/logind-user.h
+++ b/src/login/logind-user.h
@@ -72,7 +72,7 @@ void user_free(User *u);
bool user_check_gc(User *u, bool drop_not_started);
void user_add_to_gc_queue(User *u);
int user_start(User *u);
-int user_stop(User *u);
+int user_stop(User *u, bool force);
int user_finalize(User *u);
UserState user_get_state(User *u);
int user_get_idle_hint(User *u, dual_timestamp *t);
diff --git a/src/login/logind.c b/src/login/logind.c
index 84c5b7d..d68eda6 100644
--- a/src/login/logind.c
+++ b/src/login/logind.c
@@ -862,7 +862,7 @@ void manager_gc(Manager *m, bool drop_not_started) {
seat->in_gc_queue = false;

if (!seat_check_gc(seat, drop_not_started)) {
- seat_stop(seat);
+ seat_stop(seat, false);
seat_free(seat);
}
}
@@ -874,7 +874,7 @@ void manager_gc(Manager *m, bool 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);
+ session_stop(session, false);

/* Normally, this should make the session busy again,
* if it doesn't then let's get rid of it
@@ -890,7 +890,7 @@ void manager_gc(Manager *m, bool drop_not_started) {
user->in_gc_queue = false;

if (!user_check_gc(user, drop_not_started)) {
- user_stop(user);
+ user_stop(user, false);
user_finalize(user);
user_free(user);
}
--
1.8.5.3
Lennart Poettering
2014-02-11 19:39:40 UTC
Permalink
On Sat, 08.02.14 20:48, Zbigniew Jędrzejewski-Szmek (***@in.waw.pl) wrote:

This patch looks good! Please push!
Post by Zbigniew Jędrzejewski-Szmek
KillUserProcesses=yes/no should be ignored when termination is
explicitly requested.
---
This goes on top of
* logind: use session_get_state() to get sessions state of the user
* logind: just call user_stop() if user_check_gc() returns false
With this patch loginctl terminate-user/session work for me.
Zbyszek
src/login/logind-dbus.c | 6 +++---
src/login/logind-seat-dbus.c | 2 +-
src/login/logind-seat.c | 8 ++++----
src/login/logind-seat.h | 4 ++--
src/login/logind-session-dbus.c | 2 +-
src/login/logind-session.c | 12 ++++++------
src/login/logind-session.h | 2 +-
src/login/logind-user-dbus.c | 2 +-
src/login/logind-user.c | 4 ++--
src/login/logind-user.h | 2 +-
src/login/logind.c | 6 +++---
11 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
index d671346..bd0de33 100644
--- a/src/login/logind-dbus.c
+++ b/src/login/logind-dbus.c
@@ -939,7 +939,7 @@ static int method_terminate_session(sd_bus *bus, sd_bus_message *message, void *
if (!session)
return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_SESSION, "No session '%s' known", name);
- r = session_stop(session);
+ r = session_stop(session, true);
if (r < 0)
return r;
@@ -964,7 +964,7 @@ static int method_terminate_user(sd_bus *bus, sd_bus_message *message, void *use
if (!user)
return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_USER, "No user '%lu' known or logged in", (unsigned long) uid);
- r = user_stop(user);
+ r = user_stop(user, true);
if (r < 0)
return r;
@@ -989,7 +989,7 @@ static int method_terminate_seat(sd_bus *bus, sd_bus_message *message, void *use
if (!seat)
return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_SEAT, "No seat '%s' known", name);
- r = seat_stop_sessions(seat);
+ r = seat_stop_sessions(seat, true);
if (r < 0)
return r;
diff --git a/src/login/logind-seat-dbus.c b/src/login/logind-seat-dbus.c
index 909007c..26cddfe 100644
--- a/src/login/logind-seat-dbus.c
+++ b/src/login/logind-seat-dbus.c
@@ -201,7 +201,7 @@ static int method_terminate(sd_bus *bus, sd_bus_message *message, void *userdata
assert(message);
assert(s);
- r = seat_stop_sessions(s);
+ r = seat_stop_sessions(s, true);
if (r < 0)
return r;
diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c
index c7f112a..631be5f 100644
--- a/src/login/logind-seat.c
+++ b/src/login/logind-seat.c
@@ -418,7 +418,7 @@ int seat_start(Seat *s) {
return 0;
}
-int seat_stop(Seat *s) {
+int seat_stop(Seat *s, bool force) {
int r = 0;
assert(s);
@@ -430,7 +430,7 @@ int seat_stop(Seat *s) {
"MESSAGE=Removed seat %s.", s->id,
NULL);
- seat_stop_sessions(s);
+ seat_stop_sessions(s, force);
unlink(s->state_file);
seat_add_to_gc_queue(s);
@@ -443,14 +443,14 @@ int seat_stop(Seat *s) {
return r;
}
-int seat_stop_sessions(Seat *s) {
+int seat_stop_sessions(Seat *s, bool force) {
Session *session;
int r = 0, k;
assert(s);
LIST_FOREACH(sessions_by_seat, session, s->sessions) {
- k = session_stop(session);
+ k = session_stop(session, force);
if (k < 0)
r = k;
}
diff --git a/src/login/logind-seat.h b/src/login/logind-seat.h
index 9e21e3a..9e469d4 100644
--- a/src/login/logind-seat.h
+++ b/src/login/logind-seat.h
@@ -80,8 +80,8 @@ bool seat_can_graphical(Seat *s);
int seat_get_idle_hint(Seat *s, dual_timestamp *t);
int seat_start(Seat *s);
-int seat_stop(Seat *s);
-int seat_stop_sessions(Seat *s);
+int seat_stop(Seat *s, bool force);
+int seat_stop_sessions(Seat *s, bool force);
bool seat_check_gc(Seat *s, bool drop_not_started);
void seat_add_to_gc_queue(Seat *s);
diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c
index 7ee4956..f9305dd 100644
--- a/src/login/logind-session-dbus.c
+++ b/src/login/logind-session-dbus.c
@@ -188,7 +188,7 @@ static int method_terminate(sd_bus *bus, sd_bus_message *message, void *userdata
assert(message);
assert(s);
- r = session_stop(s);
+ r = session_stop(s, true);
if (r < 0)
return r;
diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index f661cc8..d4742e1 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -565,7 +565,7 @@ int session_start(Session *s) {
return 0;
}
-static int session_stop_scope(Session *s) {
+static int session_stop_scope(Session *s, bool force) {
_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
char *job;
int r;
@@ -575,7 +575,7 @@ static int session_stop_scope(Session *s) {
if (!s->scope)
return 0;
- if (manager_shall_kill(s->manager, s->user->name)) {
+ if (force || 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));
@@ -595,7 +595,7 @@ static int session_stop_scope(Session *s) {
return 0;
}
-int session_stop(Session *s) {
+int session_stop(Session *s, bool force) {
int r;
assert(s);
@@ -609,7 +609,7 @@ int session_stop(Session *s) {
session_remove_fifo(s);
/* Kill cgroup */
- r = session_stop_scope(s);
+ r = session_stop_scope(s, force);
s->stopping = true;
@@ -672,7 +672,7 @@ static int release_timeout_callback(sd_event_source *es, uint64_t usec, void *us
assert(es);
assert(s);
- session_stop(s);
+ session_stop(s, false);
return 0;
}
@@ -812,7 +812,7 @@ static int session_dispatch_fifo(sd_event_source *es, int fd, uint32_t revents,
/* EOF on the FIFO means the session died abnormally. */
session_remove_fifo(s);
- session_stop(s);
+ session_stop(s, false);
return 1;
}
diff --git a/src/login/logind-session.h b/src/login/logind-session.h
index 42552bc..c9af5eb 100644
--- a/src/login/logind-session.h
+++ b/src/login/logind-session.h
@@ -136,7 +136,7 @@ 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);
int session_start(Session *s);
-int session_stop(Session *s);
+int session_stop(Session *s, bool force);
int session_finalize(Session *s);
void session_release(Session *s);
int session_save(Session *s);
diff --git a/src/login/logind-user-dbus.c b/src/login/logind-user-dbus.c
index 2d49b8b..18eea89 100644
--- a/src/login/logind-user-dbus.c
+++ b/src/login/logind-user-dbus.c
@@ -180,7 +180,7 @@ static int method_terminate(sd_bus *bus, sd_bus_message *message, void *userdata
assert(message);
assert(u);
- r = user_stop(u);
+ r = user_stop(u, true);
if (r < 0)
return r;
diff --git a/src/login/logind-user.c b/src/login/logind-user.c
index 2a11bab..a1e35b2 100644
--- a/src/login/logind-user.c
+++ b/src/login/logind-user.c
@@ -494,13 +494,13 @@ static int user_remove_runtime_path(User *u) {
return r;
}
-int user_stop(User *u) {
+int user_stop(User *u, bool force) {
Session *s;
int r = 0, k;
assert(u);
LIST_FOREACH(sessions_by_user, s, u->sessions) {
- k = session_stop(s);
+ k = session_stop(s, force);
if (k < 0)
r = k;
}
diff --git a/src/login/logind-user.h b/src/login/logind-user.h
index b0fefe9..f237d2a 100644
--- a/src/login/logind-user.h
+++ b/src/login/logind-user.h
@@ -72,7 +72,7 @@ void user_free(User *u);
bool user_check_gc(User *u, bool drop_not_started);
void user_add_to_gc_queue(User *u);
int user_start(User *u);
-int user_stop(User *u);
+int user_stop(User *u, bool force);
int user_finalize(User *u);
UserState user_get_state(User *u);
int user_get_idle_hint(User *u, dual_timestamp *t);
diff --git a/src/login/logind.c b/src/login/logind.c
index 84c5b7d..d68eda6 100644
--- a/src/login/logind.c
+++ b/src/login/logind.c
@@ -862,7 +862,7 @@ void manager_gc(Manager *m, bool drop_not_started) {
seat->in_gc_queue = false;
if (!seat_check_gc(seat, drop_not_started)) {
- seat_stop(seat);
+ seat_stop(seat, false);
seat_free(seat);
}
}
@@ -874,7 +874,7 @@ void manager_gc(Manager *m, bool 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);
+ session_stop(session, false);
/* Normally, this should make the session busy again,
* if it doesn't then let's get rid of it
@@ -890,7 +890,7 @@ void manager_gc(Manager *m, bool drop_not_started) {
user->in_gc_queue = false;
if (!user_check_gc(user, drop_not_started)) {
- user_stop(user);
+ user_stop(user, false);
user_finalize(user);
user_free(user);
}
Lennart
--
Lennart Poettering, Red Hat
Djalal Harouni
2014-02-09 15:19:34 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Djalal Harouni
Post by Zbigniew Jędrzejewski-Szmek
Post by Djalal Harouni
In function user_get_state() remove the session_is_active() check, just
count on the session_get_state() function to get the correct session
state.
session_is_active() may return true before starting the session scope
and user service, this means it will return true even before the creation
of the session fifo_fd which will produce incorrect states.
So be consistent and just use session_get_state().
sshd[18756]: pam_unix(sshd:session): session closed for user user2
systemd-logind[18687]: Sent message type=method_call sender=n/a destination=org.freedesktop.systemd1 object=/org/freedesktop/systemd1/unit/session_2d10_2escope interface=org.freedesktop.systemd1.Scope member=Abandon cookie=27 reply_cookie=0 error=n/a
And nothing afterwards. User manager for user2 is undisturbed.
Ah this patch fixes the user state.
http://lists.freedesktop.org/archives/systemd-devel/2014-February/016754.html
It should work, please give it a try!
You're right, I applied the wrong patch. It really seems that with your
* logind: use session_get_state() to get sessions state of the user
* logind: just call user_stop() if user_check_gc() returns false
things actually work.
Ok nice, thanks :-)
Post by Zbigniew Jędrzejewski-Szmek
One thing that still does not work is terminate-user, even though
kill-session works.
method_terminate_user -> user_stop -*> session_stop -> session_stop_scope -> manager_abandom_scope
-> user_stop_service
-> user_stop_slice
manager_stop_scope calls manager_shall_kill which returns false, so it
only calls manager_abandon_scope. There seems to be a conflation between
explicit termination of users and sessions, and automatic termination when
KillUserProcesses=
Takes a boolean argument. Configures whether the processes
of a user should be killed when she or he completely logs
out (i.e. after her/his last session ended). Defaults to no.
KillUserProcesses should not apply to explicit termination. I think
session_stop_scope should be changed to unconditionally kill the
session when called from method_terminate_user.
Yes, I do agree. I did also have this one on my todo list, I saw you
posted the patch on the other thread, I'll test it this night or
probably tomorrow.

I still have 2 untested patches, that apply on top of Lennart changes
to close two small race windows...

Thanks!
Post by Zbigniew Jędrzejewski-Szmek
Zbyszek
--
Djalal Harouni
http://opendz.org
Lennart Poettering
2014-02-11 19:33:13 UTC
Permalink
Post by Djalal Harouni
In function user_get_state() remove the session_is_active() check, just
count on the session_get_state() function to get the correct session
state.
session_is_active() may return true before starting the session scope
and user service, this means it will return true even before the creation
of the session fifo_fd which will produce incorrect states.
So be consistent and just use session_get_state().
Looks good!

Applied!

Thanks!
Post by Djalal Harouni
---
src/login/logind-user.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/login/logind-user.c b/src/login/logind-user.c
index fdbf6e3..2a11bab 100644
--- a/src/login/logind-user.c
+++ b/src/login/logind-user.c
@@ -648,9 +648,11 @@ UserState user_get_state(User *u) {
bool all_closing = true;
LIST_FOREACH(sessions_by_user, i, u->sessions) {
- if (session_is_active(i))
+ SessionState state = session_get_state(i);
+
+ if (state == SESSION_ACTIVE)
return USER_ACTIVE;
- if (session_get_state(i) != SESSION_CLOSING)
+ else if (state != SESSION_CLOSING)
all_closing = false;
}
Lennart
--
Lennart Poettering, Red Hat
Djalal Harouni
2014-02-06 20:37:19 UTC
Permalink
Add the user_is_opening() and session_is_opening() functions. These
functions will check their appropriate 'opening' flag to see if we are
in the middel of the opening state.

This patch is preparation for the next patch which will use it to guard
match_job_remove() from calling session_jobs_reply() and
session_send_create_reply() during SESSION_CLOSING.
---
src/login/logind-session.c | 8 +++++++-
src/login/logind-session.h | 1 +
src/login/logind-user.c | 8 +++++++-
src/login/logind-user.h | 1 +
4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index 848e8a1..215f2b8 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -647,6 +647,12 @@ int session_finalize(Session *s) {
return r;
}

+bool session_is_opening(Session *s) {
+ assert(s);
+
+ return s->scope_opening;
+}
+
bool session_is_active(Session *s) {
assert(s);

@@ -882,7 +888,7 @@ void session_add_to_gc_queue(Session *s) {
SessionState session_get_state(Session *s) {
assert(s);

- if (s->scope_opening)
+ if (session_is_opening(s))
return SESSION_OPENING;

if (s->fifo_fd < 0)
diff --git a/src/login/logind-session.h b/src/login/logind-session.h
index 205491a..964306d 100644
--- a/src/login/logind-session.h
+++ b/src/login/logind-session.h
@@ -129,6 +129,7 @@ void session_set_user(Session *s, User *u);
bool session_check_gc(Session *s, bool drop_not_started);
void session_add_to_gc_queue(Session *s);
int session_activate(Session *s);
+bool session_is_opening(Session *s);
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);
diff --git a/src/login/logind-user.c b/src/login/logind-user.c
index 9ed216d..cf9c948 100644
--- a/src/login/logind-user.c
+++ b/src/login/logind-user.c
@@ -635,6 +635,12 @@ void user_add_to_gc_queue(User *u) {
u->in_gc_queue = true;
}

+bool user_is_opening(User *u) {
+ assert(u);
+
+ return u->slice_opening || u->service_opening;
+}
+
UserState user_get_state(User *u) {
Session *i;
SessionState session_state;
@@ -642,7 +648,7 @@ UserState user_get_state(User *u) {

assert(u);

- if (u->slice_opening || u->service_opening)
+ if (user_is_opening(u))
return USER_OPENING;

LIST_FOREACH(sessions_by_user, i, u->sessions) {
diff --git a/src/login/logind-user.h b/src/login/logind-user.h
index ac43361..2728168 100644
--- a/src/login/logind-user.h
+++ b/src/login/logind-user.h
@@ -75,6 +75,7 @@ void user_add_to_gc_queue(User *u);
int user_start(User *u);
int user_stop(User *u);
int user_finalize(User *u);
+bool user_is_opening(User *u);
UserState user_get_state(User *u);
int user_get_idle_hint(User *u, dual_timestamp *t);
int user_save(User *u);
--
1.8.3.1
Djalal Harouni
2014-02-06 20:37:20 UTC
Permalink
match_job_removed() signal is triggered when queued jobs finish during
session opening or closing.

Calling session_jobs_reply() during opening is valid, but during
session closing does not make sense.

The session_send_create_reply() function which is called by
session_jobs_reply() is able to detect if it was not called during
open time by checking the 'session->create_message'. However, making
session_jobs_reply() check session_is_opening() and user_is_opening()
is more comprehensive.
---
src/login/logind-dbus.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
index 24482fd..4b71d9e 100644
--- a/src/login/logind-dbus.c
+++ b/src/login/logind-dbus.c
@@ -1930,6 +1930,10 @@ static int session_jobs_reply(Session *s, const char *unit, const char *result,
if (!s->started)
return r;

+ /* Don't call me if this is not opening state */
+ if (!session_is_opening(s) && !user_is_opening(s->user))
+ return r;
+
if (streq(result, "done"))
r = session_send_create_reply(s, NULL, opening);
else {
@@ -2010,6 +2014,7 @@ int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_b
* 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);
}
--
1.8.3.1
Lennart Poettering
2014-02-07 15:52:34 UTC
Permalink
On Thu, 06.02.14 21:37, Djalal Harouni (***@opendz.org) wrote:

Hmm, isn't this issue already caught by the "if (!s->create_message)"
check in session_send_reply()?

Thanks!
Post by Djalal Harouni
match_job_removed() signal is triggered when queued jobs finish during
session opening or closing.
Calling session_jobs_reply() during opening is valid, but during
session closing does not make sense.
The session_send_create_reply() function which is called by
session_jobs_reply() is able to detect if it was not called during
open time by checking the 'session->create_message'. However, making
session_jobs_reply() check session_is_opening() and user_is_opening()
is more comprehensive.
---
src/login/logind-dbus.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
index 24482fd..4b71d9e 100644
--- a/src/login/logind-dbus.c
+++ b/src/login/logind-dbus.c
@@ -1930,6 +1930,10 @@ static int session_jobs_reply(Session *s, const char *unit, const char *result,
if (!s->started)
return r;
+ /* Don't call me if this is not opening state */
+ if (!session_is_opening(s) && !user_is_opening(s->user))
+ return r;
+
if (streq(result, "done"))
r = session_send_create_reply(s, NULL, opening);
else {
@@ -2010,6 +2014,7 @@ int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_b
* 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);
}
Lennart
--
Lennart Poettering, Red Hat
Lennart Poettering
2014-02-07 15:29:48 UTC
Permalink
On Thu, 06.02.14 21:37, Djalal Harouni (***@opendz.org) wrote:

Heya!

So, I was working on fixes for this in parallel which I have pushed
earlier today, which change a couple of things around in more complex
ways. Some of them conflict with yours changes. I'll commit the
ones of yours that still appear necessary. If you think I am missing
some, please let me know.

The new logic I commited changes around how scopes are used by
logind. Previously the KillUserProcesses= option in logind.conf would
influence KillMode= of the scope. However that was a bad idea since we
actually want to provide people a way to terminate sessions and we
actually need them to order things properly on shutdown so that scopes
go away before the network is removed, and suchlike. Hence I changed the
logic so we are OK with leavig scopes around from the logind side, we
just mark the session entries as "closing". KillUserProcesses= hence
simply controls whether we leave them around and hence the session in
"closing" or whether we actively shut them down.

Sorry for commiting into the middle of your work!

Lennart
--
Lennart Poettering, Red Hat
Djalal Harouni
2014-02-07 19:24:31 UTC
Permalink
Post by Lennart Poettering
Heya!
So, I was working on fixes for this in parallel which I have pushed
earlier today, which change a couple of things around in more complex
ways. Some of them conflict with yours changes. I'll commit the
ones of yours that still appear necessary. If you think I am missing
some, please let me know.
Ok, I'll do.
Post by Lennart Poettering
The new logic I commited changes around how scopes are used by
logind. Previously the KillUserProcesses= option in logind.conf would
influence KillMode= of the scope. However that was a bad idea since we
actually want to provide people a way to terminate sessions and we
actually need them to order things properly on shutdown so that scopes
go away before the network is removed, and suchlike. Hence I changed the
Yes, I was going to ask about why scopes are influenced by
KillUserProcesses? anyway I'll read your changes.
Post by Lennart Poettering
logic so we are OK with leavig scopes around from the logind side, we
just mark the session entries as "closing". KillUserProcesses= hence
simply controls whether we leave them around and hence the session in
"closing" or whether we actively shut them down.
Ok.
Post by Lennart Poettering
Sorry for commiting into the middle of your work!
No worries, I did a dive into the internals and the picture is still not
clear :-)
Post by Lennart Poettering
Lennart
Thanks
Post by Lennart Poettering
--
Lennart Poettering, Red Hat
--
Djalal Harouni
http://opendz.org
Zbigniew Jędrzejewski-Szmek
2014-02-07 23:39:25 UTC
Permalink
Post by Djalal Harouni
Currently logind will not clear sessions on logout. The bug is confirmed
for getty and ssh logins. This series is preparation for next patches to
address that bug, it does not fix it.
However, this series also fixe real races on user and session states.
This ensures that user_save() and session_save() functions will write the
correct user and session state to the state files.
http://lists.freedesktop.org/archives/systemd-devel/2014-January/015968.html
So, I did some testing with current git (ba978d7). And this bug
appears to be only partially gone. After my second user logs out, loginctl
stops showing the session. But the manager *remains*.

Hm, the manager remains, but /run/user/<uid>/systemd/ gets nuked.
So when the user logins in a second time, communication with user manager
is broken.

Attaching strace to systemd --user during the orignial session close yields...
nothing. Both systemd and (sd-pam) remain undisturbed...

In the logs I see (during user login and manager startup):

systemd-logind[2998]: Failed to process message [type=signal sender=:1.1 path=/org/freedesktop/systemd1/job/3284 interface=org.freedesktop.DBus.Properties member=PropertiesChanged signature=sa{sv}as]: Invalid argument

but nothing interesting during logout:

sshd[3075]: Received disconnect...
sshd[3043]: pam_unix(sshd:session): session closed for user user2
...
systemd-logind[2998]: Got message type=method_call sender=:1.70 destination=org.freedesktop.login1 object=/org/freedesktop/login1 interface=org.freedesktop.login1.Manager member=ReleaseSession cookie=2 reply_cookie=0 error=n/a
systemd-logind[2998]: Sent message type=method_call sender=n/a destination=org.freedesktop.systemd1 object=/org/freedesktop/systemd1/unit/session_2d10_2escope interface=org.freedesktop.systemd1.Scope member=Abandon cookie=31 reply_cookie=0 error=n/a
systemd-logind[2998]: Got message type=signal sender=:1.1 destination=:1.68 object=/org/freedesktop/systemd1 interface=org.freedesktop.systemd1.Manager member=UnitRemoved cookie=1585 reply_cookie=0 error=n/a
...
systemd-logind[2998]: Removed session 10.
systemd-logind[2998]: Sent message type=signal sender=n/a destination=n/a object=/org/freedesktop/login1 interface=org.freedesktop.login1.Manager member=SessionRemoved cookie=34 reply_cookie=0 error=n/a
...
systemd-logind[2998]: User user2 logged out.
systemd-logind[2998]: Sent message type=signal sender=n/a destination=n/a object=/org/freedesktop/login1 interface=org.freedesktop.login1.Manager member=UserRemoved cookie=36 reply_cookie=0 error=n/a

Zbyszek
Djalal Harouni
2014-02-08 16:00:57 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Djalal Harouni
Currently logind will not clear sessions on logout. The bug is confirmed
for getty and ssh logins. This series is preparation for next patches to
address that bug, it does not fix it.
However, this series also fixe real races on user and session states.
This ensures that user_save() and session_save() functions will write the
correct user and session state to the state files.
http://lists.freedesktop.org/archives/systemd-devel/2014-January/015968.html
So, I did some testing with current git (ba978d7). And this bug
appears to be only partially gone. After my second user logs out, loginctl
stops showing the session. But the manager *remains*.
I confirm this, I've identified the problem, please see below.
Post by Zbigniew Jędrzejewski-Szmek
Hm, the manager remains, but /run/user/<uid>/systemd/ gets nuked.
So when the user logins in a second time, communication with user manager
is broken.
I also confirm, I didn't have time to debug this one.
Post by Zbigniew Jędrzejewski-Szmek
Attaching strace to systemd --user during the orignial session close yields...
nothing. Both systemd and (sd-pam) remain undisturbed...
Yes sd-pam and the user instance will stay alive for *another* user
after logout.

In manager_gc() the user_stop() is never called.

if (!user_check_gc(user, drop_not_started) &&
user_get_state(user) != USER_CLOSING)
user_stop(user);


Note: with the recent commits, user_stop() will set user->stopping to
say that user_stop() was called.

But currently with this logic if the user_check_gc(...) returns false,
this means that 'u->sessions' is NULL, so user_get_state(user) will for
sure return USER_CLOSING if linger is not set and if it is not open
state.

I think that we should remove that USER_CLOSING check, will just test
it.
Djalal Harouni
2014-02-08 18:20:42 UTC
Permalink
Currently if the user logs out, the GC may never call user_stop(),
this will not terminate the systemd user and (sd-pam) of that user.

To fix this, remove the USER_CLOSING state check that is blocking the
GC from calling user_stop(). We do not need it since with the current
logic we have:

1) if user_check_gc() returns false this means that all the sessions of
the user were removed.

2) if all the sessions were removed and if linger is not set and if it
is not open state then user_get_state() will return always USER_CLOSING.

So that check will never be satisfied for normal cases, and for linger
user_check_gc() will return true which prevents the GC from collecting
the user.
---
src/login/logind.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/login/logind.c b/src/login/logind.c
index a6f84e8..84c5b7d 100644
--- a/src/login/logind.c
+++ b/src/login/logind.c
@@ -889,11 +889,8 @@ 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) &&
- user_get_state(user) != USER_CLOSING)
- user_stop(user);
-
if (!user_check_gc(user, drop_not_started)) {
+ user_stop(user);
user_finalize(user);
user_free(user);
}
--
1.8.3.1
Lennart Poettering
2014-02-11 19:38:48 UTC
Permalink
Post by Djalal Harouni
Currently if the user logs out, the GC may never call user_stop(),
this will not terminate the systemd user and (sd-pam) of that user.
To fix this, remove the USER_CLOSING state check that is blocking the
GC from calling user_stop(). We do not need it since with the current
1) if user_check_gc() returns false this means that all the sessions of
the user were removed.
2) if all the sessions were removed and if linger is not set and if it
is not open state then user_get_state() will return always USER_CLOSING.
So that check will never be satisfied for normal cases, and for linger
user_check_gc() will return true which prevents the GC from collecting
the user.
Hmm, this doesn't look right... We really shouldn't finalize the user
object as long as the stop job for the ***@.service unit is still
running. This is why we have this weird two-step GC logic here:

First we check whether the user is still needed. If it is, then we'll
issue the stop job. However, this action means that the user is needed
again, as long as the stop job is not finished. Hence we then wait for
that, and then recheck the thing in the GC. This time we are completely
dead, and can remove the thing entirely. Your patch would break that,
no?
Post by Djalal Harouni
---
src/login/logind.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/login/logind.c b/src/login/logind.c
index a6f84e8..84c5b7d 100644
--- a/src/login/logind.c
+++ b/src/login/logind.c
@@ -889,11 +889,8 @@ 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) &&
- user_get_state(user) != USER_CLOSING)
- user_stop(user);
-
if (!user_check_gc(user, drop_not_started)) {
+ user_stop(user);
user_finalize(user);
user_free(user);
}
Lennart
--
Lennart Poettering, Red Hat
Djalal Harouni
2014-02-11 23:38:11 UTC
Permalink
Post by Lennart Poettering
Post by Djalal Harouni
Currently if the user logs out, the GC may never call user_stop(),
this will not terminate the systemd user and (sd-pam) of that user.
To fix this, remove the USER_CLOSING state check that is blocking the
GC from calling user_stop(). We do not need it since with the current
1) if user_check_gc() returns false this means that all the sessions of
the user were removed.
2) if all the sessions were removed and if linger is not set and if it
is not open state then user_get_state() will return always USER_CLOSING.
So that check will never be satisfied for normal cases, and for linger
user_check_gc() will return true which prevents the GC from collecting
the user.
Hmm, this doesn't look right... We really shouldn't finalize the user
Ok.
Post by Lennart Poettering
First we check whether the user is still needed. If it is, then we'll
issue the stop job. However, this action means that the user is needed
again, as long as the stop job is not finished. Hence we then wait for
But currently with this logic user_stop() is never called, we never queue
a stop job for the user service.

If you pass the user_check_gc() successfully this means that u->sessions
is for sure NULL, and the "user_get_state() != USER_CLOSING" will not
succeed since "user_get_state() == USER_CLOSING", so user_stop() may
never be called.


When you say the "user is needed again as long as the stop job is not
finished. Hence we then wait for that", ok so why not just drop that
user_get_state() == USER_CLOSING ?

and if you worry that perhaps user_stop() has already been called and we
may endup calling it twice even if the state is USER_CLOSING, we can
just check "user->stopping" or check if units are still active before
queueing a second stop job...
Post by Lennart Poettering
that, and then recheck the thing in the GC. This time we are completely
dead, and can remove the thing entirely. Your patch would break that,
no?
Yes it will break that behaviour and I would say here we are in a race
against jobs. So:

If we only drop the "user_get_state() != USER_CLOSING" this will not have
any effect, since the code has already passed the user_check_gc() and as
I've said above, that check is buggy. So we call user_stop() and queue
the stop jobs, and the second user_check_gc() will guard against
finalizing the user until jobs have finished.

I'm following with a patch.

Thanks
--
Djalal Harouni
http://opendz.org
Djalal Harouni
2014-02-11 23:56:06 UTC
Permalink
Currently if the user logs out, the GC may never call user_stop(),
this will not terminate the systemd user and (sd-pam) of that user.

To fix this, remove the USER_CLOSING state check that is blocking the
GC from calling user_stop(). Since if user_check_gc() returns false
this means that all the sessions of the user were removed which will
make user_get_state() return USER_CLOSING.

Conclusion: that test will never be statisfied.

So we remove the USER_CLOSING check and replace it with a check inside
user_stop() this way we know that user_stop() has already queued stop
jobs, no need to redo.

This ensures that the GC will get its two steps correctly as pointed out
by Lennart:
http://lists.freedesktop.org/archives/systemd-devel/2014-February/016825.html

Note: this also fixes another bug that prevents creating the user
private dbus socket which will break communications with the user
manager.

---
src/login/logind-user.c | 6 ++++++
src/login/logind.c | 5 +++--
2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/login/logind-user.c b/src/login/logind-user.c
index fdbf6e3..79d75ad 100644
--- a/src/login/logind-user.c
+++ b/src/login/logind-user.c
@@ -499,6 +499,12 @@ int user_stop(User *u) {
int r = 0, k;
assert(u);

+ /* Stop jobs have already been queued */
+ if (u->stopping) {
+ user_save(u);
+ return r;
+ }
+
LIST_FOREACH(sessions_by_user, s, u->sessions) {
k = session_stop(s);
if (k < 0)
diff --git a/src/login/logind.c b/src/login/logind.c
index a6f84e8..0511dee 100644
--- a/src/login/logind.c
+++ b/src/login/logind.c
@@ -889,10 +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) &&
- user_get_state(user) != USER_CLOSING)
+ /* First step: queue stop jobs */
+ if (!user_check_gc(user, drop_not_started))
user_stop(user);

+ /* Second step: finalize user */
if (!user_check_gc(user, drop_not_started)) {
user_finalize(user);
user_free(user);
--
1.8.3.1
Djalal Harouni
2014-02-10 14:48:07 UTC
Permalink
Post by Djalal Harouni
Post by Zbigniew Jędrzejewski-Szmek
Post by Djalal Harouni
Currently logind will not clear sessions on logout. The bug is confirmed
for getty and ssh logins. This series is preparation for next patches to
address that bug, it does not fix it.
However, this series also fixe real races on user and session states.
This ensures that user_save() and session_save() functions will write the
correct user and session state to the state files.
http://lists.freedesktop.org/archives/systemd-devel/2014-January/015968.html
So, I did some testing with current git (ba978d7). And this bug
appears to be only partially gone. After my second user logs out, loginctl
stops showing the session. But the manager *remains*.
I confirm this, I've identified the problem, please see below.
Post by Zbigniew Jędrzejewski-Szmek
Hm, the manager remains, but /run/user/<uid>/systemd/ gets nuked.
So when the user logins in a second time, communication with user manager
is broken.
I also confirm, I didn't have time to debug this one.
Ok this one was also fixed by the same patch:
http://lists.freedesktop.org/archives/systemd-devel/2014-February/016754.html

I guess, the runtime dir will be removed on logout in user_finalize(),
but user_stop() was never called so the service is still active. During
next loggin, the service unit which is still active will prevent creating
the private dbus socket... anyway it works now, we'll see what Lennart
thinks.
--
Djalal Harouni
http://opendz.org
Zbigniew Jędrzejewski-Szmek
2014-02-08 22:23:33 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
systemd-logind[2998]: Failed to process message [type=signal sender=:1.1 path=/org/freedesktop/systemd1/job/3284 interface=org.freedesktop.DBus.Properties member=PropertiesChanged signature=sa{sv}as]: Invalid argument
So, this happens because match_properties_changed() expects
path=/org/freedesktop/systemd1/unit/... but gets .../job/...

I'll push a patch to quietly ignore those.

Zbyszek
Loading...