[systemd-devel] [PATCH] logind: just call user_stop() if user_check_gc() returns false

[Djalal Harouni]

On Tue, Feb 11, 2014 at 08:38:48PM +0100, Lennart Poettering wrote:
> On Sat, 08.02.14 19:20, Djalal Harouni (tixxdz at opendz.org) wrote:
> 
> > 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.
> 
> Hmm, this doesn't look right... We really shouldn't finalize the user
> object as long as the stop job for the user at .service unit is still
> running. This is why we have this weird two-step GC logic here: 
Ok.

> 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...

> 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