Bug 67273

Summary:	logind writes out session state file before creating fifo so users end up in CLOSING state
Product:	systemd	Reporter:	Ray Strode [halfline] <rstrode>
Component:	general	Assignee:	systemd-bugs
Status:	RESOLVED FIXED	QA Contact:	systemd-bugs
Severity:	normal
Priority:	medium	CC:	colin, dwalsh, i.gnatenko.brain, pachoramos1, ph.wolfer, rstrode
Version:	unspecified
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:

Description Ray Strode [halfline] 2013-07-24 20:40:09 UTC

Dan Walsh came to my cube with a user who couldn't get unlocked.  After some debugging we discovered the user's session was in the closing state and so was being ignored by GDM. After some more code digging we found out why.  This commit:

commit fb6becb4436ae4078337011b2017ce294e7361cf
Author: Lennart Poettering <lennart@poettering.net>
Date:   Tue Jul 2 01:46:30 2013 +0200

    logind: port over to use scopes+slices for all cgroup stuff


defers creating a reply for CreateSession until after hearing back from systemd about the user's slice job.  Since the CreateSession reply contains one end of the fifo fd, that commit also defers creating the fifo fd until hearing back.  Previously the fifo fd was created before session_start() was called.  session_start() calls session_save() which depends on the fifo fd being around to give correct state information. Because the fifo fd isn't created with session_save() is called, CLOSING gets written into it instead of ACTIVE.  session_save() isn't gauranteed to get called again for the session and so the session stays in the CLOSING state indefinitely.

Possible fixes I can think of are:

 1) keep create_fifo where it was previously and tuck the resulting fifo fd away like the create_message is tucked away, so it can be used to construct the reply
 2) create the replies up front and tuck them away instead of tucking the create_message away
 3) call session_save again after the slice job finishes (and maybe change the initial session_save call to write OPENING instead of CLOSING into the file)

Comment 1 Lennart Poettering 2013-07-26 15:33:28 UTC

cba38758b4d49c6fe7c2f0eea255e11ee9df23eb should fix the issue.

Comment 2 Ray Strode [halfline] 2013-07-26 16:08:49 UTC

Looks fine, thanks!

Two comments, but these things probably don't matter that much in practice:

1) the state file will briefly begin its life with CLOSING written in it, which isn't really accurate, though maybe that's inconsequential.
2) you send out the reply before saving the state file, so, in theory, there's a small race where a client could get the reply and then proceed to check the state file and still read stale information.  It's a pretty small race though.

Comment 3 Lennart Poettering 2013-07-26 17:02:09 UTC

(In reply to comment #2)
> Looks fine, thanks!
> 
> Two comments, but these things probably don't matter that much in practice:
> 
> 1) the state file will briefly begin its life with CLOSING written in it,
> which isn't really accurate, though maybe that's inconsequential.

No it will briefly have "OPENING" in it. See session_get_state(): it will return OPENING if there's the scope job still pending. The problem was simply that we saved the state at a time where the scope job was finished but the fifo not set up yet...

> 2) you send out the reply before saving the state file, so, in theory,
> there's a small race where a client could get the reply and then proceed to
> check the state file and still read stale information.  It's a pretty small
> race though.

Hmm, so that's indeed something to fix! 

Done so now in 7c6e214aeb141e6d514ddd9237a956278c4afc8c.

Thanks for have a closer look, much appreciated!

Comment 4 Ray Strode [halfline] 2013-07-26 17:16:30 UTC

(In reply to comment #3)
> No it will briefly have "OPENING" in it. See session_get_state(): it will
> return OPENING if there's the scope job still pending. The problem was
> simply that we saved the state at a time where the scope job was finished
> but the fifo not set up yet...
Ah okay. When I first started looking into this issue with dwalsh, I was originally looking at a stale git source checkout, and the behavior didn't match what I was reading, so I updated the tree, and then pretty soon after we saw the fifo_fd problem in the code.  The last time I read the session_get_state() function I was looking at the stale source before updating, and it didn't have an OPENING state.

> > 2) you send out the reply before saving the state file, so, in theory,
> > there's a small race where a client could get the reply and then proceed to
> > check the state file and still read stale information.  It's a pretty small
> > race though.
> 
> Hmm, so that's indeed something to fix! 
> 
> Done so now in 7c6e214aeb141e6d514ddd9237a956278c4afc8c.
cool.

Comment 5 Lennart Poettering 2013-07-29 14:43:42 UTC

*** Bug 67267 has been marked as a duplicate of this bug. ***

Comment 6 Colin Guthrie 2013-09-02 14:50:00 UTC

For the record, I think you meant 76e665855edef5b7103cb09d114377d477bfae02 in comment 3.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.