Skip to content

Multiseat bug fixes, and some cleanups - #300

Merged
robert-ancell merged 9 commits into
ubuntu:mainfrom
rhansen:multiseat
Apr 28, 2023
Merged

Multiseat bug fixes, and some cleanups#300
robert-ancell merged 9 commits into
ubuntu:mainfrom
rhansen:multiseat

Conversation

@rhansen

@rhansen rhansen commented Apr 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR contains a few fixes that affect multiseat, especially with Wayland. There are also several cleanups.

The important bits:

  • Initialize WaylandSessionPrivate.vt to -1 so that LightDM doesn't erroneously attempt to switch to VT 0 or set XDG_VTNR on non-seat0 seats.
  • Use systemd-logind to discover the active session on non-seat0 seats rather than always returning NULL. This prevents LightDM from immediately restarting and switching to the greeter concurrently with a new Wayland session.
  • Disable user switching if logind says it isn't supported. Note that logind v245 and earlier erroneously reports CanMultiSession=no when it is actually supported so I believe this will break user switching for such users. See [RFC] Declare that non-seat0 seats can support multisession. systemd/systemd#15337.

I believe the latter two together fix LP#1371250 (at least I can't reproduce it).

Please do not squash this when merging—the commits are intended to be separate. I will rebase these as needed to incorporate feedback or update to the latest main.

@rhansen
rhansen force-pushed the multiseat branch 5 times, most recently from 9d4d4ed to b1026a5 Compare April 22, 2023 01:05
@rhansen
rhansen marked this pull request as draft April 22, 2023 01:07
@rhansen

rhansen commented Apr 22, 2023

Copy link
Copy Markdown
Contributor Author

This PR now depends on the commits from PR #301 so I'm marking this as draft until that is merged.

@rhansen
rhansen force-pushed the multiseat branch 2 times, most recently from 6927a71 to fadca4e Compare April 25, 2023 23:33
@rhansen
rhansen marked this pull request as ready for review April 25, 2023 23:34
robert-ancell
robert-ancell reviewed Apr 26, 2023
View reviewed changes
Comment thread src/seat.c Outdated
rhansen added 9 commits April 27, 2023 03:00
@rhansen
Delete redundant cleanup code
1944e40
@rhansen
Initialize WaylandSessionPrivate.vt to -1
04036a1 
This matters when starting a Wayland session on a non-seat0 seat:
  * It eliminates an erroneous attempt to switch to VT 0.
  * The XDG_VTNR environment variable is no longer set.

Also add a multiseat test.

A similar change for XServerLocalPrivate is not needed because its vt field is
already initialized to -1.
@rhansen
tests: New FENCE script command to ensure event order
bda3e97 
By default, emitted status text is allowed to match a status matcher
line from anywhere in the script.  Thus, a script like the
following:

    #?*FOO
    #?BAR

will happily accept a sequence of events like this:

    BAR
    *FOO

This loose ordering avoids test flakiness in the presence of
concurrent events, but can be problematic if the test wants to assert
that the BAR event is an effect of the FOO command and not
coincidental.  (A concrete example: assert that a greeter was
launched *because* the user session terminated, not in parallel with
the briefly-lived user session.)

Script authors may be tempted to assert causality by introducing a
WAIT like this:

    #?*WAIT
    #?*FOO
    #?BAR

but that would not have the desired effect because it will still
accept a BAR event before the FOO command (or even the WAIT command)
is executed.

The new FENCE command helps script authors assert causality by
ensuring that an emitted status does not match a line on the other
side of the fence.  For example, the following script works as
expected and asserts a causal relationship between FOO and BAR:

    #?*WAIT
    #?*FENCE
    #?*FOO
    #?BAR

The above script only works if the WAIT is long enough to sufficiently
rule out coincidence.  To see why, note that the following sequence of
events would be accepted by the above script:

    *WAIT
    *FENCE
    BAR
    *FOO

The order of the FENCE and FOO should not be switched, otherwise it
introduces a theoretical race condition: If the BAR event is caused by
the FOO command, and the BAR event happens to arrive before the FENCE
is executed, then the test will fail when it should not.  (Technically
it is not possible to lose the race right now because of how
run_commands is implemented, but scripts should not rely on an
implementation detail.  Besides, it doesn't eliminate the need for the
WAIT.)

This new command will be used in a future commit to add a regression
test for a Wayland multiseat bug.
@rhansen
Use systemd-logind to discover active session on non-seat0 seats
cf2f614 
Before, `seat_local_get_active_session` would always return `NULL` for
non-`seat0` seats.  This broke Wayland sessions on non-`seat0` seats:

  1.  User logged in to a Wayland session on `seat1`.
  2.  LightDM properly terminated the X server to allow Wayland to
      take over the seat's hardware.  (See the side note below.)
  3.  Wayland session started.
  4.  The X server termination triggered a call to
      `display_server_stopped_cb`.
  5.  `display_server_stopped_cb` called `seat_get_active_session` to
      see if it needed to start a greeter to replace a terminated
      session associated with the terminated display.
  6.  `seat_get_active_session` called
      `seat_local_get_active_session`.
  7.  `seat_local_get_active_session` erroneously probed the active VT
      even though VTs are only associated with `seat0`.
  8.  After finding no matching session associated with both `seat1`
      and the active VT, `seat_local_get_active_session` returned
      `NULL` when it should have returned the new Wayland session.
  9.  Due to the `NULL` response, `display_server_stopped_cb`
      erroneously arrived at the conclusion that a greeter must be
      started on `seat1`.
  10. LightDM started a new X server and greeter on `seat1`, stomping
      on the newly created Wayland session.

Side note: I don't think that terminating X to allow Wayland to take
over the seat's hardware is required because I believe X and
most?/all? Wayland compositors cooperatively share the devices via
systemd-logind or maybe libseat.  When switching sessions, logind uses
a hand-off protocol to smoothly change which process is allowed to
access the devices.  This makes session switching possible even
without virtual terminals.  For details, see
<https://dvdhrm.wordpress.com/2013/08/25/sane-session-switching/>.

The above sequence of events is apparent in the debug log from a
minimal script (the script is not included in this commit; the
relevant lines from the log are copied below, with annotations):

    #
    # User logs in to seat1 with a Wayland session:
    #
    GREETER-X-1 AUTHENTICATION-COMPLETE USERNAME=no-password2 AUTHENTICATED=TRUE
    *GREETER-X-1 START-SESSION
    [+0.32s] DEBUG: Seat seat1: Creating display server of type wayland
    [+0.32s] DEBUG: Seat seat1: Display server ready, running session
    [+0.32s] DEBUG: Registering session with bus path /org/freedesktop/DisplayManager/Session0
    [+0.32s] DEBUG: Session pid=1309577: Running command /home/rhansen/floss/lightdm/tests/src/lightdm-session test-session
    #
    # LightDM starts shutting down X+greeter for seat1 while
    # concurrently activating the new Wayland session (c1 = the
    # stopping seat1 greeter session, c2 = the starting Wayland
    # session).
    #
    [+0.33s] DEBUG: Seat seat1: Stopping greeter
    [+0.33s] DEBUG: Terminating login1 session c1
    [+0.33s] DEBUG: Session pid=1309572: Sending SIGTERM
    [+0.33s] DEBUG: Activating login1 session c2
    GREETER-X-1 TERMINATE SIGNAL=15
    LOGIN1 ACTIVATE-SESSION SESSION=c2
    #
    # The greeter session and X exit.
    #
    [+0.33s] DEBUG: Session pid=1309572: Exited with return value 0
    [+0.33s] DEBUG: Seat seat1: Session stopped
    [+0.33s] DEBUG: Seat seat1: Stopping display server, no sessions require it
    [+0.33s] DEBUG: Sending signal 15 to process 1309569
    XSERVER-1 TERMINATE SIGNAL=15
    [+0.33s] DEBUG: Process 1309569 exited with return value 0
    [+0.33s] DEBUG: XServer 1: X server stopped
    [+0.33s] DEBUG: Seat seat1: Display server stopped
    #
    # The start of the problem: LightDM should not restart the greeter
    # until the Wayland session terminates:
    #
    [+0.33s] DEBUG: Seat seat1: Active display server stopped, starting greeter
    [+0.33s] DEBUG: Seat seat1: Creating greeter session
    [+0.33s] DEBUG: Seat seat1: Creating display server of type x
    [+0.33s] DEBUG: Seat seat1: Starting local X display
    [+0.33s] DEBUG: XServer 1: Launching X Server
    [+0.33s] DEBUG: Launching process 1309583: /home/rhansen/floss/lightdm/tests/src/X :1 -seat seat1 -auth /var/run/lightdm/root/:1 -nolisten tcp
    [+0.33s] DEBUG: XServer 1: Waiting for ready signal from X server :1
    XSERVER-1 START SEAT=seat1
    #
    # The Wayland session has started concurrently with the
    # replacement X+greeter.
    #
    SESSION-WAYLAND START XDG_SEAT=seat1 XDG_GREETER_DATA_DIR=/var/lib/lightdm-data/no-password2 XDG_SESSION_TYPE=wayland XDG_SESSION_DESKTOP=wayland USER=no-password2
    *XSERVER-1 INDICATE-READY
    XSERVER-1 INDICATE-READY
    [+0.34s] DEBUG: Got signal 10 from process 1309583
    [+0.34s] DEBUG: XServer 1: Got signal from X server :1
    [+0.34s] DEBUG: XServer 1: Connecting to XServer :1
    XSERVER-1 ACCEPT-CONNECT
    [+0.34s] DEBUG: Seat seat1: Display server ready, starting session authentication
    [+0.34s] DEBUG: Session pid=1309587: Started with service 'lightdm-greeter', username 'lightdm'
    [+0.40s] DEBUG: Session pid=1309587: Running command /home/rhansen/floss/lightdm/tests/src/.libs/test-gobject-greeter
    #
    # Finally, the replacement greeter stomps on the new Wayland
    # session.
    #
    [+0.40s] DEBUG: Locking login1 session c2
    LOGIN1 LOCK-SESSION SESSION=c2
@rhansen
Improve debug logging when seat_switch_to_greeter fails
f66cf26 
This will make it easier to troubleshoot multiseat issues.
@rhansen
Try activating an existing greeter before checking for switch support
1527179 
If the seat has an existing greeter session then we should activate it
regardless of whether the seat supports user switching.  (If the seat does not
support user switching then there shouldn't be an existing greeter session, I
think.  Either way, it doesn't hurt to try.)
@rhansen
Consolidate Seat.setup and Seat.start for readability
e93fcff 
The name "setup" implies early initialization, when it was actually
run just before start.
@rhansen
Move supports_multi_session initialization to init
746723c 
This makes it possible to use seat_set_supports_multi_session to
change the support status after the seat is created but before it is
started.
@rhansen
Disable user switching if logind says it isn't supported
f891e8c 
This should work now that seat_local_get_active_session returns the
proper value for non-seat0 seats.

Note: systemd-logind v245 and older erroneously report
CanMultiSession=no on non-seat0 seats even when it is supported.  This
change will break those users.  See
<systemd/systemd#15337>.
@robert-ancell
robert-ancell merged commit e520b32 into ubuntu:main Apr 28, 2023
@rhansen
rhansen deleted the multiseat branch April 28, 2023 01:44
@rhansen

rhansen commented Apr 28, 2023

Copy link
Copy Markdown
Contributor Author

Thanks @robert-ancell!

@JPeisach JPeisach added this to the 1.33.0 milestone Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robert-ancell robert-ancell robert-ancell left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.33.0

Development

Successfully merging this pull request may close these issues.

3 participants

@rhansen @robert-ancell @JPeisach