Bug 62866

That sounds as if "slim" is reuse PAM contexts. That's a total no-no, regardless whether pam_systemd is in the mix, or not.

A PAM service needs to invoke the PAM session hooks like this:

pam_open_session(h);
pid = fork();
if (pid == 0) {
        exec();
} 
waitpid(pid);
pam_close_session(h);
exit();

That's the only correct way. i.e. the PAM handle can only be used once, and both pam_open_session() and pam_close_session() need to be called in the parent -- not the child. Also, since the session hooks will set all kinds of stuff like resource limits, security labels, audit info, selinux labels, yadda yadda yadda, the parent must exit() after the close session hook.

Anyway, closing, this is almost certainly a fuckup in slim.

Some things puzzle me about that then. Slim seems to work fine outside of systemd. And it's the pam_sytemd pam module that is asking a server (logind) for information. The log messages show the uid sent in the request is correct (for both users), but the reply has the wrong runtime_path value. I don't see the slim display manger being involved here. 

Also slim itself exits at the end and everything is restarted, even the parent slim process. 


Mar 28 11:45:44 venus slim[2922]: pam_systemd(slim:session): Asking logind to create session: uid=1004 pid=2922 service=slim type=x11 class=user seat=seat0 vtnr=7 tty= display=:0.0 remote=no remote_user=root remote_host=

Mar 28 11:45:44 venus slim[2922]: pam_systemd(slim:session): Reply from logind: id=2 object_path=/org/freedesktop/login1/session/_32 runtime_path=/run/user/1003 session_fd=10 seat=seat0 vtnr=7

Jeez. Not sure why I am even doing this, but here's the code of slim:

http://git.berlios.de/cgi-bin/gitweb.cgi?p=slim;a=blob;f=app.cpp;h=b1b9d98d596505866d8a3bada8c90186c6a226e2;hb=HEAD

If you check that out you will see that it reuses the PAM session process. i.e. After the session is closed it simply restarts the main logic from the same process. And that's what is not OK. It has to terminate it and run the next login dialog from a new process.

Anyway, what slim does here is broken, and it will break with systemd, and it will fuck up resource limits, audit trails, security labels, yadda yadda. So, yeah, if you don't use any of that in your PAM stack, then sure you have a good chance things won't explode into your face right-away, but it still isn't correct.

There's nothing to fix here in systemd, it's slim that is at fault here. Closing.

(In reply to comment #3)
> Jeez. Not sure why I am even doing this, but here's the code of slim:
> 
> http://git.berlios.de/cgi-bin/gitweb.cgi?p=slim;a=blob;f=app.cpp;
> h=b1b9d98d596505866d8a3bada8c90186c6a226e2;hb=HEAD
> 
> If you check that out you will see that it reuses the PAM session process.
> i.e. After the session is closed it simply restarts the main logic from the
> same process. And that's what is not OK. It has to terminate it and run the
> next login dialog from a new process.
> 

Why, exactly?  the PAM handle is closed and a new one initialized, what's the reason for the need of a new process?  I also am unsure as to why resource limits, audit trails, etc. etc. should get messed up by slim not cycling its PID (in principle, that is) -- slim is one process that acts as a launcher only; the Xorg server and the client session are not re-used (and terminate prior to the next iteration)...

I did trace why this (slim) is broken in systemd, and that's because logind is assuming that if the calling PID didn't change then a new session isn't necessary -- src/login/logind-dbus.c around line 487 or so.  Just because gdm, kdm, perhaps xdm, etc., don't do this doesn't to me seem to make it invalid, but rather that the assumption of the same pid initiating two sessions (note, these are not at the same time) must therefore be a case of something like 'su' triggering the session code, is just too broad an assumption.

Along these lines, it looks to me that this could be categorized as a bad cacheing issue -- when a session is closed (ie by the ReleaseSession handling code in src/login/logind-dbus.c around line 1731, as triggered by pam-module.c when pam_close_session is called), shouldn't it somehow be marked as such?  That would allow the code just after the "if (session) {" in src/login/logind-dbus.c:488 to (re-)initialize the session rather than just re-use the old/incorrect session info that's already there.

I realize changing this behaviour just for slim isn't something you're likely to do, but is it possible that this behaviour is still a bug that should be looked into?

The various PAM modules alter a number of process attributes (such as cgroup membership in the case of pam_systemd, or resource limits for pam_limits, and so on). If you recycle the PID you will inherit those. You need to start with a fresh set of attributes by forking off a clean process from some well-defined parent process, before you restart PAM.

pam_systemd for example will move the calling process into a cgroup of its own and that's really a one-way operation.

Really, you cannot have the same process create multiple PAM sessions one after the other. The various PAM modules in use do not allow that, pam_systemd is just one of them.