[systemd-devel] [Patch 0/2] logind: make sure that closed sessions will be cleaned

[Djalal Harouni]

On Thu, Jan 16, 2014 at 11:19:08AM +0100, Djalal Harouni wrote:
> On Thu, Jan 16, 2014 at 06:01:58AM +0100, Zbigniew Jędrzejewski-Szmek wrote:
> > Indeed, in a container (without your patches), sessions remain in
> > "closing" state. But with your patches, systemd --user instance is
> > started and killed immediately during login. Not good either :)
> Ok, ouch!
It seems that the systemd --user instance is killed without my patches!

Systemd git will confirm it.

Ok, after some thoughts and source code analysis, I came to the
conclusion that in order to correctly clean session and user state
files, we need:

a) Improve session_check_gc() and user_check_gc() by using the session
and user states. This implies b)

b) Make sure that the state of the session and the user is always
correctly set.


I'll discuss some notes about a) and follow with patches that will try
to honore b).

I'm sending this to have some feedback before spending more time on it,
ahh yes debugging this beast is hard!

Please note that I did not take a look at seats logic, only sessions and
users using getty and ssh logins.

Please feel free to correct me!


a) Improve session_check_gc() and user_check_gc()

* user_check_gc() has an  "if (u->sessions)" check! this should be
  updated to check if there are sessions that are not in the "closing"
  state.
  
  If *all* the sessions are in the "closing" state then check each
  session to see if there are still running processes in the background:
  screen,...
  If so then abort the user_check_gc() or depend on a per-session
  function that will perhaps do the "kill-session-processes".

* session_check_gc() should check if the session is in the "closing"
  state and if there are still running processes in the background, then
  decide to abort or force the "kill"session-processes"

  But it should not do the following check:
  "if (s->scope && manager_unit_is_active(s->manager, s->scope))"

  Since the scope will always be around, the same was applied in the
  commit 63966da86 for user_check_gc(), do not check the user manager!



b) Make sure that the state of the session and the user is always
correctly set. Not to mention that some logind clients depend on it.
 
So to make a) we must honor that b) is stable, and after an extensive
source code analysis, it seems that the state of users and sessions is
not always correct. I'll list some notes/bugs here:

* It seems that in systemd v204 after a logout from a session where
  processes are still running like "screen" the state files will show:

  systemd 204
  session state file:			user state file:
  Active = 1				State = active
  State = closing			ACTIVE_SESSIONS = X

  Where for systemd git:
  session state file:			user state file:
  Active = 0				State = closing
  State = closing			ACTIVE_SESSIONS =

  Not to mention that for systemd git, the session and user state files
  will show the same even if there are no background processes and they
  will not be cleaned  (using getty or ssh logins) which is is the object
  of this thread!

  This is a bug, not fixed yet.


* To track the state of sessions the fifo_fd should be used since the
  event manager will call session_dispatch_fifo() to remove it on eof.

  Using the session_is_active() *before* the fifo_fd check is not stable
  since it will return true before the fifo_fd was even created! and
  also before the session scope and user serivce are created, the
  session->seat->active will be set earlier before the fifo_fd creation
  and will be the last one to be unset after fifo_fd removal.

  And currently this affects src/login/logind-user.c:user_get_state()
  logic since it checks session_is_active() before calling
  session_get_state() which performs the fifo_fd check before the
  session_is_active()
 
  So user_get_state() and session_get_state() results might differ!

  This is a bug, not fixed yet, user_get_state() should count on
  session_get_state().


* It seems there are other problems with the session and user states,
  different variables are used to check the state and later these
  variables are updated at different places!

  The following patches will describe the problem and try to make the
  state more stable.

  The design was do not add extra flags to 'User' and 'Session' structs
  unless this is the only way. The logic is already there, and it needs
  a littel synchronization.


Thanks!



-- 
Djalal Harouni
http://opendz.org