GNOME Bugzilla – Bug 618047
Greeter session is not reaped when switching user
Last modified: 2018-02-12 14:56:00 UTC
Launch gdmflexiserver to spawn a new greeter. Then log in with an already logged on user. The display switches back to that user, but the greeter session is not reaped. All processes are still here, and it appears in consolekit. What makes this worse is: - that this greeter session is not re-used by further gdmflexiserver invocations, so this is technically a leak; - that when trying to shut down, it is accounted by consolekit, which asks for the root password to shut down. I tried to look at the code that is run after the console switching. if (migrated) { destroy_session (slave); /* We don't stop the slave here because when Xorg exits it switches to the VT it was started from. That interferes with fast user switching. */ queue_greeter_reset (slave); The comment is inconsistent with the action taken. Instead of stopping the slave, the "Reset" signal is sent, which keeps it perfectly awake.
Created attachment 160537 [details] [review] Kill the greeter after switching VTs This patch seems to do the trick.
I don't think the comment is inconsistent. It says why killing the greeter is a bad idea. It sounds like we may need some other fixes to account for the fact the greeter isn't killed.
Killing the greeter is a bad idea, unless you use the -novtswitch option with X.
Comment on attachment 160537 [details] [review] Kill the greeter after switching VTs The comment explains why we don't do this now.
If gdmflexiserver isn't bringing you back to the existing greeter that is a separate bug. But I don't see that here.
(In reply to comment #5) > If gdmflexiserver isn't bringing you back to the existing greeter that is a > separate bug. But I don't see that here. This was a wrong ConsoleKit configuration in our packages, and I’ve fixed it in the meantime. That doesn’t change the fact that leaving unused X servers is plain wrong. If X switches back to an undefined VT when exiting, it’s a problem with X, not something to work around in such a wrong way. And it can be fixed in the simplest way with the -novtswitch option.
Actually I should explain more how we handle user switching as a whole, since with just this patch the benefits are unclear. 1. GDM launches X with -novtswitch. Therefore when X dies it will not switch VTs by itself. 2. We add this patch for killing the greeter when it has been used for switching users. 3. We add another patch to respawn the greeter once after a session ends : http://patch-tracker.debian.org/patch/series/view/gdm3/2.30.2-3/20_endsession_respawn.patch This way the user switching experience is completely consistent IMHO. - When you log out, you always go back to the greeter, regardless of the session and the VT it was started on. Currently you will sometimes go back to an existing session, sometimes to a greeter, and in the worst case, you will go back to an empty VT. - No X servers are leaked. Once you have typed your password in the greeter, there are no greeter processes remaining.
I think we should try to turn -novtswitch on in the 2.91 cycle
Created attachment 176611 [details] [review] Updated patch for 2.30.5 Still needs to be ported to 2.91
Created attachment 176612 [details] [review] Respawn the greeter at the end of the session That’s the second piece of this change, the one respawning the greeter in all cases at the end of a session.
Additionnal note: to add the -novtswitch option, the easy path is to fix bug#586777.
Jos, I was interested to test this patch. However, I could not get these patches to compile. I applied the patches in Comments #9 and #11. The problem seems to be that because the the patch in Comment #11 references the variable slvae->priv->display_is_nested. However, I don't see any other references to this variable in GDM 2.30.5 or these patches. Is there hunk(s) missing from the patch that adds and set this variable? Am I missing something?
Oops, I meant that I applied the patches in comments #9 and #10 and the variable causing the compile issue is slave->priv->display_is_nested in comment #10.
(In reply to comment #13) > Oops, I meant that I applied the patches in comments #9 and #10 and the > variable causing the compile issue is slave->priv->display_is_nested in comment > #10. Yes, display_is_nested is here because we also apply another patch to get “gdmflexiserver --xnest” to work. I haven’t rebased the patch so far. For its scope, I think that just applying it without the check for display_is_nested should work just fine. If you want to test the patches, don’t forget to launch X with -novtswitch (or whatever is the equivalent for Xsun).
Jos: Thanks for the tips. I am doing some testing with this code, but it would help if things were a bit more clear. In comment #6 you mention that fixing the ConsoleKit configuration corrected the issue you had with gdmflexiserver not bringing you back to the existing greeter. Also, you seem to indicate in comment #6 that solving the problem in this way is not ideal solution because leaving unused X servers is plain wrong. Could you explain more detail? It would help me understand this issue if you could highlight what specific ConsoleKit configuration affected how this worked, and what specific problems you saw after you fixed the ConsoleKit configuration. Once you fixed the ConsoleKit configuration, how did the problem manifest itself? Solaris 11 uses Xorg now (Xsun is S10 and earlier), but it does not seem to support the -novtswitch option. Without using this option, I am seeing some odd behaviors. Sometimes I get a kernel panic and my system reboots. Also, it seems that when I do a switch to return to an existing session I see the xscreensaver. Before I have a chance to type a password, the login greeter restarts. Are these the sorts of odd behavior you would expect using this without the -novtswitch option? I'll talk with the Xserver team here at Sun and discuss with them the possibility of adding this option, or find out if there is some other way I can do something similar. Thanks!
(In reply to comment #15) > In comment #6 you mention that fixing the ConsoleKit configuration corrected > the issue you had with gdmflexiserver not bringing you back to the existing > greeter. Also, you seem to indicate in comment #6 that solving the problem in > this way is not ideal solution because leaving unused X servers is plain wrong. > > Could you explain more detail? It would help me understand this issue if you > could highlight what specific ConsoleKit configuration affected how this > worked, and what specific problems you saw after you fixed the ConsoleKit > configuration. Once you fixed the ConsoleKit configuration, how did the > problem manifest itself? OK, I’ll try to make it clearer. User sessions have an empty ConsoleKit session-type, while the login screen has a “LoginSession” as session-type. When you start another login screen, it stays here, and gdmflexiserver later attempts to find again a “LoginSession” session to switch back to it instead of launching another one. This is the part that didn’t work with my first attempts (due to a wrong change in the CK session types), so not only there was a remaining login session, but it was not re-used and just more and more X servers were launched. But even with this working, you have an unused X server that remains around after using user switching. And it will prevent proper shutdown without entering the root password, since ConsoleKit will see it just like another user session. Furthermore, you can easily get into a situation where you reach a VT without a X server. > Solaris 11 uses Xorg now (Xsun is S10 and earlier), but it does not seem to > support the -novtswitch option. Without using this option, I am seeing some > odd behaviors. Sometimes I get a kernel panic and my system reboots. Also, it > seems that when I do a switch to return to an existing session I see the > xscreensaver. Before I have a chance to type a password, the login greeter > restarts. Are these the sorts of odd behavior you would expect using this > without the -novtswitch option? I'll talk with the Xserver team here at Sun > and discuss with them the possibility of adding this option, or find out if > there is some other way I can do something similar. Kernel panic is definitely not an expected behavior, with or without the option :) AFAIK -novtswitch only does something for Linux and is ignored for other platforms. On Linux, when launching the X server it will automatically switch to the VT it is allocated on. When exiting, it will switch back to the VT from which it was launched - which is nice for startx, but highly undesirable for multi-user X environments. The -novtswitch option, unlike what the name says, will still perform the VT switch at startup, but it will not switch back on server exit. This way, we can just switch to the desired session, kill the login screen’s X server, and nothing harmful happens.
Created attachment 187224 [details] [review] Kill the greeter when switching VTs Here is the first patch updated for 3.0. I have added the -novtswitch hardcoded in the same patch, so that the change is self-contained.
Created attachment 187225 [details] [review] Respawn the greeter at the end of the session And here is the second patch, updated and tested too for 3.0.
Created attachment 187236 [details] [review] Kill the greeter when switching VTs As Ray noticed on IRC, the 2-second timeout is useless now. Here is an updated patch.
Review of attachment 187236 [details] [review]: So I tested this patch, it seems to work for the most part. In the future it would be good if the patch was in git-am format (maybe posted using git-bz) so I have a commit message and authorship already taken care of. what about something like this: daemon: clean up greeter when not in use When user-switching, GDM needs to show a transient greeter for the user to pick name from the list and jump to that session. After the user has been jumped to the selected session we don't really have a use for the greeter anymore. Historically, we've kept it around anyway, though, because X by default jumps back to the VT it started on when it exits, and we don't want the user to get thrown to an empty VT after they log out. This commit changes X to get started with the "-novtswitch" option, so that it doesn't do the undesirable vt-switch-on-exit thing. This allows us to clean up the useless greeter following user switches ? I have couple comments below. ::: gdm-3.0.0.orig/daemon/gdm-simple-slave.c @@ +544,3 @@ destroy_session (slave); + gdm_slave_stopped (GDM_SLAVE (slave)); why do you stop the slave here? ::: gdm-3.0.0.orig/daemon/gdm-server.c @@ +1145,2 @@ server->priv->pid = -1; +#ifdef __linux__ ifdef __linux__ is a no-no. We should either force -novtswitch unconditionally, change what we're getting from configure to be a full command line instead of a just an X server. If the former, we need to get Brian to get Alan to add -novtswitch to Sun's X server. If the latter, then we need to retain the existing behavior if we aren't passing -novtswitch
I also tested these patches and found they work well. The current plan is to add a "-novtswitch" option to the Xorg server on Solaris. Though, to be honest "-novtswitch" is not a very good option name, and does not actually describe well what this option does. It might be better if a more sensible interface were integrated upstream into Xorg to control this and used. Is there any a value in running Xorg without "-novtswitch"?
(In reply to comment #21) > I also tested these patches and found they work well. The current plan is to > add a "-novtswitch" option to the Xorg server on Solaris. ah great. That makes things easy then. > Though, to be honest "-novtswitch" is not a very good option name, and does not > actually describe well what this option does. It might be better if a more > sensible interface were integrated upstream into Xorg to control this and used. what do you have in mind? If X gets a better interface, we could certainly use that instead. > Is there any a value in running Xorg without "-novtswitch"? Not enough value to justify it given the direction of the bandwagon, I think.
(In reply to comment #18) > Created an attachment (id=187225) [details] [review] > Respawn the greeter at the end of the session > > And here is the second patch, updated and tested too for 3.0. So is the idea here that any time a session initiated from user switching gets logged out, that a greeter fills the "void" ? Won't that mean you get like N greeters potentially? Wouldn't it be better to jump back to any existing greeter session for the seat if there is one already?
(In reply to comment #20) > ::: gdm-3.0.0.orig/daemon/gdm-simple-slave.c > @@ +544,3 @@ > destroy_session (slave); > > + gdm_slave_stopped (GDM_SLAVE (slave)); > > why do you stop the slave here? Since you’re switching to an existing user session, you don’t need the slave that holds the greeter session anymore. The whole point of using -novtswitch is to be able to kill this slave (and free the resources it consumes) without having X switch back to a random VT when it dies. > ::: gdm-3.0.0.orig/daemon/gdm-server.c > @@ +1145,2 @@ > server->priv->pid = -1; > +#ifdef __linux__ > > ifdef __linux__ is a no-no. We should either force -novtswitch > unconditionally, change what we're getting from configure to be a full command > line instead of a just an X server. Currently only Xorg under linux supports this argument. I tried passing it on FreeBSD and it just refuses to start with it. > If the former, we need to get Brian to get Alan to add -novtswitch to Sun's X > server. If the latter, then we need to retain the existing behavior if we > aren't passing -novtswitch I am not sure what the X behavior is for non-Linux OSes. The reason for -novtswitch is that on Linux, Xorg has extra code to switch back to the launching VT. If this code doesn’t exist on other OSes, the reason for -novtswitch doesn’t either. It would be better if -novtswich was ignored in this case, instead of showing an error. I will ask for input from our kFreeBSD porters. (In reply to comment #23) > So is the idea here that any time a session initiated from user switching gets > logged out, that a greeter fills the "void" ? Won't that mean you get like N > greeters potentially? No, because the first patch ensures that unused greeters are killed (that’s the gdm_slave_stopped addition). > Wouldn't it be better to jump back to any existing greeter session for the seat > if there is one already? Of course, if there was one. But I think the correct behavior is to only have a greeter session running when you need it.
I had a deeper look at FreeBSD. There is code to switch back VTs at server termination, but the -novtswitch option is not implemented. Filed as https://bugs.freedesktop.org/show_bug.cgi?id=37174
(In reply to comment #24) > (In reply to comment #20) > > ::: gdm-3.0.0.orig/daemon/gdm-simple-slave.c > > @@ +544,3 @@ > > destroy_session (slave); > > > > + gdm_slave_stopped (GDM_SLAVE (slave)); > > > > why do you stop the slave here? > > Since you’re switching to an existing user session, you don’t need the slave > that holds the greeter session anymore. The whole point of using -novtswitch is > to be able to kill this slave (and free the resources it consumes) without > having X switch back to a random VT when it dies. Right, I get that. I'm just wondering why you're stopping the slave when accreditation fails. Shouldn't it just reset the greeter in that case and let the user try again? We won't be switching to an existing user session in that case will we? > > ::: gdm-3.0.0.orig/daemon/gdm-server.c > > @@ +1145,2 @@ > > server->priv->pid = -1; > > +#ifdef __linux__ > > > > ifdef __linux__ is a no-no. We should either force -novtswitch > > unconditionally, change what we're getting from configure to be a full command > > line instead of a just an X server. > > Currently only Xorg under linux supports this argument. I tried passing it on > FreeBSD and it just refuses to start with it. Right, my point was #ifdef __linux__ was something we avoid in the code. I think it may be in a place or two, but only for weird historical reasons that should be cleaned up. Put another way, it's not whether we're on linux that determines whether we should have sane vt switch behavior, it's whether or not the -novtswitch option is available. So, we shouldn't have #ifdef __linux__, we should have #ifdef HAVE_SANE_VT_SWITCH_BEHAVIOR or whatever. But it would be best if we could avoid #ifdefs at all, actually. It sounds like we may not be able to get around it on freebsd though. What I was proposing above, was, instead of collecting XSERVER in configure, we could collect FULL_XSERVER_COMMAND_LINE. that way distros could specify if they can use -novtswitch or not (with the default being yes). If the answer is no, though, we still need to keep the old code the way it was I think. Otherwise, users are going to get thrown to black screens, right? Another option, is we could declare platforms that have VTs but don't have -novtswitch as broken and not handle them as well as we are handling them now. Now is broken, too, after all, just in a different way. > (In reply to comment #23) > > So is the idea here that any time a session initiated from user switching gets > > logged out, that a greeter fills the "void" ? Won't that mean you get like N > > greeters potentially? > > No, because the first patch ensures that unused greeters are killed (that’s the > gdm_slave_stopped addition). unless the user switches vts with ctrl-alt-f8 or whatever... maybe that's a corner case though. > > Wouldn't it be better to jump back to any existing greeter session for the seat > > if there is one already? > > Of course, if there was one. But I think the correct behavior is to only have a > greeter session running when you need it. but we probably want at most one greeter per seat, so it seems like it would be better if there was code to make sure that happens.
Oh, wow, there's already a patch for freebsd here: http://lists.x.org/archives/xorg-devel/2011-May/022183.html In response to your bug report.
Ray, in response to comment #22, I was wondering if the -novtswitch option is really needed. The behavior this option controls seems like a bug. Nobody would ever want to use the Xserver without this option. So why not just fix the bug and make X always work this way, without needing the option?
Brian, oh I see what you mean. Well, for users who don't use GDM, but just use "startx" the current behavior is about right. Given startx used to be status quo, I guess that's the reasoning for the current defaults. Granted, these days startx is used much less frequently. One idea would be to add a e.g. -display-manager-mode option to X to give us the right out of the box behavior for VTs and also potentially give us display number auto selection (see http://lists.x.org/archives/xorg-devel/2011-January/018057.html for ajax's -displayfd proposal) etc.
So I worked on this a bit. I'll post some patches on what Josselin did. Would appreciate feedback/review.
Created attachment 188088 [details] [review] daemon: add switch-on-finish property to display When a display exits, sometimes it's desirable to jump to a login screen. This depends if the display is transient or not, and also whether the display is hosting an already logged in session or not. This commit adds a property to the display object that says whether a switch should happen or not. Note the code to actually perform the switch will get added in a follow up commit. Based on work by Josselin Mouette <joss@debian.org>
Created attachment 188089 [details] [review] daemon: parameterize create_display There's some logic in create_display and friends, that would be good to reuse for transient displays. This commit adds a type argument to create_display to say what kind of display to create. Right now it only supports the one type it's always supported, but that will change in a follow up commit.
Created attachment 188090 [details] [review] daemon: share display respawn logic betewen static and transient displays Right now we ignore transient displays after they're created. We make no attempt to clean them up from the display store or put the user on a login screen when their display ends or anything like that. This commit changes the display factory to monitor transient display status in much the same way it monitors static display status. Note for static displays we always respawn when finishing, where as with transient displays either die quietly, jump to an existing login display, or respawn.
Created attachment 188091 [details] [review] daemon: clean up greeter when unused When user switching GDM needs to show a transient greeter for the user to pick a name from the list and jump to that session. After the user has been jumped to the selected session we don't really have a use for the greeter session. Historically, we've kept it around anyway, though, because X by default jumps back to the VT it started on when it exits, and we don't want the user to get thrown to an empty VT after they log out. This commit changes X to get started with the "-novtswitch" option, so that it doesn't do the undesirable switch-on-exit thing. This allows us to clean up the useless greeter following user switches. Based on work by Josselin Mouette <joss@debian.org>
I still haven't fully addressed the "only ever have one login screen" issue I meantioned in comment 28, but the patches change the respawning to happen in the factory code. That leaves things structured in a way so that it should be fairly straightforward to add the login screen jumping logic later. Right now, I've left it as a FIXME.
Joss, did you get a chance to try these? I'd like to push them and leave the login screen jumping as a future FIXME, but I'd like confirmation they work beyond my limited testing, before they go out.
Thanks a lot for the cleanup work. I have run these patches with a few test cases and I haven’t noticed any regression compared to mine, so I’d say to go ahead for now.
Okay, I'll go ahead and put it in now then. Attachment 188088 [details] pushed as adc116c - daemon: add switch-on-finish property to display Attachment 188089 [details] pushed as 168f0fa - daemon: parameterize create_display Attachment 188090 [details] pushed as 8bf1de1 - daemon: share display respawn logic betewen static and transient displays Attachment 188091 [details] pushed as 15a3f0c - daemon: clean up greeter when unused
There’s a remaining issue because you didn’t integrate one of the changes: not restarting static displays. Consider the following scenario: - user1 logs on (:0, static display) - user2 logs on (:1, transient display) - spawn a new login session (:2, transient) - user1 logs in again (:2 exits, switches back to :0) - user1 logs out (to a login window on :0) - user2 logs in There gdm switches to :1 but since :0 is a static display it switches back immediately to :0 when the login session is respawned here.
Created attachment 190001 [details] [review] Don’t respawn static displays Here is the patch fixing this. There is no need to respawn systematically static displays. The new logic guarantees that whenever a display dies (static or transient), it is because you switched to a working one. The only difference between a static and a transient display is handling of first login (autologin).