gdm

Merge lp:~albertomilone/gdm/lp1697882 into lp:~ubuntu-desktop/gdm/ubuntu

Proposed by Alberto Milone on 2017-06-28
Status: Merged
Approved by: Iain Lane on 2017-09-04
Approved revision: 432
Merged at revision: 432
Proposed branch: lp:~albertomilone/gdm/lp1697882
Merge into: lp:~ubuntu-desktop/gdm/ubuntu
Diff against target: 271 lines (+256/-0)
2 files modified
debian/patches/95_hide_x11_sessions_with_nvidia_kms.patch (+255/-0)
debian/patches/series (+1/-0)
To merge this branch: bzr merge lp:~albertomilone/gdm/lp1697882
Reviewer Review Type Date Requested Status
Iain Lane Approve on 2017-09-04
Daniel van Vugt (community) Needs Fixing on 2017-07-13
Jeremy Bicha 2017-06-28 Pending
Review via email: mp+326426@code.launchpad.net

Description of the change

This change, which is (Ubuntu specific), will hide away the X11 sessions when KMS is enabled in the nvidia driver, leaving the Wayland session as the only option.

Disabling KMS will make the sessions available again after a reboot.

Fixes LP: #1697882

To post a comment you must log in.
Iain Lane (laney) wrote :

Thanks. Please could you file upstream and then add that URL into the patch as "Forwarded: <url>"?

Is there a better way than looking at a configuration file such as looking at the loaded modules somehow? Isn't that distro specific or at least fragile?

review: Needs Information
Alberto Milone (albertomilone) wrote :

> Thanks. Please could you file upstream and then add that URL into the patch as
> "Forwarded: <url>"?
>

This is something that will have to be fixed in the kernel, when the Unix Device Memory Allocator API lands in the kernel. It will take a while. I don't think it's a GNOME specific problem, but we might want to work around it for now.

> Is there a better way than looking at a configuration file such as looking at
> the loaded modules somehow? Isn't that distro specific or at least fragile?

Sure, /sys/module/nvidia_drm/parameters/modeset would be the best place to check the availability of KMS. Unfortunately, that would require root privileges, which (correct me if I'm wrong) we don't have.

Iain Lane (laney) wrote :

On Wed, Jun 28, 2017 at 04:35:35PM -0000, Alberto Milone wrote:
> > Thanks. Please could you file upstream and then add that URL into the patch as
> > "Forwarded: <url>"?
> >
>
> This is something that will have to be fixed in the kernel, when the Unix Device Memory Allocator API lands in the kernel. It will take a while. I don't think it's a GNOME specific problem, but we might want to work around it for now.

Right, but it is correct that this workaround (disabling X on nvidia +
modesetting) is not Ubuntu specific, isn't it?

> > Is there a better way than looking at a configuration file such as looking at
> > the loaded modules somehow? Isn't that distro specific or at least fragile?
>
> Sure, /sys/module/nvidia_drm/parameters/modeset would be the best place to check the availability of KMS. Unfortunately, that would require root privileges, which (correct me if I'm wrong) we don't have.

gdm3 runs as root. I'm not sure if it drops permissions before getting
to this point - I think probably not, and this is worth looking into.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Alberto Milone (albertomilone) wrote :

> On Wed, Jun 28, 2017 at 04:35:35PM -0000, Alberto Milone wrote:
> > > Thanks. Please could you file upstream and then add that URL into the
> patch as
> > > "Forwarded: <url>"?
> > >
> >
> > This is something that will have to be fixed in the kernel, when the Unix
> Device Memory Allocator API lands in the kernel. It will take a while. I don't
> think it's a GNOME specific problem, but we might want to work around it for
> now.
>
> Right, but it is correct that this workaround (disabling X on nvidia +
> modesetting) is not Ubuntu specific, isn't it?
>

Correct. It is how we check KMS that, with the current approach, is Ubuntu specific (because of the root privileges thing).

> gdm3 runs as root. I'm not sure if it drops permissions before getting
> to this point - I think probably not, and this is worth looking into.
>

I'm pretty sure I ran into this problem when I started working on this. I can double check, as the effort should be minimal.

Alberto Milone (albertomilone) wrote :

Unfortunately, I have just checked, and we no longer have root privileges by the time can_create_environment() is called.

Iain Lane (laney) wrote :

On Wed, Jun 28, 2017 at 06:41:32PM -0000, Alberto Milone wrote:
> Unfortunately, I have just checked, and we no longer have root privileges by the time can_create_environment() is called.

Ok, that's annoying - I don't suppose it's possible to grab this value
before privileges are dropped and pass it around?

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Alberto Milone (albertomilone) wrote :

> On Wed, Jun 28, 2017 at 06:41:32PM -0000, Alberto Milone wrote:
> > Unfortunately, I have just checked, and we no longer have root privileges by
> the time can_create_environment() is called.
>
> Ok, that's annoying - I don't suppose it's possible to grab this value
> before privileges are dropped and pass it around?
>

I'm not sure about that. It's probably easier to use the current udev rule, that the nvidia packages ship, to check the value when the module is loaded, and to write it somewhere in /run, so that GDM can read it later.

What do you think?

Iain Lane (laney) wrote :

On Thu, Jun 29, 2017 at 10:29:58AM -0000, Alberto Milone wrote:
> > On Wed, Jun 28, 2017 at 06:41:32PM -0000, Alberto Milone wrote:
> > > Unfortunately, I have just checked, and we no longer have root privileges by
> > the time can_create_environment() is called.
> >
> > Ok, that's annoying - I don't suppose it's possible to grab this value
> > before privileges are dropped and pass it around?
> >
>
> I'm not sure about that. It's probably easier to use the current udev rule, that the nvidia packages ship, to check the value when the module is loaded, and to write it somewhere in /run, so that GDM can read it later.
>
> What do you think?

I like that more than reading the configuration file - but it doesn't
sounds like something that could go in gdm upstream? Unless maybe you
can get some buy in for standardising on this. Maybe file a bug on
bugzilla and ask for opinions.

If it's too hard to grab the value from /sys, I guess we can do it this
way.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Alberto Milone (albertomilone) wrote :

> I like that more than reading the configuration file - but it doesn't
> sounds like something that could go in gdm upstream? Unless maybe you
> can get some buy in for standardising on this. Maybe file a bug on
> bugzilla and ask for opinions.
>

I don't think it should be upstreamed, and I hope we can drop this in the near future. I think we can live with a small patch for now (which I am going to maintain).

> If it's too hard to grab the value from /sys, I guess we can do it this
> way.

Ok, I'll work on it.

Iain Lane (laney) wrote :

On Thu, Jun 29, 2017 at 11:59:23AM -0000, Alberto Milone wrote:
> > I like that more than reading the configuration file - but it doesn't
> > sounds like something that could go in gdm upstream? Unless maybe you
> > can get some buy in for standardising on this. Maybe file a bug on
> > bugzilla and ask for opinions.
> >
>
> I don't think it should be upstreamed, and I hope we can drop this in the near future. I think we can live with a small patch for now (which I am going to maintain).

Why not? Does this somehow work on other distributions? If it doesn't,
why would it be desirable to keep showing broken sessions there?

> > If it's too hard to grab the value from /sys, I guess we can do it this
> > way.
>
> Ok, I'll work on it.

k.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Alberto Milone (albertomilone) wrote :

I have just filed an upstream bug report about this:
https://bugzilla.gnome.org/show_bug.cgi?id=784470

lp:~albertomilone/gdm/lp1697882 updated on 2017-07-13
411. By Ken VanDine on 2017-07-03

Only prompt the user to choose display managers when we haven't already determined a migration to gdm3 is recommended.

412. By Ken VanDine on 2017-07-03

Merged

413. By Ken VanDine on 2017-07-03

* debian/gdm3.config
  - Only prompt the user to choose display managers when we haven't already
    determined a migration to gdm3 is recommended.
* debian/control.in
  - Explicitly depend on ubuntu-session so we can ensure gdm3 configure gets
    run after ubuntu-session's configure to handle the transition from
    lightdm to gdm3

414. By Ken VanDine on 2017-07-10

released

415. By Ken VanDine on 2017-07-11

* debian/gdm3.config
  - Handle the transition from lightdm in gdm3 rather than ubuntu-session
* debian/control.in
  - Dropped depends from ubuntu-session

416. By Ken VanDine on 2017-07-11

Fixed ubuntu-session version check

417. By Ken VanDine on 2017-07-11

Dropped depends for ubuntu-session

418. By Ken VanDine on 2017-07-11

releasing package gdm3 version 3.24.2-1ubuntu5

419. By Didier Roche on 2017-07-12

debian/patches/ubuntu_prefer_x11_session.patch:
Prefer X11 session over wayland. As we are not ready yet for
switching to wayland yet, let's prefer X11 session when no specific
choice was made. (LP: #1703601)
This will be reverted once we switch to wayland by default.

420. By Didier Roche on 2017-07-12

releasing package gdm3 version 3.24.2-1ubuntu6

421. By Iain Lane on 2017-07-12

debian/gdm3.config: Check the version of gdm3 instead of ubuntu-session.

And some other minor improvements.

422. By Iain Lane on 2017-07-12

Compare with a ~ version, so that any backports of this exact version aren't re-migrated

423. By Iain Lane on 2017-07-12

releasing package gdm3 version 3.24.2-1ubuntu7

424. By Daniel van Vugt on 2017-07-13

First attempt

425. By Daniel van Vugt on 2017-07-13

Remember the changelog

Daniel van Vugt (vanvugt) wrote :

Pretty sure this will have conflicts now, sorry.

review: Needs Fixing
lp:~albertomilone/gdm/lp1697882 updated on 2017-08-21
426. By Will Cooke on 2017-07-13

* debian/default.pa
  - Added new file to disable Bluetooth audio devices in PulseAudio from
    gdm3. (LP: #1703415) (LP: #1489651)
* debian/gdm3.install
  - Added details of the default.pa file
* debian/gdm3.postinst
  - Added installation of default.pa and creation of dir if it doesn't
    exist.

427. By Jeremy Bicha on 2017-07-13

releasing package gdm3 version 3.24.2-1ubuntu9

428. By Jeremy Bicha on 2017-08-14

New upstream release 3.25.90.1

* debian/control.in:
  - Bump minimum gnome-settings-daemon to 3.24
* Drop patches applied in new release:
  - Avoid-double-NULL-terminated-array-LP-1704050.patch
  - revert_drop_gdm_shell_session.patch

429. By Jeremy Bicha on 2017-08-14

releasing package gdm3 version 3.25.90.1-0ubuntu1

430. By Didier Roche on 2017-08-21

* debian/patches/ubuntu_prefer_x11_session.patch:
  - remove x11 session preference now that both Ubuntu and GNOME
    switched to wayland

431. By Didier Roche on 2017-08-21

releasing package gdm3 version 3.25.90.1-0ubuntu2

Alberto Milone (albertomilone) wrote :

I have come up with a better solution, which I attached to the upstream bug report:
https://bugzilla.gnome.org/show_bug.cgi?id=784470

lp:~albertomilone/gdm/lp1697882 updated on 2017-08-24
432. By Alberto Milone on 2017-08-24

Hide X11 sessions when KMS is enabled in the nvidia driver

NOTE: this doesn't affect systems with hybrid graphics

Fixes LP: #1697882

Alberto Milone (albertomilone) wrote :

The latest patch is based on what upstream recommended. I have already pushed a change to the udev rule included in the NVIDIA driver in Artful, so that the Gnome shell (which doesn't run with root privileges) can check KMS when getting the sessions using libgdm.

My patch works well here.

Iain Lane (laney) wrote :

I left some comments upstream - please consider those for Ubuntu before the 17.10 release - but I think we can probably upload this now.

Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'debian/patches/95_hide_x11_sessions_with_nvidia_kms.patch'
2--- debian/patches/95_hide_x11_sessions_with_nvidia_kms.patch 1970-01-01 00:00:00 +0000
3+++ debian/patches/95_hide_x11_sessions_with_nvidia_kms.patch 2017-08-24 09:08:19 +0000
4@@ -0,0 +1,255 @@
5+From 06a4117ae5761c184c2c9fe54e4f297becf1d2a9 Mon Sep 17 00:00:00 2001
6+From: Alberto Milone <alberto.milone@canonical.com>
7+Date: Tue, 20 Jun 2017 16:49:10 +0200
8+Subject: [PATCH 1/1] Hide X11 sessions when KMS is enabled in the nvidia
9+ driver
10+
11+Fixes LP: #1697882
12+---
13+ common/gdm-common.c | 78 ++++++++++++++++++++++++++++++++++++++
14+ common/gdm-common.h | 2 +
15+ daemon/gdm-display.c | 10 +++++
16+ daemon/gdm-local-display-factory.c | 5 +++
17+ daemon/gdm-session.c | 6 ++-
18+ libgdm/Makefile.am | 3 ++
19+ libgdm/gdm-sessions.c | 8 +++-
20+ 7 files changed, 109 insertions(+), 3 deletions(-)
21+
22+diff --git a/common/gdm-common.c b/common/gdm-common.c
23+index 31fc810..0dcbbe2 100644
24+--- a/common/gdm-common.c
25++++ b/common/gdm-common.c
26+@@ -47,6 +47,8 @@
27+
28+ G_DEFINE_QUARK (gdm-common-error, gdm_common_error);
29+
30++int nvidia_has_kms = -1;
31++
32+ const char *
33+ gdm_make_temp_dir (char *template)
34+ {
35+@@ -797,3 +799,79 @@ gdm_shell_expand (const char *str,
36+ }
37+ return g_string_free (s, FALSE);
38+ }
39++
40++static gboolean
41++is_module_loaded (const char *module) {
42++ char line[4096];
43++ gboolean status = FALSE;
44++ FILE *file = NULL;
45++ const char *modules_file = "/proc/modules";
46++
47++ file = fopen (modules_file, "r");
48++ if (!file) {
49++ g_debug ("Failed to parse %s\n", modules_file);
50++ return status;
51++ }
52++
53++ while (fgets (line, sizeof (line), file)) {
54++ char *tok;
55++ tok = strtok (line, " \t");
56++ if (strstr (tok, module) != NULL) {
57++ status = TRUE;
58++ break;
59++ }
60++ }
61++ fclose(file);
62++
63++ return status;
64++}
65++
66++gboolean
67++is_nvidia_kms_available (void)
68++{
69++ gsize length;
70++ GError *error = NULL;
71++ gboolean status = FALSE;
72++ gchar *contents = NULL;
73++ const char *kms_file = "/sys/module/nvidia_drm/parameters/modeset";
74++
75++ g_debug ("Checking if the NVIDIA driver is loaded, and has KMS enabled\n");
76++
77++ /* Use the cached value */
78++ if (nvidia_has_kms > -1) {
79++ g_debug ("is_nvidia_kms_available: using cached value %d\n", nvidia_has_kms);
80++ return (nvidia_has_kms == 1);
81++ }
82++
83++ /* Let's leave hybrid graphics alone */
84++ if (is_module_loaded ("i915")) {
85++ g_debug ("is_nvidia_kms_available: Intel detected, doing nothing.\n");
86++ goto end;
87++ }
88++
89++ if (!g_file_test (kms_file, G_FILE_TEST_EXISTS)) {
90++ g_debug ("is_nvidia_kms_available: Cannot access '%s'", kms_file);
91++ goto end;
92++ }
93++
94++ if (!g_file_get_contents (kms_file, &contents, &length, &error)) {
95++ g_debug ("is_nvidia_kms_available: Failed to parse '%s': %s",
96++ kms_file,
97++ (error && error->message) ? error->message : "(null)");
98++ g_error_free (error);
99++ goto end;
100++ }
101++
102++ g_debug ("is_nvidia_kms_available: contents, \n%s\n", contents);
103++
104++ status = (strstr (contents, "Y\n") != NULL);
105++ g_free (contents);
106++
107++end:
108++ /* Cache the value */
109++ nvidia_has_kms = status ? 1 : 0;
110++
111++ g_debug ("is_nvidia_kms_available: setting status to %d\n", status);
112++
113++ return status;
114++}
115+diff --git a/common/gdm-common.h b/common/gdm-common.h
116+index 8d83a12..6f88f2f 100644
117+--- a/common/gdm-common.h
118++++ b/common/gdm-common.h
119+@@ -71,6 +71,8 @@ char * gdm_shell_expand (const char *str,
120+ GdmExpandVarFunc expand_func,
121+ gpointer user_data);
122+
123++gboolean is_nvidia_kms_available (void);
124++
125+ G_END_DECLS
126+
127+ #endif /* _GDM_COMMON_H */
128+diff --git a/daemon/gdm-display.c b/daemon/gdm-display.c
129+index 0057e2c..42648f9 100644
130+--- a/daemon/gdm-display.c
131++++ b/daemon/gdm-display.c
132+@@ -54,6 +54,8 @@
133+
134+ #define GDM_DISPLAY_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), GDM_TYPE_DISPLAY, GdmDisplayPrivate))
135+
136++extern int nvidia_has_kms;
137++
138+ struct GdmDisplayPrivate
139+ {
140+ char *id;
141+@@ -118,6 +120,7 @@ enum {
142+ PROP_DOING_INITIAL_SETUP,
143+ };
144+
145++
146+ static void gdm_display_class_init (GdmDisplayClass *klass);
147+ static void gdm_display_init (GdmDisplay *self);
148+ static void gdm_display_finalize (GObject *object);
149+@@ -1447,6 +1450,13 @@ can_create_environment (const char *session_id)
150+ char *path;
151+ gboolean session_exists;
152+
153++#ifdef ENABLE_WAYLAND_SUPPORT
154++ /* Disable X11 sessions if KMS is enabled in the nvidia driver
155++ * See LP: #1697882.
156++ */
157++ if (is_nvidia_kms_available ())
158++ return FALSE;
159++#endif
160+ path = g_strdup_printf (GNOME_SESSION_SESSIONS_PATH "/%s.session", session_id);
161+ session_exists = g_file_test (path, G_FILE_TEST_EXISTS);
162+
163+diff --git a/daemon/gdm-local-display-factory.c b/daemon/gdm-local-display-factory.c
164+index 7a4643d..50ddaea 100644
165+--- a/daemon/gdm-local-display-factory.c
166++++ b/daemon/gdm-local-display-factory.c
167+@@ -48,6 +48,8 @@
168+
169+ #define MAX_DISPLAY_FAILURES 5
170+
171++extern int nvidia_has_kms;
172++
173+ struct GdmLocalDisplayFactoryPrivate
174+ {
175+ GdmDBusLocalDisplayFactory *skeleton;
176+@@ -459,6 +461,9 @@ gdm_local_display_factory_sync_seats (GdmLocalDisplayFactory *factory)
177+ is_initial = FALSE;
178+ }
179+
180++ if (is_nvidia_kms_available ())
181++ session_type = "wayland";
182++
183+ create_display (factory, seat, session_type, is_initial);
184+ }
185+
186+diff --git a/daemon/gdm-session.c b/daemon/gdm-session.c
187+index 0b83223..5e597c4 100644
188+--- a/daemon/gdm-session.c
189++++ b/daemon/gdm-session.c
190+@@ -62,6 +62,8 @@
191+
192+ #define GDM_WORKER_DBUS_PATH "/org/gnome/DisplayManager/Worker"
193+
194++extern int nvidia_has_kms;
195++
196+ typedef struct
197+ {
198+ GdmSession *session;
199+@@ -354,7 +356,9 @@ get_system_session_dirs (GdmSession *self)
200+
201+ search_array = g_array_new (TRUE, TRUE, sizeof (char *));
202+
203+- g_array_append_vals (search_array, x_search_dirs, G_N_ELEMENTS (x_search_dirs));
204++ if (!is_nvidia_kms_available ()) {
205++ g_array_append_vals (search_array, x_search_dirs, G_N_ELEMENTS (x_search_dirs));
206++ }
207+
208+ #ifdef ENABLE_WAYLAND_SUPPORT
209+ if (!self->priv->ignore_wayland) {
210+diff --git a/libgdm/Makefile.am b/libgdm/Makefile.am
211+index 99ada9a..66af6b5 100644
212+--- a/libgdm/Makefile.am
213++++ b/libgdm/Makefile.am
214+@@ -36,6 +36,8 @@ AM_CPPFLAGS = \
215+ -I$(builddir) \
216+ -I$(top_srcdir) \
217+ -I$(top_builddir) \
218++ -I$(top_srcdir)/common \
219++ -I$(top_builddir)/common \
220+ -DG_LOG_DOMAIN=\"Gdm\" \
221+ -DDMCONFDIR=\""$(dmconfdir)"\" \
222+ -DDATADIR=\""$(datadir)"\" \
223+@@ -64,6 +66,7 @@ libgdm_la_LDFLAGS = \
224+ $(END_OF_LIST)
225+
226+ libgdm_la_LIBADD = \
227++ $(top_builddir)/common/libgdmcommon.la \
228+ $(LIBGDM_LIBS) \
229+ $(SYSTEMD_LIBS) \
230+ $(END_OF_LIST)
231+diff --git a/libgdm/gdm-sessions.c b/libgdm/gdm-sessions.c
232+index a645224..419f055 100644
233+--- a/libgdm/gdm-sessions.c
234++++ b/libgdm/gdm-sessions.c
235+@@ -37,6 +37,8 @@
236+
237+ #include "gdm-sessions.h"
238+
239++extern int nvidia_has_kms;
240++
241+ typedef struct _GdmSessionFile {
242+ char *id;
243+ char *path;
244+@@ -208,8 +210,10 @@ collect_sessions (void)
245+ g_free, g_free);
246+ }
247+
248+- for (i = 0; xorg_search_dirs [i] != NULL; i++) {
249+- collect_sessions_from_directory (xorg_search_dirs [i]);
250++ if (!is_nvidia_kms_available ()) {
251++ for (i = 0; xorg_search_dirs [i] != NULL; i++) {
252++ collect_sessions_from_directory (xorg_search_dirs [i]);
253++ }
254+ }
255+
256+ #ifdef ENABLE_WAYLAND_SUPPORT
257+--
258+2.7.4
259+
260
261=== modified file 'debian/patches/series'
262--- debian/patches/series 2017-08-21 08:48:14 +0000
263+++ debian/patches/series 2017-08-24 09:08:19 +0000
264@@ -5,6 +5,7 @@
265 92_systemd_unit.patch
266 93_translate-default-desktop.patch
267 94_retain_xorg_log.patch
268+95_hide_x11_sessions_with_nvidia_kms.patch
269 ubuntu_run_xsession.d.patch
270 ubuntu_xresources_is_a_dir.patch
271 ubuntu_nvidia_prime.patch

Subscribers

People subscribed via source and target branches

to all changes: