From a94c162021bbb0d6065b433040cef693d76162cb Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Tue, 6 Mar 2018 20:16:10 +0000 Subject: [PATCH 1/4] login: correct comment in session_device_free() We're not removing the pushed fd "again"; this is the only place logind removes it from PID1. (And stopping the fd doesn't always cause PID1 to remove the fd itself; it depends on the device type). --- src/login/logind-session-device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/login/logind-session-device.c b/src/login/logind-session-device.c index 73eee72515..de7d963c6a 100644 --- a/src/login/logind-session-device.c +++ b/src/login/logind-session-device.c @@ -419,7 +419,7 @@ void session_device_free(SessionDevice *sd) { if (sd->pushed_fd) { const char *m; - /* Remove the pushed fd again, just in case. */ + /* Make sure to remove the pushed fd. */ m = strjoina("FDSTOREREMOVE=1\n" "FDNAME=session-", sd->session->id); From 8b983cc74a85bda4d662fd822b433327fc568d40 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Tue, 6 Mar 2018 16:16:00 +0000 Subject: [PATCH 2/4] login: we only allow opening character devices We already don't allow directly opening block devices attached to the seat. They are handled by udisks instead. Clarify the code used when restarting logind. --- src/login/logind.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/login/logind.c b/src/login/logind.c index 5220861c1d..373e43e1f7 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -453,8 +453,8 @@ static int manager_attach_fds(Manager *m) { continue; } - if (!S_ISCHR(st.st_mode) && !S_ISBLK(st.st_mode)) { - log_debug("Device fd doesn't actually point to device node: %m"); + if (!S_ISCHR(st.st_mode)) { + log_debug("Device fd doesn't point to a character device node"); close_nointr(fd); continue; } From 1bef256cf5838d4bbc55654206aa6254d7fddb59 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Tue, 6 Mar 2018 15:59:38 +0000 Subject: [PATCH 3/4] login: don't remove all devices from PID1 when only one was removed FDSTOREREMOVE=1 removes all fds with the specified name. And we had named the fds after the session. Better fix that. Closes #8344. AFAICT there's no point providing compatibility code for this transition. No-one would be restarting logind on a system with a GUI (where the session devices are used), because doing so has been killing the GUI, and even causing startup of the GUI to fail leading to a restart loop. Upgrading logind on a running system with a GUI might start being possible after this commit (and after also fixing the display server of your choice). --- src/login/logind-session-device.c | 32 +++++++++++++----- src/login/logind.c | 56 ++++++++++++++++++++++++++----- 2 files changed, 70 insertions(+), 18 deletions(-) diff --git a/src/login/logind-session-device.c b/src/login/logind-session-device.c index de7d963c6a..04063c3764 100644 --- a/src/login/logind-session-device.c +++ b/src/login/logind-session-device.c @@ -416,15 +416,21 @@ error: void session_device_free(SessionDevice *sd) { assert(sd); + /* Make sure to remove the pushed fd. */ if (sd->pushed_fd) { - const char *m; + _cleanup_free_ char *m = NULL; + const char *id; + int r; - /* Make sure to remove the pushed fd. */ + /* Session ID does not contain separators. */ + id = sd->session->id; + assert(*(id + strcspn(id, "-\n")) == '\0'); - m = strjoina("FDSTOREREMOVE=1\n" - "FDNAME=session-", sd->session->id); - - (void) sd_notify(false, m); + r = asprintf(&m, "FDSTOREREMOVE=1\n" + "FDNAME=session-%s-device-%u-%u\n", + id, major(sd->dev), minor(sd->dev)); + if (r >= 0) + (void) sd_notify(false, m); } session_device_stop(sd); @@ -510,7 +516,8 @@ unsigned int session_device_try_pause_all(Session *s) { } int session_device_save(SessionDevice *sd) { - const char *m; + _cleanup_free_ char *m = NULL; + const char *id; int r; assert(sd); @@ -524,9 +531,16 @@ int session_device_save(SessionDevice *sd) { if (sd->pushed_fd) return 0; + + /* Session ID does not contain separators. */ + id = sd->session->id; + assert(*(id + strcspn(id, "-\n")) == '\0'); - m = strjoina("FDSTORE=1\n" - "FDNAME=session-", sd->session->id); + r = asprintf(&m, "FDSTORE=1\n" + "FDNAME=session-%s-device-%u-%u\n", + id, major(sd->dev), minor(sd->dev)); + if (r < 0) + return r; r = sd_pid_notify_with_fds(0, false, m, &sd->fd, 1); if (r < 0) diff --git a/src/login/logind.c b/src/login/logind.c index 373e43e1f7..95b29ae85e 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -37,6 +37,7 @@ #include "format-util.h" #include "fs-util.h" #include "logind.h" +#include "parse-util.h" #include "process-util.h" #include "selinux-util.h" #include "signal-util.h" @@ -411,6 +412,38 @@ static int manager_enumerate_users(Manager *m) { return r; } +static int parse_fdname(const char *fdname, char **session_id, dev_t *dev) { + _cleanup_strv_free_ char **parts = NULL; + _cleanup_free_ char *id = NULL; + unsigned int major, minor; + int r; + + parts = strv_split(fdname, "-"); + if (!parts) + return -ENOMEM; + if (strv_length(parts) != 5) + return -EINVAL; + + if (!streq(parts[0], "session")) + return -EINVAL; + id = strdup(parts[1]); + if (!id) + return -ENOMEM; + + if (!streq(parts[2], "device")) + return -EINVAL; + r = safe_atou(parts[3], &major) || + safe_atou(parts[4], &minor); + if (r < 0) + return r; + + *dev = makedev(major, minor); + *session_id = id; + id = NULL; + + return 0; +} + static int manager_attach_fds(Manager *m) { _cleanup_strv_free_ char **fdnames = NULL; int n, i, fd; @@ -424,16 +457,21 @@ static int manager_attach_fds(Manager *m) { return n; for (i = 0; i < n; i++) { + _cleanup_free_ char *id = NULL; + dev_t dev; struct stat st; SessionDevice *sd; Session *s; - char *id; + int r; fd = SD_LISTEN_FDS_START + i; - id = startswith(fdnames[i], "session-"); - if (!id) + r = parse_fdname(fdnames[i], &id, &dev); + if (r < 0) { + log_debug_errno(r, "Failed to parse fd name %s: %m", fdnames[i]); + close_nointr(fd); continue; + } s = hashmap_get(m->sessions, id); if (!s) { @@ -453,24 +491,24 @@ static int manager_attach_fds(Manager *m) { continue; } - if (!S_ISCHR(st.st_mode)) { - log_debug("Device fd doesn't point to a character device node"); + if (!S_ISCHR(st.st_mode) || st.st_rdev != dev) { + log_debug("Device fd doesn't point to the expected character device node"); close_nointr(fd); continue; } - sd = hashmap_get(s->devices, &st.st_rdev); + sd = hashmap_get(s->devices, &dev); if (!sd) { /* Weird, we got an fd for a session device which wasn't - * recorded in the session state file... */ + * recorded in the session state file... */ log_warning("Got fd for missing session device [%u:%u] in session %s", - major(st.st_rdev), minor(st.st_rdev), s->id); + major(dev), minor(dev), s->id); close_nointr(fd); continue; } log_debug("Attaching fd to session device [%u:%u] for session %s", - major(st.st_rdev), minor(st.st_rdev), s->id); + major(dev), minor(dev), s->id); session_device_attach_fd(sd, fd, s->was_active); } From f27053376074fc6d325e01e699e0125f5d03192a Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Tue, 6 Mar 2018 12:28:54 +0000 Subject: [PATCH 4/4] login: effectively revert "open device if needed" This replaces commit 4d3900f1b7ccce03366f9a57d259d0735c1cfbcf. The underlying cause of issue #8291 has been fixed, so there is no reason to paper over it any more. But it might still be useful not to crash in the face of bad restart data. That can cause several restarts, or maybe at some point an infinite loop of restarts. Fail the start (or stop!) request, and write an error to the system log. Each time reflects a user request where we fail to resume the display server's access (or revoke it), and it can be useful if the log shows the most recent one. --- src/login/logind-session-device.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/login/logind-session-device.c b/src/login/logind-session-device.c index 04063c3764..9f01497251 100644 --- a/src/login/logind-session-device.c +++ b/src/login/logind-session-device.c @@ -193,19 +193,16 @@ static int session_device_start(SessionDevice *sd) { switch (sd->type) { case DEVICE_TYPE_DRM: - if (sd->fd < 0) { - /* Open device if it isn't open yet */ - sd->fd = session_device_open(sd, true); - if (sd->fd < 0) - return sd->fd; - } else { - /* Device is kept open. Simply call drmSetMaster() and hope there is no-one else. In case it fails, we - * keep the device paused. Maybe at some point we have a drmStealMaster(). */ - r = sd_drmsetmaster(sd->fd); - if (r < 0) - return r; + log_error("Failed to re-activate DRM fd, as the fd was lost (maybe logind restart went wrong?)"); + return -EBADF; } + + /* Device is kept open. Simply call drmSetMaster() and hope there is no-one else. In case it fails, we + * keep the device paused. Maybe at some point we have a drmStealMaster(). */ + r = sd_drmsetmaster(sd->fd); + if (r < 0) + return r; break; case DEVICE_TYPE_EVDEV: @@ -239,6 +236,11 @@ static void session_device_stop(SessionDevice *sd) { switch (sd->type) { case DEVICE_TYPE_DRM: + if (sd->fd < 0) { + log_error("Failed to de-activate DRM fd, as the fd was lost (maybe logind restart went wrong?)"); + return; + } + /* On DRM devices we simply drop DRM-Master but keep it open. * This allows the user to keep resources allocated. The * CAP_SYS_ADMIN restriction to DRM-Master prevents users from